Multiple repositories should be able to provide advisories for the same package

[mxr576]

I have built a custom repository implementation that also provides advisories for packages. I turned out that due to the current implementation in \Composer\Repository\RepositorySet::getSecurityAdvisoriesForConstraints(), only one provider can provide advisories for a package.

Steps to reproduce

git clone mxr576/ddqg-composer-audit
composer -d tests/fixtures/e2e install
composer -d tests/fixtures/e2e audit

Actual behavior

./vendor/bin/composer -d tests/fixtures/e2e audit
Found 9 security vulnerability advisories affecting 7 packages:
+-------------------+----------------------------------------------------------------------------------+
| Package           | drupal/apigee_edge                                                               |
| CVE               | DDQG-insecure-drupal-apigee_edge                                                 |
| Title             | The installed version is insecure. (Reported by Drupal Dependency Quality Gate.) |
| URL               | https://www.drupal.org/project/apigee_edge                                       |
| Affected versions | >=1.0.0,<1.27.0|>=1.0.0,<1.27.0|>=2.0.0,<2.0.8                                   |
| Reported at       | 2023-04-20T13:30:28+00:00                                                        |
+-------------------+----------------------------------------------------------------------------------+
+-------------------+----------------------------------------------------------------------------------+
| Package           | drupal/core                                                                      |
| CVE               | CVE-2022-25277                                                                   |
| Title             | Drupal core - Critical - Arbitrary PHP code execution - SA-CORE-2022-014         |
| URL               | https://www.drupal.org/sa-core-2022-014                                          |
| Affected versions | >=8.9.0,<8.10.0|>=9.0.0,<9.1.0|>=9.1.0,<9.2.0|>=9.2.0,<9.3.0|>=9.3.0,<9.3.19|>=9 |
|                   | .4.0,<9.4.3                                                                      |
| Reported at       | 2022-07-20T18:00:00+00:00                                                        |
+-------------------+----------------------------------------------------------------------------------+
+-------------------+----------------------------------------------------------------------------------+
| Package           | drupal/core                                                                      |
| CVE               | CVE-2022-25275                                                                   |
| Title             | Drupal core - Moderately critical - Information Disclosure - SA-CORE-2022-012    |
| URL               | https://www.drupal.org/sa-core-2022-012                                          |
| Affected versions | >=7.0.0,<7.91.0|>=8.9.0,<8.10.0|>=9.0.0,<9.1.0|>=9.1.0,<9.2.0|>=9.2.0,<9.3.0|>=9 |
|                   | .3.0,<9.3.19|>=9.4.0,<9.4.3                                                      |
| Reported at       | 2022-07-20T18:00:00+00:00                                                        |
+-------------------+----------------------------------------------------------------------------------+

Expected behavior

Notice the number of drupal/core entries.

+-------------------+----------------------------------------------------------------------------------+
| Package           | drupal/apigee_edge                                                               |
| CVE               | DDQG-insecure-drupal-apigee_edge                                                 |
| Title             | The installed version is insecure. (Reported by Drupal Dependency Quality Gate.) |
| URL               | https://www.drupal.org/project/apigee_edge                                       |
| Affected versions | >=1.0.0,<1.27.0|>=1.0.0,<1.27.0|>=2.0.0,<2.0.8                                   |
| Reported at       | 2023-04-20T13:56:33+00:00                                                        |
+-------------------+----------------------------------------------------------------------------------+
+-------------------+----------------------------------------------------------------------------------+
| Package           | drupal/core                                                                      |
| CVE               | DDQG-insecure-drupal-core                                                        |
| Title             | The installed version is insecure. (Reported by Drupal Dependency Quality Gate.) |
| URL               | https://www.drupal.org/project/core                                              |
| Affected versions | >=9.4.0,<9.4.14|>=9.5.0,<9.5.8|>=10.0.0,<10.0.8                                  |
| Reported at       | 2023-04-20T13:56:33+00:00                                                        |
+-------------------+----------------------------------------------------------------------------------+
+-------------------+----------------------------------------------------------------------------------+
| Package           | drupal/core                                                                      |
| CVE               | CVE-2022-25277                                                                   |
| Title             | Drupal core - Critical - Arbitrary PHP code execution - SA-CORE-2022-014         |
| URL               | https://www.drupal.org/sa-core-2022-014                                          |
| Affected versions | >=8.9.0,<8.10.0|>=9.0.0,<9.1.0|>=9.1.0,<9.2.0|>=9.2.0,<9.3.0|>=9.3.0,<9.3.19|>=9 |
|                   | .4.0,<9.4.3                                                                      |
| Reported at       | 2022-07-20T18:00:00+00:00                                                        |
+-------------------+----------------------------------------------------------------------------------+
+-------------------+----------------------------------------------------------------------------------+
| Package           | drupal/core                                                                      |
| CVE               | CVE-2022-25275                                                                   |
| Title             | Drupal core - Moderately critical - Information Disclosure - SA-CORE-2022-012    |
| URL               | https://www.drupal.org/sa-core-2022-012                                          |
| Affected versions | >=7.0.0,<7.91.0|>=8.9.0,<8.10.0|>=9.0.0,<9.1.0|>=9.1.0,<9.2.0|>=9.2.0,<9.3.0|>=9 |
|                   | .3.0,<9.3.19|>=9.4.0,<9.4.3                                                      |
| Reported at       | 2022-07-20T18:00:00+00:00                                                        |
+-------------------+----------------------------------------------------------------------------------+

[mxr576]

@cmlara pointed out on Drupal Slack that he already reported something similar before: #11258

Yeah that's a limitation we'd have to live with IMO for perf reason otherwise you end up querying every repo for everything and the scenario seems pretty far fetched.

@Seldaek wrote this on January. I experience no performance issue so far and I hope the requirement is not "far fetched" anymore :)

[Seldaek]

What's the use case exactly tho? You've built one ok but why?

[mxr576]

The goal is providing more accurate audit results for Drupal projects.

Current challenges in the Drupal space:

• even security releases of Drupal and contributed packages are published on Drupal.org, Drupal core rarely get a CVEs so security releases do not pop up https://github.com/FriendsOfPHP/security-advisories, contrib package security releases never have CVEs, so they are never reported by composer audit. This is what https://github.com/drupal-composer/drupal-security-advisories tries to gap and they also started to play with the idea of extending composer audit.
• Some ppl (including myself) also worried about those Drupal components that are not supported by maintainers anymore or not covered by the Drupal security team. (See: Include major versions of contrib modules which are marked unsupported drupal-composer/drupal-security-advisories#29) AFAIK There were no tool for collecting or reporting these before and of course, these packages and package versions also would never appear on https://github.com/FriendsOfPHP/security-advisories.

I also found this commit before I started to build this tool, and I wonder, if it was not the original goal for allowing to extend the audit command like this, then what was it?
8c9f82d

[stof]

https://github.com/FriendsOfPHP/security-advisories does not require a CVE to submit an advisory

[stof]

IIRC, the reason why Drupal contrib packages were not included in https://github.com/FriendsOfPHP/security-advisories is because for contrib modules, the same version number can exist in the 7.x, 8.x and 9.x package repository but for different code of the contrib package, and so any advisory repository that assumes that name+version is enough to identify a vulnerability would break with those ones (this would also break any metadata repository that would try to aggregated metadata from 2 of those repositories btw)

[mxr576]

@stof I think you are right, but Drupal contrib packages also started to adopt semver. Besides the 7.x-1.0 vs. 9.x-1.0 problem can be addressed by focusing on one specific Drupal core major version compatibility at a time. But most of the newly created packages should use semver and the the "Drupal core compatibility prefixed versioning" should vanish away..

So if you do not mind, I'd stick with the "assumption" that challenges can be solved on the Drupal side, but when they are solved then changes are needed on the Composer side otherwise multiple repositories cannot provide advisories for the same package.

[stof]

For packages that are versioned properly rather than relying on this prefixed versioning, you can submit their advisories to https://github.com/FriendsOfPHP/security-advisories without issue

[mxr576]

@stof Are you indicating that the composer audit command, even after 8c9f82d, should be only accept input from https://github.com/FriendsOfPHP/security-advisories?

Please also describe the second requirement that I try to cover here. Those package versions can never be submitted to https://github.com/FriendsOfPHP/security-advisories, however a custom repository implementation can always mark them insecure.

[mxr576]

however a custom repository implementation can always mark them insecure.

Let me extend that a bit, so with the current implementation, only one repository can flag an issue with a package. However, one repository can flag multiple ones (multiple CVEs). So if the packagist repository flags an issue on a package, others cannot. Which seems weird since one repository can flag multiple issues, not just one...

[cmlara]

For packages that are versioned properly rather than relying on this prefixed versioning, you can submit their advisories to https://github.com/FriendsOfPHP/security-advisories without issue.

If the FriendsOfPHP repository (this would be Packagist normally) is not the first repository listed a vulnerability submitted to them may not actually appear in the feed if a higher priority repository includes a vulnerability report section and does not include the FriendsOfPhp list.

Drupal is a good example of this scenario as noted by @mxr576 because they have very limited scope of what they consider a 'security' vulnerability, their data streams significantly benefit from enrichment from a secondary source.

Drupal however is not the only vendor this would apply to, every software developer tends to have their own interpretation of what a security vulnerability is leading to cases where an issue may be considered a vulnerability in one circle and a bug in another.

The global legal landscape holds an additional complication in this discussion, a bug can hold significant legal liability in one jurisdiction, to the level it justifies occurring in a security audit as insecure, while in another legal jurisdiction the issue is just an ordinary software bug.

@mxr576 independently came up with his enrichment scheme. As I implied in #11258 I expect more of these requests to occur in the future as the composer audit framework becomes more utilized by the community.

As it currently stands in order to maintain continuity a repository that wishes to enrich the audit data stream needs to:

1. Discover all other relevant security feeds
2. Periodically query those other feeds for every package
3. Merge the repositories into their their feed otherwise their results will not be displayed to the auditor.

This begins to raise a legal question (at least in my jurisdiction) around Copyright and site usage policy violations to do so. Addtionaly this includes a delay factor, dependent upon how often the feeds are queried and merged.

Even if a repository directly includes the FriendsOfPhp list it appears they would want/need to query packagist to get the advisoryId used in order to ensure continuity in the community, as two feeds should not use different ID's for the same vulnerability in order to avoid confusion. This is all significantly simplified by having the Audit command query all the listed repositories, each repository listing only the vulnerabilities that have been directly reported through them.

[mxr576]

@cmlara Thanks for the extra context. I did not want to go such deep in details, but I am glad that you did.

@Seldaek @stof What do you think? Did we manage to explain the reasoning behind the suggested change?

[Seldaek]

OK, IMO in an ideal world the repository providing packages should really be responsible for publishing all advisories if they do publish them at all, but I suppose this isn't matching reality.

As for a fix tho, I wonder if we shouldn't instead add a flag in the packages.json so that the drupal repository for example could define itself as incomplete, like security-advisories: { complete: false } or the like. Then if that would be set ComposerRepository would not return the packages in namesFound so that the packages would be fetched from lower priority repos still. Because it seems to me like it's a fairly specific problem and thus I'm not so keen on deoptimizing everyone else.

[mxr576]

I wonder if we shouldn't instead add a flag in the packages.json so that the drupal repository for example could define itself as incomplete, like security-advisories: { complete: false } or the like.

I think it would be blindsided to believe that this is only about Drupal. Yes, we came from that community to build something for that community. Still, the following words from @cmlara should also explain that the definition of "package is secure and welcomed to be used in production" can vary per developer and company. In theory, any PHP package or any package from a vendor could be flagged by an audit command extension as insecure/unwanted.

every software developer tends to have their own interpretation of what a security vulnerability is leading to cases where an issue may be considered a vulnerability in one circle and a bug in another.

The global legal landscape holds an additional complication in this discussion, a bug can hold significant legal liability in one jurisdiction, to the level it justifies occurring in a security audit as insecure, while in another legal jurisdiction the issue is just an ordinary software bug.

[cmlara]

I'm not so keen on deoptimizing everyone else.
If we enabled this for everyone I suspect we would (on average) be only adding a relativity small number of extra web requests.

The worst impacted for extra requests would be for a package that has security advisories listed in the first repository and none in any of the lower priority repositories.

I suspect enrichment sources would be the most likely to have this occur, however those are the cases we already want to continue on to a lower priority repository so no additional impact.

The second candidate for impact is 'sole source' repositories, I don't have a large dataset however when I look at Drupal installs the vast majority of the packages come from Packagist meaning only small number of extra requests would actually be made,. Addtionaly in cases where the repository doesn't provide any advisory data on its own (repository is depending upon Packagist publishing FriendsOfPhp or GHSA advisories) it would be no additional requests as those are already passing through the entire stack.

security-advisories: { complete: false } or the like.

If we are talking about repository provided parameter I would suggest we shouldn't trust a repository to provide this information. Its (sadly) not uncommon for those who discover a vulnerability to need to go public when a software publisher fails to address a vulnerability. We wouldn't want a publishers to be able to hide the Packagest FriendsOfPhp/GHSA lists by indicating no known vulnerabilities exist causing a false sense of security for projects.

If its a client side parameter I would personally prefer that repositories are considered incomplete by default and that we allow a project owner to force it to be considered complete in their root composer.json if they desire to prevent 'unnecessary' web requests. See above regarding the actual impact which to me feel insignificant compared to the risk that a project owner fails to set the config and as such ends up with known vulnerabilities missing in an audit report.

[Seldaek]

OK so let's assume we just skip the optimization always, if you have a drupal package and a repo only serving advisories for drupal packages ("enrichment source"), you would end up with some advisories duplicated if the drupal repo also provides them..

So we need to deduplicate somehow. We do this on packagist.org when aggregating advisories from GitHub + FriendsOfPHP sources https://github.com/composer/packagist/blob/main/src/SecurityAdvisory/SecurityAdvisoryResolver.php - I don't really love duplicating this in Composer, and overall it seems to me like people having to add enrichment source repos explicitly is a pretty crappy user experience anyway. Those who don't do it will not receive advisories at all.

Aren't there other alternative ways to fix this? Could you perhaps submit some code to packagist.org so that these advisories also get synced up there? We do support storing advisories for packages not served by packagist.org. I guess it doesn't solve the problem if the drupal repo does declare it reports advisories but it doesn't contain all of them, that's really a problem that needs to be addressed there IMO..

[stof]

@Seldaek I guess the issue with reporting them to packagist is exactly the case explained above about why drupal contrib advisories are not in FriendsOfPhp: Drupal has a weird versioning scheme where it has multiple repositories (one per major version of Drupal) that could reuse the same version numbers for contrib packages but for different codebases (the actual versions in the Drupal system being something like 8.x_1.3 and 9.x_1.3). Submitting those to packagist would break as it would report a 9.x package as vulnerable when only the 8.x one is.

[Seldaek]

Yes true.. That's another very Drupal specific problem though. I'm willing to help out but it feels a bit like cleaning up someone else's mess here.

[cmlara]

So we need to deduplicate somehow... I don't really love duplicating this in Composer...

From an automated standpoint most software is going to care that none exist, so I think were mostly doing this for user experience correct? If we were to just display the source repository would that be enough to call it 'not a duplicate' and avoid needing to try and do the checks to avoid that effort inside of composer?

Addtionaly I could see a source having its own links as part of the advisories that we might not want to de-duplicate away. Imagine an ELI5 enrichment source used internal to an organization to inform its developers, it would share the same CVE ID perhaps but link to much more plain language discussion on the vulnerability internally.

overall it seems to me like people having to add enrichment source repos explicitly is a pretty crappy user experience anyway

I agree in part and I disagree in part.

The fact that Drupal needs it because they are so limited on what they publish combined with the fact they endorse Public Disclosure instead of Responsible Disclosure for a large part of their ecosystem without provdiing for the publishing of advisories is a significant fault that should be changed by them.

On the other side of that, we have the example ELI5 above. These and other sources that none of us can even imagine yet can enhance the user experience but are not 100% necessary for every deployment and probably sould not be merged into the Packagist repo.

if the drupal repo does declare it reports advisories but it doesn't contain all of them,

Going beyond that still leaves other publishers beyond Drupal. Organizations are sometimes not agile, it is not far fetched to believe even a well intended repository might be delayed on getting an advisory out. In security timing can be critical, a few minutes can be the difference for a site being protected or not.

That's another very Drupal specific problem though.

I wouldn't be surprised if there are other (much less well known) repos doing similar. Thankfully its slowly going away for Drupal (once they get rid of D7 support) however if it has happened once odds are it has happened elsewhere.

[mxr576]

@Seldaek Thanks for accepting this suggested and for the discussion here, I have learned a lot.