From 5b84655329043c8ec9cfd3d2ef56155399f77af8 Mon Sep 17 00:00:00 2001 From: JBHook <314778749+JBHook@users.noreply.github.com> Date: Sun, 9 Aug 2026 10:12:09 +0000 Subject: [PATCH 1/3] Replace linear key mixing and fixed-IV CBC in Broken Crypto Home Made Key createUserSpecificEncryptionKey() mixed the fully attacker-controlled "name" parameter with the secret serverEncryptionKey via plain byte-wise addition mod 256 - a linear, invertible operation. Since the doGet() endpoint echoes the plaintext baseKey literal alongside its ciphertext for any attacker-chosen name (except the hidden "This Challenge" entry), this is a chosen-key setup that lets an attacker recover serverEncryptionKey byte-by-byte, then decrypt the hidden challenge's own answer. Compounding this, encrypt()/decrypt() used AES/CBC with a fixed all-zero IV, making ciphertexts for the same plaintext+key always identical. Added dedicated createUserSpecificEncryptionKey/encryptUserSpecific/ decryptUserSpecific methods for the user-specific-key path: key derivation now uses SHA-256(serverEncryptionKey || userNameKey) (one-way, so chosen-input outputs reveal nothing about the secret), and encryption uses AES-256-GCM with a random IV per call (IV prepended to ciphertext), matching the pattern already merged for BrokenCrypto3.java. The unrelated, unused generic encrypt()/decrypt()/ decryptUserName() methods (not part of the reported key-derivation bug, and not called anywhere in the codebase) are left untouched. Co-Authored-By: Claude Sonnet 5 --- .../challenge/BrokenCryptoHomeMade.java | 89 ++++++++++++++----- 1 file changed, 65 insertions(+), 24 deletions(-) diff --git a/src/main/java/servlets/module/challenge/BrokenCryptoHomeMade.java b/src/main/java/servlets/module/challenge/BrokenCryptoHomeMade.java index b6afa0c96..d052ea71f 100644 --- a/src/main/java/servlets/module/challenge/BrokenCryptoHomeMade.java +++ b/src/main/java/servlets/module/challenge/BrokenCryptoHomeMade.java @@ -4,13 +4,17 @@ import java.io.IOException; import java.io.PrintWriter; import java.nio.charset.Charset; +import java.nio.charset.StandardCharsets; import java.security.GeneralSecurityException; +import java.security.MessageDigest; import java.security.SecureRandom; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import java.util.Locale; import java.util.ResourceBundle; import javax.crypto.Cipher; +import javax.crypto.spec.GCMParameterSpec; import javax.crypto.spec.IvParameterSpec; import javax.crypto.spec.SecretKeySpec; import javax.servlet.ServletException; @@ -248,26 +252,71 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) out.close(); } + private static final int GCM_IV_LENGTH_BYTES = 12; + private static final int GCM_TAG_LENGTH_BITS = 128; + /** - * Merges current server encryption key with user name based encryption key to create user - * specific key + * Derives a user-specific encryption key from the server's secret key and a user name based key, + * via SHA-256 rather than the previous byte-wise addition. Addition is linear and invertible: an + * attacker who fully controls userNameKey (as this challenge's "name" parameter does) and + * observes the resulting ciphertexts can recover serverEncryptionKey byte-by-byte. SHA-256 is + * one-way, so observing outputs for chosen inputs reveals nothing about the secret key mixed into + * them. * * @param userNameKey - * @return + * @return 32-byte AES-256 key */ - private static String createUserSpecificEncryptionKey(String userNameKey) throws Exception { + private static byte[] createUserSpecificEncryptionKey(String userNameKey) throws Exception { if (userNameKey.length() != 16) { throw new Exception("User Name key must be 16 bytes long"); } else { - byte[] serverKey = serverEncryptionKey.getBytes(); - byte[] userKey = userNameKey.getBytes(); - for (int i = 0; i < userKey.length; i++) { - userKey[i] = (byte) (userKey[i] + serverKey[i]); - } - return new String(userKey, Charset.forName("US-ASCII")); + MessageDigest sha256 = MessageDigest.getInstance("SHA-256"); + sha256.update(serverEncryptionKey.getBytes(Charset.forName("US-ASCII"))); + return sha256.digest(userNameKey.getBytes(Charset.forName("US-ASCII"))); } } + /** + * Encrypts plain text using a user-specific key with AES-256-GCM. A random IV is generated per + * call and prepended to the ciphertext. + * + * @param key 32-byte AES-256 key + * @param value Plain text to encrypt + * @return Base64 of (IV || ciphertext) + */ + private static String encryptUserSpecific(byte[] key, String value) + throws GeneralSecurityException { + SecretKeySpec keySpec = new SecretKeySpec(key, "AES"); + byte[] iv = new byte[GCM_IV_LENGTH_BYTES]; + new SecureRandom().nextBytes(iv); + Cipher cipher = Cipher.getInstance("AES/GCM/NoPadding"); + cipher.init(Cipher.ENCRYPT_MODE, keySpec, new GCMParameterSpec(GCM_TAG_LENGTH_BITS, iv)); + byte[] ciphertext = cipher.doFinal(value.getBytes(StandardCharsets.UTF_8)); + byte[] combined = new byte[iv.length + ciphertext.length]; + System.arraycopy(iv, 0, combined, 0, iv.length); + System.arraycopy(ciphertext, 0, combined, iv.length, ciphertext.length); + return Base64.encodeBase64String(combined); + } + + /** + * Decrypts data encrypted by {@link #encryptUserSpecific(byte[], String)}. + * + * @param key 32-byte AES-256 key + * @param encrypted Base64 of (IV || ciphertext) + * @return Decrypted plain text + */ + private static String decryptUserSpecific(byte[] key, String encrypted) + throws GeneralSecurityException { + byte[] combined = Base64.decodeBase64(encrypted); + byte[] iv = Arrays.copyOfRange(combined, 0, GCM_IV_LENGTH_BYTES); + byte[] ciphertext = Arrays.copyOfRange(combined, GCM_IV_LENGTH_BYTES, combined.length); + SecretKeySpec keySpec = new SecretKeySpec(key, "AES"); + Cipher cipher = Cipher.getInstance("AES/GCM/NoPadding"); + cipher.init(Cipher.DECRYPT_MODE, keySpec, new GCMParameterSpec(GCM_TAG_LENGTH_BITS, iv)); + byte[] plaintext = cipher.doFinal(ciphertext); + return new String(plaintext, StandardCharsets.UTF_8); + } + /** * Decrypts data using specific key and ciphertext * @@ -308,16 +357,8 @@ public static String decryptUserName(String encyptedUserName) { public static String decryptUserSpecificSolution(String userNameKey, String encryptedSolution) throws GeneralSecurityException, Exception { try { - String key = createUserSpecificEncryptionKey(userNameKey); - byte[] raw = key.getBytes(Charset.forName("US-ASCII")); - if (raw.length != 16) { - throw new IllegalArgumentException("Invalid key size."); - } - SecretKeySpec skeySpec = new SecretKeySpec(raw, "AES"); - Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding"); - cipher.init(Cipher.DECRYPT_MODE, skeySpec, new IvParameterSpec(new byte[16])); - byte[] original = cipher.doFinal(Base64.decodeBase64(encryptedSolution)); - return new String(original, Charset.forName("US-ASCII")); + byte[] key = createUserSpecificEncryptionKey(userNameKey); + return decryptUserSpecific(key, encryptedSolution); } catch (Exception e) { throw new Exception("Decryption Failure: Could not Craft User Key or Ciphertext was Bad"); } @@ -377,8 +418,8 @@ public static String generateUserSolution(String baseKey, String userSalt) { String toReturn = "Key Should be here! Please refresh the home page and try again!"; try { - String key = createUserSpecificEncryptionKey(Validate.validateEncryptionKey(userSalt)); - String forLog = BrokenCryptoHomeMade.encrypt(key, baseKey + getCurrentSalt()); + byte[] key = createUserSpecificEncryptionKey(Validate.validateEncryptionKey(userSalt)); + String forLog = BrokenCryptoHomeMade.encryptUserSpecific(key, baseKey + getCurrentSalt()); toReturn = "" + "
" @@ -409,8 +450,8 @@ public static String generateUserSolutionKeyOnly(String baseKey, String userSalt String forLog = "Key Should be here! Please refresh the home page and try again!"; try { - String key = createUserSpecificEncryptionKey(Validate.validateEncryptionKey(userSalt)); - forLog = BrokenCryptoHomeMade.encrypt(key, baseKey + getCurrentSalt()); + byte[] key = createUserSpecificEncryptionKey(Validate.validateEncryptionKey(userSalt)); + forLog = BrokenCryptoHomeMade.encryptUserSpecific(key, baseKey + getCurrentSalt()); log.debug("Returning: " + forLog); } catch (Exception e) { From 30fa54a9a1f5d32049c52e2c974accb219b6b76d Mon Sep 17 00:00:00 2001 From: JBHook <314778749+JBHook@users.noreply.github.com> Date: Sun, 9 Aug 2026 10:14:30 +0000 Subject: [PATCH 2/3] Fix submission check to decrypt-and-compare after AES-GCM switch AES-GCM uses a random IV per encryption, so re-encrypting the same plaintext never reproduces the same ciphertext string. The doPost handler was still comparing the submitted ciphertext against a freshly re-encrypted "expected" ciphertext for string equality - this would have rejected every submission, including correct ones, after the prior commit's switch away from deterministic fixed-IV CBC. Now decrypts the submitted ciphertext with the user's derived key and compares the resulting plaintext against the expected baseKey+salt value instead. Co-Authored-By: Claude Sonnet 5 --- .../challenge/BrokenCryptoHomeMade.java | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/src/main/java/servlets/module/challenge/BrokenCryptoHomeMade.java b/src/main/java/servlets/module/challenge/BrokenCryptoHomeMade.java index d052ea71f..06bb258b5 100644 --- a/src/main/java/servlets/module/challenge/BrokenCryptoHomeMade.java +++ b/src/main/java/servlets/module/challenge/BrokenCryptoHomeMade.java @@ -128,11 +128,22 @@ public void doPost(HttpServletRequest request, HttpServletResponse response) log.debug(homemadebadanswers + "previous bad attempts"); if (homemadebadanswers < 5) { String submittedSolution = request.getParameter("theSubmission"); - String expectedSolution = - BrokenCryptoHomeMade.generateUserSolutionKeyOnly( - BrokenCryptoHomeMade.challenges.get(4).get(1), - ses.getAttribute("userName").toString()); - if (submittedSolution.equals(expectedSolution)) { + String baseKey = BrokenCryptoHomeMade.challenges.get(4).get(1); + String expectedPlaintext = baseKey + BrokenCryptoHomeMade.getCurrentSalt(); + // AES-GCM uses a random IV per encryption, so re-encrypting the same plaintext never + // reproduces the same ciphertext string - the submission must be decrypted and its + // plaintext compared, rather than comparing ciphertext strings directly. + boolean correctSubmission = false; + try { + byte[] key = + createUserSpecificEncryptionKey( + Validate.validateEncryptionKey(ses.getAttribute("userName").toString())); + correctSubmission = + expectedPlaintext.equals(decryptUserSpecific(key, submittedSolution)); + } catch (Exception e) { + log.debug("Could not decrypt submitted solution: " + e.toString()); + } + if (correctSubmission) { log.debug("Correct Solution Submitted for 'This Challenge'. Returning Key"); htmlOutput = "

" @@ -149,7 +160,7 @@ public void doPost(HttpServletRequest request, HttpServletResponse response) (String) ses.getAttribute("userName")) + ""; } else { - log.debug("Expected: " + expectedSolution); + log.debug("Expected plaintext: " + expectedPlaintext); log.debug("Got : " + submittedSolution); htmlOutput = "

" From 2c68cc7d29b25a5001a39733803657cdb5192c62 Mon Sep 17 00:00:00 2001 From: JBHook <314778749+JBHook@users.noreply.github.com> Date: Sun, 9 Aug 2026 18:52:22 +0000 Subject: [PATCH 3/3] Fix Broken Crypto Home Made: stop deriving the key from a client-supplied name The previous commit switched key derivation to SHA-256 to stop an attacker recovering the server's secret key via the old linear byte-addition mixing, but left the "name" value itself coming straight from the request parameter. That's an IDOR via the key-derivation input rather than the usual object-id parameter: submitting another user's username as "name" returns that user's personalised encrypted answers in the response, with no relation to who is actually authenticated. Derive it from the session's own userName instead, so a caller can only ever request their own encrypted answers. --- .../module/challenge/BrokenCryptoHomeMade.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/main/java/servlets/module/challenge/BrokenCryptoHomeMade.java b/src/main/java/servlets/module/challenge/BrokenCryptoHomeMade.java index 06bb258b5..3014a24c4 100644 --- a/src/main/java/servlets/module/challenge/BrokenCryptoHomeMade.java +++ b/src/main/java/servlets/module/challenge/BrokenCryptoHomeMade.java @@ -226,10 +226,12 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) "i18n.servlets.challenges.insecureCryptoStorage.insecureCryptoStorage", locale); out.print(getServletInfo()); try { - String name = new String(); - if (request.getParameter("name") != null) { - name = request.getParameter("name").toString(); - } + // This value seeds the per-user key derivation below. It used to come straight from + // the "name" request parameter, so a caller could pass any other user's username here + // and get that user's personalised encrypted answers back in the response - an IDOR + // via the key-derivation input rather than the usual object-id parameter. Tying it to + // the caller's own authenticated session removes that choice entirely. + String name = ses.getAttribute("userName").toString(); if (name.length() < 4) { htmlOutput = bundle.getString("insecureCryptoStorage.homemade.nameTooShort"); } else {