Add Fortify scanner THREAT_MODEL.md to third_party/nvd3 repo Bug: b/548455085 Change-Id: I9513a10426c9b322f34991ed2655bac451385889 Reviewed-on: https://gnocchi-internal-review.git.corp.google.com/c/third_party/nvd3/+/316652 Reviewed-by: Adam Stone <stoneadam@google.com> Autosubmit: Hughes Hilton <hugheshilton@google.com>
diff --git a/THREAT_MODEL.md b/THREAT_MODEL.md new file mode 100644 index 0000000..5934f1b --- /dev/null +++ b/THREAT_MODEL.md
@@ -0,0 +1,312 @@ +# Security Threat Model: nvd3 (AppSheet Fork) + +## Asset Definition & Scope + +- **Component:** nvd3 — a library of reusable, composable SVG chart components + built on D3.js v3. Upstream is `novus/nvd3`; per `METADATA` the fork's base + is upstream **v1.1.15b, last upgraded 2013-12-18**, i.e. a decade-old + snapshot that no longer tracks upstream. +- **Repository:** Git-on-Borg + (`https://gnocchi-internal.googlesource.com/third_party/nvd3`, branch + `main`). +- **Fork identity:** `package.json` declares `"version": "0.0.1"`, and the + Grunt uglify banner (`GruntFile.js`) stamps the artefacts accordingly — the + built `nv.d3.min.js` begins `/*! nvd3 - v0.0.1 - 2023-09-13 */`. The version + string is deliberately detached from upstream numbering, so upstream-version + matching by vulnerability scanners will not work on this asset. +- **Scope:** `src/` (the authored sources), and the concatenated/minified build + artefacts `nv.d3.js` and `nv.d3.min.js` produced from them by `GruntFile.js` + / `Makefile`. `nv.d3.css` / `.min.css` are presentational. + +> [!IMPORTANT] +> **This fork reaches production as a *vendored copy*, not as an npm/bower +> dependency.** No `package.json` in AppSheet's `jeenee` monorepo declares a +> dependency on `nvd3` — dependency-graph tooling will report this repository as +> unused. It is not. The build artefact in this repository is **byte-identical** +> (verified by MD5, `ba459888d567a9f5375c7d90c698e9cf`) to the file checked into +> the AppSheet web frontend at `jeenee/Nirvana/Scripts/app/plugins/nv.d3.min.js`, +> which is served to authenticated AppSheet users. This repository is the +> confirmed build source of live, Internet-facing production JavaScript, and +> findings here are directly exploitable findings in AppSheet. + +**Google-authored modification.** The single substantive fork change (commits +`f07d57e` "[nwd] Patching nv.d3 trusted types violations" and `189db60` "[nwd] +Fixing a bug - DOMPurify was not imported", kjamali@, Sept 2023) **inlines +DOMPurify 3.0.5 into the bundle** as `src/purify.js` and calls it from one +tooltip sink. Upstream nvd3 has no DOMPurify dependency; `src/purify.js` is +concatenated into `nv.d3.js` ahead of the library proper. This was done to +satisfy AppSheet's Trusted Types enforcement, not as a general XSS hardening +pass — see Priority Review Area 1. + +**Production consumers.** The vendored `nv.d3.min.js` is loaded by four live +legacy Razor views in `jeenee`, all with a CSP nonce: + +| View | Line | Surface | +|---|---|---| +| `V3/MainServer/Views/template/_showTablePartial.cshtml` | 91 | App editor — table/column detail | +| `V3/MainServer/Views/Shared/_logDetailsPartial.cshtml` | 76 | Audit-log detail | +| `V3/MainServer/Views/Shared/_perfDetailsPartial.cshtml` | 56 | Performance / expression trace detail | +| `V3/MainServer/Views/template/commoneditor/appStatsPartial.cshtml` | 102 | App usage statistics | + +The driver is `jeenee/Nirvana/Scripts/sp-theme/manageGraphs.js` (loaded by +`Views/template/AppStats.cshtml:49` and `Views/manage/ViewLog.cshtml:194`). +It builds chart data at `manageGraphs.js:72-88` — series `key` comes from +`data[i].series` and point `label` from the keys of `data[i].data` — then calls +`nv.addGraph(...)` at `manageGraphs.js:114` (and again at `:336` for +`showTimeGraph`). Those series keys and point labels derive from **AppSheet app, +table, column and view names and from user spreadsheet data**, i.e. from +attacker-influenceable strings. + +## Prioritization Signals + +- **1P OSS:** No. Third-party Apache-2.0 OSS maintained as an internal Google + fork for AppSheet. +- **1P Proprietary Shipped Software:** Yes. `nv.d3.min.js` is minified + JavaScript shipped verbatim to AppSheet editor and admin users. +- **High-Risk Code Surface:** Yes. The library's core rendering idiom is + building HTML strings and assigning them to `innerHTML`, and it bundles a + pinned 2023 copy of a security-critical sanitiser (DOMPurify 3.0.5) that + receives no upstream updates. +- **Perimeter Exposure:** **Yes.** The consuming views are authenticated + AppSheet editor / admin pages served on the public Internet. +- **Data Sensitivity:** Medium–High. Chart labels are derived from customer + app schemas, view names and spreadsheet cell data; the audit-log and + app-stats surfaces additionally display end-user identifiers and activity + timelines. The library also renders inside pages that carry the operator's + authenticated session. +- **Untrusted Input Handling:** Yes. Series keys, point labels, axis label + text, tooltip header/value strings, and the optional `footer` field all flow + from application data into the DOM. +- **Business Value:** Renders AppSheet's usage-statistics, audit-log and + performance-diagnostics surfaces. These are precisely the surfaces where + **one tenant's data is displayed to another principal** — see the + cross-tenant framing below. + +### Cross-tenant framing (stated honestly) + +A malicious table name, column name or spreadsheet value primarily renders in +**the creator's own editor**, which is self-XSS and low severity on its own. The +escalation path is that the same data is rendered on shared and delegated +surfaces: + +- **team/organisation admins** viewing app statistics and audit logs for apps + they did not author; +- **AppSheet support and operations staff** opening a customer's app stats, + audit log or performance trace while investigating a ticket — a + cross-tenant, higher-privilege viewer. + +So the realistic threat is **stored XSS delivered from a low-privilege app +creator to a higher-privilege admin or support operator**, not drive-by attack +of anonymous users. That is the severity this asset should be triaged at. + +## Scanning Harness Prompts + +1. **Incomplete DOMPurify coverage in `src/tooltip.js` (highest priority).** + The file contains **two** tooltip implementations and the fix was applied to + only one: + - `nv.tooltip.show()` (the legacy helper) **is** patched: + `var safeContent = DOMPurify.sanitize(content, {RETURN_TRUSTED_TYPE: true}); + container.innerHTML = safeContent;` (`src/tooltip.js:340-341`, built + `nv.d3.js:2339-2340`). + - `nv.models.tooltip()` (the "new way", per the file's own header comment) + is **not**: its private `getTooltipContainer(newContent)` does a bare + `container.node().innerHTML = newContent;` (`src/tooltip.js:167`, built + `nv.d3.js:2166`) with **no DOMPurify call anywhere in that code path**. + Its default `contentGenerator` builds the markup with D3 `.html()` + calls — `.html(headerFormatter(d.value))` (`:83`), + `.html(function(p) {return p.key})` (`:101`), + `.html(function(p,i) { return valueFormatter(p.value,i) })` (`:104`) — + serialises via `table.node().outerHTML`, then **string-concatenates an + unescaped footer**: `html += "<div class='footer'>" + d.footer + "</div>"` + (`:119-120`). Flag every one of these as an XSS sink. +2. **Reachability of the unsanitised sink.** `nv.models.tooltip` is instantiated + by `nv.interactiveGuideline` (`src/interactiveLayer.js:11`, exposed as + `layer.tooltip` at `:157`) and invoked as + `interactiveLayer.tooltip.position(...).data({value, series})()` by + `src/models/lineChart.js:276`, `src/models/stackedAreaChart.js:401` and + `src/models/cumulativeLineChart.js:498` — but only when the consumer calls + `chart.useInteractiveGuideline(true)`. Scanners should treat this as a live, + reachable sink in the shipped bundle and as a public API + (`nv.models.tooltip` is exported on `window.nv.models`). +3. **Stale bundled sanitiser.** `src/purify.js` is a frozen copy of **DOMPurify + 3.0.5** (`DOMPurify.version = '3.0.5'`, `src/purify.js:319`), released 2023. + It is inlined into `nv.d3.js`/`nv.d3.min.js`, so it will never be updated by + `npm audit`, Dependabot, or any lockfile-based tooling, and it is invisible to + SCA that keys off `package.json`. Check 3.0.5 against subsequently disclosed + DOMPurify bypasses and mutation-XSS issues, and flag the absence of any + update mechanism as a finding in its own right. +4. **`nv.utils.pjax` — network fetch plus history rewrite.** + `src/utils.js:87-105` binds click handlers that call + `history.pushState(this.href, …)` and then `d3.html(href, …)`, parsing the + response and splicing a selected node into the live DOM via + `target.parentNode.replaceChild(...)`. There is no origin or scheme check on + `href`. This is a general-purpose fetch-and-inject primitive with no + relationship to charting. +5. **SVG identifier and selector construction.** Several models build element + IDs and `clip-path` URL references by string concatenation from a chart `id` + — e.g. `src/models/historicalBar.js:95,98,102` + (`'nv-chart-clip-path-' + id`), `src/models/line.js:99,101` + (`'url(#nv-edge-clip-' + scatter.id() + ')'`), and the equivalents in + `multiBar.js`, `multiBarTimeSeries.js`, `ohlcBar.js`, `scatter.js`, + `lineWithFisheye.js`. Those IDs are also fed back into + `wrap.select('#…')` selectors. Verify that no caller can set a chart `id` to + an attacker-controlled string and thereby break out of the ID/selector + context. +6. **Build-artefact parity.** `nv.d3.js` and `nv.d3.min.js` are hand-committed, + not produced by CI. Confirm they are faithful builds of `src/` at every + commit; the minified artefact is what actually ships. + +## Entry Points and Untrusted Inputs + +| Entry Point | Type | Trusted? | Validation | +|---|---|---|---| +| Chart datum `series[].key` | JS data → DOM | No — derives from app/table/column/view names (`manageGraphs.js:76`) | `nv.models.tooltip` default `contentGenerator`: `.html(function(p){return p.key})` — **none**. Legacy `nv.tooltip.show` path: DOMPurify | +| Chart datum point `label` / `value` | JS data → DOM | No — derives from user spreadsheet data (`manageGraphs.js:81-82`) | Passed through `valueFormatter` then `.html(...)` — **no escaping** in `nv.models.tooltip` | +| `d.value` (tooltip header) | JS data → DOM | No | `.html(headerFormatter(d.value))` — `headerFormatter` defaults to identity, **no escaping** | +| `d.footer` | JS data → DOM | No | String-concatenated into `"<div class='footer'>" + d.footer + "</div>"` — **no escaping** | +| `chart.tooltipContent(fn)` / `contentGenerator(fn)` | JS API | Caller-supplied | Caller-defined HTML string; the default in e.g. `src/models/multiBarChart.js:28-31` concatenates `key`, `x`, `y` into `<h3>`/`<p>` markup | +| `chart.xAxis.axisLabel(text)` | JS API → SVG | No — `options.xAxisName` (`manageGraphs.js:106,108`) | Rendered via D3 `.text()` on an SVG `<text>` node in `src/models/axis.js` (**not** `.html()`), so this is text-context and comparatively safe | +| `nv.utils.pjax(links, content)` link `href` | DOM attribute → HTTP | No | **None.** No scheme or origin allowlist before `d3.html(href, …)` | +| Chart `id` | JS API → SVG ids and selectors | Caller-supplied | Concatenated into `clipPath` `id` attributes and `#…` selectors without escaping | + +## Trust Boundaries and Auth Assumptions + +- **Authentication:** None at the library layer. All four consuming views are + behind AppSheet's authenticated editor/admin session; the library runs with + that session's full ambient authority in the browser. +- **Authorization:** None at the library layer. The library renders whatever + data the hosting page hands it and makes no distinction between + first-party-generated labels and customer-authored strings. +- **Implicit trust:** The library assumes chart data — series keys, point + labels, header values, footers — is trustworthy HTML. In AppSheet that + assumption is false: those strings originate from app creators and end-user + spreadsheet content. +- **Boundary crossings:** + 1. App creator / end user authors a table name, column name, view name or + spreadsheet cell value. + 2. AppSheet server surfaces it in stats / audit-log / performance data. + 3. `manageGraphs.js` maps it into `series[].key` and point `label`. + 4. nvd3 renders it into the DOM of an **editor, team-admin or support + operator's** authenticated page. + Step 4 is the trust boundary. The sanitisation gap in `nv.models.tooltip` is + exactly at that boundary. + +### Mitigating controls (verify, do not assume) + +- **CSP nonce.** All four views load the bundle with `nonce="@CspNonce"`, + indicating a nonce-based CSP. A correct nonce CSP blocks injected + `<script>` elements. This limits, but does not eliminate, impact: injected + markup, event-handler attributes rejected by CSP still leave overlay/UI-redress + and data-exfiltration-via-markup avenues open, and the protection evaporates + if `script-src` carries a permissive fallback. +- **Trusted Types.** The 2023 fork exists to satisfy TT enforcement. Note that + the bare `container.node().innerHTML = newContent` in `getTooltipContainer` + would be *blocked* by TT enforcement rather than sanitised — meaning under + full TT the unsanitised path fails closed (tooltip breaks) rather than + executing. **Confirm whether TT is enforced (not report-only) on each of the + four consuming routes**; if it is report-only, the sink is live. +- **Current AppSheet usage narrows exposure.** The only nvd3 chart model + AppSheet instantiates is `nv.models.multiBarChart` + (`manageGraphs.js:46`, `V3/MainServer/Views/template/commoneditor/_perfDisplayPartial.cshtml:148`), + whose tooltip routes through the **DOMPurify-sanitised** `nv.tooltip.show` + (`src/models/multiBarChart.js:72`). AppSheet does **not** currently call + `useInteractiveGuideline(true)`, so the unsanitised `nv.models.tooltip` path + is not exercised by today's call sites. It remains shipped, exported, and one + line of caller code away from being live — which is why it is still ranked + first below. + +## Sensitive Data Paths + +| Data Type | Source | Destination | Protection | +|---|---|---|---| +| App / table / column / view names | Customer-authored app schema | `series[].key` → tooltip DOM | DOMPurify on the `nv.tooltip.show` path; **none** on the `nv.models.tooltip` path | +| Spreadsheet cell values | Customer / end-user data source | Point `label` / `value` → tooltip and axis DOM | Axis labels use `.text()` (safe); tooltip values use `.html()` (unprotected on the `nv.models.tooltip` path) | +| End-user activity and identifiers | AppSheet audit log | `_logDetailsPartial.cshtml` charts | Rendered in an admin/support session; same tooltip sinks | +| App usage statistics | AppSheet telemetry | `appStatsPartial.cshtml` / `AppStats.cshtml` charts | Same tooltip sinks; cross-tenant viewers possible | +| Expression / performance traces | AppSheet app compiler & runtime | `_perfDetailsPartial.cshtml` charts | Same tooltip sinks; traces may embed user formulas and column names | + +## Privileged Actions + +| Action | Location | Guard | +|---|---|---| +| Assign untrusted string to `innerHTML` | `src/tooltip.js` : `nv.models.tooltip` → `getTooltipContainer` | **None** | +| Assign string to `innerHTML` after sanitisation | `src/tooltip.js` : `nv.tooltip.show` | `DOMPurify.sanitize(content, {RETURN_TRUSTED_TYPE: true})` | +| Render untrusted values as HTML via D3 | `src/tooltip.js` : `nv.models.tooltip` default `contentGenerator` (`.html()` × 3, plus footer concatenation) | **None** | +| Outbound HTTP fetch of a DOM-supplied URL and DOM replacement | `src/utils.js` : `nv.utils.pjax` → `load` (`d3.html`, `replaceChild`) | **None.** No scheme/origin allowlist | +| Browser history mutation | `src/utils.js` : `nv.utils.pjax` (`history.pushState`) | **None** | +| Bundled HTML sanitiser (security-critical dependency) | `src/purify.js` (DOMPurify 3.0.5), inlined into `nv.d3.js` / `nv.d3.min.js` | Pinned; no update path, invisible to SCA | +| SVG `id` / `clip-path` / selector construction from a chart `id` | `src/models/{historicalBar,line,multiBar,multiBarTimeSeries,ohlcBar,scatter,lineWithFisheye,discreteBarChart}.js` | String concatenation, no escaping | + +## Priority Review Areas + +1. **`nv.models.tooltip` was left out of the DOMPurify fix + (`src/tooltip.js` → `getTooltipContainer` and the default + `contentGenerator`).** The 2023 patch sanitised the legacy `nv.tooltip.show` + helper and stopped there, even though the file's own header comment calls + `nv.models.tooltip` "the updated, new way to render tooltips". The result is + a shipped, exported tooltip renderer that takes series keys, point values, + header values and a footer straight into `.html()` and `innerHTML` with no + escaping and no sanitiser. It is invoked in the bundle by + `lineChart`/`stackedAreaChart`/`cumulativeLineChart` under + `useInteractiveGuideline(true)`. **Accuracy note:** AppSheet's current call + sites use only `multiBarChart`, which routes through the *sanitised* helper, + so this is a **latent** rather than presently-exercised sink — but it is one + `useInteractiveGuideline(true)` away, and the same file already proves the + project knows this data is untrusted. Fix by routing + `getTooltipContainer` through `DOMPurify.sanitize(..., {RETURN_TRUSTED_TYPE: true})` + and replacing the `.html()` calls with `.text()`. +2. **Stale, inlined DOMPurify 3.0.5 with no update path (`src/purify.js`).** A + security control frozen at a 2023 release and concatenated into a build + artefact is invisible to every dependency scanner AppSheet runs. Establish + ownership and an update cadence, check 3.0.5 against subsequently published + bypasses, and prefer consuming DOMPurify as a real, version-pinned dependency + over an inlined copy. +3. **`nv.utils.pjax` (`src/utils.js`).** A fetch-and-inject plus + `history.pushState` primitive with no origin validation, shipped inside a + charting library. Determine reachability: no AppSheet code references + `nv.utils.pjax`, and it serves no charting purpose. **Recommendation: delete + it from the fork** — it is pure attack surface with zero product value. +4. **Cross-tenant exposure of the audit-log, app-stats and performance views.** + Confirm which principals can view another tenant's charts (team/org admins, + AppSheet support/ops tooling) and treat any tooltip XSS on those routes as + privilege-escalating stored XSS rather than self-XSS. +5. **Effective CSP and Trusted Types enforcement mode on the four consuming + routes.** Verify `script-src` has no permissive fallback and determine + whether `require-trusted-types-for 'script'` is enforcing or report-only. + These two controls are what currently bound the impact of every sink above. +6. **SVG identifier and selector construction.** Audit chart `id` propagation + into `clipPath` `id` attributes, `url(#…)` references, and `d3.select('#'+id)` + selectors across `src/models/` for injection or selector-confusion. +7. **Decade-old upstream base.** The fork is built on nvd3 v1.1.15b (2013) and + D3 v3 (`bower.json` pins `d3 ~3.3.5`). Neither receives security + maintenance. Evaluate migrating these four legacy views off nvd3 entirely as + the durable remediation. + +## Out of Scope + +- **`jeenee/Nirvana/Scripts/app/plugins/nv.d3.indentedTree.js` is NOT produced + by this fork.** Its header comment pins it to upstream + `https://github.com/novus/nvd3/blob/v1.2.1/src/models/indentedTree.js` — a + *different* upstream version from this repository's v1.1.15b base — and it is + a separately vendored, standalone file in the `jeenee` repository. It is + loaded alongside `nv.d3.min.js` by `_showTablePartial.cshtml:92`, + `_logDetailsPartial.cshtml:77` and `_perfDetailsPartial.cshtml:57`, and + instantiated as `nv.models.indentedTree()` at `_showTablePartial.cshtml:207` + and `_perfDetailsPartial.cshtml:352`. This repository does contain a + `src/models/indentedTree.js`, but it is **not** the file that ships. + **`nv.d3.indentedTree.js` must be reviewed on the `jeenee` side, not here.** +- `deprecated/`, `examples/`, `test/`, `lib/` — sample pages, fixtures and + vendored demo dependencies with no production code path. +- `src/models/backup/` — superseded model sources retained in-tree; not + concatenated into the build. +- `GruntFile.js`, `Makefile`, `build.bat`, `.jshintrc` and the Grunt/Babel + `devDependencies` — build-time only, not executed in CI for this repository, + and producing no automatically served artefact. +- `nv.d3.css` / `nv.d3.min.css` — stylesheets with no scripting surface. +- D3.js itself. The library requires a global `d3` (v3), supplied separately by + AppSheet; it is not vendored in this repository. +- Server-side generation and authorisation of the app-stats, audit-log and + performance data feeds, `manageGraphs.js`, and the CSP/Trusted Types + middleware. All live in the `jeenee` repository and must be reviewed there; + they are referenced here only as context and as mitigating controls to be + verified.