2 ms·

The thing that bugs me is so many people are saying there is no way that could be a merge/editing error so it had to have been deliberate bad actor, but all the

by mzs 13y ago

The thing that bugs me is so many people are saying there is no way that could be a merge/editing error so it had to have been deliberate bad actor, but all they are comparing is a diff between the sources that match two released versions and using that as evidence that it was not very plausible that it was such en error. What I think happened is very plausible though: if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) goto fail; if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) goto fail; Some other check was added: if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) goto fail; + if ((err = SSLHashSHA1.foobar(&hashCtx, &signedParams)) != 0) + goto fail; if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) goto fail; Then that check was merged into another check, otherwise refactored, or decided it was not needed there anymore - but mistakenly only the added if ... line was removed: if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) goto fail; - if ((err = SSLHashSHA1.foobar(&hashCtx, &signedParams)) != 0) goto fail; if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) goto fail; And that's how that ended-up the way that it did. if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) goto fail; goto fail; if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) goto fail; Then of course it was not the only change submitted, it must have been a pretty sizable diff of many files, and it slipped-by in all that.

cpeterso 13y ago
The scenario you've described would require multiple changes to the same function, but the file diffs making their around the blogosphere show only cosmetic changes to that file and none of those affect the "goto fail" function.
- mzs 13y ago
  Except there was stuff going on related to this: Security-55471/libsecurity_ssl/security_ssl/tls_digest.c #ifdef KERNEL static int HashSHA1Update(SSLBuffer *digestCtx, const SSLBuffer *data) { return ccHashUpdate(ccsha1_di(), digestCtx, data); } #else static int HashSHA1Update(SSLBuffer *digestCtx, const SSLBuffer *data) { /* 64 bits cast: safe, SSL records are always smaller than 2^32 bytes */ assert(digestCtx->length >= sizeof(CC_SHA1_CTX)); CC_SHA1_CTX *ctx = (CC_SHA1_CTX *)digestCtx->data; CC_SHA1_Update(ctx, data->data, (CC_LONG)data->length); return 0; } #endif const HashReference SSLHashSHA1 = { SSL_SHA1_DIGEST_LENGTH, 40, SSL_SHA1_CONTEXT_SIZE, HashSHA1Init, HashSHA1Update, HashSHA1Final, HashSHA1Close, HashSHA1Clone }; Security-55179.13/libsecurity_ssl/security_ssl/sslDigests.c static OSStatus HashSHA1Update(SSLBuffer *digestCtx, const SSLBuffer *data) { /* 64 bits cast: safe, SSL records are always smaller than 2^32 bytes */ assert(digestCtx->length >= sizeof(CC_SHA1_CTX)); CC_SHA1_CTX *ctx = (CC_SHA1_CTX *)digestCtx->data; CC_SHA1_Update(ctx, data->data, (CC_LONG)data->length); return noErr; } const HashReference SSLHashSHA1 = { sizeof(CC_SHA1_CTX), CC_SHA1_DIGEST_LENGTH, 40, HashSHA1Init, HashSHA1Update, HashSHA1Final, HashSHA1Close, HashSHA1Clone }; Do you see how there was some simple refactoring going on there in the structure and how the entire in-kernel functionality was added? I could totally see debugging function being created and sprinkled in all the places after all the update()s were called while verifying kernel stuff was working then removed and in this one place one line not enough deleted. edit: Ha, I had my own copy paste error after getting bad gateway error, I fixed the duplicated code, sorry.

by mzs 13y ago

cpeterso 13y ago

The scenario you've described would require multiple changes to the same function, but the file diffs making their around the blogosphere show only cosmetic changes to that file and none of those affect the "goto fail" function.

mzs 13y ago
Except there was stuff going on related to this: Security-55471/libsecurity_ssl/security_ssl/tls_digest.c #ifdef KERNEL static int HashSHA1Update(SSLBuffer *digestCtx, const SSLBuffer *data) { return ccHashUpdate(ccsha1_di(), digestCtx, data); } #else static int HashSHA1Update(SSLBuffer *digestCtx, const SSLBuffer *data) { /* 64 bits cast: safe, SSL records are always smaller than 2^32 bytes */ assert(digestCtx->length >= sizeof(CC_SHA1_CTX)); CC_SHA1_CTX *ctx = (CC_SHA1_CTX *)digestCtx->data; CC_SHA1_Update(ctx, data->data, (CC_LONG)data->length); return 0; } #endif const HashReference SSLHashSHA1 = { SSL_SHA1_DIGEST_LENGTH, 40, SSL_SHA1_CONTEXT_SIZE, HashSHA1Init, HashSHA1Update, HashSHA1Final, HashSHA1Close, HashSHA1Clone }; Security-55179.13/libsecurity_ssl/security_ssl/sslDigests.c static OSStatus HashSHA1Update(SSLBuffer *digestCtx, const SSLBuffer *data) { /* 64 bits cast: safe, SSL records are always smaller than 2^32 bytes */ assert(digestCtx->length >= sizeof(CC_SHA1_CTX)); CC_SHA1_CTX *ctx = (CC_SHA1_CTX *)digestCtx->data; CC_SHA1_Update(ctx, data->data, (CC_LONG)data->length); return noErr; } const HashReference SSLHashSHA1 = { sizeof(CC_SHA1_CTX), CC_SHA1_DIGEST_LENGTH, 40, HashSHA1Init, HashSHA1Update, HashSHA1Final, HashSHA1Close, HashSHA1Clone }; Do you see how there was some simple refactoring going on there in the structure and how the entire in-kernel functionality was added? I could totally see debugging function being created and sprinkled in all the places after all the update()s were called while verifying kernel stuff was working then removed and in this one place one line not enough deleted. edit: Ha, I had my own copy paste error after getting bad gateway error, I fixed the duplicated code, sorry.