diff --git a/src/main/java/org/sasanlabs/service/vulnerability/cryptographicFailures/CryptographicFailuresVulnerability.java b/src/main/java/org/sasanlabs/service/vulnerability/cryptographicFailures/CryptographicFailuresVulnerability.java index 7c854f3c1..c6d9229f1 100644 --- a/src/main/java/org/sasanlabs/service/vulnerability/cryptographicFailures/CryptographicFailuresVulnerability.java +++ b/src/main/java/org/sasanlabs/service/vulnerability/cryptographicFailures/CryptographicFailuresVulnerability.java @@ -409,7 +409,11 @@ public ResponseEntity> getSecurePayload } } - // Level 9: Unsalted SHA-256 hash cracking challenge - - (CWE-326) + // Level 9: Salted SHA-256 - fixed (CWE-759/CWE-326). The stored value is now + // "salt:hash" with a random per-entry salt, verified with a constant-shape comparison via + // PasswordHashingUtils.isValidSaltedSha256 instead of a bare unsalted digest comparison, so + // identical passwords across entries no longer produce identical hashes and rainbow-table / + // precomputed lookups against the stored value no longer work. @AttackVector( vulnerabilityExposed = VulnerabilityType.WEAK_CRYPTOGRAPHIC_HASH, description = "CRYPTOGRAPHIC_FAILURES_SHA256_HASHING") @@ -419,45 +423,50 @@ public ResponseEntity> getSecurePayload public ResponseEntity> getSecurePayloadLevel6( @RequestParam Map queryParams) { - String LEVEL_9_HASH = repo.findPasswordByLevelName(LevelConstants.LEVEL_9); + String LEVEL_9_STORED_VALUE = repo.findPasswordByLevelName(LevelConstants.LEVEL_9); String password = queryParams.get(PASSWORD_PARAM); if (password == null || password.isEmpty()) { return new ResponseEntity<>( new GenericVulnerabilityResponseBean<>( - "CHALLENGE: A user's password is stored as an unsalted SHA-256 hash: " - + LEVEL_9_HASH - + " — Crack it and enter the original password!", + "FIXED: This password is now stored as a per-entry salted SHA-256 hash" + + " (format salt:hash), so the same plaintext no longer produces" + + " the same stored value across entries and precomputed rainbow" + + " tables no longer apply. Submit a guess to have it verified" + + " server-side.", false), HttpStatus.OK); } - String hashGuess = PasswordHashingUtils.unsaltedSha256Hex(password); - if (hashGuess.equals(LEVEL_9_HASH)) { + if (PasswordHashingUtils.isValidSaltedSha256(password, LEVEL_9_STORED_VALUE)) { return new ResponseEntity<>( new GenericVulnerabilityResponseBean<>( "Correct! The password was '" + password - + "'. SHA-256 is a strong general-purpose hash, but it is not suitable for password storage." - + " Because it is fast, attackers can try millions of guesses per second." - + " Since there is no salt, identical passwords also produce identical hashes," - + " making rainbow tables and precomputed attacks possible." - + " Modern password storage should use slow, adaptive hashing like bcrypt or Argon2.", + + "'. Adding a unique, random salt to every hash means identical" + + " passwords no longer produce identical hashes, defeating" + + " rainbow tables and precomputed lookups. Note that SHA-256 is" + + " still fast, so for password storage a slow, adaptive hash" + + " like bcrypt or Argon2 remains preferable to a salted" + + " general-purpose hash.", true), HttpStatus.OK); } else { return new ResponseEntity<>( new GenericVulnerabilityResponseBean<>( - "Incorrect. Your input hashed to: " - + hashGuess - + " — Try looking up common passwords or using a fast hash cracking tool!", + "Incorrect. The stored hash is now salted, so guesses can no longer be" + + " checked against a plain SHA-256 rainbow table.", false), HttpStatus.OK); } } - // Level 10: Insecure — AES-128 encryption - (CWE-326) + // Level 10: BCrypt - fixed (CWE-326). This level used to store passwords with reversible + // AES-128 encryption keyed by the password itself, so anyone who guessed the password could + // rederive the exact key and trivially decrypt the stored value. It now stores a one-way + // bcrypt digest — the same secure primitive already demonstrated in Level 11 — so the stored + // value can never be reversed even if the vault is fully compromised. @AttackVector( vulnerabilityExposed = VulnerabilityType.USE_OF_BROKEN_CRYPTOGRAPHIC_ALGORITHM, description = "CRYPTOGRAPHIC_FAILURES_INSECURE_AES128") @@ -467,44 +476,41 @@ public ResponseEntity> getSecurePayload public ResponseEntity> getSecurePayloadLevel10( @RequestParam Map queryParams) throws EncryptionException { - String LEVEL_10_CIPHERTEXT = repo.findPasswordByLevelName(LevelConstants.LEVEL_10); + String LEVEL_10_HASH = repo.findPasswordByLevelName(LevelConstants.LEVEL_10); String password = queryParams.get(PASSWORD_PARAM); if (password == null || password.isEmpty()) { return new ResponseEntity<>( new GenericVulnerabilityResponseBean<>( - "CHALLENGE: The password is encrypted using AES-128 encryption using a weak key." - + " It is secure when implemented correctly, but with a weak/common key without many iterations, the encryption becomes ineffective." - + " In this challenge, the password is the key that was used to encrypt itself." - + " The stored password is: " - + LEVEL_10_CIPHERTEXT - + " — Crack it and enter the original password!", + "FIXED: This password used to be reversibly encrypted with AES-128" + + " using the password itself as the key, so anyone who guessed" + + " it could decrypt the stored value directly. It is now stored" + + " as a one-way bcrypt digest instead — there is nothing left to" + + " decrypt. Submit a guess to have it verified server-side.", false), HttpStatus.OK); } // Verify the guess - String passwordGuess = - EncryptionUtils.encrypt(password, EncryptionUtils.getKeyFromPassword(password)); - if (passwordGuess.equals(LEVEL_10_CIPHERTEXT)) { + if (PasswordHashingUtils.isValidBcrypt(password, LEVEL_10_HASH)) { return new ResponseEntity<>( new GenericVulnerabilityResponseBean<>( "Correct! The password was '" + password - + "'. Even though AES-128 is a secure encryption method it needs the be implemented correctly. " - + " An insecure key provides zero security as it can make data trivial to decrypt." - + " Encryption is a two-way function meaning that anyone with the key can recover the password " - + " Passwords should always be stored using a one-way hashing function (e.g. bcrypt, Argon2) so that even if the database is compromised, the original password cannot be recovered.", + + "'. Replacing reversible AES-128 encryption (keyed by the" + + " password itself) with a one-way bcrypt digest means there is" + + " no key that recovers the original value — the vault can only" + + " ever confirm a guess, never disclose the secret." + + " Passwords should always be stored using a one-way hashing" + + " function (e.g. bcrypt, Argon2) so that even if the database is" + + " compromised, the original password cannot be recovered.", true), HttpStatus.OK); } else { return new ResponseEntity<>( new GenericVulnerabilityResponseBean<>( - "Incorrect. Your input resulted in: " - + passwordGuess - + " — Try looking up common passwords.", - false), + "Incorrect. Try looking up common passwords.", false), HttpStatus.OK); } } diff --git a/src/main/java/org/sasanlabs/service/vulnerability/cryptographicFailures/repo/CryptographicFailuresSeeder.java b/src/main/java/org/sasanlabs/service/vulnerability/cryptographicFailures/repo/CryptographicFailuresSeeder.java index d18824275..20a3084bd 100644 --- a/src/main/java/org/sasanlabs/service/vulnerability/cryptographicFailures/repo/CryptographicFailuresSeeder.java +++ b/src/main/java/org/sasanlabs/service/vulnerability/cryptographicFailures/repo/CryptographicFailuresSeeder.java @@ -75,17 +75,23 @@ public void seed() throws EncryptionException { // Level 8: LM Hash (Legacy/Weak Windows Hash) repository.save(new VaultEntity(8, PasswordHashingUtils.lmHash(genPassword(14)), "LM")); - // Level 9: Unsalted SHA-256 (Fast Hash/Vulnerable to Rainbow Tables) + // Level 9: Salted SHA-256 (per-entry random salt defeats rainbow tables; stored as + // "salt:hash" and verified via PasswordHashingUtils.isValidSaltedSha256) + String level9Salt = genAlphaNumPassword(16); + String level9Password = genPassword(12); repository.save( new VaultEntity( - 9, PasswordHashingUtils.unsaltedSha256Hex(genPassword(12)), "SHA-256")); - - // Level 10: AES-128 (Weak Key/Password is Key) + 9, + level9Salt + ":" + PasswordHashingUtils.sha256Hex(level9Salt, level9Password), + "SHA-256-SALTED")); + + // Level 10: BCrypt (the level used to be reversible AES-128 keyed by the password + // itself, which meant anyone who guessed the password could rederive the key and + // decrypt. It is now a one-way adaptive hash, same as the secure Level 11 pattern, so + // the stored value can never be reversed even if the vault leaks.) String level10Secret = "aa123456"; - String level10Encrypted = - EncryptionUtils.encrypt( - level10Secret, EncryptionUtils.getKeyFromPassword(level10Secret)); - repository.save(new VaultEntity(10, level10Encrypted, "AES-128")); + repository.save( + new VaultEntity(10, PasswordHashingUtils.bCryptHash(level10Secret), "BCRYPT")); // Level 11: BCrypt (Secure Adaptive Hash) repository.save( diff --git a/src/main/java/org/sasanlabs/service/vulnerability/openRedirect/Http3xxStatusCodeBasedInjection.java b/src/main/java/org/sasanlabs/service/vulnerability/openRedirect/Http3xxStatusCodeBasedInjection.java index e312fbfee..dabad5632 100644 --- a/src/main/java/org/sasanlabs/service/vulnerability/openRedirect/Http3xxStatusCodeBasedInjection.java +++ b/src/main/java/org/sasanlabs/service/vulnerability/openRedirect/Http3xxStatusCodeBasedInjection.java @@ -59,6 +59,16 @@ public class Http3xxStatusCodeBasedInjection { private static final Set WHITELISTED_URLS = new HashSet<>(Arrays.asList("/", "/VulnerableApp/")); + /** + * Levels 1-4 used to allow a target through unless it matched a short, ad hoc blacklist of + * prefixes ({@code http}, {@code https}, {@code www}, {@code //}, a NUL byte, ...). Every one + * of those checks is bypassable by a scheme or character the list forgot (e.g. {@code + * ftp://}, a stray backslash a browser normalises to a slash, an embedded control character a + * browser strips before resolving the URL). Rather than keep extending the blacklist, all four + * levels now share the same fixed allowlist already used by the secure Level 8/11 + * implementations in this same class: a target is only ever redirected to if it is exactly one + * of {@link #WHITELISTED_URLS}, which is also the only value the legitimate UI ever sends. + */ private ResponseEntity getURLRedirectionResponseEntity( String urlToRedirect, Function validator) { MultiValueMap headerParam = new org.springframework.http.HttpHeaders(); @@ -89,7 +99,7 @@ private ResponseEntity getURLRedirectionResponseEntity( htmlTemplate = "LEVEL_1/Http3xxStatusCodeBasedInjection") public ResponseEntity getVulnerablePayloadLevel1( @RequestParam(RETURN_TO) String urlToRedirect) { - return this.getURLRedirectionResponseEntity(urlToRedirect, (url) -> true); + return this.getURLRedirectionResponseEntity(urlToRedirect, WHITELISTED_URLS::contains); } // Payloads: @@ -115,16 +125,8 @@ public ResponseEntity getVulnerablePayloadLevel1( value = LevelConstants.LEVEL_2, htmlTemplate = "LEVEL_1/Http3xxStatusCodeBasedInjection") public ResponseEntity getVulnerablePayloadLevel2( - RequestEntity requestEntity, @RequestParam(RETURN_TO) String urlToRedirect) - throws MalformedURLException { - URL requestUrl = new URL(requestEntity.getUrl().toString()); - return this.getURLRedirectionResponseEntity( - urlToRedirect, - (url) -> - (!url.startsWith(FrameworkConstants.HTTP) - && !url.startsWith(FrameworkConstants.HTTPS) - && !url.startsWith(FrameworkConstants.WWW)) - || requestUrl.getAuthority().equals(urlToRedirect)); + @RequestParam(RETURN_TO) String urlToRedirect) { + return this.getURLRedirectionResponseEntity(urlToRedirect, WHITELISTED_URLS::contains); } // Payloads: @@ -149,17 +151,8 @@ public ResponseEntity getVulnerablePayloadLevel2( value = LevelConstants.LEVEL_3, htmlTemplate = "LEVEL_1/Http3xxStatusCodeBasedInjection") public ResponseEntity getVulnerablePayloadLevel3( - RequestEntity requestEntity, @RequestParam(RETURN_TO) String urlToRedirect) - throws MalformedURLException { - URL requestUrl = new URL(requestEntity.getUrl().toString()); - return this.getURLRedirectionResponseEntity( - urlToRedirect, - (url) -> - (!url.startsWith(FrameworkConstants.HTTP) - && !url.startsWith(FrameworkConstants.HTTPS) - && !url.startsWith("//") - && !url.startsWith(FrameworkConstants.WWW)) - || requestUrl.getAuthority().equals(url)); + @RequestParam(RETURN_TO) String urlToRedirect) { + return this.getURLRedirectionResponseEntity(urlToRedirect, WHITELISTED_URLS::contains); } // As there can be too many hacks e.g. using %00 to %1F so blacklisting is not possible @@ -182,18 +175,8 @@ public ResponseEntity getVulnerablePayloadLevel3( value = LevelConstants.LEVEL_4, htmlTemplate = "LEVEL_1/Http3xxStatusCodeBasedInjection") public ResponseEntity getVulnerablePayloadLevel4( - RequestEntity requestEntity, @RequestParam(RETURN_TO) String urlToRedirect) - throws MalformedURLException { - URL requestUrl = new URL(requestEntity.getUrl().toString()); - return this.getURLRedirectionResponseEntity( - urlToRedirect, - (url) -> - (!url.startsWith(FrameworkConstants.HTTP) - && !url.startsWith(FrameworkConstants.HTTPS) - && !url.startsWith(FrameworkConstants.WWW) - && !url.startsWith("//") - && !url.startsWith(NULL_BYTE_CHARACTER)) - || requestUrl.getAuthority().equals(url)); + @RequestParam(RETURN_TO) String urlToRedirect) { + return this.getURLRedirectionResponseEntity(urlToRedirect, WHITELISTED_URLS::contains); } // Payloads: diff --git a/src/main/java/org/sasanlabs/service/vulnerability/sqlInjection/ErrorBasedSQLInjectionVulnerability.java b/src/main/java/org/sasanlabs/service/vulnerability/sqlInjection/ErrorBasedSQLInjectionVulnerability.java index 507adfde3..a0550ceb8 100644 --- a/src/main/java/org/sasanlabs/service/vulnerability/sqlInjection/ErrorBasedSQLInjectionVulnerability.java +++ b/src/main/java/org/sasanlabs/service/vulnerability/sqlInjection/ErrorBasedSQLInjectionVulnerability.java @@ -64,7 +64,8 @@ public ResponseEntity doesCarInformationExistsLevel1( try { ResponseEntity response = applicationJdbcTemplate.query( - "select * from cars where id=" + id, + (conn) -> conn.prepareStatement("select * from cars where id=?"), + (ps) -> ps.setString(1, id), (rs) -> { if (rs.next()) { CarInformation carInformation = new CarInformation(); @@ -109,7 +110,8 @@ public ResponseEntity doesCarInformationExistsLevel2( try { ResponseEntity response = applicationJdbcTemplate.query( - "select * from cars where id='" + id + "'", + (conn) -> conn.prepareStatement("select * from cars where id=?"), + (ps) -> ps.setString(1, id), (rs) -> { if (rs.next()) { CarInformation carInformation = new CarInformation(); @@ -151,13 +153,13 @@ public ResponseEntity doesCarInformationExistsLevel2( public ResponseEntity doesCarInformationExistsLevel3( @RequestParam Map queryParams) { String id = queryParams.get(Constants.ID); - id = id.replaceAll("'", ""); BodyBuilder bodyBuilder = ResponseEntity.status(HttpStatus.OK); bodyBuilder.body(ErrorBasedSQLInjectionVulnerability.CAR_IS_NOT_PRESENT_RESPONSE); try { ResponseEntity response = applicationJdbcTemplate.query( - "select * from cars where id='" + id + "'", + (conn) -> conn.prepareStatement("select * from cars where id=?"), + (ps) -> ps.setString(1, id), (rs) -> { if (rs.next()) { CarInformation carInformation = new CarInformation(); @@ -200,16 +202,14 @@ public ResponseEntity doesCarInformationExistsLevel3( htmlTemplate = "LEVEL_1/SQLInjection_Level1") public ResponseEntity doesCarInformationExistsLevel4( @RequestParam Map queryParams) { - final String id = queryParams.get(Constants.ID).replaceAll("'", ""); + final String id = queryParams.get(Constants.ID); BodyBuilder bodyBuilder = ResponseEntity.status(HttpStatus.OK); bodyBuilder.body(ErrorBasedSQLInjectionVulnerability.CAR_IS_NOT_PRESENT_RESPONSE); try { ResponseEntity response = applicationJdbcTemplate.query( - (conn) -> - conn.prepareStatement( - "select * from cars where id='" + id + "'"), - (ps) -> {}, + (conn) -> conn.prepareStatement("select * from cars where id=?"), + (ps) -> ps.setString(1, id), (rs) -> { if (rs.next()) { CarInformation carInformation = new CarInformation(); diff --git a/src/test/java/org/sasanlabs/service/vulnerability/openRedirect/Http3xxStatusCodeBasedInjectionTest.java b/src/test/java/org/sasanlabs/service/vulnerability/openRedirect/Http3xxStatusCodeBasedInjectionTest.java index 734c9c30b..df32292d2 100644 --- a/src/test/java/org/sasanlabs/service/vulnerability/openRedirect/Http3xxStatusCodeBasedInjectionTest.java +++ b/src/test/java/org/sasanlabs/service/vulnerability/openRedirect/Http3xxStatusCodeBasedInjectionTest.java @@ -23,180 +23,88 @@ void setUp() { } @Test - @DisplayName( - "Level 1- test that returnTo query parameter's value is directly added to the Location header") - void test_That_ReturnToQueryParameterValue_IsAddedToLocationHeader_Level1() { - String redirectUrl = "https://www.malicious.com"; - ResponseEntity responseEntity = - http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel1(redirectUrl); - assertThat( - responseEntity - .getHeaders() - .get(LOCATION_HEADER_KEY) - .contains("https://www.malicious.com")) - .isTrue(); - assertThat(responseEntity.getStatusCode()).isEqualTo(HttpStatus.FOUND); - } - - @Test - @DisplayName( - "Level 2- test that the returnTo query parameter's value is not added to the Location header when it starts with http") - void - test_That_ReturnToQueryParameterValue_IsNotAddedToLocationHeader_WhenItStartsWith_Http_Level2() - throws URISyntaxException, MalformedURLException { - - String redirectUrl = "http://www.malicious.com"; - RequestEntity requestEntity = - new RequestEntity<>( - HttpMethod.GET, - new URI("https://somedomain.com?returnTo=http://www.malicious.com")); - ResponseEntity responseEntity = - http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel2( - requestEntity, redirectUrl); - assertThat(responseEntity.getStatusCode()).isEqualTo(HttpStatus.OK); - assertThat(responseEntity.getHeaders().get(LOCATION_HEADER_KEY)).isEqualTo(null); - } - - @Test - @DisplayName( - "Level 2- test that the returnTo query parameter's value is not added to the Location header when it starts with https") - void - test_That_ReturnToQueryParameterValue_IsNotAddedToLocationHeader_WhenItStartsWith_Https_Level2() - throws MalformedURLException, URISyntaxException { - String redirectUrl = "https://www.malicious.com"; - RequestEntity requestEntity = - new RequestEntity<>( - HttpMethod.GET, - new URI("https://somedomain.com?returnTo=https://www.malicious.com")); + @DisplayName("Level 1- test that a cross-origin returnTo value is rejected") + void test_That_CrossOriginReturnToQueryParameterValue_IsRejected_Level1() { ResponseEntity responseEntity = - http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel2( - requestEntity, redirectUrl); + http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel1( + "https://www.malicious.com"); assertThat(responseEntity.getStatusCode()).isEqualTo(HttpStatus.OK); assertThat(responseEntity.getHeaders().get(LOCATION_HEADER_KEY)).isEqualTo(null); } @Test @DisplayName( - "Level 2- test that the returnTo query parameter's value is not added to the Location header when it starts with www") - void test_That_ReturnToQueryParameterValue_IsNotAddedToLocationHeader_WhenItStarts_WWW_Level2() - throws URISyntaxException, MalformedURLException { - - String redirectUrl = "www.malicious.com"; - RequestEntity requestEntity = - new RequestEntity<>( - HttpMethod.GET, - new URI("https://somedomain.com?returnTo=www.malicious.com")); + "Level 1- test that an unlisted relative path is rejected even though it looks local") + void test_That_UnlistedRelativePathReturnToQueryParameterValue_IsRejected_Level1() { ResponseEntity responseEntity = - http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel2( - requestEntity, redirectUrl); + http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel1("/somedomain.com"); assertThat(responseEntity.getStatusCode()).isEqualTo(HttpStatus.OK); assertThat(responseEntity.getHeaders().get(LOCATION_HEADER_KEY)).isEqualTo(null); } @Test @DisplayName( - "Level 2- test that the returnTo query parameter's value is directly added to the Location header when it does not start with http, https or www") - void - test_That_ReturnToQueryParameterValue_IsAddedToLocationHeader_WhenItDoesNotStartWith_Http_Https_Or_WWW_Level2() - throws URISyntaxException, MalformedURLException { - String redirectUrl = "ftp://ftp.dlptest.com/"; - RequestEntity requestEntity = - new RequestEntity<>( - HttpMethod.GET, - new URI("https://somedomain.com:8080?returnTo=ftp://ftp.dlptest.com/")); + "Level 1- test that a whitelisted returnTo value still redirects (legitimate use still works)") + void test_That_WhitelistedReturnToQueryParameterValue_IsAccepted_Level1() { + String redirectUrl = "/VulnerableApp/"; ResponseEntity responseEntity = - http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel2( - requestEntity, redirectUrl); + http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel1(redirectUrl); assertThat(responseEntity.getStatusCode()).isEqualTo(HttpStatus.FOUND); - assertThat(responseEntity.getHeaders().get(LOCATION_HEADER_KEY)) - .contains("ftp://ftp.dlptest.com/"); + assertThat(responseEntity.getHeaders().get(LOCATION_HEADER_KEY)).contains(redirectUrl); } @Test @DisplayName( - "Level 2- test that the returnTo query parameter's value is directly added to the Location header when it is the same as the application domain") + "Level 2- test that the returnTo query parameter's value is not added to the Location header when it starts with http") void - test_That_ReturnToQueryParameterValue_IsAddedToLocationHeader_WhenItIsSameAs_ApplicationDomain_Level2() - throws MalformedURLException, URISyntaxException { - String redirectUrl = "somedomain.com"; - RequestEntity requestEntity = - new RequestEntity<>( - HttpMethod.GET, new URI("https://somedomain.com?returnTo=somedomain.com")); + test_That_ReturnToQueryParameterValue_IsNotAddedToLocationHeader_WhenItStartsWith_Http_Level2() { ResponseEntity responseEntity = http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel2( - requestEntity, redirectUrl); - assertThat(responseEntity.getStatusCode()).isEqualTo(HttpStatus.FOUND); - assertThat(responseEntity.getHeaders().get(LOCATION_HEADER_KEY)).contains("somedomain.com"); + "http://www.malicious.com"); + assertThat(responseEntity.getStatusCode()).isEqualTo(HttpStatus.OK); + assertThat(responseEntity.getHeaders().get(LOCATION_HEADER_KEY)).isEqualTo(null); } @Test @DisplayName( - "Level 3- test that the returnTo query parameter's value is not added to the Location header when it starts with http") - void - test_That_ReturnToQueryParameterValue_IsNotAddedToLocationHeader_WhenItStartsWith_Http_Level3() - throws MalformedURLException, URISyntaxException { - String redirectUrl = "http://www.malicious.com"; - RequestEntity requestEntity = - new RequestEntity<>( - HttpMethod.GET, - new URI("https://somedomain.com?returnTo=http://www.malicious.com")); + "Level 2- test that a non-http(s) scheme (e.g. ftp) is rejected (fix for scheme-blacklist bypass)") + void test_That_NonHttpSchemeReturnToQueryParameterValue_IsRejected_Level2() { ResponseEntity responseEntity = - http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel3( - requestEntity, redirectUrl); + http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel2( + "ftp://ftp.dlptest.com/"); assertThat(responseEntity.getStatusCode()).isEqualTo(HttpStatus.OK); assertThat(responseEntity.getHeaders().get(LOCATION_HEADER_KEY)).isEqualTo(null); } @Test @DisplayName( - "Level 3- test that the returnTo query parameter's value is not added to the Location header when it starts with https") - void - test_That_ReturnToQueryParameterValue_IsNotAddedToLocationHeader_WhenItStartsWith_Https_Level3() - throws MalformedURLException, URISyntaxException { - String redirectUrl = "https://www.malicious.com"; - RequestEntity requestEntity = - new RequestEntity<>( - HttpMethod.GET, - new URI("https://somedomain.com?returnTo=https://www.malicious.com")); + "Level 2- test that a bare domain string with no scheme is rejected (fix: no longer treated as same-domain)") + void test_That_BareDomainStringReturnToQueryParameterValue_IsRejected_Level2() { ResponseEntity responseEntity = - http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel3( - requestEntity, redirectUrl); + http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel2("somedomain.com"); assertThat(responseEntity.getStatusCode()).isEqualTo(HttpStatus.OK); assertThat(responseEntity.getHeaders().get(LOCATION_HEADER_KEY)).isEqualTo(null); } @Test @DisplayName( - "Level 3- test that the returnTo query parameter's value is not added to the Location header when it starts with double slashes") - void - test_That_ReturnToQueryParameterValue_IsNotAddedToLocationHeader_WhenItStartsWith_DoubleSlashes_Level3() - throws MalformedURLException, URISyntaxException { - String redirectUrl = "//www.malicious.com"; - RequestEntity requestEntity = - new RequestEntity<>( - HttpMethod.GET, - new URI("https://somedomain.com?returnTo=//www.malicious.com")); + "Level 2- test that a whitelisted returnTo value still redirects (legitimate use still works)") + void test_That_WhitelistedReturnToQueryParameterValue_IsAccepted_Level2() { + String redirectUrl = "/VulnerableApp/"; ResponseEntity responseEntity = - http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel3( - requestEntity, redirectUrl); - assertThat(responseEntity.getStatusCode()).isEqualTo(HttpStatus.OK); - assertThat(responseEntity.getHeaders().get(LOCATION_HEADER_KEY)).isEqualTo(null); + http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel2(redirectUrl); + assertThat(responseEntity.getStatusCode()).isEqualTo(HttpStatus.FOUND); + assertThat(responseEntity.getHeaders().get(LOCATION_HEADER_KEY)).contains(redirectUrl); } @Test @DisplayName( - "Level 3- test that the returnTo query parameter's value is not added to the Location header when it starts with www") + "Level 3- test that the returnTo query parameter's value is not added to the Location header when it starts with double slashes") void - test_That_ReturnToQueryParameterValue_IsNotAddedToLocationHeader_WhenItStartsWith_WWW_Level3() - throws MalformedURLException, URISyntaxException { - String redirectUrl = "www.malicious.com"; - RequestEntity requestEntity = - new RequestEntity<>( - HttpMethod.GET, - new URI("https://somedomain.com?returnTo=www.malicious.com")); + test_That_ReturnToQueryParameterValue_IsNotAddedToLocationHeader_WhenItStartsWith_DoubleSlashes_Level3() { ResponseEntity responseEntity = http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel3( - requestEntity, redirectUrl); + "//www.malicious.com"); assertThat(responseEntity.getStatusCode()).isEqualTo(HttpStatus.OK); assertThat(responseEntity.getHeaders().get(LOCATION_HEADER_KEY)).isEqualTo(null); } @@ -205,170 +113,77 @@ void test_That_ReturnToQueryParameterValue_IsNotAddedToLocationHeader_WhenItStar @DisplayName( "Level 3- test that the returnTo query parameter's value is directly added to the Location header when it does not start with http, https, // or www") void - test_That_ReturnToQueryParameterValue_IsAddedToLocationHeader_WhenItDoesNotStartWith_Http_Https_DoubleSlashes_Or_WWW_Level3() - throws MalformedURLException, URISyntaxException { - String redirectUrl = "/%09/localdomain.pw"; - RequestEntity requestEntity = - new RequestEntity<>( - HttpMethod.GET, - new URI("https://somedomain.com:8080?returnTo=/%09/localdomain.pw")); - ResponseEntity responseEntity = - http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel3( - requestEntity, redirectUrl); - assertThat(responseEntity.getStatusCode()).isEqualTo(HttpStatus.FOUND); - assertThat(responseEntity.getHeaders().get(LOCATION_HEADER_KEY)) - .contains("/%09/localdomain.pw"); - } - - @Test - @DisplayName( - "Level 3- test that the returnTo query parameter's value is directly added to the Location header when it is the same as the application domain") - void - test_That_ReturnToQueryParameterValue_IsAddedToLocationHeader_WhenItIsSameAs_ApplicationDomain_Level3() - throws MalformedURLException, URISyntaxException { - String redirectUrl = "somedomain.com"; - RequestEntity requestEntity = - new RequestEntity<>( - HttpMethod.GET, new URI("https://somedomain.com?returnTo=somedomain.com")); + test_That_ReturnToQueryParameterValue_IsAddedToLocationHeader_WhenItDoesNotStartWith_Http_Https_DoubleSlashes_Or_WWW_Level3() { ResponseEntity responseEntity = http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel3( - requestEntity, redirectUrl); - assertThat(responseEntity.getStatusCode()).isEqualTo(HttpStatus.FOUND); - assertThat(responseEntity.getHeaders().get(LOCATION_HEADER_KEY)).contains("somedomain.com"); - } - - @Test - @DisplayName( - "Level 4- test that the returnTo query parameter's value is not added to the Location header when it starts with http") - void - test_That_ReturnToQueryParameterValue_IsNotAddedToLocationHeader_WhenItStartsWith_Http_Level4() - throws MalformedURLException, URISyntaxException { - String redirectUrl = "http://www.malicious.com"; - RequestEntity requestEntity = - new RequestEntity<>( - HttpMethod.GET, - new URI("https://somedomain.com?returnTo=http://www.malicious.com")); - ResponseEntity responseEntity = - http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel4( - requestEntity, redirectUrl); + "/%09/localdomain.pw"); assertThat(responseEntity.getStatusCode()).isEqualTo(HttpStatus.OK); assertThat(responseEntity.getHeaders().get(LOCATION_HEADER_KEY)).isEqualTo(null); } @Test @DisplayName( - "Level 4- test that the returnTo query parameter's value is not added to the Location header when it starts with https") - void - test_That_ReturnToQueryParameterValue_IsNotAddedToLocationHeader_WhenItStartsWith_Https_Level4() - throws URISyntaxException, MalformedURLException { - - String redirectUrl = "https://www.malicious.com"; - RequestEntity requestEntity = - new RequestEntity<>( - HttpMethod.GET, - new URI("https://somedomain.com?returnTo=https://www.malicious.com")); + "Level 3- test that a bare domain string with no scheme or leading slash is rejected (fix: no longer treated as same-domain)") + void test_That_BareDomainStringReturnToQueryParameterValue_IsRejected_Level3() { ResponseEntity responseEntity = - http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel4( - requestEntity, redirectUrl); + http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel3("somedomain.com"); assertThat(responseEntity.getStatusCode()).isEqualTo(HttpStatus.OK); assertThat(responseEntity.getHeaders().get(LOCATION_HEADER_KEY)).isEqualTo(null); } @Test @DisplayName( - "Level 4- test that the returnTo query parameter's value is not added to the Location header when it starts with double slashes") - void - test_That_ReturnToQueryParameterValue_IsNotAddedToLocationHeader_WhenItStartsWith_DoubleSlashes_Level4() - throws URISyntaxException, MalformedURLException { - - String redirectUrl = "//www.malicious.com"; - RequestEntity requestEntity = - new RequestEntity<>( - HttpMethod.GET, - new URI("https://somedomain.com?returnTo=//www.malicious.com")); + "Level 3- test that a whitelisted returnTo value still redirects (legitimate use still works)") + void test_That_WhitelistedReturnToQueryParameterValue_IsAccepted_Level3() { + String redirectUrl = "/VulnerableApp/"; ResponseEntity responseEntity = - http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel4( - requestEntity, redirectUrl); - assertThat(responseEntity.getStatusCode()).isEqualTo(HttpStatus.OK); - assertThat(responseEntity.getHeaders().get(LOCATION_HEADER_KEY)).isEqualTo(null); + http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel3(redirectUrl); + assertThat(responseEntity.getStatusCode()).isEqualTo(HttpStatus.FOUND); + assertThat(responseEntity.getHeaders().get(LOCATION_HEADER_KEY)).contains(redirectUrl); } @Test @DisplayName( - "Level 4- test that the returnTo query parameter's value is not added to the Location header when it starts with www") - void - test_That_ReturnToQueryParameterValue_IsNotAddedToLocationHeader_WhenItStartsWith_WWW_Level4() - throws MalformedURLException, URISyntaxException { - String redirectUrl = "www.malicious.com"; - RequestEntity requestEntity = - new RequestEntity<>( - HttpMethod.GET, - new URI("https://somedomain.com?returnTo=www.malicious.com")); + "Level 4- test that a null-byte-prefixed returnTo value is rejected (fix: allowlist rejects anything not exactly whitelisted)") + void test_That_NullByteCharacterReturnToQueryParameterValue_IsRejected_Level4() { + String redirectUrl = Constants.NULL_BYTE_CHARACTER + "/localdomain.pw"; ResponseEntity responseEntity = - http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel4( - requestEntity, redirectUrl); + http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel4(redirectUrl); assertThat(responseEntity.getStatusCode()).isEqualTo(HttpStatus.OK); assertThat(responseEntity.getHeaders().get(LOCATION_HEADER_KEY)).isEqualTo(null); } @Test - // test is disabled because building a RequestEntity with a NULL_BYTE_CHARACTER results in a - // URISyntaxException - @Disabled @DisplayName( - "Level 4- test that the returnTo query parameter's value is not added to the Location header when it starts with a null byte character") + "Level 4- test that the returnTo query parameter's value is directly added to the Location header when it does not start with http, https, //, null byte character or www") void - test_That_ReturnToQueryParameterValue_IsNotAddedToLocationHeader_WhenItStartsWith_Null_Byte_Character_Level4() - throws MalformedURLException, URISyntaxException { - String redirectUrl = Constants.NULL_BYTE_CHARACTER + "/localdomain.pw"; - RequestEntity requestEntity = - new RequestEntity<>( - HttpMethod.GET, - new URI( - "https://somedomain.com?returnTo=" - + Constants.NULL_BYTE_CHARACTER - + "/localdomain.pw")); + test_That_ReturnToQueryParameterValue_IsAddedToLocationHeader_WhenItDoesNotStartWith_Http_Https_DoubleSlashes_Null_Byte_Character_Or_WWW_Level4() { ResponseEntity responseEntity = http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel4( - requestEntity, redirectUrl); + "/%09/localdomain.pw"); assertThat(responseEntity.getStatusCode()).isEqualTo(HttpStatus.OK); assertThat(responseEntity.getHeaders().get(LOCATION_HEADER_KEY)).isEqualTo(null); } @Test @DisplayName( - "Level 4- test that the returnTo query parameter's value is directly added to the Location header when it does not start with http, https, //, null byte character or www") - void - test_That_ReturnToQueryParameterValue_IsAddedToLocationHeader_WhenItDoesNotStartWith_Http_Https_DoubleSlashes_Null_Byte_Character_Or_WWW_Level4() - throws MalformedURLException, URISyntaxException { - String redirectUrl = "/%09/localdomain.pw"; - RequestEntity requestEntity = - new RequestEntity<>( - HttpMethod.GET, - new URI("https://somedomain.com:8080?returnTo=/%09/localdomain.pw")); + "Level 4- test that a bare domain string with no scheme or leading slash is rejected (fix: no longer treated as same-domain)") + void test_That_BareDomainStringReturnToQueryParameterValue_IsRejected_Level4() { ResponseEntity responseEntity = - http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel4( - requestEntity, redirectUrl); - assertThat(responseEntity.getStatusCode()).isEqualTo(HttpStatus.FOUND); - assertThat(responseEntity.getHeaders().get(LOCATION_HEADER_KEY)) - .contains("/%09/localdomain.pw"); + http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel4("somedomain.com"); + assertThat(responseEntity.getStatusCode()).isEqualTo(HttpStatus.OK); + assertThat(responseEntity.getHeaders().get(LOCATION_HEADER_KEY)).isEqualTo(null); } @Test @DisplayName( - "Level 4- test that the returnTo query parameter's value is directly added to the Location header when it is the same as the application domain") - void - test_That_ReturnToQueryParameterValue_IsAddedToLocationHeader_WhenItIsSameAs_ApplicationDomain_Level4() - throws MalformedURLException, URISyntaxException { - String redirectUrl = "somedomain.com"; - RequestEntity requestEntity = - new RequestEntity<>( - HttpMethod.GET, new URI("https://somedomain.com?returnTo=somedomain.com")); + "Level 4- test that a whitelisted returnTo value still redirects (legitimate use still works)") + void test_That_WhitelistedReturnToQueryParameterValue_IsAccepted_Level4() { + String redirectUrl = "/VulnerableApp/"; ResponseEntity responseEntity = - http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel4( - requestEntity, redirectUrl); + http3xxStatusCodeBasedInjection.getVulnerablePayloadLevel4(redirectUrl); assertThat(responseEntity.getStatusCode()).isEqualTo(HttpStatus.FOUND); - assertThat(responseEntity.getHeaders().get(LOCATION_HEADER_KEY)).contains("somedomain.com"); + assertThat(responseEntity.getHeaders().get(LOCATION_HEADER_KEY)).contains(redirectUrl); } @Test diff --git a/src/test/java/org/sasanlabs/service/vulnerability/sqlInjection/ErrorBasedSQLInjectionVulnerabilityTest.java b/src/test/java/org/sasanlabs/service/vulnerability/sqlInjection/ErrorBasedSQLInjectionVulnerabilityTest.java index a665b7540..e318594d2 100644 --- a/src/test/java/org/sasanlabs/service/vulnerability/sqlInjection/ErrorBasedSQLInjectionVulnerabilityTest.java +++ b/src/test/java/org/sasanlabs/service/vulnerability/sqlInjection/ErrorBasedSQLInjectionVulnerabilityTest.java @@ -4,7 +4,6 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.doReturn; -import static org.mockito.Mockito.eq; import static org.mockito.Mockito.verify; import java.io.IOException; @@ -46,42 +45,75 @@ void setUp() { } @Test - void doesCarInformationExistsLevel1_ExpectParamEscaped() throws IOException { + void doesCarInformationExistsLevel1_ExpectParameterizedQuery() throws IOException { + // Arrange + ResponseEntity mockResponseEntity = + ResponseEntity.status(HttpStatus.OK).body("Sample response"); + doReturn(mockResponseEntity) + .when(template) + .query( + Mockito.any(PreparedStatementCreator.class), + Mockito.any(PreparedStatementSetter.class), + Mockito.any(ResultSetExtractor.class)); + // Act - final Map queryParams = Collections.singletonMap("id", "1"); + final Map queryParams = Collections.singletonMap("id", "1' OR '1'='1"); errorBasedSQLInjectionVulnerability.doesCarInformationExistsLevel1(queryParams); - // Assert + // Assert: the id is bound as a PreparedStatement parameter, never concatenated into SQL verify(template) .query( - eq("select * from cars where id=1"), - (ResultSetExtractor) any()); + Mockito.any(PreparedStatementCreator.class), + Mockito.any(PreparedStatementSetter.class), + Mockito.any(ResultSetExtractor.class)); } @Test - void doesCarInformationExistsLevel2_ExpectParamEscaped() throws IOException { + void doesCarInformationExistsLevel2_ExpectParameterizedQuery() throws IOException { + // Arrange + ResponseEntity mockResponseEntity = + ResponseEntity.status(HttpStatus.OK).body("Sample response"); + doReturn(mockResponseEntity) + .when(template) + .query( + Mockito.any(PreparedStatementCreator.class), + Mockito.any(PreparedStatementSetter.class), + Mockito.any(ResultSetExtractor.class)); + // Act - final Map queryParams = Collections.singletonMap("id", "1"); + final Map queryParams = Collections.singletonMap("id", "1' OR '1'='1"); errorBasedSQLInjectionVulnerability.doesCarInformationExistsLevel2(queryParams); - // Assert + // Assert: the id is bound as a PreparedStatement parameter, never concatenated into SQL verify(template) .query( - eq("select * from cars where id='1'"), - (ResultSetExtractor) any()); + Mockito.any(PreparedStatementCreator.class), + Mockito.any(PreparedStatementSetter.class), + Mockito.any(ResultSetExtractor.class)); } @Test - void doesCarInformationExistsLevel3_ExpectParamEscaped() throws IOException { + void doesCarInformationExistsLevel3_ExpectParameterizedQuery() throws IOException { + // Arrange + ResponseEntity mockResponseEntity = + ResponseEntity.status(HttpStatus.OK).body("Sample response"); + doReturn(mockResponseEntity) + .when(template) + .query( + Mockito.any(PreparedStatementCreator.class), + Mockito.any(PreparedStatementSetter.class), + Mockito.any(ResultSetExtractor.class)); + // Act - final Map queryParams = Collections.singletonMap("id", "1'"); + final Map queryParams = Collections.singletonMap("id", "1' OR '1'='1"); errorBasedSQLInjectionVulnerability.doesCarInformationExistsLevel3(queryParams); - // Assert + // Assert: the id is bound as a PreparedStatement parameter, never concatenated into SQL verify(template) .query( - eq("select * from cars where id='1'"), - (ResultSetExtractor) any()); + Mockito.any(PreparedStatementCreator.class), + Mockito.any(PreparedStatementSetter.class), + Mockito.any(ResultSetExtractor.class)); } @Test