1287711 - Update SSLKEYLOGFILE for TLS 1.3

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Martin Thomson [:mt:]]

David Benjamin had a proposed format:

EARLY_TRAFFIC_SECRET <client_random> <early_traffic_secret>
HANDSHAKE_TRAFFIC_SECRET <client_random> <handshake_secret>
TRAFFIC_SECRET_0 <client_random> <application_secret>

Comment 1

[David Benjamin]

In case there's confusion, the proposal is s/handshake_secret/handshake_traffic_secret/ and s/application_secret/traffic_secret_0/ on the RHS too, to match the names the spec uses. (I typo'd the names and forgot to update both halves.)

Comment 2

[David Benjamin]

With the ladder split, this will need to be tweaked. I think the natural fix is to stick CLIENT_ or SERVER_ in front of each of these. It does mean we'll be logging a whole 5 (no SERVER_EARLY_TRAFFIC_SECRET) keys per connection, but ah well.

The other option is to record the base secrets (early, handshake, and master), but I think this is better. Wireshark doesn't need to maintain a handshake transcript and never sees non-traffic key material.

Comment 3

[Peter Wu]

Pending patches for Wireshark implement the following:

- "CLIENT_HANDSHAKE_TRAFFIC_SECRET xxxx yyyy"
- "SERVER_HANDSHAKE_TRAFFIC_SECRET xxxx yyyy"
- "CLIENT_TRAFFIC_SECRET_0 xxxx yyyy"
- "SERVER_TRAFFIC_SECRET_0 xxxx yyyy"
  Where xxxx is the client_random from the ClientHello (hex-encoded)
  Where yyyy is the secret (hex-encoded) derived from the handshake or
  master secrets.

For the early traffic secret, David proposed "CLIENT_EARLY_TRAFFIC_SECRET xxxx yyyy".

Note about sizes: A TLS 1.2 session currently adds 13 + 3 + 2*32 + 2*48 = 176 bytes to the keylog file ("CLIENT_RANDOM", spaces+newline, random, master secret).

For the proposed TLS 1.3 log format, the fixed part (spaces+newline, random, secret) takes 131 (for SHA256) or 163 (for SHA384) per secret. The four labels take 2*31 + 2*23 = 108 bytes. The proposed label for early takes 27 bytes (27+108=131). So, in total we have:
         SHA256  SHA384
w/o 0-RTT   632     760
w/ 0-RTT    790     950
(and the exporter secret for DTLS-SRTP is not considered yet.)

Alternatively, since a full pcap is usually available to Wireshark, it can also find the messages transcript. Then only two (early and handshake) secrets need to be logged. WDYT?

(Wireshark TLS 1.3 work is tracked at https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=12779)

Comment 4

[Martin Thomson [:mt:]]

I really don't see space as an issue.

Comment 5

[Peter Wu]

Update: the development version of Wireshark (git master) now supports decryption of early data too. Keylog format is:
CLIENT_EARLY_TRAFFIC_SECRET xxxx yyyy
where xxxx as before and yyyy the client_early_traffic_secret (*not* Early Secret).

Status with other implementations for the updated TLS 1.3 keylog format:
- BoringSSL has implemented all but early
- OpenSSL has implemented all but early
- picotls has implemented all, including early
- NSS has not implemented anything (correct?)

Comment 6

[Peter Wu]

Hey all,

0-RTT decryption also depends on the cipher suite that was stored with the PSK (which might be different from the cipher suite for the new suite). Would it be possible to add the cipher suite to the CLIENT_EARLY_TRAFFIC_SECRET line?

E.g.
CLIENT_EARLY_TRAFFIC_SECRET xxxx yyyy zzzz
where xxxx is the client random, yyyy the client_early_traffic_secret and zzzz the cipher suite ID (e.g. 1303 for TLS_CHACHA20_POLY1305_SHA256).

At the moment, given the secrets, we can try all cipher suites (currently 5) and check whether the auth tag passses, but this is not ideal.

Comment 7

[David Benjamin]

Oh, that's a good point! I hadn't thought of that.

Your proposal seems reasonable enough to me. One nuisance: for full generality, you also need ALPN and possibly other session state is needed to fully decode the result.

The do-nothing alternative would be that you can’t decrypt early data until you get the ServerHello/EncryptedExtensions. This naturally generalizes to all session state, but it means you don’t get to process rejected early data, which is a loss of debugging coverage. (But such data is also unprocessed so maybe not that useful?)

I think if we start dumping all session state in there, maybe just waiting for SH/EE is saner? I’d rather we not have to adjust this stuff every time we add a new extension. Then again, maybe you're unlikely to care about more than the cipher suite and ALPN. Though, in that case, that would suggest something like:

CLIENT_EARLY_TRAFFIC_SECRET xxxx yyyy
EARLY_TRAFFIC_CIPHER xxxx zzzz
EARLY_TRAFFIC_ALPN xxxx wwww
[...]

since that's easier to extend. But it means you need to correlate more stuff together. (Maybe we still put cipher with the secret as in your proposal since they're so tightly tied together.)


Do you just care about decrypting the early data, or would you also want ALPN and whatnot? Note you can always fall back to the SH/EE version at a cost of rejected early data.

Comment 8

[Peter Wu]

Ah yes, ALPN would be nice too. The proposed EARLY_TRAFFIC_xxx looks good to me. For CIPHER, the value will be sth like 1303, for ALPN sth like 6832 (for "h2") right?

Even when early data is rejected by the server, for debugging it would still be interesting to see what's inside. For this early_traffic_secret + cipher suite are required. (The less unknowns in an analysis, the better. That makes it easier to exclude implementation errors.)
For making a better judgement on the protocol inside the early data, ALPN is required (otherwise there will be a fallback to heuristics on the port number or contents).

Summarizing: at minimum cipher suite is needed. ALPN is nice-to-have.

Comment 9

[David Benjamin]

Yeah, I think hex-encoding ALPN is simplest. Why worry about ALPN tokens with spaces and newlines when we can just hex-encode and move on with life? :-)

If correlating the lines isn't trouble for you, making EARLY_TRAFFIC_ALPN and other things separate lines is probably the way to go. That way we can easily express a session which did not negotiate ALPN (the line is missing; use whatever logic you would have used without ALPN) and generalizes better. Though I certainly hope we don't need to generalize this much.

I don't particularly care whether EARLY_TRAFFIC_CIPHER is separate or an extra field on the same line. I guess that'll always be there and is fairly tied to the traffic secret, so same line as you suggested is shorter, and you don't need to handle the case when it's missing:

CLIENT_EARLY_TRAFFIC_SECRET xxxx yyyy zzzz
EARLY_TRAFFIC_ALPN xxxx wwww

Comment 10

[Peter Wu]

Correlating lines is no problem (Wireshark currently reads line-by-line and populates a map client_random -> secret).

With draft -20, I realized another problem, the TLS (draft) version number is also significant. All of these (version, cipher, ALPN) are not visible on the wire and stored with the PSK.

This does not have to be a problem:
if multiple TLS sessions are recorded in a session and the PSK can be correlated to a previous session, then the above parameters can be detected. Only the secret is really required (which is currently already provided).

Given that, what about logging the PSK state for 0RTT separately? Something like:
0RTT_STATE xxxx aaaa zzzz wwww
xxxx - client random
aaaa - TLS version (e.g. 7f14 or 0304)
zzzz - cipher suite (e.g. 1303)
wwww - ALPN (or empty if there is none)

Comment 11

[Martin Thomson [:mt:]]

What about the exporter secret?

EXPORTER_SECRET xxxx yyyy

We might want that for QUIC, for instance.

FWIW, a separate line for 0-RTT state would be ideal (it makes the key logging easier to implement).  I think that the early data state as proposed in comment 10 is what I would prefer.

Comment 12

[David Benjamin]

EXPORTER_SECRET works for me.

I'm fine with all three early data state proposals (comment #7, comment #9, comment #10). Though I think I slightly prefer comment #7 over comment #9 or comment #10 just because it's a bit more uniform. Each line is always:

  <entry type> <client random> <exactly one value>

It's also easier to generalize to stuffing other random things into the session. We just add new entry types.

Comment 13

[Martin Thomson [:mt:]]

Thanks David, I would be OK with that.  How about we let Peter decide this then.

Comment 14

[Peter Wu]

"EXPORTER_SECRET xxxx yyyy" sounds good. It can also be useful for DTLS-SRTP (DTLS 1.2 + use_srtp extension, used in WebRTC), would it make sense to add this to TLS 1.2 implementations too?

I have a small preference for putting all state on one line (for compactness), but label+key+value would also be good (and since version and ALPN can be represented in hex, it is still consistent).

When spreading information over multiple lines, there is another possible complication: if the Client Hello value is not unique, Wireshark currently only considers the newest line and decryption might not be successful. In practice this occurred during development of the Go crypto/tls library (it uses an all-zeroes client random).

Since trial decryption also needs to be implemented, I was considering to extend Wireshark to try multiple keys in such cases. Having a single line with all state would simplify building the state tuple (less possible combinations).

Comment 15

[Martin Thomson [:mt:]]

Exporters in TLS 1.2 use the master secret, so I think that the existing format works for that.

Based on reviewing Peter's code again, I think that we might be better off with one line.  The 0-RTT state is somewhat exceptional.  Keeping the keys separate is easier, but we can easily dump 0-RTT state in the one place.

Comment 16

[David Benjamin]

Wouldn't you all also be able to take advantage of the uniform format were:

  ssl3_RecordKeyLog(sslSocket *ss, const char *label, PK11SymKey *secret)

replaced with 

  ssl3_RecordKeyLog(sslSocket *ss, const char *label, SECItem *data)

or

  ssl3_RecordKeyLog(sslSocket *ss, const char *label, const unsigned char *data, size_t length)

This way there's only one place where you mess with locks, add up lengths for hex, etc.

Comment 17

[Martin Thomson [:mt:]]

That means having the key extraction outside that function, but sure.  (The locking isn't needed, yay NSS locks.)  As I said, I don't care either way.

Comment 18

[Peter Wu]

ssl3_RecordKeyLog is an internal implementation detail which is not publically exposed as API, at the moment it does not matter whether a secret or a string is passed.

In theory fwrite makes "size" fputc calls to write the data, so the FILE handle might require a lock if there are concurrent connections. In practice it has not been a problem yet, otherwise a global mutex would have to be added.

I'm still undecided whether to use a single line for 0RTT state or one for each field.

Comment 19

[Phabricator Automation]

Comment on attachment 8912502 [details]
Bug 1287711 - Implement SSLKEYLOGFILE for TLS 1.3 (v2)

Martin Thomson [:mt:] has approved the revision.

https://phabricator.services.mozilla.com/D82#1831

Comment 20

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/3e803b9a08c8820f035cfe9189409c101144f8a9

Note that this is on the NSS_TLS13_DRAFT19_BRANCH branch.  Peter had some trouble rebasing onto trunk and I've not the time to debug the problem further.

Comment 21

[Peter Wu]

(In reply to Martin Thomson [:mt:] from comment #20)
> Note that this is on the NSS_TLS13_DRAFT19_BRANCH branch.  Peter had some
> trouble rebasing onto trunk and I've not the time to debug the problem
> further.

The patches do not apply cleanly on the default branch, but I am willing to fix that in order to merge it into default. Hopefully it will not cause issues when you merge NSS_TLS13_DRAFT19_BRANCH at a later point.

Also reported is a build failure on Windows due to use of setenv without including stdlib.h, will fix that as well while porting the patch.

Comment 22

[Martin Thomson [:mt:]]

Peter, have backed this out for this test failure (the windows thing is benign and I would just fix that if if weren't for this):
https://public-artifacts.taskcluster.net/eGj3T2-BTiWEGHxqfFMRtQ/0/public/logs/live_backing.log

I have to hop on a plane, but I will take a look if you can't.

Comment 23

[Peter Wu]

7 failed tests, all of them are due to memleaks. I can certainly have a deeper look at that.

Question, would you mind if I base the patch off trunk? It is quite independent from draft 19..21 changes.

Comment 24

[Martin Thomson [:mt:]]

I don't mind if this is on trunk.  I have a slight preference to only land in one place, but I can manage merge conflict reasonably well if it comes to that.

Comment 25

[Peter Wu]

Since the patch is already reverted, I'd like to prepare it for default then. This will later conflict with the draft 19 branch due to removal of the "hashes" parameter, something to keep in mind. I'll make sure to test it with ASAN enabled.

Have a safe flight!

Comment 26

[Peter Wu]

I was not able to reproduce the memleak on Arch Linux with Clang 5.0.0, but on Ubuntu Xenial with clang-4.0 it somehow did die. Perhaps it has registered and atexit hook to check for leaks.

The new patchset is up and rebased on the default branch: https://phabricator.services.mozilla.com/D82#1874

Comment 27

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/bc6d9e5391daa9807599dffee447ec4368210bf7

Comment 28

[Peter Wu]

Additionally: removal of the "RSA" format and thread-safety by adding a lock:
https://hg.mozilla.org/projects/nss/rev/965b5533bf61ecf85e71203fa26ab33e60bb7125
https://hg.mozilla.org/projects/nss/rev/c1866d7d7f15b3ea9de3659fbd2fdaaf4f2ae273

Since the patches have landed, I have updated the wiki page for the new format:
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/Key_Log_Format


Still missing is 0RTT state info.

Comment 29

[Phabricator Automation]

Attachment: Bug 1287711 - Remove RSA premaster secret from keylog file

Comment 30

[Phabricator Automation]

Attachment: Bug 1287711 - Make writes to SSLKEYLOGFILE thread-safe

Comment 31

[Phabricator Automation]

Comment on attachment 8940777 [details]
Bug 1287711 - Make writes to SSLKEYLOGFILE thread-safe

Martin Thomson [:mt:] has approved the revision.

https://phabricator.services.mozilla.com/D86

Comment 32

[Phabricator Automation]

Comment on attachment 8940776 [details]
Bug 1287711 - Remove RSA premaster secret from keylog file

Martin Thomson [:mt:] has approved the revision.

https://phabricator.services.mozilla.com/D85