php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Sec Bug #71923 integer overflow in ZipArchive::getFrom*
Submitted: 2016-03-29 23:45 UTC Modified: 2016-04-27 06:34 UTC
From: hji at dyntopia dot com Assigned: stas
Status: Closed Package: Zip Related
PHP Version: 7.0.5RC1 OS:
Private report: No CVE-ID: 2016-3078
 [2016-03-29 23:45 UTC] hji at dyntopia dot com
Description:
------------
An integer wrap may occur in PHP 7.x when reading zip files with the
getFromindex() and getFromName() methods of ZipArchive, resulting in a
heap overflow.

php-7.0.5/ext/zip/php_zip.c
,----
| 2679 static void php_zip_get_from(INTERNAL_FUNCTION_PARAMETERS, int type) /* {{{ */
| 2680 {
| ....
| 2684     struct zip_stat sb;
| ....
| 2689     zend_long len = 0;
| ....
| 2692     zend_string *buffer;
| ....
| 2702     if (type == 1) {
| 2703         if (zend_parse_parameters(ZEND_NUM_ARGS(), "P|ll", &filename, &len, &flags) == FAILURE) {
| 2704             return;
| 2705         }
| 2706         PHP_ZIP_STAT_PATH(intern, ZSTR_VAL(filename), ZSTR_LEN(filename), flags, sb);  // (1)
| 2707     } else {
| 2708         if (zend_parse_parameters(ZEND_NUM_ARGS(), "l|ll", &index, &len, &flags) == FAILURE) {
| 2709             return;
| 2710         }
| 2711         PHP_ZIP_STAT_INDEX(intern, index, 0, sb);                                      // (1)
| 2712     }
| ....
| 2718     if (len < 1) {
| 2719         len = sb.size;
| 2720     }
| ....
| 2731     buffer = zend_string_alloc(len, 0);                     // (2)
| 2732     n = zip_fread(zf, ZSTR_VAL(buffer), ZSTR_LEN(buffer));  // (3)
| ....
| 2742 }
`----

With `sb.size' from (1) being:

php-7.0.5/ext/zip/lib/zip_stat_index.c
,----
| 038 ZIP_EXTERN int
| 039 zip_stat_index(zip_t *za, zip_uint64_t index, zip_flags_t flags,
| 040                zip_stat_t *st)
| 041 {
| ...
| 043     zip_dirent_t *de;
| 044
| 045     if ((de=_zip_get_dirent(za, index, flags, NULL)) == NULL)
| 046         return -1;
| ...
| 063         st->size = de->uncomp_size;
| ...
| 086 }
`----

Both `size' and `uncomp_size' are unsigned 64bit integers:

php-7.0.5/ext/zip/lib/zipint.h
,----
| 339 struct zip_dirent {
| ...
| 351     zip_uint64_t uncomp_size;        /* (cl) size of uncompressed data */
| ...
| 332 };
`----

php-7.0.5/ext/zip/lib/zip.h
,----
| 279 struct zip_stat {
| ...
| 283     zip_uint64_t size;            /* size of file (uncompressed) */
| ...
| 290 };
`----

Whereas `len' is signed and has a platform-dependent size:

php-7.0.5/Zend/zend_long.h
,----
| 028 #if defined(__x86_64__) || defined(__LP64__) || defined(_LP64) || defined(_WIN64)
| 029 # define ZEND_ENABLE_ZVAL_LONG64 1
| 030 #endif
| ...
| 033 #ifdef ZEND_ENABLE_ZVAL_LONG64
| 034 typedef int64_t zend_long;
| ...
| 043 #else
| 044 typedef int32_t zend_long;
| ...
| 053 #endif
`----

Uncompressed file sizes in zip-archives may be specified as either 32-
or 64bit values; with the latter requiring that the size be specified in
the extra field in zip64 mode.

Anyway, as for the invocation of `zend_string_alloc()' in (2):

php-7.0.5/Zend/zend_string.h
,----
| 119 static zend_always_inline zend_string *zend_string_alloc(size_t len, int persistent)
| 120 {
| 121     zend_string *ret = (zend_string *)pemalloc(ZEND_MM_ALIGNED_SIZE(_ZSTR_STRUCT_SIZE(len)), persistent); // (4)
| ...
| 133     ZSTR_LEN(ret) = len;  // (5)
| 134     return ret;
| 135 }
`----

The `size' argument to the `pemalloc' macro is aligned/adjusted in (4)
whilst the *original* value of `len' is stored as the size of the
allocated buffer in (5).  No boundary checking is done in (4) and it may
thus wrap, which would lead to a heap overflow during the invocation of
`zip_fread()' in (3) as the `toread' argument is `ZSTR_LEN(buffer)':

php-7.0.5/Zend/zend_string.h
,----
| 041 #define ZSTR_LEN(zstr)  (zstr)->len
`----

On a 32bit system:

,----
| (gdb) p/x ZEND_MM_ALIGNED_SIZE(_ZSTR_STRUCT_SIZE(0xfffffffe))
| $1 = 0x10
`----

The wraparound may also occur on 64bit systems with `uncomp_size'
specified in the extra field (Zip64 mode; ext/zip/lib/zip_dirent.c:463).
However, it won't result in a buffer overflow because of `zip_fread()'
bailing on a size that would have wrapped the allocation in (4):

php-7.0.5/ext/zip/lib/zip_fread.c
,----
| 038 ZIP_EXTERN zip_int64_t
| 039 zip_fread(zip_file_t *zf, void *outbuf, zip_uint64_t toread)
| 040 {
| ...
| 049     if (toread > ZIP_INT64_MAX) {
| 050         zip_error_set(&zf->error, ZIP_ER_INVAL, 0);
| 051         return -1;
| 052     }
| ...
| 063 }
`----

php-7.0.5/ext/zip/lib/zipconf.h
,----
| 130 #define ZIP_INT64_MAX     0x7fffffffffffffffLL
`----

,----
| (gdb) p/x ZEND_MM_ALIGNED_SIZE(_ZSTR_STRUCT_SIZE(0xffffffffffffffff))
| $1 = 0x18
| (gdb) p/x ZEND_MM_ALIGNED_SIZE(_ZSTR_STRUCT_SIZE(0x7fffffffffffffff))
| $2 = 0x8000000000000018
`----


Test script:
---------------
CVE-2016-3078.py: https://gist.github.com/dyntopia/42ec435c6671dc2c2d3c2d05804432d0
upload.php: https://gist.github.com/dyntopia/003b30d86f176c343168982c56f92cca

Actual result:
--------------
On a 32bit machine running Arch Linux with php-fpm 7.0.5 behind nginx:

,----
| $ python CVE-2016-3078.py --bind-port 5555 http://1.2.3.4/upload.php
| [*] this may take a while
| [*] 103 of 4096 (0x67fd0)...
| [+] connected to 1.2.3.4:5555
| id
| uid=33(http) gid=33(http) groups=33(http)
| 
| uname -a
| Linux tmparch 4.4.5-1-ARCH #1 SMP PREEMPT Thu Mar 10 07:54:30 CET
| 2016 i686 GNU/Linux
| 
| pacman -Qs php-fpm
| local/php-fpm 7.0.5-1
|     FastCGI Process Manager for PHP
`----

-- Hans Jerry Illikainen



Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2016-04-18 00:33 UTC] stas@php.net
-Summary: CVE-2016-3078: integer overflow in ZipArchive::getFrom* +Summary: integer overflow in ZipArchive::getFrom* -CVE-ID: +CVE-ID: 2016-3078
 [2016-04-20 06:59 UTC] stas@php.net
In security repo 7dfb5076ad4bce2382d5c2eb31d8b103466f65db and in https://gist.github.com/smalyshev/00d7a252a1a2a720fefdf34dcdda1103. Please verify.
 [2016-04-20 07:00 UTC] stas@php.net
-Assigned To: +Assigned To: stas
 [2016-04-22 23:09 UTC] hji at dyntopia dot com
The patch fixes the issue.  However, there are currently >200
invocations of zend_string_alloc().  I haven't had time to go through
them, although a cursory view seems to suggest that many of them don't
rely on ZSTR_LEN.  Even so, the wraparound may potentially affect other
callers where `size' is user-controllable and where ZSTR_LEN is later
relied upon when dealing with the string.  As such, it may make sense
to avoid the erroneous condition in zend_string_alloc() too, eg.

https://gist.github.com/dyntopia/d373f15c60dc1a37c49daeae696bd542
 [2016-04-27 06:34 UTC] stas@php.net
-Status: Assigned +Status: Closed
 [2016-04-27 06:34 UTC] stas@php.net
The fix for this bug has been committed.

Snapshots of the sources are packaged every three hours; this change
will be in the next snapshot. You can grab the snapshot at
http://snaps.php.net/.

 For Windows:

http://windows.php.net/snapshots/
 
Thank you for the report, and for helping us make PHP better.


 [2016-04-27 16:11 UTC] ab@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=3b8d4de300854b3517c7acb239b84f7726c1353c
Log: Fix bug #71923 - integer overflow in ZipArchive::getFrom*
 [2016-07-20 11:31 UTC] davey@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=ccc12efa32f855e6057cb9b7e1e45afe08503a00
Log: Fix bug #71923 - integer overflow in ZipArchive::getFrom*
 
PHP Copyright © 2001-2017 The PHP Group
All rights reserved.
Last updated: Tue Aug 29 15:01:52 2017 UTC