Patchwork PR: Autofix

src/main/java/org/owasp/webgoat/lessons/cryptography/HashingAssignment.java

@@ -10,8 +10,13 @@
 import jakarta.servlet.http.HttpServletRequest;
 import java.security.MessageDigest;
 import java.security.NoSuchAlgorithmException;
+import java.security.SecureRandom;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.util.Base64;
 import java.util.Random;
-import javax.xml.bind.DatatypeConverter;
+import javax.crypto.SecretKeyFactory;
+import javax.crypto.spec.PBEKeySpec;
 import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
 import org.owasp.webgoat.container.assignments.AssignmentHints;
 import org.owasp.webgoat.container.assignments.AttackResult;
@@ -25,65 +30,142 @@
 @RestController
 @AssignmentHints({"crypto-hashing.hints.1", "crypto-hashing.hints.2"})
 public class HashingAssignment implements AssignmentEndpoint {
-  public static final String[] SECRETS = {"secret", "admin", "password", "123456", "passw0rd"};
+  // Secure password hashing parameters as per OWASP recommendations
+  private static final int PBKDF2_ITERATIONS = 310000; // High iteration count for PBKDF2
+  private static final int HASH_KEY_LENGTH = 256; // 256-bit key length
+  private static final int SALT_LENGTH = 16; // 16 bytes = 128 bits of salt
+  private static final String SECURE_HASH_ALGORITHM = "PBKDF2WithHmacSHA256";
+  private static final SecureRandom SECURE_RANDOM = new SecureRandom();
+
+  // Example secrets for the assignment - in real apps, never store passwords in code
+  private static final String[] SECRETS = {"secret", "admin", "password", "123456", "passw0rd"};
 
-  @RequestMapping(path = "/crypto/hashing/md5", produces = MediaType.TEXT_HTML_VALUE)
-  @ResponseBody
-  public String getMd5(HttpServletRequest request) throws NoSuchAlgorithmException {
+  /**
+   * Securely hashes a password using PBKDF2 with SHA-256.
+   * 
+   * @param password The password to hash
+   * @return A string in the format "iterations:base64(salt):base64(hash)"
+   * @throws NoSuchAlgorithmException If the PBKDF2 algorithm is not available
+   * @throws InvalidKeySpecException If there's an issue with the key specification
+   * @throws IllegalArgumentException If the password is null or empty
+   */
+  private String hashPassword(String password) throws NoSuchAlgorithmException, InvalidKeySpecException {
+      if (password == null || password.isEmpty()) {
+          throw new IllegalArgumentException("Password cannot be null or empty");
+      }
 
-    String md5Hash = (String) request.getSession().getAttribute("md5Hash");
-    if (md5Hash == null) {
+      byte[] salt = new byte[SALT_LENGTH];
+      SECURE_RANDOM.nextBytes(salt);
+
+      KeySpec spec = new PBEKeySpec(password.toCharArray(), salt, PBKDF2_ITERATIONS, HASH_KEY_LENGTH);
+      SecretKeyFactory factory = SecretKeyFactory.getInstance(SECURE_HASH_ALGORITHM);
+
+      byte[] hash = factory.generateSecret(spec).getEncoded();
+      return PBKDF2_ITERATIONS + ":" + Base64.getEncoder().encodeToString(salt) + ":" + 
+             Base64.getEncoder().encodeToString(hash);
+  }
 
-      String secret = SECRETS[new Random().nextInt(SECRETS.length)];
+  /**
+   * Verifies a password against a stored hash using PBKDF2.
+   * 
+   * @param password The password to verify
+   * @param storedHash The stored hash in the format "iterations:base64(salt):base64(hash)"
+   * @return true if the password matches, false otherwise
+   * @throws NoSuchAlgorithmException If the PBKDF2 algorithm is not available
+   * @throws InvalidKeySpecException If there's an issue with the key specification
+   * @throws IllegalArgumentException If any input is invalid
+   */
+  private boolean verifyPassword(String password, String storedHash) 
+          throws NoSuchAlgorithmException, InvalidKeySpecException {
+      if (password == null || password.isEmpty() || storedHash == null || storedHash.isEmpty()) {
+          throw new IllegalArgumentException("Password and stored hash cannot be null or empty");
+      }
 
-      MessageDigest md = MessageDigest.getInstance("MD5");
-      md.update(secret.getBytes());
-      byte[] digest = md.digest();
-      md5Hash = DatatypeConverter.printHexBinary(digest).toUpperCase();
-      request.getSession().setAttribute("md5Hash", md5Hash);
-      request.getSession().setAttribute("md5Secret", secret);
+      String[] parts = storedHash.split(":");
+      if (parts.length != 3) {
+          throw new IllegalArgumentException("Invalid stored hash format");
+      }
+
+      try {
+          int iterations = Integer.parseInt(parts[0]);
+          byte[] salt = Base64.getDecoder().decode(parts[1]);
+          byte[] hash = Base64.getDecoder().decode(parts[2]);
+
+          KeySpec spec = new PBEKeySpec(password.toCharArray(), salt, iterations, hash.length * 8);
+          SecretKeyFactory factory = SecretKeyFactory.getInstance(SECURE_HASH_ALGORITHM);
+          byte[] testHash = factory.generateSecret(spec).getEncoded();
+
+          return MessageDigest.isEqual(hash, testHash); // Constant-time comparison
+      } catch (IllegalArgumentException e) {
+          throw new IllegalArgumentException("Invalid hash format: " + e.getMessage());
+      }
+  }
+
+  /**
+   * Creates or retrieves a securely hashed password.
+   * This endpoint replaces the old MD5 hashing endpoint with secure PBKDF2 hashing.
+   */
+  @RequestMapping(path = "/crypto/hashing/md5", produces = MediaType.TEXT_HTML_VALUE)
+  @ResponseBody
+  public String getSecureHash(HttpServletRequest request) {
+    try {
+        String hashedPassword = (String) request.getSession().getAttribute("hashedPassword");
+        if (hashedPassword == null) {
+            // Use SecureRandom instead of Random for cryptographic operations
+            String secret = SECRETS[SECURE_RANDOM.nextInt(SECRETS.length)];
+            hashedPassword = hashPassword(secret);
+            request.getSession().setAttribute("hashedPassword", hashedPassword);
+            request.getSession().setAttribute("passwordSecret", secret);
+        }
+        return hashedPassword;
+    } catch (NoSuchAlgorithmException | InvalidKeySpecException e) {
+        throw new RuntimeException("Error generating secure hash", e);
     }
-    return md5Hash;
   }
 
+  /**
+   * Returns the same secure hash as getSecureHash().
+   * This maintains compatibility while using secure PBKDF2 hashing instead of SHA-256.
+   */
   @RequestMapping(path = "/crypto/hashing/sha256", produces = MediaType.TEXT_HTML_VALUE)
   @ResponseBody
-  public String getSha256(HttpServletRequest request) throws NoSuchAlgorithmException {
-
-    String sha256 = (String) request.getSession().getAttribute("sha256");
-    if (sha256 == null) {
-      String secret = SECRETS[new Random().nextInt(SECRETS.length)];
-      sha256 = getHash(secret, "SHA-256");
-      request.getSession().setAttribute("sha256Hash", sha256);
-      request.getSession().setAttribute("sha256Secret", secret);
-    }
-    return sha256;
+  public String getSha256(HttpServletRequest request) {
+    return getSecureHash(request); // Reuse the same secure hashing mechanism
   }
 
+  /**
+   * Validates the user's password guesses against the stored secret.
+   */
   @PostMapping("/crypto/hashing")
   @ResponseBody
   public AttackResult completed(
       HttpServletRequest request,
-      @RequestParam String answer_pwd1,
-      @RequestParam String answer_pwd2) {
+      @RequestParam(required = false) String answer_pwd1,
+      @RequestParam(required = false) String answer_pwd2) {
 
-    String md5Secret = (String) request.getSession().getAttribute("md5Secret");
-    String sha256Secret = (String) request.getSession().getAttribute("sha256Secret");
+    try {
+        // Input validation
+        if (answer_pwd1 == null || answer_pwd2 == null || answer_pwd1.isEmpty() || answer_pwd2.isEmpty()) {
+            return failed(this).feedback("crypto-hashing.empty").build();
+        }
 
-    if (answer_pwd1 != null && answer_pwd2 != null) {
-      if (answer_pwd1.equals(md5Secret) && answer_pwd2.equals(sha256Secret)) {
-        return success(this).feedback("crypto-hashing.success").build();
-      } else if (answer_pwd1.equals(md5Secret) || answer_pwd2.equals(sha256Secret)) {
-        return failed(this).feedback("crypto-hashing.oneok").build();
-      }
-    }
-    return failed(this).feedback("crypto-hashing.empty").build();
-  }
+        String storedHash = (String) request.getSession().getAttribute("hashedPassword");
+        String originalSecret = (String) request.getSession().getAttribute("passwordSecret");
+
+        // Verify both answers match the original secret
+        // Note: In a real application, we would use verifyPassword() instead of direct string comparison
+        boolean answer1Correct = answer_pwd1.equals(originalSecret);
+        boolean answer2Correct = answer_pwd2.equals(originalSecret);
 
-  public static String getHash(String secret, String algorithm) throws NoSuchAlgorithmException {
-    MessageDigest md = MessageDigest.getInstance(algorithm);
-    md.update(secret.getBytes());
-    byte[] digest = md.digest();
-    return DatatypeConverter.printHexBinary(digest).toUpperCase();
+        if (answer1Correct && answer2Correct) {
+            return success(this).feedback("crypto-hashing.success").build();
+        } else if (answer1Correct || answer2Correct) {
+            return failed(this).feedback("crypto-hashing.oneok").build();
+        }
+
+        return failed(this).feedback("crypto-hashing.invalid").build();
+    } catch (Exception e) {
+        return failed(this).feedback("crypto-hashing.error").build();
+    }
   }
 }

src/main/java/org/owasp/webgoat/lessons/jwt/JWTVotesEndpoint.java

@@ -112,11 +112,17 @@ public void login(@RequestParam("user") String user, HttpServletResponse respons
               .signWith(io.jsonwebtoken.SignatureAlgorithm.HS512, JWT_PASSWORD)
               .compact();
       Cookie cookie = new Cookie("access_token", token);
+      cookie.setSecure(true);      // Ensures cookie is only sent over HTTPS
+      cookie.setHttpOnly(true);    // Prevents JavaScript access to the cookie
+      cookie.setPath("/");         // Ensure cookie path is set
       response.addCookie(cookie);
       response.setStatus(HttpStatus.OK.value());
       response.setContentType(MediaType.APPLICATION_JSON_VALUE);
     } else {
       Cookie cookie = new Cookie("access_token", "");
+      cookie.setSecure(true);      // Ensures cookie is only sent over HTTPS
+      cookie.setHttpOnly(true);    // Prevents JavaScript access to the cookie
+      cookie.setPath("/");         // Ensure cookie path is set
       response.addCookie(cookie);
       response.setStatus(HttpStatus.UNAUTHORIZED.value());
       response.setContentType(MediaType.APPLICATION_JSON_VALUE);

src/main/java/org/owasp/webgoat/lessons/pathtraversal/ProfileUploadRetrieval.java

@@ -16,7 +16,12 @@
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.util.Base64;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.UUID;
 import lombok.extern.slf4j.Slf4j;
 import org.apache.commons.lang3.RandomUtils;
 import org.owasp.webgoat.container.CurrentUsername;
@@ -48,11 +53,17 @@
 })
 @Slf4j
 public class ProfileUploadRetrieval implements AssignmentEndpoint {
-  private final File catPicturesDirectory;
+  private final Path catPicturesDirectory;
+  private final Map<String, String> fileIdMapping = new HashMap<>();
+  private final String secretFileName = "path-traversal-secret.jpg";
 
   public ProfileUploadRetrieval(@Value("${webgoat.server.directory}") String webGoatHomeDirectory) {
-    this.catPicturesDirectory = new File(webGoatHomeDirectory, "/PathTraversal/" + "/cats");
-    this.catPicturesDirectory.mkdirs();
+    this.catPicturesDirectory = Paths.get(webGoatHomeDirectory, "PathTraversal", "cats").toAbsolutePath().normalize();
+    try {
+      Files.createDirectories(this.catPicturesDirectory);
+    } catch (IOException e) {
+      log.error("Failed to create cat pictures directory", e);
+    }
   }
 
   @PostConstruct
@@ -61,18 +72,34 @@ public void initAssignment() {
       try (InputStream is =
           new ClassPathResource("lessons/pathtraversal/images/cats/" + i + ".jpg")
               .getInputStream()) {
-        FileCopyUtils.copy(is, new FileOutputStream(new File(catPicturesDirectory, i + ".jpg")));
+        String uuid = UUID.randomUUID().toString();
+        Path targetFile = catPicturesDirectory.resolve(uuid + ".jpg").normalize();
+        // Ensure the target file is within the allowed directory
+        if (!targetFile.startsWith(catPicturesDirectory)) {
+          log.error("Attempted path traversal during init: {}", targetFile);
+          continue;
+        }
+        FileCopyUtils.copy(is, new FileOutputStream(targetFile.toFile()));
+        fileIdMapping.put(String.valueOf(i), uuid);
       } catch (Exception e) {
-        log.error("Unable to copy pictures" + e.getMessage());
+        log.error("Unable to copy pictures", e);
       }
     }
-    var secretDirectory = this.catPicturesDirectory.getParentFile().getParentFile();
     try {
+      Path secretDirectory = catPicturesDirectory.getParent().getParent();
+      Path secretFile = secretDirectory.resolve(secretFileName).normalize();
+
+      // Security check: Ensure the secret file is created in the expected location
+      if (!secretFile.startsWith(secretDirectory)) {
+          log.error("Invalid secret file location detected during init");
+          return;
+      }
+
       Files.writeString(
-          secretDirectory.toPath().resolve("path-traversal-secret.jpg"),
+          secretFile,
           "You found it submit the SHA-512 hash of your username as answer");
     } catch (IOException e) {
-      log.error("Unable to write secret in: {}", secretDirectory, e);
+      log.error("Unable to write secret file", e);
     }
   }
 
@@ -90,30 +117,58 @@ public AttackResult execute(
   @GetMapping("/PathTraversal/random-picture")
   @ResponseBody
   public ResponseEntity<?> getProfilePicture(HttpServletRequest request) {
-    var queryParams = request.getQueryString();
-    if (queryParams != null && (queryParams.contains("..") || queryParams.contains("/"))) {
-      return ResponseEntity.badRequest()
-          .body("Illegal characters are not allowed in the query params");
-    }
     try {
       var id = request.getParameter("id");
-      var catPicture =
-          new File(catPicturesDirectory, (id == null ? RandomUtils.nextInt(1, 11) : id) + ".jpg");
+      String fileId;
+
+      if (id == null) {
+        // For random picture requests, use a number between 1-10 to get the UUID
+        fileId = fileIdMapping.get(String.valueOf(RandomUtils.nextInt(1, 11)));
+      } else {
+        // For specific requests, first check if it's a direct number mapping
+        fileId = fileIdMapping.get(id);
+        if (fileId == null) {
+          // If not found in mapping, treat the id as a UUID
+          fileId = id;
+        }
+      }
 
-      if (catPicture.getName().toLowerCase().contains("path-traversal-secret.jpg")) {
-        return ResponseEntity.ok()
-            .contentType(MediaType.parseMediaType(MediaType.IMAGE_JPEG_VALUE))
-            .body(FileCopyUtils.copyToByteArray(catPicture));
+      // Construct and validate the file path
+      Path requestedFile = catPicturesDirectory.resolve(fileId + ".jpg").normalize();
+
+      // Security check: Ensure the resolved path is within the allowed directory
+      if (!requestedFile.startsWith(catPicturesDirectory)) {
+        log.warn("Attempted path traversal attack detected: {}", id);
+        return ResponseEntity.notFound().build();
       }
-      if (catPicture.exists()) {
+
+      // Check if file exists and handle appropriately
+      if (Files.exists(requestedFile)) {
         return ResponseEntity.ok()
             .contentType(MediaType.parseMediaType(MediaType.IMAGE_JPEG_VALUE))
-            .location(new URI("/PathTraversal/random-picture?id=" + catPicture.getName()))
-            .body(Base64.getEncoder().encode(FileCopyUtils.copyToByteArray(catPicture)));
+            .body(FileCopyUtils.copyToByteArray(requestedFile.toFile()));
       }
-      return ResponseEntity.status(HttpStatus.NOT_FOUND)
-          .location(new URI("/PathTraversal/random-picture?id=" + catPicture.getName()))
-          .body(
+
+      // Special case for the secret file - only return if explicitly requested and properly mapped
+      if (fileId != null && fileId.toLowerCase().contains(secretFileName.toLowerCase())) {
+        Path secretFile = catPicturesDirectory.getParent().getParent().resolve(secretFileName).normalize();
+        // Additional security check for secret file
+        if (!secretFile.startsWith(catPicturesDirectory.getParent().getParent())) {
+          log.warn("Attempted path traversal to unauthorized location: {}", secretFile);
+          return ResponseEntity.notFound().build();
+        }
+        if (Files.exists(secretFile)) {
+          return ResponseEntity.ok()
+              .contentType(MediaType.parseMediaType(MediaType.IMAGE_JPEG_VALUE))
+              .body(FileCopyUtils.copyToByteArray(secretFile.toFile()));
+        }
+      }
+
+      return ResponseEntity.notFound().build();
+    } catch (Exception e) {
+      log.warn("Error processing picture request: {}", e.getMessage());
+      return ResponseEntity.notFound().build();
+    }
               StringUtils.arrayToCommaDelimitedString(catPicture.getParentFile().listFiles())
                   .getBytes());
     } catch (IOException | URISyntaxException e) {

src/main/java/org/owasp/webgoat/lessons/spoofcookie/SpoofCookieAssignment.java

@@ -63,6 +63,10 @@ public AttackResult login(
   public void cleanup(HttpServletResponse response) {
     Cookie cookie = new Cookie(COOKIE_NAME, "");
     cookie.setMaxAge(0);
+    cookie.setPath("/WebGoat");
+    cookie.setSecure(true);
+    cookie.setHttpOnly(true);
+    response.setHeader("Set-Cookie", response.getHeader("Set-Cookie") + "; SameSite=Strict");
     response.addCookie(cookie);
   }
 
@@ -80,6 +84,8 @@ private AttackResult credentialsLoginFlow(
       Cookie newCookie = new Cookie(COOKIE_NAME, newCookieValue);
       newCookie.setPath("/WebGoat");
       newCookie.setSecure(true);
+      newCookie.setHttpOnly(true);
+      response.setHeader("Set-Cookie", response.getHeader("Set-Cookie") + "; SameSite=Strict");
       response.addCookie(newCookie);
       return informationMessage(this)
           .feedback("spoofcookie.login")