php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Sec Bug #79383 ZipArchive::extractTo() sets excessive permissions by default
Submitted: 2020-03-14 19:19 UTC Modified: 2020-05-31 11:46 UTC
From: bigshaq at wearehackerone dot com Assigned:
Status: Not a bug Package: Zip Related
PHP Version: Irrelevant OS: Linux (Ubuntu)
Private report: No CVE-ID: None
View Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
If you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: bigshaq at wearehackerone dot com
New email:
PHP Version: OS:

 

 [2020-03-14 19:19 UTC] bigshaq at wearehackerone dot com
Description:
------------
I was reading some of the zip files here:
and stumbled across this line: https://github.com/php/php-src/blob/master/ext/zip/php_zip.c

[snippet]
ret = php_stream_mkdir(pathto, 0777,  PHP_STREAM_MKDIR_RECURSIVE, NULL);
[/snippet]

which creates the directory (provided in the extractTo parameter) with 777 permissions.

Moreover, during zip extraction(in php_zip_extract_file()): all of the directories are extracted with 777 permissions:
[snippet]
ret = php_stream_mkdir(file_dirname_fullpath, 0777,  PHP_STREAM_MKDIR_RECURSIVE|REPORT_ERRORS, NULL);
[/snippet]

Anything that is not a directory is extracted with 664 permission (which allows everyone to read, i.e: rw-rw-r-- )

The combination of all of these things allows a malicious user on the same linux server to read the extracted files (because of the 664 permissions) and list the directories' info because of the 777 permissions, which allows easy navigation and make the attack surface even bigger. 

Recommendation: I would suggest one of the following:
1. Remove public access by default. Change the permissions to 660 and let the programmer use chmod() by himself if he really want to share those extracted files with everyone. 
2. Alternatively, Set the default file permission to be a configureable thing (instead of hard-coded)






Test script:
---------------
<?php
$zip = new ZipArchive;
if ($zip->open('test.zip') === TRUE) {
    $zip->extractTo('/tmp/phpfuzz/outdir/');
    $zip->close();
    echo 'ok';
} else {
    echo 'failed';
}
?>

Expected result:
----------------
(notice the permissions)

drwxrwxr-x 3 ryanc ryanc 4096 Mar 14 11:26 outdir
  |
  |  drwxrwxr-x 3 ryanc ryanc 4096 Mar 14 11:26 directory1
          | -rw-rw---- 1 ryanc ryanc    4 Mar 14 11:26 secret1.txt
          |
          | drwxrwx--- 2 ryanc ryanc 4096 Mar 14 11:26 directory2
             | -rw-rw---- 1 ryanc ryanc    4 Mar 14 11:26 secret2_recursive.txt

Actual result:
--------------
(notice the permissions)

drwxrwxr-x 3 ryanc ryanc 4096 Mar 14 11:26 outdir
  |
  |  drwxrwxr-x 3 ryanc ryanc 4096 Mar 14 11:26 directory1
          | -rw-rw-r-- 1 ryanc ryanc    4 Mar 14 11:26 secret1.txt
          |
          | drwxrwxr-x 2 ryanc ryanc 4096 Mar 14 11:26 directory2
             | -rw-rw-r-- 1 ryanc ryanc    4 Mar 14 11:26 secret2_recursive.txt

Patches

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2020-03-14 19:33 UTC] bigshaq at wearehackerone dot com
-: bighsaq at wearehackerone dot com +: bigshaq at wearehackerone dot com
 [2020-03-14 19:33 UTC] bigshaq at wearehackerone dot com
Edit message: I had a typo with my email, Updating from bighsaq to bigshaq (had a typo with "sh" characters)
 [2020-03-14 19:40 UTC] bigshaq at wearehackerone dot com
-Summary: ZipArchive::extractTo() sets excessive permissions +Summary: ZipArchive::extractTo() sets excessive permissions by default
 [2020-03-14 19:40 UTC] bigshaq at wearehackerone dot com
updating title
 [2020-03-14 19:40 UTC] bigshaq at wearehackerone dot com
sorry, this is the expected result:

Expected result:
----------------
(notice the permissions)

drwxrwx--- 3 ryanc ryanc 4096 Mar 14 11:26 outdir
  |
  |  drwxrwx--- 3 ryanc ryanc 4096 Mar 14 11:26 directory1
          | -rw-rw---- 1 ryanc ryanc    4 Mar 14 11:26 secret1.txt
          |
          | drwxrwx--- 2 ryanc ryanc 4096 Mar 14 11:26 directory2
             | -rw-rw---- 1 ryanc ryanc    4 Mar 14 11:26 secret2_recursive.txt
 [2020-03-15 06:33 UTC] remi@php.net
I don't really think is a security issue, perhaps it needs some documentation.

extractTo honours umask, so this have to be set in the script.

umask(0027);
$z = new ZipArchive();
$z->open('ext/zip/tests/test.zip');
$z->extractTo('/tmp/foo');

ll /tmp/foo
-rw-r-----. 1 remi remi  4  4 août   2005 bar
-rw-r-----. 1 remi remi  8  6 juil.  2006 entry1.txt
drwxr-x---. 2 remi remi 60 15 mars  07:30 foobar

> 2. Alternatively, Set the default file permission to be a configureable thing (instead of hard-coded)

So it is already under the user control.
 [2020-03-15 06:36 UTC] stas@php.net
If it honors umask, it's not a bug, it's how it is supposed to work.
 [2020-03-15 06:42 UTC] stas@php.net
-Status: Open +Status: Not a bug
 [2020-03-15 06:42 UTC] stas@php.net
Thank you for taking the time to write to us, but this is not
a bug. Please double-check the documentation available at
http://www.php.net/manual/ and the instructions on how to report
a bug at http://bugs.php.net/how-to-report.php


 [2020-05-30 15:28 UTC] bigshaq at wearehackerone dot com
Hi, can I make a short blog post about this? just to raise the awareness (the report is private so I don't know if it's ok to disclose details outside of the bug tracker platform).

thanks!
 [2020-05-31 11:46 UTC] cmb@php.net
> […] can I make a short blog post about this?

Since the behavior is documented now, I don't see a problem with
that.
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Thu Dec 26 14:01:30 2024 UTC