1283165 - [Coverity 1362937+1362936] Fix some TLS 1.3 issues reported by Coverity

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

*** CID 1362937:  Null pointer dereferences  (FORWARD_NULL)
/lib/ssl/tls13hkdf.c: 75 in tls13_HkdfExtract()
69         } else {
70             /* Per documentation for CKM_NSS_HKDF_*:
71              *
72              *  If the optional salt is given, it is used;
otherwise, the salt is
73              *  set to a sequence of zeros equal in length to the
HMAC output.
74              */
>>>     CID 1362937:  Null pointer dereferences  (FORWARD_NULL)
>>>     Assigning: "params.pSalt" = "NULL".
75             params.pSalt = NULL;
76             params.ulSaltLen = 0UL;
77         }
78         paramsi.data = (unsigned char *)&params;
79         paramsi.len = sizeof(params);
80


*** CID 1362936:  Control flow issues  (DEADCODE)
/external_tests/ssl_gtest/libssl_internals.c: 188 in
sslint_DamageTrafficSecret()
182       }
183       if (!slot) {
184         return PR_FALSE;
185       }
186       keyPtr = (PK11SymKey **)((char *)&ss->ssl3.hs + offset);
187       if (!keyPtr)
>>>     CID 1362936:  Control flow issues  (DEADCODE)
>>>     Execution cannot reach this statement: "return 0;".
188         return PR_FALSE;
189       PK11_FreeSymKey(*keyPtr);
190       *keyPtr = PK11_ImportSymKey(slot,
191                                   CKM_NSS_HKDF_SHA256, PK11_OriginUnwrap,
192                                   CKA_DERIVE, &key_item, NULL);
193       PK11_FreeSlot(slot);

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1283164-Fix-some-TLS-1.3-issues-reported-by-Cove.patch

Comment 2

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8766376 [details] [diff] [review]
0001-Bug-1283164-Fix-some-TLS-1.3-issues-reported-by-Cove.patch

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

::: external_tests/ssl_gtest/libssl_internals.c
@@ +183,5 @@
>    if (!slot) {
>      return PR_FALSE;
>    }
>    keyPtr = (PK11SymKey **)((char *)&ss->ssl3.hs + offset);
> +  if (!*keyPtr)

uh, I missed that. please add braces.

::: lib/ssl/ssltrace.c
@@ +49,5 @@
>          SSL_TRACE(("%d: SSL: %s [Len: %d]", SSL_GETPID(), msg, len));
>      }
> +
> +    if (!cp) {
> +        SSL_TRACE(("   <NULL>"));

I don't think this will silence coverity. It'll still think that we execute the while loop further down (which we could if len and cp don't match). So if we want to silence coverity, we have to make sure we don't end up in there (while --len >=0 && cp).

Comment 3

[Martin Thomson [:mt:]]

Comment on attachment 8766376 [details] [diff] [review]
0001-Bug-1283164-Fix-some-TLS-1.3-issues-reported-by-Cove.patch

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

::: lib/ssl/ssltrace.c
@@ +49,5 @@
>          SSL_TRACE(("%d: SSL: %s [Len: %d]", SSL_GETPID(), msg, len));
>      }
> +
> +    if (!cp) {
> +        SSL_TRACE(("   <NULL>"));

can you return?

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Martin Thomson [:mt:] from comment #3)
> ::: lib/ssl/ssltrace.c
> @@ +49,5 @@
> >          SSL_TRACE(("%d: SSL: %s [Len: %d]", SSL_GETPID(), msg, len));
> >      }
> > +
> > +    if (!cp) {
> > +        SSL_TRACE(("   <NULL>"));
> 
> can you return?

Oops, yes. That's what I actually wanted to do. I was even confused by Franziskus' comment because I was so sure I put it there...

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1283164-Fix-some-TLS-1.3-issues-reported-by-Cove.patch, v2

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/projects/nss/rev/2a37ae44ebb9