Page MenuHomePhabricator
Create Task
Maniphest T241735

Challenge extension: classic CSRF
Closed, ResolvedPublic
Actions

Description

Challenge is at this point 14 years old, and 2006 code standards were a lot different in general...

Regardless, on Special:ChallengeView/<challenge ID> for a completed challenge involving yourself, the rating form isn't even a <form> and IIRC the submission is done via JS. While the JS POSTs to Special:ChallengeAction, no anti-CSRF token is present on the page and thus no such token gets POSTed and even if it were, it's not like Special:ChallengeAction cares about any other parameters than action. The proper modern-day solution for this would be to kill off Special:ChallengeAction altogether in favor of a properly written API module.

Special:ChallengeUser, which actually does contain a real <form> element, is similarly vulnerable to similar CSRF attack as no token is present nor is any token validation done. For that special page, the fix is roughly two lines in two different files: the hidden edit token <input> is to be added to /extensions/Challenge/includes/templates/ChallengeUser.template.php and the special page in /extensions/Challenge/includes/specials/SpecialChallengeUser.php should check for $user->matchEditToken( $request->getVal( 'wpEditToken' ) ) in addition to $request->wasPosted().

cc @lcawte

Details

SubjectRepoBranchLines +/-
[SECURITY] Add token checks to counter classic CSRFmediawiki/extensions/Challengemaster+40 -15
Customize query in gerrit

Related Objects

Event Timeline

ashley created this task.Jan 2 2020, 2:59 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 2 2020, 2:59 PM
Reedy added a project: Vuln-CSRF.Jan 2 2020, 3:18 PM
sbassett moved this task from Backlog / Other to External (Non-WMF) Issues on the acl*security board.Jan 2 2020, 5:23 PM
sbassett triaged this task as High priority.Jan 6 2020, 4:41 PM
sbassett subscribed.

The Security-Team would set the issue itself (csrf) as a high priority, though given that this isn't a wmf-deployed or bundled extension, we can't prioritize this within any of our queues. Feel free to let us know if there are any further questions or concerns here.

Aklapper added a project: Challenge.Jan 6 2020, 5:18 PM

@ashley: Assuming this task is about Challenge, hence adding project tag so others can find this task when searching for tasks under that project or looking at that project workboard (if they also have access to Security tasks).

Restricted Application added a project: Social-Tools. · View Herald TranscriptJan 6 2020, 5:18 PM
ashley claimed this task.Jan 6 2020, 10:59 PM

@Aklapper: Indeed, thanks! Tags aren't editable (at least for me) on the "New Security Issue" form when reporting the issue, hence why I try to #mention them in the initial report instead. Seems that after creating the task, they are editable, though -- didn't know that until now!

Regardless, here's a quick and dirty patch that should nevertheless get the job done and has been slightly tested locally. Code review would be lovely, but let's be real, Challenge is essentially an unfinished extension that doesn't have any production users so it's not a super high priority thing for anyone (not even me). I'll try to push & merge this patch in a week or so if no obvious concerns are raised.

diff --git a/includes/specials/SpecialChallengeAction.php b/includes/specials/SpecialChallengeAction.php
index 4df206e..8765f44 100644
--- a/includes/specials/SpecialChallengeAction.php
+++ b/includes/specials/SpecialChallengeAction.php
@@ -12,7 +12,10 @@ class ChallengeAction extends UnlistedSpecialPage {
 	 * @param mixed $par Parameter passed to the page or null
 	 */
 	public function execute( $par ) {
+		$out = $this->getOutput();
 		$request = $this->getRequest();
+		$user = $this->getUser();
+
 		$c = new Challenge();
 		$action = $request->getVal( 'action' );
 
@@ -20,7 +23,13 @@ class ChallengeAction extends UnlistedSpecialPage {
 			// This page isn't supposed to be accessed directly, but who knows
 			// what the users will do, so show an informative message in case
 			// if some poor soul ends up here directly (bug T152890)
-			$this->getOutput()->addWikiMsg( 'challengeaction-go-away' );
+			$out->addWikiMsg( 'challengeaction-go-away' );
+			return;
+		}
+
+		// status code 2 means "challenge removed by admin" which uses a different anti-CSRF token
+		if ( !$user->matchEditToken( $request->getVal( 'wpEditToken' ) ) && $request->getInt( 'status' ) !== 2 ) {
+			$out->addWikiMsg( 'sessionfailure' );
 			return;
 		}
 
@@ -32,13 +41,16 @@ class ChallengeAction extends UnlistedSpecialPage {
 				);
 				break;
 			case 2:
-				//if ( $this->getUser()->isAllowed( 'challengeadmin' ) ) {
+				if (
+					$user->isAllowed( 'challengeadmin' ) &&
+					$user->matchEditToken( $request->getVal( 'wpAdminToken' ) )
+				) {
 					$c->updateChallengeWinner(
 						$request->getInt( 'id' ),
 						$request->getInt( 'actorid' )
 					);
 					$c->updateChallengeStatus( $request->getInt( 'id' ), 3 );
-				//}
+				}
 				break;
 			case 3:
 				// Update social stats for both users involved in challenge
@@ -59,7 +71,7 @@ class ChallengeAction extends UnlistedSpecialPage {
 					'challenge_rate',
 					[
 						'challenge_id' => $request->getVal( 'id' ),
-						'challenge_rate_submitter_actor' => $this->getUser()->getActorId(),
+						'challenge_rate_submitter_actor' => $user->getActorId(),
 						'challenge_rate_actor' => $request->getInt( 'loser_actorid' ),
 						'challenge_rate_date' => $dbw->timestamp(),
 						'challenge_rate_score' => $request->getVal( 'challenge_rate' ),
@@ -70,6 +82,6 @@ class ChallengeAction extends UnlistedSpecialPage {
 				break;
 		}
 
-		$this->getOutput()->setArticleBodyOnly( true );
+		$out->setArticleBodyOnly( true );
 	}
 }
\ No newline at end of file
diff --git a/includes/specials/SpecialChallengeUser.php b/includes/specials/SpecialChallengeUser.php
index 6ff6e00..3214912 100644
--- a/includes/specials/SpecialChallengeUser.php
+++ b/includes/specials/SpecialChallengeUser.php
@@ -72,7 +72,11 @@ class ChallengeUser extends SpecialPage {
 			$output->setPageTitle( $this->msg( 'challengeuser-error-page-title' ) );
 			$output->addHTML( $this->msg( 'challengeuser-nouser' )->plain() );
 		} else {
-			if ( $request->wasPosted() && $_SESSION['alreadysubmitted'] === false ) {
+			if (
+				$request->wasPosted() &&
+				$user->matchEditToken( $request->getVal( 'wpEditToken' ) ) &&
+				$_SESSION['alreadysubmitted'] === false
+			) {
 				$_SESSION['alreadysubmitted'] = true;
 				$c = new Challenge();
 				$c->addChallenge(
diff --git a/includes/specials/SpecialChallengeView.php b/includes/specials/SpecialChallengeView.php
index f13e7d4..38b1297 100644
--- a/includes/specials/SpecialChallengeView.php
+++ b/includes/specials/SpecialChallengeView.php
@@ -63,7 +63,7 @@ class ChallengeView extends SpecialPage {
 		$out = '';
 		switch ( $challenge['status'] ) {
 			case 0:
-				if ( $this->getUser()->getActorId() != $challenge['challengee_actor'] ) {
+				if ( $u->getActorId() != $challenge['challengee_actor'] ) {
 					$out .= $this->msg( 'challengeview-acceptance' )->plain();
 				} else {
 					$out .= $this->msg( 'challengeview-sent-to-you' )->plain();
@@ -77,14 +77,15 @@ class ChallengeView extends SpecialPage {
 					<input type=\"hidden\" id=\"challenge_id\" value=\"{$challenge['id']}\" />
 					<input type=\"button\" class=\"challenge-response-button site-button\" value=\"" . $this->msg( 'challengeview-submit-button' )->plain() .
 						'" />';
+					$out .= Html::hidden( 'wpEditToken', $u->getEditToken() );
 				}
 				break;
 			case 1:
 			case 2: // treat "counter terms" as "in progress" b/c that's basically what it is
 				if (
-					!$this->getUser()->isAllowed( 'challengeadmin' ) ||
-					$challenge['challenger_actor'] == $this->getUser()->getActorId() ||
-					$challenge['challengee_actor'] == $this->getUser()->getActorId()
+					!$u->isAllowed( 'challengeadmin' ) ||
+					$challenge['challenger_actor'] == $u->getActorId() ||
+					$challenge['challengee_actor'] == $u->getActorId()
 				)
 				{
 					$out .= $this->msg( 'challengeview-inprogress' )->plain();
@@ -105,6 +106,7 @@ class ChallengeView extends SpecialPage {
 					<input type=\"button\" class=\"challenge-approval-button site-button\" value=\"" .
 						$this->msg( 'challengeview-submit-button' )->plain() .
 					'" />';
+					$out .= Html::hidden( 'wpEditToken', $u->getEditToken() );
 				}
 				break;
 			case -1:
@@ -132,7 +134,7 @@ class ChallengeView extends SpecialPage {
 							<br />';
 						$out .= $this->msg( 'challengeview-comment', $challenge['rating_comment'] )->parse();
 					} else {
-						if ( $this->getUser()->getActorId() == $challenge['winner_actor'] ) {
+						if ( $u->getActorId() == $challenge['winner_actor'] ) {
 							$out .= '<span class="challenge-title">';
 							$out .= $this->msg( 'challengeview-rating' )->plain();
 							$out .= '</span><br />
@@ -162,6 +164,7 @@ class ChallengeView extends SpecialPage {
 								<input type="button" class="challenge-rate-button site-button" value="' .
 									$this->msg( 'challengeview-submit-button' )->plain() .
 									'" />';
+							$out .= Html::hidden( 'wpEditToken', $u->getEditToken() );
 						} else {
 							$out .= $this->msg( 'challengeview-notyetrated' )->plain();
 						}
diff --git a/includes/templates/ChallengeUser.template.php b/includes/templates/ChallengeUser.template.php
index b3c84a1..a08fb30 100644
--- a/includes/templates/ChallengeUser.template.php
+++ b/includes/templates/ChallengeUser.template.php
@@ -91,6 +91,7 @@ class ChallengeUserTemplate extends QuickTemplate {
 			<input type="button" class="createbox challenge-send-button site-button" value="<?php echo $this->data['class']->msg( 'challengeuser-submit-button' )->plain() ?>" size="20" />
 		</div>
 		<div class="visualClear"></div>
+		<input type="hidden" name="wpEditToken" value="<?php echo $this->data['class']->getUser()->getEditToken() ?>" />
 	</form>
 <?php
 	} // execute()
diff --git a/includes/templates/ChallengeView.template.php b/includes/templates/ChallengeView.template.php
index d3807a5..9745776 100644
--- a/includes/templates/ChallengeView.template.php
+++ b/includes/templates/ChallengeView.template.php
@@ -110,6 +110,7 @@ class ChallengeViewTemplate extends QuickTemplate {
 		$adminLink = "<a class=\"challenge-admin-cancel-link\" data-challenge-id=\"{$challenge['id']}\" href=\"#\">";
 		$adminLink .= wfMessage( 'challengeview-admin' )->plain();
 		$adminLink .= '</a>';
+		$adminLink .= Html::hidden( 'wpAdminToken', $user->getEditToken() );
 		echo $adminLink;
 	}
 ?>
diff --git a/resources/js/Challenge.js b/resources/js/Challenge.js
index 7145d46..785d5ad 100644
--- a/resources/js/Challenge.js
+++ b/resources/js/Challenge.js
@@ -42,7 +42,8 @@ var Challenge = {
 				title: 'Special:ChallengeAction',
 				action: 1,
 				'id': id,
-				status: -2
+				status: -2,
+				wpAdminToken: $( 'input[name="wpAdminToken"]' ).val()
 			}
 		} ).done( function() {
 			var text = mw.msg( 'challenge-js-challenge-removed' );
@@ -60,7 +61,8 @@ var Challenge = {
 				title: 'Special:ChallengeAction',
 				action: 1,
 				'id': $( '#challenge_id' ).val(),
-				status: $( '#challenge_action' ).val()
+				status: $( '#challenge_action' ).val(),
+				wpEditToken: $( 'input[name="wpEditToken"]' ).val()
 			}
 		} ).done( function() {
 			var newStatus;
@@ -89,7 +91,8 @@ var Challenge = {
 				title: 'Special:ChallengeAction',
 				action: 2,
 				'id': $( '#challenge_id' ).val(),
-				actorid: $( '#challenge_winner_actorid' ).val()
+				actorid: $( '#challenge_winner_actorid' ).val(),
+				wpEditToken: $( 'input[name="wpEditToken"]' ).val()
 			}
 		} ).done( function() {
 			$( '#challenge-status' ).text( mw.msg( 'challenge-js-winner-recorded' ) ).show( 500 );
@@ -107,7 +110,8 @@ var Challenge = {
 				'id': $( '#challenge_id' ).val(),
 				challenge_rate: $( '#challenge_rate' ).val(),
 				rate_comment: $( '#rate_comment' ).val(),
-				loser_actorid: $( '#loser_actorid' ).val()
+				loser_actorid: $( '#loser_actorid' ).val(),
+				wpEditToken: $( 'input[name="wpEditToken"]' ).val()
 			}
 		} ).done( function() {
 			$( '#challenge-status' ).text( mw.msg( 'challenge-js-rating-submitted' ) ).show( 500 );
Legoktm closed this task as Resolved.Jan 13 2020, 8:32 PM
Legoktm changed the visibility from "Custom Policy" to "Public (No Login Required)".
gerritbot added a comment.Jan 13 2020, 8:36 PM

Change 563757 merged by jenkins-bot:
[mediawiki/extensions/Challenge@master] [SECURITY] Add token checks to counter classic CSRF

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

ashley mentioned this in T242689: SocialProfile: classic CSRF (no token check) in various special pages which perform write actions.Jan 13 2020, 11:47 PM
• chasemp added a project: Security.Feb 10 2020, 10:53 PM
• chasemp removed a project: acl*security.Feb 20 2020, 8:14 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL