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 (profile)
Status: Closed Package: Zip Related
PHP Version: 7.0.5RC1 OS:
Private report: No CVE-ID: 2016-3078
Welcome back! If you're the original bug submitter, here's where you can edit the bug or add additional notes.
If you forgot your password, you can retrieve your password here.
Password:
Status:
Package:
Bug Type:
Summary:
From: hji at dyntopia dot com
New email:
PHP Version: OS:

 

 [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

Pull Requests

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-2024 The PHP Group
All rights reserved.
Last updated: Fri Nov 22 03:01:27 2024 UTC