Error thrown by VirtualRESTService when POST variable starts with '@'
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Catrope
	Nov 6 2015, 9:10 PM

Description

This happens because VRS doesn't set CURLOPT_SAFE_UPLOAD to true (it's supposed to default to true in PHP >=5.6.0, but it doesn't in production with 5.6.99-hhvm), which allows curl to try to be "helpful" by interpreting array( 'foo' => '@bar' ) as a file upload.

I haven't managed to exploit this from the web interface because Parsoid doesn't seem to like file uploads and returns a 404 rather than exposing the contents of the file. Also, absolute paths like @/etc/crontab don't appear to work.

patches:

master -
T118032b.patch4 KBDownload
1.23 - included in
omnibus-v2-REL1_23.patch34 KBDownload
1.24 - included in
omnibus-v3-REL1_24.patch35 KBDownload
1.25 - included in
omnibus-REL1_25.patch34 KBDownload
1.26 - included in
omnibus-master.patch18 KBDownload

affected versions:
type: CWE-159
CVE: CVE-2015-8625

Details

Subject	Repo	Branch	Lines +/-
SECURITY: Work around CURL insanity breaking POST parameters that start with '@'	mediawiki/core	master	+29 -1
SECURITY: Work around CURL insanity breaking POST parameters that start with '@'	mediawiki/core	REL1_26	+29 -1
SECURITY: Work around CURL insanity breaking POST parameters that start with '@'	mediawiki/core	REL1_25	+29 -1
SECURITY: Work around CURL insanity breaking POST parameters that start with '@'	mediawiki/core	REL1_23	+29 -1
SECURITY: Work around CURL insanity breaking POST parameters that start with '@'	mediawiki/core	REL1_24	+29 -1

Customize query in gerrit

Related Objects
Search...

Status	Assigned	Task
Resolved	Catrope	T117938 Parsoid failures trying to save or preview posts with @ mentions on mw.org (curl error: 26)
Resolved	• demon	T115722 MediaWiki Security release 1.25.4
Resolved	Catrope	T118032 Error thrown by VirtualRESTService when POST variable starts with '@'

Event Timeline

0001-Work-around-CURL-insanity-breaking-POST-parameters-t.patch1 KBDownload

Legoktm mentioned this in T117938: Parsoid failures trying to save or preview posts with @ mentions on mw.org (curl error: 26).Nov 6 2015, 10:27 PM

Patch looks good to me. It doesn't look like it should break anything else in core.

I'm wondering if CurlHttpRequest in core should be patched too?

@awight, it looks like DonationInterface does a bunch of curl POST's, you want to make sure it doesn't have this same flaw?

• csteipp added a subscriber: awight.Nov 6 2015, 10:30 PM

I'm suspicious ApiContentTranslationPublish.php and TranslationNotificationJob.php could be used to exploit this, through their use of MWHttpRequest.

I've looked through pretty much all extensions using MWHttpRequest, and it doesn't look like any try to use @ intentionally. So I think setting CURLOPT_SAFE_UPLOAD in CurlHttpRequest shouldn't break anything.

0001-Work-around-CURL-insanity-breaking-POST-parameters-t.patch2 KBDownload

Updated patch that also patches CurlHttpRequest

In T118032#1790417, @Catrope wrote:

0001-Work-around-CURL-insanity-breaking-POST-parameters-t.patch2 KBDownload

Updated patch that also patches CurlHttpRequest

LGTM. Let's deploy this Monday.

(it's supposed to default to true in PHP >=5.6.0, but it doesn't in production with 5.6.99-hhvm)

We should file this upstream against whatever repo the HHVM CURL implementation is in.

In T118032#1790435, @csteipp wrote:

In T118032#1790417, @Catrope wrote:

0001-Work-around-CURL-insanity-breaking-POST-parameters-t.patch2 KBDownload

Updated patch that also patches CurlHttpRequest

LGTM. Let's deploy this Monday.

It's not deployed yet. Will this be done tomorrow? I'd be OK deploying it myself, too, if that helps.

Yeah, I forgot Monday. If your around, please do deploy. Otherwise I'll do
it sometime tomorrow morning.

Attempted to deploy this, and the cluster started spewing,

> exception.log <

2015-11-13 17:37:11 mw1050 enwiki exception ERROR: [4ff0fb71] /w/index.php?title=Special:Book&bookcmd=render_article&arttitle=Buck+Rogers&returnto=Buck+Rogers&oldid=689325485&writer=rdf2latex MWException from line 803 of /srv/mediawiki/php-1.27.0-wmf.6/includes/HttpFunctions.php: Error setting curl options. {"exception_id":"4ff0fb71"}
[Exception MWException] (/srv/mediawiki/php-1.27.0-wmf.6/includes/HttpFunctions.php:803) Error setting curl options.

#0 /srv/mediawiki/php-1.27.0-wmf.6/includes/HttpFunctions.php(76): CurlHttpRequest->execute()
#1 /srv/mediawiki/php-1.27.0-wmf.6/includes/HttpFunctions.php(123): Http::request(string, string, array, string)
#2 /srv/mediawiki/php-1.27.0-wmf.6/extensions/Collection/RenderingAPI.php(297): Http::post(string, array, string)
#3 /srv/mediawiki/php-1.27.0-wmf.6/extensions/Collection/RenderingAPI.php(69): MWServeRenderingAPI->makeRequest(string, array)
#4 /srv/mediawiki/php-1.27.0-wmf.6/extensions/Collection/RenderingAPI.php(46): CollectionRenderingAPI->doRender(array)
#5 /srv/mediawiki/php-1.27.0-wmf.6/extensions/Collection/Collection.body.php(988): CollectionRenderingAPI->render(array)
#6 /srv/mediawiki/php-1.27.0-wmf.6/extensions/Collection/Collection.body.php(278): SpecialCollection->renderCollection(array, Title, string)
#7 /srv/mediawiki/php-1.27.0-wmf.6/includes/specialpage/SpecialPage.php(384): SpecialCollection->execute(NULL)
#8 /srv/mediawiki/php-1.27.0-wmf.6/includes/specialpage/SpecialPageFactory.php(553): SpecialPage->run(NULL)
#9 /srv/mediawiki/php-1.27.0-wmf.6/includes/MediaWiki.php(280): SpecialPageFactory::executePath(Title, RequestContext)
#10 /srv/mediawiki/php-1.27.0-wmf.6/includes/MediaWiki.php(704): MediaWiki->performRequest()
#11 /srv/mediawiki/php-1.27.0-wmf.6/includes/MediaWiki.php(506): MediaWiki->main()
#12 /srv/mediawiki/php-1.27.0-wmf.6/index.php(41): MediaWiki->run()
#13 /srv/mediawiki/w/index.php(3): include(string)
#14 {main}

> hhvm.log <

Nov 13 17:37:12 mw1164: #012Notice: Use of undefined constant CURLOPT_SAFE_UPLOAD - assumed 'CURLOPT_SAFE_UPLOAD' in /srv/mediawiki/php-1.27.0-wmf.6/includes/HttpFunctions.php on line 789
Nov 13 17:37:12 mw1164: #012Warning: Invalid argument: option: 0 in /srv/mediawiki/php-1.27.0-wmf.6/includes/HttpFunctions.php on line 802

If I had paid more attention to the php docs, CURLOPT_SAFE_UPLOAD was added in php 5.5.0, an apparently not supported by our current HHVM install.

I think we'll need to actually filter out values that start with "@".

In T118032#1804048, @csteipp wrote:

If I had paid more attention to the php docs, CURLOPT_SAFE_UPLOAD was added in php 5.5.0, an apparently not supported by our current HHVM install.

...even though our HHVM install claims to be PHP 5.6.99-hhhvm

I think we'll need to actually filter out values that start with "@".

That's going to be a problem. There's no way of escaping leading @ symbols. We can drop values that start with @ (or requests that contain them), but it sounds like we won't be able to make legitimate requests for things like convert?from=wikitext&to=html&content=@csteipp how's it going. If we are going to be unable to make them though, we should probably reject them instead of doing something insecure.

If we are going to be unable to make them though, we should probably reject them instead of doing something insecure.

Agree. I think either generating an exception and not making the call, or removing the @ (and risking data corruption) are the two bad options we have to choose from. I'm not sure which is better.

I found a way to support HHVM (as well as pre-5.5 versions of PHP) by using an obscure detail in curl's '@' behavior: it only kicks in if POSTFIELDS is an array, not if it's a string, so serializing all POSTFIELDS arrays to strings using wfArrayToCgi() works around the issue as well.

0001-Work-around-CURL-insanity-breaking-POST-parameters-t.patch3 KBDownload

In T118032#1804750, @Catrope wrote:

I found a way to support HHVM (as well as pre-5.5 versions of PHP) by using an obscure detail in curl's '@' behavior: it only kicks in if POSTFIELDS is an array, not if it's a string, so serializing all POSTFIELDS arrays to strings using wfArrayToCgi() works around the issue as well.

0001-Work-around-CURL-insanity-breaking-POST-parameters-t.patch3 KBDownload

Does this need to send $this->postData through wfArrayToCgi() in HttpFunctions too? Or is that already happening somewhere?

In T118032#1804965, @csteipp wrote:

In T118032#1804750, @Catrope wrote:

I found a way to support HHVM (as well as pre-5.5 versions of PHP) by using an obscure detail in curl's '@' behavior: it only kicks in if POSTFIELDS is an array, not if it's a string, so serializing all POSTFIELDS arrays to strings using wfArrayToCgi() works around the issue as well.

0001-Work-around-CURL-insanity-breaking-POST-parameters-t.patch3 KBDownload

Does this need to send $this->postData through wfArrayToCgi() in HttpFunctions too? Or is that already happening somewhere?

No, we do, I had just completely forgotten about that file. Will fix.

0001-Work-around-CURL-insanity-breaking-POST-parameters-t.patch4 KBDownload

Updated patch with HttpFunctions change. I don't have an easy way to test that though.

mediawiki-errors is trending with various issues possibly caused by this:

Warning: curl_setopt() expects parameter 2 to be integer, string given in /srv/mediawiki/php-1.27.0-wmf.7/includes/libs/MultiHttpClient.php on line 345

Notice: Use of undefined constant CURLOPT_SAFE_UPLOAD - assumed 'CURLOPT_SAFE_UPLOAD' in /srv/mediawiki/php-1.27.0-wmf.7/includes/libs/MultiHttpClient.php on line 345

Warning: Invalid argument: option: 0 in /srv/mediawiki/php-1.27.0-wmf.7/includes/HttpFunctions.php on line 802

Notice: Use of undefined constant CURLOPT_SAFE_UPLOAD - assumed 'CURLOPT_SAFE_UPLOAD' in /srv/mediawiki/php-1.27.0-wmf.7/includes/HttpFunctions.php on line 789

Error setting curl options.

@Krinkle,

I saw a bunch of those when I deployed Roan's first patch around Nov 13 17:37 (https://phabricator.wikimedia.org/T118032#1804036). Are you seeing more of these come in since?

@csteipp: yes, a ton more errors like that after deploying wmf.7. see T118988

I looks like the issue is I forgot to remove the original patch form /srv/patches/core when I undeployed it, and it went out in the next branch.

I'm still seeing the same error in fatalmonitor :-/

In T118032#1804265, @Catrope wrote:

In T118032#1804048, @csteipp wrote:

If I had paid more attention to the php docs, CURLOPT_SAFE_UPLOAD was added in php 5.5.0, an apparently not supported by our current HHVM install.

...even though our HHVM install claims to be PHP 5.6.99-hhhvm

Filed as https://github.com/facebook/hhvm/issues/6569 .

I guess it was just lag in fatalmonitor, the error count has dwindled now.

awight unsubscribed.Nov 18 2015, 11:29 PM

Krenair mentioned this in T119013: Phabricator unsubscribed me from a task?.Nov 19 2015, 1:12 AM

0001-Work-around-CURL-insanity-breaking-POST-parameters-t.patch4 KBDownload

This should fix the issue. Minor functionality change in that the request will have a different Content-Type, from multipart/form-data to application/x-www-form-urlencoded. I don't think that should be a problem, but something we'll want to watch out for.

T118032b.patch4 KBDownload

Prefix commit summary with SECURITY

22:13 csteipp: deployed patch for T118032

New patch looks like it's working ok.

• csteipp added a parent task: T115722: MediaWiki Security release 1.25.4.Nov 19 2015, 10:34 PM

• demon added subscribers: • demon, Grunny.Dec 17 2015, 1:18 AM

• demon changed the visibility from "Custom Policy" to "Public (No Login Required)".Dec 18 2015, 12:39 AM

• demon changed the edit policy from "Custom Policy" to "All Users".

Restricted Application changed the visibility from "Public (No Login Required)" to "Custom Policy". · View Herald TranscriptDec 18 2015, 12:39 AM

Restricted Application changed the edit policy from "All Users" to "Custom Policy". · View Herald Transcript

• demon changed the visibility from "Custom Policy" to "Public (No Login Required)".Dec 18 2015, 12:39 AM

• demon changed the edit policy from "Custom Policy" to "All Users".

• demon changed Security from Software security bug to None.

Change 259948 had a related patch set uploaded (by Chad):
SECURITY: Work around CURL insanity breaking POST parameters that start with '@'

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

gerritbot added a project: Patch-For-Review.Dec 18 2015, 12:40 AM

Change 259913 merged by Chad:
SECURITY: Work around CURL insanity breaking POST parameters that start with '@'

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

Change 259902 merged by Reedy:
SECURITY: Work around CURL insanity breaking POST parameters that start with '@'

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

Change 259937 merged by Chad:
SECURITY: Work around CURL insanity breaking POST parameters that start with '@'

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

Change 259926 merged by Reedy:
SECURITY: Work around CURL insanity breaking POST parameters that start with '@'

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

ReleaseTaggerBot added projects: MW-1.25-release, MW-1.24-release, MW-1.23-release, MW-1.26-release.Dec 18 2015, 2:00 AM

Change 259948 merged by jenkins-bot:
SECURITY: Work around CURL insanity breaking POST parameters that start with '@'

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

ReleaseTaggerBot added projects: MW-1.27-release (WMF-deploy-2016-01-12_(1.27.0-wmf.10)), MW-1.27-release-notes.Dec 18 2015, 10:00 AM

• csteipp updated the task description. (Show Details)Dec 24 2015, 12:02 AM

Restricted Application added a subscriber: Luke081515. · View Herald TranscriptDec 24 2015, 12:02 AM

• chasemp added a project: Security.Feb 10 2020, 10:50 PM

• chasemp removed a project: acl*security.Feb 20 2020, 8:11 PM

	F2961455: 0001-Work-around-CURL-insanity-breaking-POST-parameters-t.patch
	Nov 14 2015, 1:40 AM

	F2961094: 0001-Work-around-CURL-insanity-breaking-POST-parameters-t.patch
	Nov 13 2015, 10:04 PM

	F2924372: 0001-Work-around-CURL-insanity-breaking-POST-parameters-t.patch
	Nov 7 2015, 12:44 AM

	F3110761: omnibus-master.patch
	Dec 24 2015, 12:02 AM

	F3110764: omnibus-REL1_25.patch
	Dec 24 2015, 12:02 AM

	F2976920: T118032b.patch
	Nov 19 2015, 9:31 PM

Error thrown by VirtualRESTService when POST variable starts with '@'Closed, ResolvedPublicActions