CVE-2023-49081/2 fix question #8058
Comments
|
The fix we implemented was basically a minimal change that stops the issue happening for the most part. The primary concern was accepting a JSON loaded value and passing it to the version. The change just switched from indexing the tuple to using the attributes, which means that no JSON object would be able to get past that code (as you'll now get an AttributeError). We could take a PR to make it a bit tighter if desired, but the vulnerability was basically considered a longshot and with that change in place it now stretches credulity.
If you find you can actually get an attacker modified output with a list, then we need to check it. But, you should be hitting that AttributeError instead. |
Makes sense. I appreciate you taking the time to clarify.
No need. I was confused given only the information in the PoC and advisory.
Sounds good. Thanks again! Could you please link the PR(s) for the fix for future reference? I'm having trouble finding it. It might clear up future confusion in case anyone stumbles on this. |
kylebambrick
changed the title
|
@kylebambrick I'd like to call out that it is important to report anything security-sensitive and related concerns through the private channels as specified by https://github.com/aio-libs/aiohttp/security/policy and keep it that way until it's determined if it's acceptable to be shared publicly. So that such things are processed and disclosed responsibly. |
|
Sorry. Given the PoC and CVE were public (and not novel), I felt it more appropriate to provide all the information in a public bug. I will follow that process in the future. Thanks for letting me know. |
|
Right. It's just that you claim that a CVE is not actually addressed and this information may mean there's something extra to consider. I think that the rule of thumb should be that whenever there's any hint or uncertainty that a post may point to some potential vulnerability, it's best to double-check and privately confirm whether it's okay to go public. Of course, all parties here are volunteers, and I recognize that I do set the bar high. So I can't expect anybody to always be on top of everything. Just trying to be mindful of the responsibility to the community we're all a part of. Is there anything that would make such messaging clearer? Maybe, an extra paragraph in the security policy or a warning in the issue forms? |
|
No, I think it's fine how it is. @webknjaz Do you know the PRs that fixed CVE-2023-49081 and CVE-2023-49082? |
|
It looks like they didn't make it into the changelog: https://docs.aiohttp.org/en/latest/changes.html#id177. |
https://github.com/aio-libs/aiohttp/pull/7835/files
If we're not disclosing the issue immediately, then it probably shouldn't be highlighted in the changelog, right? But, we could certainly update the CVE to include the PR. |
Don't we disclose on release? Anyway, it's useful to update the changelog even if it's disclosed later. |
|
These particular ones I think were snuck into a release candidate and disclosed around a week or so later. But, I think many others we've disclosed a day or 2 later (and there was the one you had in draft for years ;) ). |
|
Also, just realised I don't know how they've produced those severity ratings on the CVEs. That one for the version is rated as high, while our own advisory is rated low (internally, I suggested this was a 1/10 severity): GHSA-q3qx-c6g2-7pw2 |
|
Anyway, I assume the questions are resolved, so closing this. |
Describe the bug
Hello,
Please let me know if I'm missing something. While looking into backporting the recent CVEs, I found the fixes for GHSA-q3qx-c6g2-7pw2 and GHSA-qvrw-v9rv-5rjx do not stop the PoCs at https://gist.github.com/jnovikov/184afb593d9c2114d77f508e0ccd508e and https://gist.github.com/jnovikov/7f411ae9fe6a9a7804cf162a3bdbb44b.
It looks like 3.9.0 introduced checks/sanitization in the http_parser.py. However, the PoC demonstrates an attack in client.py.See the #8057 for something that may catch the malformed parameters. For GHSA-q3qx-c6g2-7pw2, the changes add an HTTP version check to ClientSession(...) and throw an error if the version is not an HTTP version. For GHSA-qvrw-v9rv-5rjx, the changes add a check for the HTTP methods passed to request(...) and throw an error if the str is not an RFC 2616 method.
Given the attack path, it's reasonable to argue the vulnerability is informational. Using untrusted or tainted input for the HTTP version or method is really bad practice and very unlikely. In other projects, I've seen them state the parameters for a request like this (not response handling) are considered trusted input. It's up to the developer to validate the input before calling.
However, given the PoC already has a link to a CVE, every scanner is flagging older aiohttp libraries. For us downstream folks, arguing against fixing a published CVE (or NVD's severity) is harder than getting the issue fixed. It's equally painful to deal with a rejected or disputed CVE. So, I recommend fixing the issue and updating the CVEs with a new fix version (and a new severity). Whatever was fixed (with http_parser?) should get a new CVE. Anyway, your call.
Please let me know if you have any questions.
Regards,
Kyle
To Reproduce
See the PoCs at the PoCs at https://gist.github.com/jnovikov/184afb593d9c2114d77f508e0ccd508e and https://gist.github.com/jnovikov/7f411ae9fe6a9a7804cf162a3bdbb44b.
Expected behavior
We would expect to throw a value error when supplied an invalid RFC 2616 method or HTTP version.
Logs/tracebacks
Python Version
aiohttp Version
multidict Version
yarl Version
OS
Windows 11
Version 22H2 (OS Build 22621.3007)
Related component
Client
Additional context
No response
Code of Conduct
The text was updated successfully, but these errors were encountered: