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 <chadh@wikimedia.org>
Change-Id: I3f996e2eb87c7bd3b94ca9d3cc14a3e12f34f241

diff --git a/includes/HttpFunctions.php b/includes/HttpFunctions.php
index e6801e3..5ede04f 100644 (file)
--- 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 c6fa914..6ad7741 100644 (file)
--- 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'] !== '' ) {