Page MenuHomePhabricator

CreateRedirect extension vulnerable to CSRF
Closed, ResolvedPublic

Description

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.

Event Timeline

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]"
 	],
-			$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. :)

Legoktm assigned this task to ashley.
Legoktm changed the visibility from "Custom Policy" to "Public (No Login Required)".

Change 389178 merged by jenkins-bot:
[mediawiki/extensions/CreateRedirect@master] [SECURITY] Proper edit token handling to protect against cross-site request forgeries (CSRF)

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