From 1375f98baeabe12b9e0d2497745cbd003b500d3d 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 | 79 ++++++++++++++----- .../resources/help/contents/pscanrules.html | 4 +- .../pscanrules/resources/Messages.properties | 4 +- .../BigRedirectsScanRuleUnitTest.java | 63 +++++++++++++-- 5 files changed, 123 insertions(+), 31 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..4207d97e096 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; @@ -53,8 +56,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 +72,22 @@ 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( + createBigAlert( Constant.messages.getString( MESSAGE_PREFIX + "extrainfo", responseLocationHeaderURILength, locationHeaderValue, predictedResponseSize, responseBodyLength)) - .setSolution(getSolution()) - .setReference(getReference()) - .setCweId(201) - .setWascId(13) .raise(); + } else { + Pattern pattern = Pattern.compile("href", Pattern.CASE_INSENSITIVE); + Matcher matcher = pattern.matcher(msg.getResponseBody().toString()); + long hrefCount = matcher.results().count(); + if (hrefCount > 1) { + // Response more than one HREF so raise an alert + createMultiAlert(String.valueOf(hrefCount)).raise(); + } } } } @@ -106,6 +108,46 @@ private int getPredictedResponseSize(int redirectURILength) { return predictedResponseSize; } + private AlertBuilder createBaseAlert() { + return newAlert() + .setRisk(Alert.RISK_LOW) + .setConfidence(Alert.CONFIDENCE_MEDIUM) + .setCweId(201) + .setWascId(13); + } + + private AlertBuilder createBigAlert(String otherinfo) { + return createBaseAlert() + .setDescription(Constant.messages.getString(MESSAGE_PREFIX + "desc")) + .setOtherInfo(otherinfo) + .setSolution(getSolution()) + .setAlertRef(String.valueOf(PLUGIN_ID) + "-1"); + } + + private AlertBuilder createMultiAlert(String otherinfo) { + return createBaseAlert() + .setName(Constant.messages.getString(MESSAGE_PREFIX + "multi.name")) + .setDescription(Constant.messages.getString(MESSAGE_PREFIX + "multi.desc")) + .setOtherInfo( + Constant.messages.getString(MESSAGE_PREFIX + "multi.extrainfo", otherinfo)) + .setSolution(getSolution()) + .setAlertRef(String.valueOf(PLUGIN_ID) + "-2"); + } + + @Override + public List getExampleAlerts() { + return List.of( + createBigAlert( + Constant.messages.getString( + MESSAGE_PREFIX + "extrainfo", + 18, + "http://example.com", + 318, + 319)) + .build(), + createMultiAlert("3").build()); + } + @Override public int getPluginId() { return PLUGIN_ID; @@ -116,20 +158,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..7d1a7c5b1a1 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,13 @@

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..2ee4e0f9d98 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 provide multiple links. 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.multi.extrainfo = The response contained {0} occurrences of "HREF". +pscanrules.bigredirects.multi.name = Strange 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..00ea7b2a538 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)); + assertBigRedirectBigAlertAttributes(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)); + assertBigRedirectMultiAlertAttributes(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))); + assertBigRedirectBigAlertAttributes(alerts.get(0)); + assertBigRedirectMultiAlertAttributes(alerts.get(1), "3"); + } + + private static void assertBigRedirectBigAlertAttributes(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 assertBigRedirectMultiAlertAttributes(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) {