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

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

SSLバグ

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

元のソース

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ってラベルなら分かりやすい。さらに言うとリソース確保、解放を行う関数から、実処理を行う関数を呼び出す方が綺麗に書けるのではないでしょうか？

コードレビュー

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

そして

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