diff --git a/addOns/ascanrules/CHANGELOG.md b/addOns/ascanrules/CHANGELOG.md index 8509d402c85..94588ed2527 100644 --- a/addOns/ascanrules/CHANGELOG.md +++ b/addOns/ascanrules/CHANGELOG.md @@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - SQL Injection - Hypersonic SQL - SQL Injection - MsSQL - SQL Injection - MySQL +- SQL Injection will now be more tolerant to redirects (Issue 6883). ### Added - Help entry for the Spring Actuators scan rule (missed during previous promotion). 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 15ef347bb3c..1a434b68193 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 @@ -35,6 +35,7 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; import org.apache.commons.httpclient.URI; +import org.apache.commons.lang3.StringUtils; import org.apache.commons.text.StringEscapeUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -43,6 +44,7 @@ import org.parosproxy.paros.core.scanner.AbstractAppParamPlugin; import org.parosproxy.paros.core.scanner.Alert; import org.parosproxy.paros.core.scanner.Category; +import org.parosproxy.paros.network.HttpHeader; import org.parosproxy.paros.network.HttpMessage; import org.zaproxy.addon.commonlib.CommonAlertTag; import org.zaproxy.zap.extension.authentication.ExtensionAuthentication; @@ -830,6 +832,8 @@ && matchBodyPattern(msg1, errorPattern, sb)) { // String mResBodyNormal = getBaseMsg().getResponseBody().toString(); mResBodyNormalUnstripped = refreshedmessage.getResponseBody().toString(); mResBodyNormalStripped = this.stripOff(mResBodyNormalUnstripped, origParamValue); + String locationHeaderNormal = + refreshedmessage.getResponseHeader().getHeader(HttpHeader.LOCATION); if (!sqlInjectionFoundForUrl && doExpressionBased @@ -863,7 +867,8 @@ && matchBodyPattern(msg1, errorPattern, sb)) { param, origParamValue, modifiedParamValueForAdd, - modifiedParamValueConfirmForAdd); + modifiedParamValueConfirmForAdd, + locationHeaderNormal); // bale out if we were asked nicely if (isStop()) { LOGGER.debug("Stopping the scan due to a user request"); @@ -888,7 +893,8 @@ && matchBodyPattern(msg1, errorPattern, sb)) { param, origParamValue, modifiedParamValueForMult, - modifiedParamValueConfirmForMult); + modifiedParamValueConfirmForMult, + locationHeaderNormal); // bale out if we were asked nicely if (isStop()) { LOGGER.debug("Stopping the scan due to a user request"); @@ -977,6 +983,9 @@ && matchBodyPattern(msg1, errorPattern, sb)) { mResBodyNormalUnstripped = refreshedmessage.getResponseBody().toString(); mResBodyNormalStripped = this.stripOff(mResBodyNormalUnstripped, origParamValue); + locationHeaderNormal = + refreshedmessage.getResponseHeader().getHeader(HttpHeader.LOCATION); + // boolean booleanBasedSqlInjectionFoundForParam = false; // try each of the AND syntax values in turn. @@ -1020,6 +1029,7 @@ && matchBodyPattern(msg1, errorPattern, sb)) { String resBodyANDTrueStripped = stripOffOriginalAndAttackParam( resBodyANDTrueUnstripped, origParamValue, sqlBooleanAndTrueValue); + String headerAndTrue = msg2.getResponseHeader().getHeader(HttpHeader.LOCATION); // set up two little arrays to ease the work of checking the unstripped output, and // then the stripped output @@ -1077,6 +1087,8 @@ && matchBodyPattern(msg1, errorPattern, sb)) { resBodyANDFalseUnstripped, origParamValue, sqlBooleanAndFalseValue); + String headerAndFalse = + msg2_and_false.getResponseHeader().getHeader(HttpHeader.LOCATION); String andFalseBodyOutput[] = { resBodyANDFalseUnstripped, resBodyANDFalseStripped @@ -1138,8 +1150,22 @@ && matchBodyPattern(msg1, errorPattern, sb)) { sqlInjectionFoundForUrl = true; - break; // No further need to loop through SQL_AND + break; // No further need to loop through header check + + } else if (StringUtils.compare(locationHeaderNormal, headerAndTrue) == 0 + && StringUtils.compare(headerAndTrue, headerAndFalse) != 0) { + // or we get redirected, likely an SQL Injection too + sqlInjectionAttack = sqlBooleanAndTrueValue; + newAlert() + .setConfidence(Alert.CONFIDENCE_MEDIUM) + .setParam(param) + .setAttack(sqlInjectionAttack) + .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 @@ -1186,6 +1212,8 @@ && matchBodyPattern(msg1, errorPattern, sb)) { String resBodyORTrueStripped = stripOffOriginalAndAttackParam( resBodyORTrueUnstripped, origParamValue, orValue); + String headerOrTrue = + msg2_or_true.getResponseHeader().getHeader(HttpHeader.LOCATION); String orTrueBodyOutput[] = { resBodyORTrueUnstripped, resBodyORTrueStripped @@ -1193,7 +1221,10 @@ && matchBodyPattern(msg1, errorPattern, sb)) { int compareOrToOriginal = orTrueBodyOutput[booleanStrippedUnstrippedIndex].compareTo( - normalBodyOutput[booleanStrippedUnstrippedIndex]); + normalBodyOutput[ + booleanStrippedUnstrippedIndex]) + | StringUtils.compare( + locationHeaderNormal, headerOrTrue); if (compareOrToOriginal != 0) { LOGGER.debug( "Check 2, {} html output for OR TRUE condition [{}] different to (refreshed) original results for {}", @@ -1346,12 +1377,14 @@ && matchBodyPattern(msg1, errorPattern, sb)) { countBooleanBasedRequests++; String resBodyORTrueUnstripped = msg2.getResponseBody().toString(); + String locationHeaderOrTrue = + msg2.getResponseHeader().getHeader(HttpHeader.LOCATION); // if the results of the "OR 1=1" exceed the original query (unstripped, by more - // than a 20% size difference, say), we may be onto something. + // than a 20% size difference, say) or got redirected, we may be onto something. // TODO: change the percentage difference threshold based on the alert threshold - if ((resBodyORTrueUnstripped.length() - > (mResBodyNormalUnstripped.length() * 1.2))) { + if ((resBodyORTrueUnstripped.length() > (mResBodyNormalUnstripped.length() * 1.2)) + || (StringUtils.compare(locationHeaderNormal, locationHeaderOrTrue) != 0)) { LOGGER.debug( "Check 2a, unstripped html output for OR TRUE condition [{}] produced sufficiently larger results than the original message", sqlBooleanOrTrueValue); @@ -1377,6 +1410,8 @@ && matchBodyPattern(msg1, errorPattern, sb)) { resBodyANDFalseUnstripped, origParamValue, sqlBooleanAndFalseValue); + String headerAndFalse = + msg2_and_false.getResponseHeader().getHeader(HttpHeader.LOCATION); // does the "AND 1=2" version produce the same as the original (for // stripped/unstripped versions) @@ -1384,7 +1419,12 @@ && matchBodyPattern(msg1, errorPattern, sb)) { resBodyANDFalseUnstripped.compareTo(mResBodyNormalUnstripped) == 0; boolean verificationUsingStripped = resBodyANDFalseStripped.compareTo(mResBodyNormalStripped) == 0; - if (verificationUsingUnstripped || verificationUsingStripped) { + boolean verificationUsingLocationHeader = + StringUtils.compare(headerAndFalse, locationHeaderOrTrue) != 0; + + if (verificationUsingUnstripped + || verificationUsingStripped + || verificationUsingLocationHeader) { LOGGER.debug( "Check 2, {} html output for AND FALSE condition [{}] matches the (refreshed) original results", (verificationUsingStripped ? "STRIPPED" : "UNSTRIPPED"), @@ -1398,7 +1438,7 @@ && matchBodyPattern(msg1, errorPattern, sb)) { sqlBooleanOrTrueValue, sqlBooleanAndFalseValue, ""); - } else { + } else if (verificationUsingUnstripped) { extraInfo = Constant.messages.getString( MESSAGE_PREFIX + "alert.booleanbased.extrainfo", @@ -1852,7 +1892,8 @@ private void expressionBasedAttack( String param, String originalParam, String modifiedParamValue, - String modifiedParamValueConfirm) + String modifiedParamValueConfirm, + String originalLocationHeader) throws IOException { // those of you still paying attention will note that if handled as expressions (such as by // a database), these represent the same value. @@ -1876,12 +1917,14 @@ private void expressionBasedAttack( stripOffOriginalAndAttackParam( modifiedExpressionOutputUnstripped, originalParam, modifiedParamValue); String normalBodyOutputStripped = stripOff(mResBodyNormalStripped, modifiedParamValue); + String locationHeaderModified = msg.getResponseHeader().getHeader(HttpHeader.LOCATION); 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 (modifiedExpressionOutputStripped.compareTo(normalBodyOutputStripped) == 0 + && StringUtils.compare(locationHeaderModified, originalLocationHeader) == 0) { LOGGER.debug( "Check 4, STRIPPED html output for modified expression parameter [{}] matched (refreshed) original results for {}", modifiedParamValue, @@ -1913,11 +1956,16 @@ private void expressionBasedAttack( confirmExpressionOutputUnstripped, originalParam, modifiedParamValueConfirm); + String confirmExpressionLocationHeader = + msg.getResponseHeader().getHeader(HttpHeader.LOCATION); normalBodyOutputStripped = stripOff(mResBodyNormalStripped, modifiedParamValueConfirm); - if (confirmExpressionOutputStripped.compareTo(normalBodyOutputStripped) != 0) { + if (confirmExpressionOutputStripped.compareTo(normalBodyOutputStripped) != 0 + || StringUtils.compare( + confirmExpressionLocationHeader, locationHeaderModified) + != 0) { // 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 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..81ef0fbc06f 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 @@ -22,6 +22,7 @@ import static fi.iki.elonen.NanoHTTPD.newFixedLengthResponse; import static org.apache.commons.text.StringEscapeUtils.escapeXml10; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.hasSize; @@ -164,6 +165,56 @@ void shouldNotTargetNonDbTechs() { assertThat(targets, is(equalTo(false))); } + @Test + void shouldAlertIfTrueExpressionsAreSuccessful() throws Exception { + // Given + String param = "id"; + nano.addHandler( + new ConditionBasedHandler( + "/", param, ConditionBasedHandler.Condition.AND_FALSE.expression)); + + rule.init( + getHttpMessage("GET", NanoHTTPD.MIME_HTML, "/?" + param + "=admin", "Some content"), + parent); + // When + rule.scan(); + // Then + assertThat(httpMessagesSent, hasSize(greaterThan(1))); + assertThat(alertsRaised, hasSize(1)); + assertThat(alertsRaised.get(0).getEvidence(), is(equalTo(""))); + assertThat(alertsRaised.get(0).getParam(), is(equalTo(param))); + assertThat( + alertsRaised.get(0).getAttack(), + is(containsString(ConditionBasedHandler.Condition.AND_TRUE.expression))); + assertThat(alertsRaised.get(0).getRisk(), is(equalTo(Alert.RISK_HIGH))); + assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_MEDIUM))); + } + + @Test + void shouldAlertIfFalseExpressionsAreSuccessful() throws Exception { + // Given + String param = "id"; + nano.addHandler( + new ConditionBasedHandler( + "/", param, ConditionBasedHandler.Condition.OR_TRUE.expression)); + + rule.init( + getHttpMessage("GET", NanoHTTPD.MIME_HTML, "/?" + param + "=admin", "Some content"), + parent); + // When + rule.scan(); + // Then + assertThat(httpMessagesSent, hasSize(greaterThan(1))); + assertThat(alertsRaised, hasSize(1)); + assertThat(alertsRaised.get(0).getEvidence(), is(equalTo(""))); + assertThat(alertsRaised.get(0).getParam(), is(equalTo(param))); + assertThat( + alertsRaised.get(0).getAttack(), + is(containsString(ConditionBasedHandler.Condition.OR_TRUE.expression))); + assertThat(alertsRaised.get(0).getRisk(), is(equalTo(Alert.RISK_HIGH))); + assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_MEDIUM))); + } + @Test void shouldAlertIfSumExpressionsAreSuccessful() throws Exception { // Given @@ -187,6 +238,38 @@ void shouldAlertIfSumExpressionsAreSuccessful() throws Exception { assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_MEDIUM))); } + @Test + void shouldAlertIfSumExpressionsAreRedirected() throws Exception { + // Given + String param = "id"; + nano.addHandler( + new ExpressionBasedHandler("/", param, ExpressionBasedHandler.Expression.SUM) { + @Override + protected Response getResponse(String value) { + Response response = + newFixedLengthResponse( + Response.Status.REDIRECT, NanoHTTPD.MIME_HTML, ""); + response.addHeader("Location", "http://somewhere"); + return response; + } + }); + rule.init( + getHttpMessage("/?" + param + "=" + ExpressionBasedHandler.Expression.SUM.value), + parent); + // When + rule.scan(); + // Then + assertThat(httpMessagesSent, hasSize(greaterThan(1))); + assertThat(alertsRaised, hasSize(1)); + assertThat(alertsRaised.get(0).getEvidence(), is(equalTo(""))); + assertThat(alertsRaised.get(0).getParam(), is(equalTo(param))); + assertThat( + alertsRaised.get(0).getAttack(), + is(equalTo(ExpressionBasedHandler.Expression.SUM.baseExpression))); + assertThat(alertsRaised.get(0).getRisk(), is(equalTo(Alert.RISK_HIGH))); + assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_MEDIUM))); + } + @Test void shouldAlertIfSumExpressionsAreSuccessfulAndReflectedInResponse() throws Exception { // Given @@ -278,6 +361,38 @@ void shouldAlertIfMultExpressionsAreSuccessful() throws Exception { assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_MEDIUM))); } + @Test + void shouldAlertIfMultExpressionsAreRedirected() throws Exception { + // Given + String param = "id"; + nano.addHandler( + new ExpressionBasedHandler("/", param, ExpressionBasedHandler.Expression.MULT) { + @Override + protected Response getResponse(String value) { + Response response = + newFixedLengthResponse( + Response.Status.REDIRECT, NanoHTTPD.MIME_HTML, ""); + response.addHeader("Location", "http://somewhere"); + return response; + } + }); + rule.init( + getHttpMessage("/?" + param + "=" + ExpressionBasedHandler.Expression.MULT.value), + parent); + // When + rule.scan(); + // Then + assertThat(httpMessagesSent, hasSize(greaterThan(1))); + assertThat(alertsRaised, hasSize(1)); + assertThat(alertsRaised.get(0).getEvidence(), is(equalTo(""))); + assertThat(alertsRaised.get(0).getParam(), is(equalTo(param))); + assertThat( + alertsRaised.get(0).getAttack(), + is(equalTo(ExpressionBasedHandler.Expression.MULT.baseExpression))); + assertThat(alertsRaised.get(0).getRisk(), is(equalTo(Alert.RISK_HIGH))); + assertThat(alertsRaised.get(0).getConfidence(), is(equalTo(Alert.CONFIDENCE_MEDIUM))); + } + @Test void shouldAlertIfMultExpressionsAreSuccessfulAndReflectedInResponse() throws Exception { // Given @@ -412,12 +527,12 @@ public ExpressionBasedHandler( } public ExpressionBasedHandler( - String parth, + String path, String param, Expression expression, boolean confirmationFails, String contentAddition) { - this(parth, param, expression, confirmationFails); + this(path, param, expression, confirmationFails); this.contentAddition = contentAddition; } @@ -425,8 +540,7 @@ public ExpressionBasedHandler( protected Response serve(IHTTPSession session) { String value = getFirstParamValue(session, param); if (isValidValue(value)) { - return newFixedLengthResponse( - Response.Status.OK, NanoHTTPD.MIME_HTML, getContent(value)); + return getResponse(value); } return newFixedLengthResponse( Response.Status.NOT_FOUND, NanoHTTPD.MIME_HTML, "404 Not Found"); @@ -442,5 +556,52 @@ private boolean isValidValue(String value) { protected String getContent(String value) { return "Some Content " + contentAddition; } + + protected Response getResponse(String value) { + return newFixedLengthResponse( + Response.Status.OK, NanoHTTPD.MIME_HTML, getContent(value)); + } + } + + private static class ConditionBasedHandler extends NanoServerHandler { + + public enum Condition { + AND_TRUE(" AND 1=1 --"), + AND_FALSE(" AND 1=2 --"), + OR_TRUE(" OR 1=1 --"); + + private final String expression; + + Condition(String expression) { + this.expression = expression; + } + } + + private final String param; + + private final String confirmationExpression; + + public ConditionBasedHandler(String name, String param, String confirmationExpression) { + super(name); + this.param = param; + this.confirmationExpression = confirmationExpression; + } + + @Override + protected Response serve(IHTTPSession session) { + String value = getFirstParamValue(session, param); + if (value.contains(confirmationExpression)) { + Response response = + newFixedLengthResponse( + Response.Status.REDIRECT, NanoHTTPD.MIME_HTML, getContent()); + response.addHeader("Location", "http://somewhere"); + return response; + } + return newFixedLengthResponse(Response.Status.OK, NanoHTTPD.MIME_HTML, getContent()); + } + + protected String getContent() { + return "Some content"; + } } }