パスワードを忘れた? アカウント作成
356654 story
ソフトウェア

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がリリースされている。

この議論は賞味期限が切れたので、アーカイブ化されています。 新たにコメントを付けることはできません。
  • by papa-pahoo (30621) on 2011年08月25日 0時11分 (#2008867) ホームページ
    この程度のバグは過去にもあったと思いますが、
    crypy()を利用する人が意外に多かったのでしょうね、きっと。

    PHPに限らず、実用システムでオープンソースを利用する場合の
    リスクの一例ということで。
  • by Anonymous Coward on 2011年08月24日 17時32分 (#2008719)
    make testですぐ引っかかる代物をリリースしたことが大問題ですよ。
    恐るべき事に、PHPというソフトは単体テストすら回さずにリリースが行われているということですよね?
    信じられないし、まともな神経を持った開発者ならあり得ない暴挙です。責任者が誰か知りませんが、ソフトをリリースする資格ないですよ。
    PHPは好きでも嫌いでもなかったんですが、こんな連中が作ってる代物だと知れた以上は、今後は出来る限り使用を避けようと思います。周りを説得する強力な材料にもなりましたし。
  • by Egtra (38265) on 2011年08月24日 22時12分 (#2008829)

    ここまで誰も話題にしないMicrosoftのCRT 関数のセキュリティが強化されたバージョン [microsoft.com]のstrcat_s。strlcatと違って黙って切り捨てることはない(そういう場合、デフォルトでは即座にプロセスごと死ぬ)ので、こっちだったら良かったのかなと思う。まだMSのコンパイラくらいでしか使えないけど。

    これベースにISO/IEC TR 24731(参考:STR07-C. TR 24731 を使用し、文字列操作を行う既存のコードの脅威を緩和する [jpcert.or.jp])なんてのも出ているので、C1X(C99の次)あたりに搭載されないか?

    • by Anonymous Coward

      > (そういう場合、デフォルトでは即座にプロセスごと死ぬ)ので、こっちだったら良かったのかなと思う。
      サーバを乗っ取られるよりはマシでしょうけどApacheが丸ごと落ちるのはちょっと…。

      • by Stealth (5277) on 2011年08月25日 13時52分 (#2009160)

        Apache httpd なら、基本的に落ちるのは子プロセスだけでは?

        # さすがに親プロセスレベルで落ちるのはリリース品質的にありえなさすぎるし。

        親コメント
      • by Anonymous Coward
        いや、大丈夫だ。例外を拾って握りつぶしてしまえば落ちることはない。
  • by Anonymous Coward on 2011年08月25日 3時43分 (#2008908)
    PHPは昔からですね。こんなの甘いほうです。
    当方が勤めている小社では、お客様が「PHP」と言ってもお断りしますし、
    PHPで作られたシステムの改修案件の場合は、
    設立当初からPHPの危険性を説いて、駄目であればお断りしています。
  • 直前で strlcpy を使ってるんだから、strlcat すればいいのに。
    ちょっと前を見れば static char passwd[MD5_HASH_MAX_LEN] ってあるように
    サイズもわかってるんだから。

    • by Anonymous Coward on 2011年08月24日 17時13分 (#2008711)

      違うよ。strncatに書き換えた時点では問題なかった。その後で [php.net]さらにstrlcatに書き換えたのに、第3引数をそのままにしてたからバグったの。
      参考: PHP5.3.7のcrypt関数のバグはこうして生まれた [tokumaru.org]

      親コメント
      • by Anonymous Coward

        まあタイトルがわかりづらいよね。
        どっちかっていうと"strlcatの罠にはまった"のであって。
        記事の内容も微妙に語弊があるかな。

        • by Anonymous Coward
          ちゃうちゃう。正しくは「静的解析ツールの罠にはまった」
          コードの意味理解しない馬鹿を黙らせるためだけに、必要のない変更を行い、そこでエンバグしたわけ。
          静的解析ツールって馬鹿判別機(使うヤツがアホ的な意味で)としては役立つけど、本当にコードを解析させちゃいけない。
          # 逆に動的解析は極まれだけど役立つことあるけどね。本当にどこにあるか分からないメモリリーク探すときとか。
          • by Anonymous Coward

            >ちゃうちゃう。正しくは「静的解析ツールの罠にはまった」
            >コードの意味理解しない馬鹿を黙らせるためだけに、
            >必要のない変更を行い、そこでエンバグしたわけ。
            >静的解析ツールって馬鹿判別機(使うヤツがアホ的な意味で)としては役立つけど、
            >本当にコードを解析させちゃいけない。

            それは一般性のない極論ですね。
            ツールは使い方次第で使い方は使う側の人間(たち)の都合次第。
            そして、あなたの周囲だけが世界のすべてではありません。

            今回の件で言えば、
            静的解析の警告を消すことはエンバグの直接の原因ではありません。
            それは疑いようのない事実で、
            タコなことが起きたのはその後の話です。

            • by fukapon (4131) on 2011年08月25日 9時26分 (#2008959)

              直接の原因ではありませんが、さらに深いところにある原因ではありますね。
              どちらを問題視するか、どちらが一般的な話として適用しやすいか、言うまでもありません。

              両方問題ではあるんですが。

              親コメント
  • by Wingard (37819) on 2011年08月24日 17時38分 (#2008720)

    バージョン5.3.7の重大なバグを修正した「PHP 5.3.8」公開
    http://osdn.jp/magazine/11/08/24/0723210 [osdn.jp]

  • by Anonymous Coward on 2011年08月24日 17時44分 (#2008722)

    リンクの解説読みましたけど、問題は

    strncat(passwd, "$", 1);

    strlcat(passwd, "$", 1);
    に、変更したことのようですから、正確には「strlcatの罠」じゃないですかね?

    strcat→strncatの時点ではバグってないわけで。

    • by ikotom (20155) on 2011年08月24日 18時03分 (#2008730)

      まあ仰ることも間違いではないのですが、C言語界の背景というか
      strncatのsize指定が「意外性最小原則にそぐわない」仕様だということで
      数々のバグの発生源となっており、散々非難されてきた、という今までの経緯があるのです。

      で、srtncat使用者はそこを注意して使用してたけど、後からそれをstrlcatに置き換えた人は
      そんな罠があったと知らず、何も考えずそのまま関数だけ置き換えてしまったと。

      ある意味、strncatというのは関数自体がバッドノウハウ的な存在なのですね。
      まさにバッドノウハウ的対応が、たとえその場は良くても将来に問題を引き起こすという好例となってしまったということでしょう。

      親コメント
      • Re:strncatの罠? (スコア:1, すばらしい洞察)

        by Anonymous Coward on 2011年08月24日 18時40分 (#2008738)
        strncat の第三引き数は size じゃないぞー(型は size_t だったりするかもしれないが)。
        第三引き数が size なのは strlcpy の方だぞ。

        引き数の意味を間違えて「意外性」どうのこうのとか、知ったかぶりするのはやめて欲しい。
        本人は悪意がないのかもしれないけど、まだぞろ誤解して、strcat() 使うなとか、
        strncat() 使うなとか、頭の悪いコード規約作ろうとうするやつが多発するから。

        今回のも罠でも何でもなくて strcat() で十分なのに、迷信に従って strncpy(), strlcpy() とかに書き換えた馬鹿がいるだけじゃないか。

        # *passwd = '$'; passwd[1] = 0; <--- 本当にやりたっかったこと
        親コメント
        • by ikotom (20155) on 2011年08月25日 2時46分 (#2008902)

          >strncat の第三引き数は size じゃないぞー(型は size_t だったりするかもしれないが)。
          ああ、ごめんなさい。あまり気にしないで書いてしまいましたが
          第三引数が「連結する文字数」なので size でなく length OR count だとかいう意味のご指摘ですよね?

          その辺にこだわる方もいらっしゃいますが私は型がsize_tなら何であれサイズと表しても
          間違いではないと思う派です。
          バッファサイズ、バイトサイズ、文字列サイズ。ということで。

          C言語で size_t が出てくる時、いつもこのうちのどれなのかで悩むので
          静的型付け大好き人間としては buf_size_t とか length_t とか標準で作って欲しいと密かに思っています。

          親コメント
        • Re:strncatの罠? (スコア:1, おもしろおかしい)

          by Anonymous Coward on 2011年08月24日 18時57分 (#2008747)
          > *passwd = '$'; passwd[1] = 0; <--- 本当にやりたっかったこと

          蛇足の部分でミスった。

            passwd_end = passwd + strlen(passwd);
            *passwd_end = '$'; passwd_end[1] = 0;

          と書かないと意味が通じないか。
          親コメント
          • Re:strncatの罠? (スコア:1, すばらしい洞察)

            by Anonymous Coward on 2011年08月24日 19時08分 (#2008752)

            そして、そんな些細なミスを誘発するような再発明をするくらいならstrcat(passwd, "$");でいいじゃん、と言うことで、振り出しに戻る。

            親コメント
            • by Anonymous Coward

              >そして、そんな些細なミスを誘発するような再発明をするくらいなら
              >strcat(passwd, "$");でいいじゃん、と言うことで、振り出しに戻る。

              戻りませんよ。

              プログラムに限らず、人間の行う論理表現では
              「明示的なある」を示せても「暗黙のない」は示せません
              (コメントなど含め、また「”ない”と書いて”ある”」まで含まれます)。

              ここで、strcatでは
              「バッファのサイズ溢れとか危険だけどその意識は”ある”のか”ない”のか」
              を示すことができません。
              ですので、そこで人間が明示的に「(意識は)ある」を保証するためには、
              適切なコード(strcatからstrncatにす

              • by Anonymous Coward
                でもstrncat(passwd, "$", 1);には「私はバカです」の表明以外の意味はないよね。
                少なくとも溢れないようにしたいですとか溢れなくしましたという意思は一切表現されてない。
                意識があろうがなかろうが、バッファ溢れするコードは溢れるし、溢れないコードは溢れません。
                こんな簡単なケースなら機械的に判断できるはずだし、静的解析を謳うんならそれくらい解析して欲しいものです。
              • by Anonymous Coward

                このタレコミについてるコメント全体に言えることだけど、
                strncat(passwd, "$", 1);
                自体は全く問題ない。
                strlcat(passwd, "$", 1);
                が間違ってるだけ。

                そもそも全体テストが通らない状態でstrcatをstrncatに書き換える意義は殆ど無いんだが、strncatに変えた事自体は問題なかった。
                その後、一番の大馬鹿者がstrncatをstrlcatに差し替えたのが問題だった。

                strlcatの存在を知っている奴がなぜstrncatとstrlcatでは動作が違い引数も違うということを意識しなかったのか…理解に苦しむ。

                # くらいの事情は編集追記の「徳丸浩氏がブログにて詳細に解説している [tokumaru.org]」を読めば理解できるぞ

      • by greentea (17971) on 2011年08月24日 18時42分 (#2008739) 日記

        文字列を安全にくっつける用途では罠が多いけど、バイナリとかでちょうどnバイト埋めたいって用途の場合はむしろ自然な仕様なんじゃないかと思う。

        # けれど、文字列に使われることの方が圧倒的に多いのでやっぱ悪か?

        --
        1を聞いて0を知れ!
        親コメント
        • by Anonymous Coward

          strncpyと勘違いしてない?
          # strncpyの仕様は当時まだ生き残っていた固定長レコード向けのもので、断じてセキュリティのためではない。

          • by greentea (17971) on 2011年08月24日 19時13分 (#2008753) 日記

            strncpyと似たものだから、似たような用途かと思ったのだけど。

            --
            1を聞いて0を知れ!
            親コメント
            • Re:strncatの罠? (スコア:1, すばらしい洞察)

              by Anonymous Coward on 2011年08月24日 19時50分 (#2008769)
              そうやってちゃんとマニュアル読まなかったのがまさにバグの原因
              親コメント
              • by greentea (17971) on 2011年08月24日 21時03分 (#2008798) 日記

                Ubuntuのuniverse/docにあるmanpages-ja-dev 0.5.0.0.20080615-1を確認した上で書いたけど。
                挙動は当然書いてあったけど、それを何に使うためのものかまでは書いてなかった。

                読んでおくことが推奨されるマニュアルで、strncatの挙動ではなく用途まで記述されているものがあるというのなら、教えてくださいな。
                ないと思うけど。

                --
                1を聞いて0を知れ!
                親コメント
              • by Anonymous Coward
                ウィキにも書いてある。

                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
              • by greentea (17971) on 2011年08月24日 22時46分 (#2008839) 日記

                まさか、人に不勉強を指摘するコメントで、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を知れ!
                親コメント
              • by Anonymous Coward

                >Wikipediaのことをウィキと略してるってことはないよね?
                いまだにWikipediaとWikiの区別がつかないヤツって多いんだよ。
                そっとしておいてやれよ。:p

                # MediaWikiの話をすると不思議そうな顔をされるのも恒例

              • by Anonymous Coward
                ウィキと書いたのはもちろん君をバカにするためだが、まさかマニュアルを読んでこの理解とは恐れ入った。全面降伏するよ。

                > NUL終端するとは限らないstrncat
              • by Anonymous Coward

                >ウィキと書いたのはもちろん君をバカにするためだが、
                >まさかマニュアルを読んでこの理解とは恐れ入った。全面降伏するよ。
                >
                > NUL終端するとは限らないstrncat

                横槍になりますが、
                (最終的にヌル止め同等と必ず見なせる状況になる場合を除外しても)
                strncatはコピー先バッファ溢れになったときの挙動が未定義です。
                それに対してsnprintfはバッファ溢れになってもヌル止めは入ります。

                実際にはstrncatで上記状況でもヌル止めされる処理系が多いんでしょうけど、
                str"n"cat、s"n"printfともにバッファオーバーラン対策が主眼で導入されたものですから、
                「実際にやっちゃった」ときの挙動の違いは非常に重要でしょう。

              • by greentea (17971) on 2011年08月25日 1時12分 (#2008887) 日記

                > ウィキと書いたのはもちろん君をバカにするためだが

                それで人をバカにできたと思えるのはすごい。

                --
                1を聞いて0を知れ!
                親コメント
              • by Anonymous Coward
                皮肉も通じない並外れたバカのために、ウィキ(笑)とでも書いときゃよかったか。
                それはいいからつまらないバグで他人に迷惑をかける前にちゃんとマニュアル読んどけよ。って偉そうに読んだと宣言した上での恥だったな(笑)
    • by Anonymous Coward

      正確には「strlcatの罠」じゃないですかね?

      いいえ、strncat を使った事自体が罠なのです。
      (end pointer くらい、自分で管理しろよ;-<)

    • by Anonymous Coward
      バグではないにしろ

      strcat(passwd, "$");
                ↓
      strncat(passwd, "$", 1);

      と、警告消しのためだけの有害かつ極めて愚かな変更です。
      strlcatでバグったのも当然。
    • by Anonymous Coward
      例のC言語なんてどーちゃらのリンクは誰も貼らないのか
      • by Anonymous Coward
        ですね。C言語をdisる人が暴れてるのかなーと思ったけど意外
  • by Anonymous Coward on 2011年08月24日 20時53分 (#2008794)
    うちのは5.3.6だった、セーフ
  • by Anonymous Coward on 2011年08月24日 21時48分 (#2008816)
    確認テストすらしてないこと
typodupeerror

普通のやつらの下を行け -- バッドノウハウ専門家

読み込み中...