Skip to content

Commit 0afa95e

Browse files
authored
Merge commit from fork
[SECURITY] Prevent bypass via mixed-case SVG attributes
2 parents 5e47746 + 5a0a1ea commit 0afa95e

File tree

5 files changed

+62
-23
lines changed

5 files changed

+62
-23
lines changed

src/Sanitizer.php

Lines changed: 38 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -421,7 +421,7 @@ protected function cleanAttributesOnWhitelist(\DOMElement $element)
421421
* Such as xlink:href when the xlink namespace isn't imported.
422422
* We have to do this as the link is still ran in this case.
423423
*/
424-
if (false !== strpos($attrName, 'href')) {
424+
if (false !== stripos($attrName, 'href')) {
425425
$href = $element->getAttribute($attrName);
426426
if (false === $this->isHrefSafeValue($href)) {
427427
$element->removeAttribute($attrName);
@@ -453,14 +453,17 @@ protected function cleanAttributesOnWhitelist(\DOMElement $element)
453453
*/
454454
protected function cleanXlinkHrefs(\DOMElement $element)
455455
{
456-
$xlinks = $element->getAttributeNS('http://www.w3.org/1999/xlink', 'href');
457-
if (false === $this->isHrefSafeValue($xlinks)) {
458-
$element->removeAttributeNS( 'http://www.w3.org/1999/xlink', 'href' );
459-
$this->xmlIssues[] = array(
460-
'message' => 'Suspicious attribute \'href\'',
461-
'line' => $element->getLineNo(),
462-
);
456+
foreach ($element->attributes as $attribute) {
457+
// remove attributes with unexpected namespace prefix, e.g. `XLinK:href` (instead of `xlink:href`)
458+
if ($attribute->prefix === '' && strtolower($attribute->nodeName) === 'xlink:href') {
459+
$element->removeAttribute($attribute->nodeName);
460+
$this->xmlIssues[] = array(
461+
'message' => sprintf('Unexpected attribute \'%s\'', $attribute->nodeName),
462+
'line' => $element->getLineNo(),
463+
);
464+
}
463465
}
466+
$this->cleanHrefAttributes($element, 'xlink');
464467
}
465468

466469
/**
@@ -470,13 +473,33 @@ protected function cleanXlinkHrefs(\DOMElement $element)
470473
*/
471474
protected function cleanHrefs(\DOMElement $element)
472475
{
473-
$href = $element->getAttribute('href');
474-
if (false === $this->isHrefSafeValue($href)) {
475-
$element->removeAttribute('href');
476-
$this->xmlIssues[] = array(
477-
'message' => 'Suspicious attribute \'href\'',
478-
'line' => $element->getLineNo(),
479-
);
476+
$this->cleanHrefAttributes($element);
477+
}
478+
479+
protected function cleanHrefAttributes(\DOMElement $element, string $prefix = ''): void
480+
{
481+
$relevantAttributes = array_filter(
482+
iterator_to_array($element->attributes),
483+
static function (\DOMAttr $attr) use ($prefix) {
484+
return strtolower($attr->name) === 'href' && strtolower($attr->prefix) === $prefix;
485+
}
486+
);
487+
foreach ($relevantAttributes as $attribute) {
488+
if (!$this->isHrefSafeValue($attribute->value)) {
489+
$element->removeAttribute($attribute->nodeName);
490+
$this->xmlIssues[] = array(
491+
'message' => sprintf('Suspicious attribute \'%s\'', $attribute->nodeName),
492+
'line' => $element->getLineNo(),
493+
);
494+
continue;
495+
}
496+
// in case the attribute name is `HrEf`/`xlink:HrEf`, adjust it to `href`/`xlink:href`
497+
if (!in_array($attribute->nodeName, $this->allowedAttrs, true)
498+
&& in_array(strtolower($attribute->nodeName), $this->allowedAttrs, true)
499+
) {
500+
$element->removeAttribute($attribute->nodeName);
501+
$element->setAttribute(strtolower($attribute->nodeName), $attribute->value);
502+
}
480503
}
481504
}
482505

tests/data/hrefCleanOne.svg

Lines changed: 11 additions & 4 deletions
Loading

tests/data/hrefTestOne.svg

Lines changed: 11 additions & 4 deletions
Loading

tests/data/svgCleanOne.svg

Lines changed: 1 addition & 0 deletions
Loading

tests/data/svgTestOne.svg

Lines changed: 1 addition & 0 deletions
Loading

0 commit comments

Comments
 (0)