![ソフトウェア ソフトウェア](https://srad.jp/static/topics/software_64.png)
PHP 5.3.7に重大なバグ、strncatの罠にはまる 81
ストーリー by hylom
誰も気がつかなかったのか? 部門より
誰も気がつかなかったのか? 部門より
あるAnonymous Coward 曰く、
8月18日にリリースされたPHP 5.3.7であるが、 crypt()をMD5のsaltで実行した場合にsaltしか返さないという壮大すぎるバグが存在し、使用を控えるように案内が出ている。
問題になった箇所の変更はずばり こちらのコードっぽいが、 strcat(passwd, "$"); を strncat(passwd, "$", 1); に変更しただけのようだ。おそらく、size指定の意味を勘違いしていたのだろう。
strncatのsizeの罠にはまらないようにしましょう。
バグの原因については、徳丸浩氏がブログにて詳細に解説しているのでそちらもご参照を。なお、すでに問題を修正したPHP 5.3.8がリリースされている。
ま、PHPだから (スコア:3, 興味深い)
crypy()を利用する人が意外に多かったのでしょうね、きっと。
PHPに限らず、実用システムでオープンソースを利用する場合の
リスクの一例ということで。
バグを入れちゃうのは仕方ないにしても (スコア:2, すばらしい洞察)
恐るべき事に、PHPというソフトは単体テストすら回さずにリリースが行われているということですよね?
信じられないし、まともな神経を持った開発者ならあり得ない暴挙です。責任者が誰か知りませんが、ソフトをリリースする資格ないですよ。
PHPは好きでも嫌いでもなかったんですが、こんな連中が作ってる代物だと知れた以上は、今後は出来る限り使用を避けようと思います。周りを説得する強力な材料にもなりましたし。
Re:バグを入れちゃうのは仕方ないにしても (スコア:4, 参考になる)
Rasmus Lerdorfさん曰く [google.com]、てけとーに要約すると
って感じですかね。継続テスト、コードカバレッジツールも宝の持ち腐れだったんですね。
Re:バグを入れちゃうのは仕方ないにしても (スコア:4, すばらしい洞察)
静的解析ツール黙らせてる暇あったら単体テストをFAILゼロにするのが先だろ、と思ったのは俺だけじゃないよな……
Re:バグを入れちゃうのは仕方ないにしても (スコア:1)
Re: (スコア:0)
Microsoftのstrcat_s (スコア:2, 興味深い)
ここまで誰も話題にしないMicrosoftのCRT 関数のセキュリティが強化されたバージョン [microsoft.com]のstrcat_s。strlcatと違って黙って切り捨てることはない(そういう場合、デフォルトでは即座にプロセスごと死ぬ)ので、こっちだったら良かったのかなと思う。まだMSのコンパイラくらいでしか使えないけど。
これベースにISO/IEC TR 24731(参考:STR07-C. TR 24731 を使用し、文字列操作を行う既存のコードの脅威を緩和する [jpcert.or.jp])なんてのも出ているので、C1X(C99の次)あたりに搭載されないか?
Re: (スコア:0)
> (そういう場合、デフォルトでは即座にプロセスごと死ぬ)ので、こっちだったら良かったのかなと思う。
サーバを乗っ取られるよりはマシでしょうけどApacheが丸ごと落ちるのはちょっと…。
Re:Microsoftのstrcat_s (スコア:1)
Apache httpd なら、基本的に落ちるのは子プロセスだけでは?
# さすがに親プロセスレベルで落ちるのはリリース品質的にありえなさすぎるし。
Re: (スコア:0)
PHPは昔からですね (スコア:2, 興味深い)
当方が勤めている小社では、お客様が「PHP」と言ってもお断りしますし、
PHPで作られたシステムの改修案件の場合は、
設立当初からPHPの危険性を説いて、駄目であればお断りしています。
なぜわざわざstrncat (スコア:1)
直前で strlcpy を使ってるんだから、strlcat すればいいのに。
ちょっと前を見れば static char passwd[MD5_HASH_MAX_LEN] ってあるように
サイズもわかってるんだから。
Re:なぜわざわざstrncat (スコア:5, 参考になる)
違うよ。strncatに書き換えた時点では問題なかった。その後で [php.net]さらにstrlcatに書き換えたのに、第3引数をそのままにしてたからバグったの。
参考: PHP5.3.7のcrypt関数のバグはこうして生まれた [tokumaru.org]
Re: (スコア:0)
まあタイトルがわかりづらいよね。
どっちかっていうと"strlcatの罠にはまった"のであって。
記事の内容も微妙に語弊があるかな。
Re: (スコア:0)
コードの意味理解しない馬鹿を黙らせるためだけに、必要のない変更を行い、そこでエンバグしたわけ。
静的解析ツールって馬鹿判別機(使うヤツがアホ的な意味で)としては役立つけど、本当にコードを解析させちゃいけない。
# 逆に動的解析は極まれだけど役立つことあるけどね。本当にどこにあるか分からないメモリリーク探すときとか。
Re: (スコア:0)
>ちゃうちゃう。正しくは「静的解析ツールの罠にはまった」
>コードの意味理解しない馬鹿を黙らせるためだけに、
>必要のない変更を行い、そこでエンバグしたわけ。
>静的解析ツールって馬鹿判別機(使うヤツがアホ的な意味で)としては役立つけど、
>本当にコードを解析させちゃいけない。
それは一般性のない極論ですね。
ツールは使い方次第で使い方は使う側の人間(たち)の都合次第。
そして、あなたの周囲だけが世界のすべてではありません。
今回の件で言えば、
静的解析の警告を消すことはエンバグの直接の原因ではありません。
それは疑いようのない事実で、
タコなことが起きたのはその後の話です。
Re:なぜわざわざstrncat (スコア:1)
直接の原因ではありませんが、さらに深いところにある原因ではありますね。
どちらを問題視するか、どちらが一般的な話として適用しやすいか、言うまでもありません。
両方問題ではあるんですが。
Re: (スコア:0)
いやいや、PHPはPHPで書かれてないから.
Re: (スコア:0)
http://jp2.php.net/manual/ja/function.strstr.php [php.net]
OK?
Re: (スコア:0)
いや、だからね、PHP本体はC言語で書かれてるから。
今回の問題の箇所ももちろんCだから。
Re: (スコア:0)
開発者までそうではないと思いたいが、PHPな人たちって
開発者 = CでPHPランタイムを書いてる人たち
PHPな人たち = PHPでアプリケーションを作成する人たち
こういう解釈じゃないのかい?
# まあ一般人がアホだと政治家もアホが選ばれるというか...
もう修正されたみたいです (スコア:1, 既出)
バージョン5.3.7の重大なバグを修正した「PHP 5.3.8」公開
http://osdn.jp/magazine/11/08/24/0723210 [osdn.jp]
strncatの罠? (スコア:0)
リンクの解説読みましたけど、問題は
strncat(passwd, "$", 1);
を
strlcat(passwd, "$", 1);
に、変更したことのようですから、正確には「strlcatの罠」じゃないですかね?
strcat→strncatの時点ではバグってないわけで。
Re:strncatの罠? (スコア:5, 興味深い)
まあ仰ることも間違いではないのですが、C言語界の背景というか
strncatのsize指定が「意外性最小原則にそぐわない」仕様だということで
数々のバグの発生源となっており、散々非難されてきた、という今までの経緯があるのです。
で、srtncat使用者はそこを注意して使用してたけど、後からそれをstrlcatに置き換えた人は
そんな罠があったと知らず、何も考えずそのまま関数だけ置き換えてしまったと。
ある意味、strncatというのは関数自体がバッドノウハウ的な存在なのですね。
まさにバッドノウハウ的対応が、たとえその場は良くても将来に問題を引き起こすという好例となってしまったということでしょう。
Re:strncatの罠? (スコア:1, すばらしい洞察)
第三引き数が size なのは strlcpy の方だぞ。
引き数の意味を間違えて「意外性」どうのこうのとか、知ったかぶりするのはやめて欲しい。
本人は悪意がないのかもしれないけど、まだぞろ誤解して、strcat() 使うなとか、
strncat() 使うなとか、頭の悪いコード規約作ろうとうするやつが多発するから。
今回のも罠でも何でもなくて strcat() で十分なのに、迷信に従って strncpy(), strlcpy() とかに書き換えた馬鹿がいるだけじゃないか。
# *passwd = '$'; passwd[1] = 0; <--- 本当にやりたっかったこと
Re:strncatの罠? (スコア:2)
>strncat の第三引き数は size じゃないぞー(型は size_t だったりするかもしれないが)。
ああ、ごめんなさい。あまり気にしないで書いてしまいましたが
第三引数が「連結する文字数」なので size でなく length OR count だとかいう意味のご指摘ですよね?
その辺にこだわる方もいらっしゃいますが私は型がsize_tなら何であれサイズと表しても
間違いではないと思う派です。
バッファサイズ、バイトサイズ、文字列サイズ。ということで。
C言語で size_t が出てくる時、いつもこのうちのどれなのかで悩むので
静的型付け大好き人間としては buf_size_t とか length_t とか標準で作って欲しいと密かに思っています。
Re:strncatの罠? (スコア:1, おもしろおかしい)
蛇足の部分でミスった。
passwd_end = passwd + strlen(passwd);
*passwd_end = '$'; passwd_end[1] = 0;
と書かないと意味が通じないか。
Re:strncatの罠? (スコア:1, すばらしい洞察)
そして、そんな些細なミスを誘発するような再発明をするくらいならstrcat(passwd, "$");でいいじゃん、と言うことで、振り出しに戻る。
Re: (スコア:0)
>そして、そんな些細なミスを誘発するような再発明をするくらいなら
>strcat(passwd, "$");でいいじゃん、と言うことで、振り出しに戻る。
戻りませんよ。
プログラムに限らず、人間の行う論理表現では
「明示的なある」を示せても「暗黙のない」は示せません
(コメントなど含め、また「”ない”と書いて”ある”」まで含まれます)。
ここで、strcatでは
「バッファのサイズ溢れとか危険だけどその意識は”ある”のか”ない”のか」
を示すことができません。
ですので、そこで人間が明示的に「(意識は)ある」を保証するためには、
適切なコード(strcatからstrncatにす
Re: (スコア:0)
少なくとも溢れないようにしたいですとか溢れなくしましたという意思は一切表現されてない。
意識があろうがなかろうが、バッファ溢れするコードは溢れるし、溢れないコードは溢れません。
こんな簡単なケースなら機械的に判断できるはずだし、静的解析を謳うんならそれくらい解析して欲しいものです。
Re: (スコア:0)
このタレコミについてるコメント全体に言えることだけど、
strncat(passwd, "$", 1);
自体は全く問題ない。
strlcat(passwd, "$", 1);
が間違ってるだけ。
そもそも全体テストが通らない状態でstrcatをstrncatに書き換える意義は殆ど無いんだが、strncatに変えた事自体は問題なかった。
その後、一番の大馬鹿者がstrncatをstrlcatに差し替えたのが問題だった。
strlcatの存在を知っている奴がなぜstrncatとstrlcatでは動作が違い引数も違うということを意識しなかったのか…理解に苦しむ。
# くらいの事情は編集追記の「徳丸浩氏がブログにて詳細に解説している [tokumaru.org]」を読めば理解できるぞ
Re:strncatの罠? (スコア:1)
文字列を安全にくっつける用途では罠が多いけど、バイナリとかでちょうどnバイト埋めたいって用途の場合はむしろ自然な仕様なんじゃないかと思う。
# けれど、文字列に使われることの方が圧倒的に多いのでやっぱ悪か?
1を聞いて0を知れ!
Re: (スコア:0)
strncpyと勘違いしてない?
# strncpyの仕様は当時まだ生き残っていた固定長レコード向けのもので、断じてセキュリティのためではない。
Re:strncatの罠? (スコア:1)
strncpyと似たものだから、似たような用途かと思ったのだけど。
1を聞いて0を知れ!
Re:strncatの罠? (スコア:1, すばらしい洞察)
Re:strncatの罠? (スコア:1)
Ubuntuのuniverse/docにあるmanpages-ja-dev 0.5.0.0.20080615-1を確認した上で書いたけど。
挙動は当然書いてあったけど、それを何に使うためのものかまでは書いてなかった。
読んでおくことが推奨されるマニュアルで、strncatの挙動ではなく用途まで記述されているものがあるというのなら、教えてくださいな。
ないと思うけど。
1を聞いて0を知れ!
Re: (スコア:0)
strncpy
strncpy writes exactly the given number of bytes, either only copying the start of the string if it is too long, or adding zeros to the end of the copy to fill the buffer. It was introduced into the C library to deal with fixed-length name fields in structures such as directory entries. Despite its name it is not a bounded version of strcpy, it does not guarantee that the result is a null-terminated string. The name of the function is misleading because strncat and snprintf are
Re:strncatの罠? (スコア:1)
まさか、人に不勉強を指摘するコメントで、Wikipediaのことをウィキと略してるってことはないよね?
> The name of the function is misleading because strncat and snprintf are respectively bounded versions of strcat and sprintf.
っつーのは、そのWiki書いてる人の解釈でしかないようにしか読めない。
俺には、strncpyに対するものとしてNUL終端するとは限らないstrncatとNUL終端するsnprintfを同列に書いているのはミスリーディングに見えるし、
strncatの挙動がstrncpy(NUL終端するとは限らない。指定したサイズに達するまでNULで埋める)かsnprintfかどっちに近いと聞かれると、俺ならstrncpyに近いと答える。
別に、俺の見方が正しいというつもりはないが、どこかのWikiに書いてあることも、書いた人の印象でしかない。
1を聞いて0を知れ!
Re: (スコア:0)
>Wikipediaのことをウィキと略してるってことはないよね?
いまだにWikipediaとWikiの区別がつかないヤツって多いんだよ。
そっとしておいてやれよ。:p
# MediaWikiの話をすると不思議そうな顔をされるのも恒例
Re: (スコア:0)
> NUL終端するとは限らないstrncat
Re: (スコア:0)
>ウィキと書いたのはもちろん君をバカにするためだが、
>まさかマニュアルを読んでこの理解とは恐れ入った。全面降伏するよ。
>
> NUL終端するとは限らないstrncat
横槍になりますが、
(最終的にヌル止め同等と必ず見なせる状況になる場合を除外しても)
strncatはコピー先バッファ溢れになったときの挙動が未定義です。
それに対してsnprintfはバッファ溢れになってもヌル止めは入ります。
実際にはstrncatで上記状況でもヌル止めされる処理系が多いんでしょうけど、
str"n"cat、s"n"printfともにバッファオーバーラン対策が主眼で導入されたものですから、
「実際にやっちゃった」ときの挙動の違いは非常に重要でしょう。
Re:strncatの罠? (スコア:1)
> ウィキと書いたのはもちろん君をバカにするためだが
それで人をバカにできたと思えるのはすごい。
1を聞いて0を知れ!
Re: (スコア:0)
それはいいからつまらないバグで他人に迷惑をかける前にちゃんとマニュアル読んどけよ。って偉そうに読んだと宣言した上での恥だったな(笑)
Re: (スコア:0)
いいえ、strncat を使った事自体が罠なのです。
(end pointer くらい、自分で管理しろよ;-<)
Re: (スコア:0)
strcat(passwd, "$");
↓
strncat(passwd, "$", 1);
と、警告消しのためだけの有害かつ極めて愚かな変更です。
strlcatでバグったのも当然。
Re: (スコア:0)
Re: (スコア:0)
よかった (スコア:0)
何が問題って (スコア:0)
Re:何が問題って (スコア:2, おもしろおかしい)
知らなかっただけたよ!
全然違うよ!