From bc0091984ba93f8d16997a2f6811d4968f2529a0 Mon Sep 17 00:00:00 2001 From: Andrew Trompler <133278657+atrompler@users.noreply.github.com> Date: Fri, 9 Jun 2023 14:27:25 -0700 Subject: [PATCH] Improving rule accuracy (#523) --- Changelog.md | 6 +- .../Microsoft.DevSkim.CLI.csproj | 4 +- .../Microsoft.DevSkim.LanguageServer.csproj | 4 +- .../Microsoft.DevSkim.Tests.csproj | 10 +- ...evSkim.VisualStudio.SourceGenerator.csproj | 2 +- .../Microsoft.DevSkim.VisualStudio.csproj | 8 +- .../Microsoft.DevSkim.csproj | 2 +- guidance/DS132781.md | 12 ++ guidance/DS440001.md | 17 +++ rules/default/security/TLS/tls_generic.json | 18 +-- .../security/cryptography/hardcoded_tls.json | 11 +- .../security/cryptography/hash_algorithm.json | 1 + .../default/security/frameworks/android.json | 136 ++++++++++++++++++ .../default/security/xml/xslt_scripting.json | 49 +++++++ 14 files changed, 251 insertions(+), 29 deletions(-) create mode 100644 guidance/DS132781.md create mode 100644 guidance/DS440001.md create mode 100644 rules/default/security/frameworks/android.json create mode 100644 rules/default/security/xml/xslt_scripting.json diff --git a/Changelog.md b/Changelog.md index d51599e2..136515f7 100644 --- a/Changelog.md +++ b/Changelog.md @@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [1.0.8] - 2023-06-09 +### Rules +- Adds new rules and improves precision of some existing rules. + ## [1.0.7] - 2023-06-06 ### Fixed - Fixes issue where the CLI global tool package was attempting to run with a mismatched runtime. @@ -65,4 +69,4 @@ New: `devskim analyze -I path/to/src -O path/to/out.sarif` - Pack, test and catalogue commands removed from CLI ### Fixes -- Rule improvements and DevSkim engine performance and reliablity improvements. \ No newline at end of file +- Rule improvements and DevSkim engine performance and reliablity improvements. diff --git a/DevSkim-DotNet/Microsoft.DevSkim.CLI/Microsoft.DevSkim.CLI.csproj b/DevSkim-DotNet/Microsoft.DevSkim.CLI/Microsoft.DevSkim.CLI.csproj index 81df476a..eaff21dc 100644 --- a/DevSkim-DotNet/Microsoft.DevSkim.CLI/Microsoft.DevSkim.CLI.csproj +++ b/DevSkim-DotNet/Microsoft.DevSkim.CLI/Microsoft.DevSkim.CLI.csproj @@ -37,9 +37,9 @@ - + - + diff --git a/DevSkim-DotNet/Microsoft.DevSkim.LanguageServer/Microsoft.DevSkim.LanguageServer.csproj b/DevSkim-DotNet/Microsoft.DevSkim.LanguageServer/Microsoft.DevSkim.LanguageServer.csproj index 406bfc19..a0a32a43 100644 --- a/DevSkim-DotNet/Microsoft.DevSkim.LanguageServer/Microsoft.DevSkim.LanguageServer.csproj +++ b/DevSkim-DotNet/Microsoft.DevSkim.LanguageServer/Microsoft.DevSkim.LanguageServer.csproj @@ -13,10 +13,10 @@ - + - + diff --git a/DevSkim-DotNet/Microsoft.DevSkim.Tests/Microsoft.DevSkim.Tests.csproj b/DevSkim-DotNet/Microsoft.DevSkim.Tests/Microsoft.DevSkim.Tests.csproj index dff02ca4..00da7a88 100644 --- a/DevSkim-DotNet/Microsoft.DevSkim.Tests/Microsoft.DevSkim.Tests.csproj +++ b/DevSkim-DotNet/Microsoft.DevSkim.Tests/Microsoft.DevSkim.Tests.csproj @@ -9,13 +9,9 @@ - - - - - all - runtime; build; native; contentfiles; analyzers; buildtransitive - + + + diff --git a/DevSkim-DotNet/Microsoft.DevSkim.VisualStudio.SourceGenerator/Microsoft.DevSkim.VisualStudio.SourceGenerator.csproj b/DevSkim-DotNet/Microsoft.DevSkim.VisualStudio.SourceGenerator/Microsoft.DevSkim.VisualStudio.SourceGenerator.csproj index 56968b5d..4e729a89 100644 --- a/DevSkim-DotNet/Microsoft.DevSkim.VisualStudio.SourceGenerator/Microsoft.DevSkim.VisualStudio.SourceGenerator.csproj +++ b/DevSkim-DotNet/Microsoft.DevSkim.VisualStudio.SourceGenerator/Microsoft.DevSkim.VisualStudio.SourceGenerator.csproj @@ -13,7 +13,7 @@ - + diff --git a/DevSkim-DotNet/Microsoft.DevSkim.VisualStudio/Microsoft.DevSkim.VisualStudio.csproj b/DevSkim-DotNet/Microsoft.DevSkim.VisualStudio/Microsoft.DevSkim.VisualStudio.csproj index 3ced294d..99046a74 100644 --- a/DevSkim-DotNet/Microsoft.DevSkim.VisualStudio/Microsoft.DevSkim.VisualStudio.csproj +++ b/DevSkim-DotNet/Microsoft.DevSkim.VisualStudio/Microsoft.DevSkim.VisualStudio.csproj @@ -85,17 +85,17 @@ - + - 17.5.62 + 17.6.42 17.2.8 - + compile; build; native; contentfiles; analyzers; buildtransitive - + all runtime; build; native; contentfiles; analyzers; buildtransitive diff --git a/DevSkim-DotNet/Microsoft.DevSkim/Microsoft.DevSkim.csproj b/DevSkim-DotNet/Microsoft.DevSkim/Microsoft.DevSkim.csproj index 1b476442..f3084be2 100644 --- a/DevSkim-DotNet/Microsoft.DevSkim/Microsoft.DevSkim.csproj +++ b/DevSkim-DotNet/Microsoft.DevSkim/Microsoft.DevSkim.csproj @@ -24,7 +24,7 @@ - + diff --git a/guidance/DS132781.md b/guidance/DS132781.md new file mode 100644 index 00000000..f5c1aaa5 --- /dev/null +++ b/guidance/DS132781.md @@ -0,0 +1,12 @@ +## XSLT scripting should be disabled if possible + +### Summary +XSLT scripting contains significant security risks if used on untrustworthy data. + +### Details +TO DO - put more details of problem and solution here + +### References + +*[https://learn.microsoft.com/en-us/previous-versions/windows/desktop/ms763800(v=vs.85)](https://learn.microsoft.com/en-us/previous-versions/windows/desktop/ms763800(v=vs.85)) +* [https://learn.microsoft.com/en-us/dotnet/api/system.xml.xsl.xsltsettings.enablescript?view=net-8.0](https://learn.microsoft.com/en-us/dotnet/api/system.xml.xsl.xsltsettings.enablescript?view=net-8.0) diff --git a/guidance/DS440001.md b/guidance/DS440001.md new file mode 100644 index 00000000..778d259e --- /dev/null +++ b/guidance/DS440001.md @@ -0,0 +1,17 @@ +## Hard-coded Cryptography + +### Summary + +Usage of hard-coded cryptography reduces the crypto agility of an application, which can hinder attempts to move away from vulnerable cryptography. + +### Details + +Crypto agility is the ability of an application to migrate from one cryptographic algorithm to another. This ability is valuable both in situations where new, stronger cryptographic algorithms become available, and new attacks against existing cryptographic algorithms become known, making those algorithms no longer safe to use. + +Crypto agility includes being able to switch parameters of an existing algorithm, such as switching between AES-128 and AES-256. + +In order to facilitate this ability, hard-coding crypto algorithms and parameters is not recommended. + +### References + +* [https://learn.microsoft.com/en-us/archive/msdn-magazine/2009/brownfield/cryptographic-agility](https://learn.microsoft.com/en-us/archive/msdn-magazine/2009/brownfield/cryptographic-agility) diff --git a/rules/default/security/TLS/tls_generic.json b/rules/default/security/TLS/tls_generic.json index dd50dd81..179281c9 100644 --- a/rules/default/security/TLS/tls_generic.json +++ b/rules/default/security/TLS/tls_generic.json @@ -16,7 +16,7 @@ "DS440000" ], "severity": "ManualReview", - "rule_info": "DS440000.md", + "rule_info": "DS440001.md", "patterns": [ { "pattern": "\\b(SSL|D?TLS) ?v?[0123][0123_\\.]+", @@ -60,7 +60,7 @@ ], "severity": "ManualReview", "_comment": "Applies to all languages since many just wrap OpenSSL constructs.", - "rule_info": "DS440000.md", + "rule_info": "DS440001.md", "patterns": [ { "pattern": "SSLv2?3_method|D?TLSv1_([123]_)?(client_|server_)?method", @@ -129,7 +129,7 @@ ], "severity": "ManualReview", "_comment": "Applies to all languages since many just wrap OpenSSL constructs.", - "rule_info": "DS440000.md", + "rule_info": "DS440001.md", "patterns": [ { "pattern": "\\b(SSL|D?TLS) ?v?[0123_\\.]*_VERSION(_MAJOR)?", @@ -158,7 +158,7 @@ ], "severity": "ManualReview", "_comment": "Applies to all languages since many just wrap GnuTLS constructs.", - "rule_info": "DS440000.md", + "rule_info": "DS440001.md", "patterns": [ { "pattern": "GNUTLS_(SSL|D?TLS)[01239_\\.]+", @@ -187,7 +187,7 @@ ], "severity": "ManualReview", "_comment": "Applies to all languages since many just wrap LibreSSL constructs.", - "rule_info": "DS440000.md", + "rule_info": "DS440001.md", "patterns": [ { "pattern": "(SSL|D?TLS)[01239_\\.]+_VERSION(_MAJOR|_MINOR)?", @@ -216,7 +216,7 @@ ], "severity": "ManualReview", "_comment": "Applies to all languages since many just wrap mbedTLS constructs.", - "rule_info": "DS440000.md", + "rule_info": "DS440001.md", "patterns": [ { "pattern": "MBEDTLS_SSL_PROTO_(SSL|D?TLS)[123_]+", @@ -252,7 +252,7 @@ "Cryptography.Protocol.TLS.Hard-Coded" ], "severity": "ManualReview", - "rule_info": "DS440000.md", + "rule_info": "DS440001.md", "patterns": [ { "pattern": "--(sslv2|sslv3|tlsv1|tlsv11|tlsv1\\.1|tlsv1\\.2)", @@ -294,7 +294,7 @@ "Cryptography.Protocol.TLS.Hard-Coded" ], "severity": "ManualReview", - "rule_info": "DS440000.md", + "rule_info": "DS440001.md", "patterns": [ { "pattern": "--secure-protocol=", @@ -365,7 +365,7 @@ "Cryptography.Protocol.TLS.Elliptic-Curve.Hard-Coded" ], "severity": "ManualReview", - "rule_info": "DS440100.md", + "rule_info": "DS440001.md", "patterns": [ { "pattern": "arbitrary_explicit_char2_curves|arbitrary_explicit_prime_curves|brainpoolP160r1|brainpoolP160t1|brainpoolP192r1|brainpoolP192t1|brainpoolP224r1|brainpoolP224t1|brainpoolP256r1|brainpoolp256r1|brainpoolP256t1|brainpoolP320r1|brainpoolP320t1|brainpoolP384r1|brainpoolp384r1|brainpoolP384t1|brainpoolP512r1|brainpoolp512r1|brainpoolP512t1|curve25519|ec192wapi|nistP192|nistP224|nistP256|nistP384|nistP521|numsP256t1|numsP384t1|numsP512t1|secP160k1|secp160k1|secP160r1|secp160r1|secP160r2|secp160r2|secP192k1|secp192k1|secP192r1|secp192r1|secP224k1|secp224k1|secP224r1|secp224r1|secP256k1|secp256k1|secP256r1|secp256r1|secP384r1|secp384r1|secP521r1|secp521r1|sect163k1|sect163r1|sect163r2|sect193r1|sect193r2|sect233k1|sect233r1|sect239k1|sect283k1|sect283r1|sect409k1|sect409r1|sect571k1|sect571r1|wtls12|wtls7|wtls9|X25519|X448|x962P192v1|x962P192v2|x962P192v3|x962P239v1|x962P239v2|x962P239v3|x962P256v1", diff --git a/rules/default/security/cryptography/hardcoded_tls.json b/rules/default/security/cryptography/hardcoded_tls.json index b5ea6da5..dd0d0774 100644 --- a/rules/default/security/cryptography/hardcoded_tls.json +++ b/rules/default/security/cryptography/hardcoded_tls.json @@ -13,10 +13,17 @@ ], "severity": "important", "rule_info": "DS440000.md", - "must-not-match": ["LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCg==", "curl --tlsv1.3"], + "must-match": [ + "SecurityProtocolType.Tls11" + ], + "must-not-match": [ + "LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCg==", + "curl --tlsv1.3", + "axolotls" + ], "patterns": [ { - "pattern": "(SSL|D?TLS)[_v]*[123]([123_\\.])*", + "pattern": "(?<=\\W)(SSL|D?TLS)[_v]*[123]([123_\\.])*", "type": "regex", "scopes": [ "code" diff --git a/rules/default/security/cryptography/hash_algorithm.json b/rules/default/security/cryptography/hash_algorithm.json index 40ddaa2b..707ce072 100644 --- a/rules/default/security/cryptography/hash_algorithm.json +++ b/rules/default/security/cryptography/hash_algorithm.json @@ -7,6 +7,7 @@ "tags": [ "Cryptography.BannedHashAlgorithm" ], + "does_not_apply_to": ["json"], "severity": "critical", "rule_info": "DS126858.md", "patterns": [ diff --git a/rules/default/security/frameworks/android.json b/rules/default/security/frameworks/android.json new file mode 100644 index 00000000..2e73c40b --- /dev/null +++ b/rules/default/security/frameworks/android.json @@ -0,0 +1,136 @@ +[ + { + "name": "Android debug is enabled.", + "id": "DS180000", + "description": "The android:debuggable element is set to true, which should be disabled for release builds.", + "recommendation": "Set android:debuggable to false for release builds.", + "applies_to_file_regex": [ + "AndroidManifest.xml" + ], + "tags": [ + "Framework.Android" + ], + "severity": "ManualReview", + "rule_info": "DS180000.md", + "patterns": [ + { + "xpaths": ["//default:application/@android:debuggable"], + "xpathnamespaces": { + "default": "http://maven.apache.org/POM/4.0.0", + "android": "http://schemas.android.com/apk/res/android" + } + "pattern": "true", + "type": "regex", + "scopes": [ + "code" + ], + "modifiers" : ["i"] + } + ], + "fix_its": [ + { + "name": "Change to false", + "type": "RegexReplace", + "replacement": "false", + "pattern": { + "pattern": "true", + "type": "regex", + "scopes": [ + "code" + ], + "_comment": "" + } + } + ], + "must-match": [ + "" + ], + "must-not-match": [ + "" + ] + }, + { + "name": "Android debug is enabled.", + "id": "DS180001", + "description": "The setWebContentsDebuggingEnabled element is set to true, which should be disabled for release builds.", + "recommendation": "Set setWebContentsDebuggingEnabled to false for release builds.", + "applies_to": [ + "java" + ], + "tags": [ + "Framework.Android" + ], + "severity": "ManualReview", + "rule_info": "DS180000.md", + "patterns": [ + { + "pattern": "setWebContentsDebuggingEnabled\\(true\\)", + "type": "regex", + "scopes": [ + "code" + ], + "modifiers" : ["i"] + } + ], + "fix_its": [ + { + "name": "Change to false", + "type": "RegexReplace", + "replacement": "false", + "pattern": { + "pattern": "true", + "type": "regex", + "scopes": [ + "code" + ], + "_comment": "" + } + } + ], + "must-match": [ + "setWebContentsDebuggingEnabled(true)" + ], + "must-not-match": [ + "setWebContentsDebuggingEnabled(false)" + ] + }, + { + "name": "Android StrictMode is enabled.", + "id": "DS180002", + "description": "StrictMode is detected, which is useful for developers but should be disabled for release builds.", + "recommendation": "Disable StrictMode for release builds.", + "applies_to": [ + "java" + ], + "tags": [ + "Framework.Android" + ], + "severity": "ManualReview", + "rule_info": "DS180001.md", + "patterns": [ + { + "pattern": "StrictMode.setThreadPolicy(", + "type": "substring", + "scopes": [ + "code" + ], + "modifiers" : ["i"] + }, + { + "pattern": "StrictMode.setVmPolicy(", + "type": "substring", + "scopes": [ + "code" + ], + "modifiers" : ["i"] + } + ], + "must-match": [ + "StrictMode.setThreadPolicy(new StrictMode.ThreadPolicy.Builder()", + "StrictMode.setVmPolicy(new StrictMode.VmPolicy.Builder()" + ], + "must-not-match": [ + "application(StrictMode)" + ] + } +] diff --git a/rules/default/security/xml/xslt_scripting.json b/rules/default/security/xml/xslt_scripting.json new file mode 100644 index 00000000..a930e3bd --- /dev/null +++ b/rules/default/security/xml/xslt_scripting.json @@ -0,0 +1,49 @@ +[ + { + "name": "XSLT Scripting Enabled", + "id": "DS132781", + "description": "XSLT Scripting is a feature that should only be enabled if script support is necessary and you are certain this is used in a trusted environment.", + "recommendation": "Disable XSLT scripting.", + "applies_to": [ + "CSharp" + ], + "tags": [ + "XSLT" + ], + "severity": "ManualReview", + "rule_info": "DS132781.md", + "patterns": [ + { + "pattern": "EnableScript\\s*=\\s*(true|TRUE)", + "type": "RegexWord", + "scopes": [ + "code" + ] + } + ], + "conditions" : [ + { + "pattern" : + { + "pattern": "XsltSettings", + "type": "regex", + "scopes": [ + "code" + ], + "_comment": "EnableScript is a property of XsltSettings." + }, + "negate_finding": false, + "search_in": "only-before" + } + ], + "must-match": [ + "XsltSettings n = new XsltSettings();\n n.EnableScript=true", + "XsltSettings n = new XsltSettings(){ EnableScript = true };", + "var settings = new XsltSettings(){ EnableScript = true };", + "XsltSettings n = new(){ EnableScript = true };" + ], + "must-not-match": [ + "CustomObject object = new CustomObject();\n object.EnableScript=true" + ] + } +] \ No newline at end of file