From c793885b7624be4e2a95c69a2b8b3fee969b312f Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Mon, 2 Jul 2018 17:24:35 +0200 Subject: [PATCH] Fixed bug #74670 Validate that "C" serialization payload is followed by "}" prior to calling the unserialize() handler. This mitigates issues caused by unserialize() not correctly handling strings that are not NUL terminated. Making sure that there is a "}" at the end avoids the problem. --- NEWS | 4 ++ ext/gmp/bug74670.phpt | 10 +++ ext/gmp/tests/serialize.phpt | 2 +- ext/standard/tests/serialize/bug71311.phpt | 2 +- ext/standard/tests/serialize/bug73341.phpt | 4 +- ext/standard/var_unserializer.c | 74 ++++++++++++---------- ext/standard/var_unserializer.re | 12 +++- 7 files changed, 67 insertions(+), 41 deletions(-) create mode 100644 ext/gmp/bug74670.phpt diff --git a/NEWS b/NEWS index b2310d8c45..17c1b81781 100644 --- a/NEWS +++ b/NEWS @@ -15,6 +15,10 @@ PHP NEWS . Fixed bug #73342 (Vulnerability in php-fpm by changing stdin to non-blocking). (Nikita) +- GMP: + . Fixed bug #76470 (Integer Underflow when unserializing GMP and possible + other classes). (Nikita) + - intl: . Fixed bug #76556 (get_debug_info handler for BreakIterator shows wrong type). (cmb) diff --git a/ext/gmp/bug74670.phpt b/ext/gmp/bug74670.phpt new file mode 100644 index 0000000000..a28640b5c6 --- /dev/null +++ b/ext/gmp/bug74670.phpt @@ -0,0 +1,10 @@ +--TEST-- +Bug #74670: Integer Underflow when unserializing GMP and possible other classes +--FILE-- + +--EXPECTF-- +Notice: unserialize(): Error at offset 13 of 29 bytes in %s on line %d +bool(false) diff --git a/ext/gmp/tests/serialize.phpt b/ext/gmp/tests/serialize.phpt index 5e308fa14e..e9b9bb549f 100644 --- a/ext/gmp/tests/serialize.phpt +++ b/ext/gmp/tests/serialize.phpt @@ -18,7 +18,7 @@ try { } catch (Exception $e) { var_dump($e->getMessage()); } try { - unserialize('C:3:"GMP":8:{s:2:"42";}'); + unserialize('C:3:"GMP":9:{s:2:"42";}'); } catch (Exception $e) { var_dump($e->getMessage()); } ?> diff --git a/ext/standard/tests/serialize/bug71311.phpt b/ext/standard/tests/serialize/bug71311.phpt index 3c70783d37..70267c5b0b 100644 --- a/ext/standard/tests/serialize/bug71311.phpt +++ b/ext/standard/tests/serialize/bug71311.phpt @@ -2,7 +2,7 @@ Bug #71311 Use-after-free vulnerability in SPL(ArrayObject, unserialize) --FILE-- --EXPECTF-- diff --git a/ext/standard/tests/serialize/bug73341.phpt b/ext/standard/tests/serialize/bug73341.phpt index 55423217c3..86fd457c60 100644 --- a/ext/standard/tests/serialize/bug73341.phpt +++ b/ext/standard/tests/serialize/bug73341.phpt @@ -3,7 +3,7 @@ Bug #73144 (Use-afte-free in ArrayObject Deserialization) --FILE-- getMessage()."\n"; @@ -21,4 +21,4 @@ unserialize($exploit); Error at offset 6 of 7 bytes Notice: ArrayObject::unserialize(): Unexpected end of serialized data in %sbug73341.php on line %d -Error at offset 24 of 34 bytes \ No newline at end of file +Error at offset 24 of 34 bytes diff --git a/ext/standard/var_unserializer.c b/ext/standard/var_unserializer.c index 35c6160138..218a5c2273 100644 --- a/ext/standard/var_unserializer.c +++ b/ext/standard/var_unserializer.c @@ -1,4 +1,4 @@ -/* Generated by re2c 0.16 */ +/* Generated by re2c 1.0.1 */ #line 1 "ext/standard/var_unserializer.re" /* +----------------------------------------------------------------------+ @@ -477,6 +477,13 @@ static inline int object_custom(UNSERIALIZE_PARAMETER, zend_class_entry *ce) return 0; } + /* Check that '}' is present before calling ce->unserialize() to mitigate issues + * with unserialize reading past the end of the passed buffer if the string is not + * appropriately terminated (usually NUL terminated, but '}' is also sufficient.) */ + if ((*p)[datalen] != '}') { + return 0; + } + if (ce->unserialize == NULL) { zend_error(E_WARNING, "Class %s has no unserializer", ZSTR_VAL(ce->name)); object_init_ex(rval, ce); @@ -484,9 +491,8 @@ static inline int object_custom(UNSERIALIZE_PARAMETER, zend_class_entry *ce) return 0; } - (*p) += datalen; - - return finish_nested_data(UNSERIALIZE_PASSTHRU); + (*p) += datalen + 1; /* +1 for '}' */ + return 1; } static inline zend_long object_common1(UNSERIALIZE_PARAMETER, zend_class_entry *ce) @@ -603,7 +609,7 @@ static int php_var_unserialize_internal(UNSERIALIZE_PARAMETER) start = cursor; -#line 607 "ext/standard/var_unserializer.c" +#line 613 "ext/standard/var_unserializer.c" { YYCTYPE yych; static const unsigned char yybm[] = { @@ -661,9 +667,9 @@ static int php_var_unserialize_internal(UNSERIALIZE_PARAMETER) yy2: ++YYCURSOR; yy3: -#line 982 "ext/standard/var_unserializer.re" +#line 988 "ext/standard/var_unserializer.re" { return 0; } -#line 667 "ext/standard/var_unserializer.c" +#line 673 "ext/standard/var_unserializer.c" yy4: yych = *(YYMARKER = ++YYCURSOR); if (yych == ':') goto yy17; @@ -710,13 +716,13 @@ yy14: goto yy3; yy15: ++YYCURSOR; -#line 976 "ext/standard/var_unserializer.re" +#line 982 "ext/standard/var_unserializer.re" { /* this is the case where we have less data than planned */ php_error_docref(NULL, E_NOTICE, "Unexpected end of serialized data"); return 0; /* not sure if it should be 0 or 1 here? */ } -#line 720 "ext/standard/var_unserializer.c" +#line 726 "ext/standard/var_unserializer.c" yy17: yych = *++YYCURSOR; if (yybm[0+yych] & 128) { @@ -728,13 +734,13 @@ yy18: goto yy3; yy19: ++YYCURSOR; -#line 660 "ext/standard/var_unserializer.re" +#line 666 "ext/standard/var_unserializer.re" { *p = YYCURSOR; ZVAL_NULL(rval); return 1; } -#line 738 "ext/standard/var_unserializer.c" +#line 744 "ext/standard/var_unserializer.c" yy21: yych = *++YYCURSOR; if (yych <= ',') { @@ -984,7 +990,7 @@ yy62: goto yy18; yy63: ++YYCURSOR; -#line 611 "ext/standard/var_unserializer.re" +#line 617 "ext/standard/var_unserializer.re" { zend_long id; @@ -1009,7 +1015,7 @@ yy63: return 1; } -#line 1013 "ext/standard/var_unserializer.c" +#line 1019 "ext/standard/var_unserializer.c" yy65: yych = *++YYCURSOR; if (yych == '"') goto yy84; @@ -1020,13 +1026,13 @@ yy66: goto yy18; yy67: ++YYCURSOR; -#line 666 "ext/standard/var_unserializer.re" +#line 672 "ext/standard/var_unserializer.re" { *p = YYCURSOR; ZVAL_BOOL(rval, parse_iv(start + 2)); return 1; } -#line 1030 "ext/standard/var_unserializer.c" +#line 1036 "ext/standard/var_unserializer.c" yy69: ++YYCURSOR; if ((YYLIMIT - YYCURSOR) < 4) YYFILL(4); @@ -1046,7 +1052,7 @@ yy69: } yy71: ++YYCURSOR; -#line 714 "ext/standard/var_unserializer.re" +#line 720 "ext/standard/var_unserializer.re" { #if SIZEOF_ZEND_LONG == 4 use_double: @@ -1055,7 +1061,7 @@ use_double: ZVAL_DOUBLE(rval, zend_strtod((const char *)start + 2, NULL)); return 1; } -#line 1059 "ext/standard/var_unserializer.c" +#line 1065 "ext/standard/var_unserializer.c" yy73: yych = *++YYCURSOR; if (yych <= ',') { @@ -1077,7 +1083,7 @@ yy75: goto yy18; yy76: ++YYCURSOR; -#line 672 "ext/standard/var_unserializer.re" +#line 678 "ext/standard/var_unserializer.re" { #if SIZEOF_ZEND_LONG == 4 int digits = YYCURSOR - start - 3; @@ -1103,14 +1109,14 @@ yy76: ZVAL_LONG(rval, parse_iv(start + 2)); return 1; } -#line 1107 "ext/standard/var_unserializer.c" +#line 1113 "ext/standard/var_unserializer.c" yy78: yych = *++YYCURSOR; if (yych == '"') goto yy92; goto yy18; yy79: ++YYCURSOR; -#line 636 "ext/standard/var_unserializer.re" +#line 642 "ext/standard/var_unserializer.re" { zend_long id; @@ -1134,14 +1140,14 @@ yy79: return 1; } -#line 1138 "ext/standard/var_unserializer.c" +#line 1144 "ext/standard/var_unserializer.c" yy81: yych = *++YYCURSOR; if (yych == '"') goto yy94; goto yy18; yy82: ++YYCURSOR; -#line 824 "ext/standard/var_unserializer.re" +#line 830 "ext/standard/var_unserializer.re" { size_t len, len2, len3, maxlen; zend_long elements; @@ -1293,10 +1299,10 @@ yy82: return object_common2(UNSERIALIZE_PASSTHRU, elements); } -#line 1297 "ext/standard/var_unserializer.c" +#line 1303 "ext/standard/var_unserializer.c" yy84: ++YYCURSOR; -#line 755 "ext/standard/var_unserializer.re" +#line 761 "ext/standard/var_unserializer.re" { size_t len, maxlen; zend_string *str; @@ -1330,10 +1336,10 @@ yy84: ZVAL_STR(rval, str); return 1; } -#line 1334 "ext/standard/var_unserializer.c" +#line 1340 "ext/standard/var_unserializer.c" yy86: ++YYCURSOR; -#line 789 "ext/standard/var_unserializer.re" +#line 795 "ext/standard/var_unserializer.re" { zend_long elements = parse_iv(start + 2); /* use iv() not uiv() in order to check data range */ @@ -1357,7 +1363,7 @@ yy86: return finish_nested_data(UNSERIALIZE_PASSTHRU); } -#line 1361 "ext/standard/var_unserializer.c" +#line 1367 "ext/standard/var_unserializer.c" yy88: yych = *++YYCURSOR; if (yych <= ',') { @@ -1382,7 +1388,7 @@ yy91: goto yy18; yy92: ++YYCURSOR; -#line 813 "ext/standard/var_unserializer.re" +#line 819 "ext/standard/var_unserializer.re" { zend_long elements; if (!var_hash) return 0; @@ -1393,10 +1399,10 @@ yy92: } return object_common2(UNSERIALIZE_PASSTHRU, elements); } -#line 1397 "ext/standard/var_unserializer.c" +#line 1403 "ext/standard/var_unserializer.c" yy94: ++YYCURSOR; -#line 723 "ext/standard/var_unserializer.re" +#line 729 "ext/standard/var_unserializer.re" { size_t len, maxlen; char *str; @@ -1428,7 +1434,7 @@ yy94: ZVAL_STRINGL(rval, str, len); return 1; } -#line 1432 "ext/standard/var_unserializer.c" +#line 1438 "ext/standard/var_unserializer.c" yy96: yych = *++YYCURSOR; if (yych <= '/') goto yy18; @@ -1436,7 +1442,7 @@ yy96: goto yy18; yy97: ++YYCURSOR; -#line 698 "ext/standard/var_unserializer.re" +#line 704 "ext/standard/var_unserializer.re" { *p = YYCURSOR; @@ -1452,9 +1458,9 @@ yy97: return 1; } -#line 1456 "ext/standard/var_unserializer.c" +#line 1462 "ext/standard/var_unserializer.c" } -#line 984 "ext/standard/var_unserializer.re" +#line 990 "ext/standard/var_unserializer.re" return 0; diff --git a/ext/standard/var_unserializer.re b/ext/standard/var_unserializer.re index 5ff512bb68..221a03489a 100644 --- a/ext/standard/var_unserializer.re +++ b/ext/standard/var_unserializer.re @@ -481,6 +481,13 @@ static inline int object_custom(UNSERIALIZE_PARAMETER, zend_class_entry *ce) return 0; } + /* Check that '}' is present before calling ce->unserialize() to mitigate issues + * with unserialize reading past the end of the passed buffer if the string is not + * appropriately terminated (usually NUL terminated, but '}' is also sufficient.) */ + if ((*p)[datalen] != '}') { + return 0; + } + if (ce->unserialize == NULL) { zend_error(E_WARNING, "Class %s has no unserializer", ZSTR_VAL(ce->name)); object_init_ex(rval, ce); @@ -488,9 +495,8 @@ static inline int object_custom(UNSERIALIZE_PARAMETER, zend_class_entry *ce) return 0; } - (*p) += datalen; - - return finish_nested_data(UNSERIALIZE_PASSTHRU); + (*p) += datalen + 1; /* +1 for '}' */ + return 1; } static inline zend_long object_common1(UNSERIALIZE_PARAMETER, zend_class_entry *ce) -- 2.40.0