Top-level menu entries from MediaWiki:Sidebar are not properly escaped in the Metrolook skin, resulting in classic stored XSS. The output of Sanitizer::escapeIdForAttribute is not HTML-safe, but the skin incorrectly assumes it is.
Minimal test case:
- As an (interface) administrator or other similarly privileged (editinterface) user, create a top level menu entry "named" "><script>alert('XSS')</script> in MediaWiki:Sidebar
- Load a page with the Metrolook skin
Expected result:
No alert.
Actual result:
The alertruns.
Proposed & tested patch:
diff --git a/MetrolookTemplate.php b/MetrolookTemplate.php index 533b557..f4a3119 100644 --- a/MetrolookTemplate.php +++ b/MetrolookTemplate.php @@ -596,12 +596,12 @@ class MetrolookTemplate extends BaseTemplate { $labelId = Sanitizer::escapeIdForAttribute( "p-$name-label" ); ?> <div class="portal" role="navigation" id='<?php - echo Sanitizer::escapeIdForAttribute( "p-$name" ) + echo htmlspecialchars( Sanitizer::escapeIdForAttribute( "p-$name" ), ENT_QUOTES ) ?>'<?php // If it isn't escaped, phan complains; if it _is_ escaped, phan still complains :^) // @phan-suppress-next-line SecurityCheck-DoubleEscaped echo htmlspecialchars( Linker::tooltip( 'p-' . $name ), ENT_QUOTES ) - ?> aria-labelledby='<?php echo $labelId ?>'> + ?> aria-labelledby='<?php echo htmlspecialchars( $labelId, ENT_QUOTES ) ?>'> <h5<?php $this->html( 'userlangattributes' ) ?> id='<?php echo $labelId ?>'><?php echo htmlspecialchars( $msgObj->exists() ? $msgObj->text() : $msg ); ?></h5>
Notes about the patch:
- I tested it against edb0029fe4cbd8211c2be1d6a67b27425ad41e34 since it's the last 1.39-compatible version of the skin; it doesn't matter really since the bug is still present in master
- Only the first htmlspecialchars call added in the patch is needed to fix the security bug here; the second one merely prevents some garbage HTML being output. As you can see if you test it out, the alert regardless initially, prior to applying the patch, runs only once, not twice.