From 2eabb04b4f4f9c4776b02ec549be63ede4f844c3 Mon Sep 17 00:00:00 2001 From: Gyula Sallai <61680650+gyula-sallai-sonarsource@users.noreply.github.com> Date: Tue, 21 Sep 2021 08:30:46 +0200 Subject: [PATCH] SONAR-15408 Custom config for security analysis supports validators (#4735) Co-authored-by: MikeBirnstiehl --- .../pages/analysis/security_configuration.md | 85 ++++++++++++++++--- .../main/resources/json-schemas/security.json | 26 +++++- .../server/setting/ws/SetActionTest.java | 76 ++++++++++++++++- 3 files changed, 171 insertions(+), 16 deletions(-) diff --git a/server/sonar-docs/src/pages/analysis/security_configuration.md b/server/sonar-docs/src/pages/analysis/security_configuration.md index 29984218fde..ef225262fe2 100644 --- 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 8b1cd81748c..e064fa83b54 100644 --- 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 f2156a5814d..262e1d10ac8 100644 --- 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) { -- 2.39.5