$_FILES['name'] check

  108624
February 16, 2020 23:24 craig@craigfrancis.co.uk (Craig Francis)
Hi,

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

Craig
  108663
February 19, 2020 05:22 bishop@php.net (Bishop Bettini)
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
  108670
February 19, 2020 15:29 craig@craigfrancis.co.uk (Craig Francis)
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 Craig
  108672
February 19, 2020 16:41 bishop@php.net (Bishop Bettini)
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
  108673
February 19, 2020 16:49 craig@craigfrancis.co.uk (Craig Francis)
On Wed, 19 Feb 2020 at 16:42, Bishop Bettini <bishop@php.net> wrote:

> 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 >
Thanks again Bishop.