fix: dicht drie gaten in de SVG-sanitizer (SMIL, CSS, puntkomma-lijsten) #547

Merged

brenno merged 1 commit from fix/svg-allowlist into main 2026-07-22 13:18:41 +00:00

Conversation 0 Commits 1 Files changed 2 +102 -1

brenno commented 2026-07-22 13:18:40 +00:00

Owner

Copy link

Summary

Fourth item from #516: the SVG sanitiser's deny-list. Three holes closed; the allow-list deliberately not attempted yet, with the reason recorded in the file.

The three holes, and what they have in common

Each one is the same shape: the list was complete for the attack somebody had in mind, and blind to the mechanism next door.

SMIL could install an event handler that did not exist at parse time. <set attributeName="onload" to="alert(1)"/> writes the attribute when the animation runs, so the name.startsWith('on') check on the parent never saw anything — at parse time the parent carries no on… attribute at all. All four animation elements now go. Mermaid animates with CSS, not SMIL, so they earn nothing here; filtering on attributeName would have left a narrower version of the same trust.

<style> carried CSS, and CSS carries URLs. Nothing in this file parses that dialect, so a url(javascript:…) or an @import inside a stylesheet travelled straight past a check that only ever looked at attributes.

The URL check read only the start of the whole value. SMIL and CSS both use semicolon-separated lists, so values="a;javascript:alert(1)" was read as safe because its first entry was.

Why this is still a deny-list

An allow-list is the right shape — it refuses what it does not recognise instead of hoping the list of dangers is complete, and these three holes are the argument for it.

What it needs first is the actual element and attribute vocabulary Mermaid emits, per diagram type. That is not available here: the tests use hand-written SVG, not real Mermaid output. An allow-list built on intuition renders the first sequence diagram half-way with no error at all — and silent loss is a worse trade than a known gap.

The route is written into the file: collect real output per diagram type through the actual renderer, derive the lists from that, and make a rejected element log a warning rather than vanish quietly.

Verification

Four new tests, one per hole plus the animation family. Mutation-checked: reverting the three fixes makes three of the twelve fail.

make check green.

Part of #516; five items left there, of which audienceBoundary and the signing questions are design work rather than fixes.

## Summary Fourth item from #516: the SVG sanitiser's deny-list. Three holes closed; the allow-list deliberately not attempted yet, with the reason recorded in the file. ## The three holes, and what they have in common Each one is the same shape: the list was complete for the attack somebody had in mind, and blind to the mechanism next door. **SMIL could install an event handler that did not exist at parse time.** `<set attributeName="onload" to="alert(1)"/>` writes the attribute when the animation runs, so the `name.startsWith('on')` check on the parent never saw anything — at parse time the parent carries no `on…` attribute at all. All four animation elements now go. Mermaid animates with CSS, not SMIL, so they earn nothing here; filtering on `attributeName` would have left a narrower version of the same trust. **`<style>` carried CSS, and CSS carries URLs.** Nothing in this file parses that dialect, so a `url(javascript:…)` or an `@import` inside a stylesheet travelled straight past a check that only ever looked at attributes. **The URL check read only the start of the whole value.** SMIL and CSS both use semicolon-separated lists, so `values="a;javascript:alert(1)"` was read as safe because its first entry was. ## Why this is still a deny-list An allow-list is the right shape — it refuses what it does not recognise instead of hoping the list of dangers is complete, and these three holes are the argument for it. What it needs first is the actual element and attribute vocabulary Mermaid emits, per diagram type. That is not available here: **the tests use hand-written SVG, not real Mermaid output.** An allow-list built on intuition renders the first sequence diagram half-way with no error at all — and silent loss is a worse trade than a known gap. The route is written into the file: collect real output per diagram type through the actual renderer, derive the lists from that, and make a rejected element log a warning rather than vanish quietly. ## Verification Four new tests, one per hole plus the animation family. **Mutation-checked:** reverting the three fixes makes three of the twelve fail. `make check` green. Part of #516; five items left there, of which `audienceBoundary` and the signing questions are design work rather than fixes.

brenno added 1 commit 2026-07-22 13:18:40 +00:00

fix(beveiliging): dicht drie gaten in de SVG-sanitizer ... bc297b19d8

Alle drie hetzelfde patroon: de deny-list was volledig voor het
aanvalstype waar iemand aan dacht, en blind voor het mechanisme ernaast.

SMIL kon een event-handler installeren die er bij het parsen nog niet
stond: <set attributeName="onload" to="..."/> schrijft het attribuut pas
bij het afspelen, dus geen enkele attribuutcontrole op de ouder zag het.
Alle vier de animatie-elementen gaan weg — Mermaid animeert met CSS, niet
met SMIL, dus ze verdienen hier niets.

<style> droeg CSS, en CSS draagt URL's. Niets hier leest die taal, dus een
url(javascript:) of een @import reisde langs een controle die alleen naar
attributen keek.

En de URL-controle keek naar het begin van de héle waarde, terwijl SMIL
en CSS puntkomma-lijsten gebruiken: een onschuldige eerste entry dekte de
tweede af.

Bewust nog géén allow-list, met de reden in het bestand: die vraagt de
woordenschat die Mermaid werkelijk uitstuurt, en de tests hier gebruiken
handgeschreven SVG. Een allow-list op gevoel laat het eerste
sequentiediagram half renderen zonder foutmelding, en stil verlies is
erger dan een bekend gat.

Getoetst met een mutatie: de drie reparaties terugdraaien laat drie van de
twaalf tests vallen.

Onderdeel van #516.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

brenno merged commit ced043d930 into main 2026-07-22 13:18:41 +00:00

brenno deleted branch fix/svg-allowlist 2026-07-22 13:18:41 +00:00

brenno referenced this pull request from a commit 2026-07-22 13:18:43 +00:00

Merge pull request 'fix: dicht drie gaten in de SVG-sanitizer (SMIL, CSS, puntkomma-lijsten)' (#547) from fix/svg-allowlist into main

Sign in to join this conversation.

Reviewers

No reviewers

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

Subscribe

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!547

No description provided.