From: Tatu Lund <tatu@vaadin.com>
Date: Wed, 3 Feb 2021 14:52:08 +0000 (+0200)
Subject: fix: use time-constant comparison for security tokens (#12192)
X-Git-Tag: 7.7.24~2
X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=d0d2cfbda0f96b68293ce723bf776332d4ecd4de;p=vaadin-framework.git

fix: use time-constant comparison for security tokens (#12192)

This is the same as #12190, 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.

Backporting of #12189
---

diff --git a/server/src/main/java/com/vaadin/server/VaadinSession.java b/server/src/main/java/com/vaadin/server/VaadinSession.java
index 8bdf51683b..6537d84c73 100644
--- a/server/src/main/java/com/vaadin/server/VaadinSession.java
+++ b/server/src/main/java/com/vaadin/server/VaadinSession.java
@@ -747,6 +747,10 @@ public class VaadinSession implements HttpSessionBindingListener, Serializable {
 
     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();
 
     /**
diff --git a/server/src/main/java/com/vaadin/server/communication/FileUploadHandler.java b/server/src/main/java/com/vaadin/server/communication/FileUploadHandler.java
index 1209c562ac..d5dc16c6e7 100644
--- a/server/src/main/java/com/vaadin/server/communication/FileUploadHandler.java
+++ b/server/src/main/java/com/vaadin/server/communication/FileUploadHandler.java
@@ -23,6 +23,7 @@ import java.io.InputStream;
 import java.io.OutputStream;
 import java.io.OutputStreamWriter;
 import java.io.PrintWriter;
+import java.security.MessageDigest;
 
 import com.vaadin.server.ClientConnector;
 import com.vaadin.server.NoInputStreamException;
@@ -273,8 +274,10 @@ public class FileUploadHandler implements RequestHandler {
             streamVariable = uI.getConnectorTracker()
                     .getStreamVariable(connectorId, variableName);
             String secKey = uI.getConnectorTracker().getSeckey(streamVariable);
-            if (secKey == null || !secKey.equals(parts[3])) {
-                // TODO Should rethink error handling
+            String securityKey = parts[3];
+            if (secKey == null || !MessageDigest.isEqual(
+                    secKey.getBytes(UTF8),
+                    securityKey.getBytes(UTF8))) {
                 return true;
             }
 
diff --git a/server/src/main/java/com/vaadin/server/communication/PushHandler.java b/server/src/main/java/com/vaadin/server/communication/PushHandler.java
index 8c359b3d36..5bd7fd3232 100644
--- a/server/src/main/java/com/vaadin/server/communication/PushHandler.java
+++ b/server/src/main/java/com/vaadin/server/communication/PushHandler.java
@@ -18,6 +18,7 @@ package com.vaadin.server.communication;
 
 import java.io.IOException;
 import java.io.Reader;
+import java.security.MessageDigest;
 import java.util.Collection;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -57,6 +58,8 @@ public class PushHandler {
 
     private int longPollingSuspendTimeout = -1;
 
+    private static final String UTF8 = "UTF-8";
+
     /**
      * Callback interface used internally to process an event with the
      * corresponding UI properly locked.
@@ -471,7 +474,9 @@ public class PushHandler {
     }
 
     /**
-     * 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
@@ -480,10 +485,12 @@ public class PushHandler {
      * @return {@code true} if the id is valid, {@code false} otherwise
      */
     private static boolean isPushIdValid(VaadinSession session,
-            String requestPushId) {
+            String requestPushId) throws IOException {
 
         String sessionPushId = session.getPushId();
-        if (requestPushId == null || !requestPushId.equals(sessionPushId)) {
+        if (requestPushId == null || !MessageDigest.isEqual(
+                requestPushId.getBytes(UTF8),
+                sessionPushId.getBytes(UTF8))) {
             return false;
         }
         return true;
diff --git a/uitest/src/test/java/com/vaadin/tests/VerifyBrowserVersionTest.java b/uitest/src/test/java/com/vaadin/tests/VerifyBrowserVersionTest.java
index b044571ca3..ec3fb525a3 100644
--- a/uitest/src/test/java/com/vaadin/tests/VerifyBrowserVersionTest.java
+++ b/uitest/src/test/java/com/vaadin/tests/VerifyBrowserVersionTest.java
@@ -25,7 +25,7 @@ public class VerifyBrowserVersionTest extends MultiBrowserTest {
             // Chrome version does not necessarily match the desired version
             // because of auto updates...
             browserIdentifier = getExpectedUserAgentString(
-                    getDesiredCapabilities()) + "87";
+                    getDesiredCapabilities()) + "88";
         } else {
             browserIdentifier = getExpectedUserAgentString(desiredCapabilities)
                     + desiredCapabilities.getVersion();