LibreKAT/Ocideck
LibreKAT/Ocideck
Watch
4
0
Fork
You've already forked Ocideck
 0
Code Issues 2 Pull requests Projects Releases 4 Packages Activity Actions

[Feature] Security hardening: nine items below the blocking layer #516

New issue
Closed
opened 2026-07-22 09:28:34 +00:00 by brenno · 3 comments
brenno commented 2026-07-22 09:28:34 +00:00
Owner
Copy link

Nine security items from the release review that are hardening rather than open holes. The blocking ones are fixed; these are the layer underneath.

  • audienceBoundary only checks three hard-coded files (tool/check_conventions.dart), while SECURITY_DESIGN §8 claims a general compile-time guarantee. A fourth export surface would not be seen. Make the gate generic.
  • sanitize_svg.dart is a deny-list. <style> contents, <set attributeName="on…"> and semicolon lists in values= slip through. Harmless today (only flutter_svg reads it), but a deny-list is the wrong shape for this. Make it an allow-list.
  • Unbounded reads: chart data (file_service_open.dart:86), sidecars (file_service.dart:669,682), the nested CVE zip (cve_bulk_ingest.dart:223).
  • connect-src 'self' https: in web/index.html permits a no-cors POST to any host.
  • Embed navigation compares with contains('youtube.com') rather than on host (media_previews_video.dart).
  • DocumentSignature is an unkeyed hash over a name — consider real signing (Ed25519/PKI) alongside it.
  • WinZip AES-256 uses PBKDF2-SHA1 with 1000 iterations; password_strength.dart reasons as if the KDF were stronger. The claim is literally true and the KDF is weak.
  • CMS and certificate-chain verification of the RFC 3161 token. Deliberately not built when the seal moved to a sidecar, with the reasoning recorded; the nonce is now sent and echoed but not checked. Revisit whether the trust-anchor cost is worth it.
  • network_sink_guard_test does not cover Process.start or WebViewController.

Each is small on its own. Weigh them individually rather than as one batch: some are worth a gate, some are worth a line of code, and at least one (the KDF) is a compatibility decision.

Nine security items from the release review that are hardening rather than open holes. The blocking ones are fixed; these are the layer underneath. - **`audienceBoundary` only checks three hard-coded files** (`tool/check_conventions.dart`), while SECURITY_DESIGN §8 claims a general compile-time guarantee. A fourth export surface would not be seen. Make the gate generic. - **`sanitize_svg.dart` is a deny-list.** `<style>` contents, `<set attributeName="on…">` and semicolon lists in `values=` slip through. Harmless today (only `flutter_svg` reads it), but a deny-list is the wrong shape for this. Make it an allow-list. - **Unbounded reads**: chart data (`file_service_open.dart:86`), sidecars (`file_service.dart:669,682`), the nested CVE zip (`cve_bulk_ingest.dart:223`). - **`connect-src 'self' https:`** in `web/index.html` permits a no-cors POST to any host. - **Embed navigation compares with `contains('youtube.com')`** rather than on host (`media_previews_video.dart`). - **`DocumentSignature` is an unkeyed hash over a name** — consider real signing (Ed25519/PKI) alongside it. - **WinZip AES-256 uses PBKDF2-SHA1 with 1000 iterations**; `password_strength.dart` reasons as if the KDF were stronger. The claim is literally true and the KDF is weak. - **CMS and certificate-chain verification of the RFC 3161 token.** Deliberately not built when the seal moved to a sidecar, with the reasoning recorded; the nonce is now sent and echoed but not checked. Revisit whether the trust-anchor cost is worth it. - **`network_sink_guard_test` does not cover `Process.start` or `WebViewController`.** Each is small on its own. Weigh them individually rather than as one batch: some are worth a gate, some are worth a line of code, and at least one (the KDF) is a compatibility decision.
brenno commented 2026-07-22 12:05:45 +00:00
Author
Owner
Copy link

Three of the nine are done, in the order the maintainer's rule now requires — security before bugs before features.

Done

  • Unbounded sidecar reads (#542). All three read paths in ImageSidecarStore were unbounded. The write path was the sharper one: without a cap it carried on with an empty map and wrote that over the file, wiping every other caption in the folder. 1 MiB cap, mutation-checked.
  • Embed host comparison (#543). The YouTube branch already compared on host and its comment names the exact attack; the Vimeo branch beside it used contains. Not a gap in knowledge — the reasoning was written down and applied to one of the two branches. Neither had a test.
  • connect-src (#546). No directive changed; the reasoning did. The comment credited CORS with protecting this, which is true for reads and false for a no-cors POST — that request is sent, only the response is opaque. The actual control is script-src 'self' without the dynamic-code escapes. A security comment that credits the wrong mechanism is worse than none: the next reader believes it is covered.

Left, with what I learned about two of them

  • audienceBoundary generic. Measured before attempting: 208 raw Deck/List<Slide> parameters across 67 files outside privacy/. So the obvious inversion — baseline every Deck-taking function — produces noise, not signal. A workable rule has to key on output: a function that takes a raw deck and reaches an output primitive. That is a design, not a quick fix.
  • PBKDF2-SHA1 at 1000 iterations is already a recorded deviation in assurance/ASVS-5.0.0-afwijkingen.md, with the reasoning that the number is fixed by the WinZip-AES specification and not raisable from OciDeck. If it is resolved it should be resolved there, by leaving WinZip AES — not re-decided in a hardening pass.
  • sanitize_svg deny-list → allow-list; the remaining unbounded reads (chart data, the nested CVE zip); DocumentSignature as an unkeyed hash; CMS/chain verification of the RFC 3161 token; network_sink_guard_test coverage for Process.start and WebViewController.

Relabelled bug — several of these are defects rather than improvements, and under the new ordering that matters for when they get picked up.

Three of the nine are done, in the order the maintainer's rule now requires — security before bugs before features. **Done** - **Unbounded sidecar reads** (#542). All three read paths in `ImageSidecarStore` were unbounded. The write path was the sharper one: without a cap it carried on with an empty map and wrote that over the file, wiping every other caption in the folder. 1 MiB cap, mutation-checked. - **Embed host comparison** (#543). The YouTube branch already compared on host and its comment names the exact attack; the Vimeo branch beside it used `contains`. Not a gap in knowledge — the reasoning was written down and applied to one of the two branches. Neither had a test. - **`connect-src`** (#546). No directive changed; the *reasoning* did. The comment credited CORS with protecting this, which is true for reads and false for a `no-cors` POST — that request is sent, only the response is opaque. The actual control is `script-src 'self'` without the dynamic-code escapes. A security comment that credits the wrong mechanism is worse than none: the next reader believes it is covered. **Left, with what I learned about two of them** - **`audienceBoundary` generic.** Measured before attempting: 208 raw `Deck`/`List<Slide>` parameters across 67 files outside `privacy/`. So the obvious inversion — baseline every Deck-taking function — produces noise, not signal. A workable rule has to key on *output*: a function that takes a raw deck **and** reaches an output primitive. That is a design, not a quick fix. - **PBKDF2-SHA1 at 1000 iterations** is already a recorded deviation in `assurance/ASVS-5.0.0-afwijkingen.md`, with the reasoning that the number is fixed by the WinZip-AES specification and not raisable from OciDeck. If it is resolved it should be resolved *there*, by leaving WinZip AES — not re-decided in a hardening pass. - `sanitize_svg` deny-list → allow-list; the remaining unbounded reads (chart data, the nested CVE zip); `DocumentSignature` as an unkeyed hash; CMS/chain verification of the RFC 3161 token; `network_sink_guard_test` coverage for `Process.start` and `WebViewController`. Relabelled `bug` — several of these are defects rather than improvements, and under the new ordering that matters for when they get picked up.
brenno commented 2026-07-22 15:10:16 +00:00
Author
Owner
Copy link

Eight of the nine are done; the ninth is split off.

Done this round (#560, #561, #562)

  • Unbounded reads. The four deck sidecars now read behind a cap, and — the sharper half — what was not read may not be replaced either. The CVE bulk archive had the same hole one door earlier than reported: not only the nested zip extraction was unbounded, the download itself was too. Chart data turned out to be capped already; that part of the issue text was stale.
  • network_sink_guard_test for Process.start and WebViewController. The reasoning was already written down at disk_traces.dart ("a second subprocess here must raise the alarm") — but semgrep is not in make check, so that alarm existed nowhere.
  • sanitize_svg is an allow-list. The objection recorded in that file was real, and it dissolved once the list stopped being guesswork: flutter_svg is the only reader this SVG ever gets, and its parser names its own vocabulary. The list is never stricter than the reader. It matters that this was checked — x1/y1/x2/y2 were missing from the first draft, which turned every <line> into a point.
  • audienceBoundary generic. The measurement in the previous comment held: 208 raw Deck parameters is noise. Keying on output gives thirteen. The gate now finds them itself and refuses to pass until each is classified — audience surface, or deliberately source-faithful. It does not decide which, because a .ocideck package must carry the source verbatim and a PDF must not, and no static rule derives that difference. That surfaced twelve output channels nobody had ever classified. None of them a leak; twelve places where the next builder had to guess.
  • DocumentSignature. Decided: no Ed25519/PKI alongside it. The reason is sovereignty, not cryptography — a checkable signature needs a trust anchor, and that anchor lands in the critical path. Recorded in SECURITY_DESIGN §12 with the conditions under which it would be revisited.

PBKDF2-SHA1 stands as recorded in the previous comment: it is a deviation in assurance/ASVS-5.0.0-afwijkingen.md, fixed by the WinZip-AES specification, and if it is resolved it is resolved there by leaving that format — not re-decided in a hardening pass.

Split off: the RFC 3161 nonce echo (#563). The CMS and chain verification that the same item asked about stays unbuilt, with the reasoning that is already recorded and still holds.

One consequence of this round is filed separately: a skipped sidecar is currently invisible to the user (#564).

Eight of the nine are done; the ninth is split off. **Done this round** (#560, #561, #562) - **Unbounded reads.** The four deck sidecars now read behind a cap, and — the sharper half — what was not read may not be replaced either. The CVE bulk archive had the same hole one door earlier than reported: not only the nested zip extraction was unbounded, the download itself was too. Chart data turned out to be capped already; that part of the issue text was stale. - **`network_sink_guard_test` for `Process.start` and `WebViewController`.** The reasoning was already written down at `disk_traces.dart` ("a second subprocess here must raise the alarm") — but semgrep is not in `make check`, so that alarm existed nowhere. - **`sanitize_svg` is an allow-list.** The objection recorded in that file was real, and it dissolved once the list stopped being guesswork: `flutter_svg` is the only reader this SVG ever gets, and its parser names its own vocabulary. The list is never stricter than the reader. It matters that this was checked — `x1/y1/x2/y2` were missing from the first draft, which turned every `<line>` into a point. - **`audienceBoundary` generic.** The measurement in the previous comment held: 208 raw Deck parameters is noise. Keying on *output* gives thirteen. The gate now finds them itself and refuses to pass until each is classified — audience surface, or deliberately source-faithful. It does not decide which, because a `.ocideck` package must carry the source verbatim and a PDF must not, and no static rule derives that difference. That surfaced twelve output channels nobody had ever classified. None of them a leak; twelve places where the next builder had to guess. - **`DocumentSignature`.** Decided: no Ed25519/PKI alongside it. The reason is sovereignty, not cryptography — a checkable signature needs a trust anchor, and that anchor lands in the critical path. Recorded in SECURITY_DESIGN §12 with the conditions under which it would be revisited. **PBKDF2-SHA1** stands as recorded in the previous comment: it is a deviation in `assurance/ASVS-5.0.0-afwijkingen.md`, fixed by the WinZip-AES specification, and if it is resolved it is resolved there by leaving that format — not re-decided in a hardening pass. **Split off:** the RFC 3161 nonce echo (#563). The CMS and chain verification that the same item asked about stays unbuilt, with the reasoning that is already recorded and still holds. One consequence of this round is filed separately: a skipped sidecar is currently invisible to the user (#564).
brenno commented 2026-07-22 15:10:27 +00:00
Author
Owner
Copy link

All nine are now resolved: seven built, one decided against (DocumentSignature), one split off (#563). Closing.

All nine are now resolved: seven built, one decided against (`DocumentSignature`), one split off (#563). Closing.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
codex/markdown-editor-translations
fix/markdown-editor-gate-followup-1123
feat/native-calls-f2-xmpp
codex/markdown-editor-ux
codex/template-followup
cockpit-wip
v0.1.3
v0.1.2
v0.1.1
v0.1.0
archive/git-mirror
Labels
Clear labels   
accepted
Assessed and will be built

  
bug
Something is broken

  
declined
Assessed and will not be built

  
docs
Documentation only

  
duplicate
Already tracked elsewhere

  
enhancement
New feature or improvement request

  
good first issue
Small, self-contained, and a good place to start

  
in-progress
Someone is working on this right now. Do not start it in parallel.

  
needs-info
Waiting for the reporter

  
privacy
Personal data can leak. Handled before anything else.

  
security
A weakness someone could use. Handled before anything else.

  
triage
Awaiting assessment

No labels
accepted
bug
declined
docs
duplicate
enhancement
good first issue
in-progress
needs-info
privacy
security
triage
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
LibreKAT/Ocideck#516
No description provided.