Use tokens for anons solely based on IP + non-unique secret
No One
Actions

Authored By

	Bawolff
	Feb 7 2016, 6:52 PM

Size

5 KB

Referenced Files

None

Subscribers

None

Use tokens for anons solely based on IP + non-unique secret
View Options

	From 2772b2923a52c283b07cf353e9da68449004099f Mon Sep 17 00:00:00 2001
	From: Brian Wolff <bawolff+wn@gmail.com>
	Date: Sun, 7 Feb 2016 12:20:42 -0500
	Subject: [PATCH] [SECURITY] Prevent CSRF for anons using tokens based on IP.

	Due to performance concerns, we can't have sessions for anon users.
	So instead, lets give them tokens based on their IP and a non-unique
	secret. Anons sharing an IP will still be vulnerable to a CSRF, but
	there's not much point doing a CSRF in that case since anon actions
	are all associated with the IP, so any action you do via CSRF, may
	as well just be done directly.

	This primary attack this is meant to deal with, would be someone
	trying to evade an IP-based block using CSRF, possibly using CSRF
	to make the damage appear to be coming from many IPs.

	Its important that tokens using this scheme expire in a reasonable
	amount of time. Otherwise someone could try to "collect" a large
	number of tokens to use in a targetted CSRF attack later. This
	sets anon tokens to expire after 4 hours.

	Change-Id: I0aa2dd3bd208336967afebc9c4bee196abb5b9fd
	---
	includes/api/ApiCheckToken.php \| 5 +++++
	includes/htmlform/HTMLForm.php \| 9 +--------
	.../ResourceLoaderUserTokensModule.php \| 6 ++++++
	includes/user/User.php \| 21 +++++++++++++++++----
	4 files changed, 29 insertions(+), 12 deletions(-)

	diff --git a/includes/api/ApiCheckToken.php b/includes/api/ApiCheckToken.php
	index dfcbaf8..1259385 100644
	--- a/includes/api/ApiCheckToken.php
	+++ b/includes/api/ApiCheckToken.php
	@@ -34,6 +34,11 @@ class ApiCheckToken extends ApiBase {
	$maxage = $params['maxtokenage'];
	$salts = ApiQueryTokens::getTokenTypeSalts();

	+ // Anon tokens have an upper limit of 4 hours for maxage
	+ if ( $this->getUser()->isAnon() && ( !$maxage \|\| $maxage > 46060 ) ) {
	+ $maxage = 46060;
	+ }
	+
	$res = array();

	$tokenObj = ApiQueryTokens::getToken(
	diff --git a/includes/htmlform/HTMLForm.php b/includes/htmlform/HTMLForm.php
	index 2282dc2..b06ea63 100644
	--- a/includes/htmlform/HTMLForm.php
	+++ b/includes/htmlform/HTMLForm.php
	@@ -489,14 +489,7 @@ class HTMLForm extends ContextSource {
	$submit = true; // no session check needed
	} elseif ( $this->getRequest()->wasPosted() ) {
	$editToken = $this->getRequest()->getVal( 'wpEditToken' );
	- if ( $this->getUser()->isLoggedIn() \|\| $editToken != null ) {
	- // Session tokens for logged-out users have no security value.
	- // However, if the user gave one, check it in order to give a nice
	- // "session expired" error instead of "permission denied" or such.
	- $submit = $this->getUser()->matchEditToken( $editToken, $this->mTokenSalt );
	- } else {
	- $submit = true;
	- }
	+ $submit = $this->getUser()->matchEditToken( $editToken, $this->mTokenSalt );
	}

	if ( $submit ) {
	diff --git a/includes/resourceloader/ResourceLoaderUserTokensModule.php b/includes/resourceloader/ResourceLoaderUserTokensModule.php
	index 78fec50..247faee 100644
	--- a/includes/resourceloader/ResourceLoaderUserTokensModule.php
	+++ b/includes/resourceloader/ResourceLoaderUserTokensModule.php
	@@ -43,6 +43,12 @@ class ResourceLoaderUserTokensModule extends ResourceLoaderModule {
	protected function contextUserTokens( ResourceLoaderContext $context ) {
	$user = $context->getUserObj();

	+ if ( $user->isAnon() ) {
	+ // Do not load tokens for anons.
	+ // The anon token depends on the user IP address, and we
	+ // do not want them getting into varnish cache.
	+ return array();
	+ }
	return array(
	'editToken' => $user->getEditToken(),
	'patrolToken' => $user->getEditToken( 'patrol' ),
	diff --git a/includes/user/User.php b/includes/user/User.php
	index dccfd77..25ffaf8 100644
	--- a/includes/user/User.php
	+++ b/includes/user/User.php
	@@ -21,6 +21,7 @@
	*/

	use MediaWiki\Session\SessionManager;
	+use MediaWiki\Session\Token;

	/**
	* String Some punctuation to prevent editing from broken text-mangling proxies.
	@@ -4156,13 +4157,19 @@ class User implements IDBAccessObject {
	* @return MediaWiki\\Session\\Token The new edit token
	*/
	public function getEditTokenObject( $salt = '', $request = null ) {
	- if ( $this->isAnon() ) {
	- return new LoggedOutEditToken();
	- }
	-
	+ global $wgSecretKey;
	if ( !$request ) {
	$request = $this->getRequest();
	}
	+
	+ if ( $this->isAnon() ) {
	+ // For anons, we use tokens based on IP.
	+ // Since we do not want to start sessions for them
	+ // due to performance concerns.
	+ $sharedSecret = $wgSecretKey;
	+ $ip = $request->getIP();
	+ return new Token( $ip . $sharedSecret, $salt );
	+ }
	return $request->getSession()->getToken( $salt );
	}

	@@ -4205,6 +4212,12 @@ class User implements IDBAccessObject {
	* @return bool Whether the token matches
	*/
	public function matchEditToken( $val, $salt = '', $request = null, $maxage = null ) {
	+ // Anon tokens are reused by people with the same IP, so its
	+ // important that they don't last too long. Set a maxage of at
	+ // most 4 hours.
	+ if ( $this->isAnon() && ( !$maxage \|\| $maxage > 46060 ) ) {
	+ $maxage = 46060;
	+ }
	return $this->getEditTokenObject( $salt, $request )->match( $val, $maxage );
	}

	--
	2.0.1

File Metadata

Mime Type: text/x-diff
Storage Engine: blob
Storage Format: Raw Data
Storage Handle: 3327880
Default Alt Text: Use tokens for anons solely based on IP + non-unique secret (5 KB)

Use tokens for anons solely based on IP + non-unique secretNo OneActions

Use tokens for anons solely based on IP + non-unique secretView Options

File Metadata

Event Timeline

Use tokens for anons solely based on IP + non-unique secret
No One
Actions

Use tokens for anons solely based on IP + non-unique secret
View Options