Help us understand the problem. What is going on with this article?

AppleがiOS7.0.6で修正したSSLバグの簡単な解説

More than 5 years have passed since last update.

ImperialVioletを読みながら、日本語でまとめています。

SSLバグ

iOS 7.0.6で修正されたSSLの脆弱性は「全体未聞」とか、「史上最悪」とか言われてますが、エンジニアとしてはどんなバグだったか興味ありますよね?

元のソース

sslKeyExchange.cからの引用

static OSStatus
SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,
                                 uint8_t *signature, UInt16 signatureLen)
{
    OSStatus        err;
    SSLBuffer       hashOut, hashCtx, clientRandom, serverRandom;
    uint8_t         hashes[SSL_SHA1_DIGEST_LEN + SSL_MD5_DIGEST_LEN];
    SSLBuffer       signedHashes;
    uint8_t         *dataToSign;
    size_t          dataToSignLen;

    signedHashes.data = 0;
    hashCtx.data = 0;

    clientRandom.data = ctx->clientRandom;
    clientRandom.length = SSL_CLIENT_SRVR_RAND_SIZE;
    serverRandom.data = ctx->serverRandom;
    serverRandom.length = SSL_CLIENT_SRVR_RAND_SIZE;


    if(isRsa) {
        /* skip this if signing with DSA */
        dataToSign = hashes;
        dataToSignLen = SSL_SHA1_DIGEST_LEN + SSL_MD5_DIGEST_LEN;
        hashOut.data = hashes;
        hashOut.length = SSL_MD5_DIGEST_LEN;

        if ((err = ReadyHash(&SSLHashMD5, &hashCtx)) != 0)
            goto fail;
        if ((err = SSLHashMD5.update(&hashCtx, &clientRandom)) != 0)
            goto fail;
        if ((err = SSLHashMD5.update(&hashCtx, &serverRandom)) != 0)
            goto fail;
        if ((err = SSLHashMD5.update(&hashCtx, &signedParams)) != 0)
            goto fail;
        if ((err = SSLHashMD5.final(&hashCtx, &hashOut)) != 0)
            goto fail;
    }
    else {
        /* DSA, ECDSA - just use the SHA1 hash */
        dataToSign = &hashes[SSL_MD5_DIGEST_LEN];
        dataToSignLen = SSL_SHA1_DIGEST_LEN;
    }

    hashOut.data = hashes + SSL_MD5_DIGEST_LEN;
    hashOut.length = SSL_SHA1_DIGEST_LEN;
    if ((err = SSLFreeBuffer(&hashCtx)) != 0)
        goto fail;

    if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0)
        goto fail;
    if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0)
        goto fail;
    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;

    err = sslRawVerify(ctx,
                       ctx->peerPubKey,
                       dataToSign,              /* plaintext */
                       dataToSignLen,           /* plaintext length */
                       signature,
                       signatureLen);
    if(err) {
        sslErrorLog("SSLDecodeSignedServerKeyExchange: sslRawVerify "
                    "returned %d\n", (int)err);
        goto fail;
    }

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

}

簡単な解説

どこが問題かわかりましたか?
そう、「goto fail;」が二行連続している部分です。一行目は直前のif文によって実行されていますが、二行目は無条件に実行され、SSLHashSHA1.final()および、sslRawVerify()がエラーになるケースを拾えなくなっています。
なお、使用するSSLオプションによって検証関数も異なるので、すべてのSSL接続が影響を受けた訳ではありません。

原因

テスト不足

理想的にはテストで拾いたいところですが、ハンドシェイクのかなり深い部分にあるので、すべてのケースを網羅するテストケースを書くのはなかなか難しそうです。

コンパイラのWarningを無視したのか?

今回のコードは100%実行されないdead codeがあります。
しかし、-Wallオプションをつけてもコンパイラから警告はなく、-Wunreachable-codeをつけて確認する必要があるようです。

コーディングスタイル

  • 括弧{}を省略したifの記法
  • gotoを用いたerror処理

というコーディングスタイルがミスを誘発していると思います。

あと、個人的にはfailというラベルのリソース解放コードが常に実行されるのも気持ち悪いです。これがfinallyってラベルなら分かりやすい。さらに言うとリソース確保、解放を行う関数から、実処理を行う関数を呼び出す方が綺麗に書けるのではないでしょうか?

コードレビュー

結局これですね。
誰でもミスをするので、適切なコードレビューによってこのようなバグを回避すべきです。

そして

僕は今日も一人でユニットテストも、コードレビューもないコードを書くのでした。
バグらないように書けばいいのさ!(おしまい

tomohisaota
かつてiOSエンジニアでしたが、今はnodejsばかり書いています。最近はデータ解析系の話が増えてきました。
Why not register and get more from Qiita?
  1. We will deliver articles that match you
    By following users and tags, you can catch up information on technical fields that you are interested in as a whole
  2. you can read useful information later efficiently
    By "stocking" the articles you like, you can search right away
Comments
No comments
Sign up for free and join this conversation.
If you already have a Qiita account
Why do not you register as a user and use Qiita more conveniently?
You need to log in to use this function. Qiita can be used more conveniently after logging in.
You seem to be reading articles frequently this month. Qiita can be used more conveniently after logging in.
  1. We will deliver articles that match you
    By following users and tags, you can catch up information on technical fields that you are interested in as a whole
  2. you can read useful information later efficiently
    By "stocking" the articles you like, you can search right away
ユーザーは見つかりませんでした