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

GHSA-w596-4wvx-j9j6: mark as withdrawn #762

Conversation

woodruffw
Copy link

This is believed to be CVE spam: it's a low-value ReDoS report on an 18 year old code snippet that doesn't affect its primary dependent, and is not known to be used by any other significant codebases.

See: pytest-dev/py#287

Signed-off-by: William Woodruff william@trailofbits.com

Signed-off-by: William Woodruff <william@trailofbits.com>
@github-actions github-actions bot changed the base branch from main to woodruffw/advisory-improvement-762 October 20, 2022 00:40
@darakian
Copy link
Contributor

Hi @woodruffw 👋
I see from pytest-dev/py#287 (comment) that you're also reaching out to mitre to withdraw the CVE. Given that they are the root for this issue would you mind letting us know when that is complete?

@woodruffw
Copy link
Author

I see from pytest-dev/py#287 (comment) that you're also reaching out to mitre to withdraw the CVE. Given that they are the root for this issue would you mind letting us know when that is complete?

I can do that, although IMO you would not be wrong to preempt MITRE here: GitHub's advisory database is a direct reporting source for OSV and PyPI's vulnerability feeds, which means that withdrawing the vulnerability here would solve the spam that thousands of pytest users are currently experiencing from this vulnerability report.

There is also precedent for withdrawing a GHSA prior to CVE retraction: for example GHSA-h385-52j6-9984 is marked as withdrawn, but its corresponding CVE (https://nvd.nist.gov/vuln/detail/CVE-2020-7670) is not yet retracted or even disputed. Similarly, GHSA-rwqr-c348-m5wr is withdrawn but its CVE is only considered "disputed," not fully retracted.

Combined with the fact that the maintainers of the project consider this to be a non-vulnerability, I think withdrawing immediately would be appropriate.

@darakian
Copy link
Contributor

Ya if you don't mind going to mitre first I would prefer that route get taken. It's most ideal if advisory validity can be dealt with at the source.

@woodruffw
Copy link
Author

Ya if you don't mind going to mitre first I would prefer that route get taken. It's most ideal if advisory validity can be dealt with at the source.

Yep, I sent them the initial revocation request last night. I haven't heard back from them, however, and they don't guarantee any particular response time.

I figure I can give them a couple more weekdays, but given the degree of spam here (which is affecting your own tools as well, like Dependabot) I think GitHub should strongly consider withdrawing the GHSA if MITRE hasn't responded by around this time next week.

@darakian
Copy link
Contributor

I think GitHub should strongly consider withdrawing the GHSA if MITRE hasn't responded by around this time next week.

We can cross that bridge if/when we come to it. In the mean time I'm happy to help clarify the advisory as much as possible, but to my eye the advisory is in fact talking about a real redos vulnerability. I do understand the annoyance with what you call CVE spam, but if the advisory is valid then we want to include it in our data set.

@The-Compiler
Copy link

The-Compiler commented Oct 21, 2022

but to my eye the advisory is in fact talking about a real redos vulnerability. I do understand the annoyance with what you call CVE spam, but if the advisory is valid then we want to include it in our data set.

In theory, when viewed in a vacuum: indeed.

However, even a very broad search on GitHub for the affected code yields 92 results. I've looked through them all, and from what I can tell, all the matches fall into one of those categories:

  • Copies of py using it (in svnurl(), which the search captures as well)
  • Copies of pypy (where the py library originates from historically), in it's development tools directory. However, PyPy has moved away from SVN in 2010, so they're probably just bitrotting. In any case, they use the repository the current file is in, or are hardcoded to run on a specific old PyPy team server (codespeak.net). If someone has control over that, they might as well just change those scripts. Even if there are certain scripts which actually run on arbitrary SVN servers, the context they are run in is unlikely to make a DoS a real problem. But given that they won't actually work anymore for their intended original purpose, the chance of anyone running them is pretty much zero.

On the other side of the coin, you have at least half a million projects depending on pylib via pytest, which got noise in their inbox.

I believe the main point of a CVE and security advisories is to make people aware of problems which have a real (even if small) chance of affecting them. An advisory which just adds noise to what I believe will be 100.0% of the receivers is just going to hurt the whole system and community.

In this particular case, and when viewed in context, the sheer amount of noise the advisory generates vs. its real usage is so high that the word "spam" is unfortunately very fitting. I can't help but think that "oh, I get a shiny CVE in a popular project" was the only motivation behind it.

@darakian
Copy link
Contributor

I believe the main point of a CVE and security advisories is to make people aware of problems which have a real (even if small) chance of affecting them. An advisory which just adds noise to what I believe will be 100.0% of the receivers is just going to hurt the whole system and community.

The difficulty for me there is that a redos is a small but real issue. I hear you on the noise issue, but this is not the appropriate place to litigate CVEs. @woodruffw is taking this up with mitre which is the appropriate place to dispute this. Lets see how that goes and we can address the outcome of that step once its complete.

If I am mistaken and there regular expression in question does not have have exponential behavior please let me know.

@woodruffw
Copy link
Author

FWIW, I took @The-Compiler's point to be not about litigating CVEs, but pointing out the problem with ecosystems like GHSA blindly trusting CVEs and other feeds, rather than performing their own curation.

This isn't a problem that's unique to GHSA (not even remotely!), but it's certainly amplified by GHSA's prominence and breadth of users.

You're right that the regular expression does have exponential behavior, but exponential behavior is just a normal thing in lots of program contexts. The context in which ReDoS applies is generally web applications as a remote client; the original reporter provided no evidence that any uses of this library in that context exist, and the maintainers themselves were unable to find any either.

@darakian
Copy link
Contributor

I take that point as well and we do not blindly trust CVEs. I do however want to respect our colleagues in the security community and give them a chance to respond to this before GitHub takes unilateral action.

You're right that the regular expression does have exponential behavior, but exponential behavior is just a normal thing in lots of program contexts. The context in which ReDoS applies is generally web applications as a remote client; the original reporter provided no evidence that any uses of this library in that context exist, and the maintainers themselves were unable to find any either.

Context is insanely hard for software libraries given that they can be embedded in anything from a toy to a medical device. I do want to reassure that we are working on the problem of precision delivery of alerts as well as precision data.

So again; if I can help clarify the advisory that is live and is being delivered to people today I want that to be as nice an experience or as easy a no-op as possible. Please let me know if there's more I can do there.

@SCH227
Copy link

SCH227 commented Oct 24, 2022

Hi! I am the one who reported this issue.
IMO, the regex is vulnerable, it can be triggered by calling py code, and the attack scenario is possible. Also, a very similar issue in py was assigned a CVE in the past. So, it's valid.
However, I see that the package maintainers may have a more complete understanding of their code usage. If they are completely sure that the vulnerable code is not used anywhere (GH, a bank, an app, a device, etc) and no one will ever think on using it (py was downloaded +47.000.000 times just last month), I am ok with the withdrawn.
Still, I recommend fixing the regex, so it doesn't gets copied.

@The-Compiler
Copy link

py was downloaded +47.000.000 times just last month

That's almost solely because it's a dependency of pytest, which depends on py and was downloaded 46.8M times in that timeframe (linking to a different page because pypistats.org somehow doesn't want to display pytest for me).

@SCH227
Copy link

SCH227 commented Oct 24, 2022

That's weird, on the link you provided, I had problems finding py.
We can see both here:
image
You are right, the downloads numbers are on the same order of magnitude. However, a difference of +700.000 downloads for py is not a negligible number

@woodruffw
Copy link
Author

woodruffw commented Oct 25, 2022

However, a difference of +700.000 downloads for py is not a negligible number

It would not surprise me if a significant fraction of this difference came from standard development practices, e.g. upgrading the environment that a development copy of pytest is installed into.

Edit: meaning that a copy of py is downloaded and installed, but not pytest since the local copy is used.

@darakian
Copy link
Contributor

I'm going to close this out given the new release of pytest which removes the dependency on py ref: pytest-dev/py#287 (comment)
I'll add some extra text to the advisory to indicate that version 7.2.0 of pytest will resolve any scanner issues as well.

@woodruffw feel free to open a new pr/issue if/when the CVE is revoked and we can pick this back up 👍

geoabraham pushed a commit to geoabraham/practical-cli-game that referenced this pull request Jan 18, 2023
@AnshumanParashar
Copy link

I'm going to close this out given the new release of pytest which removes the dependency on py ref: pytest-dev/py#287 (comment) I'll add some extra text to the advisory to indicate that version 7.2.0 of pytest will resolve any scanner issues as well.

@woodruffw feel free to open a new pr/issue if/when the CVE is revoked and we can pick this back up 👍

Hi, I would like to bring this to your attention that it's not just about pytest using "py" module. Even the retry module is dependent on py.

@darakian
Copy link
Contributor

darakian commented Mar 1, 2023

@AnshumanParashar the advisory is still live.
GHSA-w596-4wvx-j9j6

Is there another concern you have that I can address?

@merwok
Copy link

merwok commented Mar 1, 2023

Yes that’s the issue being pointed out! Previous comment said that the update of pytest meant that it wasn’t necessary to apply this PR, but as AnshumanParashar pointed out that doesn’t solve all cases, so it would still be useful to withdraw this advisory that’s noise for most if not all projects.

@darakian
Copy link
Contributor

darakian commented Mar 1, 2023

@merwok if you would like to get the CVE withdrawn please contact mitre.

@merwok
Copy link

merwok commented Mar 1, 2023

At the end of October you said you would give a little time to see Mitre’s response before editing the data on Github’s side.
Can you now reconsider editing the data on Github’s side?

(For the reasons already explained in detail by other people)

@darakian
Copy link
Contributor

darakian commented Mar 1, 2023

I can, however it seems that mitre has not changed their stance. The CVE is still live
https://cve.mitre.org/cgi-bin/cvename.cgi?name=2022-42969

Can you elaborate on your concern with this advisory?

@woodruffw
Copy link
Author

Not to relitigate this, but I think a more accurate framing is that "MITRE has not bothered to respond to anyone," not that they haven't changed their stance 🙂

I think GitHub is a much better actor in this space than MITRE and the other CNAs have been, and is in a good position to lead by example here!

@darakian
Copy link
Contributor

darakian commented Mar 2, 2023

I think a more accurate framing is that "MITRE has not bothered to respond to anyone," not that they haven't changed their stance 🙂

Fair.

I think GitHub is a much better actor in this space than MITRE and the other CNAs have been, and is in a good position to lead by example here!

Thank you for the praise, but I'm going to immediately disappoint you here and say that my stance has not changed. I think there's value in indexing the vulnerability. The delivery of the advisory to the correct parties absolutely needs to get better and I can't speak to specifics but we are working to address that issue.

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.

None yet

6 participants