ます’s Blog - どうでもいい記事100選

どうでもいい記事100選

mcryptで不正な長さのIVや鍵を渡した場合について（続き）

PHP patch internal

昨日の件ですが。ちょっとだけ深追い。しつこい。
shimookaさんが仰っている通り「イケてない実装」というのも確かにあるのですが、こうなっているからには理由があるハズ（理由も無しに実装するハズが無い）。。。と勝手に思い、背景を探るべく修正履歴を探してみました。ありました。
キー長を超えるサイズを渡した場合にPHP側で切り詰めて処理するようになったのは4.0.6からのようです（4.0.5・4.0.6）。結構前からなんですね。っていうか、Derick。。。＿|￣|○
バグ・データベースはコレが該当するようです。
変更差分の内容は以下の通り。

--- mcrypt.c	2001/05/17 21:48:10	1.50.2.1
+++ mcrypt.c	2001/05/18 20:55:45	1.50.2.2
@@ -439,7 +439,7 @@
 	zval **mcryptind;
 	unsigned char *key_s, *iv_s;
 	char dummy[256];
-	int max_key_size, iv_size;
+	int max_key_size, key_size, iv_size;
 	MCRYPT td;
 	int argc;
     MCLS_FETCH();
@@ -464,7 +464,10 @@
 	if (Z_STRLEN_PP(key) > max_key_size) {
 		sprintf (dummy, "key size too large; supplied length: %d, max: %d", 
 			Z_STRLEN_PP(key), max_key_size);
-		php_error (E_NOTICE, dummy);
+		php_error (E_WARNING, dummy);
+		key_size = max_key_size;
+	} else {
+		key_size = Z_STRLEN_PP(key);
 	}
 	memcpy (key_s, Z_STRVAL_PP(key), Z_STRLEN_PP(key));
 
@@ -475,7 +478,7 @@
 	}
 	memcpy (iv_s, Z_STRVAL_PP(iv), iv_size);
 
-	RETVAL_LONG (mcrypt_generic_init (td, key_s, Z_STRLEN_PP(key), iv_s));
+	RETVAL_LONG (mcrypt_generic_init (td, key_s, key_size, iv_s));
 	efree (iv_s);
 	efree (key_s);
 }

昔はsprintfでエラーメッセージを埋め込んでいたんですね。。。微妙。
当時のNEWSエントリは以下の通り。

--- NEWS	2001/05/19 07:47:37	1.661.2.4
+++ NEWS	2001/05/19 10:43:34	1.661.2.5
@@ -3,6 +3,9 @@
 
 ?? ??? 200?, Version 4.0.6
 - Fixed readfile/passthru losing resources during connection abort (Sascha)
+- Fixed bug in the mcrypt extension that caused segfaults when using a key
+  that is too large for the used algorithm, and a bug that caused
+  mcrypt_generic() to segfault PHP (Derick)
 - Fixed race in writing session files (Sascha)
 - Fixed a possible crash in the PHP CGI when no input file is
   specified (Zeev)

全ての過ちはここから始まった！？なんて。
この頃はセキュリティよりも使い易さを優先していた時代であった為、このように実装したのかな。。。と勝手に想像。
ただ、libmcrypt（本体）の地雷を踏んだようにも見えますね。。。libmcrypt-2.4.11のソースがダウンロードできないので想像の範囲ですが（バックトレースと修正履歴を見た限りでは）過去のlibmcrypt（本体）ではキー長をチェックしていなかったように思えます。
libmcrypt（本体）のNEWSファイルを見てみましたが、mcrypt_generic_init関数についての記述が一つだけ見つかりました。
ただ、内容的に違うっぽい。「Several fixes in modules」に含まれていたのだろうか。ソースが見る事が出来れば分かるんだけどなぁ。

May 17th 2001: (version 2.4.12)
- Updated the libltdl library included
- Several fixes in modules
- Added scripts to allow easier and faster library version detection

April 30th 2001: (version 2.4.11)
- Corrected memory leaks in mcrypt_module_close()

March 18th 2001: (version 2.4.10)
- Corrected bugs in blowfish and blowfish-compat

January 23th 2001: (version 2.4.9-beta)
- Due to an endianness handling problem Blowfish algorithm was not compatible 
  with other implementations. Now it has been corrected. If you want
  to access the old algorithm used use the "blowfish-compat" module.
- Fixes in mcrypt_list_algorithms() for some systems. Bugs pointed out by 
  Jonathan Woolmington <jwool@ind.tansu.com.au>
- Fixes in stream mode.
- mcrypt_generic_init() no longer fails if smaller key is used. It uses
  the most appropriate key size of the algorithm and pads with zeros.
- Fixes in wake algorithm (and support for IV).
- IV is now used in arcfour (arcfour-iv is now longer used). Speedups in Arcfour.
- mcrypt_generic_deinit() function added.

結局、何故そういう風に変更したのか、という経緯については未だ不明です。
何か理由があったのか、この方が良いと判断したのか、単に判断を誤ったのか、謎のママ。徒労に終わりそう。。。うーん。
この件については、エラーに変更するのは簡単ですが、長い間この状態でリリースされてきている為（後方互換性が崩れる為）、適用するのは難しいように思えます。
個人的には互換性を重視してチェックを強化するパラメータ（もしくはiniセクションかコンパイル・オプション）を追加したい気分ですが、なんとなく欧米の人は、その手の実装を嫌いそうな気がする。お国柄なんでしょうか（実装は一つだけ、みたいな）。
yandodさんの方でも、この件について調べていたようです。ちょっと遅かったか。。。申し訳ない。