PHP5.3.7のcrypt関数のバグはこうして生まれた

昨日のブログエントリ「PHP5.3.7のcrypt関数に致命的な脆弱性(Bug #55439)」にて、crypt関数の重大な脆弱性について報告しました。脆弱性の出方が近年まれに見るほどのものだったので、twitterやブクマなどを見ても、「どうしてこうなった」という疑問を多数目にしました。
そこで、このエントリでは、この脆弱性がどのように混入したのかを追ってみたいと思います。

PHPのレポジトリのログや公開されているソースの状況から、PHP5.3.7RC4までこのバグはなく、PHP5.3.7RC5でこのバグが混入した模様です。RC5はPHP5.3.7最後のRelease Candidateですから、まさに正式リリースの直前でバグが入ったことになります。
バグの入る直前のソースは、ここの関数php_md5_crypt_rから参照することができます。以下に、おおまかな流れを図示します。まずはバグの入る前です。

上図の各処理の概要は以下の通りです。

1. passwdはstatic配列なので初期値は全て0('\0')
2. MD5を示すマジック「$1$」をpasswdの先頭にmemcpyでコピー
3. ソルトを定位置にstrlcpyでコピー
4. 現時点の文字列末尾に$をstrcatで追加
5. ハッシュ値を定位置に書き込み
6. 文字列の終端を示す'\0'を書き込み

このような状態で、ソースコードの静的解析が実施され、その結果、(4)のstrcatが指摘されたようです。strcatはバッファ長を指定できないので、データの内容によってはバッファオーバーフローの原因になります。それを指摘されたのでしょう。もっとも、この処理の場合はデータが固定長なので脆弱性の心配はないのですが、コミットログのコメントには以下のように書いてあります。

Make static analyzers happy

静的解析ツールの警告表示を消したかったのでしょうね。このため、以下の変更が行われました（差分）。

strcat(passwd, "$");

　　↓

strncat(passwd, "$", 1);

strncatは書き込む文字列の最大長を指定するstrcatの改良版です。この場合は1が指定されているので、最大1文字書き込み、その後に終端の'\0'を書き込みます。

この時点ではバグは入っていません。問題は、この後です。いったんstrncatに変更した箇所が、さらにstrlcatに変更されています。

strlcat関数はstrncatの改良版です。strncatは、追加する文字列の最大長を指定しますが、元の文字列長と追加文字列、さらに終端の'\0'のトータルの長さを意識しなければならないという点で使いにくいという問題があります。これに対して、strlcatはバッファ長を指定するので、文字列長の計算を呼び出し側で意識しなくてもよいという利点があります。バッファオーバーフロー対策としてはstrncatよりも確実です。

この変更点を以下に示します（差分）。

strncat(passwd, "$", 1);

　　↓

strlcat(passwd, "$", 1);

ここで不幸にもバグが入りました。strncatとstrlcatでは、第3パラメータの意味が異なります。strncatは、追加する文字列の最大長なので、1で問題ありません。一方、strlcatはバッファ長を指定するので、既に"$1$"とソルト（8文字）の都合11文字が入っているバッファに対して、バッファ長1を指定したことになり、「バッファは既にいっぱいなので文字を追加する余地はない」と解釈されます。その結果、"$"の書き込みは行われません。バッファはそのままの状態になります。
ここで、strlcatへの変更後の処理を以下に示します。(4)の部分が変更点です。

処理(5)と(6)でハッシュ値と文字列終端'\0'が書き込まれますが、その直前の箇所が'\0'のままです。このため、C言語の文字列としては、"$1$"とソルトだけでちぎれた状態になります。このため、肝心のハッシュが出力されないという結果になりました。

以上がBug #55439の混入した経緯です。

ところで、strlcpy/strlcatの仕様をWikipediaで確認していたところ、興味深い記述を見つけました。

一方で、GNU Cライブラリ (glibc) の開発者たちは、GNU Coding Standardsで禁じられている「長い行を黙って切り詰める」関数である、このような仕様の関数はバグである、いい加減なプログラムを助長してしまう、新たなセキュリティ問題を生む、など否定的な見解を示しており、標準規格に含まれない限りはglibcには実装しない意向である。

上記の意見には賛否両論あるところでしょうが、今回のケースでは、glibcの開発者達の予想が不幸にも的中してしまったことになります。

プロジェクトマネジメント上の問題としては、テスト不足というのは明らかですが、その前段階の問題として、RC5というリリース直前の状態で不急の修正をしたことが大きな要因だと考えます。

追記

今回の原因を作ったRasmus Lerdorf（PHP/FIのオリジナル開発者）がGoogle+で今回の経緯を説明しています。英語ですが、興味のある方はご覧下さい。私のエントリの修正は必要なさそうです。

追記2

今さらですが、PHP5.3.7のmake testを走らせたところ、ちゃんとテストでFAILしているのですね。

TEST 8080/8990 [ext/standard/tests/strings/crypt.phpt]FAIL crypt() function [ext/standard/tests/strings/crypt.phpt]

テスト(crypt.phpt)の内容を見ると非常に単純なテストであり単体レベルではもっと色々なパターンで試験すべきだと思います。しかし、せめてこのテストを単体テストで実施することと、テストしたことをコミット時に確認していれば、バグ流出は防げたと思います。