From bc14d8275f44cc7544f465033fa3895382ea6df7 Mon Sep 17 00:00:00 2001 From: "mikhail.evtikhov" Date: Tue, 5 Mar 2024 16:44:04 +0300 Subject: [PATCH 1/6] Added threshold - low, for directory browsing detects in PathTraversal scan rule Signed-off-by: mikhail.evtikhov --- addOns/ascanrules/CHANGELOG.md | 4 +++- .../ascanrules/PathTraversalScanRule.java | 18 ++++++++++-------- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/addOns/ascanrules/CHANGELOG.md b/addOns/ascanrules/CHANGELOG.md index 751ca1828c6..fbf1c69de36 100644 --- a/addOns/ascanrules/CHANGELOG.md +++ b/addOns/ascanrules/CHANGELOG.md @@ -4,7 +4,9 @@ All notable changes to this add-on will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ## Unreleased - +### Added +- Threshold LOW is set for: + - Directory browsing detects in PathTraversal scan rule ## [63] - 2024-02-12 ### Changed diff --git a/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRule.java b/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRule.java index f210e86cee7..61026ef5871 100644 --- a/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRule.java +++ b/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRule.java @@ -360,14 +360,16 @@ public void scan(HttpMessage msg, String param, String value) { } // Check 3: Detect if this page is a directory browsing component - for (int h = 0; h < dirCount; h++) { - - // Check if a there was a finding or the scan has been stopped - // if yes dispose resources and exit - if (sendAndCheckPayload(param, LOCAL_DIR_TARGETS[h], DIR_PATTERN, 3) || isStop()) { - // Dispose all resources - // Exit the scan rule - return; + if (getAlertThreshold().equals(AlertThreshold.LOW)) { + for (int h = 0; h < dirCount; h++) { + + // Check if a there was a finding or the scan has been stopped + // if yes dispose resources and exit + if (sendAndCheckPayload(param, LOCAL_DIR_TARGETS[h], DIR_PATTERN, 3) || isStop()) { + // Dispose all resources + // Exit the scan rule + return; + } } } From 5b7244188a936f7fcca2439d48dfa25c5c83aedc Mon Sep 17 00:00:00 2001 From: "mikhail.evtikhov" Date: Tue, 5 Mar 2024 16:50:16 +0300 Subject: [PATCH 2/6] Added threshold - low, for expression based detects in SQLi scan rule Signed-off-by: mikhail.evtikhov --- addOns/ascanrules/CHANGELOG.md | 3 +- .../ascanrules/SqlInjectionScanRule.java | 152 +++++++++--------- 2 files changed, 79 insertions(+), 76 deletions(-) diff --git a/addOns/ascanrules/CHANGELOG.md b/addOns/ascanrules/CHANGELOG.md index fbf1c69de36..ceec8fd3d67 100644 --- a/addOns/ascanrules/CHANGELOG.md +++ b/addOns/ascanrules/CHANGELOG.md @@ -5,8 +5,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ## Unreleased ### Added -- Threshold LOW is set for: +- Threshold LOW is added for: - Directory browsing detects in PathTraversal scan rule + - Expression based detects in SQLi scan rule ## [63] - 2024-02-12 ### Changed 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..1ef89e78e35 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 @@ -813,97 +813,99 @@ && matchBodyPattern(msg1, errorPattern, sb)) { // query was last run (could be hours ago) // so to work around this, simply re-run the query again now at this point. // Note that we are not counting this request in our max number of requests to be issued - refreshedmessage = getNewMsg(); - try { - sendAndReceive(refreshedmessage, false); // do not follow redirects - } catch (SocketException ex) { - LOGGER.debug( - "Caught {} {} when accessing: {}", - ex.getClass().getName(), - ex.getMessage(), - refreshedmessage.getRequestHeader().getURI()); - return; // Something went wrong, no point continuing - } + if (getAlertThreshold().equals(AlertThreshold.LOW)) { + refreshedmessage = getNewMsg(); + try { + sendAndReceive(refreshedmessage, false); // do not follow redirects + } catch (SocketException ex) { + LOGGER.debug( + "Caught {} {} when accessing: {}", + ex.getClass().getName(), + ex.getMessage(), + refreshedmessage.getRequestHeader().getURI()); + return; // Something went wrong, no point continuing + } - // String mResBodyNormal = getBaseMsg().getResponseBody().toString(); - mResBodyNormalUnstripped = refreshedmessage.getResponseBody().toString(); - mResBodyNormalStripped = this.stripOff(mResBodyNormalUnstripped, origParamValue); + // String mResBodyNormal = getBaseMsg().getResponseBody().toString(); + mResBodyNormalUnstripped = refreshedmessage.getResponseBody().toString(); + mResBodyNormalStripped = this.stripOff(mResBodyNormalUnstripped, origParamValue); - if (!sqlInjectionFoundForUrl - && doExpressionBased - && countExpressionBasedRequests < doExpressionMaxRequests) { + if (!sqlInjectionFoundForUrl + && doExpressionBased + && countExpressionBasedRequests < doExpressionMaxRequests) { - // first figure out the type of the parameter.. - try { - // is it an integer type? - // ZAP: removed URLDecoding because on Variants - // int paramAsInt = - // Integer.parseInt(SqlInjectionScanRule.getURLDecode(origParamValue)); - int paramAsInt = Integer.parseInt(origParamValue); - - LOGGER.debug("The parameter value [{}] is of type Integer", origParamValue); - // This check is implemented using two variant PLUS(+) and MULT(*) + // first figure out the type of the parameter.. try { - // PLUS variant check the param value "3-2" gives same result as original - // request and param value "4-2" gives different result if original param - // value is 1 - // set the parameter value to a string value like "3-2", if the original - // parameter value was "1" - int paramPlusTwo = Math.addExact(paramAsInt, 2); - String modifiedParamValueForAdd = String.valueOf(paramPlusTwo) + "-2"; - // set the parameter value to a string value like "4-2", if the original - // parameter value was "1" - int paramPlusThree = Math.addExact(paramAsInt, 3); - String modifiedParamValueConfirmForAdd = - String.valueOf(paramPlusThree) + "-2"; - // Do the attack for ADD variant - expressionBasedAttack( - param, - origParamValue, - modifiedParamValueForAdd, - modifiedParamValueConfirmForAdd); - // bale out if we were asked nicely - if (isStop()) { - LOGGER.debug("Stopping the scan due to a user request"); - return; - } - // MULT variant check the param value "2/2" gives same result as original - // request and param value "4/2" gives different result if original param - // value is 1 - if (!sqlInjectionFoundForUrl - && countExpressionBasedRequests < doExpressionMaxRequests) { - // set the parameter value to a string value like "2/2", if the original + // is it an integer type? + // ZAP: removed URLDecoding because on Variants + // int paramAsInt = + // Integer.parseInt(SqlInjectionScanRule.getURLDecode(origParamValue)); + int paramAsInt = Integer.parseInt(origParamValue); + + LOGGER.debug("The parameter value [{}] is of type Integer", origParamValue); + // This check is implemented using two variant PLUS(+) and MULT(*) + try { + // PLUS variant check the param value "3-2" gives same result as original + // request and param value "4-2" gives different result if original param + // value is 1 + // set the parameter value to a string value like "3-2", if the original // parameter value was "1" - int paramMultTwo = Math.multiplyExact(paramAsInt, 2); - String modifiedParamValueForMult = String.valueOf(paramMultTwo) + "/2"; - // set the parameter value to a string value like "4/2", if the original + int paramPlusTwo = Math.addExact(paramAsInt, 2); + String modifiedParamValueForAdd = String.valueOf(paramPlusTwo) + "-2"; + // set the parameter value to a string value like "4-2", if the original // parameter value was "1" - int paramMultFour = Math.multiplyExact(paramAsInt, 4); - String modifiedParamValueConfirmForMult = - String.valueOf(paramMultFour) + "/2"; - // Do the attack for MULT variant + int paramPlusThree = Math.addExact(paramAsInt, 3); + String modifiedParamValueConfirmForAdd = + String.valueOf(paramPlusThree) + "-2"; + // Do the attack for ADD variant expressionBasedAttack( param, origParamValue, - modifiedParamValueForMult, - modifiedParamValueConfirmForMult); + modifiedParamValueForAdd, + modifiedParamValueConfirmForAdd); // bale out if we were asked nicely if (isStop()) { LOGGER.debug("Stopping the scan due to a user request"); return; } + // MULT variant check the param value "2/2" gives same result as original + // request and param value "4/2" gives different result if original param + // value is 1 + if (!sqlInjectionFoundForUrl + && countExpressionBasedRequests < doExpressionMaxRequests) { + // set the parameter value to a string value like "2/2", if the original + // parameter value was "1" + int paramMultTwo = Math.multiplyExact(paramAsInt, 2); + String modifiedParamValueForMult = String.valueOf(paramMultTwo) + "/2"; + // set the parameter value to a string value like "4/2", if the original + // parameter value was "1" + int paramMultFour = Math.multiplyExact(paramAsInt, 4); + String modifiedParamValueConfirmForMult = + String.valueOf(paramMultFour) + "/2"; + // Do the attack for MULT variant + expressionBasedAttack( + param, + origParamValue, + modifiedParamValueForMult, + modifiedParamValueConfirmForMult); + // bale out if we were asked nicely + if (isStop()) { + LOGGER.debug("Stopping the scan due to a user request"); + return; + } + } + } catch (ArithmeticException ex) { + LOGGER.debug( + "Caught {} {}. When performing integer math with the parameter value [{}]", + ex.getClass().getName(), + ex.getMessage(), + origParamValue); } - } catch (ArithmeticException ex) { - LOGGER.debug( - "Caught {} {}. When performing integer math with the parameter value [{}]", - ex.getClass().getName(), - ex.getMessage(), - origParamValue); + } catch (Exception e) { + LOGGER.debug("The parameter value [{}] is NOT of type Integer", origParamValue); + // TODO: implement a similar check for string types? This probably needs to be + // RDBMS specific (ie, it should not live in this scanner) } - } catch (Exception e) { - LOGGER.debug("The parameter value [{}] is NOT of type Integer", origParamValue); - // TODO: implement a similar check for string types? This probably needs to be - // RDBMS specific (ie, it should not live in this scanner) } } From fc81a357818ccff1fd9b651427ce3d8108a3cbeb Mon Sep 17 00:00:00 2001 From: "mikhail.evtikhov" Date: Tue, 5 Mar 2024 16:53:19 +0300 Subject: [PATCH 3/6] Gradlew spotlessApply code formatting Signed-off-by: mikhail.evtikhov --- .../ascanrules/PathTraversalScanRule.java | 3 ++- .../ascanrules/SqlInjectionScanRule.java | 27 ++++++++++++------- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRule.java b/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRule.java index 61026ef5871..9ec8ad82090 100644 --- a/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRule.java +++ b/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRule.java @@ -365,7 +365,8 @@ public void scan(HttpMessage msg, String param, String value) { // Check if a there was a finding or the scan has been stopped // if yes dispose resources and exit - if (sendAndCheckPayload(param, LOCAL_DIR_TARGETS[h], DIR_PATTERN, 3) || isStop()) { + if (sendAndCheckPayload(param, LOCAL_DIR_TARGETS[h], DIR_PATTERN, 3) + || isStop()) { // Dispose all resources // Exit the scan rule return; 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 1ef89e78e35..194543720db 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 @@ -845,8 +845,10 @@ && matchBodyPattern(msg1, errorPattern, sb)) { LOGGER.debug("The parameter value [{}] is of type Integer", origParamValue); // This check is implemented using two variant PLUS(+) and MULT(*) try { - // PLUS variant check the param value "3-2" gives same result as original - // request and param value "4-2" gives different result if original param + // PLUS variant check the param value "3-2" gives same result as + // original + // request and param value "4-2" gives different result if original + // param // value is 1 // set the parameter value to a string value like "3-2", if the original // parameter value was "1" @@ -868,16 +870,21 @@ && matchBodyPattern(msg1, errorPattern, sb)) { LOGGER.debug("Stopping the scan due to a user request"); return; } - // MULT variant check the param value "2/2" gives same result as original - // request and param value "4/2" gives different result if original param + // MULT variant check the param value "2/2" gives same result as + // original + // request and param value "4/2" gives different result if original + // param // value is 1 if (!sqlInjectionFoundForUrl && countExpressionBasedRequests < doExpressionMaxRequests) { - // set the parameter value to a string value like "2/2", if the original + // set the parameter value to a string value like "2/2", if the + // original // parameter value was "1" int paramMultTwo = Math.multiplyExact(paramAsInt, 2); - String modifiedParamValueForMult = String.valueOf(paramMultTwo) + "/2"; - // set the parameter value to a string value like "4/2", if the original + String modifiedParamValueForMult = + String.valueOf(paramMultTwo) + "/2"; + // set the parameter value to a string value like "4/2", if the + // original // parameter value was "1" int paramMultFour = Math.multiplyExact(paramAsInt, 4); String modifiedParamValueConfirmForMult = @@ -902,8 +909,10 @@ && matchBodyPattern(msg1, errorPattern, sb)) { origParamValue); } } catch (Exception e) { - LOGGER.debug("The parameter value [{}] is NOT of type Integer", origParamValue); - // TODO: implement a similar check for string types? This probably needs to be + LOGGER.debug( + "The parameter value [{}] is NOT of type Integer", origParamValue); + // TODO: implement a similar check for string types? This probably needs to + // be // RDBMS specific (ie, it should not live in this scanner) } } From 36f6e5600f9c2a90678974dde0e6b7160cd3d44c Mon Sep 17 00:00:00 2001 From: "mikhail.evtikhov" Date: Wed, 13 Mar 2024 18:52:34 +0300 Subject: [PATCH 4/6] Fixed errors in tests after adding thresholds Added FP test in Path Traversal Signed-off-by: mikhail.evtikhov --- .../PathTraversalScanRuleUnitTest.java | 60 ++++++++++++++- .../SqlInjectionScanRuleUnitTest.java | 76 ++++++++++++++----- 2 files changed, 114 insertions(+), 22 deletions(-) diff --git a/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRuleUnitTest.java b/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRuleUnitTest.java index acb7ed17b6a..0fa5c62824e 100644 --- a/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRuleUnitTest.java +++ b/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRuleUnitTest.java @@ -148,11 +148,16 @@ void shouldNotAlertIfAttackResponseDoesNotListDirectories() throws Exception { assertThat(alertsRaised, hasSize(0)); } - @Test - void shouldAlertIfAttackResponseListsWindowsDirectories() throws Exception { + @ParameterizedTest + @EnumSource( + value = Plugin.AlertThreshold.class, + names = {"LOW"}) + void shouldAlertIfAttackResponseListsWindowsDirectories(AlertThreshold alertThreshold) + throws Exception { // Given nano.addHandler(new ListWinDirsOnAttack("/", "p", "c:/")); rule.init(getHttpMessage("/?p=v"), parent); + rule.setAlertThreshold(alertThreshold); // When rule.scan(); // Then @@ -166,11 +171,39 @@ void shouldAlertIfAttackResponseListsWindowsDirectories() throws Exception { assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-3"))); } - @Test - void shouldAlertIfAttackResponseListsLinuxDirectories() throws Exception { + @ParameterizedTest + @EnumSource( + value = Plugin.AlertThreshold.class, + names = {"LOW"}) + void shouldAlertIfAttackResponseListsLinuxDirectories(AlertThreshold alertThreshold) + throws Exception { // Given nano.addHandler(new ListLinuxDirsOnAttack("/", "p", "/")); rule.init(getHttpMessage("/?p=v"), parent); + rule.setAlertThreshold(alertThreshold); + // When + rule.scan(); + // Then + assertThat(httpMessagesSent, hasSize(greaterThan(1))); + assertThat(alertsRaised, hasSize(1)); + assertThat(alertsRaised.get(0).getEvidence(), is(equalTo("etc"))); + assertThat(alertsRaised.get(0).getParam(), is(equalTo("p"))); + assertThat(alertsRaised.get(0).getAttack(), is(equalTo("/"))); + assertThat(alertsRaised.get(0).getRisk(), is(equalTo(Alert.RISK_HIGH))); + assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_MEDIUM))); + assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-3"))); + } + + @ParameterizedTest + @EnumSource( + value = Plugin.AlertThreshold.class, + names = {"LOW"}) + void shouldNotAlertIfAttackResponseListsHasFalsePositivePattern(AlertThreshold alertThreshold) + throws Exception { + // Given + nano.addHandler(new ListFalsePositiveDirsOnAttack("/", "p", "/")); + rule.init(getHttpMessage("/?p=v"), parent); + rule.setAlertThreshold(alertThreshold); // When rule.scan(); // Then @@ -384,6 +417,25 @@ protected String getDirs() { } } + private static class ListFalsePositiveDirsOnAttack extends ListDirsOnAttack { + + private static final String DIRS_LISTING = + "" + + "
" + // etc + + "
"; + + public ListFalsePositiveDirsOnAttack(String path, String param, String attack) { + super(path, param, attack); + } + + @Override + protected String getDirs() { + return DIRS_LISTING; + } + } + private static class ListBogusLinuxDirsOnAttack extends ListDirsOnAttack { private static final String DIRS_LISTING = 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..337611063cb 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 @@ -32,7 +32,11 @@ import fi.iki.elonen.NanoHTTPD.Response; import java.util.Map; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; import org.parosproxy.paros.core.scanner.Alert; +import org.parosproxy.paros.core.scanner.Plugin; +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; @@ -164,8 +168,11 @@ void shouldNotTargetNonDbTechs() { assertThat(targets, is(equalTo(false))); } - @Test - void shouldAlertIfSumExpressionsAreSuccessful() throws Exception { + @ParameterizedTest + @EnumSource( + value = Plugin.AlertThreshold.class, + names = {"LOW"}) + void shouldAlertIfSumExpressionsAreSuccessful(AlertThreshold alertThreshold) throws Exception { // Given String param = "id"; nano.addHandler( @@ -173,6 +180,7 @@ void shouldAlertIfSumExpressionsAreSuccessful() throws Exception { rule.init( getHttpMessage("/?" + param + "=" + ExpressionBasedHandler.Expression.SUM.value), parent); + rule.setAlertThreshold(alertThreshold); // When rule.scan(); // Then @@ -187,8 +195,12 @@ void shouldAlertIfSumExpressionsAreSuccessful() throws Exception { assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_MEDIUM))); } - @Test - void shouldAlertIfSumExpressionsAreSuccessfulAndReflectedInResponse() throws Exception { + @ParameterizedTest + @EnumSource( + value = Plugin.AlertThreshold.class, + names = {"LOW"}) + void shouldAlertIfSumExpressionsAreSuccessfulAndReflectedInResponse( + AlertThreshold alertThreshold) throws Exception { // Given String param = "id"; nano.addHandler( @@ -202,6 +214,7 @@ protected String getContent(String value) { rule.init( getHttpMessage("/?" + param + "=" + ExpressionBasedHandler.Expression.SUM.value), parent); + rule.setAlertThreshold(alertThreshold); // When rule.scan(); // Then @@ -216,8 +229,12 @@ protected String getContent(String value) { assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_MEDIUM))); } - @Test - void shouldNotAlertIfSumConfirmationExpressionIsNotSuccessful() throws Exception { + @ParameterizedTest + @EnumSource( + value = Plugin.AlertThreshold.class, + names = {"LOW"}) + void shouldNotAlertIfSumConfirmationExpressionIsNotSuccessful(AlertThreshold alertThreshold) + throws Exception { // Given String param = "id"; nano.addHandler( @@ -226,6 +243,7 @@ void shouldNotAlertIfSumConfirmationExpressionIsNotSuccessful() throws Exception rule.init( getHttpMessage("/?" + param + "=" + ExpressionBasedHandler.Expression.SUM.value), parent); + rule.setAlertThreshold(alertThreshold); // When rule.scan(); // Then @@ -233,9 +251,12 @@ void shouldNotAlertIfSumConfirmationExpressionIsNotSuccessful() throws Exception assertThat(alertsRaised, hasSize(0)); } - @Test - void shouldNotAlertIfSumConfirmationExpressionIsNotSuccessfulAndIsReflectedInResponse() - throws Exception { + @ParameterizedTest + @EnumSource( + value = Plugin.AlertThreshold.class, + names = {"LOW"}) + void shouldNotAlertIfSumConfirmationExpressionIsNotSuccessfulAndIsReflectedInResponse( + AlertThreshold alertThreshold) throws Exception { // Given String param = "id"; nano.addHandler( @@ -248,6 +269,7 @@ void shouldNotAlertIfSumConfirmationExpressionIsNotSuccessfulAndIsReflectedInRes rule.init( getHttpMessage("/?" + param + "=" + ExpressionBasedHandler.Expression.SUM.value), parent); + rule.setAlertThreshold(alertThreshold); // When rule.scan(); // Then @@ -255,8 +277,11 @@ void shouldNotAlertIfSumConfirmationExpressionIsNotSuccessfulAndIsReflectedInRes assertThat(alertsRaised, hasSize(0)); } - @Test - void shouldAlertIfMultExpressionsAreSuccessful() throws Exception { + @ParameterizedTest + @EnumSource( + value = Plugin.AlertThreshold.class, + names = {"LOW"}) + void shouldAlertIfMultExpressionsAreSuccessful(AlertThreshold alertThreshold) throws Exception { // Given String param = "id"; nano.addHandler( @@ -264,6 +289,7 @@ void shouldAlertIfMultExpressionsAreSuccessful() throws Exception { rule.init( getHttpMessage("/?" + param + "=" + ExpressionBasedHandler.Expression.MULT.value), parent); + rule.setAlertThreshold(alertThreshold); // When rule.scan(); // Then @@ -278,8 +304,12 @@ void shouldAlertIfMultExpressionsAreSuccessful() throws Exception { assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_MEDIUM))); } - @Test - void shouldAlertIfMultExpressionsAreSuccessfulAndReflectedInResponse() throws Exception { + @ParameterizedTest + @EnumSource( + value = Plugin.AlertThreshold.class, + names = {"LOW"}) + void shouldAlertIfMultExpressionsAreSuccessfulAndReflectedInResponse( + AlertThreshold alertThreshold) throws Exception { // Given String param = "id"; nano.addHandler( @@ -293,6 +323,7 @@ protected String getContent(String value) { rule.init( getHttpMessage("/?" + param + "=" + ExpressionBasedHandler.Expression.MULT.value), parent); + rule.setAlertThreshold(alertThreshold); // When rule.scan(); // Then @@ -307,8 +338,12 @@ protected String getContent(String value) { assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_MEDIUM))); } - @Test - void shouldNotAlertIfMultConfirmationExpressionIsNotSuccessful() throws Exception { + @ParameterizedTest + @EnumSource( + value = Plugin.AlertThreshold.class, + names = {"LOW"}) + void shouldNotAlertIfMultConfirmationExpressionIsNotSuccessful(AlertThreshold alertThreshold) + throws Exception { // Given String param = "id"; nano.addHandler( @@ -317,6 +352,7 @@ void shouldNotAlertIfMultConfirmationExpressionIsNotSuccessful() throws Exceptio rule.init( getHttpMessage("/?" + param + "=" + ExpressionBasedHandler.Expression.MULT.value), parent); + rule.setAlertThreshold(alertThreshold); // When rule.scan(); // Then @@ -324,9 +360,12 @@ void shouldNotAlertIfMultConfirmationExpressionIsNotSuccessful() throws Exceptio assertThat(alertsRaised, hasSize(0)); } - @Test - void shouldNotAlertIfMultConfirmationExpressionIsNotSuccessfulAndReflectedInResponse() - throws Exception { + @ParameterizedTest + @EnumSource( + value = Plugin.AlertThreshold.class, + names = {"LOW"}) + void shouldNotAlertIfMultConfirmationExpressionIsNotSuccessfulAndReflectedInResponse( + AlertThreshold alertThreshold) throws Exception { // Given String param = "id"; nano.addHandler( @@ -339,6 +378,7 @@ void shouldNotAlertIfMultConfirmationExpressionIsNotSuccessfulAndReflectedInResp rule.init( getHttpMessage("/?" + param + "=" + ExpressionBasedHandler.Expression.MULT.value), parent); + rule.setAlertThreshold(alertThreshold); // When rule.scan(); // Then From 9397943f8abd6379f95befcdd43cd9b1c9925592 Mon Sep 17 00:00:00 2001 From: "mikhail.evtikhov" Date: Fri, 29 Mar 2024 19:11:25 +0300 Subject: [PATCH 5/6] Checks for windows and nix directories are implemented with different functions Enhanced regex for nix architecture Signed-off-by: mikhail.evtikhov --- .../ascanrules/PathTraversalScanRule.java | 90 ++++++++++++------- .../PathTraversalScanRuleUnitTest.java | 35 ++------ 2 files changed, 66 insertions(+), 59 deletions(-) diff --git a/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRule.java b/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRule.java index 9ec8ad82090..ca133c846cd 100644 --- a/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRule.java +++ b/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRule.java @@ -145,19 +145,15 @@ public class PathTraversalScanRule extends AbstractAppParamPlugin * Windows/Unix/Linux/etc. local directory targets and detection pattern */ private static final ContentsMatcher DIR_PATTERN = new DirNamesContentsMatcher(); - private static final String[] LOCAL_DIR_TARGETS = { + private static final String[] WIN_LOCAL_DIR_TARGETS = { "c:/", - "/", "c:\\", - "../../../../../../../../../../../../../../../../", "..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\", - "/../../../../../../../../../../../../../../../../", "\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\", "file:///c:/", "file:///c:\\", "file:\\\\\\c:\\", "file:\\\\\\c:/", - "file:///", "file:\\\\\\", "d:\\", "d:/", @@ -166,6 +162,12 @@ public class PathTraversalScanRule extends AbstractAppParamPlugin "file:\\\\\\d:\\", "file:\\\\\\d:/" }; + private static final String[] NIX_LOCAL_DIR_TARGETS = { + "/", + "../../../../../../../../../../../../../../../../", + "/../../../../../../../../../../../../../../../../", + "file:///", + }; private static final ContentsMatcher WAR_PATTERN = new PatternContentsMatcher(Pattern.compile("")); @@ -223,7 +225,8 @@ public void scan(HttpMessage msg, String param, String value) { // figure out how aggressively we should test int nixCount = 0; int winCount = 0; - int dirCount = 0; + int nixDirCount = 0; + int winDirCount = 0; int localTraversalLength = 0; String extension = null; boolean includeNullByteInjectionPayload = false; @@ -233,33 +236,37 @@ public void scan(HttpMessage msg, String param, String value) { switch (this.getAttackStrength()) { case LOW: - // This works out as a total of 2+2+2+0*4+1 = 7 reqs / param + // This works out as a total of 2+2+1+1+0*4+1 = 7 reqs / param nixCount = 2; winCount = 2; - dirCount = 2; + nixDirCount = 1; + winDirCount = 1; break; case MEDIUM: - // This works out as a total of 2+4+4+1*4+1 = 15 reqs / param + // This works out as a total of 2+4+2+2+1*4+1 = 15 reqs / param nixCount = 2; winCount = 4; - dirCount = 4; + nixDirCount = 2; + winDirCount = 2; localTraversalLength = 1; break; case HIGH: - // This works out as a total of 4+8+7+2*4+1 = 28 reqs / param + // This works out as a total of 4+8+3+4+2*4+1 = 28 reqs / param nixCount = 4; winCount = 8; - dirCount = 7; + nixDirCount = 3; + winDirCount = 4; localTraversalLength = 2; break; case INSANE: - // This works out as a total of 6+18+19+4*4+1 = 60 reqs / param + // This works out as a total of 6+18+15+4+4*4+1 = 60 reqs / param nixCount = NIX_LOCAL_FILE_TARGETS.length; winCount = WIN_LOCAL_FILE_TARGETS.length; - dirCount = LOCAL_DIR_TARGETS.length; + nixDirCount = NIX_LOCAL_DIR_TARGETS.length; + winDirCount = WIN_LOCAL_DIR_TARGETS.length; localTraversalLength = 4; includeNullByteInjectionPayload = true; if (value != null) { @@ -360,17 +367,23 @@ public void scan(HttpMessage msg, String param, String value) { } // Check 3: Detect if this page is a directory browsing component - if (getAlertThreshold().equals(AlertThreshold.LOW)) { - for (int h = 0; h < dirCount; h++) { - - // Check if a there was a finding or the scan has been stopped - // if yes dispose resources and exit - if (sendAndCheckPayload(param, LOCAL_DIR_TARGETS[h], DIR_PATTERN, 3) - || isStop()) { - // Dispose all resources - // Exit the scan rule - return; - } + for (int h = 0; h < nixDirCount; h++) { + + // Check if a there was a finding or the scan has been stopped + // if yes dispose resources and exit + if (sendAndCheckPayload(param, NIX_LOCAL_DIR_TARGETS[h], DIR_PATTERN, 3) + || isStop()) { + // Dispose all resources + // Exit the scan rule + return; + } + } + for (int h = 0; h < winDirCount; h++) { + if (sendAndCheckPayload(param, WIN_LOCAL_DIR_TARGETS[h], DIR_PATTERN, 3) + || isStop()) { + // Dispose all resources + // Exit the scan rule + return; } } @@ -677,15 +690,30 @@ private static class DirNamesContentsMatcher implements ContentsMatcher { @Override public String match(String contents) { - if (contents.contains("etc") && contents.contains("bin") && contents.contains("boot")) { - Pattern nixDoubleCheckPattern = Pattern.compile("\\betc\\b"); - Matcher nixDoubleCheckMatcher = nixDoubleCheckPattern.matcher(contents); + String result = matchNixDirectories(contents); + if (result != null) { + return result; + } - if (nixDoubleCheckMatcher.find()) { - return "etc"; - } + return matchWinDirectories(contents); + } + + private String matchNixDirectories(String contents) { + Pattern binPattern = Pattern.compile("bin/?"); + Pattern etcPattern = Pattern.compile("etc/?"); + Pattern bootPattern = Pattern.compile("boot/?"); + + Matcher binMatcher = binPattern.matcher(contents); + Matcher etcMatcher = etcPattern.matcher(contents); + Matcher bootMatcher = bootPattern.matcher(contents); + if (binMatcher.find() && etcMatcher.find() && bootMatcher.find()) { + return "etc"; } + return null; + } + + private String matchWinDirectories(String contents) { if (contents.contains("Windows") && Pattern.compile("Program\\sFiles").matcher(contents).find()) { return "Windows"; diff --git a/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRuleUnitTest.java b/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRuleUnitTest.java index 0fa5c62824e..625738fbdd1 100644 --- a/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRuleUnitTest.java +++ b/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRuleUnitTest.java @@ -148,16 +148,11 @@ void shouldNotAlertIfAttackResponseDoesNotListDirectories() throws Exception { assertThat(alertsRaised, hasSize(0)); } - @ParameterizedTest - @EnumSource( - value = Plugin.AlertThreshold.class, - names = {"LOW"}) - void shouldAlertIfAttackResponseListsWindowsDirectories(AlertThreshold alertThreshold) - throws Exception { + @Test + void shouldAlertIfAttackResponseListsWindowsDirectories() throws Exception { // Given nano.addHandler(new ListWinDirsOnAttack("/", "p", "c:/")); rule.init(getHttpMessage("/?p=v"), parent); - rule.setAlertThreshold(alertThreshold); // When rule.scan(); // Then @@ -171,16 +166,11 @@ void shouldAlertIfAttackResponseListsWindowsDirectories(AlertThreshold alertThre assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-3"))); } - @ParameterizedTest - @EnumSource( - value = Plugin.AlertThreshold.class, - names = {"LOW"}) - void shouldAlertIfAttackResponseListsLinuxDirectories(AlertThreshold alertThreshold) - throws Exception { + @Test + void shouldAlertIfAttackResponseListsLinuxDirectories() throws Exception { // Given nano.addHandler(new ListLinuxDirsOnAttack("/", "p", "/")); rule.init(getHttpMessage("/?p=v"), parent); - rule.setAlertThreshold(alertThreshold); // When rule.scan(); // Then @@ -194,27 +184,16 @@ void shouldAlertIfAttackResponseListsLinuxDirectories(AlertThreshold alertThresh assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-3"))); } - @ParameterizedTest - @EnumSource( - value = Plugin.AlertThreshold.class, - names = {"LOW"}) - void shouldNotAlertIfAttackResponseListsHasFalsePositivePattern(AlertThreshold alertThreshold) - throws Exception { + @Test + void shouldNotAlertIfAttackResponseListsHasFalsePositivePattern() throws Exception { // Given nano.addHandler(new ListFalsePositiveDirsOnAttack("/", "p", "/")); rule.init(getHttpMessage("/?p=v"), parent); - rule.setAlertThreshold(alertThreshold); // When rule.scan(); // Then assertThat(httpMessagesSent, hasSize(greaterThan(1))); - assertThat(alertsRaised, hasSize(1)); - assertThat(alertsRaised.get(0).getEvidence(), is(equalTo("etc"))); - assertThat(alertsRaised.get(0).getParam(), is(equalTo("p"))); - assertThat(alertsRaised.get(0).getAttack(), is(equalTo("/"))); - assertThat(alertsRaised.get(0).getRisk(), is(equalTo(Alert.RISK_HIGH))); - assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_MEDIUM))); - assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-3"))); + assertThat(alertsRaised, hasSize(0)); } @Test From c6356272f7f036ca88f07e0e02a87ca6dcce59fe Mon Sep 17 00:00:00 2001 From: "mikhail.evtikhov" Date: Fri, 7 Jun 2024 19:03:24 +0300 Subject: [PATCH 6/6] Everything that is implemented in PR 5387 has been removed Signed-off-by: mikhail.evtikhov --- addOns/ascanrules/CHANGELOG.md | 1 - .../ascanrules/PathTraversalScanRule.java | 75 ++++++------------- .../PathTraversalScanRuleUnitTest.java | 31 -------- 3 files changed, 22 insertions(+), 85 deletions(-) diff --git a/addOns/ascanrules/CHANGELOG.md b/addOns/ascanrules/CHANGELOG.md index ceec8fd3d67..d5949c6417d 100644 --- a/addOns/ascanrules/CHANGELOG.md +++ b/addOns/ascanrules/CHANGELOG.md @@ -6,7 +6,6 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ## Unreleased ### Added - Threshold LOW is added for: - - Directory browsing detects in PathTraversal scan rule - Expression based detects in SQLi scan rule ## [63] - 2024-02-12 diff --git a/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRule.java b/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRule.java index ca133c846cd..f210e86cee7 100644 --- a/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRule.java +++ b/addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRule.java @@ -145,15 +145,19 @@ public class PathTraversalScanRule extends AbstractAppParamPlugin * Windows/Unix/Linux/etc. local directory targets and detection pattern */ private static final ContentsMatcher DIR_PATTERN = new DirNamesContentsMatcher(); - private static final String[] WIN_LOCAL_DIR_TARGETS = { + private static final String[] LOCAL_DIR_TARGETS = { "c:/", + "/", "c:\\", + "../../../../../../../../../../../../../../../../", "..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\", + "/../../../../../../../../../../../../../../../../", "\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\", "file:///c:/", "file:///c:\\", "file:\\\\\\c:\\", "file:\\\\\\c:/", + "file:///", "file:\\\\\\", "d:\\", "d:/", @@ -162,12 +166,6 @@ public class PathTraversalScanRule extends AbstractAppParamPlugin "file:\\\\\\d:\\", "file:\\\\\\d:/" }; - private static final String[] NIX_LOCAL_DIR_TARGETS = { - "/", - "../../../../../../../../../../../../../../../../", - "/../../../../../../../../../../../../../../../../", - "file:///", - }; private static final ContentsMatcher WAR_PATTERN = new PatternContentsMatcher(Pattern.compile("")); @@ -225,8 +223,7 @@ public void scan(HttpMessage msg, String param, String value) { // figure out how aggressively we should test int nixCount = 0; int winCount = 0; - int nixDirCount = 0; - int winDirCount = 0; + int dirCount = 0; int localTraversalLength = 0; String extension = null; boolean includeNullByteInjectionPayload = false; @@ -236,37 +233,33 @@ public void scan(HttpMessage msg, String param, String value) { switch (this.getAttackStrength()) { case LOW: - // This works out as a total of 2+2+1+1+0*4+1 = 7 reqs / param + // This works out as a total of 2+2+2+0*4+1 = 7 reqs / param nixCount = 2; winCount = 2; - nixDirCount = 1; - winDirCount = 1; + dirCount = 2; break; case MEDIUM: - // This works out as a total of 2+4+2+2+1*4+1 = 15 reqs / param + // This works out as a total of 2+4+4+1*4+1 = 15 reqs / param nixCount = 2; winCount = 4; - nixDirCount = 2; - winDirCount = 2; + dirCount = 4; localTraversalLength = 1; break; case HIGH: - // This works out as a total of 4+8+3+4+2*4+1 = 28 reqs / param + // This works out as a total of 4+8+7+2*4+1 = 28 reqs / param nixCount = 4; winCount = 8; - nixDirCount = 3; - winDirCount = 4; + dirCount = 7; localTraversalLength = 2; break; case INSANE: - // This works out as a total of 6+18+15+4+4*4+1 = 60 reqs / param + // This works out as a total of 6+18+19+4*4+1 = 60 reqs / param nixCount = NIX_LOCAL_FILE_TARGETS.length; winCount = WIN_LOCAL_FILE_TARGETS.length; - nixDirCount = NIX_LOCAL_DIR_TARGETS.length; - winDirCount = WIN_LOCAL_DIR_TARGETS.length; + dirCount = LOCAL_DIR_TARGETS.length; localTraversalLength = 4; includeNullByteInjectionPayload = true; if (value != null) { @@ -367,20 +360,11 @@ public void scan(HttpMessage msg, String param, String value) { } // Check 3: Detect if this page is a directory browsing component - for (int h = 0; h < nixDirCount; h++) { + for (int h = 0; h < dirCount; h++) { // Check if a there was a finding or the scan has been stopped // if yes dispose resources and exit - if (sendAndCheckPayload(param, NIX_LOCAL_DIR_TARGETS[h], DIR_PATTERN, 3) - || isStop()) { - // Dispose all resources - // Exit the scan rule - return; - } - } - for (int h = 0; h < winDirCount; h++) { - if (sendAndCheckPayload(param, WIN_LOCAL_DIR_TARGETS[h], DIR_PATTERN, 3) - || isStop()) { + if (sendAndCheckPayload(param, LOCAL_DIR_TARGETS[h], DIR_PATTERN, 3) || isStop()) { // Dispose all resources // Exit the scan rule return; @@ -690,30 +674,15 @@ private static class DirNamesContentsMatcher implements ContentsMatcher { @Override public String match(String contents) { - String result = matchNixDirectories(contents); - if (result != null) { - return result; - } - - return matchWinDirectories(contents); - } + if (contents.contains("etc") && contents.contains("bin") && contents.contains("boot")) { + Pattern nixDoubleCheckPattern = Pattern.compile("\\betc\\b"); + Matcher nixDoubleCheckMatcher = nixDoubleCheckPattern.matcher(contents); - private String matchNixDirectories(String contents) { - Pattern binPattern = Pattern.compile("bin/?"); - Pattern etcPattern = Pattern.compile("etc/?"); - Pattern bootPattern = Pattern.compile("boot/?"); - - Matcher binMatcher = binPattern.matcher(contents); - Matcher etcMatcher = etcPattern.matcher(contents); - Matcher bootMatcher = bootPattern.matcher(contents); - if (binMatcher.find() && etcMatcher.find() && bootMatcher.find()) { - return "etc"; + if (nixDoubleCheckMatcher.find()) { + return "etc"; + } } - return null; - } - - private String matchWinDirectories(String contents) { if (contents.contains("Windows") && Pattern.compile("Program\\sFiles").matcher(contents).find()) { return "Windows"; diff --git a/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRuleUnitTest.java b/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRuleUnitTest.java index 625738fbdd1..acb7ed17b6a 100644 --- a/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRuleUnitTest.java +++ b/addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/PathTraversalScanRuleUnitTest.java @@ -184,18 +184,6 @@ void shouldAlertIfAttackResponseListsLinuxDirectories() throws Exception { assertThat(alertsRaised.get(0).getAlertRef(), is(equalTo("6-3"))); } - @Test - void shouldNotAlertIfAttackResponseListsHasFalsePositivePattern() throws Exception { - // Given - nano.addHandler(new ListFalsePositiveDirsOnAttack("/", "p", "/")); - rule.init(getHttpMessage("/?p=v"), parent); - // When - rule.scan(); - // Then - assertThat(httpMessagesSent, hasSize(greaterThan(1))); - assertThat(alertsRaised, hasSize(0)); - } - @Test void shouldNotAlertIfAttackResponseListsBogusLinuxDirectories() throws Exception { // Given @@ -396,25 +384,6 @@ protected String getDirs() { } } - private static class ListFalsePositiveDirsOnAttack extends ListDirsOnAttack { - - private static final String DIRS_LISTING = - "" - + "
" - // etc - + "
"; - - public ListFalsePositiveDirsOnAttack(String path, String param, String attack) { - super(path, param, attack); - } - - @Override - protected String getDirs() { - return DIRS_LISTING; - } - } - private static class ListBogusLinuxDirsOnAttack extends ListDirsOnAttack { private static final String DIRS_LISTING =