Until recently Symfony allowed HTTP request method overrides via request parameters. This introduces a critical CSRF vulnerability in REST module. Example: a POST request like this
POST http://example.com/entity/node/1?_method=DELETE
is automatically transformed to
DELETE http://example.com/entity/node/1
That means an attacker could prepare a malicious form on another site to trick a priviledged user into submitting a form like this:
<html>
<form action=http://example.com/entity/node/1?_method=DELETE method=post enctype="text/plain" >
<input type=submit>
</form>
</html>This would delete node 1 on the target example.com site.
This was fixed in the Symfony master branch, so we just need to upgrade Symfony. Which means this issue depends on #1834594: Update dependencies (Symfony and Twig) follow up fixes for Composer.
This issue has been discussed in the Drupal security team and has been cleared for public discussion after a similar issue was fixed in RESTWS and the Symfony maintainers released the fix (they don't consider this a security issue in Symfony).
| Comment | File | Size | Author |
|---|---|---|---|
| #3 | rest-csrf-1854902.patch | 2.28 KB | klausi |
| #3 | rest-csrf-1854902-fail.patch | 2.94 KB | klausi |
Comments
Comment #1
klausiTagging.
Comment #2
scor commented#1834594: Update dependencies (Symfony and Twig) follow up fixes for Composer was committed, could you check this is fixed as expected? should we have some kind of test for this?
Comment #3
klausiYes, we should have simpletests for this.
Two patches attached, one demonstrates the exploit when HTTP method overrides are enabled and should fail.
Now the remaining question is whether we should use Request::getRealMethod() instead of Request::getMethod() in the first place. Pro: improved security, no CSRF issue even if another module enables HTTP method overrides globally. Contra: X-HTTP-METHOD-OVERRIDE HTTP headers will not work with REST module, which might be relevant for sites sitting behind obscure firewalls that only allow GET/POST and not DELETE requests for example.
Comment #5
klausiFailed as expected, yay!
Comment #6
Crell commentedThere's 1001 ways that you can make your site insecure by doing something dumb. No need to wall of this one in particular. Let's just make sure it's documented to not enable that "feature" and call it a day.
Comment #7
klausiFine with me, where do we document this?
And the question remains: should we use Request::getRealMethod() in the first place?
Comment #8
Crell commentedgetRealMethod() would imply disabling the header override. Unless that's a security risk I don't think we need to disable it. Plus, I'm 100% certain contrib developers will still use getMethod() (if they use anything at all), so that would just introduce inconsistency.
Comment #9
plachSorry, guys, correct me if I am wrong but I don't see how this is critical or even major now that the security issue is fixed. Shouldn't this be a normal task?
Comment #10
plachSomething like this?
Comment #11
Crell commentedAgreed with #10. There's no hole anymore unless you do something dumb. We just need to document how to not be dumb.
Comment #12
klausiNow that we have to require tokens anyway (see #1891052: Protect write operations from CRSF by requiring an token (when using cookie/session-based authentication)) there is no harm in using HTTP method overrides. So we can simply close this.
Comment #12.0
klausiAdding an H2 to Solution.