CVE-2023-49081/2 fix question about aiohttp

[Dreamsorcerer]

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.

do not remediate the PoCs

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.

[kylebambrick]

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).

Makes sense. I appreciate you taking the time to clarify.

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.

No need. I was confused given only the information in the PoC and advisory.

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.

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.

[webknjaz]

@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.

[kylebambrick]

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.

[webknjaz]

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?

[kylebambrick]

No, I think it's fine how it is.

@webknjaz Do you know the PRs that fixed CVE-2023-49081 and CVE-2023-49082?

[webknjaz]

It looks like they didn't make it into the changelog: https://docs.aiohttp.org/en/latest/changes.html#id177.
@Dreamsorcerer do you have proper links to the patches? When making security releases I'd normally include change fragments. Although, I was doing it in a way that I had local commits and a tag. I was pushing the tag first, to trigger publishing. And only when it succeeded, I was pushing the branch with those commits. Do you think you could integrate something similar into your release routine?

[Dreamsorcerer]

@webknjaz Do you know the PRs that fixed CVE-2023-49081 and CVE-2023-49082?

https://github.com/aio-libs/aiohttp/pull/7835/files
https://github.com/aio-libs/aiohttp/pull/7806/files

When making security releases I'd normally include change fragments

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.

[webknjaz]

When making security releases I'd normally include change fragments

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.

[Dreamsorcerer]

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 ;) ).

[Dreamsorcerer]

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

[Dreamsorcerer]

Anyway, I assume the questions are resolved, so closing this.