SONAR-15408 Custom config for security analysis supports validators (#4735)

Co-authored-by: MikeBirnstiehl <michael.birnstiehl@sonarsource.com>

diff --git a/server/sonar-docs/src/pages/analysis/security_configuration.md b/server/sonar-docs/src/pages/analysis/security_configuration.md
index 29984218fde91842f1657fee2629d0b6bf573414..ef225262fe2d4540466dfc191d76bed6167b6dd7 100644 (file)
--- a/server/sonar-docs/src/pages/analysis/security_configuration.md
+++ b/server/sonar-docs/src/pages/analysis/security_configuration.md
@@ -90,7 +90,10 @@ You can add the following elements to your custom configuration:
 
 * **Source** – Where you get user data. You should always consider user data tainted and vulnerable to injection attacks.
   Example: Calling `HttpServletRequest#getParam("foo")` will return tainted content
-* **Sanitizer** – Finds and removes malicious content from tainted data.
+* **Sanitizer** – Finds and removes malicious content from one or more potentially tainted arguments.
+  Example: `DatabaseUtils#sqlEscapeString(String str)` returns a modified version of `str` where characters used in an SQL injection attack are removed.    
+* **Validator** - Marks one or more arguments as safe from malicious content.
+  Example: `String#matches(String str)` can be used to verify that `str` does not contain any content which may be used in an injection attack. 
 * **Passthrough** – Allows you to keep track of tainted data sent to a library outside the current function. When you pass a tainted value to a library function outside the current function, SonarQube automatically assumes it's being passed to a sanitizer. If the tainted data isn't being passed to a sanitizer, you can set up a passthrough to keep track of the data.
 * **Sink** – A piece of code that can perform a security-sensitive task. Data should not contain any malicious content once it reaches a sink.
   Example: Running an SQL query with `java.sql.Statement#execute`
@@ -192,7 +195,18 @@ Your JSON file should include the rule you're adding a custom element to, the el
 |     ],
 |     "sanitizers": [
 |       {
-|         "methodId": "my.package.StringUtils#stringReplace(Ljava/lang/String;Ljava/lang/String;)Ljava/lang/String;"
+|         "methodId": "my.package.StringUtils#stringReplace(Ljava/lang/String;Ljava/lang/String;)Ljava/lang/String;",
+|         "args": [
+|           2 
+|         ]
+|       }
+|     ],
+|     "validators": [
+|       {
+|         "methodId": "my.package.StringUtils#equals(Ljava/lang/String;)Z",
+|         "args": [
+|           1
+|         ]
 |       }
 |     ],
 |     "passthroughs": [
@@ -248,7 +262,8 @@ Your JSON file should include the rule you're adding a custom element to, the el
 |
 | The `args` is the index of the parameter that can receive a tainted variable. Index starts:
 | * `1` for a function call. 
-| * `0` for a method call, index `0` being the current instance (`this`)
+| * `0` for a method call, index `0` being the current instance (`this`).
+| The `args` field must be a non-empty array of non-negative integers, and it is a mandatory field for sanitizers and validators.
 
 [[collapse]]
 | ## PHP JSON file example
@@ -263,7 +278,18 @@ Your JSON file should include the rule you're adding a custom element to, the el
 |     ],
 |     "sanitizers": [
 |       {
-|         "methodId": "str_replace"
+|         "methodId": "str_replace",
+|         "args": [
+|           3
+|         ]
+|       }
+|     ],
+|     "validators": [
+|       {
+|         "methodId": "My\\Namespace\\Validator\\inArray::isValid",
+|         "args": [
+|           1
+|         ]
 |       }
 |     ],
 |     "passthroughs": [
@@ -319,7 +345,8 @@ Your JSON file should include the rule you're adding a custom element to, the el
 |
 | The `args` is the index of the parameter that can receive a tainted variable. Index starts:
 | * `1` for a function call. 
-| * `0` for a method call, index `0` being the current instance (`this`)
+| * `0` for a method call, index `0` being the current instance (`this`).
+| The `args` field must be a non-empty array of non-negative integers, and it is a mandatory field for sanitizers and validators.
 
 [[collapse]]
 | ## C# JSON file example
@@ -334,7 +361,18 @@ Your JSON file should include the rule you're adding a custom element to, the el
 |     ],
 |     "sanitizers": [
 |       {
-|         "methodId": "My.Namespace.StringUtils.StringReplace(string, string)"
+|         "methodId": "My.Namespace.StringUtils.StringReplace(string, string)",
+|         "args": [
+|           0
+|         ]
+|       }
+|     ],
+|     "validators": [
+|       {
+|         "methodId": "My.Namespace.StringUtils.Regex.Matches(string)",
+|         "args": [
+|           0
+|         ]
 |       }
 |     ],
 |     "passthroughs": [
@@ -391,7 +429,8 @@ Your JSON file should include the rule you're adding a custom element to, the el
 |
 | The `args` is the index of the parameter that can receive a tainted variable. Index starts:
 | * `1` for a function call. 
-| * `0` for a method call, index `0` being the current instance (`this`)
+| * `0` for a method call, index `0` being the current instance (`this`).
+| The `args` field must be a non-empty array of non-negative integers, and it is a mandatory field for sanitizers and validators.
 
 [[collapse]]
 | ## Python JSON file example
@@ -406,7 +445,18 @@ Your JSON file should include the rule you're adding a custom element to, the el
 |     ],
 |     "sanitizers": [
 |       {
-|         "methodId": "str_replace"
+|         "methodId": "str_replace",
+|         "args": [
+|           1
+|         ]
+|       }
+|     ],
+|     "validators": [
+|       {
+|         "methodId": "my.namespace.regex.matches",
+|         "args": [
+|           1
+|         ]
 |       }
 |     ],
 |     "passthroughs": [
@@ -463,7 +513,8 @@ Your JSON file should include the rule you're adding a custom element to, the el
 |
 | The `args` is the index of the parameter that can receive a tainted variable. Index starts:
 | * `1` for a function call. 
-| * `0` for a method call, index `0` being the current instance (`this`)
+| * `0` for a method call, index `0` being the current instance (`this`).
+| The `args` field must be a non-empty array of non-negative integers, and it is a mandatory field for sanitizers and validators.
 
 ### (Deprecated) Customizing through analysis parameters
 
@@ -515,7 +566,18 @@ Configuration is provided using JSON files. Click the heading below to expand an
 |   ],
 |   "sanitizers": [
 |     {
-|       "methodId": "str_replace"
+|       "methodId": "str_replace",
+|       "args": [
+|         3
+|       ]
+|     }
+|   ],
+|   "validators": [
+|     {
+|      "methodId": "My\\Namespace\\Validator\\inArray::isValid",
+|      "args": [
+|         1
+|       ]
 |     }
 |   ],
 |  "passthroughs": [
@@ -553,7 +615,8 @@ Configuration is provided using JSON files. Click the heading below to expand an
 |
 | The `args` is the index of the parameter that can receive a tainted variable. Index starts:
 | * `1` for a function call. 
-| * `0` for a method call, index `0` being the current instance (`this`)  
+| * `0` for a method call, index `0` being the current instance (`this`)  .
+| The `args` field must be a non-empty array of non-negative integers, and it is a mandatory field for sanitizers and validators.
 
 ## Deactivating the core configuration
 

diff --git a/server/sonar-webserver-webapi/src/main/resources/json-schemas/security.json b/server/sonar-webserver-webapi/src/main/resources/json-schemas/security.json
index 8b1cd81748c1411811ddfff7797f81b45ef9a656..e064fa83b54eb191ca8817580012a5d859936593 100644 (file)
--- a/server/sonar-webserver-webapi/src/main/resources/json-schemas/security.json
+++ b/server/sonar-webserver-webapi/src/main/resources/json-schemas/security.json
@@ -18,8 +18,10 @@
         "args": {
           "type": "array",
           "items": {
-            "type": "integer"
-          }
+            "type": "integer",
+            "minimum": 0
+          },
+          "minItems": 1
         },
         "interval": {
           "$ref": "#/definitions/Interval"
@@ -42,6 +44,16 @@
       ],
       "additionalProperties": false
     },
+    "SanitizerOrValidator": {
+      "allOf": [
+        {
+          "$ref": "#/definitions/CommonConfiguration"
+        }
+      ],
+      "required": [
+        "args"
+      ]
+    },
     "RuleConfiguration": {
       "type": "object",
       "properties": {
@@ -66,7 +78,13 @@
         "sanitizers": {
           "type": "array",
           "items": {
-            "$ref": "#/definitions/CommonConfiguration"
+            "$ref": "#/definitions/SanitizerOrValidator"
+          }
+        },
+        "validators": {
+          "type": "array",
+          "items": {
+            "$ref": "#/definitions/SanitizerOrValidator"
           }
         },
         "sinks": {
@@ -89,4 +107,4 @@
   "additionalProperties": {
     "$ref": "#/definitions/RuleConfiguration"
   }
-}
\ No newline at end of file
+}

diff --git a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/setting/ws/SetActionTest.java b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/setting/ws/SetActionTest.java
index f2156a5814d9fd9e760185d57d1f9ec299f444da..262e1d10ac8e2ee2a6d51c73b71d79b07fd0c127 100644 (file)
--- a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/setting/ws/SetActionTest.java
+++ b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/setting/ws/SetActionTest.java
@@ -459,7 +459,18 @@ public class SetActionTest {
       "    ],\n" +
       "    \"sanitizers\": [\n" +
       "      {\n" +
-      "        \"methodId\": \"str_replace\"\n" +
+      "        \"methodId\": \"str_replace\"," +
+      "        \"args\": [\n" +
+      "           0\n" +
+      "         ]\n" +
+      "      }\n" +
+      "    ],\n" +
+      "    \"validators\": [\n" +
+      "      {\n" +
+      "        \"methodId\": \"is_valid\"," +
+      "        \"args\": [\n" +
+      "           1\n" +
+      "         ]\n" +
       "      }\n" +
       "    ],\n" +
       "    \"sinks\": [\n" +
@@ -516,6 +527,69 @@ public class SetActionTest {
       .hasMessageContaining("S3649/sinks/0/methodId: expected type: String, found: Integer");
   }
 
+  @Test
+  @UseDataProvider("securityJsonProperties")
+  public void fail_json_schema_validation_when_sanitizers_have_no_args(String securityPropertyKey) {
+    String security_custom_config = "{\n" +
+      "  \"S3649\": {\n" +
+      "    \"sources\": [\n" +
+      "      {\n" +
+      "        \"methodId\": \"My\\\\Namespace\\\\ClassName\\\\ServerRequest::getQuery\"\n" +
+      "      }\n" +
+      "    ],\n" +
+      "    \"sanitizers\": [\n" +
+      "       {\n" +
+      "         \"methodId\": \"SomeSanitizer\"\n" +
+      "       }\n" +
+      "    ]\n" +
+      "  }\n" +
+      "}";
+    definitions.addComponent(PropertyDefinition
+      .builder(securityPropertyKey)
+      .name("foo")
+      .description("desc")
+      .category("cat")
+      .subCategory("subCat")
+      .type(PropertyType.JSON)
+      .build());
+
+    assertThatThrownBy(() -> callForGlobalSetting(securityPropertyKey, security_custom_config))
+      .isInstanceOf(IllegalArgumentException.class)
+      .hasMessageContaining("Provided JSON is invalid [#/S3649/sanitizers/0: #: only 1 subschema matches out of 2]");
+  }
+
+  @Test
+  @UseDataProvider("securityJsonProperties")
+  public void fail_json_schema_validation_when_validators_have_empty_args_array(String securityPropertyKey) {
+    String security_custom_config = "{\n" +
+      "  \"S3649\": {\n" +
+      "    \"sources\": [\n" +
+      "      {\n" +
+      "        \"methodId\": \"My\\\\Namespace\\\\ClassName\\\\ServerRequest::getQuery\"\n" +
+      "      }\n" +
+      "    ],\n" +
+      "    \"validators\": [\n" +
+      "       {\n" +
+      "         \"methodId\": \"SomeValidator\",\n" +
+      "         \"args\": []\n" +
+      "       }\n" +
+      "    ]\n" +
+      "  }\n" +
+      "}";
+    definitions.addComponent(PropertyDefinition
+      .builder(securityPropertyKey)
+      .name("foo")
+      .description("desc")
+      .category("cat")
+      .subCategory("subCat")
+      .type(PropertyType.JSON)
+      .build());
+
+    assertThatThrownBy(() -> callForGlobalSetting(securityPropertyKey, security_custom_config))
+      .isInstanceOf(IllegalArgumentException.class)
+      .hasMessageContaining("Provided JSON is invalid [#/S3649/validators/0: #: only 1 subschema matches out of 2]");
+  }
+
   @Test
   @UseDataProvider("securityJsonProperties")
   public void fail_json_schema_validation_when_property_has_unknown_attribute(String securityPropertyKey) {