[PP-1] Disable CSRF token check for non-CSRF vulnerable authentication providers

#2869426: EntityResource should add _entity_access requirement to REST routes is the only significant change in 8.7.x, but should not have caused this behavioral change.

Note that \Drupal\Core\Access\CsrfRequestHeaderAccessCheck only generates this error response if

 $this->sessionConfiguration->hasSession($request)

evaluates to true. That means a session cookie must exist on the request. Any chance you're sending both OAuth credentials and a session cookie?

Just saw this issue in our project.
We have a decoupled client-server App, that uses Bearer token authentication.
Error occurred when sending POST requests to create comments.

From analysis, there was indeed a session cookie sent at the request.
And the reason we had this cookie, is a login we performed to Drupal, while client and server shared same domain.
So we logged in on server, got the cookie, then client logged in with Bearer token,
and sent the cookie as part of POST, since cookie was set on the domain.

This started to be a problem for us with https://www.drupal.org/sa-contrib-2018-021 . We worked around it by getting a token and sending it along. Not a good solution, and an unreliable one at that. I had not investigated at the time why Drupal had opened a session (sometimes it did, sometimes it did not).

I recently got new bug reports because of this so I tracked it down in our case: We are using jsonapi to get the contents of nodes in one of several languages. In order to determine the language to return we are adding a query parameter "content_langcode" and we configured the language detection to use this parameter ("Language from a request/session parameter."). I just learnt that this plugin persists the parameter to the session thus opening a new one if none exists.

So after the first request that contains a "content_langcode" parameter the token is suddenly required and checked for the verbs / routes that became protected with the fix to sa-contrib-2018-021.

I rerolled the patch from #5 with minor changes:

• do not remove comment (I think that was done accidentally)
• fix formatting / parenthesis
• implement performance hint from PHP Inspections: strpos instead of substr saves memory.

I noticed I haven't stated it yet: The patch from #5 / #11 solves the issue for me.

We are writing a big e-learn (SPA) application using oauth2 / JsonApi and experience the same problem when testing with Cypress (https://www.cypress.io/) for e2e testing. During testing everything starts up fine (GET) but later in the process POST and PATCH requests fail with X-CSRF-Token request header missing.

Direclty starting application works fine and from the debug window the requests (headers) directly and from behind the Cypress proxy are exactly the same (no cookies). The Cypress proxy might add something after this (a cookie?) but it's hard to tell.

Anyway the patch of #11 is solving the problem. I was wondering if this patch is going to be accepted to core.
Cypress is a wonderful testing tool and it would be a shame if we were unable to use it.

Patch #5 and #11 solved this problem.

It's a big problem, when will it fix?

Well, to me this patch is a security issue. Just add Authorization header with any bearer token (without having enabled simple_oauth at all), and CSRF token check will be cancelled. As I would see the solution, we would need to know not only that the user is authenticated, but which method was used for authentication. AFAIK, it's not possible now.

How do you mount a CSRF attack that adds a header?
And even if that is possible, shouldn't the presence of an "Authorization: Bearer ..." header switch off any secondary attempts at cookie-based auth altogether?

From what I've been told the CSRF protection is only needed for cookie-based auth.
@Wim Leers: Please correct me if I'm wrong there. I recall talking with you about a related issue.

Moved this issue to Simple Oauth issue queue to solve it within its codebase. If Simple Oauth is not enabled, Bearer header makes no sense.

Implementing approach suggested by hchonov: decorating a CsrfRequestHeaderAccessCheck from within simple_oauth module, and skipping csrf token check within this decorator.

Minor stylistic update.

Moving back to Drupal core. Many modules (contributed and custom alike) may use that header.

But there is no way to ensure that the authentication has been made indeed through one of them.

I don't think that this belongs into core where there is no such implemention. Moreover I think that this should be tied to the responsible authentication provider directly.

Recently we've fixed an issue in simple_oauth where with both sent Bearer Authorization header and a session cookie, the oauth authentication failed and the session cookie authentication succeeded. In this case it would've been wrong to skip the CSRF header check because core has "possibly" a different implementation than a custom or contrib authentication provider. The issue I am talking about is #3063292: Bearer auth without token not detected correctly and might mix up users.

For consistency and security reasons I think this should be tied to the authentication provider. I don't think that it makes sense to consider what is currently possible and what not. Even if the responsible code lives in core simple_oauth should still actively turn off the checking if it is the one authenticating the request.

Patch #11 works fine for me on Drupal Core 8.8.1

I don't think that this belongs into core where there is no such implemention.

Basic Auth is a core module, has the same problem.

Moreover I think that this should be tied to the responsible authentication provider directly.

Yes, this should be core's cookie authentication provider's responsibility, as that's the only authentication method that cares about X-CSRF tokens. It is quite unreasonable to place the burden on literally every other authentication provider to override/decorate core.

I think to solve this properly, we need a service which keeps track of which authentication provider was *actually* used for the current request, and then the CSRF access check can use that service to be *sure* about what is actually going on during this request and only fire if the session cookie is not only present but also actively being used to authenticate the user.

Also, core should stop adding _csrf_request_header_token requirements to all REST routes even if that resource doesn't even allow "cookie" as an authentication method. But fixing that still doesn't fix the use-case where both cookie and another authentication method are enabled on a resource.

---

The reason we can't trust "applies" and let every provider use that, is because just because it applies doesn't mean it's being used.

For example, hypothetical, bear with me: a site uses an OAuth provider and an "API key" provider.

The API key provider has the highest priority.

A request comes in, with both Bearer token and API key in a custom header. I know, but this is just for argument's sake.

API key wins, and authenticates the user.

The OAuth Provider, meanwhile, has added a _scopes requirement to all routes that can authenticate with OAuth, much like core adds a CSRF requirement. Its access checking class relies on the "applies" method of the authentication provider. Which returns true, as there is a Bearer token present. And it starts enforcing _scopes. Incorrectly, as OAuth was *not* actually used to authenticate the user during this request, even though "applies" returned true.

And we have the same situation as we do here. Now every provider with a higher priority than this theoretical OAuth provider has to decorate the OAuth access checking class and return AccessResult::allowed if their own provider also applies.

And we can keep going around in circles forever.

It's not a perfect example, of course, but y'all get the idea.

But it would all be fixed if we could know for sure which provider "won".

I think to solve this properly, we need a service which keeps track of which authentication provider was *actually* used for the current request, and then the CSRF access check can use that service to be *sure* about what is actually going on during this request and only fire if the session cookie is not only present but also actively being used to authenticate the user.

This makes sense. But could we instead of using a service add an attribute on the request object, which we could then retrieve during the access check?

Also, core should stop adding _csrf_request_header_token requirements to all REST routes even if that resource doesn't even allow "cookie" as an authentication method. But fixing that still doesn't fix the use-case where both cookie and another authentication method are enabled on a resource.

Yes, it does not make sense to add the requirement if cookie auth is not supported, but when we solve it in the access check, then this will have a low priority.

Patch #11 solves the issue for me when using JWT.

I also agree that we should probably determine that the current request was Authenticated by the Authorization header and not any other.

Patch #5 worked well for me thusfar.

This patch adds an extra condition to the CsrfRequestHeaderAccessCheck that gives us the ability to bypass the CSRF check if the request was made using a Bearer token.

Just starting building a partially decoupled React app for the first time and ran into this.

In my case, authenticated users can access the Drupal site normally through cookies, and other users log in exclusively through the React webapp using OAuth. This should definitely be fixed because I thought I was doing something wrong in React when it was actually this bug in core.

I'm not sure this patch is needed.

You can retrieve your csrf token from /session/token and add that value it into your header data (X-CSRF-Token)

Was able to apply the patch for 8.9.13.

But this didn't fix my problem, which occurs with feeds module and feeds extensible parsers
when trying to fetch data from json datasource which requires CSRF Tokens.
So is this also a Feeds module problem, or still core problem?

Any chance you're sending both OAuth credentials and a session cookie?

I was seeing this issue, and indeed, it was that Postman was automatically adding cookies (eg, a session) to the request, resulting in this error. Disabling the cookie jar in Postman resolves this (and/or manually removing the cookie that gets set.)

Simple OAuth maintainer here.

While this is not a Simple OAuth issue (Matteu was right to put it back in the core issue queue at #23 I think it is important to note that, in my opinion, applying patches such as #32 or #11 are insecure. Neither of these patches ensures that the Bearer token authentication was actually successful/used, an issue pointed out in #27, #24, et. al. While yes, it is unlikely that an attacker would set an Authorization header in a CSRF attack, headers are more easily added to such requests than cookies (due to designed protections in fetch and browsers.) In short, do not do this. Is the risk as significant as "straight" CSRF? No. But I can think of some clever ways to exploit this, particularly on top of an XSS vulnerability.

I came to this issue looking to do what a number of folks in this issue have already suggested - disabling the CSRF token check if authentication is successful using a Bearer token. If Drupal can in fact validate that the current request was authenticated via a Bearer token, then yes, this check can be safely disabled. This StackExchange post sums up the reasoning pretty well. This post also points out the issue with "quick" attempts to fix this like the patches I recommend against, above; if your application is somehow fooled into falling through to cookie auth, you've invalidated the CSRF check. That wouldn't happen with Simple OAuth (the token would be rejected and the request would not proceed) but we can't guarantee that's the OAuth solution in place.

I agree with much of the analysis in #26, particularly:

I think to solve this properly, we need a service which keeps track of which authentication provider was *actually* used for the current request, and then the CSRF access check can use that service to be *sure* about what is actually going on during this request and only fire if the session cookie is not only present but also actively being used to authenticate the user.

Finally, to the point raised about whether core modules should implement CSRF checks by default:

Also, core should stop adding _csrf_request_header_token requirements to all REST routes even if that resource doesn't even allow "cookie" as an authentication method. But fixing that still doesn't fix the use-case where both cookie and another authentication method are enabled on a resource.

That mostly becomes irrelevant if there is a sensible (perhaps even opt-in?) way to determine the winning authentication provider.

I'm working on this as part of a project so will be able to contribute some code to this soon.

This is similar in spirit to #2730351: CSRF check always fails for users without a session, where the CSRF check is conditional on whether there's a session or not.

Postponed on #3253911: Store the successful authentication plugin's ID on the request

Opening an MR and hiding the other patches for reasons explained above. Marking this NR to get some eyes on the approach, I'll make some comments on the MR during self-review on some things I need input on.

Should we also implement this on basic auth in core? That makes the change surface much larger and is probably a BC break and really edge-case security concern if someone is depending on CSRF with basic auth and... really means it.

This MR depends on #3253911: Store the successful authentication plugin's ID on the request

This will need a testing MR that incorporates the linked issue until/unless that gets merged first.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

A similar logic is happening on the CSRF header access check - it simply tests whether there's a session loaded (presumably) as the result of cookie usage.

https://git.drupalcode.org/issue/drupal-3343031/-/blob/175c46afabb6cfdb1...

I think the same criticism as I cited above regarding "...we need a service which keeps track of which authentication provider was *actually* used for the current request..." applies to that test, as well.

I agree with much of the analysis in #26, particularly:

I think to solve this properly, we need a service which keeps track of which authentication provider was *actually* used for the current request, and then the CSRF access check can use that service to be *sure* about what is actually going on during this request and only fire if the session cookie is not only present but also actively being used to authenticate the user.

I just would like to add that watch out with a solution like this, do not make the system more dependent on state as it is currently, because that is going to work against us to solve #2218651: [meta] Make Drupal compatible with persistent app servers like ReactPHP, PHP-PM, PHPFastCGI, FrankenPHP, Swoole

I just would like to add that watch out with a solution like this, do not make the system more dependent on state as it is currently

I get where you're going and have been active on that linked issue, however if the information is on the request, I think that's totally compatible with persistent servers. Authentication providers are authenticating a particular request, and so storing this information there should not poison anything else.

super! :)

and so storing this information there should not poison anything else.

+1

The key_auth module got the same problem. https://www.drupal.org/project/key_auth/issues/3174961