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)

3.12

Tracking

(Not tracked)

RESOLVED FIXED
3.12.3

People

(Reporter: nelson, Assigned: nelson)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Patch v1 for NSS Trunk (obsolete) — 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.
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)
Severity: normal → major
Priority: -- → P2
Target Milestone: --- → 3.12.3
Attachment #356700 - Flags: review?(julien.pierre.boogz)
Attachment #356700 - Flags: review?(julien.pierre.boogz) → review-
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 ?
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)
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 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 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.
(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.
Try this one.
Attachment #366973 - Attachment is obsolete: true
Attachment #366992 - Flags: review?(julien.pierre.boogz)
Attachment #366992 - Flags: review?(julien.pierre.boogz) → review+
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.