1276618 - Follow-up to SHA-384 cipher suite patches

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

I reviewed https://hg.mozilla.org/projects/nss/rev/3522552c289a again and think I found two issues that should be fixed. Although they don't seem to fix/break any existing tests.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1276618-Fix-tls13_GetHashSize-and-tls13_RecoverW.patch

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1276618-A-few-cleanups-after-the-landing-of-SHA-.patch, v2

Some more cleanup.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1276618-A-few-cleanups-after-the-landing-of-SHA-.patch, v3

Comment 4

[Martin Thomson [:mt:]]

Comment on attachment 8757918 [details] [diff] [review]
0001-Bug-1276618-A-few-cleanups-after-the-landing-of-SHA-.patch, v3

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

Yikes, those are pretty bad :)

::: lib/ssl/ssl3con.c
@@ -4750,5 @@
>  {
>      PRUint8 serialized[2];
>      SECOidTag hashAlg = ssl3_TLSHashAlgorithmToOID(sigAndHash->hashAlg);
>      if (hashAlg == SEC_OID_UNKNOWN) {
> -        PORT_Assert(hashAlg != SEC_OID_UNKNOWN);

I wonder, what do you think the rules need to be here?  This was in the SHA-384 patch, and I thought that it was OK.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Martin Thomson [:mt:] from comment #4)
> ::: lib/ssl/ssl3con.c
> @@ -4750,5 @@
> >  {
> >      PRUint8 serialized[2];
> >      SECOidTag hashAlg = ssl3_TLSHashAlgorithmToOID(sigAndHash->hashAlg);
> >      if (hashAlg == SEC_OID_UNKNOWN) {
> > -        PORT_Assert(hashAlg != SEC_OID_UNKNOWN);
> 
> I wonder, what do you think the rules need to be here?  This was in the
> SHA-384 patch, and I thought that it was OK.

I found it harder to read than PORT_Assert(0), and if the condition ever changes we wouldn't need to touch the code in the branch. Guess I also don't really like the repetition here :)

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/projects/nss/rev/60cd5f78dc3e

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

tls13con.c:383:17: error: variable ‘hashType’ set but not used [-Werror=unused-but-set-variable]

https://hg.mozilla.org/projects/nss/rev/d46591f44da4