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

[PyPI] Data quality issue for markdown2 #430

Closed
woodruffw opened this issue May 18, 2022 · 7 comments
Closed

[PyPI] Data quality issue for markdown2 #430

woodruffw opened this issue May 18, 2022 · 7 comments

Comments

@woodruffw
Copy link

@alex reported this upstream to pip-audit:

pip-audit -r <(echo 'markdown2==2.4.2') --no-deps

Produces:

Found 1 known vulnerability in 1 package
Name      Version ID                  Fix Versions
--------- ------- ------------------- ------------
markdown2 2.4.2   GHSA-p6h9-gw49-rqm4

But GHSA-p6h9-gw49-rqm4 isn't valid for 2.4.2 (it's only valid for <2.3.6): GHSA-p6h9-gw49-rqm4

It looks like OSV has both GHSA-p6h9-gw49-rqm4 and its CVE alias, but with a missing "version fixed" for the GHSA version: https://osv.dev/list?ecosystem=&q=CVE-2018-5773

cc @di as well for visibility.

@oliverchang
Copy link
Collaborator

Thanks for flagging! This is because GitHub's OSV encoding does not include the fix: https://github.com/github/advisory-database/blob/d6004eb8de91ad341605da869ab1b9f1e4abe433/advisories/github-reviewed/2018/07/GHSA-p6h9-gw49-rqm4/GHSA-p6h9-gw49-rqm4.json#L3

On GHSA-p6h9-gw49-rqm4, there's no listed patch, which is what led to this missing "fix" property. I believe the correct course of action here is to figure out the right fix version and amend the GHSA entry.

@alex
Copy link

alex commented May 19, 2022

Not understanding the architecture of OSV that well: should OSV be merging these two reports into one, and therefore having the "fixed" data in either is sufficient?

@woodruffw
Copy link
Author

On GHSA-p6h9-gw49-rqm4, there's no listed patch, which is what led to this missing "fix" property. I believe the correct course of action here is to figure out the right fix version and amend the GHSA entry.

Sounds good. I can look into a PR on GitHub's side later today.

Not understanding the architecture of OSV that well: should OSV be merging these two reports into one, and therefore having the "fixed" data in either is sufficient?

My understanding of OSV (which might be very wrong!) is that it keeps all reports around, and only links them via alias sets. So on the client's side, there's an annoying ambiguity: we might deduplicate via aliases, only to have the "authoritative" record (which we choose semi-arbitrarily) be lacking some metadata. It'd be nice for OSV to do some kind of consistency checking between aliased reports, but I suspect that would require a lot of manual cleanup.

@oliverchang
Copy link
Collaborator

On GHSA-p6h9-gw49-rqm4, there's no listed patch, which is what led to this missing "fix" property. I believe the correct course of action here is to figure out the right fix version and amend the GHSA entry.

Sounds good. I can look into a PR on GitHub's side later today.

Not understanding the architecture of OSV that well: should OSV be merging these two reports into one, and therefore having the "fixed" data in either is sufficient?

My understanding of OSV (which might be very wrong!) is that it keeps all reports around, and only links them via alias sets. So on the client's side, there's an annoying ambiguity: we might deduplicate via aliases, only to have the "authoritative" record (which we choose semi-arbitrarily) be lacking some metadata. It'd be nice for OSV to do some kind of consistency checking between aliased reports, but I suspect that would require a lot of manual cleanup.

Right. We haven't been able to come up with a good way to merge reports automatically. For now I think it makes sense to consider each report separately -- clients can decide their appetitie/strategy for dealing with them.

Even in this case with GHSA-p6h9-gw49-rqm4 and the listed issue (trentm/python-markdown2#285 (comment)), it's not actually perfectly clear if the underlying vulnerability is fully fixed, so it's not necesarily the case that the PYSEC entry is more accurate.

@woodruffw
Copy link
Author

Did some more looking into this: according to markdown2's changelog, 2.3.6 is considered the fixing version: https://github.com/trentm/python-markdown2/blob/master/CHANGES.md#python-markdown2-236

So this is probably a data issue in the GHSA report, and I'll file an upstream issue.

@woodruffw
Copy link
Author

Fix: github/advisory-database#324

@oliverchang
Copy link
Collaborator

Fix has been merged. Thanks @woodruffw !

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

3 participants