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

Supporting POSTs with multipart/form-data is dangerous without CSRF protection #270

Open
glasser opened this issue May 25, 2022 · 2 comments

Comments

@glasser
Copy link

glasser commented May 25, 2022

We recently discovered that supporting multipart/form-data POSTs in GraphQL servers makes it very likely that your server will be vulnerable to executing GraphQL mutations in CSRF attacks unless otherwise prevented.

(I'm a maintainer of Apollo Server. We today announced an advisory against an old version of our software that supported these requests by default, and opened a PR against the GraphQL multipart spec warning about this class of issues. Some of our users also use Absinthe which brought your implementation to my attention.)

This issue generally happens if:

  • Your CORS policy implementation functions just by setting access-control-allow- headers rather than by actually rejecting operations with disallowed origins. (Note that this is probably reasonable because it's easy for JS to send cross-origin GET requests without an origin header by inserting IMG tags or similar; that said, cross-origin POST requests in modern browsers should generally have an origin header.) While I'm not an expert, it does look like this is how the CORS Plug works.
  • Your server uses cookies for authentication or depends on some other property of the browser such as its IP address or access to private networks via VPN.

At the very least, now that this issue is known, I'd highly encourage you to highlight in docs that adding :multipart to your server is something you should only do if you're definitely using the upload feature (vs just by default) and only if you understand and are mitigating CSRF.

For what it's worth, Plug.CSRFProtection seems like overkill for this case. That uses a stateful CSRF token, which typically is what you need if you require normal form submission without JS to be able to send successful requests. Since GraphQL users are typically using JS, it's enough to just require some header like GraphQL-Require-Preflight to have a non-empty value, which is enough to make browsers preflight.

(We recently wrote some docs about CORS and CSRF which we think are helpful if it's not something you are deeply familiar with.)

@benwilson512
Copy link
Contributor

Hey @glasser thanks for the heads up. Definitely something we should look into. I want to avoid reinventing the wheel within the Elixir ecosystem, since CSRF concerns are generally handled by the dedicated plugs. However I'm not sure yet as you say whether Plug.CSRFProduction is overkill.

@glasser
Copy link
Author

glasser commented May 25, 2022

I mean, if most people are using sessions which make it possible to use CSRFProtection then that's fine.

Stateful CSRF protection is kind of from the old world where you needed to be able to accept requests that don't come from JS... requests that come from form submission and other things that don't have the ability to add extra headers. If you're fine if all browser use of your API involves writing code, just having that code include a non-CORS-safelisted header is sufficient (the value doesn't even matter!).

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

2 participants