From ec348a12d11a158a7f86c74ab1566fdae3a817aa Mon Sep 17 00:00:00 2001 From: Alex Dowad Date: Wed, 6 Dec 2023 06:11:39 +0200 Subject: [PATCH] Character indices used by mb_strpos and mb_substr have same meaning, even on invalid strings Starting many years ago, libmbfl included a 'mblen_table' for selected text encodings. This table allows looking up the byte length of a (possibly multi-byte) character from the value of the first byte. libmbfl uses these tables to optimize certain operations; if a text-processing operation can be performed using an mblen_table, it may not be necessary to decode the text to codepoints. Since libmbfl's decoding filters are generally slow, this improves performance. Since mbstring is (or was) based on libmbfl, it has always used these mblen_tables to implement some functions. This design has a significant downside. Let me explain: While some mbstring functions are implemented by converting input text to codepoints and operating on the codepoints, others operate directly on the original input bytes (using an mblen_table to identify character boundaries). Both of these implementation styles, if correctly coded, yield equivalent results on valid strings. However, on strings which contain encoding errors, the results are often different. When decoding byte strings to codepoints using some text encoding, mbstring uses the non-existent codepoint 0xFFFFFFFF to represent a byte sequence which cannot be decoded. Then, when mbstring indexes into the resulting sequence of codepoints, the index of any particular character depends on the number of such 'error markers' which were produced during the decoding process. In contrast, when an mblen_table is used to split a byte sequence into characters, there is no question of counting encoding errors; rather, table lookups into the mblen_table are used to repeatedly 'bite off' some number of bytes (which are treated as one 'character'). In the presence of encoding errors, these two methods of mapping between byte indices and character indices are inherently different and will rarely agree. (For completeness, it must be said that some internal mbstring code which operates only on UTF-8 text uses a third method for mapping between byte indices and character indices, that is: counting non-continuation UTF-8 bytes, which are all bytes whose binary representation is NOT like 0b10xxxxxx. This method happens to agree with the method which involves decoding the input text to codepoints and then counting the codepoints.) I have been aware of this issue for years, but only recently became aware that in the case of mb_strstr, mb_strpos, and mb_substr, this issue can cause seriously unintuitive behavior (and even security vulnerabilities). This was reported by Stefan Schiller. Stefan Schiller shared the following example for mb_strstr: var_dump(mb_strstr("\xf0start", "start", false, "UTF-8")); // string(2) "rt" Similarly, when mb_strpos and mb_substr are used to identify and extract a substring from a string with encoding errors, Stefan Schiller pointed out that the extracted portion may be completely different than desired. This is because (for UTF-8 strings) mb_strpos works by counting non-continuation bytes, but mb_substr uses an mblen_table. Since some mbstring functions *cannot* be implemented using an mblen_table, as long as mblen_tables are used, similar inconsistencies cannot be totally avoided. But the mblen_tables are critical to mbstring's performance. Or are they? Benchmarking mb_substr on various UTF-8, SJIS, and EUC-JP strings revealed something interesting. On all SJIS and EUC-JP test cases, mb_substr was slightly faster when the mblen_table based code was deleted. For some UTF-8 test cases, the mblen_table-based code was a tiny bit faster, while for others the fallback code was a touch faster; in no case was the difference significant. Therefore, the simple fix is to delete the mblen_table-based implementation of mb_substr. Aside from making the function behave consistently with other mbstring functions on invalid strings, there is ONE case where behavior is now different on valid strings: that is, on SJIS-Mac (MacJapanese) strings which contain any of the following code units: 0x85AB-0x85AD, 0x85BF, 0x85C0, 0x85C1, 0x8645, 0x864B, 0x865D, 0x869E, 0x86CE, 0x86D3-0x86D5, 0x86D6, 0x8971, 0x8792, 0x879D, 0x87FB, 0x87FC, 0xEB41, 0xEB42, 0xEB50, 0xEB5B, 0xEB5D, 0xEB60-0xEB6E, and all from 0xEB81 and above. All of these SJIS-Mac code units share the (very unusual) property that they do not correspond to any one Unicode codepoint. When converting from SJIS-Mac to Unicode, these must be converted to 2, 3, 4, or 5 codepoints each. The previous, mblen_table-based implementation of mb_substr would treat all of these SJIS-Mac byte sequences as 'one character'. Now, they are treated as multiple characters (one for each of the Unicode codepoints which they decode to). The new behavior is more consistent with other mbstring functions. I don't know if SJIS-Mac users will like this change or not (probably most will never notice), but the BC break is justified by the very real security impact of the previous, inconsistent behavior. Finally, I should comment on whether similar changes are needed elsewhere. The remaining functions which use an mblen_table are: mb_str_split, mb_strcut, and various search functions (such as mb_strpos). The search functions are only affected now when they receive a positive 'offset' parameter specifying where to start searching from. The search functions should definitely be fixed so they do not use an mblen_table to implement the 'offset' parameter. I am not convinced that there is any good reason to change mb_str_split and mb_strcut. --- ext/mbstring/mbstring.c | 46 ++++--------------------------- ext/mbstring/tests/mb_strstr.phpt | 9 +++++- ext/mbstring/tests/mb_substr.phpt | 39 ++++++++++++++++---------- 3 files changed, 38 insertions(+), 56 deletions(-) diff --git a/ext/mbstring/mbstring.c b/ext/mbstring/mbstring.c index a9f6cd6282922..405806604a0e4 100644 --- a/ext/mbstring/mbstring.c +++ b/ext/mbstring/mbstring.c @@ -38,6 +38,7 @@ #include "libmbfl/mbfl/mbfilter_wchar.h" #include "libmbfl/mbfl/eaw_table.h" #include "libmbfl/filters/mbfilter_base64.h" +#include "libmbfl/filters/mbfilter_cjk.h" #include "libmbfl/filters/mbfilter_qprint.h" #include "libmbfl/filters/mbfilter_htmlent.h" #include "libmbfl/filters/mbfilter_uuencode.h" @@ -2105,8 +2106,9 @@ static zend_string* mb_get_substr(zend_string *input, size_t from, size_t len, c unsigned char *in = (unsigned char*)ZSTR_VAL(input); size_t in_len = ZSTR_LEN(input); - if (from >= in_len) { - /* No supported text encoding decodes to more than one codepoint per byte + if (len == 0 || (from >= in_len && enc != &mbfl_encoding_sjis_mac)) { + /* Other than MacJapanese, no supported text encoding decodes to + * more than one codepoint per byte * So if the number of codepoints to skip >= number of input bytes, * then definitely the output should be empty */ return zend_empty_string; @@ -2127,30 +2129,6 @@ static zend_string* mb_get_substr(zend_string *input, size_t from, size_t len, c len = in_len; } return zend_string_init_fast((const char*)in, len); - } else if (enc->mblen_table) { - /* The use of the `mblen_table` means that for encodings like MacJapanese, - * we treat each character in its native charset as "1 character", even if it - * maps to a sequence of several codepoints */ - const unsigned char *mbtab = enc->mblen_table; - unsigned char *limit = in + in_len; - while (from && in < limit) { - in += mbtab[*in]; - from--; - } - if (in >= limit) { - return zend_empty_string; - } else if (len == MBFL_SUBSTR_UNTIL_END) { - return zend_string_init_fast((const char*)in, limit - in); - } - unsigned char *end = in; - while (len && end < limit) { - end += mbtab[*end]; - len--; - } - if (end > limit) { - end = limit; - } - return zend_string_init_fast((const char*)in, end - in); } return mb_get_substr_slow(in, in_len, from, len, enc); @@ -2343,21 +2321,7 @@ PHP_FUNCTION(mb_substr) size_t mblen = 0; if (from < 0 || (!len_is_null && len < 0)) { - if (enc->mblen_table) { - /* Because we use the `mblen_table` when iterating over the string and - * extracting the requested part, we also need to use it here for counting - * the "length" of the string - * Otherwise, we can get wrong results for text encodings like MacJapanese, - * where one native 'character' can map to a sequence of several codepoints */ - const unsigned char *mbtab = enc->mblen_table; - unsigned char *p = (unsigned char*)ZSTR_VAL(str), *e = p + ZSTR_LEN(str); - while (p < e) { - p += mbtab[*p]; - mblen++; - } - } else { - mblen = mb_get_strlen(str, enc); - } + mblen = mb_get_strlen(str, enc); } /* if "from" position is negative, count start position from the end diff --git a/ext/mbstring/tests/mb_strstr.phpt b/ext/mbstring/tests/mb_strstr.phpt index 4cdbb3df24839..85b0eeb19f404 100644 --- a/ext/mbstring/tests/mb_strstr.phpt +++ b/ext/mbstring/tests/mb_strstr.phpt @@ -26,6 +26,11 @@ var_dump(FROM_EUC_JP(mb_strstr(EUC_JP("あいうえおかきくけこ"), EUC_JP( var_dump(bin2hex(mb_strstr("\xdd\x00", "", false, 'UTF-8'))); var_dump(bin2hex(mb_strstr("M\xff\xff\xff\x00", "\x00", false, "SJIS"))); +// Test handling of invalid UTF-8 string +// Thanks to Stefan Schiller +var_dump(mb_strstr("\xf0start", "start", false, "UTF-8")); +var_dump(mb_strstr("\xf0start", "start", true, "UTF-8")); + ?> --EXPECT-- string(18) "おかきくけこ" @@ -36,5 +41,7 @@ string(12) "あいうえ" string(18) "おかきくけこ" string(18) "おかきくけこ" string(12) "あいうえ" -string(4) "dd00" +string(4) "3f00" string(2) "00" +string(5) "start" +string(1) "?" diff --git a/ext/mbstring/tests/mb_substr.phpt b/ext/mbstring/tests/mb_substr.phpt index ef37ca6dc192a..ad94c3b6069b1 100644 --- a/ext/mbstring/tests/mb_substr.phpt +++ b/ext/mbstring/tests/mb_substr.phpt @@ -118,6 +118,15 @@ print "3: " . mb_convert_encoding(mb_substr($utf7, -5, 3, 'UTF-7'), 'UTF-8', 'UT print "4: " . mb_convert_encoding(mb_substr($utf7, 1, null, 'UTF-7'), 'UTF-8', 'UTF-7') . "\n"; print "5:" . mb_convert_encoding(mb_substr($utf7, 10, 0, 'UTF-7'), 'UTF-8', 'UTF-7') . "\n"; +echo "Testing agreement with mb_strpos on invalid UTF-8 string:\n"; +/* Stefan Schiller pointed out that on invalid UTF-8 strings, character indices returned + * by mb_strpos would not extract the desired part of the string when passed to mb_substr. + * This is the test case which he provided: */ +$data = "\xF0AAA"; +$pos = mb_strpos($data, "<", 0, "UTF-8"); +$out = mb_substr($data, 0, $pos, "UTF-8"); +print $out . "\n"; + echo "Regression:\n"; /* During development, one >= comparison in mb_get_substr was wrongly written as > * This was caught by libFuzzer */ @@ -138,30 +147,30 @@ SJIS: 4: 967b8cea8365834c8358836782c582b781423031323334825482558256825782588142 5: -- Testing illegal SJIS byte 0x80 -- -6380 -806162 +633f +3f6162 SJIS-2004: -6380 -806162 +633f +3f6162 MacJapanese: 6380 806162 SJIS-Mobile#DOCOMO: -6380 -806162 +633f +3f6162 SJIS-Mobile#KDDI: -6380 -806162 +633f +3f6162 SJIS-Mobile#SoftBank: -6380 -806162 +633f +3f6162 -- Testing MacJapanese characters which map to 3-5 codepoints each -- 616263 -85ab85ac -85ac +3f3f +58 616263 -85bf85c0 -85c0 +3f3f +78 ISO-2022-JP: 1: 1b2442212121721b284241 2: 43 @@ -200,5 +209,7 @@ UTF-7: 3: йте 4: reek: Σὲ γνωρίζω ἀπὸ τὴν κόψη Russian: Зарегистрируйтесь 5: +Testing agreement with mb_strpos on invalid UTF-8 string: +?AAA Regression: 1b28493d3d3d3d3d3d3d3e3d3d3d1b28423f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f000000003f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f3f1b28493d3d3d3d3d3d3d3e1b2842013a4f1b28492a1b2842