Closed
Bug 473357
Opened 15 years ago
Closed 15 years ago
ssltap incorrectly parses handshake messages that span record boundaries
Categories
(NSS :: Tools, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
3.12.3
People
(Reporter: nelson, Assigned: nelson)
Details
Attachments
(1 file, 2 obsolete files)
17.89 KB,
patch
|
julien.pierre
:
review+
|
Details | Diff | Splinter Review |
While investigating Bug 473254, which concerned a server that sends out an unusual cert chain, I discovered that ssltap is unable to correctly handle ssl3/TLS handshake messages that span record boundaries. The server in question sent out a "chain" of 13 certs totaling almost 25KB. ssltap was unable to parse the 25KB Certificates message because it spanned two records. It treated the 5 byte record header of the second record as if it was part of the message body, and consequently, it attempted to construct a too-large certificate, and failed. The attached patch seems to fix it. I will request review when I am more confident that it is correct.
Assignee | ||
Comment 1•15 years ago
|
||
Comment on attachment 356700 [details] [diff] [review] Patch v1 for NSS Trunk I've been using ssltap with this patch for a month now without problems. So, I'm ready to ask for review. Alexei, please review.
Attachment #356700 -
Flags: review?(alexei.volkov.bugs)
Assignee | ||
Updated•15 years ago
|
Severity: normal → major
Priority: -- → P2
Target Milestone: --- → 3.12.3
Assignee | ||
Updated•15 years ago
|
Attachment #356700 -
Flags: review?(julien.pierre.boogz)
Updated•15 years ago
|
Attachment #356700 -
Flags: review?(julien.pierre.boogz) → review-
Comment 2•15 years ago
|
||
Comment on attachment 356700 [details] [diff] [review] Patch v1 for NSS Trunk This looks mostly OK, but I have the following concerns : 1) reason for r- : in print_ssl3_handshake, there are 3 abort() calls when PR_Realloc and PR_Malloc fail. This is not an elegant way of managing errors. Given that we distribute this program with Solaris, I think this should be improved to avoid a crash, and fail with an error instead. This could be as simple as printing an error message and calling exit(). 2) nit: in print_ssl3_handshake, why did you remove the comment on line 1035 ? Was it inaccurate ?
Assignee | ||
Comment 3•15 years ago
|
||
I changed the call to display an error message before terminating when an allocation failure occurs. I do want a core dump though, so we can find/fix leaks. I removed the comment because - I thought it stated the obvious, and - there are many lines that use that same magic number. This one place didn't seem to need the comment more than the others.
Attachment #356700 -
Attachment is obsolete: true
Attachment #366973 -
Flags: review?(julien.pierre.boogz)
Attachment #356700 -
Flags: review?(alexei.volkov.bugs)
Assignee | ||
Comment 4•15 years ago
|
||
In patch v2, I also changed all the PR_*lloc calls into PORT_*lloc calls instead. This is because the PR_ memory allocation functions do not all set an NSPR error code for allocation failures, but the PORT functions do. Since the code is now displaying the NSPR error code, it must be set.
Comment 5•15 years ago
|
||
Comment on attachment 366973 [details] [diff] [review] patch v2 - address Julien's issues Why do you want this abort ? There is no precedent that I know for doing this in low memory conditions in the rest of our code. Does ssltap have a history of leaks ? Running out of memory , or contiguous memory, is not necessarily indicative of any leak. It can just be a low memory condition, the user having set very low process limits, etc. If we didn't ship this program to customers, I might be OK with keeping this code. If you really want the core dump, then you can surround the call to abort with #ifdef DEBUG . That way we get the core dump only when we really want to.
Attachment #366973 -
Flags: review?(julien.pierre.boogz) → review-
Comment 6•15 years ago
|
||
Comment on attachment 366973 [details] [diff] [review] patch v2 - address Julien's issues You also have a sleep loop that surely doesn't belong :). https://bugzilla.mozilla.org/attachment.cgi?oldid=356700&action=interdiff&newid=366973&headers=1#mozilla/security/nss/cmd/ssltap/ssltap.c_sec2 Another problem in this hunk : https://bugzilla.mozilla.org/attachment.cgi?oldid=356700&action=interdiff&newid=366973&headers=1#mozilla/security/nss/cmd/ssltap/ssltap.c_sec7 You added a showErr call before the abort. I think htese 2 lines need to be together in a block, otherwise ssltap will unconditionally abort.
Assignee | ||
Comment 7•15 years ago
|
||
(In reply to comment #6) > (From update of attachment 366973 [details] [diff] [review]) > You also have a sleep loop that surely doesn't belong :). Look at what that sleep loop replaces: a 100% CPU busy loop. I think you'll agree that it's better for the "halted" process to not be needlessly consuming 100% CPU. > Another problem in this hunk : > You added a showErr call before the abort. I think htese 2 lines need to be > together in a block, otherwise ssltap will unconditionally abort. Thanks. I'm amazed my testing didn't catch that.
Assignee | ||
Comment 8•15 years ago
|
||
Try this one.
Attachment #366973 -
Attachment is obsolete: true
Attachment #366992 -
Flags: review?(julien.pierre.boogz)
Updated•15 years ago
|
Attachment #366992 -
Flags: review?(julien.pierre.boogz) → review+
Assignee | ||
Comment 9•15 years ago
|
||
Thanks for the reviews. Checking in ssltap.c; new revision: 1.13; previous revision: 1.12
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•