pscanrules: Address Suspicious Comments rule JS FPs

- CHANGELOG > Added fix note. - InformationDisclosureSuspiciousCommentsScanRule > Updated handling to target comments in JavaScript more specifically & skip font requests. - InformationDisclosureSuspiciousCommentsScanRuleUnitTest > Updated and added tests. - Messages.properties > Updated to detail/report the findings more specifically based on the new behavior. Signed-off-by: kingthorin <[email protected]>
zaproxy · Nov 17, 2024 · 0a3b163 · 0a3b163
1 parent 8cac506
commit 0a3b163
Show file tree

Hide file tree

Showing 4 changed files with 138 additions and 48 deletions.
diff --git 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/...org/zaproxy/zap/extension/pscanrules/InformationDisclosureSuspiciousCommentsScanRule.java b/...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<Iterable<String>> 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<Iterable<String>> payloadProvider = DEFAULT_PAYLOAD_PROVIDER;
 
     private List<Pattern> patterns = null;
 
+    private static List<String> getJsComments(String content) {
+        List<String> 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<Pattern> patterns = getPatterns();
+        patterns = getPatterns();
         Map<String, List<AlertSummary>> 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<Pattern> patterns, Map<String, List<AlertSummary>> 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<String, List<AlertSummary>> alertMap, AlertSummary summary) {
         alertMap.computeIfAbsent(summary.getPattern(), k -> new ArrayList<>()).add(summary);

diff --git a/...les/src/main/resources/org/zaproxy/zap/extension/pscanrules/resources/Messages.properties b/...les/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/...oxy/zap/extension/pscanrules/InformationDisclosureSuspiciousCommentsScanRuleUnitTest.java b/...oxy/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,38 @@ void shouldReturnExpectedMappings() {
     }
 
     @Test
-    void shouldAlertOnSuspiciousCommentInJavaScriptResponse()
+    void shouldNotAlertOnSuspiciousCommentInJavaScriptResponse()
             throws HttpMalformedHeaderException, URIException {
-
         // Given
-        String line1 = "Some text <script>Some Script Element FIXME: DO something </script>";
+        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 +142,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 +170,9 @@ void shouldCreateOneAlertforMultipleAndEqualSuspiciousComments()
             throws HttpMalformedHeaderException, URIException {
 
         // Given
-        String line1 = "Some text <script>Some Script Element FIXME: DO something";
-        String line2 = "FIXME: DO something else </script>";
+        String comment = "// FIXME: DO something";
+        String line1 = "Some text <script>Some Script Element " + comment;
+        String line2 = "// FIXME: DO something else </script>";
         String body = line1 + "\n" + line2 + "\nLine 2\n";
         HttpMessage msg = createHttpMessageWithRespBody(body, "text/javascript;charset=ISO-8859-1");
 
@@ -160,7 +188,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 +211,12 @@ void shouldNotAlertWithoutSuspiciousCommentInJavaScriptResponse()
     }
 
     @Test
-    void shouldAlertOnSuspiciousCommentInHtmlScriptElements()
+    void shouldAlertOnSuspiciousCommentInHtmlScriptElementWithComment()
             throws HttpMalformedHeaderException, URIException {
 
         // Given
-        String script = "<script>Some Html Element todo DO something </script>";
+        String comment = "// todo DO something";
+        String script = "<script>Some Script Element " + comment + "\n</script>";
         String body = "<h1>Some text " + script + "</h1>\n<b>No script here</b>\n";
         HttpMessage msg = createHttpMessageWithRespBody(body, "text/html;charset=ISO-8859-1");
 
@@ -200,7 +230,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 = "<script>myArray = [\"admin\", \"password\"]\n</script>";
+        String body = "<h1>Some text " + script + "</h1>\n<b>No script here</b>\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 +383,21 @@ void shouldNotAlertIfResponseIsNotText() throws HttpMalformedHeaderException, UR
         assertEquals(0, alertsRaised.size());
     }
 
+    @Test
+    void shouldNotAlertIfResponseIsTextButReallyFontUrl()
+            throws HttpMalformedHeaderException, URIException {
+        // Given
+        HttpMessage msg =
+                createHttpMessageWithRespBody(
+                        "Some text <script>Some Script Element FixMe: DO something </script>\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,15 +434,15 @@ 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.";
         }
         return "The following pattern was used: "
                 + 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.";
     }