Closed Bug 822365 (CVE-2013-1620) Opened 11 years ago Closed 11 years ago

Non-constant time CBC decoding results in padding oracle (Lucky Thirteen attack)

Categories

(NSS :: Libraries, defect, P1)

defect

Tracking

(firefox20 affected, firefox21 fixed, firefox22 fixed, firefox-esr1720+ fixed, b2g1820+ affected)

RESOLVED FIXED
3.14.3
Tracking Status
firefox20 --- affected
firefox21 --- fixed
firefox22 --- fixed
firefox-esr17 20+ fixed
b2g18 20+ affected

People

(Reporter: wtc, Assigned: agl)

References

Details

(Keywords: sec-high)

Attachments

(17 files, 17 obsolete files)

24.95 KB, text/html
Details
10.83 KB, patch
Details | Diff | Splinter Review
4.88 KB, patch
Details | Diff | Splinter Review
1.27 MB, application/pdf
Details
61.10 KB, patch
Details | Diff | Splinter Review
67.32 KB, patch
Details | Diff | Splinter Review
2.02 KB, patch
KaiE
: review+
Details | Diff | Splinter Review
938 bytes, patch
KaiE
: review+
Details | Diff | Splinter Review
2.16 KB, patch
Details | Diff | Splinter Review
542 bytes, patch
Details | Diff | Splinter Review
625 bytes, patch
wtc
: review?
agl
Details | Diff | Splinter Review
2.79 KB, patch
wtc
: review+
Details | Diff | Splinter Review
3.20 KB, patch
wtc
: review+
KaiE
: review+
Details | Diff | Splinter Review
21.50 KB, patch
rrelyea
: superreview+
Details | Diff | Splinter Review
5.35 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
2.18 KB, patch
Details | Diff | Splinter Review
4.41 KB, patch
agl
: review+
Details | Diff | Splinter Review
Kenny Paterson of Royal Holloway, University of London sent us a
paper that Nadhem AlFardan and he wrote on a new analysis of the
TLS Record Protocol. The paper is NOT yet published.

  Lucky Thirteen: Breaking the TLS and DTLS Record Protocols

  Nadhem J. AlFardan and Kenneth G. Paterson

  Abstract
  The Transport Layer Security (TLS) protocol aims to provide
  confidentiality and integrity of data in transit across untrusted
  networks. TLS has become the de facto secure protocol of choice
  for Internet and mobile applications. DTLS is a variant of TLS
  that is growing in popularity. In this paper, we present
  distinguishing and plaintext recovery attacks against TLS and
  DTLS. The attacks are based on a delicate timing analysis of
  decryption processing in the two protocols. We include
  experimental results demonstrating the feasibility of the attacks
  in realistic network environments for several different
  implementations of TLS and DTLS, including the leading OpenSSL
  implementations. We provide countermeasures for the attacks.
  Finally, we discuss the wider implications of our attacks for the
  cryptographic design used by TLS and DTLS.

Adam is implementing an NSS patch for constant-time CBC decoding.
He should be able to attach his patch for review soon.
Group: infrasec
Attached patch patch (obsolete) — Splinter Review
This patch has not been carefully reviewed by Ryan or Wan-Teh. Readers may wish to review the HTML that I'll also attach this this bug which contains notes on the design.
Keywords: sec-high
After talking to the paper authors about their testing of the OpenSSL version of this patch, I spent yesterday running my own timings and found an ~20 cycle bias towards longer padding.

I tracked it down to a variable time DIV instruction in the core code that's shared between the NSS and OpenSSL versions. The fix is reasonably simple (although there is a danger that, in the future, the compiler might be smart enough to remove it).

I also tested that it's possible to exploit cache-line lengths on suitable processors to save ~5K cycles of computation on amd64.

I'll update the patch in the coming days with these minor changes.
Bob: Can you provide feedback on the PKCS#11 portions / the libssl<->softoken bridging? I'm presuming that the interaction between softoken and freebl are fine (because of the FIPS boundary), but the libssl portion needs particularly careful attention.

This patch attempts to permit running with a FIPS validated (but insecure) softoken and with a properly-patched softoken.
Comment on attachment 693120 [details] [diff] [review]
patch

As Adam mentioned, there are still some modifications coming for the constant-time aspect of the actual patch. This r? is to focus on the libssl<->softoken changes, which are not expected to change.
Attachment #693120 - Flags: review?(rrelyea)
Attached patch patch (obsolete) — Splinter Review
This patch updates the previous one with the addition of a 'divSpoiler' variable that causes a divide in ssl_CBCExtractMAC to be constant time on Intel machines.

There's an easy speed up that can be had in that function if I can know that the machine has a 64-byte cache-line (i.e. amd64) or, more generally, that the MAC is <= the cache-line size. Ryan pointed me at a function that's private to mpi that has this information but ssl/ can't access it and I don't see any other platform specific code in there so I didn't want to add any.

I've checked with a Chrome build that AES CBC sites function when built with a patched libssl and linked against either the patched or system versions of NSS. (The build does require updated NSS headers however.)
Attachment #693120 - Attachment is obsolete: true
Attachment #693120 - Flags: review?(rrelyea)
Attachment #703040 - Flags: review?(rrelyea)
re comment 4.

I'm still looking at the patch, but I do have some concerns that I can voice immediately.

The PKCS #11 API is unnecessarily pushing SSL knowledge down into our constant time functions. The current design, rather than taking a completed header from SSL, it takes enough data to reconstruct a header, rather than taking an SSL built header altogether. This isn't such an issue for the SSL3_MAC, it would be nice if the Constant HMAC Mechanism was really generic. I would prefer a construct where the SSL knowledge was pushed back up to SSL and the new mechanisms simply contained pointers to the completed headers.

I would also prefer if we ditched the separate headers altogether (it would simplify the code, as well the the PKCS #11 interfaces). Unfortunately I suspect the performance hit may be too high. We could probably do it without a hit by arraigning for the input buffer to have enough space in front to be able to write the header (so only the header is copied, not the full input buffer+header). On the other hand, I think I'm convinced we really do need a new mechanism. The time constant Macs need to 1) be single shot, and 2) need to know what the padded length of the data (rather than the length of data minus the pad, which is the information we are leaking).

Since we have to be single shot, we also should just use the single shot interface (C_SignInit/C_Sign) as opposed to C_SignInit/C_SignUpdate/C_Sign_Final. The trick is we need to add a new function to pkcs11 wrap which either:

1) just does the C_Sign on existing context, or
2) does the full C_SignInit/C_Sign a.la. PK11_Sign, except it takes a mechanism and a parameter.

I think the second would make for a cleaner SSL implementation (and could reuse PK11_Sign's code, so that PK11_Sign would call PK11_SignWithParams() [just making up the name for the new function on the fly.
Besides the overall API comments, I did notice some issues (some were nits, others were actual issues). My review is still far from complete, however.

In pkcs11c.c:

- sftk_MACconstantTime_End is pretty much identical to sftk_HMACCopy(). The latter should probably go away, and the former should be renamed to sftk_SignCopy() and used for both HMACK and the Constant time functions.
- the new sftk_DestroyNoop is the same as the existing sftk_Null()
- Bug: context->hashdestroy destroys the context->hashInfo, context->destroy destroys the context->cipherInfo. You are initializing them backwards, which would result in leaking your context->hashInfo. Note: you also declare  a sftk_MACConstantTime_DestroyContext in hmacct.c, but you use sftk_Space. The both currently have identical functionality, but it's probably clear to just use the destroy function you declared for your function.

If we go with a C_SignInit/C_Sign structure, then we should set context->multi = PR_FALSE.
So I believe there is a solution that does not require new PKCS #11 mechanism, but it would require extensive changes to both SSL and softoken, and I'm not sure about the performance. I'll outline my idea here, but I want to be clear I'm not actually advocating it. Given time constraints, I think my preferred solution is the new Mechanism, which is a single shot and takes a 'header buffer' in the params block.

The idea is to use the PKCS #11 SignEncrypt/VerifyDecrypt functions. These functions combine the encryption/decryption with the hash. We would switch from Raw CBC to CBC_PAD (where the PKCs #11 module deals with the padding), and that would give the PKCS #11 module the correct amount of information to be able to implement a constant time version of these function.

The rub here is that because we are use CBC_PAD, we need to Initialize and finalize the encryption/decryption operations every block, This would like add a performance burden (where currently we initialize once and only finalize on connection close, or key change).

I'm not sure all these square pegs can be pounded into the rounds holes, but I'm pretty sure it would take more than a couple of weeks to get this right. That is why I still think the external mechanism is correct.
So I should also point out some of the risks of the Private mechanism number: Neither my proposal, not Adam's are likely to be accepted for eventual inclusion as an official PKCS #11 mechanism. Here are the things that the PKCS #11 group would likely object to (While the group has been inactive for a while, there appears to be a move to reform it. Many of the previous players are involved).

1) The header pointer could be iffy. Our primary reason for wanting it is preformance of our implementation. That may be enough to carry the day (because the implementation is common).

2) The use of a single mechanism and passing another PKCS #11 mechanism in to select the type is likely to be rejected. I personally prefer the way Adam has set this up, but there is one point that speaks against this is detecting what a module supports. If you have separate mechanisms for each flavor of HMAC, then you can query the PKCS #11 module to see if it supports that HMAC mechanism. If you pass the Hashing mechanism into the params, there is no guarrentee that single HMAC mechanism will support that particular hashing mechanism.

I'm inclined to assume this will never be a fully supported PKCS #11 mechanism as is, and go with the interface supported.
Comment on attachment 703040 [details] [diff] [review]
patch

r-

for the following reasons:

1) I'd like some simplifying changes to the PKCS #11 inteface as described in comment 7.
2) I'd like the bug fix and the nits described in comment 8.

There are also several non-NSS standard style issues, particluarly in the 2 stand alone file. I would not r- to get these fixed, but I would like to see a supplimentary patch if they are not. (Read, they aren't critical enough to hold up a fix, but I'd like to see them fixed as some point).

I think the adjustments required for comment 7 and comment 8 should be relatively straightforward to fix in this patch.

bob
Attachment #703040 - Flags: review?(rrelyea) → review-
Attached patch updated, full patch. (obsolete) — Splinter Review
Robert, thank you for your review! I'm attaching an updated patch and I'll also attach the inter-patch diff after this.

I fear that I don't know the PKCS#11 code well enough to take advantage of your review but, point by point:

> The PKCS #11 API is unnecessarily pushing SSL knowledge down into our constant time functions.

I've updated the code so that construction of the TLS pseudo-header is done within lib/ssl now and the PKCS#11 simply sees an opaque header.

> I would also prefer if we ditched the separate headers altogether (it would simplify the code, as well the the PKCS #11 interfaces). Unfortunately I suspect the performance hit may be too high.

Doing a copy wouldn't be hard and I can certainly do so, but I didn't do it yet because of the overhead. Adjusting all the buffer handling to reverse space for the header seemed like too much for this patch.

> Since we have to be single shot, we also should just use the single shot interface (C_SignInit/C_Sign) as opposed to C_SignInit/C_SignUpdate/C_Sign_Final.

I'm afraid that I wrote the PKCS#11 code in this patch with gdb and blind luck. Can you point me at an example mechanism that implements this style? I might be able to copy it, but I'm not sure what you mean, de novo.

> sftk_MACconstantTime_End is pretty much identical to sftk_HMACCopy()

sftk_HMACCopy takes a length as the first argument, but MACConstantTime_End takes a pointer to its context. Since it's generic code that is calling this function pointer, I didn't see how to unify these.

> the new sftk_DestroyNoop is the same as the existing sftk_Null()

Done.

> Bug: context->hashdestroy destroys the context->hashInfo, context->destroy destroys the context->cipherInfo.

I've switched the functions around, which I hope is what you meant. (I don't understand why there are two destruction functions.)

> There are also several non-NSS standard style issues, particularly in the 2 stand alone file.

I've done a pass for the two mistakes that I usually make (indentation and lowercase_with_underscores variable names.) Please point out any other issues as those are problems that I can, at least, fix!
Attachment #703040 - Attachment is obsolete: true
Attached patch Inter-diff patch. (obsolete) — Splinter Review
Comment on attachment 705483 [details] [diff] [review]
Inter-diff patch.

Review of attachment 705483 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozilla/security/nss/lib/softoken/hmacct.c
@@ +95,5 @@
>      return ctx;
> +
> +loser:
> +    PORT_Free(ctx);
> +    return NULL;

There's no code that calls goto loser here. Unnecessary?

@@ +107,3 @@
>      unsigned int padLength = 40, j;
>  
> +    if (params->headerLength > sizeof(ctx->header) - ctx->secretLength) {

BUG: You haven't setup ctx yet, but you're dereferencing ctx->secretLength.
Attachment #705482 - Flags: review?(rrelyea)
(In reply to Adam Langley from comment #12)
> Created attachment 705482 [details] [diff] [review]
> updated, full patch.
> 
> Robert, thank you for your review! I'm attaching an updated patch and I'll
> also attach the inter-patch diff after this.
> 
> I fear that I don't know the PKCS#11 code well enough to take advantage of
> your review but, point by point:
> 
> > The PKCS #11 API is unnecessarily pushing SSL knowledge down into our constant time functions.
> 
> I've updated the code so that construction of the TLS pseudo-header is done
> within lib/ssl now and the PKCS#11 simply sees an opaque header.
> 
> > I would also prefer if we ditched the separate headers altogether (it would simplify the code, as well the the PKCS #11 interfaces). Unfortunately I suspect the performance hit may be too high.
> 
> Doing a copy wouldn't be hard and I can certainly do so, but I didn't do it
> yet because of the overhead. Adjusting all the buffer handling to reverse
> space for the header seemed like too much for this patch.
> 
> > Since we have to be single shot, we also should just use the single shot interface (C_SignInit/C_Sign) as opposed to C_SignInit/C_SignUpdate/C_Sign_Final.
> 
> I'm afraid that I wrote the PKCS#11 code in this patch with gdb and blind
> luck. Can you point me at an example mechanism that implements this style? I
> might be able to copy it, but I'm not sure what you mean, de novo.

In NSC_SignInit, set context->multi = PR_FALSE

In ssl3con.c, the current code does
PK11_DigestBegin (which calls C_EncryptInit/C_DecryptInit)C_SignInit/C_DigestInit as appropriate)
PK11_DigestOp (which calls C_*Update)
PK11_DigestFinal (which calls C_*Final)

Bob's suggestion is to mirror PK11_Sign, which (internally) handles calling C_SignInit and then calls C_Sign (rather than C_*Update/C*Final).

Call this hypothetical method PK11_SignWithParams. It looks identical to PK11_Sign, except that instead of inferring the mechanism from the SECKEYPrivateKey* type, it takes an explicit mechanism and params. eg:

PK11_SignWithParams(CKA_MECHANISM mechanism, void* params, PK11SymKey* key, SECItem* sig, SECItem* data)

That said, in the above signature (where sig = output, data = input), one clear and problematic way that it doesn't mirror the PK11_Sign case is that PK11_Sign expects a SECKEYPrivateKey, whereas here, we're using a PK11SymKey. The SECKey* interface seems oriented towards public/private key pairs, even if this is not strictly required, and we certainly don't need to require a SECKEY* wrapper for what is essentially an ephemeral key.

Bob, do you feel strongly about this point? Is the concern about the implications of a C_SignUpdate being 'supported' by API, but not by implementation?

Would it make more sense to have a PK11_DigestBuf(...) that matched the above signature (eg: squarely keeping it in the realm of PK11 types)? Is that too confusing with the similarly named PK11_HashBuf?

> 
> > sftk_MACconstantTime_End is pretty much identical to sftk_HMACCopy()
> 
> sftk_HMACCopy takes a length as the first argument, but MACConstantTime_End
> takes a pointer to its context. Since it's generic code that is calling this
> function pointer, I didn't see how to unify these.
> 
> > the new sftk_DestroyNoop is the same as the existing sftk_Null()
> 
> Done.
> 
> > Bug: context->hashdestroy destroys the context->hashInfo, context->destroy destroys the context->cipherInfo.
> 
> I've switched the functions around, which I hope is what you meant. (I don't
> understand why there are two destruction functions.)
> 
> > There are also several non-NSS standard style issues, particularly in the 2 stand alone file.
> 
> I've done a pass for the two mistakes that I usually make (indentation and
> lowercase_with_underscores variable names.) Please point out any other
> issues as those are problems that I can, at least, fix!
> That said, in the above signature (where sig = output, data = input), one clear and 
> problematic way that it doesn't mirror the PK11_Sign case is that PK11_Sign expects a 
> SECKEYPrivateKey, whereas here, we're using a PK11SymKey.

Right, it probably should be called PK11_SignWithSymKey(). It's signature is pretty much what Ryan suggested, except the Key should be first (pk11wrap is pretty universally 'slot' (if present) then key) and the params need to be a SECItem (since they have a length):

PK11_SignWithSymKey(PK11SymKey *symKey, CKA_MECHANISM mechanism, SECItem* params, SECItem* sig, SECItem* data);

Internally we would create a pk11_SignWithHandle() with the signature:

pk11_SignWithHandle(CK_OBJECT_HANDLE key, CK_MECHANISM_INFO mech, SECItem *param, SECItem *sig, SECItem *data);

PK11_Sign() would call pk11_SignWithHandle(key->pkcs11ID, mechanism, NULL, sig, hash) after the automatic get mechanism and check for 'is logged in'

PK11_SignWithSymKey() would call pk11SignWithHandle(symkey->objectID, mechanism, parama, sign, data);

I'll attach a patch for pk11wrap.

For softoken, we simply need to set multi = PR_FALSE; 

> Bob, do you feel strongly about this point? Is the concern about the implications of a 
> C_SignUpdate being 'supported' by API, but not by implementation?

This is pretty much the identical issue we had with GCM and CTS, I think we need to to this for consistancy, and the actual work isn't very much.
> sftk_HMACCopy takes a length as the first argument, but MACConstantTime_End
> takes a pointer to its context. Since it's generic code that is calling this
> function pointer, I didn't see how to unify these.

Oh, right, This is the cipherInfo, which you don't set. There's no reason not to, though. The code is pretty much doing exactly the same thing, with the same purpose. We should make them work the same way.
OK, the common pk11_SignWithHandle() idea won't work because PrivateKeys could have Always authenticate set, and we have to reauthenticate between the SignInit and the Sign, which we don't have to do for Symetric keys.... so basically PK11_SignWithSymKey() is a copy of PK11_Sign() except it takes the mechanism and param, doesn't do the private key authentication, and uses a symKey rather than a PublicKey... patch comming..
This new function also needs to be added to nss/lib/pk11wrap/pk11pub.h and nss/lib/nss/nss.def .
Attached patch updated, full patch. (obsolete) — Splinter Review
Updated patch attached. Delta patch in just a second.

> BUG: You haven't setup ctx yet, but you're dereferencing ctx->secretLength.

Gah, the loser: label was to fix this, but then I had a meeting and forgot :( Fixed.

> This is the cipherInfo, which you don't set. There's no reason not to, though. The code is pretty much doing exactly the same thing, with the same purpose. We should make them work the same way.

Done. I've created sftk_SignCopy. This does alter the HMAC behaviour in that it will now copy a truncated result and return SECSuccess rather than return SECFailure if the output buffer is insufficient. It's not clear which behaviour is better. Let me know if I picked the wrong one.
Attachment #705482 - Attachment is obsolete: true
Attachment #705482 - Flags: review?(rrelyea)
Attached patch Inter-diff patch. (obsolete) — Splinter Review
Attachment #705483 - Attachment is obsolete: true
> Done. I've created sftk_SignCopy. This does alter the HMAC behaviour in that it will now copy
> a truncated result and return SECSuccess rather than return SECFailure if the output buffer
> is insufficient. It's not clear which behaviour is better. Let me know if I picked the wrong
> one.

Normally this is a 6 of one half a dozen of the other case.

The 2 cases issues are 1) ABI compatibility and 2) the PKCS #11 spec. 

Technically ABI compatibility is a bit of an issue, but softoken isn't really part of the NSS ABI guarrentee. If the code actually affects the ABI of NSS proper, then it's an issue, but since NSS currently doesn't supply any way of doing the one-shot symmetric key operations (until this patch), we can be confident a true NSS user was not dependent on the old behaivor.

The PKCS #11 spec is classically vague for the HMAC mechanism, containing a paragraph basically pointing to RFC 2104 and FIPS 198. Here is what FIPS 198 says:

"A well-known practice with MACs is to truncate their outputs (i.e., the length of the
MACs used is less than the length of the output of the HMAC function L). Applications
of this standard may truncate the outputs of the HMAC. When truncation is used, the λ
leftmost bits of the output of the HMAC function shall be used as the MAC. For
information about the choice of λ and the security implications of using truncated outputs
of the HMAC function, see SP 800-107."

and here is what RCS 2104 says: 

"A well-known practice with message authentication codes is to
   truncate the output of the MAC and output only part of the bits
   (e.g., [MM, ANSI]).  Preneel and van Oorschot [PV] show some
   analytical advantages of truncating the output of hash-based MAC
   functions. The results in this area are not absolute as for the
   overall security advantages of truncation. It has advantages (less
   information on the hash result available to an attacker) and
   disadvantages (less bits to predict for the attacker).  Applications
   of HMAC can choose to truncate the output of HMAC by outputting the t
   leftmost bits of the HMAC computation for some parameter t (namely,
   the computation is carried in the normal way as defined in section 2
   above but the end result is truncated to t bits). We recommend that
   the output length t be not less than half the length of the hash
   output (to match the birthday attack bound) and not less than 80 bits
   (a suitable lower bound on the number of bits that need to be
   predicted by an attacker).  We propose denoting a realization of HMAC
   that uses a hash function H with t bits of output as HMAC-H-t. For
   example, HMAC-SHA1-80 denotes HMAC computed using the SHA-1 function
   and with the output truncated to 80 bits.  (If the parameter t is not
   specified, e.g. HMAC-MD5, then it is assumed that all the bits of the
   hash are output.)"

Clearly one is spec is dependent on the other, and both assume truncation is valid. As long as the trunction happens from the left most side, I think your semantic is fine.
> I've switched the functions around, which I hope is what you meant. (I don't
> understand why there are two destruction functions.)

I hope it's clear now with the copy function. There are 2 destructors because there are 2 actual contexts: hashInfo and cipherInfo. Most operations only use one, so they would use, say, the hashInfo/hash_destory or the cipherInfo/destroy combination. Some operations have both: CKM_RSA_SHA1 for instance is an SHA1 hash, which is then signed by an RSA signature. HMAC was sort of imitating the RSA operation so that a single shot HMAC operation would work (the 'encryption' became simply a 'copy'). The other case this can happy is with the SignEncrypt and DecryptVerify functions, which were defined so that hardware tokens could process a stream by taking data once and applying both encrypt/decrypt and sign/verify to it (sort of a primitive version of the modern Authenticated Encryption model).

bob
Attachment #705578 - Flags: review?(rrelyea)
(To be clear, I don't believe that I currently have a TODO for this bug. It goes public on Monday morning but the server side is more important than the client side.)
Comment on attachment 705578 [details] [diff] [review]
updated, full patch.

r+ rrelyea...

I would like to see the single shot implemented, but I won't hold this patch up to get it. It would be easier to check this one is as is and add single shot on top. All my other issues have been addressed, so r+.

bob
Attachment #705578 - Flags: review?(rrelyea) → review+
Comment on attachment 705578 [details] [diff] [review]
updated, full patch.

Review of attachment 705578 [details] [diff] [review]:
-----------------------------------------------------------------

I only reviewed the header files to review the new function prototypes
and look for backward compatibility issues.

Please make the changes I suggested for lib/util/hasht.h and lib/freebl/loader.h
when you check this in. I can take care of the coding style nits later.

::: mozilla/security/nss/lib/freebl/ldvector.c
@@ +263,5 @@
>  
>      /* End of Version 3.014 */
> +
> +    HMAC_ConstantTime,
> +    SSLv3_MAC_ConstantTime

Nit: add a comment
    /* End of Version 3.015 */

::: mozilla/security/nss/lib/freebl/loader.h
@@ +594,5 @@
> +     unsigned int headerLen,
> +     const unsigned char *body,
> +     unsigned int bodyLen,
> +     unsigned int bodyTotalLen);
> + };

The FREEBL_VERSION macro at the beginning of this file needs to be
updated. We should also add a comment here:
  /* Version 3.015 came to here */

::: mozilla/security/nss/lib/softoken/pkcs11i.h
@@ +701,5 @@
> +  unsigned char secret[64];
> +  unsigned int headerLength;
> +  unsigned int secretLength;
> +  unsigned int totalLength;
> +  unsigned char header[75];

Nit: indent by four spaces.

::: mozilla/security/nss/lib/util/hasht.h
@@ +49,4 @@
>      void (*begin)(void *);
>      void (*update)(void *, const unsigned char *, unsigned int);
>      void (*end)(void *, unsigned char *, unsigned int *, unsigned int);
> +    void (*end_raw)(void *, unsigned char *, unsigned int *, unsigned int);

IMPORTANT: hasht.h is a public header and this structure's definition
is public. Last time I added new members to this structure I had to
add them to the end:

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/security/nss/lib/util&command=DIFF_FRAMESET&file=hasht.h&rev2=1.7&rev1=1.6

I highly recommend we add the end_raw member to the end to avoid this
problem.

::: mozilla/security/nss/lib/util/pkcs11n.h
@@ +246,5 @@
> +typedef struct CK_NSS_MACConstantTimeParams {
> +    CK_MECHANISM_TYPE hashAlg; /* in */
> +    CK_ULONG bodyTotalLength;  /* in */
> +    CK_BYTE * header;          /* in */
> +    CK_ULONG headerLength;     /* in */

Nit: PKCS #11 uses the Hungarian notation. CK_ULONG members
should be ulBodyTotalLength and ulHeaderLength and pointer
member should be pHeader.
Adam,

The client side is still important because the WebRTC data channels are carried over
DTLS. That functionality is unimplemented in Chrome and preffed off in Firefox,
but we're pushing to preff it on in Firefox soon.
Adding Randell Jesup because he is owner for the aforementioned data channel code (and the entire WebRTC module).
I'm attaching a series of four patches.

The first is an incremental diff from the previous reviewed patch. This addresses wtc's comments in #26. It also fixes two bugs:

I had twice written

if (*ptr)
  *ptr = value

When I meant to test whether the pointer was non-NULL, not the value that it pointed it.

The second attachment is a full diff that includes this incremental diff.

The third attachment is an incremental diff that builds on the second attachment. This includes Rob's PK11_SignWithSymKey and uses it. (At least I think so; I'm unclear on much of PKCS#11.)

Rob: note that I'd changed the order of the arguments to PK11_SignWithSymKey in order to put the output argument last.

Finally, the forth attachment is a full diff that includes the one-shot code.

I believe that forth attachment is the most suitable for landing in NSS. However, Chrome probably wants to use the libssl code from the second attachment because it doesn't depend on the new PK11_SignWithSymKey function.
Attachment #705580 - Attachment is obsolete: true
Attachment #705578 - Attachment is obsolete: true
Attachment #705562 - Attachment is obsolete: true
Attachment #708763 - Flags: review?(rrelyea)
Attachment #708762 - Flags: review?(rrelyea)
Comment on attachment 708762 [details] [diff] [review]
Inter-diff that converts the code to using PK11_SignWithSymKey

Review of attachment 708762 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozilla/security/nss/lib/ssl/ssl3con.c
@@ +2116,3 @@
>  
> +    rv = PK11_SignWithSymKey(key, macType, &param, &inputItem, &outputItem);
> +    PORT_Assert(rv != SECSuccess || outputItem.len == (unsigned)spec->mac_size);

This ends up removing the fallback path if macType is not supported. (compare with line 2111 of the old code)

However, if we just checked for != SECSuccess (eg: line 2120), then an attacker would be able to exploit this by first trying the constant time, then inducing a fallback to the non-constant-time path in the event of a padding error.

I *think* if we checked for SEC_ERROR_INVALID_ALGORITHM (which corresponds to CKR_MECHANISM_INVALID) in PORT_GetError(), that could be used as a reliable signal for a non-supported mechanism, but Bob would need to confirm that.

If so, something like

rv = PK11_SignWithSymKey(...)
if (rv != SECSuccess) {
    if (PORT_GetError() == SEC_ERROR_INVALID_ALGORITHM) {
        goto fallback;
    }
    *outLen = 0;
    ...
}
(In reply to Ryan Sleevi from comment #33)
> This ends up removing the fallback path if macType is not supported.
> (compare with line 2111 of the old code)

The fallback path is just for those programs that skew libssl and NSS versions, which is just Chrome, no?

I don't think the one-shot code is suitable for Chrome because it depends on PK11_SignWithSymKey, which will be a new function in the next NSS. Thus I think Chrome would use the other patch which includes the fallback and doesn't use the one-shot code.
(In reply to Adam Langley from comment #34)
> (In reply to Ryan Sleevi from comment #33)
> > This ends up removing the fallback path if macType is not supported.
> > (compare with line 2111 of the old code)
> 
> The fallback path is just for those programs that skew libssl and NSS
> versions, which is just Chrome, no?

No, this is for programs that skew softoken and NSS, which includes Red Hat. They use a FIPS-validated softoken (3.12.7?), which won't include this new algorithm, so they'll have to continue to fallback to the old path unless/until they can do a NIST Change Letter (or have clients use the non-FIPS softoken at 3.14.x)

> 
> I don't think the one-shot code is suitable for Chrome because it depends on
> PK11_SignWithSymKey, which will be a new function in the next NSS. Thus I
> think Chrome would use the other patch which includes the fallback and
> doesn't use the one-shot code.
Ah, I see. I've updated the one-shot code as suggested and have tested it by passing in an invalid mechanism type and checking that I can see load CBC sites.
Attachment #709092 - Flags: review?(rrelyea)
Attachment #708762 - Attachment is obsolete: true
Attachment #708763 - Attachment is obsolete: true
Attachment #708762 - Flags: review?(rrelyea)
Attachment #708763 - Flags: review?(rrelyea)
Attachment #709093 - Flags: review?(rrelyea)
This is now public: http://www.isg.rhul.ac.uk/tls/ so I think the security bit can be removed from this bug.

Given the complexity of the patch, I suggest that we let it bake in Chrome/Firefox nighties for a couple of days before we cut a new NSS release.
Summary: Non-constant time CBC decoding results in padding oracle → Non-constant time CBC decoding results in padding oracle (Lucky Thirteen attack)
wtc noted a few places where I had braces on the same line as a function. This patch (and the following one in just a sec) fix this.
Attachment #709093 - Attachment is obsolete: true
Attachment #709093 - Flags: review?(rrelyea)
Attachment #709843 - Flags: review?(rrelyea)
This also corrects the brace positioning that wtc noted and also duplicates a bit of the NSS header files into ssl3con.c so that it can be built with an older version of the NSS headers.

This is not intended for NSS itself, but rather for code that builds libssl separately from NSS.
Attachment #708761 - Attachment is obsolete: true
Attachment #709844 - Flags: review?(rrelyea)
Group: core-security
Comment on attachment 709843 [details] [diff] [review]
Full patch using PK11_SignWithSymKey; suitable for NSS.

I will work on checking this patch in today.
This patch is the same as Adam's patch in attachment 709843 [details] [diff] [review].

I made only two changes.

1. I resolved the merge conflict in lib/pk11wrap/pk11pub.h.

2. I renamed lib/softoken/hmacct.c to lib/softoken/sftkhmac.c.
This patch already added a file named hmacct.c to lib/freebl.

I am going to propose some coding style changes, so this patch
will be used as a baseline.
Attachment #709843 - Attachment is obsolete: true
Attachment #709843 - Flags: review?(rrelyea)
Attachment #709969 - Flags: review?(rrelyea)
Attachment #709969 - Attachment is obsolete: true
Attachment #709969 - Flags: review?(rrelyea)
Attachment #710008 - Flags: review?(rrelyea)
Fall back on the non-constant time ssl3_ComputeRecordMAC for
SSL3_RSA_EXPORT_WITH_RC2_CBC_40_MD5.

Ekr: please test this patch with DTLS and WebRTC.

Adam: In gdb I found that in tstclnt, ssl3_HandleRecord, the
MAC computed by ssl3_ComputeRecordMACConstantTime is different
from |givenHash| so the NSS_SecureMemcmp call failed.
This gave me the idea of working around this by falling back
on ssl3_ComputeRecordMAC for SSL3_RSA_EXPORT_WITH_RC2_CBC_40_MD5.

Does this give you an idea of what might be wrong?
ssl3_ComputeRecordMACConstantTime and ssl3_ComputeRecordMAC
are not too long.
Attachment #710008 - Attachment is obsolete: true
Attachment #710008 - Flags: review?(rrelyea)
Attachment #710038 - Flags: review?(rrelyea)
Attachment #710038 - Flags: feedback?(ekr)
I'd guess it's a problem with MD5 HMAC. This cipher is the only block cipher that used MD5 in our cipher suite.

bob
Ah, good point. I only focused on the export aspect last night.
But the export aspect does not affect the write MAC secrets
(only the write keys and the write IVs).
I reordered the input and output arguments of PK11_SignWithSymKey
to match the NSS convention for such functions (output before input).
I also renamed the input argument |hash| to |data| because when
generating an SSLv3 MAC or HMAC, the input isn't a hash.

I fixed the symbol version of the new function in nss.def (but
I just realized I made a mistake, sigh!) and bumped FREEBL_VERSION.

Patch checked in on the NSS trunk (NSS 3.14.3).

Checking in lib/freebl/blapi.h;
/cvsroot/mozilla/security/nss/lib/freebl/blapi.h,v  <--  blapi.h
new revision: 1.50; previous revision: 1.49
done
RCS file: /cvsroot/mozilla/security/nss/lib/freebl/hmacct.c,v
done
Checking in lib/freebl/hmacct.c;
/cvsroot/mozilla/security/nss/lib/freebl/hmacct.c,v  <--  hmacct.c
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/lib/freebl/hmacct.h,v
done
Checking in lib/freebl/hmacct.h;
/cvsroot/mozilla/security/nss/lib/freebl/hmacct.h,v  <--  hmacct.h
initial revision: 1.1
done
Checking in lib/freebl/ldvector.c;
/cvsroot/mozilla/security/nss/lib/freebl/ldvector.c,v  <--  ldvector.c
new revision: 1.33; previous revision: 1.32
done
Checking in lib/freebl/loader.c;
/cvsroot/mozilla/security/nss/lib/freebl/loader.c,v  <--  loader.c
new revision: 1.59; previous revision: 1.58
done
Checking in lib/freebl/loader.h;
/cvsroot/mozilla/security/nss/lib/freebl/loader.h,v  <--  loader.h
new revision: 1.39; previous revision: 1.38
done
Checking in lib/freebl/manifest.mn;
/cvsroot/mozilla/security/nss/lib/freebl/manifest.mn,v  <--  manifest.mn
new revision: 1.67; previous revision: 1.66
done
Checking in lib/freebl/md5.c;
/cvsroot/mozilla/security/nss/lib/freebl/md5.c,v  <--  md5.c
new revision: 1.17; previous revision: 1.16
done
Checking in lib/freebl/rawhash.c;
/cvsroot/mozilla/security/nss/lib/freebl/rawhash.c,v  <--  rawhash.c
new revision: 1.12; previous revision: 1.11
done
Checking in lib/freebl/sha512.c;
/cvsroot/mozilla/security/nss/lib/freebl/sha512.c,v  <--  sha512.c
new revision: 1.22; previous revision: 1.21
done
Checking in lib/freebl/sha_fast.c;
/cvsroot/mozilla/security/nss/lib/freebl/sha_fast.c,v  <--  sha_fast.c
new revision: 1.16; previous revision: 1.15
done
Checking in lib/freebl/sha_fast.h;
/cvsroot/mozilla/security/nss/lib/freebl/sha_fast.h,v  <--  sha_fast.h
new revision: 1.11; previous revision: 1.10
done
Checking in lib/nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.223; previous revision: 1.222
done
Checking in lib/pk11wrap/pk11obj.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11obj.c,v  <--  pk11obj.c
new revision: 1.30; previous revision: 1.29
done
Checking in lib/pk11wrap/pk11pub.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pub.h,v  <--  pk11pub.h
new revision: 1.43; previous revision: 1.42
done
Checking in lib/softoken/manifest.mn;
/cvsroot/mozilla/security/nss/lib/softoken/manifest.mn,v  <--  manifest.mn
new revision: 1.42; previous revision: 1.41
done
Checking in lib/softoken/pkcs11.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.184; previous revision: 1.183
done
Checking in lib/softoken/pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.135; previous revision: 1.134
done
Checking in lib/softoken/pkcs11i.h;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11i.h,v  <--  pkcs11i.h
new revision: 1.64; previous revision: 1.63
done
RCS file: /cvsroot/mozilla/security/nss/lib/softoken/sftkhmac.c,v
done
Checking in lib/softoken/sftkhmac.c;
/cvsroot/mozilla/security/nss/lib/softoken/sftkhmac.c,v  <--  sftkhmac.c
initial revision: 1.1
done
Checking in lib/ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.198; previous revision: 1.197
done
Checking in lib/util/hasht.h;
/cvsroot/mozilla/security/nss/lib/util/hasht.h,v  <--  hasht.h
new revision: 1.11; previous revision: 1.10
done
Checking in lib/util/pkcs11n.h;
/cvsroot/mozilla/security/nss/lib/util/pkcs11n.h,v  <--  pkcs11n.h
new revision: 1.29; previous revision: 1.28
done
Attachment #710038 - Attachment is obsolete: true
Attachment #710038 - Flags: review?(rrelyea)
Attachment #710038 - Flags: feedback?(ekr)
Attachment #710274 - Flags: checked-in+
Attached patch Patch to address MD5 weaknesses. (obsolete) — Splinter Review
I managed to get the tests running with NSS_ENABLE_ECC=0 (which would have been useful before!)

There are two issues with MD5.

The first is a silly mistake, the mechanism is CKM_SSL3_MD5_MAC, not CKM_MD5 in this context.

The second is more substantial: I wasn't aware that the endianness of the length in the hash padding changed from MD5 to SHA-1.

Both of these issues are addressed in the attached patch.
Attachment #710311 - Flags: review?(wtc)
Comment on attachment 710311 [details] [diff] [review]
Patch to address MD5 weaknesses.

r+rrelyea
Attachment #710311 - Flags: review+
Comment on attachment 709844 [details] [diff] [review]
Full patch not using one-shot. Suitable for Chrome.

wtc's patch superceded this one, clearing review
Attachment #709844 - Flags: review?(rrelyea)
Comment on attachment 709092 [details] [diff] [review]
Inter-diff that converts the code to using PK11_SignWithSymKey

This patch looks good, but it has been superceded, clearin review.
Attachment #709092 - Flags: review?(rrelyea)
1. The SHA_HTONL macro needs a local variable named 'tmp'.

Note: SHA1_End reuses the local variable 'lenB' for that purpose.
Since SHA1_EndRaw does not need lenB, I moved "#undef lenB" to
the end of SHA1_End.

2. Visual C++ still doesn't allow declaring variables in the middle
of a block.
Attachment #710409 - Flags: review?(kaie)
Comment on attachment 710409 [details] [diff] [review]
Fix compilation errors on Android and Windows

r=kaie
Attachment #710409 - Flags: review?(kaie) → review+
Comment on attachment 710409 [details] [diff] [review]
Fix compilation errors on Android and Windows

Patch checked in on the NSS trunk (NSS 3.14.3).

Checking in hmacct.c;
/cvsroot/mozilla/security/nss/lib/freebl/hmacct.c,v  <--  hmacct.c
new revision: 1.2; previous revision: 1.1
done
Checking in sha_fast.c;
/cvsroot/mozilla/security/nss/lib/freebl/sha_fast.c,v  <--  sha_fast.c
new revision: 1.17; previous revision: 1.16
done
Attachment #710409 - Flags: checked-in+
Alias: CVE-2013-1620
Both SHA256_EndRaw and SHA512_EndRaw need a local variable named t1
for the BYTESWAP4 and BYTESWAP8 macros in certain build configurations.

Patch checked in on the NSS trunk (NSS 3.14.3).

Checking in sha512.c;
/cvsroot/mozilla/security/nss/lib/freebl/sha512.c,v  <--  sha512.c
new revision: 1.23; previous revision: 1.22
done
Attachment #710435 - Flags: review?(kaie)
Attachment #710435 - Flags: checked-in+
Comment on attachment 710311 [details] [diff] [review]
Patch to address MD5 weaknesses.

Review of attachment 710311 [details] [diff] [review]:
-----------------------------------------------------------------

This fixed the test failure for SSL3_RSA_EXPORT_WITH_RC2_CBC_40_MD5.

::: mozilla/security/nss/lib/freebl/hmacct.c
@@ +174,5 @@
>      }
> +    if (hashObj->type == HASH_AlgMD5) {
> +	/* MD5 appends a little-endian length. */
> +	for (i = 0; i < 4; i++) {
> +	    lengthBytes[i+j] = bits >> (8*i);

Just curious: why is the index for lengthBytes also different from
the non-MD5 case? (i+j vs. 4+i+j)

::: mozilla/security/nss/lib/softoken/hmacct.c
@@ +106,4 @@
>  	return NULL;
>      }
>  
> +    if (params->hashAlg == CKM_SSL3_MD5_MAC) {

Does this mean the |hashAlg| field is incorrectly named?
Should it be renamed |macAlg|?
Attachment #710311 - Flags: review?(wtc) → review+
Also remove the workaround for this bug from ssl3_ComputeRecordMACConstantTime.

Patch checked in on the NSS trunk (NSS 3.14.3).

Checking in lib/freebl/hmacct.c;
/cvsroot/mozilla/security/nss/lib/freebl/hmacct.c,v  <--  hmacct.c
new revision: 1.3; previous revision: 1.2
done
Checking in lib/softoken/sftkhmac.c;
/cvsroot/mozilla/security/nss/lib/softoken/sftkhmac.c,v  <--  sftkhmac.c
new revision: 1.2; previous revision: 1.1
done
Checking in lib/ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.199; previous revision: 1.198
done
Attachment #710311 - Attachment is obsolete: true
Attachment #710460 - Flags: checked-in+
Checking in nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.224; previous revision: 1.223
done
Attachment #710462 - Flags: checked-in+
Adam: there is a strange problem. On Linux, optimized build, the tests fail.
I tracked it down to the 'mac' function in lib/freebl/hmacct.c. If I compile
that function with optimization flag -O0, then the tests pass. The tests fail
even at -O1. I stared at the function for a while but could not see what is
wrong.
Target Milestone: 3.14.2 → 3.14.3
The variable |bits| is unsigned int, so we should not shift
|bits| for more than 32 bits.

I actually noticed the strange 7-i before, but I tried many
other things blindly in vain before I suspected this. If I
unroll the for loop so that the right operands of the >>
operators are constants, the compiler warns about them.

I don't know why this only breaks in optimized builds. (I
didn't inspect the generated assembly code.)

Checking in hmacct.c;
/cvsroot/mozilla/security/nss/lib/freebl/hmacct.c,v  <--  hmacct.c
new revision: 1.4; previous revision: 1.3
done
Attachment #710499 - Flags: review?(agl)
Attachment #710499 - Flags: checked-in+
The build on PowerPC 32 bit on our OSX machine still fails with
md5.c: In function 'MD5_EndRaw':
md5.c:550: error: 'tmp' undeclared (first use in this function)
md5.c:550: error: (Each undeclared identifier is reported only once

I've tested the attached patch and it fixes the build.

The patch uses a larger context, allowing you to quickly see that I duplicated the approach used in the previous function.

Checking in freebl/md5.c;
/cvsroot/mozilla/security/nss/lib/freebl/md5.c,v  <--  md5.c
new revision: 1.18; previous revision: 1.17
done
Attachment #710617 - Flags: review?(wtc)
Attachment #710617 - Flags: checked-in+
> Just curious: why is the index for lengthBytes also different from
> the non-MD5 case? (i+j vs. 4+i+j)

Because in both cases it's a 32-bit value packed into a 64-bit stream. In the little endian case it's packed at the beginning and the big endian case it's packed at the end.

bob
Arg... There's a bug here in the mac count still...

in mac(), the variable 'bits' is an unsigned int. The internal counters for the MAC are 64 bits. Now all the input lengths are  unsigned int, which on many platforms are actually 32 bit, so we may not be able to push more than 2^32 bytes of data through, (though we have a header plus mac). This still are platforms where unsigned int is 64 bits, and bits is the number of bits hashed (not bytes hashed), so we multiply it by 8.

We may be OK because the size of an SSL packet is limited (to something considerably less than 2^32). We should at least comment this issue.


bob
Attachment #710617 - Flags: review?(wtc) → review+
Our PowerPC-64bit machine crashes during the SSL tests.
(That machine had been restarted because of an OS upgrade to Fedora 18, I just brought the tests back online.)

Many tests crash, it's only at test #1000, and we already have 117 core files.

#0  0x0000008025598b10 in .raise () from /lib64/libc.so.6
#1  0x000000802559ab88 in .abort () from /lib64/libc.so.6
#2  0x00003fffb12d201c in PR_Assert (s=0x3fffb17039b8 "outputItem.len == (unsigned)spec->mac_size", file=0x3fffb1703428 "ssl3con.c", ln=2137) at ../../../../pr/src/io/prlog.c:554
#3  0x00003fffb16ae4e8 in ssl3_ComputeRecordMACConstantTime (spec=0x10035039330, useServerMacKey=0, isDTLS=0, type=content_application_data, version=770, seq_num=..., 
    input=0x1003500d9c0 "GET / HTTP/1.0\r\n\r\n\347:\305\360Y=\261\"\020\027ؽ\033\303da\330\071\246x\t\t\t\t\t\t\t\t\t\t\327\270\235\346\302E7J\277]g>\207!\n\bد\375\204a \344\375\366p\277\372\365\255\032&W\201Vζ\355q\304zf\222\036&@\207-߁\020=\231\255\337\314@\316\fuFW\343#r\255\270cOT\341O=o\330-sXM\256\315\373\221\252t\210\371\021 \037#a\023", inputLen=38, originalLen=48, outbuf=0x3fffb0d3b230 "\201a\310:\230\234\300\002", outLen=0x3fffb0d3b22c) at ssl3con.c:2137
#4  0x00003fffb16c5df0 in ssl3_HandleRecord (ss=0x10035037510, cText=0x3fffb0d3b380, databuf=0x100350378a8) at ssl3con.c:10054
#5  0x00003fffb16c8944 in ssl3_GatherCompleteHandshake (ss=0x10035037510, flags=0) at ssl3gthr.c:361
#6  0x00003fffb16c8b80 in ssl3_GatherAppDataRecord (ss=0x10035037510, flags=0) at ssl3gthr.c:404
#7  0x00003fffb16e02f4 in DoRecv (ss=0x10035037510, out=0x3fffb0d3bab8 "", len=10239, flags=0) at sslsecur.c:535
#8  0x00003fffb16e1f44 in ssl_SecureRecv (ss=0x10035037510, buf=0x3fffb0d3bab8 "", len=10239, flags=0) at sslsecur.c:1144
#9  0x00003fffb16e2024 in ssl_SecureRead (ss=0x10035037510, buf=0x3fffb0d3bab8 "", len=10239) at sslsecur.c:1153
#10 0x00003fffb16ef768 in ssl_Read (fd=0x10034edf300, buf=0x3fffb0d3bab8, len=10239) at sslsock.c:2112
#11 0x00003fffb12cb2f4 in PR_Read (fd=0x10034edf300, buf=0x3fffb0d3bab8, amount=10239) at ../../../../pr/src/io/priometh.c:109
#12 0x00000000100081a4 in handle_connection (tcp_sock=0x10034edf300, model_sock=0x10034e66400, requestCert=1) at selfserv.c:1125
#13 0x0000000010007128 in jobLoop (a=0x0, b=0x0, c=1) at selfserv.c:601
#14 0x0000000010006f0c in thread_wrapper (arg=0x10034ecc820) at selfserv.c:570
#15 0x00003fffb1308644 in _pt_root (arg=0x10034eccbc0) at ../../../../pr/src/pthreads/ptthread.c:156
#16 0x000000802575c5f4 in .start_thread () from /lib64/libpthread.so.0
#17 0x000000802565ac70 in .__clone () from /lib64/libc.so.6
  PORT_Assert(outputItem.len == (unsigned)spec->mac_size);

rv == success
outputitem.len == 0
Attachment #710435 - Flags: review?(kaie) → review+
Regarding the crash on ppc64:

In function NSC_SignFinal, in this call:
  (context->update)(context->cipherInfo, pSignature,
                    &outlen, maxoutlen, tmpbuf, digestLen)

we arrive in function sftk_SignCopy.
That function interprets the first argument, which is a void* pointer to a keyWrapContext, as a CK_ULONG*.

It retrieves value 85899345920 which is 0x1400000000.

(gdb) print 85899345920 >> 32
$3 = 20

20 is the value we seem to require.

However, it's in the incorrect endianness.

Because (unsigned int)85899345920 == 0, the function simply fails.
#0  NSC_SignFinal (hSession=237, pSignature=0x3fffb45eb230 "\345\240\353[y\242", <incomplete sequence \355>, pulSignatureLen=0x3fffb45eafd0) at pkcs11c.c:2455
#1  0x00003fffb7934f70 in NSC_Sign (hSession=237, 
    pData=0x1018e5a0 "GET / HTTP/1.0\r\n\r\nSS\027\273\347\037uEN\016\347\021\256\277d\367.k\243h\t\t\t\t\t\t\t\t\t\t\202;\322\035\234\066\373\fa\374\fv\253\251>ث\375\r\341J\222\236\221Z\331\022{\364\207(\351\034\322\005C\345\272\071\070\256\344\324\060\033\212\333\025\201\271\030\325x24\336*N\031\243\317j1\244jZ\244r\244\304\002Q\217)ή\325\357\037XЩ1\343$O", <incomplete sequence \345>, ulDataLen=38, pSignature=0x3fffb45eb230 "\345\240\353[y\242", <incomplete sequence \355>, pulSignatureLen=0x3fffb45eafd0) at pkcs11c.c:2511
#2  0x00003fffb7d447a0 in PK11_SignWithSymKey (symKey=0x3fffac00f730, mechanism=3461563235, param=0x3fffb45eb0c0, sig=0x3fffb45eb0f0, data=0x3fffb45eb0d8) at pk11obj.c:813
Ah...sftk_SignCopy is expecting a CKU_LONG *, but CKA_HMAC_CONSTANT_TIME is only passing an unsigned int. I bet unsigned int is 32-bits on ppc64. Try this patch:

Index: pkcs11c.c
===================================================================
RCS file: /cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v
retrieving revision 1.135
diff -u -p -r1.135 pkcs11c.c
--- pkcs11c.c	5 Feb 2013 18:10:45 -0000	1.135
+++ pkcs11c.c	6 Feb 2013 21:29:31 -0000
@@ -2250,13 +2250,13 @@ finish_rsa:
 
     case CKM_NSS_HMAC_CONSTANT_TIME: {
 	sftk_MACConstantTimeCtx *ctx = sftk_HMACConstantTime_New(pMechanism,key);
-	int *intpointer;
+	CK_ULONG *intpointer;
 
 	if (ctx == NULL) {
 	    crv = CKR_ARGUMENTS_BAD;
 	    break;
 	}
-	intpointer = PORT_Alloc(sizeof(int));
+	intpointer = PORT_Alloc(sizeof(CK_ULONG));
 	if (intpointer == NULL) {
 	    crv = CKR_HOST_MEMORY;
 	    break;
Comment on attachment 710946 [details] [diff] [review]
Fix ppc64 endianness bug

Patch created by Wan-Teh, I only was the one who typed it in...
Attachment #710946 - Flags: review?
1. The naming convention changes are in lib/util/pkcs11n.h.
Please review that file first. These changes are described
in detail at

https://codereview.chromium.org/12193010/diff/17003/net/third_party/nss/ssl/ssl3con.c#newcode2040

and have been approved by Adam.

2. Miscellaneous coding style changes.

- Format a function definition as follows:

  [static] ReturnType
  FunctionName(arguments)
  {
      code
  }

- Fold long lines.

- The 'mac' function in lib/freebl/hmacct.c is renamed 'MAC'.

- Change 'unsigned' to 'unsigned int'.
Attachment #710949 - Flags: superreview?(rrelyea)
Attachment #710949 - Flags: review?(kaie)
Comment on attachment 710946 [details] [diff] [review]
Fix ppc64 endianness bug

r=wtc. Thanks!
Attachment #710946 - Flags: review? → review+
Comment on attachment 710946 [details] [diff] [review]
Fix ppc64 endianness bug

Checking in softoken/pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.136; previous revision: 1.135
done
Attachment #710946 - Flags: review+
Attachment #710946 - Flags: checked-in+
Comment on attachment 710949 [details] [diff] [review]
PKCS #11 naming convention, NSS coding style fixes

r+ thanks wtc!

bob
Attachment #710949 - Flags: superreview?(rrelyea) → superreview+
Comment on attachment 710949 [details] [diff] [review]
PKCS #11 naming convention, NSS coding style fixes

Patch checked in on the NSS trunk (NSS 3.14.3).

Checking in lib/freebl/hmacct.c;
/cvsroot/mozilla/security/nss/lib/freebl/hmacct.c,v  <--  hmacct.c
new revision: 1.5; previous revision: 1.4
done
Checking in lib/freebl/loader.c;
/cvsroot/mozilla/security/nss/lib/freebl/loader.c,v  <--  loader.c
new revision: 1.60; previous revision: 1.59
done
Checking in lib/freebl/md5.c;
/cvsroot/mozilla/security/nss/lib/freebl/md5.c,v  <--  md5.c
new revision: 1.19; previous revision: 1.18
done
Checking in lib/softoken/pkcs11.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.185; previous revision: 1.184
done
Checking in lib/softoken/pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.137; previous revision: 1.136
done
Checking in lib/softoken/sftkhmac.c;
/cvsroot/mozilla/security/nss/lib/softoken/sftkhmac.c,v  <--  sftkhmac.c
new revision: 1.3; previous revision: 1.2
done
Checking in lib/ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.200; previous revision: 1.199
done
Checking in lib/util/pkcs11n.h;
/cvsroot/mozilla/security/nss/lib/util/pkcs11n.h,v  <--  pkcs11n.h
new revision: 1.30; previous revision: 1.29
done
Attachment #710949 - Flags: review?(kaie) → checked-in+
Bob: re: your comment 64: unsigned int cannot be 64 bits because
short has to be 16 bits, otherwise the platform would be without
a 16-bit integer type. I added the comment you suggested to
pkcs11n.h. I thought about checking for that length limit, but
found some of the functions involved in the MAC calculation
return void, so error reporting is difficult.

I suggest that we rename the 'hashAlg' field to 'macAlg' to
make it clear it is a MAC mechanism, not a hash mechanism.
An alternative fix is to do the MAC-to-hash mapping in libSSL
and store a hash mechanism in that field. Would you prefer
that? I think it is better to pass a MAC mechanism to PKCS #11.

Finally, the change to lib/softoken/pkcs11c.c is to remove a
redundant null pointer check.
Attachment #711011 - Flags: review?(rrelyea)
> Bob: re: your comment 64: unsigned int cannot be 64 bits because
> short has to be 16 bits, otherwise the platform would be without
> a 16-bit integer type.

right, so short=16, int=32, long=platform dependent, long long=64 (if it exists).

Kai was right, it was an endian issue (combined with the int/long mismatch). On little endian machines, it size mismatches cancel out:

char *a;
short b =0;
short c =1;

a= (char *)&b;
*a = c;

printf("%d\n",b);

On little endian machines this will print '1' on big endian machines it will print 256.

bob
Comment on attachment 711011 [details] [diff] [review]
Prevent the confusion of hash vs. MAC mechanism, add a comment about the 2^32 byte limit

r+ rrelyea
Attachment #711011 - Flags: review?(rrelyea) → review+
Bob: the CK_ULONG vs. int bug resulted in reading 4 bytes beyond the
4-byte int. (We allocated PORT_Alloc(sizeof(int)) but read it as
CK_ULONG.) So it was worse than your example. We were only lucky
that those four bytes were all zeros. (Probably guaranteed by malloc's
allocation granularity.) But I think valgrind would have complained
about the read.
Yes, in the big endian case. In the little endian case we read the data then threw it away.

sftk_SignCopy has the following code:

--------------------------
SECStatus
sftk_SignCopy(CK_ULONG *copyLen, .... )

{
    unsigned int toCopy = *copyLen;
------------------

In little endian, We'll read the extra 4 bytes of garbage. As long as we don't page fault, we won't notice because the assignment to toCopy will truncate the garbage off. In big endian the assignment will assign the garbage.

Upshot: only a big endian 64 bit machine would have detected the bug at all. (barring mysterious page faults depending on how malloc is implemented).

>  But I think valgrind would have complained about the read.

yes, well assuming it wasn't optimized out (would the compiler notice it was only reading 4 bytes in the little endian case?).

bob
Comment on attachment 711011 [details] [diff] [review]
Prevent the confusion of hash vs. MAC mechanism, add a comment about the 2^32 byte limit

Checking in hmacct.c;
/cvsroot/mozilla/security/nss/lib/freebl/hmacct.c,v  <--  hmacct.c
new revision: 1.6; previous revision: 1.5
done
Checking in pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.138; previous revision: 1.137
done
Checking in lib/softoken/sftkhmac.c;
/cvsroot/mozilla/security/nss/lib/softoken/sftkhmac.c,v  <--  sftkhmac.c
new revision: 1.4; previous revision: 1.3
done
Checking in lib/ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.201; previous revision: 1.200
done
Checking in lib/util/pkcs11n.h;
/cvsroot/mozilla/security/nss/lib/util/pkcs11n.h,v  <--  pkcs11n.h
new revision: 1.31; previous revision: 1.30
done
Attachment #711011 - Flags: checked-in+
Because of technical issues on the dev-tech-crypto mailing list,
here is the full text including the links.


The NSS team has worked on a fix for the "Lucky Thirteen" Attack
  http://www.isg.rhul.ac.uk/tls/
and has published a beta release which includes that work
  ftp://ftp.mozilla.org/pub/mozilla.org/security/nss/beta/NSS_3_14_3_BETA1/src/

We'd like to invite users of the NSS library to participate in beta
testing. Could you please provide your feedback prior to Thursday next
week, February 14th?

We are looking forward to your feedback on this list, or in the Bugzilla
entry 
  https://bugzilla.mozilla.org/show_bug.cgi?id=822365
which tracks this work.

Thanks in advance for your help.
Status update:

1. Ekr has tested the WebRTC and DRLS code in Firefox with this patch.

I have tested this patch with a softoken that doesn't support the
new constant-time MAC PKCS #11 mechanisms and verified the fallback
on the old ssl3_ComputeRecordMAC function is happening as expected.

2. I've asked Adam to document the new constant-time MAC functions
and PKCS #11 mechanisms in hmacct.h and pkcs11n.h. I think it is
sufficient to just document the PKCS #11 mechanisms in pkcs11n.h.

I will document the new SHA256~512_EndRaw functions.
Attached patch Add comments to pkcs11n.h (obsolete) — Splinter Review
wtc: is this what you had in mind?
Attachment #713102 - Flags: review?(wtc)
Yes, this is what I had in mind. Thanks!

(I made minor edits.)

Checking in pkcs11n.h;
/cvsroot/mozilla/security/nss/lib/util/pkcs11n.h,v  <--  pkcs11n.h
new revision: 1.32; previous revision: 1.31
done
Attachment #713102 - Attachment is obsolete: true
Attachment #713102 - Flags: review?(wtc)
Attachment #713118 - Flags: checked-in+
I standardize the comments (use "current state" instead of "raw state")
and fixed a typo in the comment for SHA1_EndRaw (16 -> 20).
Attachment #713120 - Flags: review?(agl)
Can this bug be marked as resolved fixed?
Attachment #713120 - Flags: review?(agl) → review+
Dan, can we get a CVE for this?

Also, I think that the severity of this bug depends a lot on the application. For current versions of Firefox, I think it could be rated a lot lower. But, for other applications (including, especially DTLS applications), it could be legitimately rated sec-high. How to handle this?
Flags: needinfo?(dveditz)
MITRE already assigned one, CVE-2013-1620, which reed added last week (see bug alias)

The sec-high rating already on the bug seems appropriate. Released versions of Firefox were not as vulnerable, but that is what the severity would have been if we had shipped webRTC (uses DTLS) with this bug still present. I'd lean toward rating the future advisory (when we ship the fix) as "moderate" overall because the advisories are more focused on impact to people using our released products. (In fact, you could argue "low" is appropriate for most web users).
Flags: needinfo?(dveditz)
Comment on attachment 713120 [details] [diff] [review]
Document the <Hash>_EndRaw functions

Checking in blapi.h;
/cvsroot/mozilla/security/nss/lib/freebl/blapi.h,v  <--  blapi.h
new revision: 1.51; previous revision: 1.50
done
Attachment #713120 - Flags: checked-in+
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 839141
ESR was upgraded to 3.14.3 in bug 839141.
I'm trying to build NSS on Solaris x64 with Solaris Studio 12.3. It looks like lib/freebl/sha-fast-amd64-sun.s doesn't include support for SHA1_EndRaw that was added to address this issue.

I can attempt to prepare a patch to remove sha-fast-amd64-sun.s and switch the makefile to the common SHA code if that's acceptable, but fixing sha-fast-amd64-sun.s is outside my present capabilities (I never learned x86/x64 assembly and don't have time now).
Depends on: CVE-2013-1739
(In reply to Chris Newman from comment #93)
> I'm trying to build NSS on Solaris x64 with Solaris Studio 12.3. It looks
> like lib/freebl/sha-fast-amd64-sun.s doesn't include support for SHA1_EndRaw
> that was added to address this issue.
> 
> I can attempt to prepare a patch to remove sha-fast-amd64-sun.s and switch
> the makefile to the common SHA code if that's acceptable, ...

Yes, that is acceptable. You can just undo the last patch in bug 285932.
Please open a new bug for this work.

I remember sha-fast-amd64-sun.s was added because at that time, GCC generated
faster code than the Sun Studio compiler when compiling sha_fast.c. I don't
know if that is still case.

An alternative solution is to regenerate sha-fast-amd64-sun.s by compiling
sha_fast.c with gcc -O3. But I think that's a maintenance burden. It's just
not obvious that we need to regenerate sha-fast-amd64-sun.s whenever sha_fast.c
is updated.
Depends on: 921090
No longer depends on: 921090
You need to log in before you can comment on or make changes to this bug.