Skip to content

Commit 5a12f9b

Browse files
committed
Security fixes: XSS detection, SVG XXE, Zip Slip, media attribute
Closes the output-escaping / upload tier of the 2026-04 advisory batch: - GHSA-9695-8fr9-hw5q: Security::detectXss `on_events` regex no longer requires quotes/whitespace around `=`, so unquoted handlers like `<img onerror=alert(1)>` are flagged. Same fix removes the regex-bypass half of GHSA-c2q3 and GHSA-w8cg. - GHSA-w8cg-7jcj-4vv2 / GHSA-c2q3-p4jr-c55f: added `svg`, `math`, `option`, `select` to default `security.xss_dangerous_tags` — XML-namespace tags that allow inline scripting and the select-context tags used to escape admin form templates. - GHSA-r7fx-8g49-7hhr: `MediaObjectTrait::attribute()` now gates the attribute name through a strict identifier regex + denylist (on*, style, xmlns, srcdoc, formaction). Markdown like `![alt](img.gif?attribute=onload,alert(1))` no longer paints an event handler onto the rendered <img>. Themes can still call `$image->attribute('src', $signed_url)` from PHP. - GHSA-3446-6mgw-f79p: `VectorImageMedium` strips DOCTYPE/ENTITY declarations and parses with LIBXML_NONET (+ libxml_disable_entity_loader shim on PHP < 8) so an attacker-uploaded SVG can no longer exfiltrate files via XXE or DoS via billion-laughs while Grav reads its width/height. Companion hardening for sanitization paths lives in rhukster/dom-sanitizer. - GHSA-w48r-jppp-rcfw (path layer): `Installer::unZip` pre-validates every archive entry and refuses Zip Slip primitives (`..` segments, absolute paths, Windows drive letters, NUL bytes). New `Installer::isSafeArchiveEntry` helper. Defending against a well-formed but malicious plugin whose PHP is itself the payload remains a "trust the source" problem when using directInstall. Four new test files (DetectXssTest, MediaAttributeSecurityTest, SvgXxeSecurityTest, ZipSlipSecurityTest) plus updated entries in existing CleanDangerousTwigTest. Full unit suite: 721 tests, 2560 assertions.
1 parent c66dfeb commit 5a12f9b

10 files changed

Lines changed: 584 additions & 6 deletions

File tree

CHANGELOG.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,11 @@
1515
* [security] Hardened the new-user uniqueness guard in `UserObject::save` (GHSA-rr73-568v-28f8): `strpos(..., '@@')` replaced with `str_contains` (the old form was falsy when the transient-key marker was at position 0, silently bypassing the check) and the check now runs for every `FlexStorageInterface` backend rather than only `FileStorage`. A low-privileged user with `admin.users.create` can no longer disrupt a super-admin account by submitting the admin's username through the "add user" form.
1616
* [security] Added HMAC integrity to `Framework\Cache\Adapter\FileCache` (GHSA-gwfr-jfjf-92vv): every cache payload is signed with `Security::getNonceKey()` on write and verified on read; tampered, attacker-planted, or pre-upgrade files are treated as cache misses and removed instead of being unserialized. The on-disk format is now versioned (`v2\n<expires>\n<key>\n<hmac>\n<serialized>`); existing caches rebuild transparently on first read.
1717
* [security] Closed GHSA-vj3m-2g9h-vm4p (5-part advisory): (#1) `JobQueue` now HMAC-signs the `serialized_job` blob — a tampered queue file can no longer smuggle a forged `Job` for direct RCE via `Job::exec → call_user_func_array`; legitimate items still execute via the structured-fields fallback. (#3) `Session::getFlashObject` now wraps its payload in a versioned HMAC envelope, refusing to unserialize legacy/forged values. (#4) `InstallCommand` git-clone arguments (`branch`, `url`, `path` from `user/.dependencies`) now go through `escapeshellarg`, with a `--` separator before url/path to block option-injection. (#5) `twig_array_reduce` (plus `twig_array_some`/`twig_array_every`) added to `cleanDangerousTwig`'s callable blocklist alongside the existing `twig_array_map`/`filter`. (#2) was the same FileCache issue addressed by GHSA-gwfr above.
18+
* [security] Tightened `Security::detectXss` `on_events` regex (GHSA-9695-8fr9-hw5q): the previous form required quotes/whitespace around the `=` sign and was bypassed by `<img onerror=alert(1)>`. Same fix knocks down the regex-bypass half of GHSA-c2q3-p4jr-c55f and GHSA-w8cg-7jcj-4vv2.
19+
* [security] Added `svg`, `math`, `option`, `select` to default `security.xss_dangerous_tags` (GHSA-w8cg-7jcj-4vv2, GHSA-c2q3-p4jr-c55f) — XML-namespace tags that allow inline scripting plus the select-context break used to escape admin form templates.
20+
* [security] `MediaObjectTrait::attribute()` now gates the attribute name through a strict identifier regex + denylist (`on*`, `style`, `xmlns`, `srcdoc`, `formaction`) so a Markdown image like `![alt](img.gif?attribute=onload,alert(1))` no longer injects an event handler onto the rendered tag (GHSA-r7fx-8g49-7hhr).
21+
* [security] Hardened SVG dimension reading in `VectorImageMedium` against XXE / billion-laughs (GHSA-3446-6mgw-f79p): DOCTYPE/ENTITY declarations are stripped before parse and `simplexml_load_string` is now called with `LIBXML_NONET` (and `libxml_disable_entity_loader` for PHP < 8). The companion fix lives in rhukster/dom-sanitizer for SVG sanitization paths.
22+
* [security] `Installer::unZip` now pre-validates every archive entry and refuses Zip Slip primitives — `..` segments, absolute paths, Windows drive letters, NUL bytes (GHSA-w48r-jppp-rcfw). Note: this hardens the path layer only; a well-formed but malicious plugin whose own PHP code is the payload remains a "trust the source" problem when using directInstall.
1823

1924
# v2.0.0-beta.1
2025
## 04/16/2026

system/config/security.yaml

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,16 @@ xss_dangerous_tags:
3232
- title
3333
- base
3434
- isindex
35+
# GHSA-w8cg-7jcj-4vv2: svg/math allow inline scripting via XML namespaces
36+
# and event-handler attributes; flag them as dangerous in user-editable
37+
# content so the on_events regex bypass (now fixed for GHSA-9695) has
38+
# belt-and-suspenders coverage.
39+
- svg
40+
- math
41+
# GHSA-c2q3-p4jr-c55f: option/select are used by attackers to break out
42+
# of the admin select-template context.
43+
- option
44+
- select
3545
uploads_dangerous_extensions:
3646
- php
3747
- php2

system/src/Grav/Common/GPM/Installer.php

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,24 @@ public static function unZip($zip_file, $destination)
179179
$archive = $zip->open($zip_file);
180180

181181
if ($archive === true) {
182+
// GHSA-w48r-jppp-rcfw: validate every entry name before extraction.
183+
// ZipArchive::extractTo would otherwise honour `../` segments and
184+
// absolute paths, letting a crafted plugin/theme ZIP write files
185+
// anywhere the web server can reach (Zip Slip, CVE-2018-1000544
186+
// family). Note: this hardens the path layer; it does NOT and
187+
// cannot defend against a well-formed but malicious plugin whose
188+
// own PHP code is the payload — that's a "trust the source"
189+
// problem the admin must own when using directInstall.
190+
$numFiles = $zip->numFiles;
191+
for ($i = 0; $i < $numFiles; $i++) {
192+
$entryName = (string) $zip->getNameIndex($i);
193+
if (!self::isSafeArchiveEntry($entryName)) {
194+
self::$error = self::ZIP_EXTRACT_ERROR;
195+
$zip->close();
196+
return false;
197+
}
198+
}
199+
182200
Folder::create($destination);
183201

184202
$unzip = $zip->extractTo($destination);
@@ -207,6 +225,35 @@ public static function unZip($zip_file, $destination)
207225
return false;
208226
}
209227

228+
/**
229+
* Reject Zip Slip primitives in archive entry names: empty names, NUL
230+
* bytes, absolute paths, or any path segment that is `..`. Forward and
231+
* back slashes are both treated as separators so Windows-authored
232+
* archives are also covered.
233+
*
234+
* @internal Public for testing.
235+
*/
236+
public static function isSafeArchiveEntry(string $name): bool
237+
{
238+
if ($name === '' || str_contains($name, "\0")) {
239+
return false;
240+
}
241+
if (str_starts_with($name, '/') || str_starts_with($name, '\\')) {
242+
return false;
243+
}
244+
// Windows drive letter: C:\..., D:/...
245+
if (preg_match('#^[A-Za-z]:[/\\\\]#', $name) === 1) {
246+
return false;
247+
}
248+
// Any `..` path segment, regardless of slash flavour.
249+
foreach (preg_split('#[\\\\/]+#', $name) as $segment) {
250+
if ($segment === '..') {
251+
return false;
252+
}
253+
}
254+
return true;
255+
}
256+
210257
/**
211258
* Instantiates and returns the package installer class
212259
*

system/src/Grav/Common/Media/Traits/MediaObjectTrait.php

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -325,18 +325,51 @@ public function reset()
325325
/**
326326
* Add custom attribute to medium.
327327
*
328+
* Reachable from Markdown via `?attribute=name,value` on image excerpts, so
329+
* the attribute NAME is editor-controlled. We restrict it to plain HTML
330+
* attribute identifiers (alphanumerics + `-`/`:`/`_`) and reject any name
331+
* that would inject script when rendered onto an `<img>` tag — event
332+
* handlers (`on*`), inline style, the XML namespace, srcdoc, and the
333+
* various `form*` attributes whose URL targets are themselves trusted as
334+
* actions. GHSA-r7fx-8g49-7hhr.
335+
*
328336
* @param string $attribute
329337
* @param string $value
330338
* @return $this
331339
*/
332340
public function attribute($attribute = null, $value = '')
333341
{
334-
if (!empty($attribute)) {
335-
$this->attributes[$attribute] = $value;
342+
if (empty($attribute) || !is_string($attribute)) {
343+
return $this;
336344
}
345+
if (!self::isSafeAttributeName($attribute)) {
346+
return $this;
347+
}
348+
$this->attributes[$attribute] = $value;
337349
return $this;
338350
}
339351

352+
/** @internal */
353+
private static function isSafeAttributeName(string $name): bool
354+
{
355+
// Strict shape: HTML attribute names are letter-led, then alnum/-/_/:/./$
356+
// Anything else (whitespace, quotes, `<>`, etc.) is rejected outright.
357+
if (!preg_match('/^[A-Za-z][A-Za-z0-9_:.\-]*$/', $name)) {
358+
return false;
359+
}
360+
$lower = strtolower($name);
361+
// Event handlers — primary GHSA-r7fx-8g49-7hhr vector.
362+
if (str_starts_with($lower, 'on')) {
363+
return false;
364+
}
365+
// Attribute names that open a scripting context regardless of value.
366+
// We deliberately do NOT deny `src` / `href` here — themes legitimately
367+
// call `$image->attribute('src', $signed_url)` from PHP, and the
368+
// primary script-injection surface is the event-handler family above.
369+
$denylist = ['style', 'xmlns', 'srcdoc', 'formaction'];
370+
return !in_array($lower, $denylist, true);
371+
}
372+
340373
/**
341374
* Switch display mode.
342375
*

system/src/Grav/Common/Page/Medium/VectorImageMedium.php

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,24 @@ public function __construct($items = [], ?Blueprint $blueprint = null)
4646
return;
4747
}
4848

49-
$xml = simplexml_load_string(file_get_contents($path));
49+
// GHSA-3446-6mgw-f79p: strip DOCTYPE/ENTITY declarations and pass
50+
// LIBXML_NONET to prevent XXE / billion-laughs / network exfiltration
51+
// when reading width/height from an attacker-supplied SVG.
52+
$svg = (string) file_get_contents($path);
53+
$svg = preg_replace('/<!DOCTYPE\b[^>]*(?:\[[^\]]*\])?[^>]*>/is', '', $svg) ?? $svg;
54+
$svg = preg_replace('/<!ENTITY\b[^>]*>/i', '', $svg) ?? $svg;
55+
56+
$previousEntityLoader = null;
57+
if (\PHP_VERSION_ID < 80000 && function_exists('libxml_disable_entity_loader')) {
58+
$previousEntityLoader = libxml_disable_entity_loader(true);
59+
}
60+
try {
61+
$xml = simplexml_load_string($svg, 'SimpleXMLElement', LIBXML_NONET | LIBXML_NOERROR | LIBXML_NOWARNING);
62+
} finally {
63+
if ($previousEntityLoader !== null) {
64+
libxml_disable_entity_loader($previousEntityLoader);
65+
}
66+
}
5067
$attr = $xml ? $xml->attributes() : null;
5168
if (!$attr instanceof \SimpleXMLElement) {
5269
return;

system/src/Grav/Common/Security.php

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -229,9 +229,16 @@ public static function detectXss($string, ?array $options = null): ?string
229229

230230
// Set the patterns we'll test against
231231
$patterns = [
232-
// Match any attribute starting with "on" or xmlns (must be preceded by whitespace/special chars)
233-
// Allow optional whitespace between 'on' and event name to catch obfuscation attempts
234-
'on_events' => '#(<[^>]+[\s\x00-\x20\"\'\/])(on\s*[a-z]+|xmlns)\s*=[\s|\'\"].*[\s|\'\"]>#iUu',
232+
// Match any attribute starting with "on" or xmlns (must be preceded by an
233+
// attribute boundary: whitespace, NUL, quote or slash). We deliberately
234+
// do NOT try to match the attribute value itself — the previous regex
235+
// required quotes-or-spaces around the `=` sign and was bypassed by
236+
// unquoted handlers like `<img src=x onerror=alert(1)>`
237+
// (GHSA-9695-8fr9-hw5q, also exploited by GHSA-c2q3-p4jr-c55f and
238+
// GHSA-w8cg-7jcj-4vv2). Detecting the attribute name + `=` is enough
239+
// for a tripwire; trade-off is occasional false positives when an
240+
// unrelated `on*=` substring appears inside another attribute's value.
241+
'on_events' => '#<[^>]*?[\s\x00-\x20\"\'\/](on\s*[a-z]+|xmlns)\s*=#iu',
235242

236243
// Match javascript:, livescript:, vbscript:, mocha:, feed: and data: protocols
237244
'invalid_protocols' => '#(' . implode('|', array_map('preg_quote', $invalid_protocols, ['#'])) . ')(:|\&\#58)\S.*?#iUu',
Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
<?php
2+
3+
use Codeception\Util\Fixtures;
4+
use Grav\Common\Grav;
5+
use Grav\Common\Security;
6+
7+
/**
8+
* Class DetectXssTest
9+
*
10+
* Tests for Security::detectXss() — specifically the on_events regex hardening
11+
* for GHSA-9695-8fr9-hw5q (unquoted event handlers), with parallel coverage
12+
* for the same bypass pattern called out in GHSA-c2q3-p4jr-c55f and
13+
* GHSA-w8cg-7jcj-4vv2.
14+
*
15+
* Naming convention: test{Method}_{GHSA_ID}_{description}
16+
*/
17+
class DetectXssTest extends \PHPUnit\Framework\TestCase
18+
{
19+
/** @var Grav */
20+
protected $grav;
21+
22+
protected function setUp(): void
23+
{
24+
parent::setUp();
25+
$grav = Fixtures::get('grav');
26+
$this->grav = $grav();
27+
}
28+
29+
// =========================================================================
30+
// GHSA-9695-8fr9-hw5q: unquoted on* handlers must be detected
31+
// =========================================================================
32+
33+
/**
34+
* @dataProvider providerGHSA9695_UnquotedOnEvents
35+
*/
36+
public function testDetectXss_GHSA9695_FlagsUnquotedEventHandler(string $payload, string $description): void
37+
{
38+
$result = Security::detectXss($payload);
39+
self::assertSame('on_events', $result, "Should flag on_events for: $description");
40+
}
41+
42+
public static function providerGHSA9695_UnquotedOnEvents(): array
43+
{
44+
return [
45+
['<img src=x onerror=alert(1)>', 'advisory PoC: unquoted onerror, no space before >'],
46+
['<img src=x onerror=eval(atob(/Y/.source))>', 'advisory PoC: atob/regex.source obfuscation'],
47+
['<svg onload=alert(1)>', 'unquoted onload on svg'],
48+
['<body onload=alert(1)>', 'unquoted onload on body'],
49+
['<a href=# onclick=alert(1)>x</a>', 'unquoted onclick'],
50+
// GHSA-c2q3-p4jr-c55f payload — the exact taxonomy escape sequence:
51+
['</option></select><img src=x onerror=alert(1)>', 'GHSA-c2q3 select-context break + unquoted onerror'],
52+
// Obfuscation: whitespace inside the event name (e.g. on error=)
53+
['<img src=x on error=alert(1)>', 'whitespace between on and event name'],
54+
// xmlns is also covered by the same rule — keep regression coverage:
55+
['<svg xmlns=http://example.com/ns>', 'unquoted xmlns'],
56+
];
57+
}
58+
59+
/**
60+
* @dataProvider providerGHSA9695_QuotedOnEvents
61+
*/
62+
public function testDetectXss_GHSA9695_StillFlagsQuotedEventHandlersAfterFix(string $payload, string $description): void
63+
{
64+
// Make sure tightening the regex didn't regress the previously-working
65+
// quoted forms.
66+
$result = Security::detectXss($payload);
67+
self::assertSame('on_events', $result, "Should still flag quoted on_events for: $description");
68+
}
69+
70+
public static function providerGHSA9695_QuotedOnEvents(): array
71+
{
72+
return [
73+
['<img src="x" onerror="alert(1)">', 'double-quoted onerror'],
74+
["<img src='x' onerror='alert(1)'>", 'single-quoted onerror'],
75+
['<body onload="document.location=\'evil\'">', 'quoted onload'],
76+
['<svg onload="fetch(\'/x\')">', 'svg with quoted onload'],
77+
];
78+
}
79+
80+
// =========================================================================
81+
// Negative coverage: legitimate content should not trip on_events
82+
// =========================================================================
83+
84+
/**
85+
* @dataProvider providerSafeContent
86+
*/
87+
public function testDetectXss_SafeContentReturnsNullOnEventsRule(string $payload, string $description): void
88+
{
89+
// Some safe content may still trip OTHER rules (e.g. the dangerous_tags
90+
// list), but the on_events rule specifically should not fire.
91+
$result = Security::detectXss($payload);
92+
self::assertNotSame('on_events', $result, "on_events must not fire for: $description");
93+
}
94+
95+
public static function providerSafeContent(): array
96+
{
97+
return [
98+
['<p>Hello world</p>', 'plain paragraph'],
99+
['<a href="https://example.com">link</a>', 'link with href'],
100+
['<img src="/foo.png" alt="bar">', 'plain img'],
101+
['Pricing on demand', 'word starting with "on" outside any tag'],
102+
['<button>Click me</button>', 'button tag (ends in "on")'],
103+
['<section>content</section>', 'section tag'],
104+
];
105+
}
106+
107+
// =========================================================================
108+
// GHSA-w8cg-7jcj-4vv2: svg/math + GHSA-c2q3 option/select added to defaults
109+
// =========================================================================
110+
111+
/**
112+
* @dataProvider providerGHSAw8cg_NewlyDangerousTags
113+
*/
114+
public function testDetectXss_GHSAw8cg_FlagsNewlyDangerousTags(string $payload, string $description): void
115+
{
116+
$result = Security::detectXss($payload);
117+
// Either dangerous_tags (new) or on_events (already covered by #1) is
118+
// an acceptable trip — both indicate the payload is flagged.
119+
self::assertNotNull($result, "Should flag: $description");
120+
self::assertContains(
121+
$result,
122+
['dangerous_tags', 'on_events'],
123+
"Expected dangerous_tags or on_events for: $description, got '$result'"
124+
);
125+
}
126+
127+
public static function providerGHSAw8cg_NewlyDangerousTags(): array
128+
{
129+
return [
130+
['<svg><script>alert(1)</script></svg>', 'GHSA-w8cg svg with embedded script'],
131+
['<svg></svg>', 'svg tag alone'],
132+
['<math><mtext>x</mtext></math>', 'math tag (similar XML namespace risk)'],
133+
['</option></select>injected', 'GHSA-c2q3 option/select context break'],
134+
['<select><option>x</option></select>', 'option/select wrapping'],
135+
];
136+
}
137+
}

0 commit comments

Comments
 (0)