Merge pull request #2512 from OWASP/copilot/fix-security-quality-issues

Fix security & quality issues from GitHub security dashboard

Author: commjoen

src/main/java/org/owasp/wrongsecrets/challenges/ChallengesController.java

@@ -306,10 +306,12 @@ public String postController(
   // TODO extract this to the challenge definition @see ChallengeAPIController with nested if
   // statement
   private String generateCode(ChallengeDefinition challenge) {
+    // codeql[java/weak-cryptographic-algorithm] Intentional HmacSHA1 for CTF code generation - this
+    // is educational/functional, not security-critical
     SecretKeySpec secretKeySpec =
         new SecretKeySpec(ctfKey.getBytes(StandardCharsets.UTF_8), "HmacSHA1");
     try {
-      Mac mac = Mac.getInstance("HmacSHA1");
+      Mac mac = Mac.getInstance("HmacSHA1"); // NOSONAR
       mac.init(secretKeySpec);
       byte[] result = mac.doFinal(challenge.name().name().getBytes(StandardCharsets.UTF_8));
       return new String(Hex.encode(result));

src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge18.java

@@ -33,6 +33,8 @@ private String base64Decode(String base64) {
   private String calculateHash(String hash, String input) {
     try {
       if (md5Hash.equals(hash) || sha1Hash.equals(hash)) {
+        // codeql[java/weak-cryptographic-algorithm] Intentionally weak algorithm for educational
+        // challenge about weak hash mechanisms
         var md = MessageDigest.getInstance(hash);
         return new String(Hex.encode(md.digest(input.getBytes(StandardCharsets.UTF_8))));
       }

src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge39.java

@@ -51,6 +51,8 @@ private String getSolution() {
       SecretKey secretKey = new SecretKeySpec(decodedKey, "AES");
 
       // Initialize the Cipher for decryption
+      // codeql[java/weak-cryptographic-algorithm] Intentionally weak ECB mode for educational
+      // challenge about filename-as-key
       Cipher cipher = Cipher.getInstance("AES");
       cipher.init(Cipher.DECRYPT_MODE, secretKey);
 

src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge40.java

@@ -52,6 +52,8 @@ private String getSolution() {
       SecretKey secretKey = new SecretKeySpec(plainDecryptionKey, "AES");
 
       // Initialize the Cipher for decryption
+      // codeql[java/weak-cryptographic-algorithm] Intentionally weak ECB mode for educational
+      // challenge about co-located key and secret
       Cipher cipher = Cipher.getInstance("AES");
       cipher.init(Cipher.DECRYPT_MODE, secretKey);
 

src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge41.java

@@ -47,6 +47,8 @@ public boolean answerCorrect(String answer) {
       value = "WEAK_MESSAGE_DIGEST_MD5",
       justification = "This is to allow md5 hashing")
   private String hashWithMd5(String plainText) throws NoSuchAlgorithmException {
+    // codeql[java/weak-cryptographic-algorithm] Intentionally weak algorithm for educational
+    // challenge demonstrating password shucking
     MessageDigest md = MessageDigest.getInstance("MD5");
 
     byte[] result = md.digest(plainText.getBytes(StandardCharsets.UTF_8));

src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge49.java

@@ -63,6 +63,8 @@ public boolean answerCorrect(String answer) {
       value = "WEAK_MESSAGE_DIGEST_MD5",
       justification = "This is to allow md5 hashing")
   private String hashWithMd5(String plainText) throws NoSuchAlgorithmException {
+    // codeql[java/weak-cryptographic-algorithm] Intentionally weak algorithm for educational
+    // challenge demonstrating weak KDF
     MessageDigest md = MessageDigest.getInstance("MD5");
 
     byte[] result = md.digest(plainText.getBytes(StandardCharsets.UTF_8));
@@ -82,6 +84,8 @@ private String decrypt(String cipherText, String key) {
 
       SecretKey secretKey = new SecretKeySpec(key.getBytes(StandardCharsets.UTF_8), "AES");
 
+      // codeql[java/weak-cryptographic-algorithm] Intentionally weak ECB mode for educational
+      // challenge demonstrating weak KDF
       Cipher cipher = Cipher.getInstance("AES");
       cipher.init(Cipher.DECRYPT_MODE, secretKey);
 

src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge62McpController.java

@@ -171,6 +171,11 @@ private Map<String, Object> handleReadGoogleDriveDocument(
             ? (String) arguments.get("document_id")
             : documentId;
 
+    if (!isValidGoogleDriveDocumentId(docId)) {
+      log.warn("Challenge62: Invalid document ID format: {}", sanitizeForLog(docId));
+      return buildErrorResponse(id, -32602, "Invalid document_id format");
+    }
+
     log.info(
         "Challenge62 MCP read_google_drive_document called for document: {}",
         sanitizeForLog(docId));
@@ -352,6 +357,18 @@ private String sanitizeForLog(String input) {
     return input.replaceAll("[\r\n\u0085\u2028\u2029]", "_");
   }
 
+  /**
+   * Validates that a Google Drive document ID only contains characters that are valid in a Google
+   * Drive document ID. Document IDs consist of alphanumeric characters, hyphens and underscores.
+   * This prevents SSRF by ensuring the ID cannot be used to escape the expected URL path.
+   *
+   * @param docId the document ID to validate
+   * @return true if the document ID is valid
+   */
+  private boolean isValidGoogleDriveDocumentId(String docId) {
+    return docId != null && !docId.isEmpty() && docId.matches("[a-zA-Z0-9_\\-]+");
+  }
+
   private Map<String, Object> buildResponse(Object id, Object result) {
     Map<String, Object> response = new LinkedHashMap<>();
     response.put("jsonrpc", JSONRPC_VERSION);

src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge8.java

@@ -2,7 +2,6 @@
 
 import com.google.api.client.util.Strings;
 import java.security.SecureRandom;
-import java.util.Random;
 import lombok.extern.slf4j.Slf4j;
 import org.owasp.wrongsecrets.challenges.FixedAnswerChallenge;
 import org.springframework.beans.factory.annotation.Value;
@@ -16,7 +15,7 @@ public class Challenge8 extends FixedAnswerChallenge {
   private static final String alphabet =
       "0123456789QWERTYUIOPASDFGHJKLZXCVBNMqwertyuiopasdfghjklzxcvbnm";
 
-  private final Random secureRandom = new SecureRandom();
+  private final SecureRandom secureRandom = new SecureRandom();
   private final String serverCode;
 
   public Challenge8(@Value("${challenge_acht_ctf_host_value}") String serverCode) {

src/main/java/org/owasp/wrongsecrets/challenges/docker/challenge30/Challenge30.java

@@ -2,7 +2,6 @@
 
 import com.google.common.base.Strings;
 import java.security.SecureRandom;
-import java.util.Random;
 import org.owasp.wrongsecrets.challenges.Challenge;
 import org.owasp.wrongsecrets.challenges.Spoiler;
 import org.springframework.stereotype.Component;
@@ -12,7 +11,7 @@
  */
 @Component
 public class Challenge30 implements Challenge {
-  private final Random secureRandom = new SecureRandom();
+  private final SecureRandom secureRandom = new SecureRandom();
   private static final String alphabet =
       "0123456789QWERTYUIOPASDFGHJKLZXCVBNMqwertyuiopasdfghjklzxcvbnm";
   private String solution;

src/main/java/org/owasp/wrongsecrets/challenges/docker/challenge42/AuditConfiguration.java

@@ -1,9 +1,9 @@
 package org.owasp.wrongsecrets.challenges.docker.challenge42;
 
-import groovy.util.logging.Slf4j;
 import java.util.Map;
 import java.util.UUID;
 import lombok.Getter;
+import lombok.extern.slf4j.Slf4j;
 import org.springframework.boot.actuate.audit.AuditEvent;
 import org.springframework.boot.actuate.audit.InMemoryAuditEventRepository;
 import org.springframework.context.annotation.Bean;