Add checking for location header

koneko096 · koneko096 · commit 89c1c33c5f9c · 2022-04-16T21:07:23.000+07:00
In case of redirection after authentication, the body may be empty and content comparison won't work. By adding new checking for location header, we may catch potential vulnerability especially in case of OR true injection.

Signed-off-by: koneko096 &lt;laser.survivor@gmail.com&gt;
diff --git a/addOns/ascanrules/CHANGELOG.md b/addOns/ascanrules/CHANGELOG.md
@@ -10,6 +10,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 ### Fixed
 - The Remote File Inclusion scan rule no longer follows redirects before checking the response for content indicating a vulnerability (Issue 5887).
 - False positive where Cross Site Scripting payloads are safely rendered in a textarea tag.
+- SQL Injection will now recognize authentication bypass if redirected to different page (Issue 6883).
 
 ## [46] - 2022-03-21
 ### 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
@@ -36,13 +36,15 @@
 import java.util.regex.Pattern;
 import org.apache.commons.httpclient.InvalidRedirectLocationException;
 import org.apache.commons.httpclient.URI;
+import org.apache.commons.lang3.StringUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.parosproxy.paros.Constant;
 import org.parosproxy.paros.control.Control;
 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;
@@ -937,6 +939,9 @@ && matchBodyPattern(msg1, errorPattern, sb)) {
             mResBodyNormalUnstripped = refreshedmessage.getResponseBody().toString();
             mResBodyNormalStripped = this.stripOff(mResBodyNormalUnstripped, origParamValue);
 
+            String locationHeaderNormal =
+                    refreshedmessage.getResponseHeader().getHeader(HttpHeader.LOCATION);
+
             // boolean booleanBasedSqlInjectionFoundForParam = false;
 
             // try each of the AND syntax values in turn.
@@ -980,6 +985,8 @@ && 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
@@ -1037,6 +1044,8 @@ && matchBodyPattern(msg1, errorPattern, sb)) {
                                         resBodyANDFalseUnstripped,
                                         origParamValue,
                                         sqlBooleanAndFalseValue);
+                        String headerANDFalse =
+                                msg2_and_false.getResponseHeader().getHeader(HttpHeader.LOCATION);
 
                         String andFalseBodyOutput[] = {
                             resBodyANDFalseUnstripped, resBodyANDFalseStripped
@@ -1098,8 +1107,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
@@ -1146,14 +1169,17 @@ && matchBodyPattern(msg1, errorPattern, sb)) {
                             String resBodyORTrueStripped =
                                     stripOffOriginalAndAttackParam(
                                             resBodyORTrueUnstripped, origParamValue, orValue);
+                            String headerORTrue =
+                                    msg2_or_true.getResponseHeader().getHeader(HttpHeader.LOCATION);
 
                             String orTrueBodyOutput[] = {
                                 resBodyORTrueUnstripped, resBodyORTrueStripped
                             };
 
                             int compareOrToOriginal =
                                     orTrueBodyOutput[booleanStrippedUnstrippedIndex].compareTo(
-                                            normalBodyOutput[booleanStrippedUnstrippedIndex]);
+                                            normalBodyOutput[booleanStrippedUnstrippedIndex]) |
+                                    StringUtils.compare(locationHeaderNormal, headerORTrue);
                             if (compareOrToOriginal != 0) {
                                 log.debug(
                                         "Check 2, {} html output for OR TRUE condition [{}] different to (refreshed) original results for {}",
@@ -1306,12 +1332,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)) {
                     log.debug(
                             "Check 2a, unstripped html output for OR TRUE condition [{}] produced sufficiently larger results than the original message",
                             sqlBooleanOrTrueValue);
@@ -1337,14 +1365,21 @@ && 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)
                     boolean verificationUsingUnstripped =
                             resBodyANDFalseUnstripped.compareTo(mResBodyNormalUnstripped) == 0;
                     boolean verificationUsingStripped =
                             resBodyANDFalseStripped.compareTo(mResBodyNormalStripped) == 0;
-                    if (verificationUsingUnstripped || verificationUsingStripped) {
+                    boolean verificationUsingLocationHeader =
+                            StringUtils.compare(headerANDFalse, locationHeaderORTrue) != 0;
+
+                    if (verificationUsingUnstripped
+                            || verificationUsingStripped
+                            || verificationUsingLocationHeader) {
                         log.debug(
                                 "Check 2, {} html output for AND FALSE condition [{}] matches the (refreshed) original results",
                                 (verificationUsingStripped ? "STRIPPED" : "UNSTRIPPED"),
@@ -1358,7 +1393,7 @@ && matchBodyPattern(msg1, errorPattern, sb)) {
                                             sqlBooleanOrTrueValue,
                                             sqlBooleanAndFalseValue,
                                             "");
-                        } else {
+                        } else if (verificationUsingUnstripped) {
                             extraInfo =
                                     Constant.messages.getString(
                                             MESSAGE_PREFIX + "alert.booleanbased.extrainfo",
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
@@ -1,5 +1,5 @@
 /*
- * Zed Attack Proxy (ZAP) and its related class files.
+ * Zed Attack Proxy (ZAP) `and `its related class files.
  *
  * ZAP is an HTTP/HTTPS proxy for assessing web application security.
  *
@@ -21,6 +21,7 @@
 
 import static fi.iki.elonen.NanoHTTPD.newFixedLengthResponse;
 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;
@@ -29,10 +30,13 @@
 import fi.iki.elonen.NanoHTTPD;
 import fi.iki.elonen.NanoHTTPD.IHTTPSession;
 import fi.iki.elonen.NanoHTTPD.Response;
+
+import java.util.Collections;
 import java.util.Map;
 import org.junit.jupiter.api.Test;
 import org.parosproxy.paros.core.scanner.Alert;
 import org.parosproxy.paros.core.scanner.Plugin.AttackStrength;
+import org.parosproxy.paros.network.HttpHeader;
 import org.zaproxy.addon.commonlib.CommonAlertTag;
 import org.zaproxy.zap.model.Tech;
 import org.zaproxy.zap.model.TechSet;
@@ -132,6 +136,60 @@ 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
@@ -350,12 +408,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;
         }
 
@@ -381,4 +439,48 @@ protected String getContent(String value) {
             return "Some Content " + contentAddition;
         }
     }
+
+    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";
+        }
+    }
 }
