Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:108672
MIME-Version: 1.0
References: <CAFv4g+Ecztuzf_WZXzJ=PNK2R8nSND2mB9eWrR4Z7+5OaVb+hg@mail.gmail.com>
 <CAEYWF=74Atwth0yX5dxR3ZJypVaG1xeYZHz6eV6LoS7gBLkeVg@mail.gmail.com> <CAFv4g+HMoE7YMSP=CWBO2WxxKrRaF5Gf6pZcdy_G57UC74Wjrw@mail.gmail.com>
In-Reply-To: <CAFv4g+HMoE7YMSP=CWBO2WxxKrRaF5Gf6pZcdy_G57UC74Wjrw@mail.gmail.com>
Reply-To: bishop@php.net
Date: Wed, 19 Feb 2020 11:41:41 -0500
Message-ID: <CAEYWF=76-8r6csd_x5NARDmV=BxneeD1y0Cof+4LuXDDEsQP2Q@mail.gmail.com>
To: Craig Francis <craig@craigfrancis.co.uk>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="0000000000009fa553059ef0788d"
Subject: Re: [PHP-DEV] $_FILES['name'] check
From: bishop@php.net (Bishop Bettini)

--0000000000009fa553059ef0788d
Content-Type: text/plain; charset="UTF-8"

On Wed, Feb 19, 2020 at 10:29 AM Craig Francis <craig@craigfrancis.co.uk>
wrote:

> On Wed, 19 Feb 2020 at 05:23, Bishop Bettini <bishop@php.net> wrote:
>
>> On Sun, Feb 16, 2020 at 6:24 PM Craig Francis <craig@craigfrancis.co.uk>
>> wrote:
>>
>>> Just to check, at the moment, if I was an evil hacker, and was to run:
>>>
>>> curl -F 'file=@example.jpg;filename=../../../example.php'
>>> https://example.com/upload/
>>>
>>> The $_FILES['file']['name'] would be set to "example.php", where PHP has
>>> removed the leading "../../../" (good to see).
>>>
>>> Does that happen simply because of this IE fix, where it uses _basename()
>>> in the PHP source:
>>>
>>>
>>> https://github.com/php/php-src/blob/0b4778c377a5753a0deb9cfc697d4f62acf93a29/main/rfc1867.c#L1144
>>
>>
>> Mostly, it seems. _basename will either be php_ap_basename[1] or
>> php_mb_rfc1867_basename[2], and both of those handle the base name
>> functionality regardless of platform.
>>
>> The comment's a little misleading, though. The original implementation[3]
>> had a magic quotes check when compiled under WIN32, and that's what the
>> comment's talking about. The comment's not saying that the basename call
>> itself is for Windows only.
>>
>> [1]:
>> https://github.com/php/php-src/blob/0b4778c377a5753a0deb9cfc697d4f62acf93a29/main/rfc1867.c#L558
>> [2]:
>> https://github.com/php/php-src/blob/2e97ae91c8ac404be00050eef414b555aba45a1c/ext/mbstring/mbstring.c#L852
>> [3]:
>> https://github.com/php/php-src/blob/7ee1fdb657f2a6da65087552e6dda8cf2f4bd1ef/main/rfc1867.c#L1088
>>
>
>
>
> Thanks Bishop,
>
> That's interesting, so the comment probably should be updated.
>
> I don't think it matters where PHP is compiled, as I'm more focused on
> what the browser sends to the server.
>
> Personally I'd like the comment to mention the security value it provides,
> as I've seen a few systems that don't pass $_FILES["file"]["name"]
> though basename(); and if this behaviour was to change (e.g. when "IE's
> user base drops to nill"), that would introduce a problem.
>
>
> https://stackoverflow.com/questions/18929178/move-uploaded-file-function-is-not-working
>

I've updated this comment ([1]) to reflect that basename-ing is mandatory
for RFC 7857 multipart/form-data processing of filename parameters ([2]).

Thank you for helping improve PHP!

[1]:
https://github.com/php/php-src/commit/fb57ae9084a98ac5f06cd7b2d10205489b537e20
[2]:https://tools.ietf.org/html/rfc7578

--0000000000009fa553059ef0788d--