SSLTap should display plain text for NULL cipher suites

RESOLVED FIXED in 3.12

Status

NSS
Tools
P3
enhancement
RESOLVED FIXED
15 years ago
11 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Alexei Volkov)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

When SSLTAP receives an SSL/TLS "Finished" message from either end, it stops
attempting to decode/display the record contents after that and instead 
displays the message "(encrypted)".  

It should not do this for the "NULL" cipher suites, since they do not encrypt
the data.
(Reporter)

Comment 1

15 years ago
taking bug
Assignee: wtc → nelsonb
Priority: -- → P3

Updated

13 years ago
Assignee: MisterSSL → saul.edwards

Comment 2

13 years ago
Created attachment 149221 [details] [diff] [review]
View application data in hex and ASCII with NULL ciphers

Compiled and tested on Solaris.  Does not work for SSL v2.

Comment 3

13 years ago
Created attachment 149222 [details]
Sample output with the first patch

Comment 4

13 years ago
Comment on attachment 149221 [details] [diff] [review]
View application data in hex and ASCII with NULL ciphers

The patch looks good, but I think you should include sslproto.h and use the
macros for the cipher suites, rather than typing the values again, which is
error prone. I know that's not the existing usage elsewhere in ssltap, but I
think it makes more sense, and should at least be done for the new code, if not
also for the existing.

In order to get the names of all the cipher suites, you may need to #ifdef
NSS_ENABLE_ECC . You will also need to add NSS_ENABLE_ECC support to the
manifest.mn for ssltap.
Attachment #149221 - Flags: review-
(Reporter)

Updated

12 years ago
QA Contact: bishakhabanerjee → jason.m.reid
(Reporter)

Comment 5

12 years ago
reassigning to Alexei.
Assignee: saul.edwards.bugs → alexei.volkov.bugs
Severity: minor → enhancement
(Reporter)

Updated

12 years ago
QA Contact: jason.m.reid → tools
(Assignee)

Updated

11 years ago
Target Milestone: --- → 3.12
(Assignee)

Comment 6

11 years ago
Created attachment 239276 [details]
new output when NULL cipher is used
Attachment #149222 - Attachment is obsolete: true
(Assignee)

Comment 7

11 years ago
Created attachment 239367 [details] [diff] [review]
patch

Use sslproto.h macro definition for cipher suite instead on hex numbers.
Removes obsolite ECC cipher definitions.
Prints MAC separetely from content. Calculates MAC size based on remaining
number of bytes in "finished" record. Check attached output.
Attachment #149221 - Attachment is obsolete: true
Attachment #239367 - Flags: review?(nelson)
(Reporter)

Comment 8

11 years ago
Comment on attachment 239367 [details] [diff] [review]
patch

I've asked Alexei to add some comments and also to fix print_hex to properly align the ASCII text output.
Attachment #239367 - Flags: review?(nelson)
(Assignee)

Comment 9

11 years ago
Created attachment 239424 [details] [diff] [review]
comments and fix for ASCII string alignment
Attachment #239367 - Attachment is obsolete: true
Attachment #239424 - Flags: review?(nelson)
(Reporter)

Comment 10

11 years ago
Comment on attachment 239424 [details] [diff] [review]
comments and fix for ASCII string alignment

r=nelson, with one small suggested change to a comment.

>+      if (!isNULLmac(currentcipher) && !hMACsize) {
>+          /* To calculate the size of MAC, we will sum the size
>+           * of verify_data(sslh.length) and 4(type + length
>+           * of finished handshake message) and subtract the
>+           * result from the total number of bytes in handshake
>+           * message(alloclen) */

This should say "from the total number of bytes in the handshake record",
the crucial difference being that the MAC comes at the end of a record,
and is not part of the message.  (records contain messages and MACs).
Attachment #239424 - Flags: review?(nelson) → review+
(Assignee)

Comment 11

11 years ago
Integrated into 3.12 with corrected comment.

/cvsroot/mozilla/security/nss/cmd/ssltap/ssltap.c,v  <--  ssltap.c
new revision: 1.10; previous revision: 1.9
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.