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から参照することができます。以下に、おおまかな流れを図示します。まずはバ

[ockeghem]

日記書いた

[fukken]

追記で提示されているRasmusの報告が興味深い。バグレポートをテストという形で取り込むので、未解決のバグレポートが存在するとテストはその分失敗する、だから「本当のテスト失敗」を見逃すことになった、という経緯

[hide_o_55]

セキュリティ向上のために実施した静的解析が裏目に出たわけか。あの手のツールの警告は「問題無いとわかっていても残ってると気持ち悪い」と感じる人がいるし

[torin]

なんというあるあるバグ。やらかし記憶が次々と…

[quaoar]

memcpy()→strlcpy()→strcat()→ポインタ進めながら書き込み って何だこの一貫性の無さは。文字列終端を自前で管理するならそもそもstrcat()なんか使う必要ない。

[k-holy]

リリース直前の変更も、テストになってないテストコードも経験済な身としては「ないわー」とは言えない

[rryu]

そもそもmem系とstr系の関数を混ぜて使わなければ、mem系のみ＝起こらない、str系のみ＝区切りの$が無くなるだけで、こんなインパクトの強い結果にはならなかったのに。

[mas-higa]

こんなの PASS して当然だろ! と思って UnitTest 用意してないと痛い目にあうという例。/ と思ったら違ったみたい。普段から全て PASS しない前提だったらしい。そりゃ見落とすわ。

[tsucchi1022]

ひどいバグではあるけど、こうしてみるとやってしまいそうな「わりと普通な修正」ではあるな。テストが大事ですね。

[terurou]

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

[vivit_jc]

戒め

[a-kuma3]

make test がクリアになってない状況で、滑り込みの修正は駄目だよね。たとえ、単純な修正に見えても。【自戒】

[sidestepism]

ふむ

[kabiy]

徳丸浩の日記: PHP5.3.7のcrypt関数のバグはこうして生まれた

[rna]

静的解析ツールの警告に従って(本当は必要ないけど)コードを安全にしたつもりが引数の意味を勘違いして危険なバグを作ってしまった、単体テストはfailしてたのに気付かず(?)そのままリリースしてしまった、という話。

[tarchan]

＞「長い行を黙って切り詰める」関数

[s-tomo]

「passwdはstatic配列なので初期値は全て0('\0')」って二回目に呼ばれたときに違う動作になりそうな。というかC言語のcrypt()関数のラッパーじゃなかったのか.......。

[ebo-c]

リファクタリングでエンバグ

[nilab]

理想と現実。「テスト不足というのは明らかですが、その前段階の問題として、RC5というリリース直前の状態で不急の修正をしたことが大きな要因」徳丸浩の日記: PHP5.3.7のcrypt関数のバグはこうして生まれた

[mumincacao]

静的解析つーるの警告避けようとして迂回の迂回したら単体てすとで引っかかるとか・・・てすとこーども描くだけじゃだめだめっていい例になっちゃってるにう．．．〆（．．＿；【みかん

[uunfo]

テストを通らないのに公開されてしまったのか…。

[MagnesiumRibbon]

狼少年テスト

[youhey]

わかりやすい。小さな規模で似たようなケースが身近にゴロゴロ

[coolstyle]

とても丁寧な解説でわかりやすい。うっかりミスだけど、誰でもしそうなミスではある。

[tamtam3]

より安全な筈の関数strlcatに代替したつもりが、パラメーターの意味が異なったがゆえに、酷い目にあったでござるの巻

[hiragisan]

なるほど。「strncatとstrlcatでは、第3パラメータの意味が異なります」

[Kmusiclife]

笑えない。まじ笑えない。phpはかなり生活の中まで入り込んでる事がこれでわかった。

[zorio]

まさに他山の石

[kitaj]

分かりやすい解説

[Untouchable]

静的解析ツールの報告するエラーを、「潰せばいい」という気持ちで取り組んでしまっていたのだろうか。自分もこういうミスをやりそうで怖い

[teppeis]

エンバグの可能性はゼロにはできないけど、「テストがFAILしていたのに気づかなかった」という部分は改善できそうだと思った。普段からFAILゼロにしてないと気づけないよね。自戒。

[cubed-l]

こうしてまとめてくださる方がいるとソース読むのをサボってしまうな…自省しよう

[quwachy]

テストに通らなくてもコミットできる管理体制に驚愕した

[tgs]

だせぇｗ……と笑えるハナシじゃねーわ

[tsupo]

strlcat というのは strncat_s の類似品だよなぁ……

[vid]

あはははははははは、笑えねえよ！！　自分もやってしまいそうで怖い（－－；；

[todesking]

テスト落ちてたのにリリースされてしまったのか

[mohno]

「strncatとstrlcatでは、第3パラメータの意味が異なります」「GNU Cライブラリ (glibc) の開発者たちは、GNU Coding Standardsで禁じられている「長い行を黙って切り詰める」関数である…否定的な見解を示しており」