Page MenuHomePhabricator

$i(s) in username being treated as a regex backreference capture group during linkMarker replacement
Closed, DuplicatePublicSecurity

Description

Steps to replicate the issue (include links if applicable):

  • Rollback an edit by a user with $i in their username (e.g. RAMSES$44932)

What happens?:
The edit summary shows Reverted edits by RAMSES932 (talk) to last version ...

What should have happened instead?:
The edit summary shows Reverted edits by RAMSES$44932 (talk) to last version ...

Other information (browser name/version, screenshots, etc.):

  • Ref. the title of this task, that's only a guess at what's going on, I've not done any digging — it stands to reason that system message parameters probably only go up to $99 so only the $44 part of the username was removed?
  • Also present on the beta cluster so it's not en.wiki specific.

Event Timeline

TheresNoTime changed the visibility from "Public (No Login Required)" to "Custom Policy".
TheresNoTime changed the subtype of this task from "Bug Report" to "Security Issue".

This is feeling a tiny bit sensitive now

Some local testing with a user named UserWith$1and$2and$3and$99and$100 results in edit summaries of:

  • Rollback: Reverted edits by UserWithSpecial:Contributions/UserWith$1and$2and$3and$99and$100|UserWith$1and$2and$3and$99and$100andandandand0 (talk) to last revision by Admin
  • Undo: Undo revision 90 by UserWithSpecial:Contributions/UserWith$1and$2and$3and$99and$100|UserWith$1and$2and$3and$99and$100andandandand0 (talk)

image.png (335×1 px, 104 KB)


The wikitext [[Special:Contributions/UserWith$1and$2and$3and$99and$100|UserWith$1and$2and$3and$99and$100]] renders correctly on a previewed/saved page (UserWith$1and$2and$3and$99and$100), but when rendered in an edit summary (both a saved and previewed edit summary) it displays as UserWithSpecial:Contributions/UserWith$1and$2and$3and$99and$100|UserWith$1and$2and$3and$99and$100andandandand0

TheresNoTime renamed this task from $i in username being treated as system message parameters in rollback edit summary to $i(s) in username being treated as system message parameters in edit summary.Aug 4 2022, 11:33 PM
sbassett changed Author Affiliation from N/A to WMF Product.Aug 5 2022, 1:50 PM
sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".
sbassett changed Risk Rating from N/A to Low.
sbassett edited projects, added SecTeam-Processed; removed Security-Team.
sbassett removed a project: Security.

It's not a system message parameter issue — the preg_replace during the linkMarker replacement is directly parsing the $i (i.e. $1) as a backreference and re-inserting the capture group content. There's only one capture group in the regex (\[\[(.*?)\]\]), so only $1 causes this garbled return, whereas $2+ is just blank.


With the preg_replace in CommentParser.php#422:

$comment = preg_replace(
    $linkRegexp,
    $linkMarker,
    $comment,
    1
);

where

  • $linkRegexp = \[\[(.*?)\]\]
  • $linkMarker = <a href="/index.php/Special:Contributions/User$1name" title="Special:Contributions/User$1name">User$1name</a>
  • $comment = [[Special:Contributions/User$1name|User$1name]] (

the following is returned

<a href="/index.php/Special:Contributions/UserSpecial:Contributions/User$1name|User$1namename" title="Special:Contributions/UserSpecial:Contributions/User$1name|User$1namename">UserSpecial:Contributions/User$1name|User$1namename</a> (

We need to ensure any instances of $i in the $linkMarker (CommentParser.php#414) have been escaped — to test this, I added the following preg_replace:

diff --git a/includes/CommentFormatter/CommentParser.php b/includes/CommentFormatter/CommentParser.php
index 9814956510..7521aec5df 100644
--- a/includes/CommentFormatter/CommentParser.php
+++ b/includes/CommentFormatter/CommentParser.php
@@ -412,6 +412,7 @@ class CommentParser {

    $linkMarker = $this->addPageLink( $target, $linkText . $inside, $wikiId );
    $linkMarker .= $trail;
+   $linkMarker = preg_replace( '/\$(?=\d+)/i', '\\\$', $linkMarker );
    } catch ( MalformedTitleException $e ) {
        // Fall through
    }

which results in $linkMarker = <a href="/index.php/Special:Contributions/User\$1name" title="Special:Contributions/User\$1name">User\$1name</a> and the following (correctly) being returned:

<a href="/index.php/Special:Contributions/User$1name" title="Special:Contributions/User$1name">User$1name</a> (

I'm sure using a preg_replace here is less than optimal (perhaps there is already something in Sanitizer that'll do it?)

TheresNoTime renamed this task from $i(s) in username being treated as system message parameters in edit summary to $i(s) in username being treated as a regex backreference capture group during linkMarker replacement.Aug 5 2022, 3:37 PM
TheresNoTime updated the task description. (Show Details)

Change 820774 had a related patch set uploaded (by Samtar; author: Samtar):

[mediawiki/core@master] CommentParser: replace $ with \$ in usernames

https://gerrit.wikimedia.org/r/820774

Change 820774 abandoned by Samtar:

[mediawiki/core@master] CommentParser: replace $ with \$ in usernames

Reason:

https://gerrit.wikimedia.org/r/820774

Change 820836 had a related patch set uploaded (by Samtar; author: Samtar):

[mediawiki/core@master] CommentParser: escapeRegexReplacement on linkMarker

https://gerrit.wikimedia.org/r/820836

Nice investigation. I think you're looking for StringUtils::escapeRegexReplacement(): https://gerrit.wikimedia.org/g/mediawiki/core/+/73188b46aa6a6bf79c4e9ec703e79701c656a242/includes/libs/StringUtils.php#314

Thanks! Admittedly I didn't look for a dedicated function ^^

PleaseStand subscribed.

This bug appears to have been originally fixed in r11323 (69e2b7946c764901) and reintroduced during refactoring in f7f84dddb33ee4bd.

@TheresNoTime Would it make sense to remove the WIP status in the Gerrit patch?

Change 820836 abandoned by Samtar:

[mediawiki/core@master] CommentParser: escapeRegexReplacement on linkMarker

Reason:

https://gerrit.wikimedia.org/r/820836