From 1919a22510460601805fdab3896b775f79c51528 Mon Sep 17 00:00:00 2001 From: FiveOFive Date: Wed, 30 Oct 2024 15:41:05 -0700 Subject: [PATCH 1/5] unit tests for error and union based sql injection Signed-off-by: FiveOFive --- .../ascanrules/SqlInjectionScanRule.java | 4 +- .../SqlInjectionScanRuleUnitTest.java | 192 ++++++++++++++++++ 2 files changed, 194 insertions(+), 2 deletions(-) diff --git a/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRule.java b/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRule.java index 6cfa31a8e43..c31b86dc50e 100644 --- a/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRule.java +++ b/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRule.java @@ -120,7 +120,7 @@ public class SqlInjectionScanRule extends AbstractAppParamPlugin * maximise SQL errors Note that we do separate runs for each family of characters, in case one * family are filtered out, the others might still get past */ - private static final String[] SQL_CHECK_ERR = {"'", "\"", ";", "'(", ")", "(", "NULL", "'\""}; + static final String[] SQL_CHECK_ERR = {"'", "\"", ";", "'(", ")", "(", "NULL", "'\""}; /** * A collection of RDBMS with its error message fragments and {@code Tech}. @@ -449,7 +449,7 @@ private static List asList(String... strings) { * generic UNION statements. Hoping these will cause a specific error message that we will * recognise */ - private static String[] SQL_UNION_APPENDAGES = { + static String[] SQL_UNION_APPENDAGES = { " UNION ALL select NULL" + SQL_ONE_LINE_COMMENT, "' UNION ALL select NULL" + SQL_ONE_LINE_COMMENT, "\" UNION ALL select NULL" + SQL_ONE_LINE_COMMENT, diff --git a/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRuleUnitTest.java b/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRuleUnitTest.java index bd343731e8a..c31b3b9d07c 100644 --- a/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRuleUnitTest.java +++ b/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRuleUnitTest.java @@ -31,8 +31,10 @@ import fi.iki.elonen.NanoHTTPD.IHTTPSession; import fi.iki.elonen.NanoHTTPD.Response; import java.util.Map; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.parosproxy.paros.core.scanner.Alert; +import org.parosproxy.paros.core.scanner.Plugin.AlertThreshold; import org.parosproxy.paros.core.scanner.Plugin.AttackStrength; import org.zaproxy.addon.commonlib.CommonAlertTag; import org.zaproxy.zap.model.Tech; @@ -376,6 +378,196 @@ void shouldAlertByBodyComparisonIgnoringXmlEscapedPayload() throws Exception { assertThat(actual.getAttack(), is(equalTo(attackPayload))); } + @Nested + class ErrorBasedSqlInjection { + @Test + void shouldAlert_emptyPrefix() throws Exception { + // Given + final String param = "param"; + final String normalValue = "test"; + final String emptyPrefixErrorValue = "" + SqlInjectionScanRule.SQL_CHECK_ERR[0]; + + final UrlParamValueHandler handler = + UrlParamValueHandler.builder() + .targetParam(param) + .whenParamValueIs(param) + .thenReturnHtml(normalValue) + .whenParamValueIs(emptyPrefixErrorValue) + .thenReturnHtml("You have an error in your SQL syntax") + .build(); + nano.addHandler(handler); + rule.init(getHttpMessage("/?param=" + normalValue), parent); + + // When + rule.scan(); + + // Then + assertThat(alertsRaised, hasSize(1)); + } + + @Test + void shouldAlert_originalParamPrefix() throws Exception { + // Given + final String param = "param"; + final String normalValue = "test"; + final String originalParamErrorValue = + normalValue + SqlInjectionScanRule.SQL_CHECK_ERR[0]; + + final UrlParamValueHandler handler = + UrlParamValueHandler.builder() + .targetParam(param) + .whenParamValueIs(param) + .thenReturnHtml(normalValue) + .whenParamValueIs(originalParamErrorValue) + .thenReturnHtml("You have an error in your SQL syntax") + .build(); + nano.addHandler(handler); + rule.init(getHttpMessage("/?param=" + normalValue), parent); + + // When + rule.scan(); + + // Then + assertThat(alertsRaised, hasSize(1)); + } + + @Test + void shouldNotAlert_nonSqlMessage() throws Exception { + // Given + final String param = "param"; + final String normalValue = "test"; + final String originalParamErrorValue = + normalValue + SqlInjectionScanRule.SQL_CHECK_ERR[0]; + + final UrlParamValueHandler handler = + UrlParamValueHandler.builder() + .targetParam(param) + .whenParamValueIs(param) + .thenReturnHtml(normalValue) + .whenParamValueIs(originalParamErrorValue) + .thenReturnHtml("Not a SQL error message") + .build(); + nano.addHandler(handler); + rule.init(getHttpMessage("/?param=" + normalValue), parent); + + // When + rule.scan(); + + // Then + assertThat(alertsRaised, hasSize(0)); + } + + @Test + void shouldAlert_genericRDBMSError() throws Exception { + // Given + rule.setAlertThreshold( + AlertThreshold.LOW); // Generic are currently only checked at the low threshold + final String param = "param"; + final String normalValue = "test"; + final String originalParamErrorValue = + normalValue + SqlInjectionScanRule.SQL_CHECK_ERR[0]; + + final UrlParamValueHandler handler = + UrlParamValueHandler.builder() + .targetParam(param) + .whenParamValueIs(param) + .thenReturnHtml(normalValue) + .whenParamValueIs(originalParamErrorValue) + .thenReturnHtml("java.sql.SQLException") + .build(); + nano.addHandler(handler); + rule.init(getHttpMessage("/?param=" + normalValue), parent); + + // When + rule.scan(); + + // Then + assertThat(alertsRaised, hasSize(1)); + } + } + + @Nested + class UnionBasedSqlInjection { + @Test + void shouldAlert_RDBMSErrorMessage() throws Exception { + // Given + final String param = "param"; + final String normalValue = "test"; + final String unionValueString = + normalValue + SqlInjectionScanRule.SQL_UNION_APPENDAGES[0]; + + final UrlParamValueHandler handler = + UrlParamValueHandler.builder() + .targetParam(param) + .whenParamValueIs(param) + .thenReturnHtml(normalValue) + .whenParamValueIs(unionValueString) + .thenReturnHtml("You have an error in your SQL syntax") + .build(); + nano.addHandler(handler); + rule.init(getHttpMessage("/?param=" + normalValue), parent); + + // When + rule.scan(); + + // Then + assertThat(alertsRaised, hasSize(1)); + } + + @Test + void shouldNotAlert_nonErrorMessageResponse() throws Exception { + // Given + final String param = "param"; + final String normalValue = "test"; + final String unionValueString = + normalValue + SqlInjectionScanRule.SQL_UNION_APPENDAGES[0]; + + final UrlParamValueHandler handler = + UrlParamValueHandler.builder() + .targetParam(param) + .whenParamValueIs(param) + .thenReturnHtml(normalValue) + .whenParamValueIs(unionValueString) + .thenReturnHtml("This is not a sql error message") + .build(); + nano.addHandler(handler); + rule.init(getHttpMessage("/?param=" + normalValue), parent); + + // When + rule.scan(); + + // Then + assertThat(alertsRaised, hasSize(0)); + } + + @Test + void shouldNotRun_strengthLOW() throws Exception { + // Given + rule.setAttackStrength(AttackStrength.LOW); + final String param = "param"; + final String normalValue = "test"; + final String unionValueString = + normalValue + SqlInjectionScanRule.SQL_UNION_APPENDAGES[0]; + + final UrlParamValueHandler handler = + UrlParamValueHandler.builder() + .targetParam(param) + .whenParamValueIs(param) + .thenReturnHtml(normalValue) + .whenParamValueIs(unionValueString) + .thenReturnHtml("You have an error in your SQL syntax") + .build(); + nano.addHandler(handler); + rule.init(getHttpMessage("/?param=" + normalValue), parent); + + // When + rule.scan(); + + // Then + assertThat(alertsRaised, hasSize(0)); + } + } + private static class ExpressionBasedHandler extends NanoServerHandler { public enum Expression { From 366804e016dbd0600ea58f3e3898c1301f347590 Mon Sep 17 00:00:00 2001 From: FiveOFive Date: Thu, 3 Oct 2024 23:17:15 -0700 Subject: [PATCH 2/5] SqlInjectionScanRule unit tests for boolean based Signed-off-by: FiveOFive --- .../ascanrules/SqlInjectionScanRule.java | 24 +- .../SqlInjectionScanRuleUnitTest.java | 397 ++++++++++++++++-- 2 files changed, 388 insertions(+), 33 deletions(-) diff --git a/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRule.java b/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRule.java index 6cfa31a8e43..369e361f9e6 100644 --- a/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRule.java +++ b/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRule.java @@ -403,7 +403,7 @@ private static List asList(String... strings) { * *not* in the last where clause in a SQL query so as a result, the rest of the query needs to * be closed off with the comment. */ - private static final String[] SQL_LOGIC_AND_TRUE = { + static final String[] SQL_LOGIC_AND_TRUE = { " AND 1=1" + SQL_ONE_LINE_COMMENT, "' AND '1'='1'" + SQL_ONE_LINE_COMMENT, "\" AND \"1\"=\"1\"" + SQL_ONE_LINE_COMMENT, @@ -416,7 +416,7 @@ private static List asList(String... strings) { }; /** always false statement for comparison in boolean based SQL injection check */ - private static final String[] SQL_LOGIC_AND_FALSE = { + static final String[] SQL_LOGIC_AND_FALSE = { " AND 1=2" + SQL_ONE_LINE_COMMENT, "' AND '1'='2'" + SQL_ONE_LINE_COMMENT, "\" AND \"1\"=\"2\"" + SQL_ONE_LINE_COMMENT, @@ -433,7 +433,7 @@ private static List asList(String... strings) { * injection check Note that, if necessary, the code also tries a variant with the one-line * comment " -- " appended to the end. */ - private static final String[] SQL_LOGIC_OR_TRUE = { + static final String[] SQL_LOGIC_OR_TRUE = { " OR 1=1" + SQL_ONE_LINE_COMMENT, "' OR '1'='1'" + SQL_ONE_LINE_COMMENT, "\" OR \"1\"=\"1\"" + SQL_ONE_LINE_COMMENT, @@ -573,7 +573,7 @@ public void init() { doExpressionBased = true; doExpressionMaxRequests = 8; doBooleanBased = true; - doBooleanMaxRequests = 6; + doBooleanMaxRequests = 6; // will not run all the LIKE attacks.. these are done at high doUnionBased = true; doUnionMaxRequests = 5; doOrderByBased = false; @@ -588,8 +588,7 @@ public void init() { doExpressionBased = true; doExpressionMaxRequests = 16; doBooleanBased = true; - doBooleanMaxRequests = - 20; // will not run all the LIKE attacks.. these are done at insane.. + doBooleanMaxRequests = 20; doUnionBased = true; doUnionMaxRequests = 10; doOrderByBased = true; @@ -1988,7 +1987,18 @@ protected String stripOff(String body, String pattern) { return result; } - /** Replace body by stripping off pattern strings. */ + /** + * Replace body by stripping off pattern strings. + * + *

Stripping both the originalPattern and attackPattern prevents false negatives when the + * originalPattern is always part of the response. + * + *

For example: there is a website about cats and the response body is always "This is a page + * about cats. You submitted {value}". If the originalPattern is "cats", the stripped response + * is "This is a page about . You submitted ". When an attack payload is sent, such as "cats AND + * 1=1" if only the attackPattern is stripped, the stripped response becomes "This is a page + * about cats. You submitted ". So the original "cats" value needs to be stripped as well. + */ protected String stripOffOriginalAndAttackParam( String body, String originalPattern, String attackPattern) { String result = this.stripOff(this.stripOff(body, attackPattern), originalPattern); diff --git a/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRuleUnitTest.java b/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRuleUnitTest.java index bd343731e8a..b70f1d67a1d 100644 --- a/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRuleUnitTest.java +++ b/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRuleUnitTest.java @@ -30,8 +30,16 @@ import fi.iki.elonen.NanoHTTPD; import fi.iki.elonen.NanoHTTPD.IHTTPSession; import fi.iki.elonen.NanoHTTPD.Response; +import java.util.List; import java.util.Map; +import java.util.function.Function; +import java.util.stream.Stream; +import org.apache.commons.text.StringEscapeUtils; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; +import org.parosproxy.paros.core.scanner.AbstractPlugin; import org.parosproxy.paros.core.scanner.Alert; import org.parosproxy.paros.core.scanner.Plugin.AttackStrength; import org.zaproxy.addon.commonlib.CommonAlertTag; @@ -346,34 +354,371 @@ void shouldNotAlertIfMultConfirmationExpressionIsNotSuccessfulAndReflectedInResp assertThat(alertsRaised, hasSize(0)); } - @Test - void shouldAlertByBodyComparisonIgnoringXmlEscapedPayload() throws Exception { - // Given - String param = "topic"; - String normalPayload = "cats"; - String attackPayload = "cats' AND '1'='1' -- "; - String verificationPayload = "cats' AND '1'='2' -- "; - UrlParamValueHandler handler = - UrlParamValueHandler.builder() - .targetParam(param) - .whenParamValueIs(normalPayload) - .thenReturnHtml(normalPayload + ": A") - .whenParamValueIs(attackPayload) - .thenReturnHtml(escapeXml10(attackPayload + ": A")) - .whenParamValueIs(verificationPayload) - .thenReturnHtml(escapeXml10(verificationPayload + ": ")) - .build(); - nano.addHandler(handler); - rule.init(getHttpMessage("/?topic=" + normalPayload), parent); + static final List> ENCODING_FUNCTIONS = + List.of( + SqlInjectionScanRule::getURLEncode, + SqlInjectionScanRule::getHTMLEncode, + s -> SqlInjectionScanRule.getHTMLEncode(SqlInjectionScanRule.getURLEncode(s)), + StringEscapeUtils::escapeXml10, + s -> s // Make sure to test for no encoding as well + ); + + static Stream> reflectionEncodings() { + return ENCODING_FUNCTIONS.stream(); + } - // When - rule.scan(); + @Nested + class BooleanBasedSqlInjection { + + @Test + void shouldAlert_ANDTrueMatches_ANDFalseDoesNotMatch() throws Exception { + // Given + final String param = "param"; + final String normalValue = "payload"; + final String ANDTrueValue = normalValue + SqlInjectionScanRule.SQL_LOGIC_AND_TRUE[0]; + final String ANDFalseValue = normalValue + SqlInjectionScanRule.SQL_LOGIC_AND_FALSE[0]; + + final UrlParamValueHandler handler = + UrlParamValueHandler.builder() + .targetParam(param) + .whenParamValueIs(normalValue) + .thenReturnHtml("normal response") + .whenParamValueIs(ANDTrueValue) + .thenReturnHtml("normal response") + .whenParamValueIs(ANDFalseValue) + .thenReturnHtml(constructReflectedResponse("different response")) + .build(); + nano.addHandler(handler); + rule.init(getHttpMessage("/?param=" + normalValue), parent); + + // When + rule.scan(); + + // Then + assertThat(alertsRaised, hasSize(1)); + Alert actual = alertsRaised.get(0); + assertThat(actual.getParam(), is(equalTo(param))); + assertThat(actual.getAttack(), is(equalTo(ANDTrueValue))); + } - // Then - assertThat(alertsRaised, hasSize(1)); - Alert actual = alertsRaised.get(0); - assertThat(actual.getParam(), is(equalTo(param))); - assertThat(actual.getAttack(), is(equalTo(attackPayload))); + @Test + void shouldAlert_ANDTrueMatches_ANDFalseMatches_ORTrueDoesNotMatch() throws Exception { + // Given + final String param = "param"; + final String normalValue = "payload"; + final String ANDTrueValue = normalValue + SqlInjectionScanRule.SQL_LOGIC_AND_TRUE[0]; + final String ANDFalseValue = normalValue + SqlInjectionScanRule.SQL_LOGIC_AND_FALSE[0]; + final String ORTrueValue = normalValue + SqlInjectionScanRule.SQL_LOGIC_OR_TRUE[0]; + + final UrlParamValueHandler handler = + UrlParamValueHandler.builder() + .targetParam(param) + .whenParamValueIs(normalValue) + .thenReturnHtml("normal response") + .whenParamValueIs(ANDTrueValue) + .thenReturnHtml("normal response") + .whenParamValueIs(ANDFalseValue) + .thenReturnHtml(constructReflectedResponse("normal response")) + .whenParamValueIs(ORTrueValue) + .thenReturnHtml("different response") + .build(); + nano.addHandler(handler); + rule.init(getHttpMessage("/?param=" + normalValue), parent); + + // When + rule.scan(); + + // Then + assertThat(alertsRaised, hasSize(1)); + Alert actual = alertsRaised.get(0); + assertThat(actual.getParam(), is(equalTo(param))); + assertThat(actual.getAttack(), is(equalTo(ANDTrueValue))); + } + + @Test + void shouldNotAlert_ANDTrueMatches_ANDFalseMatches_ORTrueMatches() throws Exception { + // Given + final String param = "param"; + final String normalValue = "payload"; + final String ANDTrueValue = normalValue + SqlInjectionScanRule.SQL_LOGIC_AND_TRUE[0]; + final String ANDFalseValue = normalValue + SqlInjectionScanRule.SQL_LOGIC_AND_FALSE[0]; + final String ORTrueValue = normalValue + SqlInjectionScanRule.SQL_LOGIC_OR_TRUE[0]; + + final UrlParamValueHandler handler = + UrlParamValueHandler.builder() + .targetParam(param) + .whenParamValueIs(normalValue) + .thenReturnHtml("normal response") + .whenParamValueIs(ANDTrueValue) + .thenReturnHtml("normal response") + .whenParamValueIs(ANDFalseValue) + .thenReturnHtml("normal response") + .whenParamValueIs(ORTrueValue) + .thenReturnHtml("normal response") + .build(); + nano.addHandler(handler); + rule.init(getHttpMessage("/?param=" + normalValue), parent); + + // When + rule.scan(); + + // Then + assertThat(alertsRaised, hasSize(0)); + } + + @Test + void shouldNotAlert_ANDTrueDoesNotMatch() throws Exception { + // Given + final String param = "param"; + final String normalValue = "payload"; + final String ANDTrueValue = normalValue + SqlInjectionScanRule.SQL_LOGIC_AND_TRUE[0]; + + final UrlParamValueHandler handler = + UrlParamValueHandler.builder() + .targetParam(param) + .whenParamValueIs(normalValue) + .thenReturnHtml("normal response") + .whenParamValueIs(ANDTrueValue) + .thenReturnHtml("different response") + .build(); + nano.addHandler(handler); + rule.init(getHttpMessage("/?param=" + normalValue), parent); + + // When + rule.scan(); + + // Then + assertThat(alertsRaised, hasSize(0)); + } + + @ParameterizedTest + @MethodSource( + "org.zaproxy.zap.extension.ascanrules.SqlInjectionScanRuleUnitTest#reflectionEncodings") + void shouldAlert_encodedPayloadReflected(Function encodingFunction) + throws Exception { + final String param = "param"; + final String normalValue = "%test"; // Includes characters that will be encoded + final String ANDTrueValue = normalValue + SqlInjectionScanRule.SQL_LOGIC_AND_TRUE[0]; + final String ANDFalseValue = normalValue + SqlInjectionScanRule.SQL_LOGIC_AND_FALSE[0]; + + // Set up the positive case where normal and ANDTrue responses match but ANDFalse is + // different + final UrlParamValueHandler handler = + UrlParamValueHandler.builder() + .targetParam(param) + .whenParamValueIs(normalValue) + .thenReturnHtml(constructReflectedResponse(normalValue)) + .whenParamValueIs(ANDTrueValue) + .thenReturnHtml( + constructReflectedResponse(encodingFunction.apply(normalValue))) + .whenParamValueIs(ANDFalseValue) + .thenReturnHtml( + constructReflectedResponse(encodingFunction.apply(normalValue)) + + "something different") + .build(); + nano.addHandler(handler); + rule.init(getHttpMessage("/?param=" + normalValue), parent); + + // When + rule.scan(); + + // Then + assertThat(alertsRaised, hasSize(1)); + Alert actual = alertsRaised.get(0); + assertThat(actual.getParam(), is(equalTo(param))); + assertThat(actual.getAttack(), is(equalTo(ANDTrueValue))); + } + + @Test + void shouldAlert_valueReflectedMultipleTimes_andWithDifferentEncodings() throws Exception { + // Given + final String param = "param"; + final String normalValue = "%test"; // Includes characters that will be encoded + final String ANDTrueValue = normalValue + SqlInjectionScanRule.SQL_LOGIC_AND_TRUE[0]; + final String ANDFalseValue = normalValue + SqlInjectionScanRule.SQL_LOGIC_AND_FALSE[0]; + + // Set up the positive case where normal and ANDTrue responses match but ANDFalse is + // different + final UrlParamValueHandler handler = + UrlParamValueHandler.builder() + .targetParam(param) + .whenParamValueIs(normalValue) + .thenReturnHtml(constructReflectedResponse(normalValue)) + .whenParamValueIs(ANDTrueValue) + .thenReturnHtml( + constructReflectedResponse(escapeXml10(ANDTrueValue), 4)) + .whenParamValueIs(ANDFalseValue) + .thenReturnHtml( + constructReflectedResponse( + AbstractPlugin.getURLEncode(ANDFalseValue), 2) + + "something different") + .build(); + nano.addHandler(handler); + rule.init(getHttpMessage("/?param=" + normalValue), parent); + + // When + rule.scan(); + + // Then + assertThat(alertsRaised, hasSize(1)); + Alert actual = alertsRaised.get(0); + assertThat(actual.getParam(), is(equalTo(param))); + assertThat(actual.getAttack(), is(equalTo(ANDTrueValue))); + } + + @Test + void shouldNotAlert_responseIsSameForAllParameter_originalParameterIsAlwaysInResponse() + throws Exception { + // Given + final String param = "param"; + final String normalValue = "normal"; + final String ANDTrueValue = normalValue + SqlInjectionScanRule.SQL_LOGIC_AND_TRUE[0]; + final String ANDFalseValue = normalValue + SqlInjectionScanRule.SQL_LOGIC_AND_FALSE[0]; + final String ORTrueValue = normalValue + SqlInjectionScanRule.SQL_LOGIC_OR_TRUE[0]; + + final UrlParamValueHandler handler = + UrlParamValueHandler.builder() + .targetParam(param) + .whenParamValueIs(normalValue) + .thenReturnHtml(constructReflectedResponse(normalValue) + normalValue) + .whenParamValueIs(ANDTrueValue) + .thenReturnHtml(constructReflectedResponse(ANDTrueValue) + normalValue) + .whenParamValueIs(ANDFalseValue) + .thenReturnHtml(constructReflectedResponse(ANDFalseValue) + normalValue) + .whenParamValueIs(ORTrueValue) + .thenReturnHtml(constructReflectedResponse(ORTrueValue) + normalValue) + .build(); + nano.addHandler(handler); + rule.init(getHttpMessage("/?param=" + normalValue), parent); + + // When + rule.scan(); + + // Then + assertThat(alertsRaised, hasSize(0)); + } + + @Test + void shouldNotAlert_LIKEAttacks_StrengthMedium() throws Exception { + // Given + rule.setAttackStrength(AttackStrength.MEDIUM); + final String param = "param"; + final String normalValue = "payload"; + final String ANDTrueValue = + normalValue + + SqlInjectionScanRule.SQL_LOGIC_AND_TRUE[ + 6]; // 6 is the current index of the first LIKE payload + final String ANDFalseValue = + normalValue + + SqlInjectionScanRule.SQL_LOGIC_AND_FALSE[ + 6]; // 6 is the current index of the first LIKE payload + + // Set up the positive case where normal and ANDTrue responses match but ANDFalse is + // different + final UrlParamValueHandler handler = + UrlParamValueHandler.builder() + .targetParam(param) + .whenParamValueIs(normalValue) + .thenReturnHtml(constructReflectedResponse(normalValue)) + .whenParamValueIs(ANDTrueValue) + .thenReturnHtml(constructReflectedResponse(ANDTrueValue)) + .whenParamValueIs(ANDFalseValue) + .thenReturnHtml( + constructReflectedResponse("different from normal and ANDTrue")) + .build(); + nano.addHandler(handler); + rule.init(getHttpMessage("/?param=" + normalValue), parent); + + // When + rule.scan(); + + // Then + assertThat(alertsRaised, hasSize(0)); + } + + @Test + void shouldAlert_LIKEAttacks_StrengthHigh() throws Exception { + // Given + rule.setAttackStrength(AttackStrength.HIGH); + final String param = "param"; + final String normalValue = "payload"; + final String ANDTrueValue = + normalValue + + SqlInjectionScanRule.SQL_LOGIC_AND_TRUE[ + 6]; // 6 is the current index of the first LIKE payload + final String ANDFalseValue = + normalValue + + SqlInjectionScanRule.SQL_LOGIC_AND_FALSE[ + 6]; // 6 is the current index of the first LIKE payload + + // Set up the positive case where normal and ANDTrue responses match but ANDFalse is + // different + final UrlParamValueHandler handler = + UrlParamValueHandler.builder() + .targetParam(param) + .whenParamValueIs(normalValue) + .thenReturnHtml(constructReflectedResponse(normalValue)) + .whenParamValueIs(ANDTrueValue) + .thenReturnHtml(constructReflectedResponse(ANDTrueValue)) + .whenParamValueIs(ANDFalseValue) + .thenReturnHtml( + constructReflectedResponse("different from normal and ANDTrue")) + .build(); + nano.addHandler(handler); + rule.init(getHttpMessage("/?param=" + normalValue), parent); + + // When + rule.scan(); + + // Then + assertThat(alertsRaised, hasSize(1)); + Alert actual = alertsRaised.get(0); + assertThat(actual.getParam(), is(equalTo(param))); + assertThat(actual.getAttack(), is(equalTo(ANDTrueValue))); + } + + /** Build a short response that contains the payload reflected in some text */ + private String constructReflectedResponse(String payload) { + return constructReflectedResponse(payload, 1); + } + + private String constructReflectedResponse(String payload, int reflectionCount) { + String response = "foo "; + for (int i = 0; i < reflectionCount; i++) { + response += payload; + } + return response + " foo "; + } + + @Test + void shouldAlertByBodyComparisonIgnoringXmlEscapedPayload() throws Exception { + // Given + String param = "topic"; + String normalPayload = "cats"; + String attackPayload = "cats' AND '1'='1' -- "; + String verificationPayload = "cats' AND '1'='2' -- "; + UrlParamValueHandler handler = + UrlParamValueHandler.builder() + .targetParam(param) + .whenParamValueIs(normalPayload) + .thenReturnHtml(normalPayload + ": A") + .whenParamValueIs(attackPayload) + .thenReturnHtml(escapeXml10(attackPayload + ": A")) + .whenParamValueIs(verificationPayload) + .thenReturnHtml(escapeXml10(verificationPayload + ": ")) + .build(); + nano.addHandler(handler); + rule.init(getHttpMessage("/?topic=" + normalPayload), parent); + + // When + rule.scan(); + + // Then + assertThat(alertsRaised, hasSize(1)); + Alert actual = alertsRaised.get(0); + assertThat(actual.getParam(), is(equalTo(param))); + assertThat(actual.getAttack(), is(equalTo(attackPayload))); + } } private static class ExpressionBasedHandler extends NanoServerHandler { From c68f36c784f285367f2bb7b0a6bbf3d296e35378 Mon Sep 17 00:00:00 2001 From: FiveOFive Date: Mon, 2 Dec 2024 13:30:39 -0800 Subject: [PATCH 3/5] migrate sqli expression + boolean checks to ComparableResponse --- .../ascanrules/SqlInjectionScanRule.java | 429 +++++++----------- 1 file changed, 164 insertions(+), 265 deletions(-) diff --git a/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRule.java b/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRule.java index 0e6a3d843ae..dbf3c5ffcbd 100644 --- a/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRule.java +++ b/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRule.java @@ -19,9 +19,6 @@ */ package org.zaproxy.zap.extension.ascanrules; -import difflib.Delta; -import difflib.DiffUtils; -import difflib.Patch; import java.io.IOException; import java.net.SocketException; import java.net.URLDecoder; @@ -30,7 +27,6 @@ import java.util.Collections; import java.util.HashMap; import java.util.Iterator; -import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.regex.Matcher; @@ -47,6 +43,7 @@ import org.parosproxy.paros.network.HttpMessage; import org.zaproxy.addon.commonlib.CommonAlertTag; import org.zaproxy.addon.commonlib.PolicyTag; +import org.zaproxy.addon.commonlib.http.ComparableResponse; import org.zaproxy.zap.extension.authentication.ExtensionAuthentication; import org.zaproxy.zap.model.Context; import org.zaproxy.zap.model.Tech; @@ -911,8 +908,8 @@ private void testExpressionBasedSqlInjection(String param, String origParamValue return; // Something went wrong, no point continuing } - mResBodyNormalUnstripped = refreshedmessage.getResponseBody().toString(); - mResBodyNormalStripped = this.stripOff(mResBodyNormalUnstripped, origParamValue); + final ComparableResponse normalResponse = + new ComparableResponse(refreshedmessage, origParamValue); if (!sqlInjectionFoundForUrl && doExpressionBased @@ -939,6 +936,7 @@ private void testExpressionBasedSqlInjection(String param, String origParamValue String modifiedParamValueConfirmForAdd = String.valueOf(paramPlusThree) + "-2"; // Do the attack for ADD variant expressionBasedAttack( + normalResponse, param, origParamValue, modifiedParamValueForAdd, @@ -964,6 +962,7 @@ private void testExpressionBasedSqlInjection(String param, String origParamValue String.valueOf(paramMultFour) + "/2"; // Do the attack for MULT variant expressionBasedAttack( + normalResponse, param, origParamValue, modifiedParamValueForMult, @@ -1055,8 +1054,8 @@ private void testBooleanBasedSqlInjection(String param, String origParamValue) return; // Something went wrong, no point continuing } - mResBodyNormalUnstripped = refreshedmessage.getResponseBody().toString(); - mResBodyNormalStripped = this.stripOff(mResBodyNormalUnstripped, origParamValue); + final ComparableResponse normalResponse = + new ComparableResponse(refreshedmessage, origParamValue); // try each of the AND syntax values in turn. // Which one is successful will depend on the column type of the table/view column into @@ -1093,113 +1092,138 @@ private void testBooleanBasedSqlInjection(String param, String origParamValue) } countBooleanBasedRequests++; - String resBodyANDTrueUnstripped = msg2.getResponseBody().toString(); - String resBodyANDTrueStripped = - stripOffOriginalAndAttackParam( - resBodyANDTrueUnstripped, origParamValue, sqlBooleanAndTrueValue); + final ComparableResponse andTrueResponse = + new ComparableResponse(msg2, sqlBooleanAndTrueValue); - // set up two little arrays to ease the work of checking the unstripped output, and - // then the stripped output - String normalBodyOutput[] = {mResBodyNormalUnstripped, mResBodyNormalStripped}; - String andTrueBodyOutput[] = {resBodyANDTrueUnstripped, resBodyANDTrueStripped}; - boolean strippedOutput[] = {false, true}; + if (isStop()) { + LOGGER.debug("Stopping the scan due to a user request"); + return; + } - for (int booleanStrippedUnstrippedIndex = 0; - booleanStrippedUnstrippedIndex < 2; - booleanStrippedUnstrippedIndex++) { - if (isStop()) { - LOGGER.debug("Stopping the scan due to a user request"); - return; + // if the results of the "AND 1=1" match the original query, we may be onto something. + if (compareResponses(normalResponse, andTrueResponse) == 1) { + LOGGER.debug( + "Check 2, response for AND TRUE condition [{}] matched (refreshed) original results for {}", + sqlBooleanAndTrueValue, + refreshedmessage.getRequestHeader().getURI()); + // so they match. Was it a fluke? See if we get the same result by tacking on "AND 1 + // = 2" to the original + HttpMessage msg2_and_false = getNewMsg(); + + setParameter(msg2_and_false, param, sqlBooleanAndFalseValue); + + try { + sendAndReceive(msg2_and_false, false); // do not follow redirects + } catch (SocketException ex) { + LOGGER.debug( + "Caught {} {} when accessing: {}", + ex.getClass().getName(), + ex.getMessage(), + msg2_and_false.getRequestHeader().getURI()); + continue; // Something went wrong, continue on to the next item in the + // loop } + countBooleanBasedRequests++; - // if the results of the "AND 1=1" match the original query (using either the - // stripped or unstripped versions), we may be onto something. - if (andTrueBodyOutput[booleanStrippedUnstrippedIndex].compareTo( - normalBodyOutput[booleanStrippedUnstrippedIndex]) - == 0) { + final ComparableResponse andFalseResponse = + new ComparableResponse(msg2_and_false, sqlBooleanAndFalseValue); + + if (compareResponses(normalResponse, andFalseResponse) < 1) { LOGGER.debug( - "Check 2, {} html output for AND TRUE condition [{}] matched (refreshed) original results for {}", - (strippedOutput[booleanStrippedUnstrippedIndex] - ? "STRIPPED" - : "UNSTRIPPED"), - sqlBooleanAndTrueValue, + "Check 2, response output for AND FALSE condition [{}] differed from (refreshed) original results for {}", + sqlBooleanAndFalseValue, refreshedmessage.getRequestHeader().getURI()); - // so they match. Was it a fluke? See if we get the same result by tacking - // on "AND 1 = 2" to the original - HttpMessage msg2_and_false = getNewMsg(); - setParameter(msg2_and_false, param, sqlBooleanAndFalseValue); + // it's different (suggesting that the "AND 1 = 2" appended on gave + // different results because it restricted the data set to nothing + // Likely a SQL Injection. Raise it + String extraInfo = + Constant.messages.getString( + MESSAGE_PREFIX + "alert.booleanbased.extrainfo", + sqlBooleanAndTrueValue, + sqlBooleanAndFalseValue) + + "\n" + + Constant.messages.getString( + MESSAGE_PREFIX + + "alert.booleanbased.extrainfo.dataexists"); + // raise the alert, and save the attack string for the "Authentication + // Bypass" alert, if necessary + sqlInjectionAttack = sqlBooleanAndTrueValue; + newAlert() + .setConfidence(Alert.CONFIDENCE_MEDIUM) + .setParam(param) + .setAttack(sqlInjectionAttack) + .setOtherInfo(extraInfo) + .setMessage(msg2) + .raise(); + + sqlInjectionFoundForUrl = true; + + break; // No further need to loop through SQL_AND + + } else { + // the results of the always false condition are the same as for the + // original unmodified parameter + // this could be because there was *no* data returned for the original + // unmodified parameter + // so consider the effect of adding comments to both the always true + // condition, and the always false condition + // the first value to try.. + // ZAP: Removed getURLDecode() + String orValue = origParamValue + SQL_LOGIC_OR_TRUE[i]; + + // this is where that comment comes in handy: if the RDBMS supports + // one-line comments, add one in to attempt to ensure that the + // condition becomes one that is effectively always true, returning ALL + // data (or as much as possible), allowing us to pinpoint the SQL + // Injection + LOGGER.debug( + "Check 2 , response for AND FALSE condition [{}] SAME as (refreshed) original results for {} ### (forcing OR TRUE check)", + sqlBooleanAndFalseValue, + refreshedmessage.getRequestHeader().getURI()); + HttpMessage msg2_or_true = getNewMsg(); + setParameter(msg2_or_true, param, orValue); try { - sendAndReceive(msg2_and_false, false); // do not follow redirects + sendAndReceive(msg2_or_true, false); // do not follow redirects } catch (SocketException ex) { LOGGER.debug( "Caught {} {} when accessing: {}", ex.getClass().getName(), ex.getMessage(), - msg2_and_false.getRequestHeader().getURI()); - continue; // Something went wrong, continue on to the next item in the - // loop + msg2_or_true.getRequestHeader().getURI()); + continue; // Something went wrong, continue on to the next item in + // the loop } countBooleanBasedRequests++; - String resBodyANDFalseUnstripped = msg2_and_false.getResponseBody().toString(); - String resBodyANDFalseStripped = - stripOffOriginalAndAttackParam( - resBodyANDFalseUnstripped, - origParamValue, - sqlBooleanAndFalseValue); - - String andFalseBodyOutput[] = { - resBodyANDFalseUnstripped, resBodyANDFalseStripped - }; + final ComparableResponse orTrueResponse = + new ComparableResponse(msg2_or_true, orValue); - // which AND False output should we compare? the stripped or the unstripped - // version? - // depends on which one we used to get to here.. use the same as that.. - - // build an always false AND query. Result should be different to prove the - // SQL works. - if (andFalseBodyOutput[booleanStrippedUnstrippedIndex].compareTo( - normalBodyOutput[booleanStrippedUnstrippedIndex]) - != 0) { + if (compareResponses(normalResponse, orTrueResponse) < 1) { LOGGER.debug( - "Check 2, {} html output for AND FALSE condition [{}] differed from (refreshed) original results for {}", - (strippedOutput[booleanStrippedUnstrippedIndex] - ? "STRIPPED" - : "UNSTRIPPED"), - sqlBooleanAndFalseValue, + "Check 2, response for OR TRUE condition [{}] different to (refreshed) original results for {}", + orValue, refreshedmessage.getRequestHeader().getURI()); - // it's different (suggesting that the "AND 1 = 2" appended on gave - // different results because it restricted the data set to nothing + // it's different (suggesting that the "OR 1 = 1" appended on gave + // different results because it broadened the data set from nothing + // to something // Likely a SQL Injection. Raise it - String extraInfo = null; - if (strippedOutput[booleanStrippedUnstrippedIndex]) { - extraInfo = - Constant.messages.getString( - MESSAGE_PREFIX + "alert.booleanbased.extrainfo", - sqlBooleanAndTrueValue, - sqlBooleanAndFalseValue, - ""); - } else { - extraInfo = - Constant.messages.getString( - MESSAGE_PREFIX + "alert.booleanbased.extrainfo", - sqlBooleanAndTrueValue, - sqlBooleanAndFalseValue, - "NOT "); - } - extraInfo = - extraInfo + String extraInfo = + Constant.messages.getString( + MESSAGE_PREFIX + "alert.booleanbased.extrainfo", + sqlBooleanAndTrueValue, + orValue, + "") + "\n" + Constant.messages.getString( MESSAGE_PREFIX - + "alert.booleanbased.extrainfo.dataexists"); + + "alert.booleanbased.extrainfo.datanotexists"); - // raise the alert, and save the attack string for the "Authentication - // Bypass" alert, if necessary - sqlInjectionAttack = sqlBooleanAndTrueValue; + // raise the alert, and save the attack string for the + // "Authentication Bypass" alert, if necessary + sqlInjectionAttack = orValue; newAlert() .setConfidence(Alert.CONFIDENCE_MEDIUM) .setParam(param) @@ -1209,168 +1233,25 @@ private void testBooleanBasedSqlInjection(String param, String origParamValue) .raise(); sqlInjectionFoundForUrl = true; + // booleanBasedSqlInjectionFoundForParam = true; //causes us to + // skip past the other entries in SQL_AND. Only one will expose a + // vuln for a given param, since the database column is of only 1 + // type - break; // No further need to loop through SQL_AND - - } else { - // the results of the always false condition are the same as for the - // original unmodified parameter - // this could be because there was *no* data returned for the original - // unmodified parameter - // so consider the effect of adding comments to both the always true - // condition, and the always false condition - // the first value to try.. - // ZAP: Removed getURLDecode() - String orValue = origParamValue + SQL_LOGIC_OR_TRUE[i]; - - // this is where that comment comes in handy: if the RDBMS supports - // one-line comments, add one in to attempt to ensure that the - // condition becomes one that is effectively always true, returning ALL - // data (or as much as possible), allowing us to pinpoint the SQL - // Injection - LOGGER.debug( - "Check 2 , {} html output for AND FALSE condition [{}] SAME as (refreshed) original results for {} ### (forcing OR TRUE check)", - (strippedOutput[booleanStrippedUnstrippedIndex] - ? "STRIPPED" - : "UNSTRIPPED"), - sqlBooleanAndFalseValue, - refreshedmessage.getRequestHeader().getURI()); - HttpMessage msg2_or_true = getNewMsg(); - setParameter(msg2_or_true, param, orValue); - try { - sendAndReceive(msg2_or_true, false); // do not follow redirects - } catch (SocketException ex) { - LOGGER.debug( - "Caught {} {} when accessing: {}", - ex.getClass().getName(), - ex.getMessage(), - msg2_or_true.getRequestHeader().getURI()); - continue; // Something went wrong, continue on to the next item in - // the loop - } - countBooleanBasedRequests++; - - String resBodyORTrueUnstripped = msg2_or_true.getResponseBody().toString(); - String resBodyORTrueStripped = - stripOffOriginalAndAttackParam( - resBodyORTrueUnstripped, origParamValue, orValue); - - String orTrueBodyOutput[] = { - resBodyORTrueUnstripped, resBodyORTrueStripped - }; - - int compareOrToOriginal = - orTrueBodyOutput[booleanStrippedUnstrippedIndex].compareTo( - normalBodyOutput[booleanStrippedUnstrippedIndex]); - if (compareOrToOriginal != 0) { - LOGGER.debug( - "Check 2, {} html output for OR TRUE condition [{}] different to (refreshed) original results for {}", - (strippedOutput[booleanStrippedUnstrippedIndex] - ? "STRIPPED" - : "UNSTRIPPED"), - orValue, - refreshedmessage.getRequestHeader().getURI()); - - // it's different (suggesting that the "OR 1 = 1" appended on gave - // different results because it broadened the data set from nothing - // to something - // Likely a SQL Injection. Raise it - String extraInfo = null; - if (strippedOutput[booleanStrippedUnstrippedIndex]) { - extraInfo = - Constant.messages.getString( - MESSAGE_PREFIX + "alert.booleanbased.extrainfo", - sqlBooleanAndTrueValue, - orValue, - ""); - } else { - extraInfo = - Constant.messages.getString( - MESSAGE_PREFIX + "alert.booleanbased.extrainfo", - sqlBooleanAndTrueValue, - orValue, - "NOT "); - } - extraInfo = - extraInfo - + "\n" - + Constant.messages.getString( - MESSAGE_PREFIX - + "alert.booleanbased.extrainfo.datanotexists"); - - // raise the alert, and save the attack string for the - // "Authentication Bypass" alert, if necessary - sqlInjectionAttack = orValue; - newAlert() - .setConfidence(Alert.CONFIDENCE_MEDIUM) - .setParam(param) - .setAttack(sqlInjectionAttack) - .setOtherInfo(extraInfo) - .setMessage(msg2) - .raise(); - - sqlInjectionFoundForUrl = true; - // booleanBasedSqlInjectionFoundForParam = true; //causes us to - // skip past the other entries in SQL_AND. Only one will expose a - // vuln for a given param, since the database column is of only 1 - // type - - break; - } - } - } // if the results of the "AND 1=1" match the original query, we may be onto - // something. - else { - // the results of the "AND 1=1" do NOT match the original query, for - // whatever reason (no sql injection, or the web page is not stable) - if (this.debugEnabled) { - LOGGER.debug( - "Check 2, {} html output for AND condition [{}] does NOT match the (refreshed) original results for {}", - (strippedOutput[booleanStrippedUnstrippedIndex] - ? "STRIPPED" - : "UNSTRIPPED"), - sqlBooleanAndTrueValue, - refreshedmessage.getRequestHeader().getURI()); - Patch diffpatch = - DiffUtils.diff( - new LinkedList<>( - Arrays.asList( - normalBodyOutput[ - booleanStrippedUnstrippedIndex] - .split("\\n"))), - new LinkedList<>( - Arrays.asList( - andTrueBodyOutput[ - booleanStrippedUnstrippedIndex] - .split("\\n")))); - - // and convert the list of patches to a String, joining using a newline - StringBuilder tempDiff = new StringBuilder(250); - for (Delta delta : diffpatch.getDeltas()) { - String changeType = null; - if (delta.getType() == Delta.TYPE.CHANGE) { - changeType = "Changed Text"; - } else if (delta.getType() == Delta.TYPE.DELETE) { - changeType = "Deleted Text"; - } else if (delta.getType() == Delta.TYPE.INSERT) { - changeType = "Inserted text"; - } else { - changeType = "Unknown change type [" + delta.getType() + "]"; - } - - tempDiff.append("\n(" + changeType + ")\n"); // blank line before - tempDiff.append( - "Output for Unmodified parameter: " - + delta.getOriginal() - + "\n"); - tempDiff.append( - "Output for modified parameter: " - + delta.getRevised() - + "\n"); - } - LOGGER.debug("DIFFS: {}", tempDiff); + break; } } + } // if the results of the "AND 1=1" match the original query, we may be onto + // something. + else { + // the results of the "AND 1=1" do NOT match the original query, for + // whatever reason (no sql injection, or the web page is not stable) + if (this.debugEnabled) { + LOGGER.debug( + "Check 2, response for AND condition [{}] does NOT match the (refreshed) original results for {}", + sqlBooleanAndTrueValue, + refreshedmessage.getRequestHeader().getURI()); + } } } } @@ -1834,6 +1715,7 @@ private boolean checkUnionErrors( } private void expressionBasedAttack( + ComparableResponse normalResponse, String param, String originalParam, String modifiedParamValue, @@ -1856,19 +1738,16 @@ private void expressionBasedAttack( } countExpressionBasedRequests++; - String modifiedExpressionOutputUnstripped = msg.getResponseBody().toString(); - String modifiedExpressionOutputStripped = - stripOffOriginalAndAttackParam( - modifiedExpressionOutputUnstripped, originalParam, modifiedParamValue); - String normalBodyOutputStripped = stripOff(mResBodyNormalStripped, modifiedParamValue); + final ComparableResponse modifiedExpressionResponse = + new ComparableResponse(msg, modifiedParamValue); if (!sqlInjectionFoundForUrl && countExpressionBasedRequests < doExpressionMaxRequests) { // if the results of the modified request match the original query, we may be onto // something. - if (modifiedExpressionOutputStripped.compareTo(normalBodyOutputStripped) == 0) { + if (compareResponses(normalResponse, modifiedExpressionResponse) == 1) { LOGGER.debug( - "Check 4, STRIPPED html output for modified expression parameter [{}] matched (refreshed) original results for {}", + "Check 4, response for modified expression parameter [{}] matched (refreshed) original results for {}", modifiedParamValue, refreshedmessage.getRequestHeader().getURI()); // confirm that a different parameter value generates different output, to minimise @@ -1892,17 +1771,10 @@ private void expressionBasedAttack( } countExpressionBasedRequests++; - String confirmExpressionOutputUnstripped = msgConfirm.getResponseBody().toString(); - String confirmExpressionOutputStripped = - stripOffOriginalAndAttackParam( - confirmExpressionOutputUnstripped, - originalParam, - modifiedParamValueConfirm); + final ComparableResponse confirmExpressionResponse = + new ComparableResponse(msgConfirm, modifiedParamValueConfirm); - normalBodyOutputStripped = - stripOff(mResBodyNormalStripped, modifiedParamValueConfirm); - - if (confirmExpressionOutputStripped.compareTo(normalBodyOutputStripped) != 0) { + if (compareResponses(normalResponse, confirmExpressionResponse) < 1) { // the confirm query did not return the same results. This means that arbitrary // queries are not all producing the same page output. // this means the fact we earlier reproduced the original page output with a @@ -1936,6 +1808,33 @@ private void expressionBasedAttack( } } + /** + * 0 means very different and 1 very similar. Note that this is the opposite from most compareTo + * implementations but it matches the behavior of the compareWith function and heuristics in + * {@code ComparableResponse} + */ + private float compareResponses(ComparableResponse one, ComparableResponse two) { + return responseBodyHeuristic(one, two); + } + + /** + * Checks the response bodies of two requests for an exact match after stripping off the input + * parameters from both requests + */ + private float responseBodyHeuristic(ComparableResponse one, ComparableResponse two) { + final String stripped1 = + stripOffOriginalAndAttackParam( + one.getBody(), one.getValueSent(), two.getValueSent()); + final String stripped2 = + stripOffOriginalAndAttackParam( + two.getBody(), one.getValueSent(), two.getValueSent()); + if (stripped1.compareTo(stripped2) == 0) { + return 1; + } + + return 0; + } + @Override public int getRisk() { return Alert.RISK_HIGH; From bd346add1eacd413085494bf7b6e02716041cd01 Mon Sep 17 00:00:00 2001 From: FiveOFive Date: Mon, 2 Dec 2024 18:29:01 -0800 Subject: [PATCH 4/5] Fix sqli fp when responses are 3xx w/ same body but different locations --- .../ascanrules/SqlInjectionScanRule.java | 24 +++++- .../SqlInjectionScanRuleUnitTest.java | 76 +++++++++++++++++++ 2 files changed, 99 insertions(+), 1 deletion(-) diff --git a/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRule.java b/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRule.java index eed566ad260..81a15d6ac24 100644 --- a/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRule.java +++ b/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRule.java @@ -1808,13 +1808,21 @@ private void expressionBasedAttack( } } + private static final float HEURISTIC_WEIGHT = + .99f; // At this time the sqli tests just look for 0, 1, or anything in between, so the + + // exact value here doesn't matter. Anything between 0 and 1 works. + /** * 0 means very different and 1 very similar. Note that this is the opposite from most compareTo * implementations but it matches the behavior of the compareWith function and heuristics in * {@code ComparableResponse} */ private float compareResponses(ComparableResponse one, ComparableResponse two) { - return responseBodyHeuristic(one, two); + float total = 1f; + total *= locationHeaderHeuristic(one, two) * HEURISTIC_WEIGHT + (1 - HEURISTIC_WEIGHT); + total *= responseBodyHeuristic(one, two) * HEURISTIC_WEIGHT + (1 - HEURISTIC_WEIGHT); + return total; } /** @@ -1835,6 +1843,20 @@ private float responseBodyHeuristic(ComparableResponse one, ComparableResponse t return 0; } + private float locationHeaderHeuristic(ComparableResponse one, ComparableResponse two) { + if (one.getStatusCode() == two.getStatusCode() + && one.getStatusCode() >= 300 + && one.getStatusCode() < 400) { + final String locationHeaderOne = one.getHeaders().get("location"); + final String locationHeaderTwo = two.getHeaders().get("location"); + if (!locationHeaderOne.equals(locationHeaderTwo)) { + return 0; + } + } + + return 1; + } + @Override public int getRisk() { return Alert.RISK_HIGH; diff --git a/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRuleUnitTest.java b/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRuleUnitTest.java index fe0a306a098..fe6732c1746 100644 --- a/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRuleUnitTest.java +++ b/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/SqlInjectionScanRuleUnitTest.java @@ -27,13 +27,17 @@ import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.is; +import com.strobel.functions.Supplier; import fi.iki.elonen.NanoHTTPD; import fi.iki.elonen.NanoHTTPD.IHTTPSession; import fi.iki.elonen.NanoHTTPD.Response; +import fi.iki.elonen.NanoHTTPD.Response.Status; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.function.Function; import java.util.stream.Stream; +import org.apache.commons.collections.MapUtils; import org.apache.commons.text.StringEscapeUtils; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; @@ -362,6 +366,46 @@ void shouldNotAlertIfMultConfirmationExpressionIsNotSuccessfulAndReflectedInResp assertThat(alertsRaised, hasSize(0)); } + // False positive case - https://github.com/zaproxy/zaproxy/issues/8651 + @Test + void shouldNotAlertIfNormalAndModified301RedirectToDifferentLocations() throws Exception { + // Given + String param = "test"; + String normalPayload = "1"; + String attackPayload = "2/2"; + String verificationPayload = "4/2"; + Map> paramValueToResponseMap = new HashMap<>(); + paramValueToResponseMap.put( + normalPayload, + () -> { + final Response response = + newFixedLengthResponse(Status.REDIRECT, NanoHTTPD.MIME_HTML, "normal"); + response.addHeader("Location", "https://test.com/location_one"); + return response; + }); + paramValueToResponseMap.put( + attackPayload, + () -> { + final Response response = + newFixedLengthResponse(Status.REDIRECT, NanoHTTPD.MIME_HTML, "normal"); + response.addHeader("Location", "https://test.com/location_two"); + return response; + }); + paramValueToResponseMap.put( + verificationPayload, + () -> newFixedLengthResponse(Status.OK, NanoHTTPD.MIME_HTML, "text")); + ControlledStatusCodeHandler handler = + new ControlledStatusCodeHandler(param, paramValueToResponseMap); + nano.addHandler(handler); + rule.init(getHttpMessage("/?" + param + "=" + normalPayload), parent); + + // When + rule.scan(); + + // Then + assertThat(alertsRaised, hasSize(0)); + } + static final List> ENCODING_FUNCTIONS = List.of( SqlInjectionScanRule::getURLEncode, @@ -986,4 +1030,36 @@ protected String getContent(String value) { return "Some Content " + contentAddition; } } + + /** + * A test server that can respond with different status codes depending on the request payload + */ + private static class ControlledStatusCodeHandler extends NanoServerHandler { + private final String targetParam; + private final Map> + paramValueToResponseMap; // Supplier function because the test may send the same + // payload multiple times + private final Supplier fallbackResponse = + () -> newFixedLengthResponse(Status.OK, NanoHTTPD.MIME_HTML, ""); + + public ControlledStatusCodeHandler( + String targetParam, Map> paramValueToResponseMap) { + super("/"); + this.targetParam = targetParam; + this.paramValueToResponseMap = paramValueToResponseMap; + } + + @Override + protected Response serve(IHTTPSession session) { + String actualParamValue = getFirstParamValue(session, targetParam); + + @SuppressWarnings("unchecked") + final Supplier responseFn = + (Supplier) + MapUtils.getObject( + paramValueToResponseMap, actualParamValue, fallbackResponse); + final Response response = responseFn.get(); + return response; + } + } } From 364067d31213b825dbfc88842279f667130be276 Mon Sep 17 00:00:00 2001 From: FiveOFive Date: Mon, 2 Dec 2024 18:55:15 -0800 Subject: [PATCH 5/5] changelog --- addOns/ascanrules/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/addOns/ascanrules/CHANGELOG.md b/addOns/ascanrules/CHANGELOG.md index b61c8d72e2f..0cf81272fb3 100644 --- a/addOns/ascanrules/CHANGELOG.md +++ b/addOns/ascanrules/CHANGELOG.md @@ -9,6 +9,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Hidden File Finder - User Agent Fuzzer - Now depends on minimum Common Library version 1.29.0. +- Use location header in sql injection response comparisons (Issue 8651) ### Added - Standardized Scan Policy related alert tags on the rule.