diff --git a/src/main/java/servlets/module/challenge/CsrfChallengeSixGetToken.java b/src/main/java/servlets/module/challenge/CsrfChallengeSixGetToken.java index cb61a3d85..716e52d35 100644 --- a/src/main/java/servlets/module/challenge/CsrfChallengeSixGetToken.java +++ b/src/main/java/servlets/module/challenge/CsrfChallengeSixGetToken.java @@ -69,7 +69,12 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) if (Validate.validateSession(ses)) { log.debug(levelName + " servlet accessed by: " + ses.getAttribute("userName").toString()); String htmlOutput = new String("Your csrf Token for this Challenge is: "); - String userId = request.getParameter("userId").toString(); + // The userId must be the caller's own - taking it from a request parameter let any + // authenticated user fetch any other user's real CSRF token directly, and the LIKE + // match (with wildcard metacharacters left unescaped) made it worse by allowing broad + // token disclosure. This defeats the point of the token being unguessable, since the + // whole defense relies on an attacker being unable to learn the victim's value. + String userId = (String) ses.getAttribute("userStamp"); Connection conn = Database.getChallengeConnection( @@ -78,7 +83,7 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) log.debug("Preparing setCsrfChallengeSixToken call"); PreparedStatement callstmnt = conn.prepareStatement( - "SELECT csrfTokenscol FROM csrfchallengesix.csrfTokens WHERE userId LIKE ?"); + "SELECT csrfTokenscol FROM csrfchallengesix.csrfTokens WHERE userId = ?"); callstmnt.setString(1, userId); log.debug("Executing setCsrfChallengeSixTokenQuery"); ResultSet rs = callstmnt.executeQuery(); diff --git a/src/main/java/servlets/module/challenge/CsrfChallengeTargetSix.java b/src/main/java/servlets/module/challenge/CsrfChallengeTargetSix.java index 684fd90b2..d328fa901 100644 --- a/src/main/java/servlets/module/challenge/CsrfChallengeTargetSix.java +++ b/src/main/java/servlets/module/challenge/CsrfChallengeTargetSix.java @@ -5,7 +5,6 @@ import java.io.IOException; import java.io.PrintWriter; import java.util.Locale; -import java.util.Random; import java.util.ResourceBundle; import javax.servlet.ServletException; import javax.servlet.http.HttpServlet; @@ -14,6 +13,7 @@ import javax.servlet.http.HttpSession; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import utils.Hash; import utils.ShepherdLogManager; import utils.Validate; @@ -42,11 +42,6 @@ public class CsrfChallengeTargetSix extends HttpServlet { private static final long serialVersionUID = 1L; private static String moduleHash = "2fff41105149e507c75b5a54e558470469d7024929cf78d570cd16c03bee3569"; - private static final String[] csrfArray = { - "c4ca4238a0b923820dcc509a6f75849b", - "c81e728d9d4c2f636f067f89cc14862c", - "eccbc87e4b5ce2fe28308fd9f2a7baf3" - }; private static final Logger log = LogManager.getLogger(CsrfChallengeTargetSix.class); private static String levelName = "CSRF 6 Target"; @@ -85,9 +80,11 @@ public void doPost(HttpServletRequest request, HttpServletResponse response) if (ses.getAttribute(csrfTokenName) == null || ses.getAttribute(csrfTokenName).toString().isEmpty()) { log.debug("No CSRF Token associated with user"); - Random random = new Random(); - int newToken = random.nextInt(3); - storedToken = csrfArray[newToken]; + // random.nextInt(3) as an index into a 3-element array only ever produces one of 3 + // possible values, letting an attacker brute-force a valid nonce in at most 3 tries + // without ever touching the victim's session. Hash.randomString() matches the + // real-random pattern used elsewhere. + storedToken = Hash.randomString(); out.write( csrfGenerics.getString("target.noTokenNewToken") + " " + storedToken + "

"); ses.setAttribute(csrfTokenName, storedToken);