file upload implementation is flawed

Bug #46439

Submitted:

2008-10-31 19:18 UTC

Modified:

2013-01-29 06:29 UTC

Votes:	14
Avg. Score:	4.9 ± 0.3
Reproduced:	13 of 13 (100.0%)
Same Version:	11 (84.6%)
Same OS:	10 (76.9%)

From:

tom at punkave dot com

Assigned:

stas (profile)

Status:

Closed

Package:

cURL related

PHP Version:

5.*, 6CVS (2009-01-21)

OS:

Private report:

CVE-ID:

None

[2008-10-31 19:18 UTC] tom at punkave dot com

Description:
------------
PHP's cURL wrapper implements HTTP POST file uploads as follows:

curl_setopt($curl_handle, CURLOPT_POST, 1);
$args['file'] = '@/path/to/file';
curl_setopt($curl_handle, CURLOPT_POSTFIELDS, $args);

When ext/curl/interface.c sees that $args is an array and not a query-encoded string, it switches to a branch that uses CURLOPT_HTTPPOST rather than CURLOPT_POST. The code then checks for an '@' prefix in the value of every field. When an '@' is spotted, that particular field is treated as a file to be uploaded rather than a value to be sent as-is.

This implementation and the associated documentation have the following problems which are best described together for clarity's sake:

1. The fact that passing an array of arguments will trigger multipart/form-data is not documented. The documentation implies that you can use a query-encoded string or an array interchangeably. While most servers do accept multipart/form-data this is not a given. Also it is frequently the less efficient of the two encodings when files are not being uploaded.

2. When passing an array it is impossible to submit a form field value that does start with @. This is a bug in the implementation.

3. The documentation makes no mention of the '@ prefix means the rest of the value is a filename to be uploaded' issue. This is a serious security problem. PHP pages that transship form submissions from one site to another are being coded in ignorance of the fact that the '@' prefix could be used by end users to send any readable file on the first host to the second host. At a minimum, coders must check for and remove any @ prefix from user-submitted fields.

A recommended solution:

1. The '@ prefix for files, arrays trigger multipart/form-data' behavior should be controlled by a php.ini backwards compatibility option, hopefully defaulting off in the future.

2. CURLOPT_HTTPPOST and CURLOPT_HTTPPOSTFIELDS should be explicitly supported and documented as the correct way to do multipart/form_data, and

3. Instead of an @ prefix in the values of fields, CURLOPT_HTTPPOSTFILEFIELDS should be implemented to expressly pass an hash of keys => filenames.

It would work like this:

// I want a file upload with cURL
curl_setopt($curl_handle, CURLOPT_HTTPPOST, 1);
// Pass the non-file fields
curl_setopt($curl_handle, CURLOPT_HTTPPOSTFIELDS,
array("name" => "Joe Smith"));
// Pass the file fields
curl_setopt($curl_handle, CURLOPT_HTTPPOSTFILEFIELDS,
array("file" => "/path/to/file"));

HTTPPOST is a terrible, confusing name for multipart/form_data, but that's a cURL problem, not a PHP problem. (: With the above implementation at the PHP level we would at least have a correct wrapper for cURL on which friendlier classes could be correctly built.

Patches

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports

[2009-01-21 19:56 UTC] jani@php.net

It's security hole only if you don't filter the input..

[2009-01-21 20:08 UTC] tom at punkave dot com

htmlencode() won't escape @. Neither will htmlentities(). it's a security bug that no amount of reasonable prudence on the part of programmers who haven't read this particular bug report will address. And there is no reason why programmers would expect that filtering input would be necessary when they are passing individual fields to a function that ought to be ready to escape them (and in fact does, apart from the leading @ thing).

The documentation needs to be fixed at a minimum. It would be a much better idea to get rid of the broken behavior. The @ prefix is a bad idea (what if I want to pass @?) and with the current lack of documentation it's a security hole.

This needs to be patched or at least documented.

[2012-01-17 21:40 UTC] gmblar+php at gmail dot com

There is no function to escape the "@" in the CURLOPT_POSTFIELDS array and in this example 
its not possible to remove or replace the "@".

$curl = curl_init();
curl_setopt_array($curl, array(
    CURLOPT_URL            => 'http://www.example.com/',
    CURLOPT_RETURNTRANSFER => true,
    CURLOPT_POSTFIELDS     => array(
        'username'         => 'foobar',
        // Users may have strange passwords. Should be transfered as text.
        'password'         => '@/etc/hosts',
        // Upload image.
        'picture'          => '@/var/www/avatars/foobar.jpg'
    )
));
curl_exec($curl);
curl_close($curl);


My suggestion is to escape the password in this escape with \@ and then thread as text.

[2012-08-11 10:13 UTC] kristo at waher dot net

What if you are not actually trying to send a file and it's instead a POST value 
that starts with an @? What if you take user values from a website form and 
submit these values with post to another service and user writes '@config.php' 
to one of the form values? Problem here is that cURL in PHP treats the POST 
array the exact same way, any value could be a link to a file, which creates a 
huge loophole or a lot of extra work for you to filter through all POST values 
to make sure they are not pointers to files when used with cURL.

In fact, it is not possible to even make a POST request at the moment with cURL 
if one of the POST values starts with an @. There's no regular expression check, 
no formatting you'd have to follow, just a single (very common) character. What 
if you want to send Twitter handle and user writes @kristovaher?

In fact, it is so bad that you cannot even escape the character with \@, cURL 
will submit it without unescaping it. And if you don't have any control about 
the API on the other side (that would unescape it themselves), cannot make that 
POST request! You cannot make a POST API request to Twitter that is a reply to 
another user, for example.

I just wish PHP developers had the foresight to implement something like 
CURLOPT_FILEFIELDS in cURL, it's insane amount of double-validation I have to do 
in my API - that doesn't upload any files - at the moment just because of this 
potential security loophole. I love PHP, but these implementations are sometimes 
such a headache.

[2013-01-29 06:29 UTC] stas@php.net

The fix for this bug has been committed.

Snapshots of the sources are packaged every three hours; this change
will be in the next snapshot. You can grab the snapshot at
http://snaps.php.net/.

 For Windows:

http://windows.php.net/snapshots/
 
Thank you for the report, and for helping us make PHP better.

Fix as per RFC merged into 5.5.

[2013-01-29 06:29 UTC] stas@php.net

-Status: Open +Status: Closed -Assigned To: +Assigned To: stas