[PHP-dev 1338] mb_check_encoding() のテスト(Re: [PHP-doc 658] Re: mbstring の新関数のマニュアルについて)

[komura]

komura です。

話題が php-doc ではないような気がしますので、php-dev に移動します。

On Thu, 21 Sep 2006 14:19:46 -0900
Rui Hirokawa <rui_hirokawa@...> wrote:

> >> mb_check_encoding()を引数なしで起動すると，リクエスト開始時から関数実行
> >> までに，不正にエンコーディンされた文字が1文字以上あった場合にfalse，
> >> それ以外の場合に，trueを返します。
> >> これは，入力の自動エンコーディング変換やmb_convert_variables()を使用する
> >> 場合を想定しています。

...

> >> ただし，エンコーディングの自動検出等で失敗したケースなどを想定すると，
> >> 攻撃の誤検出が予想され，テストも不十分なままとなっていました。
> >>
> >> きちんとテストケースとか書かないとだめですね。。。
> >
> > もし、必要でしたら何かお手伝いさせていただきます。
> >
>
> ありがとございます。
>
> 具体的なテスト仕様の策定作業にまだかかっていないのですが，例えば，
> POST/GET/COOKIEに経由で不正にエンコーディングされた文字を挿入した入力を
> 行い，
> 1.mb_get_info('illegalchars')で文字数を検出できるか
> 2.mb_check_encoding()でtrue/falseを正しく検出できるか
> 3.複数のリクエストを繰り返した時に上記の動作がリセットされるか
> を確認して頂けるとありがたいです。

簡単にではありますが、テストしてみました。
環境は Gentoo Linux 上でコンパイルした PHP 5.2.0RC4 です。

1. 不正文字数の数値がいくつであれば正しい検出ができたと言えるのかに
   ついては分かりませんが、mb_get_info( 'illegal_chars' ) で不正な文字数
   の検出はできました。

2. mb_convert_variables() を使用した後に mb_check_encoding() を使用すると、
   正しく true/false による判定ができました。

   問題は、mbstring.encoding_translation が有効になっている場合の自動変換
   なのですが、不正なエンコーディングと思われる値を入力しても、
   mb_check_encoding() 「引数なし」では検出されません。
   mb_get_info( 'illegal_chars' ) も 0 を返します。

   この件はテストに十分な時間が取れなかったような気もしますし、私の
   環境のせいかもしれません。他の方にもテストしていただいた方が良いと
   思います。

3. 複数リクエストを繰り返した場合、mb_get_info( 'illegal_chars' ) の値は
   リセットされ、次のリクエストの際に前の値が残るということはありません
   でした。


上記テスト中に気がついたのですが、mb_check_encoding() は UTF-8 の判定精度
が異常に悪いように感じました。
例えば、mb_check_encoding( "\x80\x80", "UTF-8" ) は、FALSE を返すと考えられ
ますが、実際には TRUE を返します。

以下、例です。

  $ php -v
  PHP 5.2.0RC4 (cli) (built: Sep 17 2006 22:45:06)
  Copyright (c) 1997-2006 The PHP Group
  Zend Engine v2.2.0, Copyright (c) 1998-2006 Zend Technologies

  $ php -r 'var_dump( mb_check_encoding( "\x80\x80", "UTF-8" ) ); '
  bool(true)

mb_detect_encoding() では、UTF-8 としては検出されませんでした。

  $ php -r 'var_dump( mb_detect_encoding( "\x80\x80", "UTF-8" ) ); '
  bool(false)


9月末は忙しいので、また時間ができたら、もう少しテストしてみたいと思います。

--
komura <komura@...>
_______________________________________________
PHP-dev mailing list
PHP-dev@...
http://ns1.php.gr.jp/mailman/listinfo/php-dev

[komura]

komura です。

前のメールで報告した以下の2点について検証してみました。
PHP 内部の動作自体にはそれほど詳しい訳ではありませんので、間違い等がある
かもしれません。

1. mbstring.encoding_translation が有効にして、文字コードの自動変換を行って
   いる場合、エンコーディングが不正な文字列を入力しても、検出されない

   PHP 4.4.4 の内部では、不正文字の判定は行われていましたが、ソースコード
   の ext/mbstring/mbstring.c を読んでみたところ、以下のような動作なって
   いることを確認しました。

   1) MBSTRING_API SAPI_TREAT_DATA_FUNC(mbstr_treat_data)
      mbstring.encoding_translation が有効になっている場合は文字コード
      を変換し、不正文字数を MBSTRG(illegalchars) に記録

   2) PHP_RINIT_FUNCTION(mbstring)
      MBSTRG(illegalchars) = 0; が実行される

   3) mb_check_encoding() を引数なしで使用すると、MBSTRG(illegalchars) が
      0 かどうかのみの判定を行っているため、TRUE を返す


2. mb_check_encoding( $str, "UTF-8" ) の判定精度の問題

   UTF-8 のフィルタ(ext/mbstring/libmbfl/filters/mbfilter_utf8.c) では、
   Shift_JIS やEUC-JP と比較して、不正文字の判定を厳密には行っていない
   ようです。

   このため、ほとんどの場合、mb_check_encoding( $str, "UTF-8" ) は
   TRUE を返すことが分かりました。


正しく以上の問題を解消できているのかは分かりませんが、PHP 4.4.4 と
PHP 5.2.0RC5 の Patch を作成してみましたので、添付します。
PHP 5.2.0RC5 の Patch は PHP 5.1.6 でも適用可能であることを確認しました。
機能改善の参考にでもなれば幸いです。

1. mbstring.encoding_translation が有効になっている場合は、
   MBSTRING_API SAPI_TREAT_DATA_FUNC(mbstr_treat_data) で
   MBSTRG(illegalchars) を初期化するようにしました。

   PHP 5.2.0RC5 では、mb_gpc.c で同等の処理が行われていますが、
   不正文字数をカウントしていなかったため、カウント処理を追加しました。

2. 不正文字数による判定ではなく、変換結果が変換文字列と同じかどうか
   で判定を行うように変更しました。
   PHP スクリプトで以下の処理を行うのと同等の処理になっているはずです。
   return $str === mb_convert_encoding( $str, $encoding, $encoding );


上記の Patch では、内部エンコーディングが UTF-8 で mb_check_encoding()
を引数なしで使用する場合、ほとんど効果がないという問題は解消できません。

--
komura <komura@...>

[php-4.4.4_mb_check_encoding.patch]

---

diff -ur php-4.4.4.orig/ext/mbstring/mbstring.c php-4.4.4/ext/mbstring/mbstring.c
--- php-4.4.4.orig/ext/mbstring/mbstring.c 2006-04-03 22:03:50.000000000 +0900
+++ php-4.4.4/ext/mbstring/mbstring.c 2006-10-07 10:29:26.204433000 +0900
@@ -1013,7 +1013,10 @@
  MBSTRG(current_http_output_encoding) = MBSTRG(http_output_encoding);
  MBSTRG(current_filter_illegal_mode) = MBSTRG(filter_illegal_mode);
  MBSTRG(current_filter_illegal_substchar) = MBSTRG(filter_illegal_substchar);
- MBSTRG(illegalchars) = 0;
+
+ if (!MBSTRG(encoding_translation)) {
+ MBSTRG(illegalchars) = 0;
+ }
 
  n = 0;
  if (MBSTRG(detect_order_list)) {
@@ -1774,6 +1777,8 @@
  info.num_from_encodings     = MBSTRG(http_input_list_size);
  info.from_language          = MBSTRG(language);
 
+ MBSTRG(illegalchars) = 0;
+
  detected = php_mbstr_encoding_handler(&info, array_ptr, res TSRMLS_CC);
  MBSTRG(http_input_identify) = detected;
 
@@ -3864,8 +3869,13 @@
 
  if (ret != NULL) {
  MBSTRG(illegalchars) += illegalchars;
- efree(ret->val);
- RETURN_BOOL(illegalchars == 0);
+ if (illegalchars == 0 && strncmp(string.val, ret->val, string.len) == 0) {
+ efree(ret->val);
+ RETURN_TRUE;
+ } else {
+ efree(ret->val);
+ RETURN_FALSE;
+ }
  } else {
  RETURN_FALSE;
  }

---

[php-5.2.0RC5_mb_check_encoding.patch]

---

diff -ur php-5.2.0RC5.orig/ext/mbstring/mb_gpc.c php-5.2.0RC5/ext/mbstring/mb_gpc.c
--- php-5.2.0RC5.orig/ext/mbstring/mb_gpc.c 2006-01-01 21:49:45.000000000 +0900
+++ php-5.2.0RC5/ext/mbstring/mb_gpc.c 2006-10-07 11:06:26.014433000 +0900
@@ -154,6 +154,8 @@
  info.num_from_encodings     = MBSTRG(http_input_list_size);
  info.from_language          = MBSTRG(language);
 
+ MBSTRG(illegalchars) = 0;
+
  detected = _php_mb_encoding_handler_ex(&info, array_ptr, res TSRMLS_CC);
  MBSTRG(http_input_identify) = detected;
 
@@ -346,6 +348,7 @@
  }
 
  if (convd != NULL) {
+ MBSTRG(illegalchars) += mbfl_buffer_illegalchars(convd);
  mbfl_buffer_converter_delete(convd);
  }
  if (val_list != NULL) {
diff -ur php-5.2.0RC5.orig/ext/mbstring/mbstring.c php-5.2.0RC5/ext/mbstring/mbstring.c
--- php-5.2.0RC5.orig/ext/mbstring/mbstring.c 2006-09-24 16:10:31.000000000 +0900
+++ php-5.2.0RC5/ext/mbstring/mbstring.c 2006-10-07 10:32:33.064433000 +0900
@@ -933,7 +933,10 @@
  MBSTRG(current_http_output_encoding) = MBSTRG(http_output_encoding);
  MBSTRG(current_filter_illegal_mode) = MBSTRG(filter_illegal_mode);
  MBSTRG(current_filter_illegal_substchar) = MBSTRG(filter_illegal_substchar);
- MBSTRG(illegalchars) = 0;
+
+ if (!MBSTRG(encoding_translation)) {
+ MBSTRG(illegalchars) = 0;
+ }
 
  n = 0;
  if (MBSTRG(detect_order_list)) {
@@ -4053,8 +4056,13 @@
 
  if (ret != NULL) {
  MBSTRG(illegalchars) += illegalchars;
- efree(ret->val);
- RETURN_BOOL(illegalchars == 0);
+ if (illegalchars == 0 && strncmp(string.val, ret->val, string.len) == 0) {
+ efree(ret->val);
+ RETURN_TRUE;
+ } else {
+ efree(ret->val);
+ RETURN_FALSE;
+ }
  } else {
  RETURN_FALSE;
  }

---

_______________________________________________
PHP-dev mailing list
PHP-dev@...
http://ml.php.gr.jp/mailman/listinfo/php-dev

[Rui Hirokawa]

廣川です。

レスポンスが遅くなり申し訳ありません。

動作確認および修正いただきありがとうございます。
コードの内容を確認し，PHP 5.2,PHP 4.4系のCVSにコミットしておきました。

komura wrote:
> komura です。
>
> 前のメールで報告した以下の2点について検証してみました。
> PHP 内部の動作自体にはそれほど詳しい訳ではありませんので、間違い等がある
> かもしれません。
>
_______________________________________________
PHP-dev mailing list
PHP-dev@...
http://ml.php.gr.jp/mailman/listinfo/php-dev