Search This Blog

Wednesday, June 15, 2016

Break is goto in disguise

On the python list there was a discussion about break.  I made the comment that break is just a euphemism for goto.  I thought that this would be a commonplace. However google does not give many useful hits for this.

So thought I'd cook up an example.
Here is Apple's famous SSL-bug in shortform

It is discussed in many places 


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

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


    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;          //<----- OOOOOOPSSSSS!!
    if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
        goto fail;

    err = sslRawVerify(ctx,               // <--- Important Code
                       ctx->peerPubKey,   // That needs to run
                       dataToSign,        // Missed
                       dataToSignLen,
                       signature,
                       signatureLen);
    if(err) {
        sslErrorLog("SSLDecodeSignedServerKeyExchange: sslRawVerify "
                    "returned %d\n", (int)err);
        goto fail;
    }

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

}
Now we just slightly hypotheticalize it by enclosing in a loop:
static OSStatus
SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa,
                                 SSLBuffer signedParams,
                                 uint8_t *signature, UInt16 signatureLen)
{
    OSStatus        err;

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


    hashOut.data = hashes + SSL_MD5_DIGEST_LEN;
    hashOut.length = SSL_SHA1_DIGEST_LEN;
    :
    :
    while (somethin)  { 

       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,   
                           dataToSignLen,
                           signature,
                           signatureLen);

        if(err) {
            sslErrorLog("SSLDecodeSignedServerKeyExchange: sslRawVerify "
                        "returned %d\n", (int)err);
            goto fail;
        }
    }
fail:
    SSLFreeBuffer(&signedHashes);
    SSLFreeBuffer(&hashCtx);
    return err;
}
And voila! We can change the gotos to breaks with the buggy behavior!
static OSStatus
SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa,
                                 SSLBuffer signedParams,
                                 uint8_t *signature, UInt16 signatureLen)
{
    OSStatus        err;

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


    hashOut.data = hashes + SSL_MD5_DIGEST_LEN;
    hashOut.length = SSL_SHA1_DIGEST_LEN;

    :
    :
    while (somethin)  {

        if ((err = SSLFreeBuffer(&hashCtx)) != 0)
            break;

        if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0)
            break;
        if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0)
            break;
        if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
            break;
        if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
            break;
            break;               //<--- Same Oops as goto!!
        if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
            break;

        err = sslRawVerify(ctx,
                           ctx->peerPubKey,      // Missed
                           dataToSign,           // just the same
                           dataToSignLen,        // whether you spell it goto
                           signature,            // or break :-)
                           signatureLen);
        if(err) {
            sslErrorLog("SSLDecodeSignedServerKeyExchange: sslRawVerify "
                        "returned %d\n", (int)err);
            break;
        }
    }
    // What was fail: label; now implicit
    SSLFreeBuffer(&signedHashes);
    SSLFreeBuffer(&hashCtx);
    return err;
} 


From this example many people have concluded many things... I may write about that subsequently

For now lets just agree that break is not very different from goto

[Also thought I'd massage the same example to show that continue is no different.  But that evidently requires more work...] 

3 comments:

  1. This comment has been removed by the author.

    ReplyDelete
  2. A break is compiled into a jmp(goto). So whats new about the equivalence?

    Perhaps you are talking of structure, not so much of the code as of the thinking...

    Also, pure structured is good, totally unstructured also required sometimes. But mix the two and you have a mess!

    ReplyDelete
  3. Tnx -- I think thats an important point -- mix of structured and unstructured can be worse than pure unstructured

    ReplyDelete