Repository navigation
fix: snapshot attributes in DOM.removeAttributes to avoid skipping them - #8189
Conversation
Iterating the live NamedNodeMap from elem.attributes while calling removeAttribute skips the attribute directly after a removed one, so a second dangerous attribute can survive DOM.sanitize. Snapshot the collection with Array.from before iterating.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8189 +/- ##
=======================================
Coverage 93.92% 93.92%
=======================================
Files 290 290
Lines 24916 24916
Branches 6575 6575
=======================================
Hits 23402 23402
Misses 1514 1514 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Why are you not using https://github.com/cure53/dompurify? |
adding a new dependency to the project is an architectural change instead of a simple one line patch which was much easier for the reviewer to ensure its correctness changing the default built-in sanitizer to replace with a new dependency is the choice of the project architect, as a researcher I simply shipped a patch to the exploit and added a new rule to the sanitizer |
|
Before hacking this code I tried to find a slim dependecy, but everything was very big in terms of bundle size. |
|
@HarelM but doesn't the size of https://github.com/cure53/dompurify reflect that robust security is a lot of work? It's been hardened over many years, with more edge cases covered than most people know about. |
|
True, I don't disagree. |
##### [v6.4.1](https://github.com/maplibre/maplibre-gl-js/blob/HEAD/CHANGELOG.md#641) ##### 🐞 Bug fixes - Fix `DOM.sanitize` leaving dangerous attributes behind when multiple consecutive attributes are present. Iterating the live `NamedNodeMap` from `elem.attributes` while calling `removeAttribute` skipped the attribute directly after a removed one, so a second dangerous attribute (for example an `ontoggle` on a `<details open>` element) could survive sanitisation and later execute ([#8189](maplibre/maplibre-gl-js#8189)) (by [@0xKirisame](https://github.com/0xKirisame)) - Give custom layers the live globe transition in `CustomRenderMethodInput.defaultProjectionData.projectionTransition`, which was hardcoded to 1 for the whole globe/mercator transition, so a custom layer jumped straight to the fully bent globe while every other layer eased ([#8169](maplibre/maplibre-gl-js#8169)) (by [@mondsichtung](https://github.com/mondsichtung)) ##### [v6.4.0](https://github.com/maplibre/maplibre-gl-js/blob/HEAD/CHANGELOG.md#640) ##### ✨ Features and improvements - Avoid a per-query `Array.sort()` in cross-tile symbol matching (`TileLayerIndex.findMatches`), claiming the lowest-index unclaimed candidate in a single pass instead; reduces main-thread symbol-placement cost on dense/coincident symbol layers ([#7797](maplibre/maplibre-gl-js#7797)) (by [@pholmstr](https://github.com/pholmstr)) - Use `texelFetch` for exact DEM and color-relief elevation stop lookups instead of normalized texture coordinate arithmetic ([#7640](maplibre/maplibre-gl-js#7640)) (by [@johncarmack1984](https://github.com/johncarmack1984)) - Make default draggable markers keyboard-focusable and movable with the arrow keys (1 px per press, 10 px with Shift); custom marker elements stay application-owned ([#8020](maplibre/maplibre-gl-js#8020)) (by [@smmariquit](https://github.com/smmariquit)) ##### 🐞 Bug fixes - Fix a permanent frame rate degradation after switching styles: every sprite reload marked its images as updated forever, making every in-view tile re-check and re-upload them on every frame. Also stop leaking the images of a replaced sprite, which were never removed from the image manager ([#8052](maplibre/maplibre-gl-js#8052)) (by [@HarelM](https://github.com/HarelM)) - Prevent a rejected missing style image resolver from blocking successfully resolved images in the same batch ([#8146](maplibre/maplibre-gl-js#8146)) (by [@birkskyum](https://github.com/birkskyum)) - Explicitly request no browser color management when decoding raster-DEM tiles so their RGB-encoded elevation values are not changed (what would otherwise happen with `gfx.color_management.mode = 1` in Firefox) ([#8125](maplibre/maplibre-gl-js#8125)) (by [@tnikkel](https://github.com/tnikkel)) - Let an abort reach an image or raster tile load that is still awaiting its `transformRequest`, so `ImageSource.updateImage` no longer loses the image it was just handed and an aborted tile is no longer fetched ([#8071](maplibre/maplibre-gl-js#8071)) (by [@mondsichtung](https://github.com/mondsichtung)) - Fix raster tiles fading in again when they are reloaded, briefly flashing the map background, most visibly when switching projection ([#8106](maplibre/maplibre-gl-js#8106)) (by [@mondsichtung](https://github.com/mondsichtung)) - Fix globe panning inverting and stalling near and across the poles by rotating the globe with a versor, keeping the drag direction consistent at every latitude. Panning also eases off as the cursor approaches the edge of the globe and continues past it, instead of stopping. The bearing is preserved while panning, as before ([#5296](maplibre/maplibre-gl-js#5296)) (by [@jcolot](https://github.com/jcolot)) - Fix `fill-extrusion-rounded-corner-distance` producing spikes: corner arcs now land on the integer tile grid, and corners created by tile clipping are left sharp ([#8153](maplibre/maplibre-gl-js#8153)) (by [@HarelM](https://github.com/HarelM)) - Fix a gesture which was held still before being released still flinging the map ([#1303](maplibre/maplibre-gl-js#1303)) (by [@zdila](https://github.com/zdila)) ##### [v6.3.0](https://github.com/maplibre/maplibre-gl-js/blob/HEAD/CHANGELOG.md#630) ##### ✨ Features and improvements - Make fired/listened map events typed. This means that `map.on("something", ...)` (and `once`, `listens`) will now give you an typescript error and better autocomplete. If you relied on firing/listening custom events via the map, this still works via the escape hatches `map.fire("something" as any)` -> `map.on("something" as any, ...)` ([#8072](maplibre/maplibre-gl-js#8072)) (by [@CommanderStorm](https://github.com/CommanderStorm)) - Let a `StyleImageInterface` give a `{renderWithWebGL}` callback as its `data`, an escape hatch for plugin developers and advanced users that renders a style image on the GPU instead of moving its pixels through the CPU. Nothing new is possible that pixels could not already express, but an image that changes often, such as an animated icon, gets more performant ([#7954](maplibre/maplibre-gl-js#7954)) (by [@lucaswoj](https://github.com/lucaswoj)) - Use integer vertex attributes for packed line data instead of float conversion ([#7640](maplibre/maplibre-gl-js#7640)) (by [@johncarmack1984](https://github.com/johncarmack1984)) - Use integer vertex attributes for packed circle, heatmap, symbol, and fill-extrusion data instead of float conversion ([#7640](maplibre/maplibre-gl-js#7640), [#8143](maplibre/maplibre-gl-js#8143)) (by [@johncarmack1984](https://github.com/johncarmack1984)) - Redesign benchmarks to use vitest bench capabilities and remove custom build for benchmarks code ([#982](maplibre/maplibre-gl-js#982)) (by [@johncarmack1984](https://github.com/johncarmack1984)) ##### 🐞 Bug fixes - Fix terrain pan/zoom gestures losing the grabbed terrain point: gestures are now solved against the elevation of the terrain under the gesture instead of the frozen center elevation, so terrain under the pointer/fingers no longer slips during moving-centroid pinches and drags ([#8067](maplibre/maplibre-gl-js#8067)) (by [@StrawberryJam22](https://github.com/StrawberryJam22)) - Fix `ImageSource`, `VideoSource` and `CanvasSource` leaking a GPU texture on every image update and on removal, and a resized texture losing its wrap and filter settings ([#8094](maplibre/maplibre-gl-js#8094)) (by [@mondsichtung](https://github.com/mondsichtung)) - Fix `map.queryRenderedFeatures()` sometimes causing "Out of bounds" error due to race condition while loading tile data ([#8064](maplibre/maplibre-gl-js#8064)) (by [@smvjohansenbouvet](https://github.com/smvjohansenbouvet)) - Fix zooming the globe with the scroll wheel or a two-finger pinch drifting away from the pointer while the globe is small on screen, instead of keeping the location under the pointer as it does when zoomed in ([#8095](maplibre/maplibre-gl-js#8095)) (by [@mondsichtung](https://github.com/mondsichtung)) - Fix projective rendering for non-parallelogram image source quads ([#7887](maplibre/maplibre-gl-js#7887)) (by [@i4innovationnet](https://github.com/i4innovationnet))
##### [v6.4.1](https://github.com/maplibre/maplibre-gl-js/blob/HEAD/CHANGELOG.md#641) ##### 🐞 Bug fixes - Fix `DOM.sanitize` leaving dangerous attributes behind when multiple consecutive attributes are present. Iterating the live `NamedNodeMap` from `elem.attributes` while calling `removeAttribute` skipped the attribute directly after a removed one, so a second dangerous attribute (for example an `ontoggle` on a `<details open>` element) could survive sanitisation and later execute ([#8189](maplibre/maplibre-gl-js#8189)) (by [@0xKirisame](https://github.com/0xKirisame)) - Give custom layers the live globe transition in `CustomRenderMethodInput.defaultProjectionData.projectionTransition`, which was hardcoded to 1 for the whole globe/mercator transition, so a custom layer jumped straight to the fully bent globe while every other layer eased ([#8169](maplibre/maplibre-gl-js#8169)) (by [@mondsichtung](https://github.com/mondsichtung)) ##### [v6.4.0](https://github.com/maplibre/maplibre-gl-js/blob/HEAD/CHANGELOG.md#640) ##### ✨ Features and improvements - Avoid a per-query `Array.sort()` in cross-tile symbol matching (`TileLayerIndex.findMatches`), claiming the lowest-index unclaimed candidate in a single pass instead; reduces main-thread symbol-placement cost on dense/coincident symbol layers ([#7797](maplibre/maplibre-gl-js#7797)) (by [@pholmstr](https://github.com/pholmstr)) - Use `texelFetch` for exact DEM and color-relief elevation stop lookups instead of normalized texture coordinate arithmetic ([#7640](maplibre/maplibre-gl-js#7640)) (by [@johncarmack1984](https://github.com/johncarmack1984)) - Make default draggable markers keyboard-focusable and movable with the arrow keys (1 px per press, 10 px with Shift); custom marker elements stay application-owned ([#8020](maplibre/maplibre-gl-js#8020)) (by [@smmariquit](https://github.com/smmariquit)) ##### 🐞 Bug fixes - Fix a permanent frame rate degradation after switching styles: every sprite reload marked its images as updated forever, making every in-view tile re-check and re-upload them on every frame. Also stop leaking the images of a replaced sprite, which were never removed from the image manager ([#8052](maplibre/maplibre-gl-js#8052)) (by [@HarelM](https://github.com/HarelM)) - Prevent a rejected missing style image resolver from blocking successfully resolved images in the same batch ([#8146](maplibre/maplibre-gl-js#8146)) (by [@birkskyum](https://github.com/birkskyum)) - Explicitly request no browser color management when decoding raster-DEM tiles so their RGB-encoded elevation values are not changed (what would otherwise happen with `gfx.color_management.mode = 1` in Firefox) ([#8125](maplibre/maplibre-gl-js#8125)) (by [@tnikkel](https://github.com/tnikkel)) - Let an abort reach an image or raster tile load that is still awaiting its `transformRequest`, so `ImageSource.updateImage` no longer loses the image it was just handed and an aborted tile is no longer fetched ([#8071](maplibre/maplibre-gl-js#8071)) (by [@mondsichtung](https://github.com/mondsichtung)) - Fix raster tiles fading in again when they are reloaded, briefly flashing the map background, most visibly when switching projection ([#8106](maplibre/maplibre-gl-js#8106)) (by [@mondsichtung](https://github.com/mondsichtung)) - Fix globe panning inverting and stalling near and across the poles by rotating the globe with a versor, keeping the drag direction consistent at every latitude. Panning also eases off as the cursor approaches the edge of the globe and continues past it, instead of stopping. The bearing is preserved while panning, as before ([#5296](maplibre/maplibre-gl-js#5296)) (by [@jcolot](https://github.com/jcolot)) - Fix `fill-extrusion-rounded-corner-distance` producing spikes: corner arcs now land on the integer tile grid, and corners created by tile clipping are left sharp ([#8153](maplibre/maplibre-gl-js#8153)) (by [@HarelM](https://github.com/HarelM)) - Fix a gesture which was held still before being released still flinging the map ([#1303](maplibre/maplibre-gl-js#1303)) (by [@zdila](https://github.com/zdila)) ##### [v6.3.0](https://github.com/maplibre/maplibre-gl-js/blob/HEAD/CHANGELOG.md#630) ##### ✨ Features and improvements - Make fired/listened map events typed. This means that `map.on("something", ...)` (and `once`, `listens`) will now give you an typescript error and better autocomplete. If you relied on firing/listening custom events via the map, this still works via the escape hatches `map.fire("something" as any)` -> `map.on("something" as any, ...)` ([#8072](maplibre/maplibre-gl-js#8072)) (by [@CommanderStorm](https://github.com/CommanderStorm)) - Let a `StyleImageInterface` give a `{renderWithWebGL}` callback as its `data`, an escape hatch for plugin developers and advanced users that renders a style image on the GPU instead of moving its pixels through the CPU. Nothing new is possible that pixels could not already express, but an image that changes often, such as an animated icon, gets more performant ([#7954](maplibre/maplibre-gl-js#7954)) (by [@lucaswoj](https://github.com/lucaswoj)) - Use integer vertex attributes for packed line data instead of float conversion ([#7640](maplibre/maplibre-gl-js#7640)) (by [@johncarmack1984](https://github.com/johncarmack1984)) - Use integer vertex attributes for packed circle, heatmap, symbol, and fill-extrusion data instead of float conversion ([#7640](maplibre/maplibre-gl-js#7640), [#8143](maplibre/maplibre-gl-js#8143)) (by [@johncarmack1984](https://github.com/johncarmack1984)) - Redesign benchmarks to use vitest bench capabilities and remove custom build for benchmarks code ([#982](maplibre/maplibre-gl-js#982)) (by [@johncarmack1984](https://github.com/johncarmack1984)) ##### 🐞 Bug fixes - Fix terrain pan/zoom gestures losing the grabbed terrain point: gestures are now solved against the elevation of the terrain under the gesture instead of the frozen center elevation, so terrain under the pointer/fingers no longer slips during moving-centroid pinches and drags ([#8067](maplibre/maplibre-gl-js#8067)) (by [@StrawberryJam22](https://github.com/StrawberryJam22)) - Fix `ImageSource`, `VideoSource` and `CanvasSource` leaking a GPU texture on every image update and on removal, and a resized texture losing its wrap and filter settings ([#8094](maplibre/maplibre-gl-js#8094)) (by [@mondsichtung](https://github.com/mondsichtung)) - Fix `map.queryRenderedFeatures()` sometimes causing "Out of bounds" error due to race condition while loading tile data ([#8064](maplibre/maplibre-gl-js#8064)) (by [@smvjohansenbouvet](https://github.com/smvjohansenbouvet)) - Fix zooming the globe with the scroll wheel or a two-finger pinch drifting away from the pointer while the globe is small on screen, instead of keeping the location under the pointer as it does when zoomed in ([#8095](maplibre/maplibre-gl-js#8095)) (by [@mondsichtung](https://github.com/mondsichtung)) - Fix projective rendering for non-parallelogram image source quads ([#7887](maplibre/maplibre-gl-js#7887)) (by [@i4innovationnet](https://github.com/i4innovationnet)) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
|
@0xKirisame @HarelM should we raise an CVE about this? |
|
I believe I did, didn't I? |
|
advisory != CVE -> GHSA-jrc7-96c5-q579 I requested it |
|
Ok. thanks. |
Launch Checklist
CHANGELOG.mdunder the## mainsection.What
Fixes a bug in
DOM.removeAttributeswhere iterating the liveNamedNodeMapfromelem.attributeswhile callingelem.removeAttribute(name)skips the attribute directly after a removed one. When two dangerous attributes are consecutive, the first is removed and the second survives into the sanitised output ofDOM.sanitize.This is exploitable in the attribution control: style-supplied attribution strings pass through
DOM.sanitizebefore being assigned toinnerHTML(src/ui/control/attribution_control.ts:177). A payload such as<details open onload="1" ontoggle="alert(1)">x</details>keepsontoggle, and thetoggleevent fires on insertion, so the handler executes with no user interaction.Why
The current unit tests in
src/util/dom.test.tsonly exercise a single dangerous attribute per element, which is why the skip was never caught.How
Array.from(elem.attributes)before iterating, so removals no longer affect iteration order.Out of scope
Found and verified with an autonomous logic-audit harness I built. All testing was against local builds of this library; no MapLibre-operated service was touched. Coordinated disclosure in progress with the security team, hence the separate report; happy to share details once the fix is released.