0001-SECURITY-Add-getLoginSecurityLevel-support-to-FormSp.patch
Anomie
Actions

Authored By

	Anomie
	May 9 2018, 7:24 PM

Size

6 KB

Referenced Files

None

Subscribers

None

0001-SECURITY-Add-getLoginSecurityLevel-support-to-FormSp.patch
View Options

	From 2acd2367c7117fd1b684f3e2e002757184140fde Mon Sep 17 00:00:00 2001
	From: Brad Jorsch <bjorsch@wikimedia.org>
	Date: Wed, 9 May 2018 14:53:32 -0400
	Subject: [PATCH 1/2] SECURITY: Add getLoginSecurityLevel() support to
	FormSpecialPage

	The base SpecialPage will handle reauthentication automatically if you
	just implement getLoginSecurityLevel() to return an appropriate string.

	But it doesn't work with FormSpecialPage, and if you try calling
	checkLoginSecurityLevel() manually it'll lose any post data if the
	reauth happens when the form is posted.

	So this patch has SpecialPage::checkLoginSecurityLevel() preserve post
	data across reauth (using logic similar to that in AuthManagerSpecialPage),
	and has FormSpecialPage call checkLoginSecurityLevel() in the same
	way the base SpecialPage does.

	It also fixes the SpecialPage logic to not call
	checkLoginSecurityLevel() when the special page doesn't implement
	getLoginSecurityLevel(), as was the originally-intended behavior.
	Apparently almost nothing actually gets to SpecialPage::execute() or
	this would probably have been noticed already.

	Change-Id: Ic89dc1b6583aaecd2efe3f5109896148a188c271
	---
	includes/specialpage/FormSpecialPage.php \| 43 +++++++++++++++++++-
	includes/specialpage/SpecialPage.php \| 50 ++++++++++++++++++++++--
	2 files changed, 87 insertions(+), 6 deletions(-)

	diff --git a/includes/specialpage/FormSpecialPage.php b/includes/specialpage/FormSpecialPage.php
	index 66c7d47ea9..230168f637 100644
	--- a/includes/specialpage/FormSpecialPage.php
	+++ b/includes/specialpage/FormSpecialPage.php
	@@ -35,6 +35,12 @@ abstract class FormSpecialPage extends SpecialPage {
	*/
	protected $par = null;

	+ /**
	+ * @var array\|null POST data preserved across re-authentication
	+ * @since 1.32
	+ */
	+ protected $reauthPostData = null;
	+
	/**
	* Get an HTMLForm descriptor array
	* @return array
	@@ -89,13 +95,31 @@ abstract class FormSpecialPage extends SpecialPage {
	* @return HTMLForm\|null
	*/
	protected function getForm() {
	+ $context = $this->getContext();
	+ $onSubmit = [ $this, 'onSubmit' ];
	+
	+ if ( $this->reauthPostData ) {
	+ // Restore post data
	+ $context = new DerivativeContext( $context );
	+ $oldRequest = $this->getRequest();
	+ $context->setRequest( new DerivativeRequest(
	+ $oldRequest, $this->reauthPostData + $oldRequest->getQueryValues(), true
	+ ) );
	+
	+ // But don't treat it as a "real" submission just in case of some
	+ // crazy kind of CSRF.
	+ $onSubmit = function () {
	+ return false;
	+ };
	+ }
	+
	$form = HTMLForm::factory(
	$this->getDisplayFormat(),
	$this->getFormFields(),
	- $this->getContext(),
	+ $context,
	$this->getMessagePrefix()
	);
	- $form->setSubmitCallback( [ $this, 'onSubmit' ] );
	+ $form->setSubmitCallback( $onSubmit );
	if ( $this->getDisplayFormat() !== 'ooui' ) {
	// No legend and wrapper by default in OOUI forms, but can be set manually
	// from alterForm()
	@@ -174,6 +198,11 @@ abstract class FormSpecialPage extends SpecialPage {
	protected function checkExecutePermissions( User $user ) {
	$this->checkPermissions();

	+ $securityLevel = $this->getLoginSecurityLevel();
	+ if ( $securityLevel !== false ) {
	+ $this->checkLoginSecurityLevel( $securityLevel );
	+ }
	+
	if ( $this->requiresUnblock() && $user->isBlocked() ) {
	$block = $user->getBlock();
	throw new UserBlockedError( $block );
	@@ -199,4 +228,14 @@ abstract class FormSpecialPage extends SpecialPage {
	public function requiresUnblock() {
	return true;
	}
	+
	+ /**
	+ * Preserve post data across reauthentication
	+ *
	+ * @since 1.32
	+ * @param array $data
	+ */
	+ protected function setReauthPostData( array $data ) {
	+ $this->reauthPostData = $data;
	+ }
	}
	diff --git a/includes/specialpage/SpecialPage.php b/includes/specialpage/SpecialPage.php
	index 6828b4a400..0b7ee73975 100644
	--- a/includes/specialpage/SpecialPage.php
	+++ b/includes/specialpage/SpecialPage.php
	@@ -352,6 +352,19 @@ class SpecialPage implements MessageLocalizer {
	return false;
	}

	+ /**
	+ * Record preserved post data after a reauthentication.
	+ *
	+ * The base SpecialPage implementation does nothing. If your subclass uses
	+ * getLoginSecurityLevel() or checkLoginSecurityLevel(), it should probably
	+ * implement this to do something with the data.
	+ *
	+ * @since 1.32
	+ * @param array $data
	+ */
	+ protected function setReauthPostData( array $data ) {
	+ }
	+
	/**
	* Verifies that the user meets the security level, possibly reauthenticating them in the process.
	*
	@@ -378,16 +391,42 @@ class SpecialPage implements MessageLocalizer {
	*/
	protected function checkLoginSecurityLevel( $level = null ) {
	$level = $level ?: $this->getName();
	+ $key = 'SpecialPage:reauth:' . $this->getName();
	+ $request = $this->getRequest();
	+
	$securityStatus = AuthManager::singleton()->securitySensitiveOperationStatus( $level );
	if ( $securityStatus === AuthManager::SEC_OK ) {
	+ $uniqueId = $request->getVal( 'postUniqueId' );
	+ if ( $uniqueId ) {
	+ $key = $key . ':' . $uniqueId;
	+ $session = $request->getSession();
	+ $data = $session->getSecret( $key );
	+ if ( $data ) {
	+ $session->remove( $key );
	+ $this->setReauthPostData( $data );
	+ }
	+ }
	return true;
	} elseif ( $securityStatus === AuthManager::SEC_REAUTH ) {
	- $request = $this->getRequest();
	$title = self::getTitleFor( 'Userlogin' );
	+ $queryParams = $request->getQueryValues();
	+
	+ if ( $request->wasPosted() ) {
	+ $data = array_diff_key( $request->getValues(), $request->getQueryValues() );
	+ if ( $data ) {
	+ // unique ID in case the same special page is open in multiple browser tabs
	+ $uniqueId = MWCryptRand::generateHex( 6 );
	+ $key = $key . ':' . $uniqueId;
	+ $queryParams['postUniqueId'] = $uniqueId;
	+ $session = $request->getSession();
	+ $session->persist(); // Just in case
	+ $session->setSecret( $key, $data );
	+ }
	+ }
	+
	$query = [
	'returnto' => $this->getFullTitle()->getPrefixedDBkey(),
	- 'returntoquery' => wfArrayToCgi( array_diff_key( $request->getQueryValues(),
	- [ 'title' => true ] ) ),
	+ 'returntoquery' => wfArrayToCgi( array_diff_key( $queryParams, [ 'title' => true ] ) ),
	'force' => $level,
	];
	$url = $title->getFullURL( $query, false, PROTO_HTTPS );
	@@ -568,7 +607,10 @@ class SpecialPage implements MessageLocalizer {
	public function execute( $subPage ) {
	$this->setHeaders();
	$this->checkPermissions();
	- $this->checkLoginSecurityLevel( $this->getLoginSecurityLevel() );
	+ $securityLevel = $this->getLoginSecurityLevel();
	+ if ( $securityLevel !== false ) {
	+ $this->checkLoginSecurityLevel( $securityLevel );
	+ }
	$this->outputHeader();
	}

	--
	2.17.0

File Metadata

Mime Type: text/x-diff
Storage Engine: blob
Storage Format: Raw Data
Storage Handle: 5807234
Default Alt Text: 0001-SECURITY-Add-getLoginSecurityLevel-support-to-FormSp.patch (6 KB)

0001-SECURITY-Add-getLoginSecurityLevel-support-to-FormSp.patchAnomieActions

0001-SECURITY-Add-getLoginSecurityLevel-support-to-FormSp.patchView Options

File Metadata

Event Timeline

0001-SECURITY-Add-getLoginSecurityLevel-support-to-FormSp.patch
Anomie
Actions

0001-SECURITY-Add-getLoginSecurityLevel-support-to-FormSp.patch
View Options