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

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 -
  • 1.23 - included in
  • 1.24 - included in
  • 1.25 - included in
  • 1.26 - included in

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

Catrope created this task.Nov 6 2015, 9:10 PM
Catrope claimed this task.
Catrope changed the visibility from "Public (No Login Required)" to "Custom Policy".
Catrope changed the edit policy from "All Users" to "Custom Policy".
Catrope changed Security from None to Software security bug.

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
csteipp raised the priority of this task from "Normal" to "Unbreak Now!".EditedNov 6 2015, 11:04 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.


Updated patch that also patches CurlHttpRequest


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.


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 "@".

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.

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.

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

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.

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.

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 :-/

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 removed a subscriber: awight.Nov 18 2015, 11:29 PM

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.

Prefix commit summary with SECURITY

csteipp closed this task as "Resolved".Nov 19 2015, 10:27 PM

22:13 csteipp: deployed patch for T118032

New patch looks like it's working ok.

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

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

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

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

csteipp edited 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