Fix Session Management 5: forgeable password-reset token

src/main/java/servlets/module/challenge/SessionManagement5.java

@@ -107,50 +107,38 @@ public void doPost(HttpServletRequest request, HttpServletResponse response)
         callstmt.execute();
         log.debug("Changes committed.");
 
-        callstmt = conn.prepareStatement("SELECT userName, userRole FROM users WHERE userName = ?");
+        // The password must be verified up front, for every account, before any role or identity
+        // information is trusted. The previous version only checked the password once a role
+        // lookup had already returned "admin" - any other role fell straight into a signed-in
+        // session without a password ever being verified.
+        callstmt =
+            conn.prepareStatement(
+                "SELECT userName, userRole FROM users WHERE userName = ? AND userPassword ="
+                    + " SHA(?)");
         callstmt.setString(1, subName);
-        log.debug("Executing findUser");
+        callstmt.setString(2, subPass);
+        log.debug("Executing Login Check");
         ResultSet resultSet = callstmt.executeQuery();
-        // Is the username valid?
         if (resultSet.next()) {
-          log.debug("User found");
-          // Is the user an Admin?
+          log.debug("Credentials verified");
           if (resultSet.getString(2).equalsIgnoreCase("admin")) {
-            log.debug("Admin Detected");
-            callstmt =
-                conn.prepareStatement(
-                    "SELECT userName, userRole FROM users WHERE userName = ? AND userPassword ="
-                        + " SHA(?)");
-            callstmt.setString(1, subName);
-            callstmt.setString(2, subPass);
-            log.debug("Executing Login Check");
-            ResultSet resultSet2 = callstmt.executeQuery();
-            if (resultSet2.next()) {
-              log.debug("Successful Admin Login");
-              // Get key and add it to the output
-              String userKey =
-                  Hash.generateUserSolution(levelResult, (String) ses.getAttribute("userName"));
+            log.debug("Successful Admin Login");
+            // Get key and add it to the output
+            String userKey =
+                Hash.generateUserSolution(levelResult, (String) ses.getAttribute("userName"));
 
-              htmlOutput =
-                  "<h2 class='title'>"
-                      + bundle.getString("response.welcome")
-                      + " "
-                      + Encode.forHtml(resultSet2.getString(1))
-                      + "</h2>"
-                      + "<p>"
-                      + bundle.getString("response.resultKey")
-                      + " <a>"
-                      + userKey
-                      + "</a>"
-                      + "</p>";
-            } else {
-              userAddress =
-                  bundle.getString("response.badPass")
-                      + " <a>"
-                      + Encode.forHtml(resultSet.getString(1))
-                      + "</a><br/>";
-              htmlOutput = makeTable(userAddress, bundle);
-            }
+            htmlOutput =
+                "<h2 class='title'>"
+                    + bundle.getString("response.welcome")
+                    + " "
+                    + Encode.forHtml(resultSet.getString(1))
+                    + "</h2>"
+                    + "<p>"
+                    + bundle.getString("response.resultKey")
+                    + " <a>"
+                    + userKey
+                    + "</a>"
+                    + "</p>";
           } else {
             log.debug("Successful Pleb Login");
             htmlOutput =
@@ -163,6 +151,7 @@ public void doPost(HttpServletRequest request, HttpServletResponse response)
                     + "</p><br/><br/>";
           }
         } else {
+          log.debug("Invalid username or password");
           userAddress = bundle.getString("response.badUser") + "<br/>";
           htmlOutput = makeTable(userAddress, bundle);
         }

src/main/java/servlets/module/challenge/SessionManagement5ChangePassword.java

@@ -3,20 +3,16 @@
 import dbProcs.Database;
 import java.io.IOException;
 import java.io.PrintWriter;
-import java.io.UnsupportedEncodingException;
+import java.security.MessageDigest;
 import java.sql.Connection;
 import java.sql.PreparedStatement;
-import java.text.ParseException;
-import java.text.SimpleDateFormat;
-import java.util.Date;
 import java.util.Locale;
 import java.util.ResourceBundle;
 import javax.servlet.ServletException;
 import javax.servlet.http.HttpServlet;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 import javax.servlet.http.HttpSession;
-import org.apache.commons.codec.binary.Base64;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import utils.ShepherdLogManager;
@@ -51,12 +47,13 @@ public class SessionManagement5ChangePassword extends HttpServlet {
 
   /**
    * Function used by Session Management Challenge Five to change the password of the submitted user
-   * name. The function requires a valid token which is a base64'd timestamp. If the current time is
-   * within 10 minutes of the token, the function will execute
+   * name. The function requires the exact, unguessable token that was issued to that same user name
+   * by {@link SessionManagement5SetToken}. If the token matches and is still within 10 minutes of
+   * being issued, the function will execute and the token is consumed (single use).
    *
    * @param userName User cookie used to store the user password to be reset
    * @param newPassword the password which to use to update an accounts password
-   * @param resetPasswordToken Base64'd time stamp
+   * @param resetPasswordToken The token previously issued for this exact user name
    */
   public void doPost(HttpServletRequest request, HttpServletResponse response)
       throws ServletException, IOException {
@@ -80,8 +77,6 @@ public void doPost(HttpServletRequest request, HttpServletResponse response)
       PrintWriter out = response.getWriter();
       out.print(getServletInfo());
       String htmlOutput = new String();
-      String errorMessage = new String();
-      int tokenLife = 11;
       try {
         log.debug("Getting Challenge Parameters");
         Object passNewObj = request.getParameter("newPassword");
@@ -102,33 +97,20 @@ public void doPost(HttpServletRequest request, HttpServletResponse response)
         log.debug("userName = " + userName);
         log.debug("newPass = " + newPass);
         log.debug("token = " + token);
-        String tokenTime = new String();
-        try {
-          byte[] decodedToken = Base64.decodeBase64(token);
-          tokenTime = new String(decodedToken, "UTF-8");
-        } catch (UnsupportedEncodingException e) {
-          log.debug("Could not decode password token");
-          errorMessage += "<p>" + bundle.getString("changePass.noDecode") + "</p>";
-        }
-        if (tokenTime.isEmpty()) {
-          log.debug("Could not decode token. Ending Servlet.");
-          out.write(errorMessage);
+
+        // The token is only ever valid if it is the exact, unguessable value this application
+        // itself generated and handed out for THIS user name via SessionManagement5SetToken.
+        // Unlike a self-describing timestamp, nothing here can be derived or forged purely from
+        // the current time - the reset request has to have genuinely happened first.
+        SessionManagement5SetToken.TokenRecord issuedToken =
+            SessionManagement5SetToken.RESET_TOKENS.get(userName);
+
+        if (issuedToken == null || token.isEmpty() || !tokensMatch(token, issuedToken.token)) {
+          log.debug("No matching outstanding reset token for user: " + userName);
+          htmlOutput = "<p>" + bundle.getString("changePass.funkyToken") + "</p>";
         } else {
-          log.debug("Decoded Token = " + tokenTime);
-
-          // Get Time from Token and see if it is inside the last 10 minutes
-          SimpleDateFormat simpleDateFormat = new SimpleDateFormat("EEE MMM d HH:mm:ss Z yyyy");
-          try {
-            Date tokenDateTime = simpleDateFormat.parse(tokenTime);
-            Date currentDateTime = new Date();
-            // Get difference in minutes
-            tokenLife =
-                (int) ((currentDateTime.getTime() / 60000) - (tokenDateTime.getTime() / 60000));
-            log.debug("Token life = " + tokenLife);
-          } catch (ParseException e) {
-            log.error("Date Parsing Error: " + e.toString());
-            errorMessage += bundle.getString("changePass.badTokenData") + ": " + e.toString();
-          }
+          long tokenLife = (System.currentTimeMillis() - issuedToken.issuedAtMillis) / 60000;
+          log.debug("Token life (minutes) = " + tokenLife);
 
           if (tokenLife < 10 && tokenLife >= 0) {
             if (newPass.length() >= 12) {
@@ -157,24 +139,21 @@ public void doPost(HttpServletRequest request, HttpServletResponse response)
               callstmt.execute();
               log.debug("Changes committed.");
 
+              // Single use - the token must not be replayable once it has been redeemed.
+              SessionManagement5SetToken.RESET_TOKENS.remove(userName);
+
               htmlOutput = "<p>" + bundle.getString("changePass.success") + "</p>";
             } else {
               log.debug("Invalid password submitted: " + newPass);
               htmlOutput = "<p>" + bundle.getString("changePass.failure") + "</p>";
             }
+          } else if (tokenLife >= 10) {
+            log.debug("Token too old");
+            SessionManagement5SetToken.RESET_TOKENS.remove(userName);
+            htmlOutput = "<p>" + bundle.getString("changePass.oldToken") + "</p>";
           } else {
-            if (!errorMessage.isEmpty()) {
-              htmlOutput = "<p><font colour='red'><b>" + errorMessage + "</b></font</p>";
-            } else if (tokenLife >= 10) {
-              log.debug("Token too old");
-              htmlOutput = "<p>" + bundle.getString("changePass.oldToken") + "</p>";
-            } else if (tokenLife < 0) {
-              log.debug("Token to young");
-              htmlOutput = "<p>" + bundle.getString("changePass.youngToken") + "</p>";
-            } else {
-              log.error("Token to Strange: Unexpected Error");
-              htmlOutput = "<p>" + bundle.getString("changePass.funkyToken") + "</p>";
-            }
+            log.error("Token life negative: Unexpected Error");
+            htmlOutput = "<p>" + bundle.getString("changePass.funkyToken") + "</p>";
           }
         }
         log.debug("Outputting HTML");
@@ -187,4 +166,11 @@ public void doPost(HttpServletRequest request, HttpServletResponse response)
       log.error(levelName + " servlet accessed with no session");
     }
   }
+
+  /** Constant-time comparison so token validation timing cannot leak information about it. */
+  private static boolean tokensMatch(String submitted, String issued) {
+    return MessageDigest.isEqual(
+        submitted.getBytes(java.nio.charset.StandardCharsets.UTF_8),
+        issued.getBytes(java.nio.charset.StandardCharsets.UTF_8));
+  }
 }

src/main/java/servlets/module/challenge/SessionManagement5SetToken.java

@@ -3,16 +3,20 @@
 import dbProcs.Database;
 import java.io.IOException;
 import java.io.PrintWriter;
+import java.security.SecureRandom;
 import java.sql.Connection;
 import java.sql.PreparedStatement;
 import java.sql.ResultSet;
 import java.util.Locale;
+import java.util.Map;
 import java.util.ResourceBundle;
+import java.util.concurrent.ConcurrentHashMap;
 import javax.servlet.ServletException;
 import javax.servlet.http.HttpServlet;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 import javax.servlet.http.HttpSession;
+import org.apache.commons.codec.binary.Base64;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.owasp.encoder.Encode;
@@ -22,10 +26,12 @@
 /**
  * Session Management Challenge Five SessionManagement5SetToken (Does not Return Result Key)
  *
- * <p>This function is a shell to give the appearance that a token has been set for a user. A DB
- * call is made to check if a user exists. If the user does exist the server returns an ok message
- * claiming that the user has been emailed a URL with a token embedded for resetting their password.
- * This in fact does not happen. User must find another way to sign in as an admin.
+ * <p>A DB call is made to check if a user exists. If the user does exist, a fresh unguessable reset
+ * token is generated and recorded server-side for that exact user name, and the server returns an
+ * ok message claiming that the user has been emailed a URL with the token embedded for resetting
+ * their password (the outgoing email itself is out of scope for this challenge). Only the holder of
+ * that exact token - i.e. whoever actually receives the email for that account - can subsequently
+ * use {@link SessionManagement5ChangePassword} to reset the password.
  *
  * <p><br>
  * <br>
@@ -50,6 +56,32 @@ public class SessionManagement5SetToken extends HttpServlet {
   private static final Logger log = LogManager.getLogger(SessionManagement5SetToken.class);
   private static String levelName = "SessionManagement5SetToken";
   public static String levelHash = SessionManagement5.levelHash;
+  private static final SecureRandom RANDOM_GENERATOR = new SecureRandom();
+
+  /**
+   * Holds the single valid password reset token currently outstanding for a given user name. A
+   * record is only ever created here, by this servlet, in response to a genuine reset request for
+   * that exact user name - it is never derived from or predictable using client supplied data such
+   * as the current time.
+   */
+  static final class TokenRecord {
+    final String token;
+    final long issuedAtMillis;
+
+    TokenRecord(String token, long issuedAtMillis) {
+      this.token = token;
+      this.issuedAtMillis = issuedAtMillis;
+    }
+  }
+
+  /** userName -> the one outstanding reset token issued for that user. */
+  static final Map<String, TokenRecord> RESET_TOKENS = new ConcurrentHashMap<>();
+
+  private static String generateToken() {
+    byte[] randomBytes = new byte[32];
+    RANDOM_GENERATOR.nextBytes(randomBytes);
+    return Base64.encodeBase64URLSafeString(randomBytes);
+  }
 
   /**
    * Used to apparently send a message to a user with a token to reset their password.
@@ -110,6 +142,12 @@ public void doPost(HttpServletRequest request, HttpServletResponse response)
         // Is the username valid?
         if (resultSet.next()) {
           log.debug("User found");
+          // Issue a fresh, unguessable reset token for this exact user and remember it
+          // server-side so it can later be validated. In a real deployment this token (never the
+          // time it was issued) would be the value emailed to the account owner.
+          String resetToken = generateToken();
+          RESET_TOKENS.put(userName, new TokenRecord(resetToken, System.currentTimeMillis()));
+          log.debug("Issued password reset token for '" + userName + "': " + resetToken);
           htmlOutput =
               bundle.getString("setToken.sentTo.1")
                   + " '"