Skip to content

Commit

Permalink
fix: use time-constant comparison for security tokens (#9896)
Browse files Browse the repository at this point in the history
This is the same as #9875, 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.
  • Loading branch information
Legioth authored and tanbt committed Feb 3, 2021
1 parent dc7f6e0 commit acc7ea4
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@

package com.vaadin.flow.server;

import javax.servlet.http.HttpSession;
import javax.servlet.http.HttpSessionBindingEvent;
import javax.servlet.http.HttpSessionBindingListener;

import java.io.IOException;
import java.io.ObjectInputStream;
import java.io.Serializable;
Expand All @@ -36,10 +40,6 @@
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;

import javax.servlet.http.HttpSession;
import javax.servlet.http.HttpSessionBindingEvent;
import javax.servlet.http.HttpSessionBindingListener;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -128,6 +128,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();

private final Attributes attributes = new Attributes();
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 org.atmosphere.cpr.AtmosphereRequest;
Expand Down Expand Up @@ -451,7 +453,9 @@ private static 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 @@ -463,7 +467,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
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@
import java.io.Serializable;

import javax.servlet.http.HttpServletRequest;
import java.nio.charset.StandardCharsets;
import java.security.MessageDigest;
import java.util.Collection;
import java.util.Iterator;

import org.apache.commons.fileupload.FileItemIterator;
import org.apache.commons.fileupload.FileItemStream;
Expand Down Expand Up @@ -112,7 +116,9 @@ public void handleRequest(VaadinSession session, VaadinRequest request,
session.lock();
try {
String secKey = streamReceiver.getId();
if (secKey == null || !secKey.equals(securityKey)) {
if (secKey == null || !MessageDigest.isEqual(
secKey.getBytes(StandardCharsets.UTF_8),
securityKey.getBytes(StandardCharsets.UTF_8))) {
getLogger().warn(
"Received incoming stream with faulty security key.");
return;
Expand Down

0 comments on commit acc7ea4

Please sign in to comment.