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

RESOLVED FIXED in 3.26

Status

NSS
Libraries
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

a year ago
*** 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);
(Assignee)

Comment 1

a year ago
Created attachment 8766376 [details] [diff] [review]
0001-Bug-1283164-Fix-some-TLS-1.3-issues-reported-by-Cove.patch
Attachment #8766376 - Flags: review?(franziskuskiefer)
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).
Attachment #8766376 - Flags: review?(franziskuskiefer)
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?
(Assignee)

Comment 4

a year ago
(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...
(Assignee)

Comment 5

a year ago
Created attachment 8766793 [details] [diff] [review]
0001-Bug-1283164-Fix-some-TLS-1.3-issues-reported-by-Cove.patch, v2
Attachment #8766376 - Attachment is obsolete: true
Attachment #8766793 - Flags: review?(franziskuskiefer)
Attachment #8766793 - Flags: review?(franziskuskiefer) → review+
(Assignee)

Comment 6

a year ago
https://hg.mozilla.org/projects/nss/rev/2a37ae44ebb9
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → 3.26
You need to log in before you can comment on or make changes to this bug.