pscanrules: Address Suspicious Comments rule JS FPs

- CHANGELOG > Added fix note. - InformationDisclosureSuspiciousCommentsScanRule > Updated handling to target comments in JavaScript more specifically. - InformationDisclosureSuspiciousCommentsScanRuleUnitTest > 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 <[email protected]>
zaproxy · Oct 31, 2024 · e7cb14e · e7cb14e
1 parent b5aa3bd
commit e7cb14e
Show file tree

Hide file tree

Showing 5 changed files with 112 additions and 47 deletions.
diff --git 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/...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,10 +78,30 @@ 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) {
 
@@ -90,23 +111,7 @@ 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 (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 +137,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 +166,25 @@ public void scanHttpResponseReceive(HttpMessage msg, int id, Source source) {
         }
     }
 
+    private static void checkJsComments(
+            List<Pattern> patterns, Map<String, List<AlertSummary>> alertMap, String target) {
+        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 void recordAlertSummary(
             Map<String, List<AlertSummary>> alertMap, AlertSummary summary) {
         alertMap.computeIfAbsent(summary.getPattern(), k -> new ArrayList<>()).add(summary);

diff --git a/...ain/javahelp/org/zaproxy/zap/extension/pscanrules/resources/help/contents/pscanrules.html b/...ain/javahelp/org/zaproxy/zap/extension/pscanrules/resources/help/contents/pscanrules.html
@@ -310,7 +310,7 @@ <H2 id="id-10025">Information Disclosure: Referrer</H2>
 <H2 id="id-10027">Information Disclosure: Suspicious Comments</H2>
 Analyzes web content to identify comments which contain potentially sensitive details. Which may lead to 
 further attack or exposure of unintended data.
-<p><strong>Note:</strong> The strings to look for can be extended by using the Custom Payloads addon.
+<p><strong>Note:</strong> The strings to look for can be extended by using the Custom Payloads add-on.
 <p>
 Latest code: <a href="https://github.com/zaproxy/zap-extensions/blob/main/addOns/pscanrules/src/main/java/org/zaproxy/zap/extension/pscanrules/InformationDisclosureSuspiciousCommentsScanRule.java">InformationDisclosureSuspiciousCommentsScanRule.java</a>
 <br>

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,11 +99,11 @@ 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");
 
@@ -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 <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 +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, 1),
+                alertsRaised.get(0).getOtherInfo());
     }
 
     @Test
@@ -182,11 +214,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 +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 = "<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
@@ -369,15 +422,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.";
     }