Closed
Bug 1283165
Opened 8 years ago
Closed 8 years ago
[Coverity 1362937+1362936] Fix some TLS 1.3 issues reported by Coverity
Categories
(NSS :: Libraries, defect)
NSS
Libraries
Tracking
(Not tracked)
RESOLVED
FIXED
3.26
People
(Reporter: ttaubert, Assigned: ttaubert)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
1.97 KB,
patch
|
franziskus
:
review+
|
Details | Diff | Splinter Review |
*** 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 *)¶ms; 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•8 years ago
|
||
Attachment #8766376 -
Flags: review?(franziskuskiefer)
Comment 2•8 years ago
|
||
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 3•8 years ago
|
||
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•8 years 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•8 years ago
|
||
Attachment #8766376 -
Attachment is obsolete: true
Attachment #8766793 -
Flags: review?(franziskuskiefer)
Updated•8 years ago
|
Attachment #8766793 -
Flags: review?(franziskuskiefer) → review+
Assignee | ||
Comment 6•8 years ago
|
||
https://hg.mozilla.org/projects/nss/rev/2a37ae44ebb9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.26
You need to log in
before you can comment on or make changes to this bug.
Description
•