iOS7.0.6で修正された「最悪のセキュリティバグ」はありがちなコーディングミスで発生していた

minet 曰く、

Appleが先日公開したiOS 7.0.6では、「過去最悪のセキュリティバグ」が修正されているという（アプリオ）。

SSL/TLSの不適切な扱いにより、データが暗号化されない場合があり、これにより通信内容が盗聴されるおそれがあるようだ。

現在利用中の環境にこのバグがあるかどうかは、gotofail.comというサイトで確認できる模様。

興味深いのが、このバグの要因とされているコードだ。SSLで使われる暗号鍵の検証を行うコードにバグがあったとのことなのだが、その途中で以下のように「goto」文が続いているミスがあったという（ImperialViolet - Apple's SSL/TLS bug）。

static OSStatus
SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,
                                 uint8_t *signature, UInt16 signatureLen)
{
    OSStatus        err;
    ...

    if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
        goto fail;
    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
        goto fail;
        goto fail;
    if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
        goto fail;
    ...

fail:
    SSLFreeBuffer(&signedHashes);
    SSLFreeBuffer(&hashCtx);
    return err;
}

コード全文は修正後のコードを見てほしいが、このように「goto fail;」が2行続いたことで途中の検証作業が飛ばされてしまい、その結果本来エラーとなるケースでエラーにならない、という問題が発生している模様。

格好つけずに括弧つけろよ

[Anonymous Coward]

処理が１行だからといって、if文で格好つけるのはやめましょう。

Re:格好つけずに括弧つけろよ

[ncaq (46027)]

lisp神の守護を受けていればこんな事にはならなかった

C/C++はBSD/オールマン･スタイルで書いてます

Re:格好つけずに括弧つけろよ

[Anonymous Coward]

Python教徒がアップを始めました

Re:格好つけずに括弧つけろよ

[Anonymous Coward]

意図と違ったコードになってることには気づきやすい！はず！
# ちょっとだけ必死

Re:格好つけずに括弧つけろよ

[gyokuto (45996)]

もしかして：括弧 というより、『gotoなしで else if連鎖にすると』の間違い？ コード例的に。

Re:格好つけずに括弧つけろよ

[minet (45149)]

条件式の後に単文を実行する場合はブレースを付けないコーディング規約なのかも知れません。

私はかつて、最初にプログラマとして勤めた職場が「ブレース省略禁止」で叩き込まれたこともあり、ブレース省略の利点がさっぱりわからない（こういうバグを生む）のですが、なぜか「単文ではブレース省略」とする規約は見たことがあります。

Re:格好つけずに括弧つけろよ

[tenokida (42811)]

不要なコードは書くな主義かな。
ソースコードのサイズを制限したいとか
＃そういう時代もあったんです

あるいはK&Rスタイルに沿っているつもりとか。

単文にはそもそも中括弧は不要だからでしょう。
つまり「省略」じゃない。
Syntax Sugarの一種で水増しだといわれても仕方がない面があるから。

IFの実行部は必ずブロックにしろ、単文は使うなってのは30年以上前から言われてますけどね。

Re:格好つけずに括弧つけろよ

[ymasa (31598)]

> 以下のプログラムのPOINT2は絶対に実行されない。

それは誰でもわかってるだろ。

#2550906のプログラムと違うだろう。

Re:格好つけずに括弧つけろよ

[nim (10479)]

＞ 戻り値がboolだから条件判断を省略するとか

いやむしろそれは正しい気がする。
逆に条件句で整数が入るケースが気色悪い。

if(1) みたいな。

Re:格好つけずに括弧つけろよ

[nobanner (41086)]

K&Rから入ったせいか(i != 0)を(i)と書かないと冗長に見えて落ち着かないんです。

Re:格好つけずに括弧つけろよ

[HomuraAkemi (46038)]

if (!p) {
}

↑は多用するけど、

if (p == NULL) {
}

↑はあまり好きじゃないです。

他人が書いてるのは見て見ぬふりをするけど、自分で書くときは前者のスタイルで書きます。

Re:格好つけずに括弧つけろよ

[Anonymous Coward]

私なら何を判定しているのか自分以外(未来の自分含む)の人にもすぐ理解してもらうために省略せず後者の書き方をします。

テストしてねーの？

[Anonymous Coward]

OSのクセにこれぐらいのバグも検出できないぐらいテスト項目が粗い、ということがわかった。
他の機能も危なそうだな。

Re:

[Anonymous Coward]

検証コードを書いた人も実行してないっぽいですね。

/* FIXME: can this line be removed? */
あるあるすぎる。

これぐらいで「過去最悪」とは片腹痛い

[esuta (40045)]

証券会社は不適切なワンクリックで破産したりする
https://www.google.co.jp/search?q= [google.co.jp]誤発注
ジェイコムのより桁が２つ違う

https://www.google.co.jp/search?q= [google.co.jp]俺はバグでこんなすごい被害を出したぞ

も怖い

Re:

[Anonymous Coward]

過去最悪かどうかはわからないが、近年で見ると最悪レベルだと思う。

つまり、ネットバンクやSNSなんかをSSLで使ってる人は多いと思うが、iOS/Macを使っている人の場合は、MITM攻撃で情報を盗み見できる状態にあったってことだ。
iPhone/iPad/Macはローカルだけで使ってください、という話になるよ。

これを最悪といわずに、なんという。

だからAppleにしては珍しく、前倒しして早めに修正パッチをリリースしたわけで。

Re:これぐらいで「過去最悪」とは片腹痛い

[Anonymous Coward]

私は　apple製品を使う人間は

馬鹿か自殺志願者

と思っています。

あそこは何ひとつテストしませんからね。
そもそもテストどころか動作すらしないものえを発売するところですし。（動作しないんだからテストもできるわけがない）

http://internet.watch.impress.co.jp/cda/news/2007/09/28/17017.html [impress.co.jp]

大事な事なので二度書きました

[miyuri (33181)]

しっかり下まで落とせるわけだ。

修正後のコード

[asanagi (22217)]

>>コード全文は修正後のコードを見てほしいが

これって件の問題が修正されたものなの？

全く、これだからgotoは

[Anonymous Coward]

goto関連のエラーを見るたびに申し訳なくなる私は後藤

#本名ぶっぱなのでAC

ヨシ、さっそく営業だ！

[kohzoh (34869)]

静的コード解析ツールを扱っている営業マンがApple本社に集まってきたりして。
このパターンだと確実にツールに指摘されるところだが、Appleほどの会社がまだ使っていないのかなぁ？

コピペしてるうちにgotoの行が増えちゃった

[shima_tetsuo (46446)]

多分コピペが原因だろうね。
経験上コピペがバグの原因になる可能性は高いから(俺も含めて)みんな気をつけよう！

コピペしたら中身の修正漏れはないか、余計に(または少なく)行をコピってないか
そもそもコピペをせずに関数化するかを慎重に確認しないとね(俺が…)。

unreachable codeの警告は無視されたのか

[Anonymous Coward]

警告を無視しちゃいかんという教訓を得たと。

こんな感じに、if分を一行にまとめてしまえば、こんな不具合出なかった気がします。

if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0) goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) goto fail;
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) goto fail;

一行が長くなるのを嫌ったのかな？

Re:unreachable codeの警告は無視されたのか

[Anonymous Coward]

一行にまとめるコーディングは嫌いですし、職業プログラマーはあまり使うべきではないとも思っています。
なので対処はif分の処理が1行でも必ず括弧で括る事です。

Re:unreachable codeの警告は無視されたのか

[Anonymous Coward]

そうですね。
どっちにしろ2行以上あれば必ず括弧が必要な訳だし、if文書いたら脊髄反射で{}を書いてしまうくらいでちょうどいいです。

Re:unreachable codeの警告は無視されたのか

[Anonymous Coward]

スタイルの問題ではないでしょこれは。
こういう重要な箇所くらいテストで100%のカバレッジを確保しろよと。
どーでもいいとこに心血注ぐ必要はないけどさ。

Re:unreachable codeの警告は無視されたのか

[chromoly (46484)]

unreachable codeの検出は、停止性問題と同じなんで、goto文みたいな自由度の高い構文ではうまく動かないんじゃないのかね。
それ以前にgccもclangもgotoの警告はやる気ないから

Re:

[Anonymous Coward]

#define ErrorCheck(f) do { if ((err = f) != 0) goto fail; } while (0)

ErrorCheck(SSLHashSHA1.update(&hashCtx, &serverRandom));
ErrorCheck(SSLHashSHA1.update(&hashCtx, &signedParams));
ErrorCheck(SSLHashSHA1.final(&hashCtx, &hashOut));

ってやりたくなるんだけど、ダメ？

Re:unreachable codeの警告は無視されたのか

[ymasa (31598)]

マクロをdo-while(0)で囲むワケ
http://d.hatena.ne.jp/ryochack/20111022/1319252481 [hatena.ne.jp]

Re:unreachable codeの警告は無視されたのか

[Anonymous Coward]

マクロ関数を普通の関数のように最後に ; を付けて適切に展開されるようにするため。

do while (0) なしだと

if ()
    ErrorCheck(...);
else
    ErrorCheck(...);

が

if ()
    { ... };
else
    { ... };

となり、 if の終わりの {} の後の ; が空文となるので、 else の行で文法エラーとなる。

do while (0) ありだと

if ()
    ErrorCheck(...);
else
    ErrorCheck(...);

が

if ()
    do { ... } while (0);
else
    do { ... } while (0);

と、 if と else が適切に対応する形になるよう展開される。

Re:unreachable codeの警告は無視されたのか

[kr (10950)]

#define ErrorCheck(f) if ((err = f) != 0) goto fail
でおk

それは潜在的に危険なコードです。

ErrorCheck()を使う側で、以下ようなコードを書くと、

if (.../*条件A*/)
        ErrorCheck(...);
else
        .../*処理B*/;

この見た目と違って、実際のマクロ展開は(改行と字下げを付けると)、

if (.../*条件A*/)
        if ((err = ...) != 0)
                goto fail;
        else
                .../*処理B*/;

となり、「処理B」が「条件A」のelse部でなく、「(err = ...) != 0」のelse部にくっついてしまいます。

else節は最も近いif文にくっつくという構文規則になっているので、 マクロにelse節無しの剥き出しのif文を埋め込むのは危険なのです。

do〜while(0)で囲めば、このような副作用が無くなるので、より安全なマクロ定義となります。

関数ポインタ

[Kando (36875)]

"."が使われている場合でも、C++のメンバ関数ではなく
構造体のメンバに関数ポインタがあり、それを呼び出しているだけの可能性もあるので
呼び出し側だけではC++noコードだとは判断が付きません。

その構造体の定義は見つける前にGive upしましたが、関数ポインタをメンバに持つ似たような構造体定義は同じディレクトリの
sslTypes.h [apple.com]の下の方にもあります。

Re:unreachable codeの警告は無視されたのか

[nim (10479)]

今回はかっこつけてないから発生したバグですよ。

failに行くのにエラーにならない!?

[Anonymous Coward]

goto fail;で、全部エラーに飛ぶように見えるんだけど、違うの。
goto successじゃないよね？

Re:failに行くのにエラーにならない!?

[Anonymous Coward]

> goto fail;で、全部エラーに飛ぶように見えるんだけど、違うの。

goto fail なんだけど err になにも入ってないで帰るんでしょ。

うまく考えて書いたんだろうけど、読みにくい人がいるんでデバッグに向いてないし、バグを生む。

Re:failに行くのにエラーにならない!?

[Anonymous Coward]

実はfailじゃなくてfainal、そう、実はfinalのTypoだったんだよ！(ねーよ

Re:failに行くのにエラーにならない!?

[Tsann (15931)]

無駄な値で初期化せず、
（もしロジックに誤りがあった場合に）コンパイラに未初期化変数の使用の警告をさせることを望みたいですねえ。
そういう意味では別ツリーのコーディング規約で括弧を付けろ、よりもコンパイラにunreachable codeの検出をして欲しかった。
人間が頑張ったって限界があるし。

必ずfailへ行くのに？

[Anonymous Coward]

エラーにならんの？
どういうこと？

Re:

[Anonymous Coward]

とよく読み直したらerrが0になっててそれをそのまま返してるからか。

Re:goto文

[gyokuto (45996)]

某、try{} catch{}的に使うのは 結構多用してますが、なんだかなあ、という気にはなりますよねえ。

goto considerd harmful論争？

[tenokida (42811)]

強力すぎる（ブロックの中に飛んだり）
制御フローをぶっちするのでわかりにくく間違いの元

ということで使うな論があった。
Cの制御文の貧弱さゆえに使わざるを得ない、もしくは使ったほうがいい場合にのみ使えということになった。

＃continue文もなあ、対応間違いとかあるし

Re:gotoを久しぶりに見た

[shibuya (17159)]

久しぶりだからはしゃいでいるんでしょうねえ。賑わってて何よりです。ふだんはgotoを禁則事項とされている人が大多数なんじゃないかと。

Re:gotoを久しぶりに見た

[taka2 (14791)]

「構造化プログラミングではgoto禁止」の原理主義にはまり込む流れ

step1 とりあえずgoto禁止

    if (実行結果==err) {
        後始末;
        return err;
    }
    if (実行結果==err) {
        後始末;
        return err;
    }
    後始末;
    return success;
}

step2 後始末は一箇所にまとめたい

    result = subfunc(&変数1,&変数2,…);
    変数1, 変数2,…に対して後始末;
    return result;
}
int subfunc(型1 *変数1, 型2 *変数2, …) {
    if (実行結果1==err) return err;
    if (実行結果2==err) return err;
    return success;
}

step3 変数の使用と後始末の場所が離れてるのがイヤ

    switch (0) { default:
        result=err;
        if (実行結果1==err) break;
        if (実行結果2==err) break;
        if (実行結果3==err) break;
        …
        result=success;
    }
    後始末
    return result;
}
/*switch の代わりに、他のコメントでマクロ定義に出ているような do {…}while(0) も可。でも、ループじゃないことが明確なのでswitchの方が好みかな。どっちにしろちょいとトリッキーですが。*/

step4 break って、結局gotoだよなと気づき、goto禁止主義と撤回

    result = err;
    if (実行結果1==err) goto finally;
    if (実行結果2==err) goto finally;
    if (実行結果3==err) goto finally;
    result = success;
finally:
    後始末
    return result;
}

結局、gotoを使う方がシンプルで読みやすいって結論になる。

使って言いgotoと使ったらダメなgotoをコーディングルールで縛るのが難しいのが欠点かなぁ。

#step3のテクニックで、try～catchっぽくするマクロを見た覚えがあるんですが、今探したら見つけられなかった…
確か、do while(0) と switch とで二重にくるんで、break →switch脱出、continue→do while 脱出と、二種類の似非gotoを使い分けてた覚えがある。

Re:gotoを久しぶりに見た

[nobanner (41086)]

こんな感じですかね。
#switch (0) { default: xxxx } って構文初めて見た

#define TRY(x) do { int thrown = 0; switch (0) { default:
#define END_TRY }
#define THROW(x) do { thrown = (x); break; }
#define CATCH(x) if (thrown == (x)) {
#define END_CATCH continue; }
#define FINALLY  } while (0);
#define END_FINALLY
 
#define ERROR 1
 
static OSStatus
SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,
                                 uint8_t *signature, UInt16 signatureLen)
{
    OSStatus        status;
    ...
    TRY
        if (status = SSLHashSHA1.update(&hashCtx, &serverRandom), status) THROW(ERROR);
        if (status = SSLHashSHA1.update(&hashCtx, &serverRandom), status) THROW(ERROR);
        if (status = SSLHashSHA1.update(&hashCtx, &signedParams), status) THROW(ERROR);
        if (status = SSLHashSHA1.final(&hashCtx, &hashOut), status) THROW(ERROR);
    END_TRY
    CATCH(ERROR)
        ...
    END_CATCH
    FINALLY
        SSLFreeBuffer(&signedHashes);
        SSLFreeBuffer(&hashCtx);
    END_FINALLY
    return status;
}

Re:gotoを久しぶりに見た

[nobanner (41086)]

あらダメだ、THROWマクロにbreakは使えない

Re:gotoを久しぶりに見た

[nobanner (41086)]

#define TRY(x) switch (0) { int thrown = 0; default: do {
#define END_TRY } while (0);
#define THROW(x) switch (0) { default: thrown = (x); continue; }
#define CATCH(x) if (thrown == (x)) {
#define END_CATCH break; }
#define FINALLY  }
#define END_FINALLY

Re:gotoを久しぶりに見た

[tenokida (42811)]

よく見られた書き方ですね。
IDEないから正常処理をわかりやすくとか、
＃上から下一直線
修正忘れがあるから同じ処理を別々に書くなを重視しエラー処理をまとめるとこういうスタイルになる。
別コメにあったけどfinally無いからこうせざるを得ない。

Re:goto 使う派

[nobanner (41086)]

本来なら例外を使うべきところを、効率が求められるOS内コードなので goto で代用した、と考えると納得がいく気がします。

例外機能のありがたみがわかる事例

[Kando (36875)]

.cみたいなのでこのコードはC++ではなくCなのではないかと思います。
そうすると例外機能がないので。

自分で確認はしてないですが、コメントに「同じディレクトリに.cppのファイル」があるという話なようですね。
とすればプロジェクトは（恐らく）C++ も使える環境なんだろうと思うので
何故このコードがCでないといけないのか不思議には思いますが…。

C++である場合、例外の実装ではオーバーヘッドは主に投げる時に発生し、
投げない時には極力発生しないように工夫されているという話なので
チェックが失敗したときに少し遅くなる程度はそれほど問題にならない気もしなくはないです。
（失敗した場合、それよりうしろのチェックの処理時間はなくなるわけだし。）

参考： 「C++オブジェクトモデル―内部メカニズムの詳細 」 [amazon.co.jp]（S.B. リップマン 著）の7.2節

Re:例外機能のありがたみがわかる事例

[Kando (36875)]

その後、ディレクトリを眺めましたが殆どが.cと.hであり、.cppはごく少数にとどまるので、
どうやら基本的にCで書かれ、C++からも呼び出せるようになっているということなのかなと思います。

＃検索できないので詳細を追うのは諦めました。