Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pull SEMVER range for npm vuln #1206

Open
celek opened this issue Jan 12, 2024 · 18 comments
Open

Pull SEMVER range for npm vuln #1206

celek opened this issue Jan 12, 2024 · 18 comments

Comments

@celek
Copy link

celek commented Jan 12, 2024

select updater,name,severity,repo_name,fixed_in_version,vulnerable_range,aliases from vuln
where name = 'GHSA-67hx-6x53-jw92' 

returns no range but the osv data has some - https://api.osv.dev/v1/vulns/GHSA-67hx-6x53-jw92

 "ranges":
          [
            {
              "type": "SEMVER",
              "events": [{ "introduced": "0" }, { "fixed": "7.23.2" }],
            },

and

 "ranges":
          [
            {
              "type": "SEMVER",
              "events":
                [
                  { "introduced": "8.0.0-alpha.0" },
                  { "fixed": "8.0.0-alpha.4" },
                ],
            },
          ],
@celek
Copy link
Author

celek commented Jan 16, 2024

@hdonnay @crozzy - I am looking into providing a PR for it -
Should I look here ? https://github.com/quay/claircore/blob/main/rhel/rhcc/updater.go or https://github.com/quay/claircore/blob/main/rhel/rhcc/rhcc.go
Should we update the repo_name or can I update the vulnerable_range ?
I would love to implement that for our local version tomorrow if you can give me pointer :)

@hdonnay
Copy link
Member

hdonnay commented Jan 16, 2024

No; if you want to look at OSV, that's over in the osv package.

@celek
Copy link
Author

celek commented Jan 17, 2024

@hdonnay looking at the code - I am not sure why it does not take the Introduced from the second range...
looking at the code, this

"ranges":[{"type": "SEMVER", "events": [{ "introduced": "0" }, { "fixed": "7.23.2" }],},

should give me only the fixed in version, as the introduced is 0
but this

 "ranges":[{"type": "SEMVER","events":[{ "introduced": "8.0.0-alpha.0" },{ "fixed": "8.0.0-alpha.4" },],},],

should have created an introduced unless we believe the issue is here

semver.NewVersion(ev.Introduced)

and this throws an error ?
if you think the issue is in this code - do you know how I can run that with the string ?

@celek
Copy link
Author

celek commented Jan 17, 2024

I tested and the NewVersion works
@hdonnay @crozzy do you think the issue then is

vulnerable_range column I believe only accepts integers. So in this case, we would need to ensure the fixed_in_version column instead inputs that version like this

and thus I need to update the fixedin version to have something like

fixed=4.1.2&introduced=4.0.0

?

@hdonnay
Copy link
Member

hdonnay commented Jan 17, 2024

Yes, if the version contains non-numeric components it can't get normalized into claircore's range type. It should still be encoded in the "FixedInVersion".

@celek
Copy link
Author

celek commented Jan 17, 2024

@hdonnay thanks - trying to figure out why

SELECT name, package_name, fixed_in_version, vulnerable_range FROM public.vuln
WHERE name = 'GHSA-q4q5-c5cv-2p68' AND fixed_in_version LIKE '%fixed=%'
ORDER BY id ASC LIMIT 1 

gets us fixed=2.6.10&introduced=2.0.0-beta.4
so the issue is : why isn't this one encoded - and I believe SEMVER are encoded because CLAIR believes it will have a range ?

@hdonnay
Copy link
Member

hdonnay commented Jan 17, 2024

I'm not sure what you're saying, here. I would expect the information persisted into the database to not have a vulnerable_range column populated because one of the versions has a non-numeric component.

@celek
Copy link
Author

celek commented Jan 17, 2024

Got it - analysis

SELECT name,package_name,repo_name,fixed_in_version,vulnerable_range,version_kind FROM public.vuln
WHERE name = 'GHSA-q4q5-c5cv-2p68'
name package_name repo_name fixed_in_version vulnerable_range version_kind
"GHSA-q4q5-c5cv-2p68" "pkg:npm/vuetify" "npm" "2.6.10" "[""{0,2,0,0,0,0,0,0,0,0}"",""{0,2,6,10,0,0,0,0,0,0}"")" "semver"
"GHSA-q4q5-c5cv-2p68" "org.webjars.npm:vuetify" "npm" "fixed=2.6.10&introduced=2.0.0-beta.4" "empty"

https://api.osv.dev/v1/vulns/GHSA-q4q5-c5cv-2p68

"affected": [
    {
      "package": {
        "name": "vuetify",
        "ecosystem": "npm",
        "purl": "pkg:npm/vuetify"
      },
      "ranges": [
        {
          "type": "SEMVER",
          "events": [{ "introduced": "2.0.0-beta.4" }, { "fixed": "2.6.10" }]
        }
      ],
      "database_specific": {
        "source": "https://github.com/github/advisory-database/blob/main/advisories/github-reviewed/2022/09/GHSA-q4q5-c5cv-2p68/GHSA-q4q5-c5cv-2p68.json"
      }
    },
    {
      "package": {
        "name": "org.webjars.npm:vuetify",
        "ecosystem": "Maven",
        "purl": "pkg:maven/org.webjars.npm/vuetify"
      },
      "ranges": [
        {
          "type": "ECOSYSTEM",
          "events": [{ "introduced": "2.0.0-beta.4" }, { "fixed": "2.6.10" }]
        }
      ],

@celek
Copy link
Author

celek commented Jan 17, 2024

SELECT name,package_name,repo_name,fixed_in_version,vulnerable_range,version_kind FROM public.vuln
WHERE name = 'GHSA-67hx-6x53-jw92'

name package_name repo_name fixed_in_version vulnerable_range version_kind
"GHSA-67hx-6x53-jw92" "pkg:npm/%40babel/traverse" "npm" "7.23.2" "empty"
"GHSA-67hx-6x53-jw92" "pkg:npm/%40babel/traverse" "npm" "8.0.0-alpha.4" "empty" "semver"

https://api.osv.dev/v1/vulns/GHSA-67hx-6x53-jw92

"affected":
    [
      {
        "package":
          {
            "name": "@babel/traverse",
            "ecosystem": "npm",
            "purl": "pkg:npm/%40babel/traverse",
          },
        "ranges":
          [
            {
              "type": "SEMVER",
              "events": [{ "introduced": "0" }, { "fixed": "7.23.2" }],
            },
          ],
        "database_specific":
          {
            "source": "https://github.com/github/advisory-database/blob/main/advisories/github-reviewed/2023/10/GHSA-67hx-6x53-jw92/GHSA-67hx-6x53-jw92.json",
          },
      },
      {
        "package":
          {
            "name": "@babel/traverse",
            "ecosystem": "npm",
            "purl": "pkg:npm/%40babel/traverse",
          },
        "ranges":
          [
            {
              "type": "SEMVER",
              "events":
                [
                  { "introduced": "8.0.0-alpha.0" },
                  { "fixed": "8.0.0-alpha.4" },
                ],
            },
          ],

Let me see what is different

@celek
Copy link
Author

celek commented Jan 17, 2024

interesting - maybe the issue is is that the introduced == fixedin in case I remove -alpha - I need to check

@hdonnay
Copy link
Member

hdonnay commented Jan 17, 2024

ah, you're comparing across an ECOSYSTEM range and a SEMVER range. In the first example, it looks like there might be a bug where the maven package reports that it's an npm package

@celek
Copy link
Author

celek commented Jan 17, 2024

Ok so what should this one become ?

"type": "SEMVER",
              "events":
                [
                  { "introduced": "8.0.0-alpha.0" },
                  { "fixed": "8.0.0-alpha.4" },
                ],

should it be

name package_name repo_name fixed_in_version vulnerable_range version_kind
"GHSA-67hx-6x53-jw92" "pkg:npm/%40babel/traverse" "npm" "fixed=8.0.0-alpha.4&introduced=8.0.0-alpha.0" "empty" "semver"

or

name package_name repo_name fixed_in_version vulnerable_range version_kind
"GHSA-67hx-6x53-jw92" "pkg:npm/%40babel/traverse" "npm" "8.0.0-alpha.4" "[""{0,8,0,0,0,0,0,0,0,0}"",""{0,8,0,0,0,0,0,0,0,0}"")" "semver"

I believe the first one right ? because the introduced AND fixed in version do contain invalid SEMVER - should we move it to this code ?

case ecosystemMaven, ecosystemPyPI, ecosystemRubyGems:
						switch {
						case ev.Introduced == "0":
						case ev.Introduced != "":
							ranges.Add("introduced", ev.Introduced)
						case ev.Fixed != "":
							ranges.Add("fixed", ev.Fixed)
						case ev.LastAffected != "":
							ranges.Add("lastAffected", ev.LastAffected)
						}

even though it is a SEMVER and the ecosystem is npm ?

Notice that this one

"events": [{ "introduced": "2.0.0-beta.4" }, { "fixed": "2.6.10" }]

became this

"[""{0,2,0,0,0,0,0,0,0,0}"",""{0,2,6,10,0,0,0,0,0,0}"")"

so it dropped the beta.4 from 2.0.0

@celek
Copy link
Author

celek commented Jan 17, 2024

we might hit this if both upper and lower are the same in this unique case

func rangefmt(r *claircore.Range) (kind *string, lower, upper string) {
	lower, upper = "{}", "{}"
	if r == nil || r.Lower.Kind != r.Upper.Kind {
		return kind, lower, upper
	}

@celek
Copy link
Author

celek commented Jan 17, 2024

I am going to implement as follow
Yes, if the version contains non-numeric components it can't get normalized into claircore's range type. It should still be encoded in the "FixedInVersion".
for SEMVER ,

  • I will check introduced, fixed and lastAffected
  • if one of them contains non-numeric components, I will generate a FixedInversion string instead
  • I will check Version metadata and Prerelease for each semver.NewVersion(ev.LastAffected), semver.NewVersion(ev.Fixed) and semver.NewVersion(ev.Introduced)
  • if they contains non-numeric components I will set a boolean
  • In all cases I will add the value to ranges ranges.Add("introduced", ev.Introduced)
  • if I get a boolean then I will change fixedInVersion to be the string

@celek
Copy link
Author

celek commented Jan 17, 2024

something like

case ev.Introduced != "":
    ver, err = semver.NewVersion(ev.Introduced)
    if (ver.Metadata() != "") || (ver. Prerelease() != "") {
        encodeFixedInVersion = true
    }
    if err == nil {
        v.Range.Lower = FromSemver(ver)
    }
    ranges.Add("introduced", ev.Introduced)

and

if len(ranges) > 0 {
    if (encodeFixedInVersion){
        v.FixedInVersion = ranges.Encode()
    }
    switch af.Package.Ecosystem {
        case ecosystemMaven, ecosystemPyPI, ecosystemRubyGems:
            v.FixedInVersion = ranges.Encode()
        }
    }
}

@hdonnay ?

@celek
Copy link
Author

celek commented Jan 18, 2024

Seeing this now -
image

@celek
Copy link
Author

celek commented Jan 18, 2024

Investigate why the old entry is still there - the MD5 calculation may have failed

@celek
Copy link
Author

celek commented Jan 19, 2024

@hdonnay is there a way I can force the osv/updater to update all record regardless if the hash is the same or not ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants