536474 - Add support for logging pre-master secrets

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement)

Description

[Adam Langley]

Attachment: patch

Wireshark is a great tool for debugging and it has the ability to decrypt TLS connections, which are using an RSA ciphersuite, given the server's private key.

However, often one is debugging against servers where one doesn't have access to the private key.

This patch adds support for logging pre-master secrets to a log file. The format is very simple and requires that Wireshark perform a linear scan of the file whenever it sees a ClientKeyExchange. But there are typically only a handshake of TLS connections of interest when debugging and this is adequate for that case.

Comment 1

[Adam Langley]

ping on the review.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

This patch causes libSSL to create and, with the creation of each new RSA PMS,
extend a file.  The file consists of binary records each 58 bytes long, 
each with the format:
  length  value
  ------  -----------------------------------------------------
     8     first 8 bytes of encrypted PMS, as seen on the wire
     2     The value 48, as a 16 bit big-endian binary number
    48     The RSA PMS

Perhaps the file format is intended to be more generally records of 
length 8 + 2 + N, where N is the value in the 2 byte field.

Some questions about this, motivated by NSS's binary compatibility requirements.
Once it goes in, it's in forever.

- Is this file format something defined in/by WireShark?
- Is it likely to be useful, say, a year from now, or two or three?
- Is it likely to want/need to be enhanced with other types of records?
- Should it have a record type field?
- Should it be separately ifdef'ed from TRACE (which is built in all DEBUG builds?)
- Should it be built by default in all debug builds?

Comment 3

[Adam Langley]

Attachment: Changing the format to match Wireshark's format

> - Is this file format something defined in/by WireShark?

No, I'm making both sides of this patch. The Wireshark folks asked for a change to a hex based format which I did before Xmas. The NSS side of this change is attached:

https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=4349

- Is it likely to be useful, say, a year from now, or two or three?

I would hope so. I don't know of another way to debug protocols over TLS.

- Is it likely to want/need to be enhanced with other types of records?

I would hope not. The current format is just a list of key/value pairs when you get down to it. Since the packet sniffer sees the handshake and knows the cipher suite in use, it can interpret the fields as needed. Currently this only works for RSA cipher suites, but if it were extended to DH protocols then the key could be the first eight bytes of the server's parameters and the value remains the pre-master secret. So the format doesn't change.

- Should it have a record type field?

Since the packet sniffer sees the handshake I didn't see a need, but nor do I have an objection.

- Should it be separately ifdef'ed from TRACE (which is built in all DEBUG
builds?)
- Should it be built by default in all debug builds?

I don't really have a good answer for these questions. You should play Benevolent Dictator and just decide one way or another :)


Cheers

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

> Currently this only works for RSA cipher suites, but if it were extended to 
> DH protocols then the key could be the first eight bytes of the server's 
> parameters and the value remains the pre-master secret. So the format 
> doesn't change.

But the length does.  The PMS length is only fixed length for RSA.  

Nelson the Benevolent Dictator (:-) says:
- make the first few bytes of each record be 
  - record type   (1)
  - record length (2)
- make sure the formats of all record types are documented somewhere
- Since you're going to a humanly readable format (which I would not have 
suggested), make the first record of the file be a comment record that says 
(tersely) something to the effect of
"Wireshark pre-master secret log file generated yyyy-mm-dd hh:mm:ss".

Comment 5

[Adam Langley]

Attachment: updated to add comment line and 'RSA' at the beginning of each line.

> - make the first few bytes of each record be 
>   - record type   (1)

done

>  - record length (2)

records are newline terminated, so this would seem to be superfluous. (This newline termination might not have been clear since the format has changed from a binary one in the beginning to the current format.)

> - make sure the formats of all record types are documented somewhere

https://developer.mozilla.org/en/NSS_Key_Log_Format

(linked to in a comment also.)

> - Since you're going to a humanly readable format (which I would not have 
suggested),

It's human readable at the request of the Wireshark folks I'm afraid :(
I'm just trying to bring Wireshark and NSS together.

> make the first record of the file be a comment record that says 
> (tersely) something to the effect of
> "Wireshark pre-master secret log file generated yyyy-mm-dd hh:mm:ss".

Done: "# pre-master secret log file, generated by NSS\n"

I avoided mentioning Wireshark since there's no reason that other code couldn't read these files also. I also avoided including the time because localtime() is such a pain to sandbox. See:

http://src.chromium.org/cgi-bin/gitweb.cgi?p=chromium.git;a=blob;f=chrome/browser/zygote_main_linux.cc;h=3b9c6efd1a538c034be22a6702959b349c715b6c;hb=HEAD#l364

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 420815 [details] [diff] [review]
updated to add comment line and 'RSA' at the beginning of each line.

r=nelson

Comment 7

[Wan-Teh Chang]

Attachment: Adam's patch (as checked in)

r=wtc.  I made the following changes.
1. Renamed ssl_keylog to ssl_keylog_iob to follow the naming
convention of the other FILE variable, ssl_trace_iob.
2. Changed a fprintf call to fputs because the string doesn't
need formatting.
3. Renamed SSLKEYLOG to SSLKEYLOGFILE to follow the naming
convention of NSS environment variables that specify a file
name.  I updated the wiki page
https://developer.mozilla.org/en/NSS_Key_Log_Format
with the new environment variable name.

Nelson, should we use the new convention of adding NSS_
prefix to environment variables?

I checked in the patch on the NSS trunk (NSS 3.12.6).

Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.128; previous revision: 1.127
done
Checking in sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.72; previous revision: 1.71
done
Checking in sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.63; previous revision: 1.62
done

Comment 8

[Wan-Teh Chang]

It would be nice if NSS's own ssltap tool can support the
pre-master secret log file.