Shift_JIS では、htmlspecialchars() を使用しても XSS が可能な場合がある

以下のページに関連して、htmlspecialchars() を使用している場合でも XSS が可能かどうか少し調べてみました。

その結果、いくつかのブラウザで文字エンコーディングShift_JIS を使用していた場合、XSS が可能なことを確認しました。
テストコードは以下の通りです。リンクにマウスポインタを乗せると埋め込んだ Javascript が実行されます。

<?php

$_GET['a1'] = "\xf0";	// \xf0 - \xfc で可能
$_GET['a2'] = " href=dummy onmouseover=alert(document.title) dummy=dummy";

header( "Content-Type:text/html; charset=Shift_JIS" );
?>
<html>
<head><title>Shift_JIS test</title></head>
<body>
<p><a title="<?php echo htmlspecialchars( $_GET['a1'], ENT_QUOTES, 'SJIS' ) ?>" href="<?php echo htmlspecialchars( $_GET['a2'], ENT_QUOTES, 'SJIS' ) ?>">test</a></p>
</body>
</html>

上記のコードで XSS が発生するのは、以下が原因となるようです。

  1. PHP の htmlspecialchars() では、SJIS(Shift_JIS) の場合、\xf0 - \xfc を単独で指定しても排除しない(PHP 5.3.0 で確認)
  2. 一部のブラウザでは、\xf0 - \xfc に続く1バイトを合わせて Shift_JIS の1文字として認識する(Mozilla Firefox 3.5.3 と Internet Explorer 8(8.0.6001.18813) で確認)
  3. このため、上記の例では、\xf0 の後ろにある "(ダブルクォート) が有効にならず、その後ろに任意の属性が追加できる

このため、ブラウザによっては、XSS が可能な場合があります。
他のブラウザとして、Google Chrome 3.0.195.24 で確認してみましたが、\xf0 - \xfc の後ろに不正な文字が続く場合では、XSS は発生しませんでした。

追記(2009.10.06)

コメントで指摘いただいたように、Shift_JIS の先行バイトに関連する記述が正しくなかったため、表現を修正しました。

出力時の文字エンコーディング変換、妥当性確認について

大垣さんが以下のページで PHP について言及しておられますので、気になったことを書いておきたいと思います。

私は、現在、PHP で構築したサイト運用はしていませんので、出力時に文字エンコーディングを変換、妥当性確認する方法がどの程度有効で、どのような問題があるのかは十分把握していません。ある程度実施方法については書いておきますので、参考にされる方は、実用可能かどうかを十分検証してください。

出力時に文字エンコーディングを変換する方法

PHPで、似た様な動作にしたい場合(ブラウザ以外からの入力も怪しいなど)出力時に強制的に文字エンコーディング変換してしまえば良いです。例えば、

mbstring.internal_encoding = utf-8
mbstring.http_output = utf-8

と出力文字エンコーディングを指定すると不正な文字列はサニタイズされます。

http://blog.ohgaki.net/rails-ruby-1-9

これだけでは設定が不足しています。これだでは出力時に文字エンコーディングは変換されません。PHP マニュアルの以下に記述されている通り、出力時に文字エンコーディング変換をしたい場合は、以下の2つの設定が必要です。output_buffering は有効になっています(4096 などになっています)が、output_handler は設定する必要があります。

例2 php.ini の設定例

;; 全ての PHP ページで出力の文字エンコーディング変換を有効にする

;; 出力バッファリングを有効にする
output_buffering = On

;; mb_output_handler による出力変換を有効にする
output_handler = mb_output_handler

http://php.net/mbstring.http

mb_output_handler() では、Content-Type によって変換するかどうかを判定しています。全ての PHP スクリプトで文字エンコーディングの変換が行われるわけではありません。このため、以下の記述は誤りです。

ただし、http_outputをpass以外に設定すると副作用が発生するので注意が必要です。例えば、画像ファイルをUTF-8エンコーディングに変換すると確実に壊れます。

http://blog.ohgaki.net/rails-ruby-1-9

mb_output_handler() による、文字エンコーディング変換は、以下のようになっています。

出力時に文字エンコーディングを妥当性確認する方法

おそらく、元の記事だけでは、どうすれば良いのか分からないように思いますので、少し書いておきます。PHP スクリプト内で出力バッファリングを適用する場合は、以下のように、ob_start() を使用した後に出力します。

<?php
ob_start( 'mb_output_handler' );
// HTML の出力処理

mb_output_handler() は、mbstring が出力用に提供している関数で、mbstring.internal_encoding に設定した文字エンコーディングから mbstring.http_output に設定した文字エンコーディングに変換します。PHP が用意している出力用関数は、他にも ob_iconv_handler() や ob_gzhandler() などがあります。

ob_start() の引数は、コールバック関数で、この関数は PHP スクリプト内で定義することもできます。定義する関数には、第1引数に出力する値がそのまま渡ってきます。この関数内で、何か処理を行い、出力する文字列を返します。何も返さなかった場合、何も出力しないことになります。この関数内では、echo, print_r(), var_dump() などによる出力はできません。
今回の大垣さんの例では、以下のように、出力全体に対して mb_check_encoding() を適用し、不正な文字列が一文字でもあればエラー処理をする関数になっています。

<?php
function check_encoding_handler( $output )
{
    if ( ! mb_check_encoding( $output ) ) {
        // trigger_error('Invalid char encoding detected', E_USER_ERROR);
        return get_error_page(); // エラーページの HTML など
    }
    return $output;
}

ob_start( 'check_encoding_handler' );
// HTML の出力処理

注意点としては、ob_start() は複数回呼び出すことが可能です。このため、複数使用する場合、処理内容によっては、順番に注意してください。ob_start() は、入れ子構造で適用されますので、後で実行した方が先に適用されます。例えば、ob_gzhandler() を使用している場合は以下のようにします。

<?php
ob_start( 'ob_gzhandler' );
ob_start( 'check_encoding_handler' ); // 先に check_encoding_handler が適用されます。

この順番を反対にした場合、check_encoding_handler() では ob_gzhandler() で圧縮後のバイト列に適用することになります。このため、想定した結果にはなりません。

<?php
ob_start( 'check_encoding_handler' );
ob_start( 'ob_gzhandler' );           // 先に ob_gzhandler が適用されます。

現在の出力ハンドラ情報は、ob_list_handlers() や、ob_get_status() で取得できます。

詳しくは、PHP マニュアルの以下のページを参照してください

指定日の unixtime を求める方法

id:hnw さんの日記に以下のような投稿がありましたので、反応してみます。

特定の日時のunixtimeが知りたくなった時って皆さんどうしてますか?例えば2009/09/26 23:00:00 JSTであれば次のワンライナーで求められます。

$ php -r 'echo mktime(23,0,0,9,26,2009);'
1253973600

http://d.hatena.ne.jp/hnw/20090926

個人的は、普通に strtotime() を使う気がします。引数で悩む必要がありません。

php -r 'var_dump( strtotime( "2009-09-26 23:00:00" ) );'
int(1253973600)

最近の PHP では、DateTime クラスを使用した方が良いかもしれません。こちらは、strtotime() や mktime() よりも広い範囲を扱えます。

php -r '$d = new DateTime( "2009-09-26 23:00:00" ); var_dump( $d->format( "U" ) );'
string(10) "1253973600"

以下でも同じです。

php -r 'var_dump( date_format( date_create( "2009-09-26 23:00:00" ), "U" ) );'
string(10) "1253973600"

そういえば、以前、以下のような関数を作っていました。php.ini で timezone が設定されていない場合も想定しています。

<?php
function get_unixtime( $format = 'now', $timezone = 'Asia/Tokyo' )
{
    try {
        $datetime = new DateTime( $format, new DateTimeZone( $timezone ) );
        return $datetime->format( 'U' );
    }
    catch ( Exception $ex ) {
        return FALSE;
    }
}

文字エンコーディングの妥当性確認(バリデーション)について

大垣さんからコメントをいただきましたので、最後に追記しました(2009.09.22)。
少し時間が経ってしまいましたが、以下のページを読んで、PHP に関連する部分について思ったことを書きたいと思います。

私の理解が間違っていなければ、「Web アプリケーションで文字エンコーディングに関連する問題を無くす」ことを目的として、全ての Web アプリケーション開発者が以下を実行しようという主張だと思います。

  1. 全ての入力文字列の文字エンコーディングの妥当性を確認する
  2. 文字エンコーディングを厳格に取り扱う
  3. データベースなどで「バイナリ」に近い文字エンコーディングは利用しない

個人的には、PHP は文字エンコーディング関連の処理について、問題が多いと考えています。PHP 利用者がこれらの問題やバグまで理解して扱うのはさすがに無理があると思います。以前、この日記でも取り上げましたが、mb_check_encoding() だけでも PHP のバージョンによって相当な違いがあります。最新の PHP ではかなり改善されていますが、まだ問題は残っています(mb_check_encoding() の内部処理)。このため、単純に入力データを mb_check_encoding() で妥当性の確認をするだけでは、十分でない場合もあります。

文字エンコーディングについては、mbstring のような PHP の1モジュールで対応するのではなく、PHP 自体が文字エンコーディングを厳格に取り扱うようにした方が問題が少なくなると思います。PHP6 をスナップショットからダウンロードして試してみたところ、文字列を変数に代入すると、文字エンコーディングの妥当性が確認されるようになっていました。PHP6 の完成度を上げて早期にリリースするようにした方がこの問題の解決には早道かもしれません。PHP6 のリリースにはまだまだ時間がかかりそうですが。


以下に、大垣さん文書の中で気になった、または、よく分からなかったことを挙げておきたいと思います。

入力時の自動変換

以下の記述で、対策可能と書いておられますが、注意すべき点が3つありますので、書いておきたいと思います。

追記:「PHPで手っ取り早く対策するには?」と聞かれたので書いておきます。エントリ中にも書いていますが、UTF-8の場合は

mbstrign.strict_detetion = on
mbstring.http_input = auto
mbstring.internal_encoding = utf-8
mbstring.http_output = pass

とします。(strict_detectionを入れ忘れていたので追加しています)

http://blog.ohgaki.net/is-char-encoding-problem-difficult

(1) 間違える人は少ないと思いますが、上記の設定だけでは、入力時の自動変換は有効になりません。mbstring.encoding_translation = On が必要です。
(2) mbstring.http_input に auto や複数の文字エンコーディングを指定すると、最悪の場合は、変換されずにそのまま入力変数($_GET, $_POST など)に渡ってしまうことがあるようです(PHP 5.2.11 で確認)。入力値の不正な文字列を排除することが目的の場合、mbstring.http_input に指定するのは1つだけにした方が良いと思います。UTF-8 の場合は、以下の通りです。この場合、不正な文字列になっている部分のみ削除されます。

mbstring.internal_encoding = UTF-8
mbstring.http_input = UTF-8
mbstring.encoding_translation = On
mbstring.strict_detection = On

(3) PHP のバージョンと設定によっては、mbstring.strict_detection = On の指定は危険です。正確には、PHP 5.1.2 〜 PHP 5.2.8 では、mbstring.http_input に複数の文字エンコーディングを設定(auto の場合を含む)して、不正な文字列を変換しようとすると、無限ループが発生します。以下のコードで無限ループが発生する場合は、mbstring.strict_detection を Off にするか、mbstring.http_input に複数の文字エンコーディングを指定しないでください。

<?php
ini_set( 'mbstring.strict_detection', 1 );
mb_convert_variables( 'UTF-8', 'auto', $a = array( "\xfc" ) );

詳しく知りたい方は、以下を参照してください。

enctype="multipart/form-data" を指定した場合の自動変換

以下の部分についてです。

PHPの場合はPerlよりもっと簡単で、コードを一切書き換えなくてもphp.iniの設定をするだけで"自動"バリデーションが行えます。
と自分自身で書いていながら何故、文字エンコーディングのバリデーションが必用なのか?と疑問に思った方も居ると思います。PHPはファイルアップロードを行う場合にフォームに
enctype="multipart/form-data"
を指定すると入力エンコーディングに自動変換は動作しない仕様になっています。enctypeが違っても普通のフィールドも送信できます。もし文字エンコーディング変換を行うなら、これらのフィールドには明示的な文字エンコーディング変換が必用です。つまり、明示的なバリデーションが必用になります。

http://blog.ohgaki.net/is-char-encoding-problem-difficult

「enctype="multipart/form-data" を指定すると、入力エンコーディングで自動変換が動作しない仕様」と書いておられますが、PHP マニュアルには、以下のように記述されています。

注意: PHP 4.3.2 およびそれ以前のバージョンの場合、HTML フォームのenctype が multipart/form-data に設定された場合、mbstring は、POST データの文字エンコーディングを変換しません。この場合、文字列を内部文字エンコーディングに変換してやる必要があります。
PHP 4.3.3 以降、HTML フォームの enctype が multipart/form-data に設定され、かつ、php.ini において mbstring.encoding_translation に On が指定されている場合、POST データの変数とアップロードされたファイルの名前の文字エンコーディングは、内部文字エンコーディングに変換されます。ただし、クエリキーに関しては、変換されません。

http://php.net/mbstring.http

マニュアルでは、「PHP 4.3.3 以降では、php.ini で mbstring.encoding_translation = On に設定し、enctype="multipart/form-data" で HTML フォームを送ると、内部文字エンコーディングに変換する」と読めます。実際、試してみたところ、PHP 4.4.9 では、enctype="multipart/form-data" で HTML フォームを送っても、自動的に文字エンコーディングが内部エンコーディングに変換されました。仕様としては、マニュアルで正しいと思います。
ただし、PHP 5.2.11 や PHP 5.3.0 では、変換されませんでした。この件については、以下で報告されていましたが、結局修正されなかったようです。

mbstring がデフォルトモジュールになったのは、PHP 4.0.6 から

PHP 4.2からmbstringはデフォルトモジュールとなりました。デフォルトモジュールすべきと提案したのは私だったのでよく覚えています。 mbstringにはインプットを自動的に内部文字エンコーディング変換する機能とアウトプットに指定した文字エンコーディングを自動変換する機能を持っています。

http://blog.ohgaki.net/is-char-encoding-problem-difficult

mbstring は、4.0.6 で導入されたと記憶しています。PHP 4.2.0 では、mbregex(マルチバイト正規表現モジュール) が導入されました。

誤字・脱字について

大垣さんの文章に、以下のような記述がありました。

久しぶりにアマゾンのレビュー見ましたが、ほんと酷い評価(苦笑でもセキュリティ標準を読んだ事がある、せめて名前や内容を正しく解説できるWeb開発者はあたり前にいるとは思えません。誤字・脱字は自分でも見つけましたが、こういう本の評価に重要なのでしょうか?内容が正しいかどうかは技術者であれば判断できると思います。もしかしてコード監査で酷い目に合わせてしまった方なのかな?(汗

http://blog.ohgaki.net/char_encoding_must_be_validated

個人的には、本を買う際に誤字・脱字を結構気にする方なので、誤字・脱字が目立つ本はあまり買いません。これは、重要な部分にまで間違いがありそうで信用できなくなるためです。「内容が正しいかどうかは技術者であれば判断できる」とのことですが、勉強のために本を買う人に対してそれを期待するのは酷な気がします。
また、あまり人にえらそうに言えるた立場ではないのですが、大垣さんの文書は、誤字・脱字などの間違いが多いような気がします。誰でも、少しくらいは誤字・脱字があるものですし、あまり神経質になることはないと思うのですが、あまりに多いと文書全体が信用できなくなる気がします。それがどれだけ良い文書だったたとしても、読者から信用がないと、主張は伝わらないと思います。

コメントへの返信(2009.09.22)

大垣さんから以下のコメントをいただきましたが、少し長くなるのでこちらで返信します。

enctype="multipart/form-data" が指定された場合の自動変換について

mbstringにもいろいろ問題があるのは確かです。PHP4.4.9の動作はどうしてああなっているのか、単純な感違いで仕様を変えてしまっただけだと思います。mbstringは確かつかださんとおっしゃる方がphp3-i18n版をモジュール化して作った物です。その元のmbstringの仕様がmultipart/form-dataになっているとエンコーディング変換しなかったと記憶しています。PHP3がそのような仕様だったのかも知れません。ずいぶん前に変換しないことがバグとしてレポートされた場合に「この仕様どうする」という議論になってそのままにすることになったので、その経緯を知っている人にはバグです。(ご存知とは思いますがPHPにはこういう変更がちょくちょくあります)

過去の PHP-users や PHP-dev メーリングリストでの流れと、cvs.php.net のログを見ると、以下の2点が読み取れると思います。

  1. PHP 4.3.3 から 4.3.5 あたりで enctype="multipart/form-data" を指定した場合、内部エンコーディングに変換する機能を追加したこと
  2. php3-i18n 版でも enctype="multipart/form-data" が指定された場合、内部エンコーディングに自動変換されていたこと

今回、ファイルアップロードされたファイル名の文字コード
内部文字コードに変換するコードを追加していますので、
この辺をテストしていただけるとありがたいです。

http://ml.php.gr.jp/pipermail/php-users/2003-July/017633.html

multipart/form-dataの変数名を内部文字コードに変換するのは、
encoding_translation = On の時の動作として妥当なところだと思います。
また、PHP3-i18nの挙動と同等になったのではないかと思います。

http://ml.php.gr.jp/pipermail/php-users/2003-August/018109.html

後、PHP3-i18nで確認してみたところ、name属性に日本語を使った場合も
正常に変換しているようです。

http://ml.php.gr.jp/pipermail/php-users/2003-August/018123.html

cvs.php.net には、以下のようなログも残っています。

bug fixed: name parameter of multipart form was not converted into internal encoding when mbstring.encoding_translation is on.

http://cvs.php.net/viewvc.cgi/php-src/main/rfc1867.c?view=log#rev1.122.2.15

name/value in multipart/form-date will be converted into internal encoding when mbstring.encoding_translation is On.

http://cvs.php.net/viewvc.cgi/php-src/main/rfc1867.c?view=log#rev1.142

また、[PHP-dev 913]multipart/form-data時のencoding_translationの仕様についてから始まるスレッドも参考になります。
以上から、「enctype="multipart/form-data" が指定された場合、自動変換が行われるのは仕様である」という理解で正しいのではないかと思います。

mbstring が PHP に含まれるようになったバージョン

mbstringはPHP 4.0.xから使えたのですPHPプロジェクトのソース版に入ったのが4.2からです。

mbstring の実行時設定(PHP マニュアル) や、その他の関数でも確認できるように、PHP 4.0.6 から mbstring が使用できることが記述されています。また、museum.php.net から PHP 4.0.6 をダウンロードし、configure --help で確認したところ、--enable-mbstring オプションがありました。
cvs.php.net を確認しても PHP 4.0.6 のタグがありますので、mbstring が含まれるようになったのは PHP 4.0.6 からで間違いないと思います。

追記(2009.10.13)

誤字・脱字を修正しました。

PHP の svn Revision 287790 での修正で脆弱になったと言えるか

yohgaki さんから、以下のようなコメントをいただきましたので、少し調べてみました。
何か間違いや勘違いをしているような部分がありましたら、是非コメントでご指摘ください。

http://svn.php.net/viewvc/php/php-src/branches/PHP_5_2/ext/xml/xml.c?r1=287790&r2=287789&pathrev=287790

きちんと読んでないですが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 の場合、オーバーフローしてしまいます。このため、今回の修正が行われたように思います。

また、上記の関数(xml_utf8_decode)は、PHP の以下の関数使用時に呼び出されることがあります。

脆弱性があるかどうかの考察

上記のソースコードを見たところ、脆弱性となる可能性がある場所としては、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 はそのまま出力されるため、デコーダは定義されていません。

  • xml_decode_iso_8859_1
  • xml_decode_us_ascii

それぞれの関数の内容は以下のようになっています。関数の引数が 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) "?"

まとめ

  • 562 行目の unsigned short を unsigned int に修正したことによって、脆弱になったとは言えない
  • この修正は、不十分であり、デコーダの引数の型も合わせて修正すべきだった

utf8_decode() や xml_parse() は EUC-JP や Shift_JIS には対応しておらず、他に便利な関数などもありますので、最近、使用されることはあまりないと思います。とりあえず、入力文字列のエンコーディング確認、出力時の特殊文字列のエスケープを行うようにしておけば、セキュリティ的に問題になることはありません。

最近、PHP の関連でやったことなど

以前に書いた、UTF-16(BOM 付き Little Endian) を mb_convert_encoding() で変換すると文字列が壊れる件と、mb_detect_encoding() の挙動を調べていた際に見つけた問題について、Patch を PHP-dev のメーリングリストに送ってみました([PHP-dev 1488] mbstring の文字列変換、文字エンコーディング検出の関連の問題について)。しばらくして、反応がなかったら bugs.php.net に報告してみようかと思います。

その他、休みで少し時間があったので、PHP の mbstring に関するメモVim で PHP 関数の辞書を作成する方法についてのメモを少しだけ更新しました。PHP 5.3.0 に対する記述を追加したくらいです。

プライベートメソッドに対するテスト方法

phpunit などを使って PHP スクリプトをテストする時に、プライベートメソッドをテストしたくなる時があると思いますが、PHP では結構難しいように思います。
PHP でプライベートメソッドをテストする方法として、思い付くのは以下の方法くらいでしょうか。

  1. パブリックメソッドからプライベートメソッドを完全にテストできるように工夫する
  2. プライベートメソッドのテスト用にパブリックメソッドのラッパーを作成しておく
  3. テスト時のみ、テストしたいプライベートメソッドの Private を Public に書き換える
  4. Runkit などを使って定義を変更する

1. が実現できれば問題ないのですが、全てがうまくいくようにパブリックメソッドを作成するのは困難だと思います。また、テストのためだけにメソッドを増やしたり、変更したくないため、2. と 3. はあまりやりたくありません。4. であればできそうな気がしたのですが、Runkit では、定義済みのメソッドのアクセス定義のみを変更するようなことはできないようで、要件を満たすのは難しそうでした。

Java では、AccessibleObject を使用することで、フィールドやメソッドに対するアクセス制御ができます。PHP でも、PHP 5.3.0 から、ReflectionProperty クラスに setAccessible() が追加され、プロパティ(フィールド)へのアクセス制御ができるようになったのですが、ReflectionMethod クラスには、setAccessible() は実装されていません。
そこで、ReflectionMethod クラスに setAccessible() を実装してみることにしました。

PHP に対する Patch

以下のような Patch で実現することができました。PHP 5.3.0 に対する Patch です。PHP 5.2.10 以前のバージョンに適用できるかは確認していません。

diff -ur php-5.3.0.orig/ext/reflection/php_reflection.c php-5.3.0/ext/reflection/php_reflection.c
--- php-5.3.0.orig/ext/reflection/php_reflection.c	2009-06-16 23:33:33.000000000 +0900
+++ php-5.3.0/ext/reflection/php_reflection.c	2009-08-08 22:25:46.000000000 +0900
@@ -2945,6 +2945,28 @@
 }
 /* }}} */
 
+/* {{{ proto public int ReflectionMethod::setAccessible()
+   Sets whether non-public method can be requested */
+ZEND_METHOD(reflection_method, setAccessible)
+{
+	reflection_object *intern;
+	zend_function *mptr;
+	zend_bool accessible;
+
+	if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "b", &accessible) == FAILURE) {
+		return;
+	}
+	METHOD_NOTSTATIC(reflection_method_ptr);
+	GET_REFLECTION_OBJECT_PTR(mptr);
+
+	if (accessible) {
+		mptr->common.fn_flags |= ZEND_ACC_PUBLIC;
+	} else {
+		mptr->common.fn_flags &= ~ZEND_ACC_PUBLIC;
+	}
+}
+/* }}} */
+
 /* {{{ proto public static mixed ReflectionClass::export(mixed argument [, bool return]) throws ReflectionException
    Exports a reflection object. Returns the output if TRUE is specified for return, printing it otherwise. */
 ZEND_METHOD(reflection_class, export)
@@ -5072,6 +5094,10 @@
 	ZEND_ARG_ARRAY_INFO(0, args, 0)
 ZEND_END_ARG_INFO()
 
+ZEND_BEGIN_ARG_INFO(arginfo_reflection_method_setAccessible, 0)
+	ZEND_ARG_INFO(0, value)
+ZEND_END_ARG_INFO()
+
 static const zend_function_entry reflection_method_functions[] = {
 	ZEND_ME(reflection_method, export, arginfo_reflection_method_export, ZEND_ACC_STATIC|ZEND_ACC_PUBLIC)
 	ZEND_ME(reflection_method, __construct, arginfo_reflection_method___construct, 0)
@@ -5089,6 +5115,7 @@
 	ZEND_ME(reflection_method, invokeArgs, arginfo_reflection_method_invokeArgs, 0)
 	ZEND_ME(reflection_method, getDeclaringClass, NULL, 0)
 	ZEND_ME(reflection_method, getPrototype, NULL, 0)
+	ZEND_ME(reflection_method, setAccessible, arginfo_reflection_method_setAccessible, 0)
 	{NULL, NULL, NULL}
 };

PHP のテストコード

以下、PHP のテストコードです。

<?php

class Test
{
    public    function test1( $args ) { echo 'Public   :' . implode( ",", $args ). "\n"; }
    protected function test2( $args ) { echo 'Protected:' . implode( ",", $args ). "\n"; }
    private   function test3( $args ) { echo 'Private  :' . implode( ",", $args ). "\n"; }
}

function test( $method, $obj, $args )
{
    try {
        $method->invoke( $obj, $args );
    }
    catch ( Exception $e ) {
        echo "Error\n";
    }
}

$test = new Test();
$method1 = new ReflectionMethod( 'Test', 'test1' );
$method2 = new ReflectionMethod( 'Test', 'test2' );
$method3 = new ReflectionMethod( 'Test', 'test3' );
test( $method1, $test, array( 1, 2, 3 ) );
test( $method2, $test, array( 4, 5, 6 ) );
test( $method3, $test, array( 7, 8, 9 ) );

$method2->setAccessible( TRUE );
$method3->setAccessible( TRUE );
test( $method1, $test, array( 1, 2, 3 ) );
test( $method2, $test, array( 4, 5, 6 ) );
test( $method3, $test, array( 7, 8, 9 ) );

$method2->setAccessible( FALSE );
$method3->setAccessible( FALSE );
test( $method1, $test, array( 1, 2, 3 ) );
test( $method2, $test, array( 4, 5, 6 ) );
test( $method3, $test, array( 7, 8, 9 ) );

結果

Public   :1,2,3
Error
Error
Public   :1,2,3
Protected:4,5,6
Private  :7,8,9
Public   :1,2,3
Error
Error

以上で、ReflectionMethod クラスの setAccessible() を使ってプライベートメソッドへのアクセス制御ができるようになりました。これで、PHP スクリプトを修正せずに、プライベートメソッドのテストができそうです。
とりあえず、当初の目的はこれで達成できそうな気がするのですが、他の方がプライベートメソッドのテストをどのようにしているのか気になります。