Merge pull request #5153 from kingthorin/big-redir-links

pscanrules: Big Redirect rule - Also alert on multiple HREFs in body

addOns/pscanrules/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
-
+### Changed
+- The Big Redirect scan rule will now also alert on responses that have multiple HREFs (idea from xnl-h4ck3r).
+    - It also now includes example alert and alert reference functionality for documentation generation purposes (Issues 6119, 7100, and 8189).
 
 ## [53] - 2023-11-30
 ### Changed

addOns/pscanrules/src/main/java/org/zaproxy/zap/extension/pscanrules/BigRedirectsScanRule.java

@@ -19,14 +19,17 @@
  */
 package org.zaproxy.zap.extension.pscanrules;
 
+import java.util.List;
 import java.util.Map;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 import net.htmlparser.jericho.Source;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.parosproxy.paros.Constant;
 import org.parosproxy.paros.core.scanner.Alert;
+import org.parosproxy.paros.network.HttpHeader;
 import org.parosproxy.paros.network.HttpMessage;
-import org.parosproxy.paros.network.HttpResponseHeader;
 import org.parosproxy.paros.network.HttpStatusCode;
 import org.zaproxy.addon.commonlib.CommonAlertTag;
 import org.zaproxy.zap.extension.pscan.PluginPassiveScanner;
@@ -44,6 +47,8 @@ public class BigRedirectsScanRule extends PluginPassiveScanner {
                     CommonAlertTag.OWASP_2017_A03_DATA_EXPOSED,
                     CommonAlertTag.WSTG_V42_INFO_05_CONTENT_LEAK);
 
+    private static final Pattern HREF_PATTERN = Pattern.compile("href", Pattern.CASE_INSENSITIVE);
+
     @Override
     public void scanHttpResponseReceive(HttpMessage msg, int id, Source source) {
         long start = System.currentTimeMillis();
@@ -53,8 +58,7 @@ public void scanHttpResponseReceive(HttpMessage msg, int id, Source source) {
         if (HttpStatusCode.isRedirection(msg.getResponseHeader().getStatusCode())
                 && msg.getResponseHeader().getStatusCode() != 304) { // This response is a redirect
             int responseLocationHeaderURILength = 0;
-            String locationHeaderValue =
-                    msg.getResponseHeader().getHeader(HttpResponseHeader.LOCATION);
+            String locationHeaderValue = msg.getResponseHeader().getHeader(HttpHeader.LOCATION);
             if (locationHeaderValue != null) {
                 responseLocationHeaderURILength = locationHeaderValue.length();
             } else { // No location header found
@@ -70,22 +74,18 @@ public void scanHttpResponseReceive(HttpMessage msg, int id, Source source) {
                 // Check if response is bigger than predicted
                 if (responseBodyLength > predictedResponseSize) {
                     // Response is larger than predicted so raise an alert
-                    newAlert()
-                            .setRisk(Alert.RISK_LOW)
-                            .setConfidence(Alert.CONFIDENCE_MEDIUM)
-                            .setDescription(getDescription())
-                            .setOtherInfo(
-                                    Constant.messages.getString(
-                                            MESSAGE_PREFIX + "extrainfo",
-                                            responseLocationHeaderURILength,
-                                            locationHeaderValue,
-                                            predictedResponseSize,
-                                            responseBodyLength))
-                            .setSolution(getSolution())
-                            .setReference(getReference())
-                            .setCweId(201)
-                            .setWascId(13)
+                    createBigAlert(
+                                    responseLocationHeaderURILength,
+                                    locationHeaderValue,
+                                    predictedResponseSize,
+                                    responseBodyLength)
                             .raise();
+                } else {
+                    Matcher matcher = HREF_PATTERN.matcher(msg.getResponseBody().toString());
+                    long hrefCount = matcher.results().count();
+                    if (hrefCount > 1) {
+                        createMultiAlert(hrefCount).raise();
+                    }
                 }
             }
         }
@@ -106,6 +106,44 @@ private int getPredictedResponseSize(int redirectURILength) {
         return predictedResponseSize;
     }
 
+    private AlertBuilder createBaseAlert(String ref) {
+        return newAlert()
+                .setRisk(Alert.RISK_LOW)
+                .setConfidence(Alert.CONFIDENCE_MEDIUM)
+                .setSolution(getSolution())
+                .setCweId(201)
+                .setWascId(13)
+                .setAlertRef(String.valueOf(PLUGIN_ID) + ref);
+    }
+
+    private AlertBuilder createBigAlert(
+            int urlLength, String url, int predictedMaxLength, int actualMaxLength) {
+        return createBaseAlert("-1")
+                .setDescription(Constant.messages.getString(MESSAGE_PREFIX + "desc"))
+                .setOtherInfo(
+                        Constant.messages.getString(
+                                MESSAGE_PREFIX + "extrainfo",
+                                urlLength,
+                                url,
+                                predictedMaxLength,
+                                actualMaxLength));
+    }
+
+    private AlertBuilder createMultiAlert(long hrefCount) {
+        return createBaseAlert("-2")
+                .setName(Constant.messages.getString(MESSAGE_PREFIX + "multi.name"))
+                .setDescription(Constant.messages.getString(MESSAGE_PREFIX + "multi.desc"))
+                .setOtherInfo(
+                        Constant.messages.getString(MESSAGE_PREFIX + "multi.extrainfo", hrefCount));
+    }
+
+    @Override
+    public List<Alert> getExampleAlerts() {
+        return List.of(
+                createBigAlert(18, "http://example.com", 318, 319).build(),
+                createMultiAlert(3).build());
+    }
+
     @Override
     public int getPluginId() {
         return PLUGIN_ID;
@@ -116,20 +154,17 @@ public String getName() {
         return Constant.messages.getString(MESSAGE_PREFIX + "name");
     }
 
-    private String getDescription() {
-        return Constant.messages.getString(MESSAGE_PREFIX + "desc");
-    }
-
     private String getSolution() {
         return Constant.messages.getString(MESSAGE_PREFIX + "soln");
     }
 
-    private String getReference() {
-        return Constant.messages.getString(MESSAGE_PREFIX + "refs");
-    }
-
     @Override
     public Map<String, String> getAlertTags() {
         return ALERT_TAGS;
     }
+
+    public String getHelpLink() {
+        return "https://www.zaproxy.org/docs/desktop/addons/passive-scan-rules/#id-"
+                + getPluginId();
+    }
 }

addOns/pscanrules/src/main/javahelp/org/zaproxy/zap/extension/pscanrules/resources/help/contents/pscanrules.html

@@ -54,11 +54,14 @@ <H2>Application Errors</H2>
 <p>
 Latest code: <a href="https://github.com/zaproxy/zap-extensions/blob/main/addOns/pscanrules/src/main/java/org/zaproxy/zap/extension/pscanrules/ApplicationErrorScanRule.java">ApplicationErrorScanRule.java</a>
 
-<H2>Big Redirect Detected (Potential Sensitive Information Leak)</H2>
+<H2 id="id-10044">Big Redirect Detected (Potential Sensitive Information Leak)</H2>
 This check predicts the size of various redirect type responses and generates an alert if the response is greater than the predicted size. 
 A large redirect response may indicate that although a redirect has taken place the page actually contained content (which may reveal sensitive information, PII, etc.).
+It will also raise an alert if the body of the redirect response contains multiple HREFs.
 <p>
 Latest code: <a href="https://github.com/zaproxy/zap-extensions/blob/main/addOns/pscanrules/src/main/java/org/zaproxy/zap/extension/pscanrules/BigRedirectsScanRule.java">BigRedirectsScanRule.java</a>
+<br>
+Alert ID: <a href="https://www.zaproxy.org/docs/alerts/10044/">10044</a>.
 
 <H2>Cache Control</H2>
 Checks "Cache-Control" response headers against general industry best practice settings for protection of sensitive content.<br>

addOns/pscanrules/src/main/resources/org/zaproxy/zap/extension/pscanrules/resources/Messages.properties

@@ -30,8 +30,10 @@ pscanrules.authenticationcredentialscaptured.soln = Use HTTPS, and use a secure
 
 pscanrules.bigredirects.desc = The server has responded with a redirect that seems to provide a large response. This may indicate that although the server sent a redirect it also responded with body content (which may include sensitive details, PII, etc.).
 pscanrules.bigredirects.extrainfo = Location header URI length: {0} [{1}].\nPredicted response size: {2}.\nResponse Body Length: {3}.
+pscanrules.bigredirects.multi.desc = The server has responded with a redirect that seems to contain multiple links. This may indicate that although the server sent a redirect it also responded with body content links (which may include sensitive details, PII, lead to admin panels, etc.).
+pscanrules.bigredirects.multi.extrainfo = The response contained {0} occurrences of "HREF".
+pscanrules.bigredirects.multi.name = Multiple HREFs Redirect Detected (Potential Sensitive Information Leak)
 pscanrules.bigredirects.name = Big Redirect Detected (Potential Sensitive Information Leak)
-pscanrules.bigredirects.refs = 
 pscanrules.bigredirects.soln = Ensure that no sensitive information is leaked via redirect responses. Redirect responses should have almost no content.
 
 pscanrules.cachecontrol.desc = The cache-control header has not been set properly or is missing, allowing the browser and proxies to cache content. For static assets like css, js, or image files this might be intended, however, the resources should be reviewed to ensure that no sensitive content will be cached.

addOns/pscanrules/src/test/java/org/zaproxy/zap/extension/pscanrules/BigRedirectsScanRuleUnitTest.java

@@ -24,6 +24,7 @@
 import static org.hamcrest.Matchers.is;
 
 import java.io.IOException;
+import java.util.List;
 import java.util.Map;
 import org.apache.commons.httpclient.URI;
 import org.junit.jupiter.api.BeforeEach;
@@ -82,7 +83,28 @@ void givenRedirectHeadersWithLargeBodyThenAlertRaised() {
 
         // Then
         assertThat(alertsRaised.size(), is(1));
-        assertBigRedirectAlertAttributes(alertsRaised.get(0));
+        assertBigAlertAttributes(alertsRaised.get(0));
+    }
+
+    @Test
+    void givenRedirectHeadersWithSmallBodyButMultipleHrefsThenAlertRaised() {
+        // Given
+        msg.getResponseHeader().setStatusCode(HttpStatusCode.MOVED_PERMANENTLY);
+        msg.getResponseHeader().setHeader(HttpHeader.LOCATION, URI);
+        msg.setResponseBody(
+                "<html><a href=\""
+                        + URI.toString()
+                        + "\">Home</a>"
+                        + "<br>"
+                        + "<a href=\""
+                        + URI.toString()
+                        + "/admin\">Admin</a>"
+                        + "</html>");
+        // When
+        scanHttpResponseReceive(msg);
+        // Then
+        assertThat(alertsRaised.size(), is(1));
+        assertMultiAlertAttributes(alertsRaised.get(0), "2");
     }
 
     @Test
@@ -152,22 +174,47 @@ void shouldReturnExpectedMappings() {
                 is(equalTo(CommonAlertTag.WSTG_V42_INFO_05_CONTENT_LEAK.getValue())));
     }
 
-    private static void assertBigRedirectAlertAttributes(Alert alert) {
+    @Test
+    void shouldHaveExpectedExampleAlerts() {
+        // Given / When
+        List<Alert> alerts = rule.getExampleAlerts();
+        // Then
+        assertThat(alerts.size(), is(equalTo(2)));
+        assertBigAlertAttributes(alerts.get(0));
+        assertMultiAlertAttributes(alerts.get(1), "3");
+    }
+
+    private static void assertBigAlertAttributes(Alert alert) {
         assertThat(alert.getRisk(), is(Alert.RISK_LOW));
         assertThat(alert.getConfidence(), is(Alert.CONFIDENCE_MEDIUM));
         assertThat(alert.getName(), is(getLocalisedString("name")));
         assertThat(alert.getDescription(), is(getLocalisedString("desc")));
         assertThat(alert.getSolution(), is(getLocalisedString("soln")));
-        assertThat(alert.getReference(), is(getLocalisedString("refs")));
-        assertThat(alert.getOtherInfo(), is(getExpectedExtraInfo()));
+        assertThat(alert.getOtherInfo(), is(getExpectedBigExtraInfo()));
+        assertThat(alert.getCweId(), is(201));
+        assertThat(alert.getWascId(), is(13));
+        assertThat(alert.getAlertRef(), is("10044-1"));
+    }
+
+    private static String getExpectedBigExtraInfo() {
+        return getLocalisedString(
+                "extrainfo", URI.length(), URI, ALLOWABLE_BODY_SIZE, ALLOWABLE_BODY_SIZE + 1);
+    }
+
+    private static void assertMultiAlertAttributes(Alert alert, String count) {
+        assertThat(alert.getRisk(), is(Alert.RISK_LOW));
+        assertThat(alert.getConfidence(), is(Alert.CONFIDENCE_MEDIUM));
+        assertThat(alert.getName(), is(getLocalisedString("multi.name")));
+        assertThat(alert.getDescription(), is(getLocalisedString("multi.desc")));
+        assertThat(alert.getSolution(), is(getLocalisedString("soln")));
+        assertThat(alert.getOtherInfo(), is(getExpectedMultiExtraInfo(count)));
         assertThat(alert.getCweId(), is(201));
         assertThat(alert.getWascId(), is(13));
-        assertThat(alert.getUri(), is(URI));
+        assertThat(alert.getAlertRef(), is("10044-2"));
     }
 
-    private static String getExpectedExtraInfo() {
-        int bodySize = ALLOWABLE_BODY_SIZE;
-        return getLocalisedString("extrainfo", URI.length(), URI, bodySize, bodySize + 1);
+    private static String getExpectedMultiExtraInfo(String count) {
+        return getLocalisedString("multi.extrainfo", count);
     }
 
     private static String getLocalisedString(String key, Object... params) {