bugzilla.mozilla.org will be intermittently unavailable on Saturday, March 24th, from 16:00 until 20:00 UTC.




2 years ago
2 months ago


(Reporter: mt, Assigned: Peter Wu)


Firefox Tracking Flags

(firefox57 wontfix)



(3 attachments)



2 years ago
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

2 years ago
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

2 years ago
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

a year ago
Pending patches for Wireshark implement the following:

  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

a year ago
I really don't see space as an issue.

Comment 5

a year ago
Update: the development version of Wireshark (git master) now supports decryption of early data too. Keylog format is:
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

a year ago
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?

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

a year ago
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:


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

a year ago
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

a year ago
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:


Comment 10

11 months ago
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)


6 months ago
status-firefox57: --- → wontfix
Priority: -- → P3

Comment 11

6 months ago
What about the exporter secret?


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.
Flags: needinfo?(peter)
Flags: needinfo?(davidben)

Comment 12

6 months ago
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.
Flags: needinfo?(davidben)

Comment 13

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

Comment 14

6 months ago
"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).
Flags: needinfo?(peter)

Comment 15

6 months ago
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

6 months ago
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)


  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

6 months ago
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

6 months ago
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 on attachment 8912502 [details]
Bug 1287711 - Implement SSLKEYLOGFILE for TLS 1.3 (v2)

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

Attachment #8912502 - Flags: review+

Comment 20

6 months ago

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.
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → Future

Comment 21

6 months ago
(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

6 months ago
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):

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

Comment 23

6 months ago
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

6 months ago
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

6 months ago
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

6 months ago
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

6 months ago
Assignee: nobody → peter
Target Milestone: Future → 3.34

Comment 28

6 months ago
Additionally: removal of the "RSA" format and thread-safety by adding a lock:

Since the patches have landed, I have updated the wiki page for the new format:

Still missing is 0RTT state info.


2 months ago
Attachment #8912502 - Attachment description: Bug 1287711 - Implement SSLKEYLOGFILE for TLS 1.3 → Bug 1287711 - Implement SSLKEYLOGFILE for TLS 1.3 (v2)
Created attachment 8940776 [details]
Bug 1287711 - Remove RSA premaster secret from keylog file
Created attachment 8940777 [details]
Bug 1287711 - Make writes to SSLKEYLOGFILE thread-safe
Comment on attachment 8940777 [details]
Bug 1287711 - Make writes to SSLKEYLOGFILE thread-safe

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

Attachment #8940777 - Flags: review+
Comment on attachment 8940776 [details]
Bug 1287711 - Remove RSA premaster secret from keylog file

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

Attachment #8940776 - Flags: review+
You need to log in before you can comment on or make changes to this bug.