Page MenuHomePhabricator

Nimbus skin: XSS via the "Advertise" link interface messages
Closed, ResolvedPublicSecurity

Description

When [[MediaWiki:Nimbus-advertise-url]] exists and is not disabled (i.e. its value is not -), an "Advertise" link ([[MediaWiki:Nimbus-advertise]]) will show up in the skin's footer, and the target of this link is the URL configured in [[MediaWiki:Nimbus-advertise-url]].
Unfortunately both messages are currently vulnerable to the easiest possible XSS you can think of: "><script>alert('XSS')</script>

Here's a quick, tested patch to fix that:

@@ -76,10 +104,10 @@ class SkinNimbus extends SkinTemplate {
         */
        function advertiseLink() {
                $link = '';
-               $adMsg = wfMessage( 'nimbus-advertise-url' )->inContentLanguage();
-               if ( !$adMsg->isDisabled() ) {
+               $adMsg = $this->msg( 'nimbus-advertise-url' )->inContentLanguage();
+               if ( !$adMsg->isDisabled() && filter_var( $adMsg->text(), FILTER_VALIDATE_URL ) ) {
                        $link = '<a href="' . $adMsg->text() . '" rel="nofollow">' .
-                               wfMessage( 'nimbus-advertise' )->plain() . '</a>';
+                               $this->msg( 'nimbus-advertise' )->escaped() . '</a>';
                }
                return $link;
        }

(The RequestContext-ification is not related to the security aspect, but I figured I might as well do that while I'm editing this portion of the code. Also, line numbers etc. are probably off, given that my local copy of Nimbus has a lot of uncommitted changes.)

Event Timeline

$adMsg should probably also be ->escaped() as well for the href. FILTER_VALIDATE_URL allows urls like https://example.com/"><script>alert(1)</script>. Additionally, if you assume the message is not trusted, you can have javascript scheme urls to get xss e.g. javascript://%0aalert(1) passes FILTER_VALIDATE_URL.

You could potentially use $url = Sanitizer::validateAttributes( [ 'href' => $adMsg->text() ] )['href'] ?? false; and then htmlspecialchars the result if its not false (I did not test that).

$adMsg should probably also be ->escaped() as well for the href. FILTER_VALIDATE_URL allows urls like https://example.com/"><script>alert(1)</script>. Additionally, if you assume the message is not trusted, you can have javascript scheme urls to get xss e.g. javascript://%0aalert(1) passes FILTER_VALIDATE_URL.

Thank you for this, I learned something new! 👍

You could potentially use $url = Sanitizer::validateAttributes( [ 'href' => $adMsg->text() ] )['href'] ?? false; and then htmlspecialchars the result if its not false (I did not test that).

This seems to have done the trick for good, though I had to pass in [ 'href' ] as a 2nd param to the validateAttributes method because it apparently uses a whitelist-based approach, so you have to tell it explicitly what attributes are OK and what are not.

Here's the current patch that I have, would definitely appreciate your thoughts on it! 😃

@@ -76,10 +104,13 @@ class SkinNimbus extends SkinTemplate {
         */
        function advertiseLink() {
                $link = '';
-               $adMsg = wfMessage( 'nimbus-advertise-url' )->inContentLanguage();
+               $adMsg = $this->msg( 'nimbus-advertise-url' )->inContentLanguage();
                if ( !$adMsg->isDisabled() ) {
-                       $link = '<a href="' . $adMsg->text() . '" rel="nofollow">' .
-                               wfMessage( 'nimbus-advertise' )->plain() . '</a>';
+                       $url = Sanitizer::validateAttributes( [ 'href' => $adMsg->text() ], [ 'href' ] )['href'] ?? false;
+                       if ( $url ) {
+                               $link = '<a href="' . htmlspecialchars( $url, ENT_QUOTES ) . '" rel="nofollow">' .
+                                       $this->msg( 'nimbus-advertise' )->escaped() . '</a>';
+                       }
                }
                return $link;
        }

Many thanks for your help with this, @Bawolff, I really appreciate it! Not only did we get the XSS fixed properly, I learned a thing or two while doing it. 😉

Legoktm changed the visibility from "Custom Policy" to "Public (No Login Required)".Apr 27 2022, 6:26 PM
Legoktm changed the edit policy from "Custom Policy" to "All Users".
sbassett added a subscriber: sbassett.

Thanks, all. Tracking for the next supplemental security release: T305209.