Skip to content

Commit

Permalink
fix: use time-constant comparison for security tokens (#12189)
Browse files Browse the repository at this point in the history
This is the same as #12188,
but also applied for the upload security key
and the push id since both of those are also used to protect against
cross-site attacks. In addition, documentation for the push id is
clarified to point out its role.

Cherry-picked from: vaadin/flow#9896
  • Loading branch information
TatuLund authored and Ansku committed Feb 3, 2021
1 parent 3e9873b commit cce86cc
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 3 deletions.
4 changes: 4 additions & 0 deletions server/src/main/java/com/vaadin/server/VaadinSession.java
Original file line number Diff line number Diff line change
Expand Up @@ -743,6 +743,10 @@ public Collection<UI> getUIs() {
*/
private final String csrfToken = UUID.randomUUID().toString();

/*
* This token should be handled with care since it's used to protect against
* cross-site attacks in addition to general identifier duty.
*/
private final String pushId = UUID.randomUUID().toString();

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
import java.io.OutputStream;
import java.io.OutputStreamWriter;
import java.io.PrintWriter;
import java.nio.charset.StandardCharsets;
import java.security.MessageDigest;

import com.vaadin.server.ClientConnector;
import com.vaadin.server.NoInputStreamException;
Expand Down Expand Up @@ -279,7 +281,10 @@ public boolean handleRequest(VaadinSession session, VaadinRequest request,
streamVariable = uI.getConnectorTracker()
.getStreamVariable(connectorId, variableName);
String secKey = uI.getConnectorTracker().getSeckey(streamVariable);
if (secKey == null || !secKey.equals(parts[3])) {
String securityKey = parts[3];
if (secKey == null || !MessageDigest.isEqual(
secKey.getBytes(StandardCharsets.UTF_8),
securityKey.getBytes(StandardCharsets.UTF_8))) {
return true;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@

import java.io.IOException;
import java.io.Reader;
import java.nio.charset.StandardCharsets;
import java.security.MessageDigest;
import java.util.Collection;
import java.util.logging.Level;
import java.util.logging.Logger;
Expand Down Expand Up @@ -510,7 +512,9 @@ private static final Logger getLogger() {
}

/**
* Checks whether a given push id matches the session's push id.
* Checks whether a given push id matches the session's push id. The
* comparison is done using a time-constant method since the push id is used
* to protect against cross-site attacks.
*
* @param session
* the vaadin session for which the check should be done
Expand All @@ -522,7 +526,9 @@ private static boolean isPushIdValid(VaadinSession session,
String requestPushId) {

String sessionPushId = session.getPushId();
if (requestPushId == null || !requestPushId.equals(sessionPushId)) {
if (requestPushId == null || !MessageDigest.isEqual(
requestPushId.getBytes(StandardCharsets.UTF_8),
sessionPushId.getBytes(StandardCharsets.UTF_8))) {
return false;
}
return true;
Expand Down

0 comments on commit cce86cc

Please sign in to comment.