diff --git a/addOns/pscanrules/CHANGELOG.md b/addOns/pscanrules/CHANGELOG.md index 413f018e186..eea185b0e61 100644 --- a/addOns/pscanrules/CHANGELOG.md +++ b/addOns/pscanrules/CHANGELOG.md @@ -10,6 +10,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Information Disclosure - Suspicious Comments - Username Hash Found +### Fixed +- The Information Disclosure - Suspicious Comments scan rule: + - Should now be less false positive prone on JavaScript findings (Issues 6622 & 6736). + - Now skips obvious font requests even if their content type is text/html or text related. + ## [61] - 2024-09-24 ### Changed - Maintenance changes. 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..1dc6f2bb7af 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,6 +25,7 @@ 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 net.htmlparser.jericho.Element; @@ -77,36 +78,42 @@ 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<>(); + JS_SINGLELINE_COMMENT + .matcher(content) + .results() + .map(MatchResult::group) + .forEach(results::add); + JS_MULTILINE_COMMENT + .matcher(content) + .results() + .map(MatchResult::group) + .forEach(results::add); + return results; + } + @Override public void scanHttpResponseReceive(HttpMessage msg, int id, Source source) { - List patterns = getPatterns(); + patterns = getPatterns(); Map> alertMap = new HashMap<>(); - if (msg.getResponseBody().length() > 0 && msg.getResponseHeader().isText()) { + if (msg.getResponseBody().length() > 0 + && msg.getResponseHeader().isText() + && !ResourceIdentificationUtils.isFont(msg)) { if (ResourceIdentificationUtils.isJavaScript(msg)) { - // Just treat as text - String[] lines = msg.getResponseBody().toString().split("\n"); - for (String line : lines) { - for (Pattern pattern : patterns) { - Matcher m = pattern.matcher(line); - if (m.find()) { - recordAlertSummary( - alertMap, - new AlertSummary( - pattern.toString(), - line, - Alert.CONFIDENCE_LOW, - m.group())); - break; // Only need to record this line once - } - } - } + checkJsComments(patterns, alertMap, msg.getResponseBody().toString()); } else { // Can use the parser @@ -132,20 +139,7 @@ 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 - } - } + checkJsComments(patterns, alertMap, el.toString()); offset = el.getEnd(); } } @@ -174,6 +168,32 @@ public void scanHttpResponseReceive(HttpMessage msg, int id, Source source) { } } + private static void checkJsComments( + List patterns, Map> alertMap, String target) { + if (!isGoodCandidate(target)) { + return; + } + for (String candidate : getJsComments(target)) { + for (Pattern pattern : patterns) { + Matcher m = pattern.matcher(candidate); + if (m.find()) { + recordAlertSummary( + alertMap, + new AlertSummary( + pattern.toString(), + candidate, + Alert.CONFIDENCE_LOW, + m.group())); + return; + } + } + } + } + + private static boolean isGoodCandidate(String target) { + return target.contains("//") || target.contains("/*"); + } + private static void recordAlertSummary( Map> alertMap, AlertSummary summary) { alertMap.computeIfAbsent(summary.getPattern(), k -> new ArrayList<>()).add(summary); 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..dbaf0313096 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,14 +99,39 @@ 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"); + assertTrue(msg.getResponseHeader().isText()); + assertTrue(ResourceIdentificationUtils.isJavaScript(msg)); + // 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)); @@ -116,7 +143,8 @@ void shouldAlertOnSuspiciousCommentInJavaScriptResponse() 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 +171,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 +189,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, 1), + alertsRaised.get(0).getOtherInfo()); } @Test @@ -182,11 +212,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 +231,24 @@ 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 @@ -333,6 +381,21 @@ void shouldNotAlertIfResponseIsNotText() throws HttpMalformedHeaderException, UR assertEquals(0, alertsRaised.size()); } + @Test + void shouldNotAlertIfResponseIsTextButReallyFontUrl() + throws HttpMalformedHeaderException, URIException { + // Given + HttpMessage msg = + createHttpMessageWithRespBody( + "Some text \nLine 2\n", + "text/html"); + msg.getRequestHeader().setURI(new URI("http://example.com/shop-icons.woof", false)); + // When + scanHttpResponseReceive(msg); + // Then + assertEquals(0, alertsRaised.size()); + } + @Test void shouldHaveExpectedExample() { // Given / When @@ -369,7 +432,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 +440,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."; }