44 bytes, text/x-phabricator-request
|Details | Review|
44 bytes, text/x-phabricator-request
|Details | Review|
44 bytes, text/x-phabricator-request
|Details | Review|
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>
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.)
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.
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)
I really don't see space as an issue.
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?)
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.
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.
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.
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
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)
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.
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.
Thanks David, I would be OK with that. How about we let Peter decide this then.
"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).
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.
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.
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.
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. https://phabricator.services.mozilla.com/D82#1831
Attachment #8912502 - Flags: review+
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.
Status: NEW → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → Future
(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.
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.
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.
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.
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!
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
Assignee: nobody → peter
Target Milestone: Future → 3.34
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.
Attachment #8912502 - Attachment description: Bug 1287711 - Implement SSLKEYLOGFILE for TLS 1.3 → Bug 1287711 - Implement SSLKEYLOGFILE for TLS 1.3 (v2)
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
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. https://phabricator.services.mozilla.com/D85
Attachment #8940776 - Flags: review+
You need to log in before you can comment on or make changes to this bug.