Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » [RFC] Fixing insecure cURL file uploading »

[RFC] Fixing insecure cURL file uploading

php.internals

Stas Malyshev

13 years ago
Hi! Following the recent discussion on the list, I've drafted an RFC describing the CurlFile solution for it here: https://wiki.php.net/rfc/curl-file-upload Please review and comment. If there's a general positive feedback, I'll try to implement a patch for it pretty soon.
-- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227

Alexey Zakhlestin

13 years ago
On 06.01.2013, at 6:38, Stas Malyshev <smalyshev@sugarcrm.com> wrote:
> Following the recent discussion on the list, I've drafted an RFC > describing the CurlFile solution for it here: > > https://wiki.php.net/rfc/curl-file-upload > > Please review and comment. If there's a general positive feedback, I'll > try to implement a patch for it pretty soon.
Looks elegant and extensible. Great work!
-- Alexey Zakhlestin https://github.com/indeyets

Martin Jansen

13 years ago
On 06.01.13 06:38, Stas Malyshev wrote:
> https://wiki.php.net/rfc/curl-file-upload > > Please review and comment. If there's a general positive feedback, I'll > try to implement a patch for it pretty soon.
Looks solid to me. One thing though: The manual spells the extension "cURL" and so does http://curl.haxx.se/. Shouldn't the new class then be called CURLFile instead of CurlFile? - Martin

Lars Strojny

13 years ago
Hi Stas, Am 06.01.2013 um 06:38 schrieb Stas Malyshev <smalyshev@sugarcrm.com>: [...]
> Following the recent discussion on the list, I've drafted an RFC > describing the CurlFile solution for it here: > > https://wiki.php.net/rfc/curl-file-upload > > Please review and comment. If there's a general positive feedback, I'll > try to implement a patch for it pretty soon.
Couldn’t CurlFile extend SplFileInfo? Otherwise it looks good. cu, Lars

Stas Malyshev

13 years ago
Hi!
> Couldn’t CurlFile extend SplFileInfo? Otherwise it looks good.
I don't see much reason for that. They are very different classes - CurlFile is not meant to provide information about the file, just keep the name, MIME type, etc. So most of the SplFileInfo API will be useless there.
-- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227

Levi Morrison

13 years ago
My objection is that we are introducing a class into an otherwise completely procedural API.

Pierrick Charron

13 years ago
Hi Stas, Everything looks good to me :) Great job. About your optional section : I like the procedural function that you proposed so that you don't have to use an object if you don't want to. cURL allow you to upload file from string buffer with CURLFORM_BUFFER and we should be able to do all the streams stuff with CURLFORM_STREAM and by modifying our CURLOPT_READFUNCTION. Pierrick On 6 January 2013 00:38, Stas Malyshev <smalyshev@sugarcrm.com> wrote:

Stas Malyshev

13 years ago
Hi!
> cURL allow you to upload file from string buffer with CURLFORM_BUFFER > and we should be able to do all the streams stuff with CURLFORM_STREAM > and by modifying our CURLOPT_READFUNCTION.
CURLFORM_STREAM has one issue - you can only have one read function, but you could have many uploaded files in the form. If we're willing to accept the limitation that only one uploaded file can be a stream and that you can not use both read function and stream file at the same time, then it will work. Otherwise cURL API wouldn't let us to distinguish between the functions. I'll start with implementing it without stream/buffer support, and then add it later.
-- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227

Stas Malyshev

13 years ago
Hi! I've added the pull request for the CURLFile here: https://github.com/php/php-src/pull/255 No procedural API yet, I'm not really sure if we need it, it's not that hard writing "new CurlFile()". But if needed I can add it there. Everybody please take a look and see if you notice any problems or missing stuff there. Tests are there and they pass for me :)
-- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227

Pierrick Charron

13 years ago
Looks good to me, just it could be great to add a new cURL option at the same time to disable the '@' usage so that someone working with the new ext/curl version can disable it and therefore send values starting by @ Pierrick On 7 January 2013 01:40, Stas Malyshev <smalyshev@sugarcrm.com> wrote:

Stas Malyshev

13 years ago
Hi!
> Looks good to me, just it could be great to add a new cURL option at > the same time to disable the '@' usage so that someone working with > the new ext/curl version can disable it and therefore send values > starting by @
That is a good suggestion, I'll add CURL_SAFE_POSTFIELDS which would disable the @ option.
-- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227

Pierrick Charron

13 years ago
Hi Stas, What's the status of this fix ? Thanks Pierrick On 8 January 2013 04:23, Stas Malyshev <smalyshev@sugarcrm.com> wrote:

Stas Malyshev

13 years ago
Hi!
> What's the status of this fix ?
The pull is in the RFC, so I planned to do the vote on Monday and then get it merged if nobody objects.
-- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227

Pierrick Charron

13 years ago
Great :) Thanks for the update On 17 January 2013 15:35, Stas Malyshev <smalyshev@sugarcrm.com> wrote: