Was the iOS SSL Flaw Deliberate?

Re:Was the iOS SSL Flaw Deliberate? by mzs on Thursday February 27, 2014 @11:38AM (#7313122)

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.

[ Parent | on HN ]

Re:Was the iOS SSL Flaw Deliberate? by cpeterso on Thursday February 27, 2014 @01:00PM (#7313716)

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.

[ Parent | on HN ]

Re:Was the iOS SSL Flaw Deliberate? by mzs on Thursday February 27, 2014 @02:07PM (#7314211)

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.

[ Parent | on HN ]