You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The DevSkim ruleset is intended to cover the Microsoft SDL. Auditing the shipped rules against the 172 current SDL requirement documents found real gaps, and more importantly found that several shipped rules could never have matched anything.
The most significant finding: four rules existed in the repository but had never run for any user.Microsoft.DevSkim.csproj embeds rule files through a hand-maintained <EmbeddedResource> allowlist rather than a wildcard, and android.json and xslt_scripting.json were missing from it. The shipped set was 123 rules, not the 127 on disk. This is also why their dangling rule_info references never failed CI: the tests iterate the shipped set, so they never saw them.
What changed
Shipped rules go from 123 to 156, and every rule now has a positive self-test (previously 32 had none).
Rules that could never match. Writing those missing self-tests found three more dead rules mechanically, none of which were visible by reading the ruleset:
Rule
Defect
DS191340
Used $1 as a backreference. .NET spells that \1, and $ is an end-of-line anchor, so the pattern required end-of-line followed by 1.
DS440016
--secure-protocol= typed as string, which is word-boundary anchored. A leading hyphen is not a word character.
DS440060
Empty patterns array, so it could not produce a finding while still occupying a SARIF metadata entry.
DS180000
Bound the default XML namespace to the Maven POM namespace. A real AndroidManifest.xml has none.
DS132781
applies_to: ["CSharp"]; language names match exactly against languages.json, which defines csharp.
DS180000 is worth a second look during review: it had a self-test, and the self-test passed. The sample had been written to fit the broken XPath rather than to describe a real manifest, so it locked the defect in instead of catching it.
Coverage.
Banned APIs (ADM.10082). The requirement enumerates 194 functions and names DevSkim as a detection tool. 113 were not reported: 108 absent, and 5 present only in the wrong case, which RegexWord does not match. DS154189 goes from 80 to 178 alternatives, with four groups split into their own IDs where the rationale and useful severity differ.
Kubernetes (ADM.10210), 8 rules. These are the first rules to use the engine's ymlpaths support, which no shipped rule had used. Not a language gap; yaml, json and xml were already defined.
Deserialization (ADM.10010), XXE, secrets (AccessControl.10100), package sources (ADM.10205). Each had a narrow slice of the requirement. XXE was Objective-C and Swift only. The secret rules keyed on 30+ lowercase hex characters and so matched no modern credential format.
Language reach.javascriptreact and typescriptreact were defined and mapped to .jsx/.tsx but named by no rule, so React source received none of the JS/TS rules. Thirteen languages added; because 32 rules declare no applies_to, each addition extends those 32 rules to a previously skipped file type. Extensionless files (Dockerfile, Makefile, id_rsa) needed the file-names property, which no language definition had used.
ApplicationInspector 1.10.2. Picks up the boolean expression support from ApplicationInspector#654 plus five rules-engine fixes. One of those matters here specifically: override suppression on the sync path DevSkim uses now requires full containment rather than "starts inside". 38 DevSkim rules use overrides and no rule shipped with ApplicationInspector does, so DevSkim is the first consumer to exercise it. Every override pair was measured and brought into containment before the bump, so the upgrade lands on a ruleset that is ready for it.
Worth a careful look
DS132784 (Java XXE) is deliberately unsound in the safe direction. Six hardening indicators are each negated over same-file scope, so it fires only when none is present. A file that hardens one factory and leaves a second unhardened will not report. It is ManualReview for that reason and the guidance states the limitation rather than letting a clean result imply every parser is safe.
Duplicate rule IDs were treated case by case.DS440011 was two unrelated rules sharing an ID, which made SARIF serve one rule's findings under the other's name and helpUri; that one is split. DS440016 and DS148264 are each one logical rule split across entries and are deliberately left alone, because the shared ID is what lets a single suppression cover the whole rule.
DS114352 uses a plain regex rather than jsonpaths on purpose. An earlier revision used jsonpaths, which makes the engine parse every JSON file it scans; .vscode/*.json is JSONC and fails a strict parse, so a near-universal repository layout produced error-level log output. No rule uses jsonpaths now.
Secret rules match only issuer-assigned prefixes, not entropy. The requirement directs teams to CredScan, SPMI and GitHub secret scanning, and DevSkim should not pretend to replace them; the value here is catching a credential in the editor before it is committed.
Verification
Full suite passes on net8.0, net9.0 and net10.0. Every new rule was checked by scanning fixtures rather than by self-tests alone, including negative fixtures of the recommended safe alternatives. Scanning DevSkim itself produces no findings outside the rules' own example strings.
Verification was run against the published Microsoft.CST.ApplicationInspector.RulesEngine and ...Logging 1.10.2 packages, which are the versions this PR consumes. 1.10.2 adds the verifier fix for rules that combine expression and conditions, so the boolean-expression rules below pass the verifier that CI runs.
Follow-up, not included here
TLS consolidation is also deferred, since much of that sprawl exists because the engine could not express per-pattern conditions, and consolidating before that lands would mean rewriting the same rules twice.
android.json and xslt_scripting.json were absent from the explicit
<EmbeddedResource> allowlist in Microsoft.DevSkim.csproj, which is
hand-maintained rather than a wildcard. DS180000, DS180001, DS180002 and
DS132781 therefore parsed and validated on disk but were not part of
DevSkimRuleSet.GetDefaultRuleSet(), so they never ran for any user.
This is also why the dangling rule_info references in those two files
never failed CI: DefaultRulesTests iterates the shipped set, so it never
saw them. The missing guidance is the likely reason the rules were held
back originally.
Add both files to the allowlist and fix what that surfaces:
- Write the four missing guidance documents.
- Point DS180001 and DS180002 at their own guidance. They were shifted by
one, referencing DS180000.md and DS180001.md respectively.
- Correct DS132781's applies_to from "CSharp" to "csharp". Language names
are matched by exact string against languages.json, which defines
"csharp"; verified that the rule reports nothing until this is fixed.
- Correct DS180000's XPath. It bound the default namespace to the Maven
POM namespace and matched //default:application, but AndroidManifest.xml
has no default namespace, so the rule could not fire on a real manifest.
Its self-test used a manifest with xmlns set to the Maven namespace, so
it passed while testing something that does not occur in practice.
Bind only the android prefix, match //application, and replace the
self-tests with a realistic manifest plus a negative case for an absent
attribute.
Shipped rule count moves from 123 to 127 and no rule now references
missing guidance. Verified by scanning a real AndroidManifest.xml, a Java
source file and a C# source file with no -r argument, so the matches come
from the embedded default ruleset.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 4fc65bf3-6ed4-40fe-b21f-848c191b8bc8
Two unrelated rules shared the ID DS440011: "OpenSSL: Hard-coded SSL/TLS
Protocol" in TLS/tls_generic.json, whose guidance is DS440001.md, and
"OpenSSL: Do not hardcode SSL/TLS versions within an application." in
cryptography/hardcoded_tls.json, whose guidance is DS440000.md.
SARIF emits one tool.driver.rules entry per rule ID, so the collision left
a single entry carrying only the first rule's metadata. Findings from the
hardcoded_tls.json rule were reported with the other rule's name and a
helpUri pointing at DS440001.md, sending readers to the wrong guidance
document. Suppressing either ID also suppressed the other. Renumber the
hardcoded_tls.json rule to DS440017, which now emits its own SARIF entry
linking to DS440000.md.
The other repeated IDs are deliberate and are left alone. DS440016 (x2)
and DS148264 (x3) are each one logical rule split across entries, by
condition presence and by target language respectively. Every copy shares
a name and a guidance file, so the single SARIF entry describes them
correctly, and the shared ID means one suppression directive covers the
whole rule. Splitting those would regress suppression without fixing
anything.
Also normalise DS450003's severity from "manualreview" to "ManualReview"
for consistency with the other 44 manual-review rules. Enum parsing is
case-insensitive, so this is a readability change rather than a fix.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 4fc65bf3-6ed4-40fe-b21f-848c191b8bc8
ADM.10082 enumerates 194 banned APIs and names DevSkim as a detection
tool for it. Comparing that list against the shipped rules found 113 that
DevSkim did not report: 108 absent outright and 5 present only in the
wrong case. Since the catch-all DS154189 uses RegexWord, which is
case-sensitive, StrCpy and StrCatBuff were not matched by the lowercase
lstrcpy and strcatbuff entries already in the alternation.
Extend DS154189 from 80 to 178 alternatives, covering the Windows string
helpers (StrCpy*, StrCat*, StrNCpy*, StrNCat*, lstr* variants), the _mbs*
and _t* variants, sprintfA/W and wsprintfA/W, the makepath and splitpath
families, the itoa family, CharToOem and OemToChar, and the remaining
scanf variants. The alternation is now sorted case-insensitively so
future additions land in a predictable place.
Four groups are given their own IDs rather than being folded into the
catch-all, because each has a distinct rationale and a different useful
severity, and because a separate ID is what lets a team suppress one
class without losing the rest:
- DS154190, the IsBad*Ptr family. Reported at important because these
cannot work as documented: probing a stack guard page disarms it, and a
wrong-but-mapped pointer is reported as valid. Includes the A/W exports,
which is what real code calls.
- DS154191, CopyMemory and RtlCopyMemory. Restricted to the Windows
aliases because bare memcpy is already reported by DS121708, which also
carries a fix-it; adding it again would double-report.
- DS154192, the unbounded length family. strlen was missing entirely.
wcslen and _tcslen move here from DS154189 so that the same defect is
not reported at two severities depending on which variant is used.
BestPractice, since strlen on a terminated buffer is fine.
- DS154193, class_addMethod and class_replaceMethod. Objective-C only,
ManualReview, because swizzling is a design decision rather than a
defect.
Each new rule has guidance and positive and negative self-tests. No
overrides are declared: none of these names remain in DS154189, so an
override would suppress nothing while adding a relationship that has to
be reasoned about later.
Verified by scanning a fixture containing all 113 previously-missed names.
All 113 are now reported, no line draws more than one rule, and a fixture
of the safe replacements (strcpy_s, memcpy_s, strnlen_s, snprintf_s)
produces no findings.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 4fc65bf3-6ed4-40fe-b21f-848c191b8bc8
The five shipped deserialization rules cover pickle, Java readObject, PHP
unserialize, Ruby Marshal, and Newtonsoft TypeNameHandling. ADM.10010
names several more as unapproved or unsafe, none of which were detected.
Add rules for the ones the requirement calls out by name:
- DS425050: torch.load, joblib.load, dill.load and the marshal module.
ADM.10010 lists these specifically for AI model files obtained from
GitHub or Hugging Face. They all run code during load, which makes a
downloaded checkpoint executable content rather than data.
- DS425060: yaml.load and yaml.load_all without a safe loader. The
requirement states PyYAML load cannot be used safely and that safe_load
must be used instead. A negated same-line condition excuses an explicit
Loader=SafeLoader, and the regex does not match yaml.safe_load, so both
safe forms stay quiet.
- DS425070: BinaryFormatter, SoapFormatter, NetDataContractSerializer,
LosFormatter and ObjectStateFormatter. Critical, because one reachable
call with attacker-controlled input is generally enough for code
execution. ADM.10010 approves only System.Text.Json, XmlSerializer,
DataContractSerializer, DataContractJsonSerializer, Newtonsoft.Json and
protobuf, and notes that most built-in .NET serializers not on that list
cannot be used safely.
- DS425080: JavaScriptSerializer and SimpleTypeResolver. The requirement
records that JavaScriptSerializer stopped being approved on 2023-03-10.
Both names are matched so the resolver is still caught when the
serializer is constructed elsewhere.
- DS425090: Boost Property Tree. ADM.10010 approves Boost but singles this
out for denial of service on nested payloads. ManualReview, since it is
reasonable for trusted local configuration.
Each has guidance and positive and negative self-tests. Verified against
a Python and a C# fixture: the unsafe forms report and the approved
alternatives (json.load, yaml.safe_load, an explicit SafeLoader,
JsonSerializer.Deserialize, DataContractSerializer) produce nothing.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 4fc65bf3-6ed4-40fe-b21f-848c191b8bc8
The three shipped external-entity rules only target Objective-C and
Swift, matching shouldResolveExternalEntities. Every other ecosystem was
uncovered, including .NET, whose safe configuration ADM.10010 spells out
in its own worked example.
- DS132782: DtdProcessing.Parse and the legacy ProhibitDtd = false. The
inverse of the DtdProcessing.Prohibit that ADM.10010's example sets.
Catches entity expansion as well as external entities, since expansion
needs only DTD processing.
- DS132783: XmlResolver assigned a resolver. The other half of the same
example, which sets XmlResolver to null. XmlSecureResolver is included
because its code-access-security restriction is not enforced on .NET
Core or later, so it looks like a mitigation without being one.
- DS132784: Java parser factories with no hardening anywhere in the file.
Covers DocumentBuilderFactory, SAXParserFactory, XMLInputFactory,
TransformerFactory and SchemaFactory, all of which resolve external
entities by default.
- DS132785: PHP libxml_disable_entity_loader(false) and LIBXML_NOENT. The
latter is included because its name suggests it suppresses entities when
it in fact substitutes them.
- DS132786: lxml parsers configured with resolve_entities, load_dtd or
no_network=False.
On DS132784, the negated conditions are worth being explicit about. Six
hardening indicators are each negated over same-file scope, which the
engine evaluates as "fire when none of them is present". That is the
sound direction for this question: it cannot claim a file is safe, only
that it shows no evidence of hardening. A file that hardens one factory
and leaves a second unhardened will not report. The rule is ManualReview
for that reason, and the guidance states the limitation rather than
leaving a reader to assume a clean result means every parser is safe.
Verified on Java, C#, and PHP fixtures: the unhardened forms report, and
adding disallow-doctype-decl, DtdProcessing.Prohibit with a null
XmlResolver, or LIBXML_NONET silences them.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 4fc65bf3-6ed4-40fe-b21f-848c191b8bc8
The engine supports ymlpaths, jsonpaths and xpaths, but no shipped rule
used ymlpaths or jsonpaths at all, so every YAML and JSON configuration
requirement in the SDL was undetectable in practice. This is not a
language gap: yaml, json and xml were already defined in languages.json.
The capability was simply unused.
Kubernetes Security Baseline (ADM.10210), eight rules, the first to use
ymlpaths. Each path is declared for the Pod, Deployment/StatefulSet/
DaemonSet/Job, and CronJob roots, and for both containers and
initContainers, since the same control appears at a different depth in
each workload kind.
DS200000 privileged: true critical
DS200001 allowPrivilegeEscalation: true important
DS200002 hostPID / hostIPC / hostNetwork important
DS200003 readOnlyRootFilesystem: false moderate
DS200004 runAsNonRoot: false important
DS200005 image unpinned moderate
DS200006 SYS_ADMIN / SYS_PTRACE / ALL added important
DS200007 hostPath volume ManualReview
Each carries a same-file condition requiring apiVersion, so the rules do
not fire on unrelated YAML that happens to share a key name.
DS200005 reports only an image with no tag or an explicit :latest, not
every image lacking a digest. Flagging all tagged images would have
contradicted the rule's own description and produced a finding on
essentially every manifest.
Package sources (ADM.10205), two rules:
DS205000 nuget.config <packageSources> with no <clear /> element, via
xpaths and a negated same-file condition. Without <clear />
the sources here are added to those inherited from machine and
user level config, which is the precondition for dependency
confusion.
DS205001 --extra-index-url and PIP_EXTRA_INDEX_URL, which the
requirement names explicitly as prohibited.
DS114352 gives the orphaned "Encryption Marked Optional" guidance a rule
at last, using jsonpaths over $.ConnectionStrings.* to find Encrypt=False,
TrustServerCertificate=True, sslmode=prefer/allow/disable, MySQL
SslMode=Preferred/None and JDBC useSSL=false. The guidance file was a TODO
stub and has been written properly.
Both new rule files are registered in the csproj EmbeddedResource
allowlist. That list is hand-maintained rather than a wildcard, and
omitting a file there is exactly what left four rules unshipped before.
Verified that all 51 rule files are embedded, with no duplicate or
dangling entries.
Verified against Pod and Deployment fixtures, a hardened manifest that
produces no findings, tagged and digest-pinned images, and nuget.config
with and without <clear />. This repository's own nuget.config is
correctly not flagged.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 4fc65bf3-6ed4-40fe-b21f-848c191b8bc8
Two separate gaps, both costing coverage on rules that already exist.
React was defined but untargeted. languages.json already mapped .jsx and
.tsx to javascriptreact and typescriptreact, but zero rules named those
languages, while 7 named javascript and 5 named typescript. Because
applies_to matches on the exact language name, React source received none
of those rules. Adding the two names to the 7 rules that target their
non-React counterparts restores coverage that was intended rather than
adding anything new. Confirmed by scanning a .tsx file, which now reports
DS189424 for eval and did not before.
HTML did not exist as a language, which made the engine's Html pattern
scope unreachable and left guidance/DS610000.md orphaned: the document
for a rule nobody could write. Adding html lets that rule exist, so
DS610000 now ships against guidance that was already in the repository.
Its stub has been expanded to explain reverse tabnabbing rather than only
naming the attribute.
Also added: dockerfile, terraform, bicep, kotlin, scala, dart, toml,
gradle, msbuild (.props/.targets), dotenv and makefile, with comment
syntax for each so comment-scoped patterns behave. 32 rules declare no
applies_to and therefore apply to every known language, so each new
language extends those 32 rules to a file type DevSkim previously skipped
entirely. Verified by scanning a Terraform file and a Dockerfile, both of
which now report hard-coded TLS versions.
The extensionless conventions needed the file-names property rather than
extensions, which no DevSkim language definition used before. A file
named Dockerfile has no extension, so .dockerfile alone matched nothing
in practice; the same applies to .env, Makefile, Cargo.toml and the shell
dotfiles. Verified that a plain Dockerfile is now scanned.
Widening applies_to expands scope silently, so this was checked for
regressions: the full suite passes and every existing rule's self-tests
still hold.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 4fc65bf3-6ed4-40fe-b21f-848c191b8bc8
AccessControl.10100 forbids secrets in code. DevSkim shipped two rules
for it, both keyed on 30 or more lowercase hex characters, so anything
base64 or provider-prefixed went undetected. That excludes most modern
credential formats.
Three rules added, chosen for precision rather than coverage. The
requirement directs teams to CredScan, SPMI and GitHub secret scanning,
and DevSkim should not pretend to replace them; its value here is
catching a credential in the editor before it is ever committed. So these
match only formats with issuer-assigned prefixes and fixed structure,
which ordinary code does not produce, rather than general entropy
heuristics that belong in a dedicated scanner.
- DS173238: PEM private key blocks. Excludes public keys and
certificates.
- DS173239: GitHub, AWS, Google, Slack, Stripe, npm, SendGrid and GitLab
token formats.
- DS173240: Azure Storage AccountKey connection strings and shared access
signatures.
Added a pem language for .pem, .key, .crt and the id_rsa family of
filenames. Without it DS173238 could not fire on the files private keys
actually live in, since those have no extension and DevSkim skips file
types it does not recognise.
Also fixes a regression introduced in the previous commit. DS114352 used
jsonpaths, and any rule using jsonpaths makes the engine parse every JSON
file it scans. VS Code's .vscode/launch.json, tasks.json and
extensions.json are JSONC, so they fail a strict parse, and the engine
logs that at error level. Scanning this repository produced four such
lines, and a .vscode directory is close to universal.
DS114352 is now a plain regex. The connection string tokens it matches
are distinctive enough that path scoping added little, and dropping it
means the rule also covers web.config, C# source, .env files and YAML
rather than appsettings.json alone. No rule uses jsonpaths now, so the
JSON parse path is never entered and the repository scans with zero parse
errors.
The eight ymlpaths rules are kept. The same error-level logging applies
to unparseable YAML, but that was checked against a Helm chart, whose
{{ .Values.x }} placeholders parse as scalars and produce no errors. The
underlying issue is that the engine logs a document that does not parse
at error rather than debug level; that belongs upstream in
ApplicationInspector and is noted with the other engine findings.
Verified: the new rules fire on a private key file, a GitHub token, an
AWS key and an Azure connection string, and stay silent on
process.env.GITHUB_TOKEN and DefaultAzureCredential. Scanning DevSkim
itself produces no findings outside the rules' own example strings in
secrets.json, which the pre-existing DS117838 already did.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 4fc65bf3-6ed4-40fe-b21f-848c191b8bc8
32 rules shipped with no must-match test, so nothing verified they could
match anything at all. Writing those tests found three rules and patterns
that never fired, in two cases because of a defect in the rule itself.
- DS191340 (sensitive data in NSUserDefaults) used $1 as a backreference.
.NET spells that \1; $ is an end-of-line anchor, so the pattern required
end-of-line followed by "1" and could never match any input. Corrected
to \1, and the rule now reports a password written to NSUserDefaults.
- DS440016's --secure-protocol= pattern was typed as "string", which
applies word boundaries. A leading hyphen is not a word character, so
the boundary could never be satisfied and wget --secure-protocol=SSLv3
went unreported. Changed to "substring".
- DS440060 (Node hard-coded TLS) had an empty patterns array and an
explicit comment saying it was encompassed by DS440000 and DS440010. A
rule with no patterns cannot produce a finding, but still occupies an
entry in SARIF tool metadata, so it is removed rather than left as a
rule that appears to exist.
Writing the negative tests was also informative. Several TLS rules report
TLS 1.3 as readily as TLS 1.0, because they match any hard-coded version
rather than any weak one. That is consistent with their stated intent,
"do not hard-code TLS protocol versions", so the rules are left alone and
the must-not-match samples use code that pins no version at all, which is
the behaviour actually being recommended.
Every rule now has a positive self-test. must-not-match is left where it
would only restate the positive case; it is most valuable where a rule
has a guard condition, and those all have one.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 4fc65bf3-6ed4-40fe-b21f-848c191b8bc8
#654
ApplicationInspector PR #654 unifies override suppression on containment.
DevSkim uses the sync path, which in 1.9.50 suppresses when the overridden
match merely *starts* inside the overriding one (RuleProcessor.cs:245),
while the async path already requires containment (line 443). Adopting the
PR therefore makes DevSkim's path stricter, and findings suppressed today
can reappear. 38 DevSkim rules use overrides; no rule shipped with
ApplicationInspector does, which is why the PR could not observe this.
Measuring every override pair found two that regress, both DS440016 over
DS440000, and both traceable to a defect in DS440016 rather than to the
engine change:
--(sslv2|sslv3|tlsv1|tlsv11|tlsv1\.1|tlsv1\.2)
.NET alternation is leftmost-first, not longest-match, so tlsv1 always
wins and the tlsv11, tlsv1\.1 and tlsv1\.2 branches are unreachable. On
curl --tlsv1.1 the rule reported --tlsv1 rather than --tlsv1.1, and the
truncated span is why DS440000's match of tlsv1.1 extended past it.
Reordering longest-first fixes the reported span and the containment
relationship together. Likewise --secure-protocol= matched only the flag
name, leaving DS440000's match of the value outside it; it now covers the
value.
Separately, DS140021 (strlen) overrode DS154189, but the banned API
commit moved strlen into DS154192, so the override became inert and
strlen(s) reported twice at the same severity. Repointed at DS154192.
DS140021 keeps precedence because it matches a complete call and carries
fix-its. Verified strlen(s) now reports once and wcslen(w) still reports
via DS154192.
Re-measured across all 38 override pairs after these fixes:
regressions 0 (was 2)
contained 18
inert 15 (the overridden rule never matches the same input)
The inert group is mostly the dedicated banned-API rules over DS154189
and the platform TLS rules over DS440000, where the general rule does not
match what the specific one matches, so the override does nothing under
either semantics.
No DevSkim override relationship now regresses under containment. This
was measured by comparing match spans under 1.9.50 rather than by
building against the PR branch, since nuget.org is unreachable here, so a
build-and-diff run is still worth doing before #654 merges.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 4fc65bf3-6ed4-40fe-b21f-848c191b8bc8
Heading is 1.0.108, which is the git height this commit produces, rather
than the 1.0.107 that HEAD produced before it.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 4fc65bf3-6ed4-40fe-b21f-848c191b8bc8
1.10.1 is the first release carrying the boolean expression support from
ApplicationInspector#654, along with four rules-engine bug fixes: string
patterns previously 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.
That last one matters here. 38 DevSkim rules use overrides and no rule
shipped with ApplicationInspector does, so DevSkim is the first consumer
to exercise it. The sync path DevSkim uses previously suppressed when the
overridden match merely started inside the overriding one and now requires
containment. The preceding commit already brought every DevSkim override
pair into containment, so this bump lands on a ruleset that is ready for
it.
Also makes DS440017's self-test robust. Its condition looks for "openssl"
in code scope, but the sample I wrote put it in a /* */ comment:
/* openssl configuration */
SSL_CTX_set_cipher_list(ctx, "AES256-SHA256");
That passed under 1.9.50 only because the verifier's fallback language has
no comment styling, so the comment was treated as code. Under 1.10.1 it is
correctly recognised as a comment and the condition no longer matches. The
sample now puts openssl in a string literal, which is code under any
language and does not depend on which fallback the verifier picks.
Validation. This environment cannot reach nuget.org and cannot
authenticate to the configured feed, so the published package could not be
restored. Instead ApplicationInspector was built from source at the v1.10.1
tag (b1bbf91, the #654 merge commit) together with OAT 1.2.95 built from
its own source, with no dependency downgraded, and its 88 expression tests
pass. The whole DevSkim ruleset was then run through that engine's
verifyrules: after the DS440017 fix there are no self-test failures. The
remaining messages are artifacts of running DevSkim rules through
ApplicationInspector's CLI, which does not know DevSkim's languages
(batch, dotenv, cobol, packages.config) and rejects the duplicate rule ids
DevSkim uses deliberately.
The ruleset now verifies clean under both 1.9.50 and 1.10.1, and the full
DevSkim suite passes at 1.9.50 with the DS440017 fix in place, so the
change is not carrying a rule regression into the upgrade.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 4fc65bf3-6ed4-40fe-b21f-848c191b8bc8
Heading is 1.0.110, the git height this commit produces.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 4fc65bf3-6ed4-40fe-b21f-848c191b8bc8
GitHub push protection flagged DS173239's must-match samples as a Slack
API token, which is the rule doing its job on its own test data. The
samples were realistic enough to look like live credentials.
Replace them with values that are unmistakably placeholders while still
exercising the same patterns: ghp_EXAMPLENOTAREALTOKEN... still satisfies
gh[pousr]_[A-Za-z0-9]{36,} and xoxb-EXAMPLE-NOT-A-REAL-TOKEN still
satisfies xox[abprs]-[0-9A-Za-z-]{10,}. The AWS sample is left alone as it
is the example key from AWS's own documentation.
Rule self-tests for credential detection should not themselves look like
credentials, since they end up in every clone and every scanner's corpus.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 4fc65bf3-6ed4-40fe-b21f-848c191b8bc8
…Inspector (#787)
* Add handoff notes for the boolean expression rule work
PR #780 deliberately left out the boolean expression rules. Application
Inspector 1.10.1 shipped the feature, but a rule supplying both
`expression` and `conditions` behaved correctly at scan time while the
rule verifier failed its must-not-match samples, and the verifier is what
DefaultRulesTests runs. Correct rules could not merge.
That is fixed in 1.10.2 by ApplicationInspector#656, which routes self
tests through the same capture reduction the analyzer uses.
This commit carries the work that was already done but had nowhere to
live, so the next person does not rebuild it:
- The merged DS440016, which collapses the two same-id entries in
tls_generic.json into one rule using
`(curlFlag AND NOT tls13) OR p0 OR ...`. This is the curl/wget case
from upstream's own test suite, and retires one of the duplicate ids.
Verified byte-identical in meaning to the version built earlier.
- The remaining soundness-gap rules to write, with the reason they need
the feature: the old shape can only express P AND NOT (C0 OR C1), "no
mitigation at all", where cookie flags and HSTS need
P AND NOT (C0 AND C1), "any required mitigation missing". The
partially hardened case is the one being missed.
- The constraints that are expensive to rediscover. Chief among them:
boolean operators over pattern labels alone are nearly useless, since a
finding comes from exactly one pattern, so `a AND NOT b` is identical
to `a` and `a XOR b` is identical to `a OR b`. All the real
expressiveness needs patterns and conditions in one expression.
Both commands quoted in the notes were run against this branch before
committing: the embedded-resource check reports 155 shipped rules with
none missing, and rule verification is silent.
The file is a working note and says so; the follow-up work should delete
it.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 4fc65bf3-6ed4-40fe-b21f-848c191b8bc8
* Update ApplicationInspector dependencies and add new security rules
- Bump Microsoft.CST.ApplicationInspector.Logging and Microsoft.CST.ApplicationInspector.RulesEngine to version 1.10.2 in project files.
- Introduce new rules for cookie security attributes (DS610001) and HSTS directives (DS610002) with corresponding guidance documentation.
- Implement a boolean expression rule (DS440016) to detect hard-coded SSL/TLS protocols in code.
- Remove obsolete handoff document for boolean expression rules.
- Update existing TLS rules to reflect new conditions and expressions.
- Add new JSON rule files for HTTP headers security checks.
---------
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 4fc65bf3-6ed4-40fe-b21f-848c191b8bc8
Razor supports // only inside a C# code block, while a suppression generated next to an HTML finding is top-level markup and can be rendered as text instead of acting as a comment. This mapping also omits Razor's universal @* ... *@ comments, so disabled markup can still produce findings. The Razor language needs valid suppression generation and recognition of Razor, HTML, and embedded C# comment forms.
gradlew is the POSIX shell wrapper, not Gradle source. Classifying it as gradle assigns // comments, so # comments are scanned as code and generated suppressions are invalid shell syntax. Move this file-name mapping to shellscript; .gradle files can remain under gradle.
Adding Kotlin as a recognized language does not make the newly shipped Android code rules run on Kotlin files: DS180001 and DS180002 still apply only to java, even though the WebView and StrictMode calls use the same syntax from Kotlin. Add kotlin to both rules and regression fixtures so common modern Android sources receive this coverage.
Although vb is listed in applies_to, this case-sensitive regex requires lowercase new. A conventional VB assignment such as reader.XmlResolver = New XmlUrlResolver() will not report. Make the expression case-insensitive and cover the VB form in the regression tests.
Detect the TrustedXslt shortcut described here
guidance/DS132781.md:24
The guidance says XsltSettings.TrustedXslt should be treated the same as enabling scripting, but DS132781 only matches EnableScript = true. A caller using the documented convenience value gets no finding. Add a TrustedXslt pattern and positive fixture, or explicitly document that this form is not detected.
Qualify the current lxml entity default
guidance/DS132786.md:16
This states that a no-argument XMLParser resolves external entities, but normal lxml XML parsers have defaulted to resolve_entities='internal' since lxml 5.0, and lxml 6.1 applies that safer default across all parsers. The explicit unsafe-option rule remains useful, but this guidance and the later no-argument warning should distinguish older releases from current behavior.
Do not present imagePullPolicy as signature verification
guidance/DS200005.md:46
The text promises admission-time signature verification, but the example only sets imagePullPolicy, which controls pulling and performs no signature or admission validation. This can lead readers to believe the image is verified. Remove the misleading snippet or replace it with a concrete admission-policy example.
Razor's native block comments use @* ... *@, but this mapping recognizes only HTML comments. Because the rules engine accepts only this configured prefix/suffix when deciding code scope, C# or markup inside a Razor comment is analyzed and can produce findings. Support Razor comments as well as HTML comments and add a regression fixture using @* ... *@.
SSL_stateless() processes a stateless handshake; it does not configure or hard-code the negotiated TLS version. This positive self-test locks in a false positive for DS440011. Remove that API from the rule's pattern and make this a negative fixture.
These new rules use the objective-c language, while comments.json registers its block-comment entry as objective-C. ApplicationInspector 1.10.2 lowercases the prefix lookup but compares it case-sensitively; the mismatch yields an empty prefix, after which code scope accepts matches inside comments. Lowercase that comments entry and add an Objective-C block-comment regression; this affects all four newly added Objective-C rules.
Kubernetes defaults allowPrivilegeEscalation to true when this field is omitted, and the Restricted standard requires it to be explicitly false. This ymlpath/value check reports only an explicit true, so the common insecure manifest with no field receives a clean result. The rule needs an absence check per container, with a regression fixture for an omitted security context.
SSL_stateless and SSL_OP_NO_COMPRESSION do not pin a TLS version; the latter is security hardening that disables compression. Making them positive fixtures codifies false positives from the broad existing patterns. Narrow the patterns to version-selection APIs/options and move these cases to negative tests.
The rule applies to VB, whose canonical object-construction keyword is New, but this case-sensitive expression requires lowercase new. Consequently a normal assignment such as settings.XmlResolver = New XmlUrlResolver() is not detected.
Do not claim this rule detects memcpy
guidance/DS154191.md:5
DS154191's pattern matches only CopyMemory and RtlCopyMemory; plain memcpy is handled by a different rule. The summary currently tells users that this finding detected memcpy, which cannot be true for this rule.
Replace the duplicate pull-policy example
guidance/DS200005.md:46
The text promises admission-time signature verification, but the example only repeats imagePullPolicy: IfNotPresent, which performs no signature verification. Remove this misleading block or replace it with an actual admission-policy example.
marshal.loads is unsafe for untrusted input and malformed data can crash Python, but deserialization itself does not execute the reconstructed code object. The description currently gives marshal the arbitrary-code-execution behavior of the pickle-based loaders. Revise both this text and its guidance so the reported risk is accurate.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
Several rules still produce concrete false positives or false negatives, including the NSUserDefaults, AWS identifier, Boost reader, and TLS command cases.
Get a fresh assessment by requesting another Copilot review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The DevSkim ruleset is intended to cover the Microsoft SDL. Auditing the shipped rules against the 172 current SDL requirement documents found real gaps, and more importantly found that several shipped rules could never have matched anything.
The most significant finding: four rules existed in the repository but had never run for any user.
Microsoft.DevSkim.csprojembeds rule files through a hand-maintained<EmbeddedResource>allowlist rather than a wildcard, andandroid.jsonandxslt_scripting.jsonwere missing from it. The shipped set was 123 rules, not the 127 on disk. This is also why their danglingrule_inforeferences never failed CI: the tests iterate the shipped set, so they never saw them.What changed
Shipped rules go from 123 to 156, and every rule now has a positive self-test (previously 32 had none).
Rules that could never match. Writing those missing self-tests found three more dead rules mechanically, none of which were visible by reading the ruleset:
DS191340$1as a backreference. .NET spells that\1, and$is an end-of-line anchor, so the pattern required end-of-line followed by1.DS440016--secure-protocol=typed asstring, which is word-boundary anchored. A leading hyphen is not a word character.DS440060patternsarray, so it could not produce a finding while still occupying a SARIF metadata entry.DS180000AndroidManifest.xmlhas none.DS132781applies_to: ["CSharp"]; language names match exactly againstlanguages.json, which definescsharp.DS180000is worth a second look during review: it had a self-test, and the self-test passed. The sample had been written to fit the broken XPath rather than to describe a real manifest, so it locked the defect in instead of catching it.Coverage.
RegexWorddoes not match.DS154189goes from 80 to 178 alternatives, with four groups split into their own IDs where the rationale and useful severity differ.ymlpathssupport, which no shipped rule had used. Not a language gap;yaml,jsonandxmlwere already defined.javascriptreactandtypescriptreactwere defined and mapped to.jsx/.tsxbut named by no rule, so React source received none of the JS/TS rules. Thirteen languages added; because 32 rules declare noapplies_to, each addition extends those 32 rules to a previously skipped file type. Extensionless files (Dockerfile,Makefile,id_rsa) needed thefile-namesproperty, which no language definition had used.ApplicationInspector 1.10.2. Picks up the boolean expression support from ApplicationInspector#654 plus five rules-engine fixes. One of those matters here specifically: override suppression on the sync path DevSkim uses now requires full containment rather than "starts inside". 38 DevSkim rules use
overridesand no rule shipped with ApplicationInspector does, so DevSkim is the first consumer to exercise it. Every override pair was measured and brought into containment before the bump, so the upgrade lands on a ruleset that is ready for it.Worth a careful look
DS132784(Java XXE) is deliberately unsound in the safe direction. Six hardening indicators are each negated oversame-filescope, so it fires only when none is present. A file that hardens one factory and leaves a second unhardened will not report. It isManualReviewfor that reason and the guidance states the limitation rather than letting a clean result imply every parser is safe.DS440011was two unrelated rules sharing an ID, which made SARIF serve one rule's findings under the other's name andhelpUri; that one is split.DS440016andDS148264are each one logical rule split across entries and are deliberately left alone, because the shared ID is what lets a single suppression cover the whole rule.DS114352uses a plain regex rather thanjsonpathson purpose. An earlier revision usedjsonpaths, which makes the engine parse every JSON file it scans;.vscode/*.jsonis JSONC and fails a strict parse, so a near-universal repository layout produced error-level log output. No rule usesjsonpathsnow.Verification
Full suite passes on net8.0, net9.0 and net10.0. Every new rule was checked by scanning fixtures rather than by self-tests alone, including negative fixtures of the recommended safe alternatives. Scanning DevSkim itself produces no findings outside the rules' own example strings.
Verification was run against the published
Microsoft.CST.ApplicationInspector.RulesEngineand...Logging1.10.2 packages, which are the versions this PR consumes. 1.10.2 adds the verifier fix for rules that combineexpressionandconditions, so the boolean-expression rules below pass the verifier that CI runs.Follow-up, not included here
TLS consolidation is also deferred, since much of that sprawl exists because the engine could not express per-pattern conditions, and consolidating before that lands would mean rewriting the same rules twice.