Packagist vulnerabilities are not being reported for some packages

[G-Rath]

I've recently done an initial implementation for having osv-detector use the osv.dev api, but it looks like it's not 1:1 with the offline databases, at least for Packagist.

Using this lockfile:

{
    "_readme": [
        "This file locks the dependencies of your project to a known state",
        "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies",
        "This file is @generated automatically"
    ],
    "content-hash": "b63765525e5fabcf664728d548ecf8a2",
    "packages": [
        {
            "name": "enshrined/svg-sanitize",
            "version": "0.13.3",
            "source": {
                "type": "git",
                "url": "https://github.com/darylldoyle/svg-sanitizer.git",
                "reference": "bc66593f255b7d2613d8f22041180036979b6403"
            },
            "dist": {
                "type": "zip",
                "url": "https://api.github.com/repos/darylldoyle/svg-sanitizer/zipball/bc66593f255b7d2613d8f22041180036979b6403",
                "reference": "bc66593f255b7d2613d8f22041180036979b6403",
                "shasum": ""
            },
            "require": {
                "ext-dom": "*",
                "ext-libxml": "*"
            },
            "require-dev": {
                "codeclimate/php-test-reporter": "^0.1.2",
                "phpunit/phpunit": "^6"
            },
            "type": "library",
            "autoload": {
                "psr-4": {
                    "enshrined\\svgSanitize\\": "src"
                }
            },
            "notification-url": "https://packagist.org/downloads/",
            "license": [
                "GPL-2.0-or-later"
            ],
            "authors": [
                {
                    "name": "Daryll Doyle",
                    "email": "daryll@enshrined.co.uk"
                }
            ],
            "description": "An SVG sanitizer for PHP",
            "time": "2020-01-20T01:34:17+00:00"
        }
    ],
    "packages-dev": [],
    "aliases": [],
    "minimum-stability": "stable",
    "stability-flags": [],
    "prefer-stable": false,
    "prefer-lowest": false,
    "platform": [],
    "platform-dev": []
}

❯ osv-detector-t --use-api --parse-as composer.lock /mnt/c/Users/Gareth/Downloads/safe-svg-composer.lock.txt
/mnt/c/Users/Gareth/Downloads/safe-svg-composer.lock.txt: found 1 package
  no known vulnerabilities found

❯ osv-detector-t --use-dbs --parse-as composer.lock /mnt/c/Users/Gareth/Downloads/safe-svg-composer.lock.txt
/mnt/c/Users/Gareth/Downloads/safe-svg-composer.lock.txt: found 1 package
  Loading OSV databases for the following ecosystems:
    Packagist (862 vulnerabilities, including withdrawn - last updated Fri, 13 May 2022 23:58:47 GMT)

  enshrined/svg-sanitize@0.13.3 is affected by the following vulnerabilities:
    GHSA-fqx8-v33p-4qcc: Cross-site Scripting in enshrined/svg-sanitize (https://github.com/advisories/GHSA-fqx8-v33p-4qcc)

  1 known vulnerability found in /mnt/c/Users/Gareth/Downloads/safe-svg-composer.lock.txt

The vulnerability here correctly lists says it affects versions below 0.15.0, but it's not reported even if I use the version:

❯ curl -X POST -d '{"commit": "bc66593f255b7d2613d8f22041180036979b6403"}' 'https://api.osv.dev/v1/query'
{}
❯ curl -X POST -d '{"package": {"name": "enshrined/svg-sanitize"}, "version": "0.13.3"}' 'https://api.osv.dev/v1/query'
{}
❯ curl -X POST -d '{"package": {"name": "enshrined/svg-sanitize", "ecosystem": "Packagist"}, "version": "0.13.3"}' 'https://api.osv.dev/v1/query'
{}

Going with the lowest version for this package doesn't return anything either, when it should return three vulnerabilities.

(my current theory is that this because the advisory doesn't have any versions, and the api isn't checking against ranges?)

[oliverchang]

Yep, we are currently lacking Packagist versioning support: #230. This will need a little bit of work, as it seems to have some strange quirks.

[G-Rath]

Is this the only ecosystem you think is affected by this? (So far everything else seems to be matching between the databases and api, but I've not yet tried extensively across all the ecosystems).

fwiw I'm pretty confident that semantic should be 80% of what you need in terms of implementation, so hopefully you could just port that over? (the only thing it doesn't do right now is the "dev < alpha < beta < etc" list - it does handle arbitrary + different number of components and views rc.1 as equal to rc1).

(at some point it could be good to pull across their tests to ensure it matches: https://github.com/composer/semver/blob/a951f614bd64dcd26137bc9b7b2637ddcfc57649/tests/ComparatorTest.php)

[oliverchang]

Given the many different versioning semantics out there, I'm not sure if it makes sense to have a single versioning package which tries to handle every single scheme out there. We'll likely need to build specific support for every ecosystem we want to support for the most robust/precise results.

e.g. Maven has similarly complicated semantics (https://maven.apache.org/pom.html#Dependency_Version_Requirement_Specification) which would conflict with the PHP ones and a different set of special cases.

[oliverchang]

Is this the only ecosystem you think is affected by this?

Out of all the main ecosystems currently supported, Packagist is the only one lacking support.

[G-Rath]

Given the many different versioning semantics out there, I'm not sure if it makes sense to have a single versioning package which tries to handle every single scheme out there.

I think if you're trying to support actually handling ranges then maybe but fwiw I also have support for parsing Maven ranges into a minimal version number: https://github.com/G-Rath/osv-detector/blob/main/internal/lockfile/parse-maven-lock_test.go#L107

[oliverchang]

OSV will need this for handling ranges. Our infra takes care of "expanding" the non-semver ranges into the "versions" arrays to make these advisories easier to match without having to implement all the different version ordering schemes out there.

[G-Rath]

but isn't that just for the versions property right? why not fallback to the actual affected ranges in the event versions is empty? (which is why osv-detector is able to match the vulnerabilities when using the database, without having the versions array)

[oliverchang]

Falling back is fine if the local tool can implement it correctly (checking the ranges is part of the evaluation algorithm). OSV infra just guarantees that it will always expand and populate the "versions" field for convenience.

[G-Rath]

Maybe I'm missing something, but doesn't that then loop us back to the start of this issue / this comment? while inconvenient you don't yet have a stable way of populating "versions" for Packagist vulnerabilities, you should still be able to ensure correct support by checking the ranges which should be safer to support because you shouldn't have to deal with package-manager-specific constraints/ranges (or, you should be able to provide support for static versions right now, and hopefully be able to support constraints in future)

[oliverchang]

I think there might have been some miscommunication somewhere :) Let me try to unpack our conversation and correct me if I've misunderstood something.

For OSV: OSV.dev infra needs Packagist support for expanding affected ranges into static versions lists. This requires proper implementation of Packagist's versioning/ordering scheme. Checking ranges to match vulnerabilities also requires this to be implemented.

For osv-detector: there is a generic semantic package that you implemented which tries to support every single version scheme and their comparison rules out there. My claim is we will need to have more specific support (i.e. one package ver versioning scheme) rather than a generic library which tries to do everything because there will inevitably be conflicts.

Maybe I'm missing something, but doesn't that then loop us back to the start of this issue / #426 (comment)?

Yes, the core problem here is there is no correct implementation of Packagist version ordering support in either OSV.dev infra or your osv-detector.

you should still be able to ensure correct support by checking the ranges which should be safer to support because you shouldn't have to deal with package-manager-specific constraints/ranges

I'm not sure I fully understand this. To check ranges, we also need to implement correct version ordering support. And if we implement that, in our implementation we can just expand and index on the static versions instead. Could you please clarify?

Happy to have a call some time to talk through this :)

[G-Rath]

I think maybe there's been some confusion on the "ranges" (on my side) - right now I'm focusing on just a single version e.g. v1.0.0.

I believe currently that it should be possible to implement a best-effort package for handling most common representations of single specific versions across a number of ecosystems that is "good enough" for initial support (e.g. it'll handle 80%+ of common cases, which is far better than supporting 0%), and in turn this means you should be able to support checking "OSV ranges" that are also single-version¹ - I also believe I've proven this with osv-detector.

I think at this point I do somewhat disagree that you need to explicitly have a package per versioning schema, for similar reasons - I'm definitely not saying that it's perfect but I am also wondering if you're aiming for perfect so missing hitting "good"?

This is also completely still focused on that single static version - I completely agree that if you add constraints into the mix, things get a lot more complex (e.g. Maven which you've linked to, Pip supporting !=, etc) but personally I think the best place to start for handling that is with the native package managers as they're the ones who'll know best what the final version(s) will be - which brings me back to "perfect is the enemy of good": if you can get decent support for static versions like what I've got with semantic, that'll allow people to start by providing the exact versions given to them by their package managers (this is me starting to give my 2 cents now - happy to do a call to discuss further)

I think it'd probably be really useful if you had any examples of where semantic doesn't work (excluding constraints of course 😉)

¹: so far all of the OSVs I've seen have had this, though the specification for ECOSYSTEM does not say that these ranges have to point to a single package meaning it might be valid to have an OSV that specifies e.g. a Maven constraint that actually matches multiple versions, which might be what you were getting at earlier

[oliverchang]

so far all of the OSVs I've seen have had this, though the specification for ECOSYSTEM does not say that these ranges have to point to a single package meaning it might be valid to have an OSV that specifies e.g. a Maven constraint that actually matches multiple versions, which might be what you were getting at earlier

Versions in OSV ranges should absolutely refer to single versions. Sorry for the confusion here -- I think our terminology of native package manager constraints vs OSV ranges are mixed up. We don't want native package manger constraints to get encoded at all in any advisories.

Resolving versions/constraints from a manifest file is indeed a whole separate problem to tackle.

I think at this point I do somewhat disagree that you need to explicitly have a package per versioning schema, for similar reasons - I'm definitely not saying that it's perfect but I am also wondering if you're aiming for perfect so missing hitting "good"?

One key motivation behind OSV is to make sure we have precision and accuracy over existing standards (no false positives or negatives), and we already do have proper support for every major versioning scheme -- only Composer/Packagist is missing. I believe the effort for OSV.dev would be much better spent to just implement that vs porting or trying to support a generic version scheme.

[G-Rath]

Right I think we're on the same page now (I think the bulk was that terminology mixup 😅) - I didn't realise that you'd already got full spec implementations for all the other standards; in that case I completely agree you might as well try and land a complete spec (the only reason semantic is "generic" is because it turned out that actually works for most of the specs).

I still think semantic is pretty close to being 1:1 with composer - the only thing I know is missing is that list order. If you like I can have a look at getting that implemented, and then hopefully it should just be a matter of porting that over to the OSV.dev infrastructure codebase (which I might be able to help with too).

we already do have proper support for every major versioning scheme

Is that public somewhere? if so could you link me to it?

[oliverchang]

Is that public somewhere? if so could you link me to it?

It's at https://github.com/google/osv/blob/master/lib/osv/ecosystems.py

[oliverchang]

Closing this one as the required work is being tracked in #230.