Skip to content

Commit

Permalink
fix: use time-constant comparison for CSRF tokens (#9875)
Browse files Browse the repository at this point in the history 
This hardens the framework against a theoretical timing attack based on
comparing how quickly a request with an invalid CSRF token is rejected.
  • Loading branch information
@Legioth @caalador
Legioth authored and caalador committed Feb 3, 2021
1 parent ff5381c commit a7ff693
Showing 1 changed file with 5 additions and 1 deletion.
6 changes: 5 additions & 1 deletion flow-server/src/main/java/com/vaadin/flow/server/VaadinService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
import java.io.Serializable;
import java.lang.reflect.Constructor;
import java.net.URL;
import java.nio.charset.StandardCharsets;
import java.security.MessageDigest;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
Expand Down Expand Up @@ -1850,7 +1852,9 @@ public static boolean isCsrfTokenValid(VaadinSession session,
.isXsrfProtectionEnabled()) {
String sessionToken = session.getCsrfToken();

if (sessionToken == null || !sessionToken.equals(requestToken)) {
if (uiToken == null || !MessageDigest.isEqual(
uiToken.getBytes(StandardCharsets.UTF_8),
requestToken.getBytes(StandardCharsets.UTF_8))) {
return false;
}
}
Expand Down

2 comments on commit a7ff693

@adv851
Copy link

@adv851 adv851 commented on a7ff693 Mar 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dear developers,
I found that the patch introduced a variable "uiToken" that does not exist in the context, causing the version to fail to compile when used. This may be an error accidentally introduced when cherry picking other branches, and here is a patch change suggestion:

    public static boolean isCsrfTokenValid(VaadinSession session,
            String requestToken) {

        if (session.getService().getDeploymentConfiguration()
                .isXsrfProtectionEnabled()) {
            String sessionToken = session.getCsrfToken();
-            if (sessionToken == null || !sessionToken.equals(requestToken)) {
+            if (sessionToken == null || !MessageDigest.isEqual(
+                    sessionToken.getBytes(StandardCharsets.UTF_8),
+                    requestToken.getBytes(StandardCharsets.UTF_8))) {
                return false;
            }
        }
        return true;
    }

@Legioth
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That problem was subsequently fixed in 674425b

Please sign in to comment.