PHP の svn Revision 287790 での修正で脆弱になったと言えるか
yohgaki さんから、以下のようなコメントをいただきましたので、少し調べてみました。
何か間違いや勘違いをしているような部分がありましたら、是非コメントでご指摘ください。
きちんと読んでないですがunsigned short だとオーバーフローするからintだそうです。だとするとデコードしたとき複数のバイトが"?"に変換されてしまうケースがあるように思えました。
脆弱性としてレポートされるのか？されないのか？
http://http://d.hatena.ne.jp/t_komura/comment?date=20090719#c
私の結論としては、この修正が直接の脆弱性になるとは思えませんが、問題がありそうな部分を見つけました。この件で調べたことを書いておきたいと思います。
教えていただいた脆弱性の可能性がある修正は以下の通りです。これは、PHP_5_2 だけでなく、PHP_5_3 や trunk でも修正されています。
--- php/php-src/branches/PHP_5_2/ext/xml/xml.c 2009/08/27 04:48:24 287789 +++ php/php-src/branches/PHP_5_2/ext/xml/xml.c 2009/08/27 05:05:42 287790 @@ -559,7 +559,7 @@ { int pos = len; char *newbuf = emalloc(len + 1); - unsigned short c; + unsigned int c; char (*decoder)(unsigned short) = NULL; xml_encoding *enc = xml_get_encoding(encoding);
修正コメントは以下の通りです。
This needs to be larger to avoid an overflow on the bit-shifting in this function
修正の概要
この修正の意図は、その下の 580 行目付近から始まる UTF-8 文字列のデコード処理のビットシフトで short 型の変数 c がオーバーフローしてしまうという問題の回避です。
c を int 型に修正することで、ビットシフト後の変数がオーバーフローすることを回避できます。
この関数全体は以下の通りです。
557: /* {{{ xml_utf8_decode */ 558: PHPAPI char *xml_utf8_decode(const XML_Char *s, int len, int *newlen, const XML_Char *encoding) 559: { 560: int pos = len; 561: char *newbuf = emalloc(len + 1); 562: unsigned int c; 563: char (*decoder)(unsigned short) = NULL; 564: xml_encoding *enc = xml_get_encoding(encoding); 565: 566: *newlen = 0; 567: if (enc) { 568: decoder = enc->decoding_function; 569: } 570: if (decoder == NULL) { 571: /* If the target encoding was unknown, or no decoder function 572: * was specified, return the UTF-8-encoded data as-is. 573: */ 574: memcpy(newbuf, s, len); 575: *newlen = len; 576: newbuf[*newlen] = '\0'; 577: return newbuf; 578: } 579: while (pos > 0) { 580: c = (unsigned char)(*s); 581: if (c >= 0xf0) { /* four bytes encoded, 21 bits */ 582: if(pos-4 >= 0) { 583: c = ((s[0]&7)<<18) | ((s[1]&63)<<12) | ((s[2]&63)<<6) | (s[3]&63); 584: } else { 585: c = '?'; 586: } 587: s += 4; 588: pos -= 4; 589: } else if (c >= 0xe0) { /* three bytes encoded, 16 bits */ 590: if(pos-3 >= 0) { 591: c = ((s[0]&63)<<12) | ((s[1]&63)<<6) | (s[2]&63); 592: } else { 593: c = '?'; 594: } 595: s += 3; 596: pos -= 3; 597: } else if (c >= 0xc0) { /* two bytes encoded, 11 bits */ 598: if(pos-2 >= 0) { 599: c = ((s[0]&63)<<6) | (s[1]&63); 600: } else { 601: c = '?'; 602: } 603: s += 2; 604: pos -= 2; 605: } else { 606: s++; 607: pos--; 608: } 609: newbuf[*newlen] = decoder ? decoder(c) : c; 610: ++*newlen; 611: } 612: if (*newlen < len) { 613: newbuf = erealloc(newbuf, *newlen + 1); 614: } 615: newbuf[*newlen] = '\0'; 616: return newbuf; 617: } 618: /* }}} */
入力文字列の検査対象文字(unsigned char)が、0xf0 以上(UTF-8 で 1 文字が 4〜6 バイト以上を表わす)の場合、583 行目で、18 ビットの左シフトしています。変数 c の型が unsigned short の場合、オーバーフローしてしまいます。このため、今回の修正が行われたように思います。
脆弱性があるかどうかの考察
上記のソースコードを見たところ、脆弱性となる可能性がある場所としては、609 行目のデコード処理になると思います。
ここで、decoder を定義しているテーブルは以下のようになっていました。
167: /* All the encoding functions are set to NULL right now, since all 168: * the encoding is currently done internally by expat/xmltok. 169: */ 170: xml_encoding xml_encodings[] = { 171: { "ISO-8859-1", xml_decode_iso_8859_1, xml_encode_iso_8859_1 }, 172: { "US-ASCII", xml_decode_us_ascii, xml_encode_us_ascii }, 173: { "UTF-8", NULL, NULL }, 174: { NULL, NULL, NULL } 175: };
以上から、定義される可能性のある関数は以下の通りです。UTF-8 はそのまま出力されるため、デコーダは定義されていません。
それぞれの関数の内容は以下のようになっています。関数の引数が unsigned short になっており、呼び出し元の型と一致しないという問題があるようです。
- xml_decode_iso_8859_1
467: /* {{{ xml_decode_iso_8859_1() */ 468: inline static char xml_decode_iso_8859_1(unsigned short c) 469: { 470: return (char)(c > 0xff ? '?' : c); 471: } 472: /* }}} */
- xml_decode_us_ascii
481: /* {{{ xml_decode_us_ascii() */ 482: inline static char xml_decode_us_ascii(unsigned short c) 483: { 484: return (char)(c > 0x7f ? '?' : c); 485: } 486: /* }}} */
この場合、これらの関数に値を渡すと、上位2バイト分が切り捨てられ、変数 c が 0xff 以下、0x7f 以下になる可能性があります。
このため、UTF-8 の 4 バイト以上の文字列が存在した場合、予期しない文字列が出力される可能性があります。
ただし、これは 562 行目の unsigned short を unsigned int に修正したことによって発生するようになった問題ではありますが、以前は、ビットシフト時に上位2バイトが切り捨てられていたものが、デコーダで切り捨てられるようになっただけです。このため、結果的には、以前のバージョンと同じ動作になっています。
今回の問題に対する Patch
今回の修正では、デコーダの引数も修正する必要があったように思います。十分には検証していませんが、patch を作成してみました。PHP 5.3.0 に対する Patch です。
diff -ur php-5.3.0.orig/ext/xml/php_xml.h php-5.3.0/ext/xml/php_xml.h --- php-5.3.0.orig/ext/xml/php_xml.h 2008-12-31 20:15:46.000000000 +0900 +++ php-5.3.0/ext/xml/php_xml.h 2009-08-30 23:03:19.000000000 +0900 @@ -92,7 +92,7 @@ typedef struct { XML_Char *name; - char (*decoding_function)(unsigned short); + char (*decoding_function)(unsigned int); unsigned short (*encoding_function)(unsigned char); } xml_encoding; diff -ur php-5.3.0.orig/ext/xml/xml.c php-5.3.0/ext/xml/xml.c --- php-5.3.0.orig/ext/xml/xml.c 2008-12-31 20:15:46.000000000 +0900 +++ php-5.3.0/ext/xml/xml.c 2009-08-30 23:03:17.000000000 +0900 @@ -73,9 +73,9 @@ static void xml_parser_dtor(zend_rsrc_list_entry *rsrc TSRMLS_DC); static void xml_set_handler(zval **, zval **); inline static unsigned short xml_encode_iso_8859_1(unsigned char); -inline static char xml_decode_iso_8859_1(unsigned short); +inline static char xml_decode_iso_8859_1(unsigned int); inline static unsigned short xml_encode_us_ascii(unsigned char); -inline static char xml_decode_us_ascii(unsigned short); +inline static char xml_decode_us_ascii(unsigned int); static zval *xml_call_handler(xml_parser *, zval *, zend_function *, int, zval **); static zval *_xml_xmlchar_zval(const XML_Char *, int, const XML_Char *); static int _xml_xmlcharlen(const XML_Char *); @@ -570,7 +570,7 @@ /* }}} */ /* {{{ xml_decode_iso_8859_1() */ -inline static char xml_decode_iso_8859_1(unsigned short c) +inline static char xml_decode_iso_8859_1(unsigned int c) { return (char)(c > 0xff ? '?' : c); } @@ -584,7 +584,7 @@ /* }}} */ /* {{{ xml_decode_us_ascii() */ -inline static char xml_decode_us_ascii(unsigned short c) +inline static char xml_decode_us_ascii(unsigned int c) { return (char)(c > 0x7f ? '?' : c); } @@ -664,8 +664,8 @@ { int pos = len; char *newbuf = emalloc(len + 1); - unsigned short c; - char (*decoder)(unsigned short) = NULL; + unsigned int c; + char (*decoder)(unsigned int) = NULL; xml_encoding *enc = xml_get_encoding(encoding); *newlen = 0;
結果は以下のようになります。
- Patch 適用前(ISO-8859-1 では不正な文字列だが、空白(" ")を出力)
$ php -r 'var_dump( utf8_decode( "\xf0\x90\x80\xa0" ) );' string(1) " "
- Patch 適用後(ISO-8859-1 では不正な文字列として、? を出力)
$ php -r 'var_dump( utf8_decode( "\xf0\x90\x80\xa0" ) );' string(1) "?"
