php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login

go to bug id or search bugs for

Sec Bug #81211 Symlinks are followed when creating PHAR archive
Submitted: 2021-06-30 12:59 UTC Modified: 2021-08-26 20:59 UTC
From: trungpa at viettel dot com dot vn Assigned: stas (profile)
Status: Closed Package: PHAR related
PHP Version: 8.1Git-2021-06-30 (Git) OS: MacOS, Linux
Private report: No CVE-ID: None
View Developer Edit
 [2021-06-30 12:59 UTC] trungpa at viettel dot com dot vn 
Description:
------------
When constructing PHAR/TAR archive using PharData::buildFromDirectory and PharData::buildFromIterator, file paths returned by RecursiveIteratorIterator is validated to ensure that files are from inside the specified directory: https://github.com/php/php-src/blob/aff365871aec54c9a556d7667f131b8638d20194/ext/phar/phar_object.c#L1504
However, due to the check being too lenient, it can be bypassed with the following values:
- base:  /usr/foo
- fname: /usr/foobar
or
- base:  /tmp
- fname: /var/tmp

Test script:
---------------
<?php
mkdir('/tmp/usr');
mkdir('/tmp/usr/foobar');
mkdir('/tmp/usr/foo');

file_put_contents('/tmp/usr/foobar/file', 'this file should NOT be included in the archive!');
symlink('/tmp/usr/foobar/file', '/tmp/usr/foo/symlink');

$archive = new PharData('/tmp/usr/archive.tar');
$archive->buildFromDirectory('/tmp/usr/foo');
// $archive->buildFromIterator(new RecursiveDirectoryIterator('/tmp/usr/foo', FilesystemIterator::SKIP_DOTS), '/tmp/usr/foo');

var_dump(file_get_contents("phar:///tmp/usr/archive.tar/bar/file"));


Expected result:
----------------
UnexpectedValueException is thrown.

Fatal error: Uncaught UnexpectedValueException: Iterator RecursiveIteratorIterator returned a path "/tmp/usr/foobar/file" that is not in the base directory "/tmp/usr/foo"...

Actual result:
--------------
/tmp/usr/foobar/file (which is outside of the base directory /tmp/usr/foo) is included in archive.tar

Patches

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2021-07-01 12:59 UTC] cmb@php.net
-Assigned To: +Assigned To: stas
 [2021-07-01 12:59 UTC] cmb@php.net 
Thanks for reporting this issue!

Suggested patch for PHP-7.3 or PHP-7.4:
<https://gist.github.com/cmb69/46e4238e01a565603b4e25d1964547e9>.
 [2021-07-02 04:06 UTC] trungpa at viettel dot com dot vn 
The check will pass with fname=/foo/bar/fooxbar and base=/fooxbar.
I think we also need to ensure that fname starts with base.
Something like fname == strstr(fname, base).
https://gist.github.com/trungPa/71b557a2a775508b16119e9c21aade7d
 [2021-07-02 11:33 UTC] cmb@php.net 
> The check will pass with fname=/foo/bar/fooxbar and base=/fooxbar.

Oh, right!  I wonder why strstr() is used here at all.  A strncmp()
appears more suitable.  Even if fname is a relative path, base is
always expanded, so strstr() would fail anyway.

Then again, I think that fname needs to be normalized; otherwise
there are issues with slashes as directory separators on Windows,
which are usually supported.  But this is not really related to
the issue at hand.
 [2021-07-02 11:59 UTC] trungpa at viettel dot com dot vn 
Agree! strncmp() looks like a better alternative to strstr().
 [2021-08-23 06:29 UTC] stas@php.net 
@cmb do you want to update this patch with strncmp or leave it as is?
 [2021-08-23 11:44 UTC] cmb@php.net 
I've updated the patch to use strncmp() and to work for PHP 7.3:
<https://gist.github.com/cmb69/404cbe40a77e2e18d3331db5e9cc5092>.
 [2021-08-23 12:46 UTC] trungpa at viettel dot com dot vn 
@cmb would the test fail on linux because of the "\" on line #86-87? I remember having this problem and had to replace it with %s%ebug81211%efoobar%efile
 [2021-08-24 06:40 UTC] git@php.net 
Automatic comment on behalf of cmb69 (author) and smalyshev (committer)
Revision: https://github.com/php/php-src/commit/2ff853aa113e52637c85e28d1a03df1aa2d747b5
Log: Fix #81211: Symlinks are followed when creating PHAR archive
 [2021-08-24 06:40 UTC] git@php.net
-Status: Assigned +Status: Closed
 [2021-08-24 09:13 UTC] cmb@php.net 
> @cmb would the test fail on linux because of the "\" on line
> #86-87?

Right.  That has been fixed:
<https://github.com/php/php-src/commit/b815645aac76b494dc119fa6b88de32fa9bcccf1>.
 [2021-08-26 18:14 UTC] trungpa at viettel dot com dot vn 
Is there any plan to request a cve id for the issue?
 [2021-08-26 19:42 UTC] stas@php.net 
I'm not sure this needs a CVE - ultimately it is a local filesystem action which must be triggered by the user code. Granted, if the user is not careful it could lead to a security consequences (like adding files that should not be there to an archive) but this does not look like it needs a special notification.
 [2021-08-26 20:59 UTC] trungpa at viettel dot com dot vn 
This bug might cause problems in a local context. For example, an LPE, when a malicious local user has read/write permission to the input directory and the output archive, they can read files under the application context. But I agree that the possibility is low, the strstr(fname, base) also makes the bug limited and it's unlikely to be exploited remotely. I won't request an id from MITRE, so it's up to you to decide.
 [2021-09-20 19:39 UTC] 1234211qqq at gmail dot com 
a
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved. 		Last updated: Sat Dec 21 11:01:30 2024 UTC