diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index f156c522..cfd79699 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -18,6 +18,7 @@ /domains/performance/ @MetaMask/extension-platform @MetaMask/mobile-platform /domains/perps/ @MetaMask/perps /domains/pr-workflow/ @MetaMask/extension-platform @MetaMask/mobile-platform +/domains/security/ @MetaMask/extension-platform @MetaMask/mobile-platform /domains/swaps/ @MetaMask/swaps-engineers /domains/testing/ @MetaMask/qa /domains/ui/ @MetaMask/design-system-engineers diff --git a/domains/security/skills/lavamoat-policy/scripts/policy-audit.py b/domains/security/skills/lavamoat-policy/scripts/policy-audit.py new file mode 100644 index 00000000..e87e45d7 --- /dev/null +++ b/domains/security/skills/lavamoat-policy/scripts/policy-audit.py @@ -0,0 +1,257 @@ +#!/usr/bin/env python3 +"""Turn a LavaMoat policy base/head pair into a per-grant justification worklist, +and audit hand-written overrides for scope. + +Detection is LavaMoat's job: `@metamaskbot update-policies` regenerates the policy from a +real run of the code and CI fails on drift. This script does NOT re-derive or classify that +diff — it enumerates every capability newly granted so each can be JUSTIFIED with a permalink +to the dependency's own source (accept), or REJECTED where no call site uses it. + +With --override it also audits `policy-override.json`. Per lavamoat-core/src/mergePolicy.js the +effective policy is `mergePolicy(generated, override)`, with priority to stricter decisions in +the override. The two files are DESIGNED not to align: the generated policy is regenerated on +dependency updates while the override persists, which is the whole point of separating them. +So an override entry absent from the generated policy is the normal case, not a finding. + +What the audit reports is therefore scope, not divergence: which entries persist without a +recorded reason, which are broader than the generated policy shows a need for, and which grant +write. Narrowing a whole-object grant requires setting the parent to `false` — `validateHierarchy` +throws if `X` and `X.y` are both present. + +Usage: + policy-audit.py + [--override ] [--generated ] + +Falsifier: a listed grant for which no upstream call site can be found. +""" +import json +import sys + +# Capability classes where a wrong call is not recoverable by a follow-up PR. The script +# refuses a verdict on these by design — it reports and escalates. Naming them explicitly +# rather than scoring them keeps the escalation auditable. +CRITICAL = { + "child_process", "fs", "vm", "worker_threads", "module", "process", + "eval", "Function", "WebAssembly", "importScripts", "SharedArrayBuffer", + "fetch", "XMLHttpRequest", "WebSocket", "crypto", "indexedDB", + "localStorage", "sessionStorage", "chrome", "browser", +} +CRITICAL_PKG_HINTS = ("keyring", "vault", "snap", "lavamoat", "seed", "wallet") + +# ECMAScript and DOM intrinsics. Granting these is unremarkable — a package that renders +# anything touches Element and Object — so they never escalate on their own. Without this +# a sensitive-package hint floods the list with `Object`, `String`, `Array`, and an +# escalation list that is mostly noise trains its reader to skip it. +BENIGN = { + "Array", "Object", "String", "Number", "Boolean", "Symbol", "BigInt", "Math", + "JSON", "Date", "RegExp", "Map", "Set", "WeakMap", "WeakSet", "Weakmap", + "Promise", "Error", "TypeError", "Proxy", "Reflect", "Intl", + "Document", "DocumentFragment", "Element", "Event", "EventTarget", "Node", + "NavigateEvent", "NavigationDestination", "Clipboard", "CSS", "Text", + "console", "queueMicrotask", "structuredClone", +} + + +def resources(path): + with open(path) as f: + return json.load(f).get("resources", {}) + + +def newly_granted(head, base): + """Every (pkg, kind, capability) that is true in head and absent/false in base.""" + out = [] + for pkg, cfg in head.items(): + for kind in ("globals", "builtins", "packages"): + for cap, val in (cfg.get(kind) or {}).items(): + if val and not ((base.get(pkg, {}).get(kind) or {}).get(cap)): + out.append((pkg, kind, cap)) + return out + + +def root_of(cap): + return cap.split(".", 1)[0] + + +def criticality(pkg, cap): + """Return a reason string, or None. The reason is reported verbatim, so it must be + true of THIS row — a package-level hint is not a claim about the capability.""" + root = root_of(cap) + if root in CRITICAL: + return f"critical capability: {root}" + if root in BENIGN: + return None + if any(h in pkg.lower() for h in CRITICAL_PKG_HINTS): + return "non-intrinsic grant in a security-sensitive package" + return None + + +def is_critical(pkg, cap): + return criticality(pkg, cap) is not None + + +def audit_overrides(over, gen): + """Classify each override entry against what the generated policy observed. + + widened granted here, not observed by the toolchain — a human decision needing a reason + tightened explicit false over an observed grant — containment narrowed, no action + broad whole-object grant where only specific members were observed — narrowable + write write access; read may suffice, and only call sites can settle it + """ + widened, tightened, broad, write = [], [], [], [] + for pkg, cfg in over.items(): + gpkg = gen.get(pkg, {}) + for kind, caps in cfg.items(): + if not isinstance(caps, dict): + continue + gcaps = gpkg.get(kind) or {} + for cap, val in caps.items(): + if val == "write": + write.append((pkg, kind, cap)) + continue + if val is False: + if gcaps.get(cap): + tightened.append((pkg, kind, cap)) + continue + if not gcaps.get(cap): + # NOT "never observed" — the generated policy is regenerated + # independently, so absence here is expected. This is only a list of + # entries whose justification lives outside both files. + widened.append((pkg, kind, cap)) + if "." not in cap: + members = sorted( + c for c in gcaps if c.startswith(cap + ".") and gcaps.get(c) + ) + if members and not gcaps.get(cap): + broad.append((pkg, kind, cap, tuple(members))) + return widened, tightened, broad, write + + +def partition_escalations(rows, base, write_set): + """Split a human's worklist into decisions and consequences. + + A capability granted to a package the base policy did not contain at all arrives + because the package arrived — a bundle-graph change, not a choice anyone made about + that capability. A capability newly granted to a package already contained is + somebody's decision. Under one heading the second is buried in the first: a list + where 23 of 24 rows are not actionable teaches its reader to skim past the one that is. + """ + chosen, inherited = [], [] + for pkg, kind, cap in rows: + (inherited if pkg not in base else chosen).append((pkg, kind, cap)) + return chosen, inherited + + +def main(): + args = sys.argv[1:] + if len(args) < 2: + sys.exit("usage: policy-audit.py " + "[--override ] [--generated ]") + base_p, head_p = args[0], args[1] + over_p = gen_p = None + for i, a in enumerate(args): + if a == "--override" and i + 1 < len(args): + over_p = args[i + 1] + if a == "--generated" and i + 1 < len(args): + gen_p = args[i + 1] + + base, head = resources(base_p), resources(head_p) + grants = sorted(newly_granted(head, base)) + + print("PER-GRANT JUSTIFICATION WORKLIST") + print("=" * 74) + print("Detection is LavaMoat's; each row below needs a REASON, not a category.") + print("Justify with a permalink to the dependency's source at the installed version") + print("(accept), or reject where no call site uses the capability.\n") + + if not grants: + print(" (no new grants between base and head — nothing to justify)") + else: + for pkg, kind, cap in grants: + mark = "!" if is_critical(pkg, cap) else " " + print(f" {mark}[ ] {pkg[:38]:40s} {kind[:3]}:{cap:22s}" + " reason: verdict: accept|REJECT") + crit = [g for g in grants if is_critical(g[0], g[2])] + print(f"\n {len(grants)} grant(s) to justify" + + (f"; {len(crit)} marked ! for escalation." if crit else ".")) + print(" A grant with no locatable call site is the finding — reject it.") + + if not over_p: + return + gen = resources(gen_p) if gen_p else head + over = resources(over_p) + widened, tightened, broad, write = audit_overrides(over, gen) + + print("\n\nOVERRIDE SCOPE AUDIT") + print("=" * 74) + print("Effective policy = mergePolicy(generated, override), stricter decisions winning.") + print("The files are meant to differ: the generated one is regenerated on dependency") + print("updates while the override persists. Entries below are scoped, not \"unobserved\".") + print(f"{len(over)} package(s) overridden.\n") + print(f" tightened {len(tightened):3d} explicit false over an observed grant — containment narrowed") + print(f" persisting {len(widened):3d} in the override only — expected, but each needs a standing reason") + print(f" broad {len(broad):3d} whole-object grant where only members were observed") + print(f" write {len(write):3d} write access — read may suffice") + + if broad: + print("\n\nSUGGESTED TIGHTENINGS — evidence-based, functionality-preserving") + print("-" * 74) + print("A whole-object grant where the generated policy shows only members in use.") + print("`validateHierarchy` REJECTS a policy containing both `X` and `X.y`, so narrowing") + print("requires denying the parent explicitly — that is LavaMoat's documented form:") + print(" \"You could set the parent to false if you intended a less permissive policy.\"") + print("Regenerate and re-run the app after applying; an over-narrowed grant fails loudly.\n") + for pkg, kind, cap, members in sorted(broad)[:20]: + print(f" {pkg}") + narrowed = {cap: False} + narrowed.update({m: True for m in members}) + print(f" now: \"{cap}\": true") + print(f" → " + json.dumps(narrowed)[1:-1]) + if len(broad) > 20: + print(f"\n … {len(broad) - 20} further narrowable grant(s).") + + escalate = sorted(set( + [(p, k, c) for p, k, c in widened if is_critical(p, c)] + + [(p, k, c) for p, k, c in write] + )) + if escalate: + write_set = set(write) + chosen, inherited = partition_escalations(escalate, base, write_set) + print("\n\nRAISE WITH A HUMAN — no verdict offered") + print("-" * 74) + print("These widen a capability class where a wrong call is not recoverable by a") + print("follow-up PR, or grant write where read may suffice. Whether each is correct") + print("depends on intent and threat model, neither of which is in the policy files.") + print("This script stops here deliberately rather than guessing.\n") + def row(pkg, kind, cap): + why = "write access" if (pkg, kind, cap) in write_set else criticality(pkg, cap) + print(f" [?] {pkg[:42]:44s} {kind[:3]}:{cap:20s} {why}") + + print(f"\nCHOSEN HERE — {len(chosen)} row(s), on a package the base already contained") + if chosen: + for pkg, kind, cap in chosen: + row(pkg, kind, cap) + else: + print(" (none — every escalation below arrived with a new package)") + + if inherited: + print(f"\nARRIVES WITH A NEWLY-CONTAINED PACKAGE — {len(inherited)} row(s)") + print(" The package is new to this policy, so the grant follows from containing") + print(" it. The question is whether the package belongs in this bundle, not") + print(" whether the capability was correctly chosen.") + # Write access is never truncated. It is the smallest and highest-signal + # category, and a cap that hides it turns the section into a list whose + # most important row is the one the reader cannot see. + w = [r for r in inherited if r in write_set] + rest = [r for r in inherited if r not in write_set] + for pkg, kind, cap in w: + row(pkg, kind, cap) + for pkg, kind, cap in rest[:10]: + row(pkg, kind, cap) + if len(rest) > 10: + print(f" … {len(rest) - 10} further read-only row(s) of the same kind.") + print(f"\n {len(escalate)} decision(s) for a human. An audit that silently resolves") + print(" these has substituted a guess for the thing it was asked to check.") + + +if __name__ == "__main__": + main() diff --git a/domains/security/skills/lavamoat-policy/skill.md b/domains/security/skills/lavamoat-policy/skill.md new file mode 100644 index 00000000..851a1d53 --- /dev/null +++ b/domains/security/skills/lavamoat-policy/skill.md @@ -0,0 +1,326 @@ +--- +name: lavamoat-policy +description: Triage a LavaMoat policy change for least privilege — which newly granted capabilities can be dropped without breaking anything. Detection is delegated to `@metamaskbot update-policies` plus CI, and because the policy is generated by static analysis, every grant has a call site by construction, so reporting that one exists is a tautology and not the deliverable. `policy.json` records `"crypto": true` and nothing about what the code does with it or how much of the module it touches — recovering that discarded half is the job. Read each grant's use at the installed version to find its gate (a config flag nobody sets, an API nobody calls, a branch our payloads never take, an error-only path) and its breadth (a bare global standing in for one property is a finding even when reached), with the removal or narrowing test proposed for the policy owners to run. Lead with removal candidates and anything the reading turned up that bears on security; never render an accept/reject verdict, that call is the reviewer's. Hand it over untagged while the workflow is in trial. Triggers on /mms-lavamoat-policy, or when asked about a LavaMoat policy grant, policy.json diff, capability containment, scuttling, allowScripts, or why a package needs a global/builtin. The specialized engine behind `supply-chain-audit`'s capability-containment lane. +maturity: experimental +--- + +# /lavamoat-policy + +Detection is not the job. LavaMoat already tells you which capabilities a dependency change +grants: `@metamaskbot update-policies` regenerates the `policy.json` files, and CI's +`validate-lavamoat-policies` fails the build if the committed policy drifts from that +regeneration. Re-deriving the diff by hand, or sorting the grants into network/DOM/red-flag +buckets, only re-does a machine that is already trusted. + +**Reporting that a call site exists is not the job — that search cannot fail.** Policy is +generated by **static analysis**, not by observing a run: `lavamoat-tofu` parses each module with +`@babel/parser` and records global and builtin references (`inspectGlobals` / `inspectImports`, +[generatePolicy.js](https://github.com/LavaMoat/LavaMoat/blob/main/packages/core/src/generatePolicy.js)). +A grant means the identifier is textually present in a parsed module, so searching for it must +succeed. "Each addition has a call site, therefore each is justified" reports 11 of 11 every +time, and a check that cannot come back negative carries no information. + +**Finding the call site is the entire job — its existence is just the part not worth printing.** +`policy.json` records `"crypto": true` and stops. It does not record what the code does with the +capability, or how much of the module it touches, and that discarded half is what a reviewer +needs. Recovering it is what this skill is for. Static detection is also why removal candidates +are common: an identifier surviving into the bundle says nothing about whether our usage reaches +it — which a runtime trace would have. + +**The job is least privilege: which of these grants can be dropped without breaking +anything?** A grant exists because an identifier appears in bundled source. That is *not* the +same as a reachable path needing it — a capability read behind a config flag nobody sets, or in +a branch our usage never takes, is removable. So for each grant, ask what executes it under +*our* usage, and sort: + +| | | +|---|---| +| **removable** | nothing on our path executes the read → candidate; propose the test | +| **removable at a cost** | only a convenience or error-detail path executes it → name the cost | +| **load-bearing** | our usage genuinely needs it → say so briefly and move on | + +The lead is the first two rows plus anything the reading turned up that bears on security. +Load-bearing grants still each get a row in the capability → call-site table (step 6) — they just +don't get paragraphs. + +> **Falsifier.** A grant you called load-bearing that a build with it removed still passes. +> The test is cheap and it is the only thing that settles the question: drop the grant from the +> resource, rebuild, run the relevant e2e. +> +> **Run it yourself whenever the steps are clear enough to run.** A removal candidate handed +> over untested asks the reviewer to do the work that would settle it, and most of them will not +> — so the finding sits. A build that passes with the capability removed converts `🔍 candidate` +> into a demonstrated non-breaking reduction, and that is a different object: it can be merged +> rather than considered. Publish the run as the evidence, not the conclusion drawn from it. +> +> Hand the test over only when you genuinely cannot run it — a variant needing credentials you +> do not have, an e2e suite the environment cannot host, a policy whose regeneration needs CI's +> toolchain. Say which of those it is; "propose the test" as a default is the failure mode this +> replaces. +> +> **Exercise the capability, not the app.** A build that compiles, or an app that boots, with the +> grant removed shows only that nothing on the startup path needed it. Most grants are not on the +> startup path — that is usually *why* they look removable — so "it still builds" is the null you +> should expect either way, and it is not evidence. Name the scenario that would actually execute +> the read: the error path that formats a `span.url`, the source-map write, the importer resolving +> a relative `@use`, the config flag that turns the feature on. Then run that. +> +> **And prove the arms differ before believing either.** Two things, both cheap. That the +> effective policy really changed — merge the override into the base and print the resource, +> rather than assuming an edit took. And that a fully-denied arm actually *fails*. If denying +> everything still passes, the grant is not enforced on that path, and no arrangement of arms on +> that path can tell you anything. Report that: "this suite does not arbitrate this grant" is a +> real finding, and more useful than a green you cannot cash. + +**Corollary — the reading is where the real findings come from.** Locating each call site means +reading the code that uses the capability, and that is when genuine issues surface: unbounded work +on remote input, a decode path with no size cap, a feature-detection fallback that makes a grant +droppable. Those observations are worth more than the grant inventory. Lead with them. + +## Method + +1. **Take the diff from LavaMoat; don't re-derive it.** The bot's `update-policies` run + produces the authoritative delta and CI enforces it. Your input is the list of + newly-`true` grants per package, not a hand-rolled scan. (`scripts/policy-audit.py` turns a + base/head policy pair into that list as a worklist — it enumerates, it does not classify.) + + **When no current CI policy exists, regenerate locally — that is the fallback, not a + competing method.** The bot hasn't run, the branch is unpushed, or a variant CI didn't + cover: `yarn webpack:lavamoat:policy:build` (`:mv2` / `:mv3`) produces the same base/head + pair to feed step 2. Note what changes and what doesn't: the *worklist* is equally valid, + but its **provenance is weaker** — it reflects your node version, OS, and lockfile + resolution rather than CI's. Say which source the diff came from when handing the + justification over, and re-check against the bot's policy once it runs. Never regenerate + locally *in preference to* an available CI policy; that re-does a trusted machine and + substitutes a less reproducible artifact for a more reproducible one. + +2. **Know what denial actually does before you reason about it.** An ungranted global is **absent + from the package's endowments and reads as `undefined`** — it does not throw. + `getEndowmentsForConfig` collects `whitelistedReads`, `makeMinimalViewOfRef` builds an object + holding only those, and a `false` value simply keeps the path out of that list + ([endowmentsToolkit.js](https://github.com/LavaMoat/LavaMoat/blob/f5e52ab457c16c3aea72cc8a9dd0833547dd7d2c/packages/core/src/endowmentsToolkit.js#L101-L162)). + This is the whole basis of the analysis, so get it right: **do not confuse per-package + `globals` policy with scuttling**, which is a separate root-realm mechanism. Asserting the + wrong one to the LavaMoat maintainer got the reply "This is jibberish" (#45024, 2026-07-30). + + The consequence is what makes denial testable: a package that reads a global behind a + `globalThis.X || ` guard **keeps working when denied**, because the read yields + `undefined` and the fallback engages. A feature-detection shim is therefore evidence *for* + removability, not against it. + +3. **Read each grant's call site — then ask what executes it under our usage.** Read the + dependency's code *at the version being installed*. Locating the use is the start, not the + answer; the question the reading has to settle is whether anything on our path runs it. Look + for the gate: a config flag (`BigNumber.set({CRYPTO:true})`), an API we never call + (`.random()`), a feature-detection fallback, a branch keyed on a payload type we never send + (`data instanceof Blob` — note this one *does* break when denied, since `instanceof undefined` + throws), an error-only path. A gated read whose gate we never open is a removal candidate. + + Check reachability from *our* side too, not just the dependency's: does our code subscribe to + the feed, import the subpath, take that option? A capability behind a feature we don't use is + the cleanest removal there is — and a capability behind one we *do* use is load-bearing, which + is worth one line and no more. + +4. **Record what each grant reaches versus what it uses — breadth is the usual finding.** + Reachability and width are independent. "Nothing calls it" finds removable grants and says + nothing about a grant that *is* called and hands over far more than the call needs. LavaMoat + grants whatever path you name, and + [dotted sub-paths are supported](https://github.com/LavaMoat/LavaMoat/blob/f5e52ab457c16c3aea72cc8a9dd0833547dd7d2c/packages/core/src/endowmentsToolkit.js#L101-L162) + for globals and builtins alike — the extension's own override already uses + `document.visibilityState`. So put each grant's used surface next to its call site, and when a + grant is wider than its use, name the narrowest path that covers every call: `crypto.getRandomValues` + not `crypto`, `node:url.fileURLToPath` not `node:url`, `document.visibilityState` not `document`. + A bare global whose only use is one property is a finding even though it is reached. + + Name the exploit-relevant grants present — code loading (`eval`, `Function`, `importScripts`, + `WebAssembly`, `Worker`, `Blob`+`createObjectURL`), network (`fetch`, `XMLHttpRequest`, + `WebSocket`, `sendBeacon`, `postMessage`), crypto/storage (bare `crypto`, `indexedDB`, + `localStorage`, `chrome.storage`), UI/navigation (`clipboard`, `window.open`, `location`, + `document`, `chrome.tabs`), Node builtins (`child_process`, `fs`, `net`, `http`, `vm`) — and + **hand them over without assessing them**. Do not explain what an attacker could do, do not + rank them. That is threat-model work owned by someone else. + + Do not inflate benign grants to look like findings. `clearTimeout` takes a timer id and cancels + it; it confers no reach. (On extension#45024 the `crypto` grant was caught only because it was + *unreachable* — had `random()` been called it would have been filed as needed and its `subtle` + breadth never mentioned. That is the gap this step closes.) + +5. **Cite it at a pinned tag, not a branch head.** A permalink to `…/blob//#Ln` is + immutable; a branch-head link drifts out from under the citation. The permalink *is* the + evidence — a reader clicks it and lands on the code, convinced without re-running anything. + "It needs X" retyped into a table proves nothing about provenance. + +6. **Lead with removal candidates and anything security-relevant — then give the full table.** + Open on what can be dropped and what the reading turned up, not on an inventory. But every + grant still gets its own row in the capability → call-site table, load-bearing ones included: + that mapping is what a reviewer came for, and a load-bearing row is one short row, not a + reason to merge it into prose with its neighbours. **Removed grants get their names only** + (`WebSocket` and `CustomEvent` are removed by this bump) — no table, no justification column, + since a removal reduces capability and needs no defence. The exception is a removal that is + itself interesting: one that was load-bearing implies a behaviour change worth a sentence. + + Target a few hundred words of *prose*; the table does not count against that and must not be + compressed to hit it. (Violated on extension#45024, 2026-07-30 — a trim pass dissolved the + table into paragraphs and destroyed the comment's key content.) + + **The accept/reject call belongs to the human reviewer; never write it.** No `accept` + column, no `REJECT`, no "Verdict: safe to take", no ✅/❌. Those words do the reviewer's + deciding for them and anchor the judgment before they have read the evidence — and if the + call is wrong, it is wrong in a document that looks authoritative. Describing *risk* is in + scope where it is a fact about the capability ("this reads the global on every exception + path", "these are decode and timer primitives, no filesystem or subprocess reach"); the + disposition is not. State findings and open questions, and let the reviewer conclude. + (Violated on extension#45024, 2026-07-30 — 11 `accept` cells and a "Verdict" section.) + + **A grant with no locatable call site is a real finding, and rare.** On a generated policy it + usually means the identifier is present but the generator saw it in a path you haven't found + — say what you searched rather than implying nothing uses it. + +7. **Put it where the policy is reviewed — but do not tag anyone.** Post the justification as a + comment on the PR carrying the `policy.json` change, so it lands in front of the people who + own the policy rather than standing as a unilateral assertion elsewhere. + + **Show the full comment body in the response before running `gh pr comment`.** The permission + prompt renders the command, not the `--body-file` contents, so approving it blind is approving + unseen text published under the user's name. Paste the table and prose inline first, then post. + Naming the scratchpad path instead of showing the text is the same failure. + + **Do not add `cc @MetaMask/policy-reviewers` — or any `@`-mention — unless the user asks for + it in this session.** Authorization to post a comment is not authorization to notify a team, + and this step being written into the skill does not supply that authorization; editing the + comment afterwards does not un-send the ping. Draft without the tag, post, then offer the cc + line as a separate ready-to-paste suggestion. + + **Status as of 2026-07-30: hold the cc — this workflow is in a trial phase.** The tag is + expected to become standard once the output has proven itself; it is being withheld for now, + not forbidden on principle. So the rule is about *timing being the user's call*, not about + tagging being wrong. Re-confirm before assuming trial phase still applies — and even after it + ends, the tag goes in because the user says so, not because this line stops saying "hold". + (Violated on extension#45024, 2026-07-30.) + +8. **One reason covers the variants.** Extension builds carry several policy files + (`lavamoat/webpack/{mv2,mv3}/{beta,experimental,flask,main}/policy.json`). When the grant + delta is identical across them, a single justification covers all — confirm the identity + once. A grant that appears in one variant and not others is itself a question. + +## Output + +**The capability → call-site table is the deliverable. Never dissolve it into prose.** +One row per grant, every grant, with its permalink, its used surface and its breadth in the row. +A reviewer scans the column, not paragraphs — 11 rows is denser and faster to read than three +paragraphs carrying the same 11 facts, so the table *is* the trimmed form. Prose around it is +what gets cut. + +**Present findings; do not explain the mechanism.** Reviewers here know what a LavaMoat policy is, +what the bot does and what CI enforces. Restating it spends their attention on what they already +know. No paragraph on why call-site search is tautological, no methodology section — open on what +was found. + +``` + +> + +LavaMoat grants — -> + + + +| capability | surface used | what it does | breadth | +|---|---|---|---| +| | | | wider than its use — | +| | | | | +| | not referenced at any spelling | — | 🔍 candidate — | +…every grant gets a row… + + + +Test: . +Loose ends: +Removed: , . ← names only, no table, no justification column + +``` + +The marker pair is not decoration: a re-run replaces the region between them instead of appending +a second comment. It is deliberately *not* `VALIDATION_RUN_*` — this is a diligence artifact with +no verdict, and sharing that region would let an evidence re-run silently eat it. + +Order is the point: the lead names the findings, the table carries them, the diff makes them +actionable. Post it on the PR carrying the `policy.json` change, untagged. + +**End with a diff a reviewer can apply, not a description of one.** Every narrowing or removal in +the table gets a fenced `diff` block against the matching +`lavamoat/webpack//policy-override.json`, because that is the file a human edits — the +generated `policy.json` is regenerated and would lose the change. Prose like "could be narrowed to +`node:url.fileURLToPath`" makes the reviewer translate; a diff makes it a decision. + +```diff + "resources": { ++ "sass-loader": { ++ "builtin": { ++ "node:url.fileURLToPath": true, ++ "node:url.pathToFileURL": true, ++ "node:url": false ++ } ++ }, +``` + +Dotted paths already work in these files — `copy-webpack-plugin>serialize-javascript` is granted +`crypto.getRandomValues`, not bare `crypto`. Cite that precedent rather than asserting support. + +**Runtime claims need a runtime artifact.** A permalink witnesses what a line says; it does not +witness what *you ran*. "The tarball's complete specifier set is X", "byte-identical across all 8 +policy files", any grep or `npm pack` result — those are claims about a local run, and the reader +cannot check them. Publish the output and link it, or state the claim as the search it was +("searched N files, found no match") rather than as a property of the package. Bare integers in +prose need the same treatment: a reader who cannot trace "14 call sites" to something shown is +being asked to take it on trust. + +## Worked example — extension#42867 (@sentry/browser 8.33.1 → 10.38.0) + +The bump added grants across the `@sentry/*` subtree; the bot produced the diff and CI +enforced it — detection was never in question. On `mv2/main/policy.json` a reviewer questioned +two grants: *"I wonder what it's using this for. Likewise for `importScripts`."* Each was +answered with the upstream line, pinned to `10.38.0`: + +- **`WebAssembly`** → the event builder's `isWebAssemblyException` check, which runs on *every* + exception (defined L165, called on the exception path L186 and L203): + `https://github.com/getsentry/sentry-javascript/blob/10.38.0/packages/browser/src/eventbuilder.ts#L163-L168` +- **`importScripts`** → the profiling utils' main-thread detection at module scope + (`typeof importScripts === 'undefined'`): + `https://github.com/getsentry/sentry-javascript/blob/10.38.0/packages/browser/src/profiling/utils.ts#L33-L34` + +Both run unconditionally — the exception path and module scope — and under scuttling the read +itself throws unless excepted, so both are load-bearing with no gate to close. That is the +useful conclusion: *not* "each grant has a reason" (it always will) but "neither is removable, +and here is the unconditional path that makes it so." + +**Counter-example from extension#45024, which the first pass got wrong.** That comment reported +"11 additions, 11 reasons, each resolving to a line" as its headline. Tautological — the policy +is generated from a run, so the count was guaranteed. Reading for *gates* instead surfaced the +actual findings: `crypto` on `bignumber.js` is reachable only via `BigNumber.set({CRYPTO:true})` +or `.random()`, neither of which the consumer calls, so it is a removal candidate; and the +`DecompressionStream` grant sits on a `fastAssetCtxs` decode path that inflates remote input +with no size cap. Same reading, same permalinks — the first framing hid both. + +## Scope — what this skill is NOT + +This covers **capability containment only**: what a dependency is *permitted to reach* under +LavaMoat, and whether each new permission has a reason. It says nothing about whether the +dependency is *known-vulnerable* or *behaving maliciously* — those are different questions with +different detectors and different falsifiers, and they live in `supply-chain-audit`: + +| question | detector | skill | +|---|---|---| +| does this dep now reach a capability it didn't? | LavaMoat policy diff | **this skill** | +| is this dep version known-vulnerable? | `yarn npm audit`, advisories | `supply-chain-audit` | +| is this package behaving maliciously / newly-authored / install-scripted? | Socket Security | `supply-chain-audit` | + +A clean policy diff does not mean a safe dependency, and a known CVE does not show up as a new +grant. Run the umbrella skill when the question is "is this bump safe"; run this one when the +question is "why does it need that". + +## Called by supply-chain-audit and evidence + +`supply-chain-audit` delegates its capability-containment lane here. `evidence` keeps +**supply-chain** as an evidence category and packages the per-grant justification (accept / +reject, each with its permalink) posted where the policy is reviewed. Engine helper: +`scripts/policy-audit.py`. Usable standalone whenever a policy grant needs a reason. diff --git a/domains/security/skills/supply-chain-audit/skill.md b/domains/security/skills/supply-chain-audit/skill.md new file mode 100644 index 00000000..1401095b --- /dev/null +++ b/domains/security/skills/supply-chain-audit/skill.md @@ -0,0 +1,158 @@ +--- +name: supply-chain-audit +description: Assess whether a dependency change is safe to take, across every detector that answers a different part of that question — Socket Security (malicious/anomalous package behavior, install scripts, new maintainers), `yarn npm audit` and advisories (known vulnerabilities), lockfile and manifest diffs (what actually changed, including transitive and resolution swaps), and LavaMoat policy grants (new capabilities, delegated to `lavamoat-policy`). Also covers the fronts no upstream scanner sees because they are things your own repo does to dependencies afterwards: yarn patches that modify dependency source at install, `resolutions` that force or stub versions, `npmAuditIgnoreAdvisories` suppression lists, CI actions riding mutable tags instead of pinned SHAs, and yarn plugins that execute at install. The falsifier is a lane whose finding is unaccounted for — a flagged package, an unresolved advisory, or a grant with no call site. Detection belongs to the tools; the job is disposition, and handing it to the humans who own the dependency. Triggers on /mms-supply-chain-audit, or when asked whether a dependency bump is safe, to review a lockfile or package.json change, to triage a Socket or audit finding, or to assess supply-chain risk of a change. Callable by `evidence` as its supply-chain engine. +maturity: experimental +--- + +# /supply-chain-audit + +**"Is this bump safe?" is not one question.** A dependency can be free of known CVEs and still +reach `child_process` for the first time. It can have a clean policy diff and ship a +newly-added install script from a maintainer who joined last week. Each detector answers a +different question and is blind to the others, so a single green check is never the answer. + +> **Falsifier.** Any lane's finding left unaccounted for: a Socket alert nobody dispositioned, +> an advisory with no upgrade path or accepted-risk note, a capability grant with no call site. +> An unexplained finding is the output, not a nit to wave through. + +## Lanes + +| question | detector | disposition | +|---|---|---| +| what actually changed? | lockfile / `package.json` diff | direct vs transitive; resolution swaps; version range widening | +| known-vulnerable? | `yarn npm audit`, GitHub advisories, Dependabot | fixed-in version, or an explicit accepted-risk with reachability | +| behaving maliciously or anomalously? | **Socket Security** | per-alert disposition — see below | +| new capability reached? | **LavaMoat** policy diff | **delegate to `lavamoat-policy`** | +| install-time code execution? | `allowScripts` in `package.json` (`@lavamoat/allow-scripts`) | a newly-`true` entry is a finding in its own right | +| **is dependency source modified in-repo?** | **`.yarn/patches/*.patch`** | read the diff — see below | +| **is a version being forced?** | **`resolutions`** in `package.json` | pinned below a fix? stubbed out? | +| **are findings being suppressed?** | **`npmAuditIgnoreAdvisories`** in `.yarnrc.yml` | every entry needs a reason and a re-check date | +| **does untrusted code run in CI?** | **`uses:` pinning** in `.github/workflows` | third-party actions pinned to a full SHA, not a mutable tag | +| **does untrusted code run at install?** | **`.yarn/plugins/*.cjs`** + their `spec:` URLs | committed bundle reviewed; spec pinned, not `main` | + +## The fronts that no scanner covers + +Socket, `audit`, and LavaMoat all examine the dependency **as published**. The last five lanes +above are things *your own repo, or your CI,* does around dependencies afterwards, so no +upstream scanner sees them. Measured on `metamask-extension` today, to show these aren't hypothetical: + +- **Yarn patches — 53 of them.** A patch is arbitrary modification of a dependency's source, + applied at install, living in your repo. It is the single most direct injection point in the + list and the least watched: the package can be clean at every scanner and still execute your + patch. **Read every patch diff on change**, the same way you'd read a diff to `app/`. A patch + that grows beyond its stated purpose, or touches a file unrelated to the bug it works around, + is the finding. Record why each patch exists and what removes it (upstream fix, version bump) + — an unattributed patch is technical debt with a security surface. + +- **`resolutions` — 149 entries.** Forcing a version across the tree. Two failure modes: a pin + that holds a transitive *below* the version that fixed an advisory (audit may not flag it, + because the range resolves), and outright substitution — this repo maps several packages to + `npm:npm-empty-package@1.0.0` to neutralize them. Substitution is legitimate and deliberate, + but it means **"same version range" does not imply "same code"**, so treat a resolution + change as a dependency change and re-run the lanes on it. + +- **`npmAuditIgnoreAdvisories` — a suppression list.** Entries here are accepted risks by + definition, and this repo's numeric IDs carry no inline reason (its deprecation entries do). + This directly contradicts this skill's own falsifier: an unaccounted finding is the output. + Each entry wants a reason, an owner, and a condition that retires it. An ignore list nobody + revisits converts a finding into silence. + +- **CI action pinning — 7 of 47 third-party `uses:` are SHA-pinned.** The rest ride mutable + tags (`actions/checkout@v6`, `actions/github-script@v9`). A tag can be repointed by its owner + or by anyone who compromises that account, and CI holds secrets — this is the + `tj-actions/changed-files` failure mode. Pin third-party actions to a full 40-char commit + SHA. First-party (`MetaMask/*`, 25 here) is lower risk but the same mechanism. + +- **Yarn plugins execute at install with full privilege.** Three `.cjs` bundles are committed + (good — the committed bytes are what runs), but their `spec:` URLs point at + `raw.githubusercontent.com/.../main/...`, a moving branch. Re-importing pulls whatever `main` + holds that day. Pin the spec to a tag or SHA, and review the bundle diff when it changes. + +**Also consider `enableHardenedMode`** (Yarn 4) — not currently set here. It validates +resolutions and checksums against the registry, and is designed for exactly the untrusted-PR +case. And leave `checksumBehavior` at its default (`throw`): a checksum mismatch means the +registry served different bytes for a version you already resolved, which is a signal, not a +nuisance. + +Run the lanes the change actually touches. A lockfile-only bump of a build-time dev dependency +does not need the same treatment as a new runtime dependency in the wallet's hot path — but +say which lanes you ran and which you skipped, and why. + +## Method + +1. **Establish what changed before assessing it.** Direct bump, transitive pull-through, or a + *resolution swap* (same range, different resolved package)? The last is the easiest to miss + and the most interesting: an identifier substitution in a policy or lockfile + (`pkgC>name` replacing `pkgB>pkgA>name`) can mean the dependency was replaced rather than + updated. + +2. **Take each tool's findings as the worklist; don't re-derive them.** Socket, audit, and + LavaMoat all run in CI and are trusted machines. Re-implementing their detection by hand + re-does work and produces a less reproducible artifact. Your input is their output. + +3. **Prefer the CI-generated artifact over a local regeneration.** Where a bot regenerates + something (policies especially) and CI enforces drift, that committed artifact is + authoritative. Regenerate locally only as a **fallback** — bot hasn't run, branch unpushed, + variant not covered — and note that the provenance is weaker (your node version, OS, and + lockfile resolution, not CI's). Re-check against the bot's artifact once it runs. + +4. **Disposition every finding; the reason is the deliverable.** For each alert or advisory: + what the tool flagged, whether it is reachable from how *this* project uses the package, and + the outcome — fixed by upgrading to X, accepted with a stated reason, or blocking. Socket's + common alert classes need different reasoning: `install scripts` (what does it run, at whose + trust level), `new author` / `low download count` (typosquat and takeover surface), + `network access` / `filesystem access` in a package with no business doing either, + `obfuscated code`, `protestware`. A tool's severity is an input to that judgment, not a + substitute for it. + +5. **Cite at a pinned version, not a branch head.** Every claim about what a dependency does + resolves to a permalink at the version being installed. A branch-head link drifts out from + under the citation; the permalink *is* the evidence, because a reader clicks it and is + convinced without re-running anything. + +6. **Hand it to the owners.** Post the disposition where the people who own the dependency + read it, not as a unilateral assertion. This skill produces a justification for humans to + act on; it does not approve anything. + +## Capability containment → `lavamoat-policy` + +LavaMoat policy grants are a specialized lane with their own method and tooling. **Delegate to +`lavamoat-policy`** and fold its result in as this audit's capability-containment lane. + +Do not restate that lane's question as "does each new capability have a call site" — the policy +is generated from a real run, so it always does, and that check cannot fail. The lane's actual +output is a least-privilege triage: which grants are **removable** (their gate is never opened by +our usage), which are removable at a stated cost, which are load-bearing, plus anything the +reading turned up bearing on security. Carry those findings through; do not compress them to a +pass/fail. + +Keep the boundary straight in the writeup: **a clean policy diff does not mean a safe +dependency, and a known CVE does not appear as a new grant.** They are independent. + +## Output + +``` +Supply-chain assessment — -> () + lockfile/manifest + advisories → fixed in | accepted: | none + Socket → | no alerts + install scripts | unchanged + patches <.yarn/patches touched> → diff read: | unchanged + resolutions | unchanged + audit ignores → reason + retire-when | unchanged + ci actions → SHA-pinned? | unchanged + capabilities → lavamoat-policy: | no policy change + lanes skipped — +Unresolved: — | none +``` + +Lead with whatever is actionable — an unresolved finding, a removable capability, a patch whose +scope exceeded its purpose. Lanes that came back clean are a compact line each, not sections. +**No overall accept/reject verdict and no `@`-mentions**: the disposition belongs to the people +who own the dependency, and tagging them is the user's call, not this skill's. Close on what is +unresolved and what would settle it. + +## Related + +- `lavamoat-policy` — the capability-containment engine this skill delegates to. +- `evidence` — packages this skill's output as its [supply-chain evidence category](https://github.com/MetaMask/skills/blob/main/domains/pr-workflow/skills/evidence/references/evidence-catalog.md).