I was reviewing https://gerrit.wikimedia.org/r/373736 and noticed that the extension was already vulnerable to CSRF, as it constructs a form without any edit token. And then it bypasses EditPage's csrf check by just getting the edit token server-side.
Description
Description
Details
Details
Event Timeline
Comment Actions
Proposed and slightly tested patch, comments welcome:
diff --git a/CreateRedirect.body.php b/CreateRedirect.body.php index 650711b..1b27f01 100644 --- a/CreateRedirect.body.php +++ b/CreateRedirect.body.php @@ -50,7 +50,7 @@ class SpecialCreateRedirect extends SpecialPage { $this->setHeaders(); - if ( $request->wasPosted() ) { + if ( $request->wasPosted() && $user->matchEditToken( $request->getVal( 'wpEditToken' ) ) ) { // 1. Retrieve POST vars. First, we want "crOrigTitle", holding the // title of the page we're writing to, and "crRedirectTitle", // holding the title of the page we're redirecting to. @@ -73,7 +73,7 @@ class SpecialCreateRedirect extends SpecialPage { $wpTextbox1 = "#REDIRECT [[$crRedirectTitle]]\r\n"; // POST var "wpTextbox1" stores the content that's actually going to be written. This is where we write the #REDIRECT [[Article]] stuff. We plug in $crRedirectTitle here. $wpSave = 1; $wpMinoredit = 1; // TODO: Decide on this; should this really be marked and hardcoded as a minor edit, or not? Or should we provide an option? --Digi 11/4/07 - $wpEditToken = htmlspecialchars( $user->getEditToken() ); + $wpEditToken = htmlspecialchars( $request->getVal( 'wpEditToken' ) ); // 3. Put together the params that we'll use in "FauxRequest" into a single array. $crRequestParams = [ @@ -171,6 +171,10 @@ class SpecialCreateRedirect extends SpecialPage { $out->mRedirectCode = ''; // TODO: Implement error handling (i.e. "Edit conflict!" or "You don't have permissions to edit this page!") --Digi 11/4/07 + } elseif ( $request->wasPosted() && !$user->matchEditToken( $request->getVal( 'wpEditToken' ) ) ) { + // Possibly a CSRF attempt + $out->setPageTitle( $this->msg( 'sessionfailure-title' ) ); + $out->addWikiMsg( 'sessionfailure' ); } $action = htmlspecialchars( $this->getPageTitle()->getLocalURL() ); @@ -184,6 +188,10 @@ class SpecialCreateRedirect extends SpecialPage { $msgRedirectTo = $this->msg( 'createredirect-redirect-to' )->escaped(); $msgSave = $this->msg( 'createredirect-save' )->escaped(); + // Edit token + // @see https://phabricator.wikimedia.org/T178787 + $token = Html::hidden( 'wpEditToken', $user->getEditToken() ); + // 2. Start rendering the output! The output is entirely the form. // It's all HTML, and may be self-explanatory. $out->addHTML( $this->msg( 'createredirect-instructions' )->escaped() ); @@ -203,6 +211,7 @@ class SpecialCreateRedirect extends SpecialPage { <td><input type="submit" name="crWrite" id="crWrite" value="$msgSave" tabindex="4" /></td> </tr> </table> +{$token} </form> END ); diff --git a/extension.json b/extension.json index 8494e3a..b8d08c7 100644 --- a/extension.json +++ b/extension.json @@ -1,6 +1,6 @@ { "name": "CreateRedirect", - "version": "1.2.0", + "version": "1.3.0", "author": [ "[https://www.mediawiki.org/wiki/User:Digiku Marco Zafra]" ],
Comment Actions
- $wpEditToken = htmlspecialchars( $user->getEditToken() ); + $wpEditToken = htmlspecialchars( $request->getVal( 'wpEditToken' ) );
I know this was like this before, but the edit token should not be put through htmlspecialchars.
Otherwise this patch looks good. :)
Comment Actions
Change 389178 merged by jenkins-bot:
[mediawiki/extensions/CreateRedirect@master] [SECURITY] Proper edit token handling to protect against cross-site request forgeries (CSRF)