diff --git a/src/main/java/org/owasp/wrongsecrets/challenges/ChallengesController.java b/src/main/java/org/owasp/wrongsecrets/challenges/ChallengesController.java index e3bd7f49e..d1d92d8f6 100644 --- a/src/main/java/org/owasp/wrongsecrets/challenges/ChallengesController.java +++ b/src/main/java/org/owasp/wrongsecrets/challenges/ChallengesController.java @@ -306,10 +306,11 @@ 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)); diff --git a/src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge18.java b/src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge18.java index e87ef0ca2..9211979c0 100644 --- a/src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge18.java +++ b/src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge18.java @@ -33,6 +33,7 @@ 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)))); } diff --git a/src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge39.java b/src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge39.java index 6097cf4af..484655ba7 100644 --- a/src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge39.java +++ b/src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge39.java @@ -51,6 +51,7 @@ 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); diff --git a/src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge40.java b/src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge40.java index 2d4ae233a..9dcddc702 100644 --- a/src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge40.java +++ b/src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge40.java @@ -52,6 +52,7 @@ 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); diff --git a/src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge41.java b/src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge41.java index 4b562fe97..6a76e6c61 100644 --- a/src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge41.java +++ b/src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge41.java @@ -47,6 +47,7 @@ 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)); diff --git a/src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge49.java b/src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge49.java index 51e816be6..01d7a206f 100644 --- a/src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge49.java +++ b/src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge49.java @@ -63,6 +63,7 @@ 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 +83,7 @@ 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); diff --git a/src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge62McpController.java b/src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge62McpController.java index 3eaec6d97..259dc9008 100644 --- a/src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge62McpController.java +++ b/src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge62McpController.java @@ -171,6 +171,11 @@ private Map 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 buildResponse(Object id, Object result) { Map response = new LinkedHashMap<>(); response.put("jsonrpc", JSONRPC_VERSION); diff --git a/src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge8.java b/src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge8.java index e46c2031b..f5ace48f6 100644 --- a/src/main/java/org/owasp/wrongsecrets/challenges/docker/Challenge8.java +++ b/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) { diff --git a/src/main/java/org/owasp/wrongsecrets/challenges/docker/challenge30/Challenge30.java b/src/main/java/org/owasp/wrongsecrets/challenges/docker/challenge30/Challenge30.java index da63c0f86..4c1d5bc7d 100644 --- a/src/main/java/org/owasp/wrongsecrets/challenges/docker/challenge30/Challenge30.java +++ b/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; diff --git a/src/main/java/org/owasp/wrongsecrets/challenges/docker/challenge42/AuditConfiguration.java b/src/main/java/org/owasp/wrongsecrets/challenges/docker/challenge42/AuditConfiguration.java index ccce8aaef..23de7b1e1 100644 --- a/src/main/java/org/owasp/wrongsecrets/challenges/docker/challenge42/AuditConfiguration.java +++ b/src/main/java/org/owasp/wrongsecrets/challenges/docker/challenge42/AuditConfiguration.java @@ -1,6 +1,6 @@ package org.owasp.wrongsecrets.challenges.docker.challenge42; -import groovy.util.logging.Slf4j; +import lombok.extern.slf4j.Slf4j; import java.util.Map; import java.util.UUID; import lombok.Getter; diff --git a/src/test/java/org/owasp/wrongsecrets/challenges/docker/Challenge62McpControllerTest.java b/src/test/java/org/owasp/wrongsecrets/challenges/docker/Challenge62McpControllerTest.java index e2f8a9f95..0a2fc29aa 100644 --- a/src/test/java/org/owasp/wrongsecrets/challenges/docker/Challenge62McpControllerTest.java +++ b/src/test/java/org/owasp/wrongsecrets/challenges/docker/Challenge62McpControllerTest.java @@ -198,28 +198,68 @@ String fetchGoogleDriveDocument(String docId) { } @Test - void readGoogleDriveDocumentShouldCacheOnlyTwentyAdditionalDocuments() throws Exception { - var fetchCount = new AtomicInteger(); + void readDocumentShouldRejectInvalidDocumentId() { var controller = new Challenge62McpController( - "dGVzdA==", DEFAULT_DOC_ID, mock(RestTemplate.class), new ObjectMapper()) { + DEFAULT_KEY, DEFAULT_DOC_ID, mock(RestTemplate.class), new ObjectMapper()); + + String[] invalidIds = {"../sensitive", "doc/with/slash", "doc with space", "doc.with.dot"}; + for (String invalidId : invalidIds) { + Map request = + Map.of( + "jsonrpc", + "2.0", + "id", + 2, + "method", + "tools/call", + "params", + Map.of( + "name", + "read_google_drive_document", + "arguments", + Map.of("document_id", invalidId))); + + Map response = controller.handleMcpRequest(request); + + assertThat(response).containsKey("error"); + @SuppressWarnings("unchecked") + Map error = (Map) response.get("error"); + assertThat(error.get("code")).isEqualTo(-32602); + } + } + + @Test + void readDocumentShouldAcceptValidDocumentId() { + var controller = + new Challenge62McpController( + DEFAULT_KEY, DEFAULT_DOC_ID, mock(RestTemplate.class), new ObjectMapper()) { @Override - String fetchGoogleDriveDocument(String docId) { - fetchCount.incrementAndGet(); - return "cached_secret_for_" + docId; + String readGoogleDriveDocument(String docId) { + return "document_content"; } }; - controller.readGoogleDriveDocument(DEFAULT_DOC_ID); - for (int index = 1; index <= 20; index++) { - controller.readGoogleDriveDocument("doc-" + index); - } + Map request = + Map.of( + "jsonrpc", + "2.0", + "id", + 2, + "method", + "tools/call", + "params", + Map.of( + "name", + "read_google_drive_document", + "arguments", + Map.of("document_id", "1PlZkwEd7GouyY4cdOxBuczm6XumQeuZN31LR2BXRgPs"))); - controller.readGoogleDriveDocument("doc-1"); - controller.readGoogleDriveDocument("doc-21"); - controller.readGoogleDriveDocument(DEFAULT_DOC_ID); - controller.readGoogleDriveDocument("doc-2"); + Map response = controller.handleMcpRequest(request); - assertThat(fetchCount.get()).isEqualTo(23); + assertThat(response).containsKey("result"); + @SuppressWarnings("unchecked") + Map result = (Map) response.get("result"); + assertThat(result).containsKey("content"); } }