Add Fortify scanner THREAT_MODEL.md to third_party/@protobufjs/inquire repo Bug: b/548454751 Change-Id: I791dfc139d23cb9376831b273324a087ff89ffd6 Reviewed-on: https://gnocchi-internal-review.git.corp.google.com/c/third_party/@protobufjs/inquire/+/316631 Reviewed-by: Trevor Ryland <tryland@google.com>
diff --git a/THREAT_MODEL.md b/THREAT_MODEL.md new file mode 100644 index 0000000..a9ac1ee --- /dev/null +++ b/THREAT_MODEL.md
@@ -0,0 +1,217 @@ +# Security Threat Model: @protobufjs/inquire (AppSheet Fork) + +## Asset Definition & Scope + +`@protobufjs/inquire` is a ~25-line JavaScript shim (repository: +`sso://gnocchi-internal/third_party/@protobufjs/inquire`, branch `main`; +mirrored at `appsheet-third-party/@protobufjs/inquire`). It exports exactly one +function, `inquire(moduleName)`, whose entire purpose is to perform a *guarded, +bundler-invisible* `require()` of an optional CommonJS module and return `null` +instead of throwing when that module is absent. + +- **Upstream:** `protobufjs/protobuf.js` → `lib/inquire`, npm version `1.1.0`, + upstream revision `56b1e64979dae757b67a21d326e16acee39f2267` + (`last_upgrade_date` 2023-03-27, per `METADATA`). +- **Scope:** `index.js` (the single implementation file) and `index.d.ts`. + Everything else in the repository is packaging (`package.json`, `LICENSE`, + `README.md`, `METADATA`) or upstream `tape` tests under `tests/`. +- **Deployment context:** Bundled into the AppSheet web frontend + (`jeenee/Nirvana/Content`) and executed in the end user's browser. It is a + transitive runtime dependency of `protobufjs@7.4.0`; the AppSheet + `package.json` also pins it as a *direct* dependency purely to force npm to + resolve every copy in the tree to this fork. + +> [!IMPORTANT] +> This is a genuinely tiny library with no untrusted input surface. Its +> aggregate risk is **Low**. It is nevertheless worth scanning because the +> implementation contains two `eval()` calls, which makes it a high-noise +> target for static analysers and a plausible supply-chain gadget if a future +> caller ever passes a non-literal module name. + +## The AppSheet Customization + +The repository has a single commit — `9f141da4daaf73c32cb78a8bcf38333870e80cbc` +("gnitial import of @protobuf/inquire") — which already contains the Google +patch. Diffing `index.js` against upstream 1.1.0 shows the fork wraps the +original `eval`-based require in a Trusted Types branch: + +```js +function inquire(moduleName) { + try { + if (typeof self !== 'undefined' && self.trustedTypes && self.trustedTypes.createPolicy) { + const escapeScriptPolicy = trustedTypes.createPolicy("myEscapePolicy", { + createScript: (string) => "require " + string, + }); + safeScript = escapeScriptPolicy.createScript(moduleName); // <-- undeclared + var mod = eval(safeScript); + } else { + var mod = eval("quire".replace(/^/,"re"))(moduleName); // upstream path + } + if (mod && (mod.length || Object.keys(mod).length)) + return mod; + } catch (e) {} + return null; +} +``` + +Two independent defects in the new branch mean the dynamic require is +**effectively dead code in every Trusted-Types-capable browser** (Chrome, Edge): + +1. `safeScript` is assigned without `var`/`let`/`const`. `index.js` is under + `"use strict"`, so the assignment throws + `ReferenceError: safeScript is not defined` before `eval` is ever reached. +2. Even if `safeScript` were declared, the policy's `createScript` produces the + string `"require " + moduleName`, which is not valid JavaScript + (`require foo` is a `SyntaxError`, not a call expression). + +Either fault is swallowed by the bare `catch (e) {}` and `inquire()` returns +`null`. `protobufjs` treats `null` as "optional module unavailable" and falls +back to its pure-JS `Long`/`Buffer` code paths, so behaviour is correct — the +patch neutralises the dynamic require rather than sanitising it. The legacy +`else` branch retains the original upstream `eval("quire".replace(/^/,"re"))` +obfuscated require for non-TT environments (Node, Safari, jsdom under Jest). + +## Reachability and Call Sites + +`inquire()` is invoked from exactly two places in the dependency graph, both in +`protobufjs/src/util/minimal.js`, and both with a **string literal**: + +| Caller | Argument | +|---|---| +| `util.Buffer` initialisation | `util.inquire("buffer")` | +| `util.Long` initialisation | `util.inquire("long")` | + +There is no path by which a caller — let alone customer-authored app data — +supplies the `moduleName` argument. Arbitrary-module-load / dynamic-require +abuse is **not reachable** in the current AppSheet integration. + +## Prioritization Signals + +- **1P OSS:** No (Third-party OSS maintained as an internal customized fork + for Google AppSheet) +- **1P Proprietary Shipped Software:** Yes (Minified into the AppSheet + frontend JavaScript bundles shipped to browsers) +- **High-Risk Code Surface:** Yes, but narrowly — the file contains `eval()` + and constructs a Trusted Types policy, both intrinsically dangerous + primitives (CWE-95, CWE-676). The surface is 25 lines with no parsing, no + I/O, and no data handling. +- **Perimeter Exposure:** Yes (AppSheet is a 1P service exposed to the public + Internet and its customers; this code executes in every end user's browser + session that loads a protobuf-backed page) +- **Data Sensitivity:** Low (the module handles no data — its only argument at + every call site is a hardcoded module name, and its return value is a module + object, never user content) +- **Untrusted Input Handling:** No (customer-authored app data never reaches + `inquire`; the only inputs are the literals `"buffer"` and `"long"`) +- **Business Value:** Low-severity but non-zero: it is a mandatory transitive + dependency of `protobufjs`, which the AppSheet frontend uses for wire + (de)serialization. A regression that made `moduleName` caller-controlled, or + that leaked the Trusted Types policy object, would upgrade this from a + dormant `eval` to an exploitable script-injection sink on a + public-Internet-facing surface. + +## Scanning Harness Prompts + +1. **Dynamic require / arbitrary module load (primary question).** Confirm that + `inquire(moduleName)` in `index.js` is only ever reached with compile-time + string constants. Flag *any* call site — inside this repo, in + `protobufjs`, or in AppSheet application code — where `moduleName` derives + from a variable, a property lookup, a network response, `location.*`, or + customer-authored app configuration. That, and only that, converts these + `eval()` calls into an arbitrary-code-execution primitive. +2. **Trusted Types policy hygiene.** Review the + `trustedTypes.createPolicy("myEscapePolicy", { createScript: ... })` call in + `index.js:inquire`. Verify (a) the policy object stays a function-local + `const` and is never exported, attached to `window`, or otherwise reachable + by other page scripts — a reachable permissive `createScript` policy is a + universal Trusted Types bypass gadget; (b) the policy name is scoped in the + page CSP `trusted-types` allowlist and does not shadow `default`; (c) + repeated invocation (duplicate policy name) fails closed via the existing + `try`/`catch` rather than throwing out of the module. +3. **Both `eval()` sinks.** `eval(safeScript)` and + `eval("quire".replace(/^/,"re"))(moduleName)`. The `replace` is deliberate + upstream obfuscation to hide the identifier from webpack/browserify static + analysis; confirm no additional obfuscated indirection has been introduced + and that neither sink can be reached with attacker-influenced text. +4. **Silent failure semantics.** The bare `catch (e) {}` swallows every error, + including the strict-mode `ReferenceError` documented above. Verify that + downstream `protobufjs` code correctly handles a `null` return (it does + today: it falls back to pure-JS `Long`/`Buffer`), so the neutralised require + cannot cause an unhandled exception, an infinite retry, or a silent + correctness bug in message decoding. +5. **Fork drift.** If this fork is ever re-synced with upstream `protobufjs`, + re-verify that the Trusted Types branch survives and that the strict-mode + `safeScript` behaviour (fail-closed to `null`) is preserved or deliberately + replaced with an equally closed implementation. + +## Entry Points and Untrusted Inputs + +| Entry Point | Type | Trusted? | Validation | +|---|---|---|---| +| `inquire(moduleName)` (`index.js`) | Exported function call, in-process | Yes — every known call site passes a hardcoded literal (`"buffer"`, `"long"`) from `protobufjs/src/util/minimal.js` | None. The value is passed straight into a Trusted Types `createScript` or into the obfuscated `require`. Safety rests entirely on the caller contract, not on validation. | +| `self.trustedTypes` (`index.js`) | Ambient browser global / feature detection | Yes (user-agent supplied) | Guarded with `typeof self !== 'undefined' && self.trustedTypes && self.trustedTypes.createPolicy` before use | +| Resolved module object (`mod`) | Result of `require`/`eval` | Yes (bundle-resolved module, not user data) | Emptiness check only: `mod && (mod.length \|\| Object.keys(mod).length)` | + +## Trust Boundaries and Auth Assumptions + +- **Authentication**: None. This is an in-process library function with no + network, IPC, or filesystem interface. +- **Authorization**: None. Any code already executing in the bundle can call + `inquire`; there is nothing to authorise against. +- **Implicit trust**: The library implicitly trusts its caller to supply a + static, non-attacker-influenced module name. This is the single load-bearing + assumption in the entire threat model. +- **Boundary crossings**: None at the library level. The only boundary of note + is the browser's Trusted Types / CSP boundary, which the fork touches by + minting a `createScript` policy — the code moves *toward* that boundary but, + because of the strict-mode fault, never actually crosses it. + +## Sensitive Data Paths + +| Data Type | Source | Destination | Protection | +|---|---|---|---| +| Module name string | Hardcoded literals in `protobufjs/src/util/minimal.js` (`"buffer"`, `"long"`) | Trusted Types `createScript` → `eval`, or obfuscated `require()` | Not sensitive; not user-derived. No sanitisation applied or required at present. | +| Resolved module object | Bundler module registry | `protobufjs` `util.Buffer` / `util.Long` | Emptiness check only; returned to caller by reference | + +No secrets, credentials, tokens, PII, or customer app data pass through this +module. + +## Privileged Actions + +| Action | Location | Guard | +|---|---|---| +| Dynamic script evaluation (`eval` of a `TrustedScript`) | `index.js:inquire` (Trusted Types branch) | Trusted Types `createScript` policy; additionally fails closed via strict-mode `ReferenceError` on the undeclared `safeScript`, caught by the surrounding `try`/`catch` | +| Dynamic module load (obfuscated `eval("quire".replace(/^/,"re"))(moduleName)`) | `index.js:inquire` (legacy / non-TT branch) | None beyond the caller contract that `moduleName` is a literal; failure is swallowed by `catch (e) {}` | +| Trusted Types policy creation (`trustedTypes.createPolicy("myEscapePolicy", …)`) | `index.js:inquire` | Feature-detection guard only. Policy name must be permitted by the page's CSP `trusted-types` directive; the policy object is a function-local `const` and is not exported. | + +## Priority Review Areas + +1. **Caller-controlled `moduleName` (highest value, lowest current + likelihood).** The one question that matters for this library is whether the + require path can ever be attacker-influenced. Today it cannot: the only two + call sites pass `"buffer"` and `"long"`. Scanning should be tuned to alert + loudly on any new dynamic argument rather than on the mere presence of + `eval`. +2. **Trusted Types policy leakage in `inquire`.** A permissive `createScript` + policy that escapes function scope would be a page-wide Trusted Types bypass + — far more severe than anything else in this repository. Confirm it stays + local and that `"myEscapePolicy"` is intentional in the AppSheet CSP + allowlist (`go/trusted-types-appsheet`). +3. **Correctness of the fail-closed patch.** The Google patch neutralises the + dynamic require through two accidental-looking faults (undeclared + `safeScript` under `"use strict"`, and a `createScript` template that + produces invalid JavaScript). This is the desired security outcome but it is + fragile: a well-meaning cleanup that adds `var safeScript` and fixes the + template would *re-enable* `eval`-based dynamic require in the browser. + Treat any change to these lines as security-relevant and require the + fail-closed behaviour to be made explicit (e.g. `return null;`) rather than + incidental. +4. **Bare `catch (e) {}` error swallowing.** Low severity, but it hides the + permanent `ReferenceError` and would equally hide a genuine CSP/Trusted + Types violation or a bundler resolution failure, delaying detection of a + regression in area 3. + +## Out of Scope + +Teams can fill out this section with any known filed vulnerability bugs that +they've determined should be out of scope from future Fortify bug filing.