From 598594e6b6cf943e4a06425eec4df5b95df89b68 Mon Sep 17 00:00:00 2001 From: kingthorin Date: Sat, 9 Dec 2023 18:57:07 -0500 Subject: [PATCH] pscanrules: Big Redirect rule - Also alert on multiple HREFs in body - BigRedirectScanRule > Add further functionality to check if the response contains multiple HREFs, example alerts, and Alert Refs. - BigRedirectScanRuleUnitTest > Add tests for new behavior, example alerts, and alert refs. - CHANGELOG > Add change notes. - Messages.properties > Add supporting resources. - pscanrules.html > Update help entry. Signed-off-by: kingthorin --- addOns/pscanrules/CHANGELOG.md | 4 +- .../pscanrules/BigRedirectsScanRule.java | 87 +++++++++++++------ .../resources/help/contents/pscanrules.html | 5 +- .../pscanrules/resources/Messages.properties | 4 +- .../BigRedirectsScanRuleUnitTest.java | 63 ++++++++++++-- 5 files changed, 126 insertions(+), 37 deletions(-) diff --git a/addOns/pscanrules/CHANGELOG.md b/addOns/pscanrules/CHANGELOG.md index 40b895c2440..19ad980d174 100644 --- a/addOns/pscanrules/CHANGELOG.md +++ b/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 diff --git a/addOns/pscanrules/src/main/java/org/zaproxy/zap/extension/pscanrules/BigRedirectsScanRule.java b/addOns/pscanrules/src/main/java/org/zaproxy/zap/extension/pscanrules/BigRedirectsScanRule.java index ee45e15fcdb..a9416b38ce3 100644 --- a/addOns/pscanrules/src/main/java/org/zaproxy/zap/extension/pscanrules/BigRedirectsScanRule.java +++ b/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 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 getAlertTags() { return ALERT_TAGS; } + + public String getHelpLink() { + return "https://www.zaproxy.org/docs/desktop/addons/passive-scan-rules/#id-" + + getPluginId(); + } } diff --git a/addOns/pscanrules/src/main/javahelp/org/zaproxy/zap/extension/pscanrules/resources/help/contents/pscanrules.html b/addOns/pscanrules/src/main/javahelp/org/zaproxy/zap/extension/pscanrules/resources/help/contents/pscanrules.html index 5ce23db73c2..e870c65f0cb 100644 --- a/addOns/pscanrules/src/main/javahelp/org/zaproxy/zap/extension/pscanrules/resources/help/contents/pscanrules.html +++ b/addOns/pscanrules/src/main/javahelp/org/zaproxy/zap/extension/pscanrules/resources/help/contents/pscanrules.html @@ -54,11 +54,14 @@

Application Errors

Latest code: ApplicationErrorScanRule.java -

Big Redirect Detected (Potential Sensitive Information Leak)

+

Big Redirect Detected (Potential Sensitive Information Leak)

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.

Latest code: BigRedirectsScanRule.java +
+Alert ID: 10044.

Cache Control

Checks "Cache-Control" response headers against general industry best practice settings for protection of sensitive content.
diff --git a/addOns/pscanrules/src/main/resources/org/zaproxy/zap/extension/pscanrules/resources/Messages.properties b/addOns/pscanrules/src/main/resources/org/zaproxy/zap/extension/pscanrules/resources/Messages.properties index a35c1ef28e6..3955d7d7646 100644 --- a/addOns/pscanrules/src/main/resources/org/zaproxy/zap/extension/pscanrules/resources/Messages.properties +++ b/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. diff --git a/addOns/pscanrules/src/test/java/org/zaproxy/zap/extension/pscanrules/BigRedirectsScanRuleUnitTest.java b/addOns/pscanrules/src/test/java/org/zaproxy/zap/extension/pscanrules/BigRedirectsScanRuleUnitTest.java index 9c86f0d5b8e..b5066b5af3f 100644 --- a/addOns/pscanrules/src/test/java/org/zaproxy/zap/extension/pscanrules/BigRedirectsScanRuleUnitTest.java +++ b/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( + "Home" + + "
" + + "Admin" + + ""); + // 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 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) {