ascanrules: Address External Redirect False Positives

Signed-off-by: kingthorin <[email protected]>

Author: kingthorin

addOns/ascanrules/CHANGELOG.md

@@ -19,6 +19,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 - For Alerts raised by the SQL Injection scan rules the Attack field values are now simply the payload, not an assembled description.
 - The Cross Site Scripting (Reflected) scan rule was updated to address potential false negatives when the injection context is a tag name and there is some filtering.
 - The Path Traversal scan rule now includes further details when directory matches are made (Issue 8379).
+- The External Redirect scan rules has been updated to account for potential false positives involving JavaScript comments.
 
 ### Added
 - Rules (as applicable) have been tagged in relation to HIPAA and PCI DSS.

addOns/ascanrules/src/main/java/org/zaproxy/zap/extension/ascanrules/ExternalRedirectScanRule.java

@@ -23,8 +23,10 @@
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.UUID;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
@@ -116,6 +118,27 @@ public String getReason() {
     private static final String SITE_HOST =
             Long.toString(Math.abs(UUID.randomUUID().getMostSignificantBits()));
     private static final String REDIRECT_SITE = SITE_HOST + OWASP_SUFFIX;
+    private static final String URL_PATTERN = "https?://" + REDIRECT_SITE;
+
+    // location='http://evil.com/';
+    // location.href='http://evil.com/';
+    private static final Pattern LOCATION_SIMPLE = Pattern.compile(
+            "(?i)location(?:\\.href)?\\s*=\\s*['\"](" + URL_PATTERN + ")['\"]");
+
+    // location.reload('http://evil.com/');
+    // location.replace('http://evil.com/');
+    // location.assign('http://evil.com/');
+    private static final Pattern LOCATION_METHODS = Pattern.compile(
+            "(?i)location\\.(?:replace|reload|assign)\\s*\\(\\s*['\"]("
+                    + URL_PATTERN
+                    + ")['\"]");
+
+    // window.open('http://evil.com/');
+    // window.navigate('http://evil.com/');
+    private static final Pattern WINDOW_METHODS = Pattern.compile(
+            "(?i)window\\.(?:open|navigate)\\s*\\(\\s*['\"]("
+                    + URL_PATTERN
+                    + ")['\"]");
 
     /** The various (prioritized) payload to try */
     private enum RedirectPayloads {
@@ -445,41 +468,20 @@ private static RedirectType isRedirected(String payload, HttpMessage msg) {
         // http://code.google.com/p/html5security/wiki/RedirectionMethods
         if (StringUtils.indexOfIgnoreCase(content, payload) != -1) {
             List<Element> jsElements = htmlSrc.getAllElements(HTMLElementName.SCRIPT);
-            String matchingUrl = "https?://" + REDIRECT_SITE;
-            Pattern pattern;
 
             for (Element el : jsElements) {
                 value = el.getContent().toString();
 
-                // location='http://evil.com/';
-                // location.href='http://evil.com/';
-                pattern =
-                        Pattern.compile(
-                                "(?i)location(?:\\.href)?\\s*=\\s*['\"](" + matchingUrl + ")['\"]");
-                if (isRedirectPresent(pattern, value)) {
+                if (isRedirectPresent(LOCATION_SIMPLE, value)) {
                     return RedirectType.JAVASCRIPT;
                 }
 
-                // location.reload('http://evil.com/');
-                // location.replace('http://evil.com/');
-                // location.assign('http://evil.com/');
-                pattern =
-                        Pattern.compile(
-                                "(?i)location\\.(?:replace|reload|assign)\\s*\\(\\s*['\"]("
-                                        + matchingUrl
-                                        + ")['\"]");
-                if (isRedirectPresent(pattern, value)) {
+                
+                if (isRedirectPresent(LOCATION_METHODS, value)) {
                     return RedirectType.JAVASCRIPT;
                 }
 
-                // window.open('http://evil.com/');
-                // window.navigate('http://evil.com/');
-                pattern =
-                        Pattern.compile(
-                                "(?i)window\\.(?:open|navigate)\\s*\\(\\s*['\"]("
-                                        + matchingUrl
-                                        + ")['\"]");
-                if (isRedirectPresent(pattern, value)) {
+                if (isRedirectPresent(WINDOW_METHODS, value)) {
                     return RedirectType.JAVASCRIPT;
                 }
             }
@@ -489,11 +491,74 @@ private static RedirectType isRedirected(String payload, HttpMessage msg) {
     }
 
     private static boolean isRedirectPresent(Pattern pattern, String value) {
-        Matcher matcher = pattern.matcher(value);
+        Set<String> extractedComments = extractJsComments(value);
+        String[] valueWithoutComments = {value};
+        extractedComments.forEach(
+                comment -> valueWithoutComments[0] = valueWithoutComments[0].replace(comment, ""));
+
+        Matcher matcher = pattern.matcher(valueWithoutComments[0]);
+
         return matcher.find()
                 && StringUtils.startsWithIgnoreCase(matcher.group(1), HttpHeader.HTTP);
     }
 
+    private static Set<String> extractJsComments(String jsCode) {
+        Set<String> comments = new HashSet<>();
+        boolean inSingleQuote = false;
+        boolean inDoubleQuote = false;
+        boolean inEscape = false;
+
+        for (int i = 0; i < jsCode.length(); i++) {
+            char c = jsCode.charAt(i);
+
+            if (inEscape) {
+                inEscape = false;
+                continue;
+            }
+
+            if (c == '\\') {
+                inEscape = true;
+                continue;
+            }
+
+            if (c == '\'' && !inDoubleQuote) {
+                inSingleQuote = !inSingleQuote;
+                continue;
+            }
+
+            if (c == '"' && !inSingleQuote) {
+                inDoubleQuote = !inDoubleQuote;
+                continue;
+            }
+
+            if (!inSingleQuote && !inDoubleQuote) {
+                // Safe to check for comments
+                if (c == '/' && i + 1 < jsCode.length()) {
+                    if (jsCode.charAt(i + 1) == '/') {
+                        // Single-line comment
+                        int end = jsCode.indexOf('\n', i + 2);
+                        if (end == -1) {
+                            end = jsCode.length();
+                        }
+                        comments.add(jsCode.substring(i, end));
+                        i = end - 1;
+                        continue;
+                    } else if (jsCode.charAt(i + 1) == '*') {
+                        // Multi-line comment
+                        int end = jsCode.indexOf("*/", i + 2);
+                        if (end == -1) {
+                            end = jsCode.length() - 2;
+                        }
+                        comments.add(jsCode.substring(i, end + 2));
+                        i = end + 1;
+                        continue;
+                    }
+                }
+            }
+        }
+        return comments;
+    }
+
     /**
      * Give back the risk associated to this vulnerability (high)
      *

addOns/ascanrules/src/test/java/org/zaproxy/zap/extension/ascanrules/ExternalRedirectScanRuleUnitTest.java

@@ -531,6 +531,47 @@ void shouldReportRedirectWithJsLocationMethods(String jsMethod) throws Exception
         assertThat(alertsRaised.get(0).getEvidence().startsWith(HttpHeader.HTTP), equalTo(true));
     }
 
+    @Test
+    void shouldNotReportRedirectIfInsideJsComment() throws Exception {
+        // Given
+        String test = "/";
+        String body =
+                """
+                <!DOCTYPE html>
+                <html>
+                <head>
+                <title>Redirect commented out</title>
+                </head>
+                <body>
+
+                <script>function myRedirectFunction()
+                {/*
+                    window.location.replace('@@@content@@@');
+                */}
+                //myRedirectFunction();</script>
+                """;
+        nano.addHandler(
+                new NanoServerHandler(test) {
+                    @Override
+                    protected NanoHTTPD.Response serve(NanoHTTPD.IHTTPSession session) {
+                        String site = getFirstParamValue(session, "site");
+                        if (site != null && !site.isEmpty()) {
+                            return newFixedLengthResponse(
+                                    NanoHTTPD.Response.Status.OK,
+                                    NanoHTTPD.MIME_HTML,
+                                    body.replace(CONTENT_TOKEN, site));
+                        }
+                        return newFixedLengthResponse("<html><body></body></html>");
+                    }
+                });
+        HttpMessage msg = getHttpMessage(test + "?site=xxx");
+        rule.init(msg, parent);
+        // When
+        rule.scan();
+        // Then
+        assertThat(alertsRaised.size(), equalTo(0));
+    }
+
     private static Stream<Arguments> createJsMethodBooleanPairs() {
         return Stream.of(
                 Arguments.of("location.reload", true),