Page MenuHomePhabricator
Files F3110761

omnibus-master.patch
demon (Chad Horohoe)
Actions

Authored By
demon
Dec 17 2015, 12:33 AM
Size
18 KB
Referenced Files
None
Subscribers
None

omnibus-master.patch
View Options

From 10ba2acbe069397caa2f50038b1914138c44bbd6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Gerg=C5=91=20Tisza?= <tgr.huwiki@gmail.com>
Date: Sat, 21 Nov 2015 11:51:02 -0800
Subject: [PATCH 1/6] Use hash_equals in User::matchEditToken
There is no point in using hash_equals for the return value if we
do a normal comparison before.
Bug: T119309
Change-Id: Ia44ec5ed492105b27d0fddd845d58d27a29dc072
Signed-off-by: Chad Horohoe <chadh@wikimedia.org>
---
includes/user/User.php | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/includes/user/User.php b/includes/user/User.php
index c6d215d..2ac0f2c 100644
--- a/includes/user/User.php
+++ b/includes/user/User.php
@@ -4228,7 +4228,7 @@ class User implements IDBAccessObject {
$salt, $request ?: $this->getRequest(), $timestamp
);
- if ( $val != $sessionToken ) {
+ if ( !hash_equals( $sessionToken, $val ) ) {
wfDebug( "User::matchEditToken: broken session data\n" );
}
--
2.6.4
From d97f74c2f20b0746d9a748d6ede9a9a8817c64c4 Mon Sep 17 00:00:00 2001
From: Roan Kattouw <roan.kattouw@gmail.com>
Date: Fri, 6 Nov 2015 12:55:16 -0800
Subject: [PATCH 2/6] SECURITY: Work around CURL insanity breaking POST
parameters that start with '@'
CURL has a "feature" where passing array( 'foo' => '@bar' )
in CURLOPT_POSTFIELDS results in the contents of the file named "bar"
being POSTed. This makes it impossible to POST the literal string "@bar",
because array( 'foo' => '%40bar' ) gets double-encoded to foo=%2540bar.
Disable this "feature" by setting CURLOPT_SAFE_UPLOAD to true,
if available. According to the PHP manual, this option became
available in 5.5 and started defaulting to true in 5.6.
However, we support versions as low as 5.3, and this option
doesn't exist at all in 5.6.99-hhvm, which we run in production.
For versions where this option is not available (pre-5.5 versions
and HHVM), serialize POSTFIELDS arrays to strings. This works
around the issue because the '@' "feature" only works
for arrays, not strings, as of PHP 5.2. (We don't support pre-5.2
versions, and I've verified 5.6.99-hhvm behaves this way as well.)
Bug: T118032
Change-Id: I3f996e2eb87c7bd3b94ca9d3cc14a3e12f34f241
Signed-off-by: Chad Horohoe <chadh@wikimedia.org>
---
includes/HttpFunctions.php | 17 ++++++++++++++++-
includes/libs/MultiHttpClient.php | 13 +++++++++++++
2 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/includes/HttpFunctions.php b/includes/HttpFunctions.php
index e6801e3..d6e5170 100644
--- a/includes/HttpFunctions.php
+++ b/includes/HttpFunctions.php
@@ -782,7 +782,22 @@ class CurlHttpRequest extends MWHttpRequest {
$this->curlOptions[CURLOPT_HEADER] = true;
} elseif ( $this->method == 'POST' ) {
$this->curlOptions[CURLOPT_POST] = true;
- $this->curlOptions[CURLOPT_POSTFIELDS] = $this->postData;
+ $postData = $this->postData;
+ // Don't interpret POST parameters starting with '@' as file uploads, because this
+ // makes it impossible to POST plain values starting with '@' (and causes security
+ // issues potentially exposing the contents of local files).
+ // The PHP manual says this option was introduced in PHP 5.5 defaults to true in PHP 5.6,
+ // but we support lower versions, and the option doesn't exist in HHVM 5.6.99.
+ if ( defined( 'CURLOPT_SAFE_UPLOAD' ) ) {
+ $this->curlOptions[CURLOPT_SAFE_UPLOAD] = true;
+ } else if ( is_array( $postData ) ) {
+ // In PHP 5.2 and later, '@' is interpreted as a file upload if POSTFIELDS
+ // is an array, but not if it's a string. So convert $req['body'] to a string
+ // for safety.
+ $postData = wfArrayToCgi( $postData );
+ }
+ $this->curlOptions[CURLOPT_POSTFIELDS] = $postData;
+
// Suppress 'Expect: 100-continue' header, as some servers
// will reject it with a 417 and Curl won't auto retry
// with HTTP 1.0 fallback
diff --git a/includes/libs/MultiHttpClient.php b/includes/libs/MultiHttpClient.php
index c6fa914..0e9c423 100644
--- a/includes/libs/MultiHttpClient.php
+++ b/includes/libs/MultiHttpClient.php
@@ -338,6 +338,19 @@ class MultiHttpClient {
);
} elseif ( $req['method'] === 'POST' ) {
curl_setopt( $ch, CURLOPT_POST, 1 );
+ // Don't interpret POST parameters starting with '@' as file uploads, because this
+ // makes it impossible to POST plain values starting with '@' (and causes security
+ // issues potentially exposing the contents of local files).
+ // The PHP manual says this option was introduced in PHP 5.5 defaults to true in PHP 5.6,
+ // but we support lower versions, and the option doesn't exist in HHVM 5.6.99.
+ if ( defined( 'CURLOPT_SAFE_UPLOAD' ) ) {
+ curl_setopt( $ch, CURLOPT_SAFE_UPLOAD, true );
+ } else if ( is_array( $req['body'] ) ) {
+ // In PHP 5.2 and later, '@' is interpreted as a file upload if POSTFIELDS
+ // is an array, but not if it's a string. So convert $req['body'] to a string
+ // for safety.
+ $req['body'] = wfArrayToCgi( $req['body'] );
+ }
curl_setopt( $ch, CURLOPT_POSTFIELDS, $req['body'] );
} else {
if ( is_resource( $req['body'] ) || $req['body'] !== '' ) {
--
2.6.4
From 931574ea5dfe8e4d0d3c7749004b0a9968d2f520 Mon Sep 17 00:00:00 2001
From: Brad Jorsch <bjorsch@wikimedia.org>
Date: Wed, 14 Oct 2015 17:40:42 -0400
Subject: [PATCH 3/6] [SECURITY] 0-pad to length in random string generation
Otherwise shorter strings might be generated.
Bug: T115522
Change-Id: I110d873d56762552060fd428c236c8b0e9a859b0
Signed-off-by: Chad Horohoe <chadh@wikimedia.org>
---
includes/password/PasswordFactory.php | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/includes/password/PasswordFactory.php b/includes/password/PasswordFactory.php
index 6b634cb..f80e158 100644
--- a/includes/password/PasswordFactory.php
+++ b/includes/password/PasswordFactory.php
@@ -200,11 +200,10 @@ final class PasswordFactory {
// stopping at a minimum of 10 chars.
$length = max( 10, $minLength );
// Multiply by 1.25 to get the number of hex characters we need
- $length = $length * 1.25;
// Generate random hex chars
- $hex = MWCryptRand::generateHex( $length );
+ $hex = MWCryptRand::generateHex( ceil( $length * 1.25 ) );
// Convert from base 16 to base 32 to get a proper password like string
- return Wikimedia\base_convert( $hex, 16, 32 );
+ return substr( Wikimedia\base_convert( $hex, 16, 32, $length ), -$length );
}
/**
--
2.6.4
From ed03ae5c0a43d9f952c914a8e4df51dea64de4d6 Mon Sep 17 00:00:00 2001
From: Marius Hoch <hoo@online.de>
Date: Sat, 2 May 2015 18:48:04 +0200
Subject: [PATCH 4/6] Fix IP::toHex for IPv4 addresses with a double/triple 0
block
Bug: T97897
Change-Id: I5c0a37be42ae2c5091ead487a6d19f6e0dd89b36
Signed-off-by: Chad Horohoe <chadh@wikimedia.org>
---
includes/utils/IP.php | 22 ++++++++++++++++------
tests/phpunit/includes/utils/IPTest.php | 33 ++++++++++++++++++++++++++++-----
2 files changed, 44 insertions(+), 11 deletions(-)
diff --git a/includes/utils/IP.php b/includes/utils/IP.php
index fd7030e..979b6b3 100644
--- a/includes/utils/IP.php
+++ b/includes/utils/IP.php
@@ -132,8 +132,9 @@ class IP {
/**
* Convert an IP into a verbose, uppercase, normalized form.
- * IPv6 addresses in octet notation are expanded to 8 words.
- * IPv4 addresses are just trimmed.
+ * Both IPv4 and IPv6 addresses are trimmed. Additionally,
+ * IPv6 addresses in octet notation are expanded to 8 words;
+ * IPv4 addresses have leading zeros, in each octet, removed.
*
* @param string $ip IP address in quad or octet form (CIDR or not).
* @return string
@@ -143,8 +144,16 @@ class IP {
if ( $ip === '' ) {
return null;
}
- if ( self::isIPv4( $ip ) || !self::isIPv6( $ip ) ) {
- return $ip; // nothing else to do for IPv4 addresses or invalid ones
+ /* If not an IP, just return trimmed value, since sanitizeIP() is called
+ * in a number of contexts where usernames are supplied as input.
+ */
+ if ( !self::isIPAddress($ip) ) {
+ return $ip;
+ }
+ if ( self::isIPv4( $ip ) ) {
+ // Remove leading 0's from octet representation of IPv4 address
+ $ip = preg_replace( '/(?:^|(?<=\.))0+(?=[1-9]|0\.|0$)/', '', $ip );
+ return $ip;
}
// Remove any whitespaces, convert to upper case
$ip = strtoupper( $ip );
@@ -399,8 +408,9 @@ class IP {
if ( self::isIPv6( $ip ) ) {
$n = 'v6-' . self::IPv6ToRawHex( $ip );
} elseif ( self::isIPv4( $ip ) ) {
- // Bug 60035: an IP with leading 0's fails in ip2long sometimes (e.g. *.08)
- $ip = preg_replace( '/(?<=\.)0+(?=[1-9])/', '', $ip );
+ // T62035/T97897: An IP with leading 0's fails in ip2long sometimes (e.g. *.08),
+ // also double/triple 0 needs to be changed to just a single 0 for ip2long.
+ $ip = self::sanitizeIP( $ip );
$n = ip2long( $ip );
if ( $n < 0 ) {
$n += pow( 2, 32 );
diff --git a/tests/phpunit/includes/utils/IPTest.php b/tests/phpunit/includes/utils/IPTest.php
index 04b8f48..34aff79 100644
--- a/tests/phpunit/includes/utils/IPTest.php
+++ b/tests/phpunit/includes/utils/IPTest.php
@@ -307,12 +307,34 @@ class IPTest extends PHPUnit_Framework_TestCase {
}
/**
- * Improve IP::sanitizeIP() code coverage
- * @todo Most probably incomplete
+ * @covers IP::sanitizeIP
+ * @dataProvider provideSanitizeIP
*/
- public function testSanitizeIP() {
- $this->assertNull( IP::sanitizeIP( '' ) );
- $this->assertNull( IP::sanitizeIP( ' ' ) );
+ public function testSanitizeIP( $expected, $input ) {
+ $result = IP::sanitizeIP( $input );
+ $this->assertEquals( $expected, $result );
+ }
+
+ /**
+ * Provider for IP::testSanitizeIP()
+ */
+ public static function provideSanitizeIP() {
+ return array(
+ array( '0.0.0.0', '0.0.0.0' ),
+ array( '0.0.0.0', '00.00.00.00' ),
+ array( '0.0.0.0', '000.000.000.000' ),
+ array( '141.0.11.253', '141.000.011.253' ),
+ array( '1.2.4.5', '1.2.4.5' ),
+ array( '1.2.4.5', '01.02.04.05' ),
+ array( '1.2.4.5', '001.002.004.005' ),
+ array( '10.0.0.1', '010.0.000.1' ),
+ array( '80.72.250.4', '080.072.250.04' ),
+ array( 'Foo.1000.00', 'Foo.1000.00'),
+ array( 'Bar.01', 'Bar.01'),
+ array( 'Bar.010', 'Bar.010'),
+ array( null, ''),
+ array( null, ' ')
+ );
}
/**
@@ -336,6 +358,7 @@ class IPTest extends PHPUnit_Framework_TestCase {
array( '80000000', '128.0.0.0' ),
array( 'DEADCAFE', '222.173.202.254' ),
array( 'FFFFFFFF', '255.255.255.255' ),
+ array( '8D000BFD', '141.000.11.253' ),
array( false, 'IN.VA.LI.D' ),
array( 'v6-00000000000000000000000000000001', '::1' ),
array( 'v6-20010DB885A3000000008A2E03707334', '2001:0db8:85a3:0000:0000:8a2e:0370:7334' ),
--
2.6.4
From 31139dfdf788fdfd00a3b3e29c376cb05d7fbfaa Mon Sep 17 00:00:00 2001
From: csteipp <csteipp@wikimedia.org>
Date: Mon, 5 Oct 2015 16:58:42 -0700
Subject: [PATCH 5/6] SECURITY: Make Special:MyPage and friends fake redirect
to prevent info leak
This prevents a malicious person from using external resources on their
website to cause the victim's web browser to load
Special:MyPage -> User:Username, and then looking it up in the page hit
statistics in order to correlate IPs from the malicious person's server
log, with usernames on wiki.
This feature can be disabled with $wgHideIdentifiableRedirects.
Bug: T109724
Change-Id: Ia0e742dc92c77af4832174dfa24c6dcaa6ee80e9
Signed-off-by: Chad Horohoe <chadh@wikimedia.org>
---
includes/DefaultSettings.php | 6 ++++
includes/MediaWiki.php | 44 ++++++++++++++++++++----
includes/specialpage/RedirectSpecialPage.php | 12 +++++++
includes/specials/SpecialMyLanguage.php | 11 ++++++
includes/specials/SpecialMyRedirectPages.php | 50 ++++++++++++++++++++++++++++
5 files changed, 117 insertions(+), 6 deletions(-)
diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php
index 39b133b..19e0bf1 100644
--- a/includes/DefaultSettings.php
+++ b/includes/DefaultSettings.php
@@ -4755,6 +4755,12 @@ $wgWhitelistReadRegexp = false;
$wgEmailConfirmToEdit = false;
/**
+ * Should MediaWiki attempt to protect user's privacy when doing redirects?
+ * Keep this true if access counts to articles are made public.
+ */
+$wgHideIdentifiableRedirects = true;
+
+/**
* Permission keys given to users in each group.
*
* This is an array where the keys are all groups and each value is an
diff --git a/includes/MediaWiki.php b/includes/MediaWiki.php
index bb0f1e4..2769a6c 100644
--- a/includes/MediaWiki.php
+++ b/includes/MediaWiki.php
@@ -37,6 +37,11 @@ class MediaWiki {
private $config;
/**
+ * @var String Cache what action this request is
+ */
+ private $action;
+
+ /**
* @param IContextSource|null $context
*/
public function __construct( IContextSource $context = null ) {
@@ -141,13 +146,11 @@ class MediaWiki {
* @return string Action
*/
public function getAction() {
- static $action = null;
-
- if ( $action === null ) {
- $action = Action::getActionName( $this->context );
+ if ( $this->action === null ) {
+ $this->action = Action::getActionName( $this->context );
}
- return $action;
+ return $this->action;
}
/**
@@ -241,8 +244,37 @@ class MediaWiki {
// Handle any other redirects.
// Redirect loops, titleless URL, $wgUsePathInfo URLs, and URLs with a variant
} elseif ( !$this->tryNormaliseRedirect( $title ) ) {
+ // Prevent information leak via Special:MyPage et al (T109724)
+ if ( $title->isSpecialPage() ) {
+ $specialPage = SpecialPageFactory::getPage( $title->getDBKey() );
+ if ( $specialPage instanceof RedirectSpecialPage
+ && $this->config->get( 'HideIdentifiableRedirects' )
+ && $specialPage->personallyIdentifiableTarget()
+ ) {
+ list( , $subpage ) = SpecialPageFactory::resolveAlias( $title->getDBKey() );
+ $target = $specialPage->getRedirect( $subpage );
+ // target can also be true. We let that case fall through to normal processing.
+ if ( $target instanceof Title ) {
+ $query = $specialPage->getRedirectQuery() ?: array();
+ $request = new DerivativeRequest( $this->context->getRequest(), $query );
+ $request->setRequestURL( $this->context->getRequest()->getRequestURL() );
+ $this->context->setRequest( $request );
+ // Do not varnish cache these. May vary even for anons
+ $this->context->getOutput()->lowerCdnMaxage( 0 );
+ $this->context->setTitle( $target );
+ $wgTitle = $target;
+ // Reset action type cache. (Special pages have only view)
+ $this->action = null;
+ $title = $target;
+ $output->addJsConfigVars( array(
+ 'wgInternalRedirectTargetUrl' => $target->getFullURL(),
+ ) );
+ $output->addModules( 'mediawiki.action.view.redirect' );
+ }
+ }
+ }
- // Special pages
+ // Special pages ($title may have changed since if statement above)
if ( NS_SPECIAL == $title->getNamespace() ) {
// Actions that need to be made when we have a special pages
SpecialPageFactory::executePath( $title, $this->context );
diff --git a/includes/specialpage/RedirectSpecialPage.php b/includes/specialpage/RedirectSpecialPage.php
index 9129ee5..5047354 100644
--- a/includes/specialpage/RedirectSpecialPage.php
+++ b/includes/specialpage/RedirectSpecialPage.php
@@ -94,6 +94,18 @@ abstract class RedirectSpecialPage extends UnlistedSpecialPage {
? $params
: false;
}
+
+ /**
+ * Indicate if the target of this redirect can be used to identify
+ * a particular user of this wiki (e.g., if the redirect is to the
+ * user page of a User). See T109724.
+ *
+ * @since 1.27
+ * @return bool
+ */
+ public function personallyIdentifiableTarget() {
+ return false;
+ }
}
/**
diff --git a/includes/specials/SpecialMyLanguage.php b/includes/specials/SpecialMyLanguage.php
index 3d8ff97..d11fbe6 100644
--- a/includes/specials/SpecialMyLanguage.php
+++ b/includes/specials/SpecialMyLanguage.php
@@ -99,4 +99,15 @@ class SpecialMyLanguage extends RedirectSpecialArticle {
return $base;
}
}
+
+ /**
+ * Target can identify a specific user's language preference.
+ *
+ * @see T109724
+ * @since 1.27
+ * @return bool
+ */
+ public function personallyIdentifiableTarget() {
+ return true;
+ }
}
diff --git a/includes/specials/SpecialMyRedirectPages.php b/includes/specials/SpecialMyRedirectPages.php
index 5ef03f1..850b1f6 100644
--- a/includes/specials/SpecialMyRedirectPages.php
+++ b/includes/specials/SpecialMyRedirectPages.php
@@ -45,6 +45,16 @@ class SpecialMypage extends RedirectSpecialArticle {
return Title::makeTitle( NS_USER, $this->getUser()->getName() . '/' . $subpage );
}
+
+ /**
+ * Target identifies a specific User. See T109724.
+ *
+ * @since 1.27
+ * @return bool
+ */
+ public function personallyIdentifiableTarget() {
+ return true;
+ }
}
/**
@@ -68,6 +78,16 @@ class SpecialMytalk extends RedirectSpecialArticle {
return Title::makeTitle( NS_USER_TALK, $this->getUser()->getName() . '/' . $subpage );
}
+
+ /**
+ * Target identifies a specific User. See T109724.
+ *
+ * @since 1.27
+ * @return bool
+ */
+ public function personallyIdentifiableTarget() {
+ return true;
+ }
}
/**
@@ -90,6 +110,16 @@ class SpecialMycontributions extends RedirectSpecialPage {
public function getRedirect( $subpage ) {
return SpecialPage::getTitleFor( 'Contributions', $this->getUser()->getName() );
}
+
+ /**
+ * Target identifies a specific User. See T109724.
+ *
+ * @since 1.27
+ * @return bool
+ */
+ public function personallyIdentifiableTarget() {
+ return true;
+ }
}
/**
@@ -110,6 +140,16 @@ class SpecialMyuploads extends RedirectSpecialPage {
public function getRedirect( $subpage ) {
return SpecialPage::getTitleFor( 'Listfiles', $this->getUser()->getName() );
}
+
+ /**
+ * Target identifies a specific User. See T109724.
+ *
+ * @since 1.27
+ * @return bool
+ */
+ public function personallyIdentifiableTarget() {
+ return true;
+ }
}
/**
@@ -132,4 +172,14 @@ class SpecialAllMyUploads extends RedirectSpecialPage {
return SpecialPage::getTitleFor( 'Listfiles', $this->getUser()->getName() );
}
+
+ /**
+ * Target identifies a specific User. See T109724.
+ *
+ * @since 1.27
+ * @return bool
+ */
+ public function personallyIdentifiableTarget() {
+ return true;
+ }
}
--
2.6.4
From 47d733a0617170a3120bc4d5d387f4f17c9a6d28 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bartosz=20Dziewo=C5=84ski?= <matma.rex@gmail.com>
Date: Thu, 19 Nov 2015 17:13:13 -0500
Subject: [PATCH 6/6] Add $query to JavaScript redirect info
Bug: T109724
Change-Id: I57a8f75067365d3da6388d2f8f7fe95ed5e6f310
Signed-off-by: Chad Horohoe <chadh@wikimedia.org>
---
includes/MediaWiki.php | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/includes/MediaWiki.php b/includes/MediaWiki.php
index 2769a6c..4c0da95 100644
--- a/includes/MediaWiki.php
+++ b/includes/MediaWiki.php
@@ -267,7 +267,7 @@ class MediaWiki {
$this->action = null;
$title = $target;
$output->addJsConfigVars( array(
- 'wgInternalRedirectTargetUrl' => $target->getFullURL(),
+ 'wgInternalRedirectTargetUrl' => $target->getFullURL( $query ),
) );
$output->addModules( 'mediawiki.action.view.redirect' );
}
--
2.6.4

File Metadata

Mime Type
text/x-diff
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
3119390
Default Alt Text
omnibus-master.patch (18 KB)

Event Timeline

Content licensed under Creative Commons Attribution-ShareAlike 3.0 (CC-BY-SA) unless otherwise noted; code licensed under GNU General Public License (GPL) or 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