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

Composer Repository Vulnerability Mirroring #4515

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

valentijnscholten
Copy link
Contributor

@valentijnscholten valentijnscholten commented Jan 4, 2025

Description

Mirrors Security Advisories from Composer Repositories.
Implementation based on / tested with:

The packagist repository provides advisories that are imported from:

So mirroring from Packagist provides little benefit, unless you have GitHub advisories disabled.

The biggest benefit is in third party repositories such as the Drupal8 repository. For some vulnerabilities (plugins/themes), this is currently the only way to get these vulnerabilities into Dependency Track. These are not tracked by Trivy/GitHub/OSV/Snyk/....

For this reason this PR introduces DRUPAL is a new SOURCE of vulnerabilities.

PACKAGIST is not added as a source as it doesn't provide new vulnerabilities and doesn't have commonly known IDs (PKSA-xxxx-yyyy).

This PR does introduce COMPOSER as a new source for Affected Version Attribution.

It mimics as mich as possible the code / behaviour of the Composer CLI. I had to up my Php reading skill to get there ;-)

Addressed Issue

Fixes #4465
Frontent companion: DependencyTrack/frontend#1130

Checklist

  • I have read and understand the contributing guidelines
  • This PR fixes a defect, and I have provided tests to verify that the fix is effective
  • This PR implements an enhancement, and I have provided tests to verify that it works as intended
  • This PR introduces changes to the database model, and I have added corresponding update logic
  • This PR introduces new or alters existing behavior, and I have updated the documentation accordingly

@valentijnscholten valentijnscholten changed the title Composer Vulnerability Mirroring Composer Repository Vulnerability Mirroring Jan 4, 2025
@valentijnscholten
Copy link
Contributor Author

@nscuro It's still in draft as I need to add docs, testcases and some cleanup. Would you be able to look at the overall approach taken?

@valentijnscholten valentijnscholten force-pushed the composer-vuln-mirror branch 2 times, most recently from e2fab37 to 6489de1 Compare January 5, 2025 14:03
@valentijnscholten
Copy link
Contributor Author

There's one thing specifically I need feedback on. It is on how to handle advisories from Composer repositories that are really just a mirror of a CVE or GHSA. Composer, or more specific packagist.org, is merely just a vulnerability aggregator similar to OSV. It aggregates CVEs and GHSAs. But also vulnerabilities from other sources such as FriendsofPHP.

This means a lot of advisories look like this:

    "nilsteampassnet/teampass": [
      {
        "advisoryId": "PKSA-q4rt-5vfc-wksb",
        "packageName": "nilsteampassnet/teampass",
        "remoteId": "GHSA-2697-96mv-3gfm",
        "title": "TeamPass does not properly check whether a folder is in a user's allowed folders list",
        "link": "https://github.com/advisories/GHSA-2697-96mv-3gfm",
        "cve": "CVE-2024-50701",
        "affectedVersions": "\u003C3.1.3.1",
        "source": "GitHub",
        "reportedAt": "2024-12-30 15:31:59",
        "composerRepository": "https://packagist.org",
        "severity": "medium",
        "sources": [
          {
            "name": "GitHub",
            "remoteId": "GHSA-2697-96mv-3gfm"
          }
        ]
      },

Which currently leads to DT creating a new vulnerability with Id PKSA-q4rt-5vfc-wksb.
This is not really an authoritative Id such as CVE/GHSA. You can't google these PKSA ids, you cannot link to them.
In DT it will just be another identical vulnerability linked as alias:

image

I am wondering if this is the best approach? Osv has similar situation, but they do not put their own Id on these vulnerabilities. They use the CVE or GHSA is Id for their advisories. And DT then does not create a new vulnerability (unless NVD or GHSA mirroring is disabled/lagging behind).

Should we do the same for these Composer advisories? Or should I just accept that DT is not (yet) good at deduplicating aliases and just add a new vulnerability with PKSA Id?

The idea is that VulnerableSoftware and AffectedVersionAttribution (and aliases) should still be synced for these vulnerabilities.

BTW After writing this comment I checked the advisories of packagist.org and it looks like all of them have at least a CVE or GHSA. I believe this is relatively new and it looks like GHSA has started to create advisories for all vulnerabilities of FriendsofPHP. So why enable Packagist advisory sync at all, just for the attribution part?

For other repositories like Drupal8 the mirroring is still valid as they provide their own vulnerabilities in their feed (SA-CORE-xxxx, SA-CONTRIB-yyyy). There maybe more repositories in the future, such as for Magento, Shopware, ...

This question about whether or not to "duplicate" all the CVE/GHSA vulnerabilities applies to more future vulnerability sources, for example Wordfence or WPScan. These are also aggregating from other sources. But they also add their own vulnerabilities to their feed. At some point you may have 5-6 aliases per vulnerability.

Copy link

codacy-production bot commented Jan 6, 2025

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
+0.00% (target: -1.00%) 77.52% (target: 70.00%)
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (80bb597) 22997 18333 79.72%
Head commit (daa81e3) 23437 (+440) 18684 (+351) 79.72% (+0.00%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#4515) 467 362 77.52%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

@valentijnscholten valentijnscholten force-pushed the composer-vuln-mirror branch 2 times, most recently from e392719 to 976bafa Compare January 12, 2025 17:41
@valentijnscholten valentijnscholten force-pushed the composer-vuln-mirror branch 2 times, most recently from e222894 to 381634c Compare January 28, 2025 18:11
Signed-off-by: Valentijn Scholten <[email protected]>
@valentijnscholten
Copy link
Contributor Author

Did not yet have an answer to my question above, but went ahead and finsihed the implementation and test cases. Vulnerabilities that have a GHSA or CVE are not replicated, we only set the affected version attribution. Updated the description of the PR with the implemented logic.

@nscuro
Copy link
Member

nscuro commented Jan 29, 2025

Should we do the same for these Composer advisories? Or should I just accept that DT is not (yet) good at deduplicating aliases and just add a new vulnerability with PKSA Id?

For the moment, it would be best to use CVEs and GHSAs if possible. The alias design needs an overhaul to properly deal with a lot of identifiers (perhaps you have some input for #4528?).

Separately, what's your impression on the mapping of those aliases you've seen so far? Do PKSA's always map to one CVE/GHSA, or can there be multiple? Could those PHP sources suffer from the same problem as google/osv.dev#888?

@valentijnscholten
Copy link
Contributor Author

valentijnscholten commented Jan 29, 2025

Separately, what's your impression on the mapping of those aliases you've seen so far? Do PKSA's always map to one CVE/GHSA, or can there be multiple? Could those PHP sources suffer from the same problem as google/osv.dev#888?

It's one-on-one, also for CVEs that alias GHSA and vice versa. Just verified that with some extra checks/logging during a mirror task. Packagist also makes some effort to deduplicate things from the two sources they import from:

When we first launched this API at Packagist.org, it used FriendsOfPHP/security-advisories as its only source. We’ve since expanded this database by importing advisories from the GitHub Advisory Database as well.

We’ll aggregate the data from these two sources and deduplicate advisories into a single entry if duplicates are found. By combining these two datasets we cast a wide net to quickly catch potential security threats from the world of open source PHP packages.

source: https://blog.packagist.com/discover-security-advisories-with-composers-audit-command/

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

Successfully merging this pull request may close these issues.

Use vulnerabilities from Composer package repositories
2 participants