CSRF validation fails because CSRFHandler updates the session AFTER the session is already flushed

[haizz]

Version

4.5.7

Context

Root cause seems to be in #2500 (#2447, #2460)

CSRFHandlerImpl updates the session with End Handler:

private String generateToken(RoutingContext ctx) {
    // ...
    ctx.addEndHandler(sessionTokenEndHandler(ctx, token));
    // ...
}

But SessionHandlerImpl flushes the session with HeadersEnd Handler

private void addStoreSessionHandler(RoutingContext context) {
    context.addHeadersEndHandler(v -> {
      // skip flush if we already flushed
      Boolean flushed = context.get(SESSION_FLUSHED_KEY);
      if (flushed == null || !flushed) {
        flush(context, true, false)
          .onFailure(err -> LOG.warn("Failed to flush the session to the underlying store", err));
      }
    });
  }

So SessionHandlerImpl flushes the session on headers end, and then CSRFHandlerImpl updates the already flushed session, and no changes end up being saved in the session store.

Tests work because LocalSessionStore store raw session objects in the map. So when you update the session object, no flush is needed: next time you retrieve updated session object from the store. It won't work when sessions are serialized/deserialized in the store.

[tsegismont]

Thanks for reporting this, we'll look into it

[tsegismont]

I would like to understand why Vert.x uses the headers end signal to trigger session flushing. I looked into two other Java libraries and they use the completion of the HTTP server response:

Wildfly: https://github.com/wildfly/wildfly/blob/e8be2f8e321bca0b5309e9c929c221a9d8f10d98/clustering/web/undertow/src/main/java/org/wildfly/clustering/web/undertow/session/DistributableSession.java#L83

Spring: https://github.com/spring-projects/spring-session/blob/852efed86ce594bb1a36a2dc81372bcf1c5a9e4a/spring-session-core/src/main/java/org/springframework/session/web/http/SessionRepositoryFilter.java#L179

@pmlopes @vietj do you know why Vert.x behaves differently? I searched the git history and it has been like this since the original commit in this repository.

If there is no particular reason, perhaps we could move change the trigger. Beyond fixing this issue, I believe there are good reasons why a user may want to store data in the session as they generate the content (if the response is chunked).

[tsegismont]

For the record, @vietj can't think about a particular reason why Vert.x uses the headers end handler as a trigger for flushing the session.

In the meantime, I did some research about CSRF (admittedly not my area of expertise) and mitigation as well as the reason why #2447 was solved with #2500

CSRF and mitigation

OWASP describes CSRF attacks as well as how to prevent them.

Vert.x implements the Signed Double-Submit Cookie pattern, with a per-session token.

Issue with failed responses

#2447 was filed to describe the case of a "trapped" client that happens when a POST (or PUT... etc) request does not complete successfully:

• the client sends a request to the Vert.x with a valid token
• the server validates the token and generates a new one, then put it in the user session
• the server sends response headers, which triggers session flushing
• a server failure prevents to send the response (no data sent)

In this case, the session has a new token that is never sent to the user, because GET requests do not send a cookie with the new value. Therefore, the client cannot send any POST requests again, until the session is recreated (e.g. logout/login).

The solution implemented in #2500, as explained by @haizz above, was to make the CSRFHandler update the session only when Vert.x has successfully sent the response to the wire. But, since the session has been flushed at a previous stage, this only works when using a single server or sticky sessions, not clustered sessions.

Also, it does not prevent the client from being "trapped" if an intermediate proxy fails to send the response to the client.

Proposal

Considering the points above, changing the session persistence trigger seems like descending into the rabbit hole to me.

Failures will happen, and when they do:

• the client should send a GET request
• the CSRFHandler should send a cookie with the valid token currently stored in the session

So I will send a PR asap that does this and reverts the modification introduced in #2500 (i.e. store new tokens in the session immediately instead of waiting for the response to be sent)

It's not necessary to change the session flush trigger.

cc @vietj @pmlopes @chrispatmore

[tsegismont]

Fixed by 5b0008b5d