Last Comment Bug 473357 - ssltap incorrectly parses handshake messages that span record boundaries
: ssltap incorrectly parses handshake messages that span record boundaries
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Tools (show other bugs)
: 3.12
: All All
: P2 major (vote)
: 3.12.3
Assigned To: Nelson Bolyard (seldom reads bugmail)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-01-13 03:00 PST by Nelson Bolyard (seldom reads bugmail)
Modified: 2009-03-12 19:24 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1 for NSS Trunk (17.00 KB, patch)
2009-01-13 03:00 PST, Nelson Bolyard (seldom reads bugmail)
julien.pierre: review-
Details | Diff | Splinter Review
patch v2 - address Julien's issues (17.79 KB, patch)
2009-03-11 19:47 PDT, Nelson Bolyard (seldom reads bugmail)
julien.pierre: review-
Details | Diff | Splinter Review
Patch v3 - for review (17.89 KB, patch)
2009-03-12 00:12 PDT, Nelson Bolyard (seldom reads bugmail)
julien.pierre: review+
Details | Diff | Splinter Review

Description Nelson Bolyard (seldom reads bugmail) 2009-01-13 03:00:14 PST
Created attachment 356700 [details] [diff] [review]
Patch v1 for NSS Trunk

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 1 Nelson Bolyard (seldom reads bugmail) 2009-02-15 20:05:14 PST
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.
Comment 2 Julien Pierre 2009-03-11 15:59:28 PDT
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 ?
Comment 3 Nelson Bolyard (seldom reads bugmail) 2009-03-11 19:47:58 PDT
Created attachment 366973 [details] [diff] [review]
patch v2 - address Julien's issues

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.
Comment 4 Nelson Bolyard (seldom reads bugmail) 2009-03-11 19:50:18 PDT
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 Julien Pierre 2009-03-11 20:01:34 PDT
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.
Comment 6 Julien Pierre 2009-03-11 20:05:42 PDT
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.
Comment 7 Nelson Bolyard (seldom reads bugmail) 2009-03-12 00:03:38 PDT
(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.
Comment 8 Nelson Bolyard (seldom reads bugmail) 2009-03-12 00:12:49 PDT
Created attachment 366992 [details] [diff] [review]
Patch v3 - for review

Try this one.
Comment 9 Nelson Bolyard (seldom reads bugmail) 2009-03-12 19:24:58 PDT
Thanks for the reviews.

Checking in ssltap.c; new revision: 1.13; previous revision: 1.12

Note You need to log in before you can comment on or make changes to this bug.