diff --git a/CHANGELOG.md b/CHANGELOG.md index 81bef4b..3886fd9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,21 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Security +- **Added an XML entity-expansion ("billion laughs") guard.** `ET.parse()` + call sites that read XML from GP-conversion output / uploaded imports had + no defense against a crafted `` chain that could exhaust + memory/CPU on the request thread parsing it (classic XXE itself isn't + reachable — stdlib `expat` doesn't resolve external entities by default). + Added `_reject_entity_declarations`/`_safe_parse_xml_file` (same pattern + as `feedBack-plugin-musicxml-import`'s guard) and applied it at all three + `ET.parse()` call sites that read files from disk, and directly on the + raw upload in `parse-goplayalong-sync` — the most directly user-controlled + XML surface, since `goplayalong.py`'s `defusedxml` preference silently + falls back to the unguarded stdlib parser when `defusedxml` isn't + installed (it isn't, here). `_safe_parse_xml_file` parses the exact bytes + it validated (via `io.BytesIO`) rather than reopening the path, closing a + TOCTOU gap where a file swapped in between the two steps could have + bypassed the guard. - **Restricted `youtube-audio` to YouTube hostnames.** The route handed a caller-supplied URL straight to `yt_dlp`, which falls back to its generic extractor for anything it doesn't recognize — that extractor can fetch diff --git a/plugin.json b/plugin.json index 9bae9dc..f85b808 100644 --- a/plugin.json +++ b/plugin.json @@ -1,7 +1,7 @@ { "id": "editor", "name": "Arrangement Editor", - "version": "1.8.3", + "version": "1.8.4", "description": "Build and edit feedpak arrangements — import Guitar Pro tabs, sync audio, and tune every note.", "category": "creation", "icon": "assets/thumb.png", diff --git a/routes.py b/routes.py index 17f64a9..c1faf58 100644 --- a/routes.py +++ b/routes.py @@ -1,6 +1,7 @@ """Arrangement Editor plugin — backend routes.""" import asyncio +import io import json import logging import math @@ -17,6 +18,62 @@ from xml.dom import minidom +# ── XML entity-expansion ("billion laughs") DoS guard ─────────────────── +# +# stdlib expat doesn't resolve external entities by default, so classic XXE +# isn't reachable here, but a crafted chain (billion-laughs / +# quadratic blowup) can still exhaust memory/CPU on the request thread that +# parses it. Every ET.parse() call site in this file reads XML from files +# this plugin didn't author itself (GP-conversion output, an uploaded +# loose-folder import, or similar) — same threat and same fix as +# feedBack-plugin-musicxml-import's _reject_entity_declarations guard. +# +# Real GP/RS arrangement XML never declares a custom entity, so rejecting +# any file containing " with 0 → shift = offset # Summing them, shift = offset + startBeat, is correct for both # without inspecting which kind of file it is. - start_beat = 0.0 - for _xf in sorted(Path(tmp).glob("*.xml")): - try: - _root = ET.parse(_xf).getroot() - except Exception: - continue - if _root.tag != "song": - continue - _arr = _root.find("arrangement") - if _arr is None or not _arr.text or _arr.text.strip().lower() in ( - "vocals", "showlights", "jvocals"): - continue - _sb = _root.find("startBeat") - if _sb is not None and _sb.text: + def _find_start_beat(): + # Blocking file I/O + XML parsing — run off the event loop, + # same as the _load() call above. + for _xf in sorted(Path(tmp).glob("*.xml")): try: - start_beat = float(_sb.text) - except ValueError: - start_beat = 0.0 - break + _root = _safe_parse_xml_file(_xf).getroot() + except Exception: + continue + if _root.tag != "song": + continue + _arr = _root.find("arrangement") + if _arr is None or not _arr.text or _arr.text.strip().lower() in ( + "vocals", "showlights", "jvocals"): + continue + _sb = _root.find("startBeat") + if _sb is not None and _sb.text: + try: + return float(_sb.text) + except ValueError: + return 0.0 + return 0.0 + return 0.0 + + start_beat = await asyncio.get_event_loop().run_in_executor(None, _find_start_beat) shift = song.offset + start_beat if shift: _apply_chart_offset(song, shift) diff --git a/tests/test_entity_guard.py b/tests/test_entity_guard.py new file mode 100644 index 0000000..6280cb3 --- /dev/null +++ b/tests/test_entity_guard.py @@ -0,0 +1,115 @@ +"""Regression tests for the XML entity-expansion ("billion laughs") guard +(issue #15). + +routes._reject_entity_declarations() must block + + +]> +&b; +''' + +_LEGIT_DOC = ''' +Test SongLead +''' + + +@pytest.mark.parametrize( + 'encoding', ['utf-8', 'utf-16', 'utf-16-le', 'utf-16-be', 'utf-32', 'utf-32-le', 'utf-32-be'] +) +def test_entity_declaration_rejected_across_encodings(encoding): + xml_bytes = _ENTITY_BOMB.encode(encoding) + with pytest.raises(ValueError, match='entity'): + _reject_entity_declarations(xml_bytes) + + +def test_utf16_entity_bypasses_raw_ascii_byte_scan_but_is_still_caught(): + """The raw ASCII byte regex alone must NOT catch this (regression guard + for the bypass itself), while the full guard function must.""" + xml_bytes = _ENTITY_BOMB.encode('utf-16') + assert _ENTITY_DECL_RE.search(xml_bytes) is None + with pytest.raises(ValueError): + _reject_entity_declarations(xml_bytes) + + +@pytest.mark.parametrize( + 'encoding', ['utf-8', 'utf-16', 'utf-16-le', 'utf-16-be', 'utf-32', 'utf-32-le', 'utf-32-be'] +) +def test_legitimate_document_not_rejected_across_encodings(encoding): + xml_bytes = _LEGIT_DOC.encode(encoding) + _reject_entity_declarations(xml_bytes) # must not raise + + +def test_safe_parse_xml_file_rejects_entity_bomb_before_expat_parses_it(tmp_path): + bomb_path = tmp_path / "bomb.xml" + bomb_path.write_bytes(_ENTITY_BOMB.encode('utf-16')) + with pytest.raises(ValueError, match='entity'): + _safe_parse_xml_file(bomb_path) + + +def test_safe_parse_xml_file_parses_a_legitimate_file(tmp_path): + good_path = tmp_path / "song.xml" + good_path.write_bytes(_LEGIT_DOC.encode('utf-8')) + root = _safe_parse_xml_file(good_path).getroot() + assert root.tag == "song" + assert root.find("arrangement").text == "Lead" + + +def test_safe_parse_xml_file_parses_the_validated_bytes_not_a_reopened_path(tmp_path): + # TOCTOU regression guard: parsing must use the exact bytes already + # read and validated (via io.BytesIO), never reopen `path` a second + # time -- reopening would let a file swapped in between the two steps + # bypass the guard entirely. + good_path = tmp_path / "song.xml" + good_path.write_bytes(_LEGIT_DOC.encode('utf-8')) + real_parse = routes.ET.parse + calls = [] + + def _spy_parse(source, *args, **kwargs): + calls.append(source) + return real_parse(source, *args, **kwargs) + + with patch.object(routes.ET, "parse", side_effect=_spy_parse): + _safe_parse_xml_file(good_path) + + assert len(calls) == 1 + assert isinstance(calls[0], io.BytesIO), ( + "ET.parse must be called with the already-validated bytes, not the path" + ) + + +# The GoPlayAlong sync-upload route (parse-goplayalong-sync) hands raw +# user-uploaded bytes straight to goplayalong.py's ET.fromstring() calls, +# whose defusedxml preference silently falls back to the vulnerable stdlib +# parser when defusedxml isn't installed (it isn't, here) -- the most +# directly user-controlled XML surface in this plugin. The route now calls +# _reject_entity_declarations(raw) before either goplayalong call; this +# documents that a -shaped entity bomb is caught by the exact same +# guard function that route wires in. +_GOPLAYALONG_ENTITY_BOMB = ''' + + +]> + +''' + + +def test_goplayalong_shaped_entity_bomb_is_rejected(): + with pytest.raises(ValueError, match='entity'): + _reject_entity_declarations(_GOPLAYALONG_ENTITY_BOMB.encode('utf-8'))