From e95721aae1daad4503eb23d87a114f363b324442 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 Signed-off-by: Chad Horohoe 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 e6801e3b43..5ede04f0da 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; + } elseif ( 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 c6fa9148e7..6ad77411dc 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 ); + } elseif ( 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.20.1