From 3a227bd77da9ca96155e76b605bf4cae9cf7f8e7 Mon Sep 17 00:00:00 2001
From: Fabian Meumertzheim <meumertzheim@code-intelligence.com>
Date: Wed, 10 Mar 2021 22:28:02 +0100
Subject: [PATCH] [json-sanitizer] Add severity markup (#5350)

Annotates the findings of the various json-sanitizer fuzzers with
severities as follows:

* XSS: High
* Comment injection: Medium
* Invalid JSON: Low
* Failure to be idempotent: Not a security issue
* Undeclared exceptions: Not a security issue

This commit takes advantage of the support for severity markers in stack
traces introduced in https://github.com/google/clusterfuzz/pull/2270.
---
 projects/json-sanitizer/DenylistFuzzer.java   | 25 +++++++++++++------
 .../json-sanitizer/IdempotenceFuzzer.java     | 16 ++++++------
 projects/json-sanitizer/ValidJsonFuzzer.java  |  9 ++++---
 3 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/projects/json-sanitizer/DenylistFuzzer.java b/projects/json-sanitizer/DenylistFuzzer.java
index 9f810224a..4e73cfcb7 100644
--- a/projects/json-sanitizer/DenylistFuzzer.java
+++ b/projects/json-sanitizer/DenylistFuzzer.java
@@ -15,7 +15,8 @@
 ////////////////////////////////////////////////////////////////////////////////
 
 import com.code_intelligence.jazzer.api.FuzzedDataProvider;
-
+import com.code_intelligence.jazzer.api.FuzzerSecurityIssueHigh;
+import com.code_intelligence.jazzer.api.FuzzerSecurityIssueMedium;
 import com.google.json.JsonSanitizer;
 
 public class DenylistFuzzer {
@@ -29,12 +30,20 @@ public class DenylistFuzzer {
       // exceeded.
       return;
     }
-    // See https://github.com/OWASP/json-sanitizer#output.
-    if (output.contains("</script") || output.contains("<script")
-        || output.contains("<!--") || output.contains("]]>")) {
-      System.err.println("input : " + input);
-      System.err.println("output: " + output);
-      throw new IllegalStateException("Output contains forbidden substring");
-    }
+
+    // Check for forbidden substrings. As these would enable Cross-Site
+    // Scripting, treat every finding as a high severity vulnerability.
+    assert !output.contains("</script")
+        : new FuzzerSecurityIssueHigh("Output contains </script");
+    assert !output.contains("]]>")
+        : new FuzzerSecurityIssueHigh("Output contains ]]>");
+
+    // Check for more forbidden substrings. As these would not directly enable
+    // Cross-Site Scripting in general, but may impact script execution on the
+    // embedding page, treat each finding as a medium severity vulnerability.
+    assert !output.contains("<script")
+        : new FuzzerSecurityIssueMedium("Output contains <script");
+    assert !output.contains("<!--")
+        : new FuzzerSecurityIssueMedium("Output contains <!--");
   }
 }
diff --git a/projects/json-sanitizer/IdempotenceFuzzer.java b/projects/json-sanitizer/IdempotenceFuzzer.java
index 833ec3f0a..a42c91af9 100644
--- a/projects/json-sanitizer/IdempotenceFuzzer.java
+++ b/projects/json-sanitizer/IdempotenceFuzzer.java
@@ -21,20 +21,18 @@ import com.google.json.JsonSanitizer;
 public class IdempotenceFuzzer {
   public static void fuzzerTestOneInput(FuzzedDataProvider data) {
     String input = data.consumeRemainingAsString();
-    String output1;
+    String output;
     try {
-      output1 = JsonSanitizer.sanitize(input, 10);
+      output = JsonSanitizer.sanitize(input, 10);
     } catch (ArrayIndexOutOfBoundsException e) {
       // ArrayIndexOutOfBoundsException is expected if nesting depth is
       // exceeded.
       return;
     }
-    String output2 = JsonSanitizer.sanitize(output1, 10);
-    if (!output1.equals(output2)) {
-      System.err.println("input  : " + input);
-      System.err.println("output1: " + output1);
-      System.err.println("output2: " + output2);
-      throw new IllegalStateException("Non-idempotence detected");
-    }
+
+    // Ensure that sanitizing twice does not give different output
+    // (idempotence). Since failure to be idempotent is not a security issue in
+    // itself, fail with a regular AssertionError.
+    assert JsonSanitizer.sanitize(output).equals(output) : "Not idempotent";
   }
 }
diff --git a/projects/json-sanitizer/ValidJsonFuzzer.java b/projects/json-sanitizer/ValidJsonFuzzer.java
index 7430b9c92..c8fbe0386 100644
--- a/projects/json-sanitizer/ValidJsonFuzzer.java
+++ b/projects/json-sanitizer/ValidJsonFuzzer.java
@@ -15,6 +15,7 @@
 ////////////////////////////////////////////////////////////////////////////////
 
 import com.code_intelligence.jazzer.api.FuzzedDataProvider;
+import com.code_intelligence.jazzer.api.FuzzerSecurityIssueLow;
 
 import com.google.gson.Gson;
 import com.google.gson.JsonElement;
@@ -33,12 +34,14 @@ public class ValidJsonFuzzer {
       // exceeded.
       return;
     }
+
+    // Check that the output is valid JSON. Invalid JSON may crash other parts
+    // of the application that trust the output of the sanitizer.
     try {
+      Gson gson = new Gson();
       gson.fromJson(output, JsonElement.class);
     } catch (Exception e) {
-      System.err.println("input  : " + input);
-      System.err.println("output : " + output);
-      throw e;
+      throw new FuzzerSecurityIssueLow("Output is invalid JSON", e);
     }
   }
 }