From ae5659e78c1f496aa46904023aae4fa59dabdddd Mon Sep 17 00:00:00 2001 From: kingthorin Date: Sat, 12 Oct 2024 22:39:20 -0400 Subject: [PATCH] pscanrules: Address Suspicious Comments rule JS FPs - CHANGELOG > Added fix note. - InformationDisclosureSuspiciousCommentsScanRule > Updated handling to target comments in JavaScript more specifically. - InformationDisclosureSuspiciousCommentsScanRuleUnitTest b> Updated and added tests. - Messages.properties > Updated to detail/report the findings more specifically based on the new behavior. - pscanrules.html > Correct occurrence of "add-on" (vs addon). Signed-off-by: kingthorin --- addOns/pscanrules/CHANGELOG.md | 3 +- ...nDisclosureSuspiciousCommentsScanRule.java | 57 +++++++++----- .../resources/help/contents/pscanrules.html | 2 +- .../pscanrules/resources/Messages.properties | 6 +- ...ureSuspiciousCommentsScanRuleUnitTest.java | 75 ++++++++++++++++--- 5 files changed, 110 insertions(+), 33 deletions(-) diff --git a/addOns/pscanrules/CHANGELOG.md b/addOns/pscanrules/CHANGELOG.md index f9d1c550f5d..eb53453b7f1 100644 --- a/addOns/pscanrules/CHANGELOG.md +++ b/addOns/pscanrules/CHANGELOG.md @@ -4,7 +4,8 @@ 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 - +### Fixed +- The Information Disclosure - Suspicious Comments scan rule should now be less false positive prone on JavaScript findings (Issues 6622 & 6736). ## [61] - 2024-09-24 ### Changed diff --git a/addOns/pscanrules/src/main/java/org/zaproxy/zap/extension/pscanrules/InformationDisclosureSuspiciousCommentsScanRule.java b/addOns/pscanrules/src/main/java/org/zaproxy/zap/extension/pscanrules/InformationDisclosureSuspiciousCommentsScanRule.java index 3a0938a9097..ea47f542f23 100644 --- a/addOns/pscanrules/src/main/java/org/zaproxy/zap/extension/pscanrules/InformationDisclosureSuspiciousCommentsScanRule.java +++ b/addOns/pscanrules/src/main/java/org/zaproxy/zap/extension/pscanrules/InformationDisclosureSuspiciousCommentsScanRule.java @@ -25,8 +25,10 @@ import java.util.Map; import java.util.Map.Entry; import java.util.function.Supplier; +import java.util.regex.MatchResult; import java.util.regex.Matcher; import java.util.regex.Pattern; +import java.util.stream.Collectors; import net.htmlparser.jericho.Element; import net.htmlparser.jericho.HTMLElementName; import net.htmlparser.jericho.Source; @@ -77,10 +79,32 @@ public class InformationDisclosureSuspiciousCommentsScanRule extends PluginPassi private static final Supplier> DEFAULT_PAYLOAD_PROVIDER = () -> DEFAULT_PAYLOADS; + // https://github.com/antlr/grammars-v4/blob/c82c128d980f4ce46fb3536f87b06b45b9619922/javascript/javascript/JavaScriptLexer.g4#L49-L50 + private static final Pattern JS_MULTILINE_COMMENT = + Pattern.compile("/\\*.*?\\*/", Pattern.DOTALL); + private static final Pattern JS_SINGLELINE_COMMENT = Pattern.compile("//.*"); + private static Supplier> payloadProvider = DEFAULT_PAYLOAD_PROVIDER; private List patterns = null; + private static List getJsComments(String content) { + List results = new ArrayList<>(); + results.addAll( + JS_SINGLELINE_COMMENT + .matcher(content) + .results() + .map(MatchResult::group) + .collect(Collectors.toList())); + results.addAll( + JS_MULTILINE_COMMENT + .matcher(content) + .results() + .map(MatchResult::group) + .collect(Collectors.toList())); + return results; + } + @Override public void scanHttpResponseReceive(HttpMessage msg, int id, Source source) { @@ -90,17 +114,15 @@ public void scanHttpResponseReceive(HttpMessage msg, int id, Source source) { if (msg.getResponseBody().length() > 0 && msg.getResponseHeader().isText()) { if (ResourceIdentificationUtils.isJavaScript(msg)) { - // Just treat as text - String[] lines = msg.getResponseBody().toString().split("\n"); - for (String line : lines) { + for (String candidate : getJsComments(msg.getResponseBody().toString())) { for (Pattern pattern : patterns) { - Matcher m = pattern.matcher(line); + Matcher m = pattern.matcher(candidate); if (m.find()) { recordAlertSummary( alertMap, new AlertSummary( pattern.toString(), - line, + candidate, Alert.CONFIDENCE_LOW, m.group())); break; // Only need to record this line once @@ -132,18 +154,19 @@ public void scanHttpResponseReceive(HttpMessage msg, int id, Source source) { Element el; int offset = 0; while ((el = source.getNextElement(offset, HTMLElementName.SCRIPT)) != null) { - for (Pattern pattern : patterns) { - String elStr = el.toString(); - Matcher m = pattern.matcher(elStr); - if (m.find()) { - recordAlertSummary( - alertMap, - new AlertSummary( - pattern.toString(), - elStr, - Alert.CONFIDENCE_LOW, - m.group())); - break; // Only need to record this script once + for (String candidate : getJsComments(el.toString())) { + for (Pattern pattern : patterns) { + Matcher m = pattern.matcher(candidate); + if (m.find()) { + recordAlertSummary( + alertMap, + new AlertSummary( + pattern.toString(), + candidate, + Alert.CONFIDENCE_LOW, + m.group())); + break; // Only need to record this script once + } } } offset = el.getEnd(); 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 ccd122d8f5e..10f857977b1 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 @@ -310,7 +310,7 @@

Information Disclosure: Referrer

Information Disclosure: Suspicious Comments

Analyzes web content to identify comments which contain potentially sensitive details. Which may lead to further attack or exposure of unintended data. -

Note: The strings to look for can be extended by using the Custom Payloads addon. +

Note: The strings to look for can be extended by using the Custom Payloads add-on.

Latest code: InformationDisclosureSuspiciousCommentsScanRule.java
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 2aea75b3417..e78fe65cf8c 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 @@ -189,10 +189,10 @@ pscanrules.informationdisclosurereferrer.otherinfo.sensitiveinfo = The URL in th pscanrules.informationdisclosurereferrer.otherinfo.ssn = The URL in the HTTP referrer header field appears to contain US Social Security Number(s). pscanrules.informationdisclosurereferrer.soln = Do not pass sensitive information in URIs. -pscanrules.informationdisclosuresuspiciouscomments.desc = The response appears to contain suspicious comments which may help an attacker. Note: Matches made within script blocks or files are against the entire content not only comments. +pscanrules.informationdisclosuresuspiciouscomments.desc = The response appears to contain suspicious comments which may help an attacker. pscanrules.informationdisclosuresuspiciouscomments.name = Information Disclosure - Suspicious Comments -pscanrules.informationdisclosuresuspiciouscomments.otherinfo = The following pattern was used: {0} and was detected in the element starting with: "{1}", see evidence field for the suspicious comment/snippet. -pscanrules.informationdisclosuresuspiciouscomments.otherinfo2 = The following pattern was used: {0} and was detected {2} times, the first in the element starting with: "{1}", see evidence field for the suspicious comment/snippet. +pscanrules.informationdisclosuresuspiciouscomments.otherinfo = The following pattern was used: {0} and was detected in likely comment: "{1}", see evidence field for the suspicious comment/snippet. +pscanrules.informationdisclosuresuspiciouscomments.otherinfo2 = The following pattern was used: {0} and was detected {2} times, the first in likely comment: "{1}", see evidence field for the suspicious comment/snippet. pscanrules.informationdisclosuresuspiciouscomments.soln = Remove all comments that return information that may help an attacker and fix any underlying problems they refer to. pscanrules.infosessionidurl.desc = URL rewrite is used to track user session ID. The session ID may be disclosed via cross-site referer header. In addition, the session ID might be stored in browser history or server logs. diff --git a/addOns/pscanrules/src/test/java/org/zaproxy/zap/extension/pscanrules/InformationDisclosureSuspiciousCommentsScanRuleUnitTest.java b/addOns/pscanrules/src/test/java/org/zaproxy/zap/extension/pscanrules/InformationDisclosureSuspiciousCommentsScanRuleUnitTest.java index 4cc5da35b10..9582cf86f67 100644 --- a/addOns/pscanrules/src/test/java/org/zaproxy/zap/extension/pscanrules/InformationDisclosureSuspiciousCommentsScanRuleUnitTest.java +++ b/addOns/pscanrules/src/test/java/org/zaproxy/zap/extension/pscanrules/InformationDisclosureSuspiciousCommentsScanRuleUnitTest.java @@ -32,6 +32,8 @@ import org.apache.commons.httpclient.URI; import org.apache.commons.httpclient.URIException; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.parosproxy.paros.Constant; import org.parosproxy.paros.core.scanner.Alert; import org.parosproxy.paros.network.HttpMalformedHeaderException; @@ -97,11 +99,11 @@ void shouldReturnExpectedMappings() { } @Test - void shouldAlertOnSuspiciousCommentInJavaScriptResponse() + void shouldNotAlertOnSuspiciousCommentInJavaScriptResponse() throws HttpMalformedHeaderException, URIException { // Given - String line1 = "Some text "; + String line1 = "myArray = [\"success\",\"FIXME\"]"; String body = line1 + "\nLine 2\n"; HttpMessage msg = createHttpMessageWithRespBody(body, "text/javascript;charset=ISO-8859-1"); @@ -111,12 +113,40 @@ void shouldAlertOnSuspiciousCommentInJavaScriptResponse() // When scanHttpResponseReceive(msg); + // Then + assertEquals(0, alertsRaised.size()); + } + + @ParameterizedTest + @ValueSource( + strings = { + "/* FIXME: admin:admin01$ */", + "/*FIXME: admin:admin01$*/", + "// FIXME: admin:admin01$", + "//FIXME: admin:admin01$" + }) + void shouldAlertOnSuspiciousCommentInJavaScriptResponse(String comment) + throws HttpMalformedHeaderException, URIException { + + // Given + String line1 = "myArray = [\"success\",\"FIXME\"]"; + String line2 = "\n" + comment; + String body = line1 + line2 + "\nLine 3\n"; + HttpMessage msg = createHttpMessageWithRespBody(body, "text/javascript;charset=ISO-8859-1"); + + assertTrue(msg.getResponseHeader().isText()); + assertTrue(ResourceIdentificationUtils.isJavaScript(msg)); + + // When + scanHttpResponseReceive(msg); + // Then assertEquals(1, alertsRaised.size()); assertEquals(Alert.CONFIDENCE_LOW, alertsRaised.get(0).getConfidence()); assertEquals("FIXME", alertsRaised.get(0).getEvidence()); assertEquals( - wrapEvidenceOtherInfo("\\bFIXME\\b", line1, 1), alertsRaised.get(0).getOtherInfo()); + wrapEvidenceOtherInfo("\\bFIXME\\b", comment, 1), + alertsRaised.get(0).getOtherInfo()); } @Test @@ -143,8 +173,9 @@ void shouldCreateOneAlertforMultipleAndEqualSuspiciousComments() throws HttpMalformedHeaderException, URIException { // Given - String line1 = "Some text "; + String comment = "// FIXME: DO something"; + String line1 = "Some text "; String body = line1 + "\n" + line2 + "\nLine 2\n"; HttpMessage msg = createHttpMessageWithRespBody(body, "text/javascript;charset=ISO-8859-1"); @@ -160,7 +191,8 @@ void shouldCreateOneAlertforMultipleAndEqualSuspiciousComments() assertEquals("FIXME", alertsRaised.get(0).getEvidence()); // detected 2 times, the first in the element assertEquals( - wrapEvidenceOtherInfo("\\bFIXME\\b", line1, 2), alertsRaised.get(0).getOtherInfo()); + wrapEvidenceOtherInfo("\\bFIXME\\b", comment, 2), + alertsRaised.get(0).getOtherInfo()); } @Test @@ -182,11 +214,12 @@ void shouldNotAlertWithoutSuspiciousCommentInJavaScriptResponse() } @Test - void shouldAlertOnSuspiciousCommentInHtmlScriptElements() + void shouldAlertOnSuspiciousCommentInHtmlScriptElementWithComment() throws HttpMalformedHeaderException, URIException { // Given - String script = ""; + String comment = "// todo DO something"; + String script = ""; String body = "

Some text " + script + "

\nNo script here\n"; HttpMessage msg = createHttpMessageWithRespBody(body, "text/html;charset=ISO-8859-1"); @@ -200,7 +233,27 @@ void shouldAlertOnSuspiciousCommentInHtmlScriptElements() assertEquals(1, alertsRaised.size()); assertEquals(Alert.CONFIDENCE_LOW, alertsRaised.get(0).getConfidence()); assertEquals( - wrapEvidenceOtherInfo("\\bTODO\\b", script, 1), alertsRaised.get(0).getOtherInfo()); + wrapEvidenceOtherInfo("\\bTODO\\b", comment, 1), + alertsRaised.get(0).getOtherInfo()); + } + + @Test + void shouldNotAlertOnSuspiciousContentInHtmlScriptElement() + throws HttpMalformedHeaderException, URIException { + + // Given + String script = ""; + String body = "

Some text " + script + "

\nNo script here\n"; + HttpMessage msg = createHttpMessageWithRespBody(body, "text/html;charset=ISO-8859-1"); + + assertTrue(msg.getResponseHeader().isText()); + assertFalse(ResourceIdentificationUtils.isJavaScript(msg)); + + // When + scanHttpResponseReceive(msg); + + // Then + assertEquals(0, alertsRaised.size()); } @Test @@ -369,7 +422,7 @@ private static String wrapEvidenceOtherInfo(String evidence, String info, int co if (count == 1) { return "The following pattern was used: " + evidence - + " and was detected in the element starting with: \"" + + " and was detected in likely comment: \"" + info + "\", see evidence field for the suspicious comment/snippet."; } @@ -377,7 +430,7 @@ private static String wrapEvidenceOtherInfo(String evidence, String info, int co + evidence + " and was detected " + count - + " times, the first in the element starting with: \"" + + " times, the first in likely comment: \"" + info + "\", see evidence field for the suspicious comment/snippet."; }