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-qf9m-vfgh-m389] FastAPI Content-Type Header ReDoS #3479

Closed

Conversation

huonw
Copy link

@huonw huonw commented Feb 5, 2024

Updates

  • Affected products
  • References
  • Source code location
  • Summary

Comments
The problem seems to be in python-multipart, not fastapi (or starlette). As the existing advisory itself suggests, upgrading python-multipart itself seems to be sufficient to resolve this and so that seems like it should be the focus of report (and thus dependabot's upgrades). The same seems like it applies to GHSA-93gm-qmq6-w238

@github
Copy link
Collaborator

github commented Feb 5, 2024

Hi there @tiangolo! A community member has suggested an improvement to your security advisory. If approved, this change will affect the global advisory listed at github.com/advisories. It will not affect the version listed in your project repository.

This change will be reviewed by our Security Curation Team. If you have thoughts or feedback, please share them in a comment here! If this PR has already been closed, you can start a new community contribution for this advisory

@github-actions github-actions bot changed the base branch from main to huonw/advisory-improvement-3479 February 5, 2024 22:25
@tiangolo
Copy link

tiangolo commented Feb 6, 2024

Thanks! Yep, it makes sense. Thank you @huonw! 🙇

@darakian
Copy link
Contributor

darakian commented Feb 6, 2024

Thanks for the PR and the the update. I think fastapi should probably remain on this advisory since the published versions would include a vulnerable version of python-multipart. As for adding python-multipart we will want to get @andrew-d to chime in here to see if they're ok having their package added to what is otherwise an advisory about @tiangolo 's software. I think the ideal situation is that @andrew-d would make their own GHSA for the affected versions of their software while we leave this GHSA about @tiangolo 's software.

@huonw
Copy link
Author

huonw commented Feb 6, 2024

Hm, okay, I can see that making a bit of sense if we adjust this advisory to be about fastapi<0.87.0, since it seems 0.87.0 lifted the upper bound on python-multipart: fastapi/fastapi@89095eb

That said, it still seems a bit weird to be calling FastAPI and Starlette vulnerable because they mention a vulnerable library in their dependencies, rather than only highlighting the truly vulnerable libraries (and if people need to upgrade other libraries, like FastAPI and Starlette, to be able to apply the fix, then that's dependabot/their responsibility). In particular, if labelling reverse dependencies as vulnerable is the convention, it seems like there should be security advisories about all the libraries in https://github.com/andrew-d/python-multipart/network/dependents?dependent_type=PACKAGE (and all the ones not on GitHub too) for consistency, not just FastAPI and Starlette.

If I do a fresh install of that old fastapi version, I get the non-vulnerable version of python-multipart, i.e. seems like there's no problems:

python -m venv /tmp/fastapi-vuln
. /tmp/fastapi-vuln/bin/activate
pip install 'fastapi[all]==0.87.0'
pip freeze | grep multipart
# python-multipart==0.0.7

@darakian
Copy link
Contributor

darakian commented Feb 6, 2024

Hm, okay, I can see that making a bit of sense if we adjust this advisory to be about fastapi<0.87.0, since it seems 0.87.0 lifted the upper bound on python-multipart: fastapi/fastapi@89095eb

Seems reasonable to me 👍

That said, it still seems a bit weird to be calling FastAPI and Starlette vulnerable because they mention a vulnerable library in their dependencies

Part of how our advisory system works is that we encourage users to make advisories about their own software so that their users get alerted. It's much better for everyone if advisory publication is normal and easy 😄

wrt python-multipart, I think the ideal here is that the maintainer publish their own advisory, so lets wait and get @andrew-d to weigh in.

@darakian
Copy link
Contributor

@huonw, we now have GHSA-2jv5-9r88-3w3p live for the root vuln. Anything more you wanted to do on this PR other than update the specific version of FastAPI?

@Kludex
Copy link

Kludex commented Feb 13, 2024

[...] I think fastapi should probably remain on this advisory since the published versions would include a vulnerable version of python-multipart.

This doesn't make sense to me.

@Kludex
Copy link

Kludex commented Feb 13, 2024

Hm, okay, I can see that making a bit of sense if we adjust this advisory to be about fastapi<0.87.0, since it seems 0.87.0 lifted the upper bound on python-multipart: fastapi/fastapi@89095eb

The advisory says that all versions before 0.0.7 are vulnerable, so the constraints there doesn't change that.

@Kludex
Copy link

Kludex commented Feb 13, 2024

That said, it still seems a bit weird to be calling FastAPI and Starlette vulnerable because they mention a vulnerable library in their dependencies, rather than only highlighting the truly vulnerable libraries (and if people need to upgrade other libraries, like FastAPI and Starlette, to be able to apply the fix, then that's dependabot/their responsibility). In particular, if labelling reverse dependencies as vulnerable is the convention, it seems like there should be security advisories about all the libraries in andrew-d/python-multipart (dependents) (and all the ones not on GitHub too) for consistency, not just FastAPI and Starlette.

Yes. You are completely right.

We couldn't release an advisory on python-multipart because it was under a GitHub handler of an inactive user. I emailed him asking to transfer the project to me, and now python-multipart is under my handler. But at that point the advisory in FastAPI and Starlette was already released.

Can we remove the advisories from Starlette and FastAPI?

@huonw
Copy link
Author

huonw commented Feb 13, 2024

Great that we've now got one for the root cause. It sounds people more in the know have a handle on this particular advisory, so I'm gonna close this and defer to y'all if there's more updates to make to this or GHSA-93gm-qmq6-w238.

Thanks @Kludex!

@huonw huonw closed this Feb 13, 2024
@github-actions github-actions bot deleted the huonw-GHSA-qf9m-vfgh-m389 branch February 13, 2024 10:10
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.

5 participants