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

Import advisories from the Github security vulnerability database automatically #626

Closed
klausi opened this issue Apr 12, 2022 · 10 comments
Closed

Comments

@klausi
Copy link
Contributor

klausi commented Apr 12, 2022

Problem: Maintainers use the Github security advisory database to publish security issues. Currently random developers like me find out about them when Github's dependabot flags them in a composer.lock file in one of my repositories. That is how the FriendsOfPHP/security-advisories database missed the Dompdf security issue #625 for 3 weeks, oopsie doodle.

Proposed Solution: Write a Github action that imports Github security advisories fully automatic into this repository. It could work something like this:

  • Github action runs periodically (once per day?)
  • It uses the Github GraphQL API to fetch all PHP composer security advisories from https://github.com/advisories?query=type%3Areviewed+ecosystem%3Acomposer
  • It checks based on CVE identifier if the advisory already exists in this repository
  • If not: it creates a new advisory file and commits it automatically (I assume this is somehow possible, not sure under which user account it would push the commit)

This could be a nice Google Summer of Code project or similar for a student :-)

@Ocramius
Copy link
Contributor

@klausi FWIW, https://github.com/Roave/SecurityAdvisoriesBuilder already aggregates this repo's contents together with the Github advisories into https://github.com/Roave/SecurityAdvisories

This repo is a source of advisories, not a derived artifact 🤔

@klausi
Copy link
Contributor Author

klausi commented Apr 12, 2022

Ah ok, did not realize that.

Then my confusion comes from https://github.com/fabpot/local-php-security-checker , which does not seem to use https://github.com/Roave/SecurityAdvisories and missed the dompdf security update.

Not sure if @fabpot would want to use your database as source then for https://github.com/fabpot/local-php-security-checker ?

@klausi
Copy link
Contributor Author

klausi commented Apr 12, 2022

@Ocramius checked the mission for this repo from the README: "This database must not serve as the primary source of information for security issues, it is not authoritative for any referenced software, but it allows to centralize information for convenience and easy consumption."

So it seems this repo is not a source anyway, and its purpose is to aggregate information. So then the automated import would make sense? Maybe we can copy from your approach how to scrape Github for the advisories :)

@Ocramius
Copy link
Contributor

At that point, given that the advisories DB is already a repository, why not using that one directly?

See https://github.com/github/advisory-database - their /advisories feed and API endpoints are produced from there.

@klausi
Copy link
Contributor Author

klausi commented Apr 12, 2022

Sweet, very good info! their JSON format contains "ecosystem": "Packagist",, so it should be possible to get the relevant PHP stuff out. https://github.com/github/advisory-database/blob/main/advisories/github-reviewed/2022/04/GHSA-x752-qjv4-c4hc/GHSA-x752-qjv4-c4hc.json

I see 2 distinct options:

  1. Either a Github action here imports that into FriendsOfPHP/security-advisories
  2. local-php-security-checker takes the Github security advisory database into account.

What is better for the PHP ecosystem? Is it valuable if security info is copied here to FriendsOfPHP/security-advisories?

@jaylinski
Copy link
Contributor

Note: this repo could also adopt the OSV format, which would make collaborating much easier: #599.

@naderman
Copy link
Contributor

If you want the information from both GitHub and this FriendsOfPHP repository, you can use the packagist.org database https://packagist.org/apidoc#list-security-advisories which aggregates both of them and handles de-duplication already.

@oliverchang
Copy link

Note: this repo could also adopt the OSV format, which would make collaborating much easier: #599.

Drive by comment from an OSV maintainer: ++++1 !!

Other DBs such as https://github.com/github/advisory-database also use the OSV format, which will make sharing vulnerability data (import/export) much easier.

@naderman this could also simplify the Packagist infrastructure greatly to only have to import a single, consistent format.

@GuySartorelli
Copy link
Contributor

For what it's worth, any discussion of adopting the OSV format belongs on the issue for that topic: #576
This issue seems to be about whether this repository should pull in advisories from the Github security advisory database - but as has already been pointed out, doing so would be counter to the purpose of this repository.

I think this issue should be closed. It seems like the discussion on the topic has already been resolved.

@marcovtwout
Copy link

I also think this issue can be closed now.

@klausi

Then my confusion comes from https://github.com/fabpot/local-php-security-checker , which does not seem to use https://github.com/Roave/SecurityAdvisories and missed the dompdf security update.

You could switch from fabpot/local-php-security-checker to composer audit (supported since Composer 2.4). It uses the packagist registry which (as pointed out above) uses this project as a source.

@fabpot fabpot closed this as completed Feb 27, 2023
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

No branches or pull requests

8 participants