From 2c4eb0ef6d3dd9513fa17f28777eb66153065754 Mon Sep 17 00:00:00 2001 From: Roan Kattouw Date: Fri, 6 Nov 2015 12:55:16 -0800 Subject: [PATCH] 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 --- 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 60196ab..05b97b5 100644 --- a/includes/HttpFunctions.php +++ b/includes/HttpFunctions.php @@ -781,7 +781,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'] !== '' ) { -- 1.9.1