From 59d36bfcf2b3241843883f6f728e1b93224df12a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lauri=20Kentt=C3=A4?= Date: Wed, 25 May 2016 20:28:45 +0300 Subject: [PATCH 1/7] base64_decode: reorder to fix out of bounds read --- ext/standard/base64.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/ext/standard/base64.c b/ext/standard/base64.c index 81f826c9a8a..352e7ea52c9 100644 --- a/ext/standard/base64.c +++ b/ext/standard/base64.c @@ -143,16 +143,19 @@ PHPAPI zend_string *php_base64_decode_ex(const unsigned char *str, size_t length result = zend_string_alloc(length, 0); /* run through the whole string, converting as we go */ - while ((ch = *current++) != '\0' && length-- > 0) { + while (length-- > 0 && (ch = *current++) != '\0') { if (ch == base64_pad) { - if (*current != '=' && ((i % 4) == 1 || (strict && length > 0))) { - if ((i % 4) != 1) { - while (isspace(*(++current))) { - continue; - } - if (*current == '\0') { - continue; - } + if (i % 4 == 1) { + if (length == 0 || *current != '=') { + zend_string_free(result); + return NULL; + } + } else if (length > 0 && *current != '=' && strict) { + while (--length > 0 && isspace(*++current)) { + continue; + } + if (length == 0 || *current == '\0') { + continue; } zend_string_free(result); return NULL; From fbc74bb5f9df22a038a00f1f0e2437f1def81b93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lauri=20Kentt=C3=A4?= Date: Wed, 25 May 2016 21:15:52 +0300 Subject: [PATCH 2/7] base64_decode: remove redundant check If length == 0 || *current != '=' is false, the for loop will always end up in this same point, until the if statement becomes true. Thus, the if statement is not needed. --- ext/standard/base64.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/ext/standard/base64.c b/ext/standard/base64.c index 352e7ea52c9..e8d7f04aa4e 100644 --- a/ext/standard/base64.c +++ b/ext/standard/base64.c @@ -145,12 +145,13 @@ PHPAPI zend_string *php_base64_decode_ex(const unsigned char *str, size_t length /* run through the whole string, converting as we go */ while (length-- > 0 && (ch = *current++) != '\0') { if (ch == base64_pad) { + /* fail if the padding character is second in a group (like V===) */ + /* FIXME: why do we still allow invalid padding in other places in the middle of the string? */ if (i % 4 == 1) { - if (length == 0 || *current != '=') { - zend_string_free(result); - return NULL; - } - } else if (length > 0 && *current != '=' && strict) { + zend_string_free(result); + return NULL; + } + if (length > 0 && *current != '=' && strict) { while (--length > 0 && isspace(*++current)) { continue; } From 260c07db850266d2d65cff446ec98d4a4752d41c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lauri=20Kentt=C3=A4?= Date: Wed, 25 May 2016 21:02:41 +0300 Subject: [PATCH 3/7] base64_decode: fix bug #72152 (fail on NUL bytes in strict mode) This added check is actually for NOT failing in NON-strict mode. The ch == -2 check later causes the desired failure in strict mode. --- ext/standard/base64.c | 7 ++++++- ext/standard/tests/strings/bug72152.phpt | 11 +++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) create mode 100644 ext/standard/tests/strings/bug72152.phpt diff --git a/ext/standard/base64.c b/ext/standard/base64.c index e8d7f04aa4e..6c890e34fce 100644 --- a/ext/standard/base64.c +++ b/ext/standard/base64.c @@ -143,7 +143,12 @@ PHPAPI zend_string *php_base64_decode_ex(const unsigned char *str, size_t length result = zend_string_alloc(length, 0); /* run through the whole string, converting as we go */ - while (length-- > 0 && (ch = *current++) != '\0') { + while (length-- > 0) { + ch = *current++; + /* stop on null byte in non-strict mode (FIXME: is this really desired?) */ + if (ch == 0 && !strict) { + break; + } if (ch == base64_pad) { /* fail if the padding character is second in a group (like V===) */ /* FIXME: why do we still allow invalid padding in other places in the middle of the string? */ diff --git a/ext/standard/tests/strings/bug72152.phpt b/ext/standard/tests/strings/bug72152.phpt new file mode 100644 index 00000000000..440a90e0575 --- /dev/null +++ b/ext/standard/tests/strings/bug72152.phpt @@ -0,0 +1,11 @@ +--TEST-- +Bug #72152 (base64_decode $strict fails to detect null byte) +--FILE-- + Date: Wed, 25 May 2016 20:53:47 +0300 Subject: [PATCH 4/7] base64_decode: fix bug #72263 (skips char after padding) --- ext/standard/base64.c | 5 +++-- ext/standard/tests/strings/bug72263.phpt | 13 +++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) create mode 100644 ext/standard/tests/strings/bug72263.phpt diff --git a/ext/standard/base64.c b/ext/standard/base64.c index 6c890e34fce..ea548159c68 100644 --- a/ext/standard/base64.c +++ b/ext/standard/base64.c @@ -157,8 +157,9 @@ PHPAPI zend_string *php_base64_decode_ex(const unsigned char *str, size_t length return NULL; } if (length > 0 && *current != '=' && strict) { - while (--length > 0 && isspace(*++current)) { - continue; + while (length > 0 && isspace(*current)) { + current++; + length--; } if (length == 0 || *current == '\0') { continue; diff --git a/ext/standard/tests/strings/bug72263.phpt b/ext/standard/tests/strings/bug72263.phpt new file mode 100644 index 00000000000..d827af21ce1 --- /dev/null +++ b/ext/standard/tests/strings/bug72263.phpt @@ -0,0 +1,13 @@ +--TEST-- +Bug #72263 (base64_decode skips a character after padding in strict mode) +--FILE-- + Date: Wed, 25 May 2016 19:52:11 +0300 Subject: [PATCH 5/7] base64_decode: remove redundant code case 1 is already handled in the first lines of the for loop; it would only be entered in the invalid case where the string continues past the defined length (ch != 0 but length-- == 0). case 2 and case 3 are redundant, since k >= j and later the string is truncated to j characters anyway. --- ext/standard/base64.c | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/ext/standard/base64.c b/ext/standard/base64.c index ea548159c68..dc3e52071b1 100644 --- a/ext/standard/base64.c +++ b/ext/standard/base64.c @@ -136,8 +136,7 @@ PHPAPI zend_string *php_base64_decode(const unsigned char *str, size_t length) / PHPAPI zend_string *php_base64_decode_ex(const unsigned char *str, size_t length, zend_bool strict) /* {{{ */ { const unsigned char *current = str; - int ch, i = 0, j = 0, k; - /* this sucks for threaded environments */ + int ch, i = 0, j = 0; zend_string *result; result = zend_string_alloc(length, 0); @@ -197,19 +196,6 @@ PHPAPI zend_string *php_base64_decode_ex(const unsigned char *str, size_t length i++; } - k = j; - /* mop things up if we ended on a boundary */ - if (ch == base64_pad) { - switch(i % 4) { - case 1: - zend_string_free(result); - return NULL; - case 2: - k++; - case 3: - ZSTR_VAL(result)[k] = 0; - } - } ZSTR_LEN(result) = j; ZSTR_VAL(result)[ZSTR_LEN(result)] = '\0'; From 3380acbdd40f504b08b64c01960dbe8afc91d7ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lauri=20Kentt=C3=A4?= Date: Wed, 25 May 2016 22:06:42 +0300 Subject: [PATCH 6/7] base64_decode: fix bug #72264 ('VV= =' shouldn't fail in strict mode) --- ext/standard/base64.c | 34 ++++++++++++------------ ext/standard/tests/strings/bug72264.phpt | 7 +++++ 2 files changed, 24 insertions(+), 17 deletions(-) create mode 100644 ext/standard/tests/strings/bug72264.phpt diff --git a/ext/standard/base64.c b/ext/standard/base64.c index dc3e52071b1..d625dc0752a 100644 --- a/ext/standard/base64.c +++ b/ext/standard/base64.c @@ -136,7 +136,7 @@ PHPAPI zend_string *php_base64_decode(const unsigned char *str, size_t length) / PHPAPI zend_string *php_base64_decode_ex(const unsigned char *str, size_t length, zend_bool strict) /* {{{ */ { const unsigned char *current = str; - int ch, i = 0, j = 0; + int ch, i = 0, j = 0, padding = 0; zend_string *result; result = zend_string_alloc(length, 0); @@ -155,26 +155,26 @@ PHPAPI zend_string *php_base64_decode_ex(const unsigned char *str, size_t length zend_string_free(result); return NULL; } - if (length > 0 && *current != '=' && strict) { - while (length > 0 && isspace(*current)) { - current++; - length--; - } - if (length == 0 || *current == '\0') { - continue; - } - zend_string_free(result); - return NULL; - } + padding++; continue; } ch = base64_reverse_table[ch]; - if ((!strict && ch < 0) || ch == -1) { /* a space or some other separator character, we simply skip over */ - continue; - } else if (ch == -2) { - zend_string_free(result); - return NULL; + if (!strict) { + /* skip unknown characters and whitespace */ + if (ch < 0) { + continue; + } + } else { + /* skip whitespace */ + if (ch == -1) { + continue; + } + /* fail on bad characters or if any data follows padding */ + if (ch == -2 || padding) { + zend_string_free(result); + return NULL; + } } switch(i % 4) { diff --git a/ext/standard/tests/strings/bug72264.phpt b/ext/standard/tests/strings/bug72264.phpt new file mode 100644 index 00000000000..67dc0e9e5cc --- /dev/null +++ b/ext/standard/tests/strings/bug72264.phpt @@ -0,0 +1,7 @@ +--TEST-- +Bug #72264 (base64_decode $strict fails with whitespace between padding) +--FILE-- + Date: Tue, 5 Jul 2016 16:56:36 +0200 Subject: [PATCH 7/7] Add NEWS entries --- NEWS | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/NEWS b/NEWS index 327a4aed28b..9d94f6c6649 100644 --- a/NEWS +++ b/NEWS @@ -6,6 +6,14 @@ PHP NEWS . Fixed bug #72496 (Cannot declare public method with signature incompatible with parent private method). (Pedro Magalhães) +- Standard: + . Fixed bug #72152 (base64_decode $strict fails to detect null byte). + (Lauri Kenttä) + . Fixed bug #72263 (base64_decode skips a character after padding in strict + mode). (Lauri Kenttä) + . Fixed bug #72264 (base64_decode $strict fails with whitespace between + padding). (Lauri Kenttä) + 21 Jul 2016 PHP 7.0.9