Page MenuHomePhabricator

REST: improve CSRF token handling
Open, MediumPublic

Description

Support for CSRF token handling in the REST framework should be improved to reduce the amount of boiler plate code required in handlers. Also, we should protect against handlers forgetting about the need for CSRF protection.

Currently, handlers that should protect against CSRF must:

  • either override requireSafeAgainstCsrf() to return true, to disallow cookie based auth.
  • or use the TokenAwareHandlerTrait to implement token based CSRF protection.

Handlers that use TokenAwareHandlerTrait currently must add some boiler plate code in order to work:

  • override the validate() method to include a call to the validateToken() method provided by TokenAwareHandlerTrait.
  • override getBodyParamSettings() to include a call to the getTokenParamDefinition() method provided by TokenAwareHandlerTrait.

Handlers for POST or PUT or DELETE that fail to do any of the above will be vulnerable against CSRF attacks.

Option 1

Better integrate TokenAwareHandlerTrait with the Handler base class.

  1. Introduce a validateToken (or similar) method into the Handler base class, and make TokenAwareHandlerTrait provide an implementation for that method. The default implementation in the base class will throw a LogicException.
  2. Let the base class detect if CSRF protection is needed. This is the case if the request is not using a "safe" method (GET or HEAD) and needsWriteAccess() returns true.

This way, vulnerable handlers that forget to implement CSRF protection will fail with a LogicException. And handlers that do implement CSRF token handling no longer need to override the validate() method, though they still have to call getTokenParamDefinition() from getBodyParamSettings().

Handlers can control the name of the token parameter by overriding the getToken() method.

Experimental patch: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1034457

Option 2

Make CSRF token handling automatic in the Handler base class.

The Handler base class can detect if CSRF tokens are needed (see above). If so, it can construct a helper object, CsrfTokenHandler or some such, that implements token validation. getBodyParamSettings() will automatically return a param spec for the token parameter by calling getTokenParamDefinition(), which would also be added to the base class. Handler classes that implement getBodyParamSettings() will have to call the parent implementation.

This way, handlers don't have to know or care about CSRF protection at all. It just happens.

Option 3

Make CSRF token middleware

If we introduce support for middleware into the REST framework, CSRF token handling can be implemented in a middleware component that can be applied to endpoints by declaring it in the route definition.

This way, the Handler base class as well as concrete handlers could be entirely unaware of CSRF tokens and session handling. However, there would be no protection against handlers failing to declare the necessary middleware. Unless perhaps the CSRF protection middleware would be added automatically to handlers for methods other than GET and HEAD.

The name of the token parameter could be configurable in the middleware, but that would be in the route definition. The handler itself would be unaware of the additional parameter. It's not clear if and how middleware components could declare parameters (resp. body fields).

Related Objects

StatusSubtypeAssignedTask
In ProgressNone
OpenNone

Event Timeline

Some quick thoughts:

  • We have some non-standard token handling in production. An example (admittedly, the only one I'm aware of) is the the Reading Lists extension. Those newly-created REST endpoints replaced endpoints of the same contract in RESTBase and, to maintain compatibility, are obligated to honor that contract. These endpoints have to accept csrf tokens in an unusual way: as a query parameter named "csrf_token". However, to encourage eventual migration to standard practice, they also accept tokens in the usual way: as a body parameter named "token". These quirks are currently dealt with in a "ReadingListsTokenAwareHandlerTrait". It'd be nice for whatever solution we settle on to provide sufficient flexibility to deal with this sort of oddity.
  • I wonder if we can/should provide some protection at the testing level. We already have RestStructureTest, which requires conformance to certain conventions. Could/should csrf be another required convention? If so, this might provide a safety net for the various "sort of automatic but not really" options, as well as for handlers that override the default behavior in inadvisable ways.
  • I realize the above two bullet points might be contradictory. The first says we have things that don't work the normal way, and we should allow it. The second says we should make sure everything works the normal way. I have no immediate answer for how to resolve that complication, but we might need to.
  • Option 1 says csrf protection is needed "if the request is not using a "safe" method (GET or HEAD) and needsWriteAccess() returns true." That's not always true, is it? Protection is only needed for session types that are not safe against csrf (OAuth, for example, is safe). A handler might set requireSafeAgainstCsrf to require such a session type. But maybe a little more worrying, if developers relied on a run-time LogicException to remind them about csrf handling, but then never invoked the handler with an unsafe session type (I note that HandlerTestTrait::initHandler() default to safe) it seems not all that hard for an unsafe handler to make it to production.
  • Option 2 is basically Option 1, but a little more automatic, right? The main way derived handlers could could inadvertently mess things up is to not call the parent implementation of getBodyParamSettings()? It still seems possible to mess this up, but less likely, especially as in practice most derived handlers would be implemented via copy/paste (who types everything from scratch???), so once we have best practice implemented everywhere, it'd tend to stick.
  • I like the idea of Option 3, but there are a lot of unknowns.
  • Is there a compelling reason not to implement Option 2 soon(ish), and consider Option 3 longer term? I know that means implementing a temporary, throwaway solution, but it doesn't seem like a time-consuming one.

Additional thought: we have at least two http DELETE endpoints that do not use tokens (ReadingLists DELETE lists and DELETE lists/{id}/entries) that intentionally do not use csrf protection, and for compatibility reasons cannot. (The MW REST endpoints were created to be compatible with the original RESTBase endpoints, which did not require csrf tokens for these endpoints. Existing mobile apps will therefore call these endpoints without a csrf token, so requiring it now would be a user-facing breaking change, as users may have older version of the mobile apps on their devices.)

So unless we consider this serious enough to break things, whatever solution we use should allow specific handlers to opt out of csrf protection, even if they would by default provide it.

(How important it is for DELETE endpoints to provide csrf protection is a matter of some debate on the wider internet. I don't see a compelling reason to not do it, and am fine with our policy being to do it by default. But I personally don't consider the lack of it for these particular endpoints to be a major security hole either.)