CSRF Handler can "trap" users

[chrispatmore]

Version

4.3.8 -> 4.4.1

Context

Whilst using a web application served using vertx with vertx auth and the vertx CSRF handler on the backend APIs, I managed to get into a situation where I could no longer make successful (non GET) requests to my APIs. What had happened is:

• Post request to the API
• Post request times out (never responds) in this case was code bug but could be network issue or random failure
• CSRF token is considered "used" by the CSRF handler but as request that used it never returned the new token was never returned.
• The only way to unstick this and get new working token into the browser is to delete the session and token cookie (i.e. logout and log in again)

Do you have a reproducer?

Here is a gist that shows the behaviour: https://gist.github.com/chrispatmore/89c5c7c35df4321a62699eb1eaf4c791

If you run that and use get and postWorking buttons a bit to show its working. then use postBroken which doesn't respond to deliberately cause the issue. You will find that postWorking is now unable to work again until clearing the session token. (removing CSRF token alone doesn't work)

Steps to reproduce

1. set up a Vertx server
2. set up a session
3. add some post and get APIs behind CSRF handler
4. Make one of the POST APIs not respond
5. add some browser buttons to trigger APIs
6. load up browser and interact with buttons
7. interact with broken POST API
8. now all POST APIs are broken

Extra

There are quite a few options to fix this of varying levels of user involvement:

1. update vertx code to send back current token on get requests if not sent in
   1. i.e. on GET with no csrf token in cookies but csrf is in session, get token from session and add to response cookies. (This would require user to manually delete the csrf cookie and refresh the page to fix)
   2. i.e. on GET with no or old token in cookie and csrf in session, get token from session and add to response cookies. (This would require a user refresh or other GET trigger)
2. update vertx code to send back current token on NON GET when receiving an old token
   1. i.e. on POST with old csrf token, get current token and add to response cookies (This would require user to retry the action)
3. Allow multiple uses of the token before considering it used
   1. i.e. this token can be used 5 times, this would reduce the chances of getting stuck to random error or networking
   2. i.e. this token can be used for 10 mins, again reduces the chances of getting stuck to random error or networking (This has the added benefit off allowing the UI to do simultaneous / async posts if they require that)

The differences here (other than user interaction) are whether any of these reduce the security of the CSRF Handler as secure. i.e. is it risky to send back a valid token to an invalid request (2.i)

As I understand it, the CSRF token's job is to mitigate against pre-programmed requests against a server in other sites / email links / images etc. by requiring a header (which aren't sent in some of these scenarios) Or needing a value you couldn't pre program. I am not sure if sending back a valid token in some scenarios would invalidate this defence

[tsegismont]

Hi @chrispatmore

This is the wrong repository, this issue should have been reported to vertx-web.

Anyway, it seems ok to me if you can't make non-GET requests until you get a new token, that's the purpose of the CSRF token. If you make a GET request though, it should be sent as a response header, even after a POST request which was never replied.

Perhaps what you want is that the cookie is updated if does not contain the refreshed token?

Would that be fine regarding security @pmlopes ?

[chrispatmore]

Oops sorry, do you want me to move it?

Yea GET requests updating the token would be fine, as it would enable the user to carry on without logging in again

[tsegismont]

Oops sorry, do you want me to move it?

Yes please

[pmlopes]

CSRF is a mitigation against replay attacks, hence the issue that a request that never finishes, blocks the whole sequence of future requests. The way I see it is that:

https://github.com/vert-x3/vertx-web/blob/d64ec59431aa8d613aa4fbd869466f59b841c1a9/vertx-web/src/main/java/io/vertx/ext/web/handler/impl/CSRFHandlerImpl.java#L130-L132C7

and:

https://github.com/vert-x3/vertx-web/blob/d64ec59431aa8d613aa4fbd869466f59b841c1a9/vertx-web/src/main/java/io/vertx/ext/web/handler/impl/CSRFHandlerImpl.java#L250

Should only happen on a successful response:

https://github.com/vert-x3/vertx-web/blob/d64ec59431aa8d613aa4fbd869466f59b841c1a9/vertx-web/src/main/java/io/vertx/ext/web/RoutingContext.java#L314

By wrapping that code only in case of success. This should address the cases:

1. a never-ending response, will not rotate the token, (this example) so the next call will succeed
2. a call that fails for some reason (say internal server error) will preserve the state

Tests need to be run to verify if this works as expected.

[tsegismont]

Thanks @pmlopes !

So @chrispatmore , if you can provide a test for Vert.x Web that shows 1/ and /2 don't work, please open an issue there.

[chrispatmore]

Sorry to be clear, you would like me to:

move this issue to web, then work on this issue using suggestion from @pmlopes ?

[tsegismont]

Sorry to be clear, you would like me to:

move this issue to web, then work on this issue using suggestion from @pmlopes ?

yes please