Closed Bug 944175 Opened 11 years ago Closed 8 years ago

Implement the Certificate Transparency signed_certificate_timestamp TLS extension (RFC 6962) on the client side

Categories

(NSS :: Libraries, enhancement, P2)

Product:
NSS
Component:
Libraries
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
3.16.1

People

(Reporter: wtc, Assigned: wtc)

References

(Blocks 1 open bug,
URL
)

Details

Attachments

(4 files, 10 obsolete files)

rebased patch with CR fixes, pass 1
30.05 KB, patch
wtc
: review+
Details | Diff | Splinter Review
server side without unit tests, CR pass 4.1 (fixed indentation)
15.94 KB, patch
wtc
: review+
mt
: superreview+
Details | Diff | Splinter Review
unit tests, CR pass 2
11.87 KB, patch
ekr
: review+
Details | Diff | Splinter Review
Fix nits in conditional expression formatting and error messages
2.36 KB, patch
ekr
: review+
wtc
: checked-in+
Details | Diff | Splinter Review
Attached patch Patch by Emilia Kasper (obsolete) — — Splinter Review
The attached patch by Emilia Kasper of Google implements the Certificate Transparency signed_certificate_timestamp TLS extension (RFC 6962) on the client side. I reviewed the patch at https://codereview.chromium.org/64553002.
Target Milestone: 3.15.5 → 3.16
Target Milestone: 3.16 → 3.16.1
Does this patch still apply to the latest Firefox? If so, what is the delay in accepting it? It seems sorely needed: http://superuser.com/questions/818065/how-to-know-which-certificates-to-leave-in-my-browser-and-which-to-remove
The patch in itself (as far as I can tell) only advertises the TLS extension and decodes the response. This by itself does nothing. You'll need to convince people that lack of the extension is valid grounds to mark a site as "bad" in some way, with associated politics and UX work. Even so, the security comes from the audit log, not this patch.
I think this is probably a good thing to accept into mainstream NSS. The technology seems stable enough to be added to the NSS API, and the patch itself is simple enough. In the long run, it seems like we should probably generalize the NSS TLS API so that the calling code can just specify which extensions it wants and get back the bytes for those extensions. But given that we don't have that architecture, I think it's OK to proceed with this architecture for now.
Comment on attachment 8339605 [details] [diff] [review] Patch by Emilia Kasper Review of attachment 8339605 [details] [diff] [review]: ----------------------------------------------------------------- This needs some tests.
Comment on attachment 8339605 [details] [diff] [review] Patch by Emilia Kasper Review of attachment 8339605 [details] [diff] [review]: ----------------------------------------------------------------- I'm goint to r+ the patch, but there is some things missing: 1) a minimual server side implementation. 2) tests in all.sh (which will require (1)). bob ::: lib/ssl/ssl3ext.c @@ +77,5 @@ > + PRBool append, > + PRUint32 maxBytes); > +static SECStatus ssl3_ClientHandleSignedCertTimestampXtn(sslSocket *ss, > + PRUint16 ex_type, > + SECItem *data); where's the server send and server handle?
Attachment #8339605 - Flags: review+
(In reply to Martin Thomson [:mt:] from comment #4) > This needs some tests. (In reply to Robert Relyea from comment #5) > where's the server send and server handle? Well, like the title and patch description says, this only implements it for the client side :) This doesn't implement the server side (intentionally). If that's a hard block for this (which is unclear - it seems like Martin's opposed, and Bob's R+ is contingent upon it), there's a question about what the server side interface and public API should look like. There are a variety of options, each with long-term consequences, which is why only the client side was implemented, since that is most immediate relevant/pressing/beneficial to the main NSS consumers (read: Firefox and Chrome) Options include: 1) An API to set the extension data for the server 1.a) Should this be done as a buffer, a filepath, or a structure? 1.b) Should NSS validate, on the server side, that the input conforms to what the client side will expect? From a TLS spec perspective, arguably it should, but from what the NSS client API exposes, NSS itself is *not* validating the well-formedness of the SCT response, so why should the server? 2) A callback to provide the SCT details on demand, leaving the actual supplying/parsing of that up to NSS servers 2.a) SCTs may need to change over time during the lifetime of the server. Should it require a server restart (as somewhat, but not intrinsically, implied by #1) or should it be dynamically loaded (e.g. as proposed here with #2) If and When NSS decides to support more robust capabilities with CT, will the API decisions made still be meaningful? For example, if NSS gained native support for CT, would we still want to be supplying a buffer? Or a validated structure? And would we then want to validate it, because we could? It wasn't accident or laziness that the server side was omitted, it was more about trying to land something with a smaller API footprint and potential impact, and with more immediate and clear value :) But if that's a precursor to landing this patch, it would be good to know what the desired server-exposed interfaces would be.
Attached patch rebased patch with unit test (obsolete) — — Splinter Review
- Rebased the original patch onto the latest dev NSS. - Added unit test for extraction of signed certificate timestamp data, using ssl_gtest's packet filters to emulate server side extension support.
Bob, is having a server implementation a blocker for this, or can we move ahead with only the client for now?
Flags: needinfo?(rrelyea)
Sergei's patch looks interesting, but I don't think that it demonstrates that the handshake completes successfully with the right extension, so I think we need a full server implementation here.
Comment on attachment 8667859 [details] [diff] [review] rebased patch with unit test Review of attachment 8667859 [details] [diff] [review]: ----------------------------------------------------------------- I am marking feedback+ instead of review+ because it is still not settled whether the tests are adequate. I suggest some changes. Please upload a new patch with the issues addressed. Thanks. ::: cmd/tstclnt/tstclnt.c @@ +164,5 @@ > csa->len); > } > + scts = SSL_PeerSignedCertTimestamps(fd); > + if (scts && scts->len) { > + fprintf(stderr, "Received a Signed Certificate Timestamp item of length" Nit: if "item" is referring to the SECItem type that holds the return value of SSL_PeerSignedCertTimestamps(), then I think "item" can be omitted in this log message. (I know "item" comes from the "items" in the log message for the SSL_PeerStapledOCSPResponses() call above.) ::: external_tests/ssl_gtest/ssl_extension_unittest.cc @@ +620,5 @@ > + Handshake(); > + > + // Injecting a ServerHello extension invalidates the Finished hash, > + // failing the handshake with SSL_ERROR_BAD_HANDSHAKE_HASH_VALUE. > + // But we already have our extension data extracted at this point. This depends on a behavior of the current implementation. In general it is not good to assume what information can be extracted from an SSL socket after a handshake failure. But this may be the most expedient way to test SSL_PeerSignedCertTimestamps(). A better approach, which may be more similar to how SSL_PeerSignedCertTimestamps() will be used in Firefox, is to register a certificate authentication callback using SSL_AuthCertificateHook() and call SSL_PeerSignedCertTimestamps() in the certificate authentication callback. Can you try to do this? ::: lib/ssl/ssl.def @@ +183,5 @@ > SSL_GetPreliminaryChannelInfo; > SSL_SignaturePrefSet; > SSL_SignaturePrefGet; > SSL_SignatureMaxCount; > +SSL_PeerSignedCertTimestamps; Nit: list the exported functions in alphabetical order. Note: this patch is unlikely to go into NSS 3.21, so this is more for future reference. ::: lib/ssl/ssl3con.c @@ +6785,5 @@ > > + /* Copy Signed Certificate Timestamps, if any. */ > + if (ss->xtnData.signedCertTimestamps.data) { > + rv = SECITEM_CopyItem(NULL, &sid->u.ssl3.signedCertTimestamps, > + &ss->xtnData.signedCertTimestamps); Nit: I suggest aligning this argument with the "NULL" argument on the previous line. @@ +6791,5 @@ > + goto loser; > + } > + /* Clean up the temporary pointer to the handshake buffer. */ > + ss->xtnData.signedCertTimestamps.data = NULL; > + ss->xtnData.signedCertTimestamps.len = 0; Nit: this only needs to be done inside the if statement, when we know ss->xtnData.signedCertTimestamps.data is not NULL. ::: lib/ssl/ssl3ext.c @@ +284,5 @@ > { ssl_app_layer_protocol_xtn, &ssl3_ClientHandleAppProtoXtn }, > { ssl_use_srtp_xtn, &ssl3_ClientHandleUseSRTPXtn }, > { ssl_cert_status_xtn, &ssl3_ClientHandleStatusRequestXtn }, > { ssl_extended_master_secret_xtn, &ssl3_HandleExtendedMasterSecretXtn }, > + { ssl_signed_certificate_timestamp_xtn, Nit: I suggest we shorten this to ssl_signed_cert_timestamp_xtn. I understand you want to use the exact same extension so we can easily search for it, but this is exactly why I shortened it in the original patch. Note that I shortened application_layer_protocol_negotiation to app_layer_protocol for the same reason. The seachability can be addressed in other ways. I will suggest one way in sslt.h. ::: lib/ssl/sslt.h @@ +233,5 @@ > #endif > ssl_signature_algorithms_xtn = 13, > ssl_use_srtp_xtn = 14, > ssl_app_layer_protocol_xtn = 16, > + ssl_signed_certificate_timestamp_xtn = 18, /* RFC 6962 */ Nit: I suggest shortening this to ssl_signed_cert_timestamp_xtn and add "signed_certificate_timestamp" in the comment so that it can be found by a search for the extension name.
Attachment #8667859 - Flags: feedback+
Comment on attachment 8667859 [details] [diff] [review] rebased patch with unit test Review of attachment 8667859 [details] [diff] [review]: ----------------------------------------------------------------- If we can improve the new test in ssl_extension_unittest.cc so that it doesn't assume SSL_PeerSignedCertTimestamps() still works after a handshake failure, then I will be satisfied. Below I outline a minimal server-side implementation, good enough to implement an NSS self-test with a successful handshake. I suggest that we do that in a separate patch. The reason is code review quality. I don't think I can review a patch for this bug carefully too many times. So I'd like us to converge quickly to a patch that meets the minimum requirements. So, I hope Richard, MT, and Bob can comment on whether this patch meets the minimum requirements. ::: external_tests/ssl_gtest/ssl_extension_unittest.cc @@ +607,5 @@ > + const uint8_t val[] = { 0x00, 0x01, 0x02, 0x03, 0x04 }; > + DataBuffer timestamp_data(val, sizeof(val)); > + > + server_->SetPacketFilter(new TlsExtensionInjector( > + ssl_signed_certificate_timestamp_xtn, timestamp_data)); A minimal server-side implementation could be modeled after the SSL_SetStapledOCSPResponses() function: SSL_IMPORT SECStatus SSL_SetStapledOCSPResponses(PRFileDesc *fd, const SECItemArray *responses, SSLKEAType kea); Kai Engert would be the best person to advise on this because he wrote the SSL_SetStapledOCSPResponses() function.
Comment on attachment 8667859 [details] [diff] [review] rebased patch with unit test Review of attachment 8667859 [details] [diff] [review]: ----------------------------------------------------------------- ::: external_tests/ssl_gtest/ssl_extension_unittest.cc @@ +602,5 @@ > } > } > > +// Certificate Transparency (RFC 6962) - test extension data extraction > +TEST_P(TlsExtensionTestGeneric, SignedCertificateTimestampExtraction) { It would be good to add another test that disables the SSL_ENABLE_SIGNED_CERT_TIMESTAMPS option and verifies that SSL_PeerSignedCertTimestamps returns a zero-length SECItem. ::: lib/ssl/ssl.h @@ +566,5 @@ > + * extension data provided by the TLS server. The return value is a pointer > + * to an internal SECItem that contains the returned response (as a serialized > + * SignedCertificateTimestampList, see RFC 6962). The returned pointer is only > + * valid until the callback function that calls SSL_PeerSignedCertTimestamps > + * (e.g. the authenticate certificate hook, or the handshake callback) returns. If this limitation is true, then the new test in ssl_extension_unittest.cc should only call SSL_PeerSignedCertTimestamps from a callback function.
> Bob, is having a server implementation a blocker for this, or can we move ahead with only the client for now? We need at least a minimal server implementation so we have the server side of the test cases implemented. It doesn't have to be a full featured implementation.
Flags: needinfo?(rrelyea)
Wan-Teh - thank you for your comments, especially for the outline of the server-side implementation. I'll upload the updated code using two separate patches as suggested.
Hi Sergei, If you can finish the patch this week or next week, then just upload everything in a single patch, if it is not easy to separate it into two patches. (For example, ssl3ext.c will contain both the client-side and server-side changes and I suspect that they may be close to each other in the file.) I don't want to create extra work for you.
Hi Wan-Teh, Thanks :) At this point at least, separating the patches was easy. The first patch below contains the client side support targeting NSS 3.21 as before. All the comments from the code review were addressed. The second patch is the server side support with unit tests. On the server side, the timestamps to be sent to the client are retrieved using a callback. Since the goal of the server side implementation is only to support the unit tests, it is not available via the public API, but only by accessing internal data structures (which the tests can do). The following tests were implemented: 1. Receiving timestamps during a successful handshake flow. 2. Returning an empty buffer when no timestamps are available. 3. Server not sending the extension when no callback is set.
Attached patch server side and unit tests (obsolete) — — Splinter Review
Comment on attachment 8674206 [details] [diff] [review] rebased patch with CR fixes, pass 1 Review of attachment 8674206 [details] [diff] [review]: ----------------------------------------------------------------- r=wtc. I suggest a change to sslsock.c. ::: lib/ssl/ssl.h @@ +202,5 @@ > * disabled by default. > */ > #define SSL_ENABLE_EXTENDED_MASTER_SECRET 30 > > +/* Request Signed Certificate Timestamps via TLS extension (client) */ I wonder if the "(client)" part should be removed. ::: lib/ssl/sslsock.c @@ +2048,5 @@ > + PORT_SetError(SSL_ERROR_FEATURE_NOT_SUPPORTED_FOR_SSL2); > + return NULL; > + } > + return &ss->sec.ci.sid->u.ssl3.signedCertTimestamps; > +} I know this is the result of a "fuzzy" patch application. Please move SSL_PeerSignedCertTimestamps so that it is not in the middle of the VersionRange functions. I suggest we move it after the SSL_PeerStapledOCSPResponses() function.
Attachment #8674206 - Flags: review+
Comment on attachment 8674207 [details] [diff] [review] server side and unit tests Review of attachment 8674207 [details] [diff] [review]: ----------------------------------------------------------------- Hi Sergei, Thank you very much for the server-side implementation and unit tests. I'm afraid that you misunderstood what we asked you to do. Please add a public function to ssl.h that sets the signed certificate timestamps for a given server certificate. As I suggested at the end of comment 11, I think we can model the function after the SSL_SetStapledOCSPResponses() function: SSL_IMPORT SECStatus SSL_SetStapledOCSPResponses(PRFileDesc *fd, const SECItemArray *responses, SSLKEAType kea); Did you consider this approach and encounter a problem?
Attachment #8674207 - Flags: review-
Hi Wan-Teh, Thank you for your comments. I've used OCSP Stapling as a model, without the public function part. I'll explain. In light of Ryan Sleevi's notes in comment 6 regarding the uncertainties of how a server-side API should look like, I wanted to avoid introducing a new public function (and the implied long term commitment) just for the sake of the unit test alone. This can be done since unit tests have access to private data structures. I initially tried injecting a server extension sender into a live socket from the unit test code, mimicking ssl3_RegisterServerHelloExtensionSender. That didn't work out (I can elaborate if needed). Given that, code had to be added to the NSS core, but I tried to keep that code to the absolute minimum necessary. Hence the internal callback, which is simpler on the NSS side (since it reflects the internal flow), but messier on the unit test side (which is not a concern). What do you think of this approach?
Hi Wan-Teh, An updated version of the server side code is attached. This version follows the OCSP Stapling model more closely. Please let me know if this version matches the approach you prefer. Thanks, Sergei
Attachment #8674207 - Attachment is obsolete: true
Comment on attachment 8688570 [details] [diff] [review] server side and unit tests, CR pass 1 Review of attachment 8688570 [details] [diff] [review]: ----------------------------------------------------------------- Martin: could you please review at least the ssl_gtest part of this patch? Sergei: I only reviewed the lib/ssl part of this patch. I suggest some changes. The main suggestion is to support a server with multiple certificates. ::: lib/ssl/ssl.h @@ +590,5 @@ > SSLKEAType kea); > > /* > + * SSL_SetSignedCertTimestamps stores serialized signed_certificate_timestamp > + * extension data in the fd. The timestamp data will be sent during Nit: timestamp => signed_certificate_timestamp to be more precise. @@ +595,5 @@ > + * the handshake (if requested by the client). > + * The function will duplicate the provided data item. > + */ > +SSL_IMPORT SECStatus > +SSL_SetSignedCertTimestamps(PRFileDesc *fd, SECItem *scts); 1. Please add a |SSLKEAType kea| argument, which allows us to specify a a particular server certificate if the server has multiple certificates. NOTE: it is OK to add the |kea| argument and leave it ignored initially. If you do that, please add a comment to note the limitation. But I think it should not be hard to support an array of signed certificate timestamps. 2. Please declare |scts| with const: const SECItem *scts ::: lib/ssl/ssl3ext.c @@ +2766,5 @@ > +ssl3_ServerSendSignedCertTimestampXtn(sslSocket * ss, > + PRBool append, > + PRUint32 maxBytes) > +{ > + SECItem *scts = &ss->signedCertTimestamps; Nit: declare this variable as const: const SECItem *scts = &ss->signedCertTimestamps; @@ +2768,5 @@ > + PRUint32 maxBytes) > +{ > + SECItem *scts = &ss->signedCertTimestamps; > + > + if (!scts->data) { Nit: it may be better to test !scts->len instead. @@ +2775,5 @@ > + } > + > + PRInt32 extension_length = 2 /* extension_type */ + > + 2 /* length(extension_data) */ + > + scts->len; This is just a commentary: I compared this function with ssl3_ServerSendStatusRequestXtn. I believe ssl3_ServerSendStatusRequestXtn is wrong because it doesn't send any extension data. Do you agree with my assertion? ::: lib/ssl/sslimpl.h @@ +1335,5 @@ > > + /* Signed certificate timestamps to be sent to the client > + ** in a TLS extension (server only). > + */ > + SECItem signedCertTimestamps; This should be an array of size kt_kea_size, like the two members above: SECItem signedCertTimestamps[kt_kea_size]; ::: lib/ssl/sslsock.c @@ +1949,5 @@ > sc->serverKeyBits = mc->serverKeyBits; > } > } > + if (sm->signedCertTimestamps.data) { > + if (SECSuccess != SECITEM_CopyItem(NULL, IMPORTANT: before we copy the data, we should free any existing data: if (ss->signedCertTimestamps.data) { SECITEM_FreeItem(&ss->signedCertTimestamps, PR_FALSE); } @@ +2562,5 @@ > + SSL_GETPID(), fd)); > + return SECFailure; > + } > + > + if (scts && scts->data && !scts->len) { Nit: no need to test scts->data, unless you distinguish between the following two cases: Case 1: scts->data == NULL scts->len == 0 Case 2: scts->data != NULL scts->len == 0 I think these two cases should be considered the same. @@ +2571,5 @@ > + if (ss->signedCertTimestamps.data) { > + SECITEM_FreeItem(&ss->signedCertTimestamps, PR_FALSE); > + } > + > + if (!scts) { Hmm... this probably should be considered an invalid input argument, rather than an empty signed_certificate_timestamp extension data. If you want to interpret this as empty data, please document this in the header file ssl.h. @@ +2577,5 @@ > + } > + > + SECStatus rv = > + SECITEM_CopyItem(NULL, &ss->signedCertTimestamps, scts); > + if (rv != SECSuccess) { Nit: we can just return |rv| here.
Attachment #8688570 - Flags: superreview?(martin.thomson)
Attachment #8688570 - Flags: review-
Comment on attachment 8688570 [details] [diff] [review] server side and unit tests, CR pass 1 Review of attachment 8688570 [details] [diff] [review]: ----------------------------------------------------------------- I went through the unit tests, which I think need some work. I think you could make this a lot simpler without the "null" value for databuffer and without a listener class. MT, you shoudl hold off till they are adjusted. ::: external_tests/ssl_gtest/databuffer.h @@ +139,5 @@ > const uint8_t *data() const { return data_; } > uint8_t* data() { return data_; } > size_t len() const { return len_; } > bool empty() const { return len_ == 0; } > + bool null() const { return data_ == nullptr; } Why can't this be private? @@ +171,5 @@ > return stream; > } > > +// Note: NULL buffers and zero-length array buffers are NOT the same thing. > +inline bool operator==(const DataBuffer& a, const DataBuffer& b) { This seems like a pretty bad semantic. I would find some other way to express it, e.g., having a slot that's a null ptr to DataBuffer. ::: external_tests/ssl_gtest/ssl_extension_unittest.cc @@ +627,5 @@ > + virtual void HandshakeCallback(TlsAgent& client) { > + const SECItem *scts = SSL_PeerSignedCertTimestamps(client.ssl_fd()); > + if (scts) { > + handshake_timestamps_.Assign(scts->data, scts->len); > + } The only way scts should be NULL is when you have an internal error, so just test for it being non-NULL here and in AuthCertificateCallback @@ +645,5 @@ > +}; > + > +// Test timestamps extraction. > +TEST_P(TlsExtensionTestGeneric, SignedCertificateTimestampsHandshake) { > + uint8_t val[] = { 0x01, 0x23, 0x45, 0x67, 0x89 }; const @@ +657,5 @@ > + client_->StartConnect(); > + ASSERT_EQ(SECSuccess, > + SSL_OptionSet(client_->ssl_fd(), > + SSL_ENABLE_SIGNED_CERT_TIMESTAMPS, PR_TRUE)); > + ClientSignedTimestampsExtractor *timestamps_extractor = Trailing whitespace @@ +683,5 @@ > + CheckConnected(); > + > + DataBuffer timestamps({}, 0); // corresponds to a zero-length SECItem > + ASSERT_EQ(timestamps, timestamps_extractor->handshake_timestamps()); > + ASSERT_EQ(timestamps, timestamps_extractor->auth_timestamps()); See above comments about null versus empty. ::: external_tests/ssl_gtest/tls_agent.h @@ +38,5 @@ > + virtual void HandshakeCallback(TlsAgent& agent); > + virtual void AuthCertificateCallback(TlsAgent& agent, > + PRBool checksig, PRBool isServer); > +}; > + I would rather have individual callback functions. This is C++11 so you can use lambdas if you want to carry some state.
Attachment #8688570 - Flags: superreview?(martin.thomson) → feedback-
You should also add tests for when only one side implements CT.
Hi Wan-Teh, Attached the updated code from lib/ssl part only. All the comments were addressed. Regarding ssl3_ServerSendStatusRequestXtn - seems like it is intentionally sending a zero-length extension, while the actual data is sent by ssl3_SendCertificateStatus. I've used ssl3_SendCertificateStatus as a model for selecting the proper timestamp to send out of the available items supplied by SSL_SetSignedCertTimestamps.
Hi Eric, Thanks for your review. I'll fix the issues, add new unit tests and let you know when done.
(In reply to Sergei Chernov from comment #26) > > Regarding ssl3_ServerSendStatusRequestXtn - seems like it is intentionally > sending a zero-length extension, while the actual data is sent by > ssl3_SendCertificateStatus. You are right! Thanks.
Comment on attachment 8691451 [details] [diff] [review] server side without unit tests, CR pass 2 Review of attachment 8691451 [details] [diff] [review]: ----------------------------------------------------------------- Hi Sergei, Thanks a lot for the new patch. I suggest some changes. ::: lib/ssl/ssl.h @@ +595,5 @@ > + * during the handshake (if requested by the client). Parameter |scts| > + * is for the server certificate of the key exchange type |kea|. > + * The function will duplicate the provided data item. To clear previously > + * set data for a given key exchange type |kea|, pass NULL or an empty item > + * to |scts|. Nit: I think we should support only one way to clear previously set data. Between the two methods, I prefer an empty item, and suggest we treat a NULL |scts| as an invalid argument. @@ +598,5 @@ > + * set data for a given key exchange type |kea|, pass NULL or an empty item > + * to |scts|. > + */ > +SSL_IMPORT SECStatus > +SSL_SetSignedCertTimestamps(PRFileDesc *fd, const SECItem *scts, Nit: please delete the space at the end of this line. @@ +599,5 @@ > + * to |scts|. > + */ > +SSL_IMPORT SECStatus > +SSL_SetSignedCertTimestamps(PRFileDesc *fd, const SECItem *scts, > + SSLKEAType kea); Nit: please align this with the first argument (PRFileDesc *fd) on the previous line. ::: lib/ssl/ssl3con.c @@ +6538,5 @@ > errCode = SSL_ERROR_NO_CYPHER_OVERLAP; > goto alert_loser; > } > ss->ssl3.hs.cipher_suite = (ssl3CipherSuite)temp; > ss->ssl3.hs.suite_def = ssl_LookupCipherSuiteDef((ssl3CipherSuite)temp); IMPORTANT: I guess the changes in this file maintain the invariant that we always set ss->ssl3.hs.suite_def and ss->ssl3.hs.kea_def at the same time, right? I think this seems reasonable. Another solution is to call ssl3_SetupPendingCipherSpec() earlier in ssl3_SendServerHello, before ssl3_SendServerHello calls ssl3_CallHelloExtensionSenders. But that requires more detailed code review. @@ +6539,5 @@ > goto alert_loser; > } > ss->ssl3.hs.cipher_suite = (ssl3CipherSuite)temp; > ss->ssl3.hs.suite_def = ssl_LookupCipherSuiteDef((ssl3CipherSuite)temp); > + ss->ssl3.hs.kea_def = (This comment documents my understanding of the code. You are welcome to confirm that I understand it correctly.) The original code only sets ss->ssl3.hs.kea_def in ssl3_SetupPendingCipherSpec(). @@ +6546,5 @@ > PORT_Assert(ss->ssl3.hs.suite_def); > if (!ss->ssl3.hs.suite_def) { > PORT_SetError(errCode = SEC_ERROR_LIBRARY_FAILURE); > goto loser; /* we don't send alerts for our screw-ups. */ > } Please move the new code here, after we have verified ss->ssl3.hs.suite_def is not a pointer, because the new code derefernces ss->ssl3.hs.suite_def. NOTE: we don't seem to check the return value of ssl_LookupCipherSuiteDef() elsewhere, so I think we only need to move the new code here. @@ +8222,5 @@ > ss->ssl3.hs.suite_def = > ssl_LookupCipherSuiteDef(ss->ssl3.hs.cipher_suite); > + ss->ssl3.hs.kea_def = > + &kea_defs[ss->ssl3.hs.suite_def->key_exchange_alg]; > + ss->ssl3.hs.preliminaryInfo |= ssl_preinfo_cipher_suite; Please fix the indentation in the remaining three changes in this file. ::: lib/ssl/ssl3ext.c @@ +2766,5 @@ > +ssl3_ServerSendSignedCertTimestampXtn(sslSocket * ss, > + PRBool append, > + PRUint32 maxBytes) > +{ > + SECItem *scts; Nit: Please declare this as const: const SECItem *scts; @@ +2767,5 @@ > + PRBool append, > + PRUint32 maxBytes) > +{ > + SECItem *scts; > + SSLKEAType effectiveExchKeyType; Nit: Please declare the extension_length variable at the beginning of the function: PRInt32 extension_length; This is required by older versions of Visual C++. I don't remember whether NSS has dropped support for them. @@ +2770,5 @@ > + SECItem *scts; > + SSLKEAType effectiveExchKeyType; > + > + PORT_Assert((ss->ssl3.hs.preliminaryInfo & ssl_preinfo_cipher_suite) == > + ssl_preinfo_cipher_suite); This assertion should be removed. I know why you assert this, but it is very non-obvious. Alternatively, please add a comment. But in general code that doesn't require a comment is better. @@ +2777,5 @@ > + ss->ssl3.hs.kea_def->kea == kea_dhe_rsa) { > + effectiveExchKeyType = ssl_kea_rsa; > + } else { > + effectiveExchKeyType = ss->ssl3.hs.kea_def->exchKeyType; > + } Optional: if you have spare time, please write a patch for ssl3_ServerSendStatusRequestXtn() to fix the TODO comment inside. Your changes to set ss->ssl3.hs.kea_def early enough for the hello extension senders should allow us to fix that TODO comment. @@ +2790,5 @@ > + PRInt32 extension_length = 2 /* extension_type */ + > + 2 /* length(extension_data) */ + > + scts->len; > + > + if (append && maxBytes >= extension_length) { Nit: I suggest we restructure this if-else-if statement as follows: if (maxBytes < extension_length) { PORT_Assert(0); return 0; } if (append) { ... } ::: lib/ssl/sslimpl.h @@ +1331,5 @@ > /* server cert and key for each KEA type */ > sslServerCerts serverCerts[kt_kea_size]; > /* each cert needs its own status */ > SECItemArray * certStatusArray[kt_kea_size]; > Nit: I suggest we omit this blank line to make it clearer these three members are related. (The best solution would be to define a struct to collect these three things and just have an array of the structs. You don't need to do that in this patch.) ::: lib/ssl/sslsock.c @@ +1955,5 @@ > + if (SECSuccess != SECITEM_CopyItem(NULL, > + &ss->signedCertTimestamps[i], &sm->signedCertTimestamps[i])) { > + goto loser; > + } > + } I think your code is correct, but it would be clearer to move this code into the "if (mc->serverCert && mc->serverCertChain)" block, after we handle certStatusArray. @@ +2565,5 @@ > + SSL_GETPID(), fd)); > + return SECFailure; > + } > + > + if ( kea <= 0 || kea >= kt_kea_size) { Nit: please remove the space after the opening parenthesis '('. @@ +2575,5 @@ > + if (ss->signedCertTimestamps[kea].data) { > + SECITEM_FreeItem(&ss->signedCertTimestamps[kea], PR_FALSE); > + } > + > + if (!scts || (scts->data && !scts->len)) { After the OR (||), just check !scts->len: if (!scts || !scts->len) { I would further suggest that we support just one way to clear previous data, and treat !scts as an error: At the beginning of the function, after the if (!ss) check: if (!scts) { PORT_SetError(SEC_ERROR_INVALID_ARGS); return SECFailure; } Here: if (!scts->len)) { /* ... */ return SECSuccess; } @@ +2578,5 @@ > + > + if (!scts || (scts->data && !scts->len)) { > + /* RFC 6962 mandates non-empty contents, so we can treat an empty item > + ** the same as NULL for consistency with SSL_PeerSignedCertTimestamps, > + ** which returns an empty item when no signed cert timestamps were sent. If we treat !scts as an error, then I think we can delete this comment entirely.
Attachment #8691451 - Flags: review-
Hi Wan-Teh, See the attached review fixes. Some notes - 1. On clearing previously set data with SSL_SetSignedCertTimestamps - I think in this case, it actually makes sense to allow both NULL and an empty item: NULL for consistency with SSL_SetStapledOCSPResponses which has similar semantics and uses NULL to clear data, and empty item for symmetry with the getter (SSL_PeerSignedCertTimestamps returns an empty item if no data was sent). What's a bit weird here is the case when SSL_PeerSignedCertTimestamps returns an empty item since empty extension is not allowed, but it's probably best not to change that. 2. I've fixed the TODO in ssl3_ServerSendStatusRequestXtn using the same "if" logic, but since we have this "if" in multiple places now, it probably makes sense to add a new field to ssl3KEADef struct and set it in kea_defs[] (ssl3con.c) for all the available key exchange algorithms. I am not sure though what's the exact reasoning behind the "if" and how to call the new field, might be better to postpone this for now.
Attachment #8691451 - Attachment is obsolete: true
Attachment #8692073 - Flags: review?(wtc)
Comment on attachment 8692073 [details] [diff] [review] server side without unit tests, CR pass 3 Review of attachment 8692073 [details] [diff] [review]: ----------------------------------------------------------------- Martin: please review at least the ssl3con.c changes in this patch. This is the patch I told you about at lunch yesterday. Sergei: the main reason I gave review- to this patch is the wrong indentation in ssl3con.c. I believe the logic is all correct now. I also suggest not handling/documenting an empty |scts| item (!scts->len) as a special case, because SECITEM_CopyItem() can copy an empty item correctly. Please consider that. Thanks. ::: lib/ssl/ssl.h @@ +595,5 @@ > + * during the handshake (if requested by the client). Parameter |scts| > + * is for the server certificate of the key exchange type |kea|. > + * The function will duplicate the provided data item. To clear previously > + * set data for a given key exchange type |kea|, pass NULL or an empty item > + * to |scts|. I agree it is good to interpret a NULL |scts| argument this way to be consistent with how SSL_SetStapledOCSPResponses interprets a NULL |responses| argument. Looking at the code closer, I think it is not necessary to point out that an empty |scts| item has the same effect, because the intention of an empty item is very clear. ::: lib/ssl/ssl3con.c @@ +8222,5 @@ > + ss->ssl3.hs.suite_def = > + ssl_LookupCipherSuiteDef(ss->ssl3.hs.cipher_suite); > + ss->ssl3.hs.kea_def = > + &kea_defs[ss->ssl3.hs.suite_def->key_exchange_alg]; > + ss->ssl3.hs.preliminaryInfo |= ssl_preinfo_cipher_suite; The indentation of many changes in this file is wrong. You can see the wrong indentation at https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=944175&attachment=8692073 Please check your text editor's settings. In NSS source code, set the tabstop to 8. lib/ssl files use tab characters extensively. We are planning to fix that. Until then, please refrain from manually replacing tabs with spaces. ::: lib/ssl/ssl3ext.c @@ +987,2 @@ > > + /* See also ssl3_SendCertificateStatus. */ Nit/Optional: it would be good to also say what we should look for in ssl3_SendCertificateStatus. I guess there are two points: 1. ssl3_SendCertificateStatus uses the exact same logic to select the server certificate and determine if we have the status for that certificate. 2. The certificate status is sent in ssl3_SendCertificateStatus.. ::: lib/ssl/sslsock.c @@ +2576,5 @@ > + if (ss->signedCertTimestamps[kea].data) { > + SECITEM_FreeItem(&ss->signedCertTimestamps[kea], PR_FALSE); > + } > + > + if (!scts || !scts->len) { In the !scts->len case, we can just fall through and call SECITEM_CopyItem(). SECITEM_CopyItem() will copy an empty item correctly. We should do this if we also omit the mention of an empty item in the comments for this function in ssl.h.
Attachment #8692073 - Flags: review?(wtc)
Attachment #8692073 - Flags: review?(martin.thomson)
Attachment #8692073 - Flags: review-
Comment on attachment 8692073 [details] [diff] [review] server side without unit tests, CR pass 3 Review of attachment 8692073 [details] [diff] [review]: ----------------------------------------------------------------- I'm only giving this an r- on the basis that there are no tests here. Previous patches included tests. ::: lib/ssl/ssl.h @@ +595,5 @@ > + * during the handshake (if requested by the client). Parameter |scts| > + * is for the server certificate of the key exchange type |kea|. > + * The function will duplicate the provided data item. To clear previously > + * set data for a given key exchange type |kea|, pass NULL or an empty item > + * to |scts|. Wan-Teh, what value does an empty extension provide here? @@ +599,5 @@ > + * to |scts|. > + */ > +SSL_IMPORT SECStatus > +SSL_SetSignedCertTimestamps(PRFileDesc *fd, const SECItem *scts, > + SSLKEAType kea); For Wan-Teh: I know that we index certificates by kea type, but that is a mistake. We really should be indexing by SSLAuthType for anything certificate-related. However, I don't want to change just one of these. Do you have any ideas on how we might proceed here? Maintaining backward compatibility is going to be difficult. ::: lib/ssl/ssl3con.c @@ +6542,5 @@ > ss->ssl3.hs.suite_def = ssl_LookupCipherSuiteDef((ssl3CipherSuite)temp); > ss->ssl3.hs.preliminaryInfo |= ssl_preinfo_cipher_suite; > PORT_Assert(ss->ssl3.hs.suite_def); > if (!ss->ssl3.hs.suite_def) { > + PORT_SetError(errCode = SEC_ERROR_LIBRARY_FAILURE); Ugh, assignment within an argument list. @@ +6546,5 @@ > + PORT_SetError(errCode = SEC_ERROR_LIBRARY_FAILURE); > + goto loser; /* we don't send alerts for our screw-ups. */ > + } > + ss->ssl3.hs.kea_def = > + &kea_defs[ss->ssl3.hs.suite_def->key_exchange_alg]; This block of code (from 6541 down) is duplicated. Now that it's non-trivial in size, can you please move that into a new (static) function? I don't think that you need to ssl3_SetupPendingCipherSpec as Wan-Teh suggested, that takes locks and updates the read and write cipher specs, which is probably best deferred. However, I would remove the (re)assignments to .hs.suite_def and .hs.kea_def that happen there. ::: lib/ssl/ssl3ext.c @@ +2773,5 @@ > + if (ss->ssl3.hs.kea_def->kea == kea_ecdhe_rsa || > + ss->ssl3.hs.kea_def->kea == kea_dhe_rsa) { > + effectiveExchKeyType = ssl_kea_rsa; > + } else { > + effectiveExchKeyType = ss->ssl3.hs.kea_def->exchKeyType; I hate this code, but we don't have a better answer yet. Note that ECDSA suites succeed only by accident.
Attachment #8692073 - Flags: review?(martin.thomson) → review-
(In reply to Martin Thomson [:mt:] from comment #32) > Comment on attachment 8692073 [details] [diff] [review] > server side without unit tests, CR pass 3 [...] > I'm only giving this an r- on the basis that there are no tests here. > Previous patches included tests. Sergei split the patch into the lib/ssl part (this patch) and the ssl_gtest part (not yet attached) because I told him to focus on the lib/ssl part this week while I have spare time for code review. I only review the lib/ssl part, so he's trying to make my life easier. Please reconsider the r- in light of this.
I scanned the discussion, but missed that. Yes, I will change this to r+ when I see a revised patch. I agree that the logic is sound. Sergei, can you mark the old patches as obsolete so that we can see which ones are still relevant?
Comment on attachment 8692073 [details] [diff] [review] server side without unit tests, CR pass 3 Review of attachment 8692073 [details] [diff] [review]: ----------------------------------------------------------------- Martin: thanks for the review. I responded to some of your comments. ::: lib/ssl/ssl.h @@ +595,5 @@ > + * during the handshake (if requested by the client). Parameter |scts| > + * is for the server certificate of the key exchange type |kea|. > + * The function will duplicate the provided data item. To clear previously > + * set data for a given key exchange type |kea|, pass NULL or an empty item > + * to |scts|. Martin: I am not sure. I guess Sergei provided the ability to clear previously set data because SSL_SetStapledOCSPResponses (the function I asked him to model this function after) provides this ability. The signed_certificate_timestamp TLS extension does not permit empty extension data, so setting empty data essentially disables the extension. Perhaps that is useful? @@ +599,5 @@ > + * to |scts|. > + */ > +SSL_IMPORT SECStatus > +SSL_SetSignedCertTimestamps(PRFileDesc *fd, const SECItem *scts, > + SSLKEAType kea); Martin: I remember you brought up this issue at lunch yesterday. I am sorry that I am not familiar with this issue, so I can't evaluate your proposal. I guess you are referring to the need to handle kea_ecdhe_rsa and kea_dhe_rsa as special cases? Here are the affected functions: * SSL_SetStapledOCSPResponses (relatively recent) * SSL_ConfigSecureServer (old) * SSL_ConfigSecureServerWithCertChain (old) I think the use of an "SSLKEAType kea" argument was first introduced in SSL_ConfigSecureServer*. Then it was copied to SSL_SetStapledOCSPResponses. I am afraid that in the near term the easiest thing to do is to remain consistent with these three functions. ::: lib/ssl/ssl3con.c @@ +6542,5 @@ > ss->ssl3.hs.suite_def = ssl_LookupCipherSuiteDef((ssl3CipherSuite)temp); > ss->ssl3.hs.preliminaryInfo |= ssl_preinfo_cipher_suite; > PORT_Assert(ss->ssl3.hs.suite_def); > if (!ss->ssl3.hs.suite_def) { > + PORT_SetError(errCode = SEC_ERROR_LIBRARY_FAILURE); I also noticed this. I'll write a separate patch to fix this. ::: lib/ssl/ssl3ext.c @@ +2773,5 @@ > + if (ss->ssl3.hs.kea_def->kea == kea_ecdhe_rsa || > + ss->ssl3.hs.kea_def->kea == kea_dhe_rsa) { > + effectiveExchKeyType = ssl_kea_rsa; > + } else { > + effectiveExchKeyType = ss->ssl3.hs.kea_def->exchKeyType; Martin: I regret I didn't ask you to explain this to me in person yesterday. Please elaborate why this code is wrong, and why the ECDSA suites succeed only by accident.
(In reply to Wan-Teh Chang from comment #35) > > + if (ss->ssl3.hs.kea_def->kea == kea_ecdhe_rsa || > > + ss->ssl3.hs.kea_def->kea == kea_dhe_rsa) { > > + effectiveExchKeyType = ssl_kea_rsa; > > + } else { > > + effectiveExchKeyType = ss->ssl3.hs.kea_def->exchKeyType; > > Martin: I regret I didn't ask you to explain this to me in person > yesterday. Please elaborate why this code is wrong, and why the > ECDSA suites succeed only by accident. So, I think in the long-distant past, the separation between kea and auth was not present. That is why we have kt_dh and kt_rsa and kt_dsa as #defines. However, whatever split happened incorrectly bound these to SSLKEAType values. At the same time, the values for storage are also indexed by SSLKEAType, and all the (deprecated) kt_* macros are defined by SSLKEAType values. The most important being serverCerts: https://dxr.mozilla.org/mozilla-central/rev/47b49b0d32360fab04b11ff9120970979c426911/security/nss/lib/ssl/sslimpl.h#1314 This works, but only by accident. As it turns out the values for key exchange line up with the values for authentication, see: typedef enum { typedef enum { ssl_kea_null = 0, ssl_auth_null = 0, ssl_kea_rsa = 1, ssl_auth_rsa = 1, ssl_kea_dh = 2, ssl_auth_dsa = 2, ssl_kea_fortezza = 3, ssl_auth_kea = 3, ssl_kea_ecdh = 4, ssl_auth_ecdsa = 4 ssl_kea_size ... } SSLKEAType; } SSLAuthType; As you can see, this is why dsa certs are stored against kt_dh. (I have no idea how fortezza works, so I don't know what happens with kt_fortezza.) As you can see here, the fact that we index based on ECDH(E) key exchange works because, coincidentally, all ECDHE cipher suites also have ECDSA variants, apart from those with kea_ecdhe_rsa. The same twisted logic applies for DHE cipher suites. I think that the correct approach would be to move these definitions to SSLAuthType. As a practical matter, this would be easy. As for the functions that depend on this, a new function could be defined. The old ones could have a simple switch statement so that old code could continue to work, I would ensure that this included `default: return SECFailure;`.
Hi Wan-Teh, An updated patch is attached. Thanks for the info regarding tabstops - seems like ssl3con.c uses a weird mixture of tabs and spaces, hope I got it right this time. I've also marked irrelevant files as obsolete. Besides the already reviewed client-side part and the attached server-side part, there are also unit tests that I will upload soon after addressing Eric's comments.
Attachment #8667859 - Attachment is obsolete: true
Attachment #8688570 - Attachment is obsolete: true
Attachment #8692073 - Attachment is obsolete: true
Attachment #8694372 - Flags: review?(wtc)
Attached patch Patch for ssl3con.c (obsolete) — — Splinter Review
Sergei: the indentation of a change in ssl3con.c is still not right. There are also some extraneous whitespace changes. I prepared this patch for ssl3con.c for you. Please revert your changes to ssl3con.c and apply this patch. Thanks.
Wan-Teh: Thank you for fixing the indentation issue. The updated patch is attached.
Attachment #8694372 - Attachment is obsolete: true
Attachment #8694372 - Flags: review?(wtc)
Attachment #8694568 - Flags: review?(wtc)
Attached patch unit tests, CR pass 1 (obsolete) — — Splinter Review
Hi Eric, An updated patch for the unit tests is attached. All the comments were addressed.
Attachment #8694747 - Flags: review?(ekr)
Comment on attachment 8694568 [details] [diff] [review] server side without unit tests, CR pass 4.1 (fixed indentation) Review of attachment 8694568 [details] [diff] [review]: ----------------------------------------------------------------- r=wtc. Martin, the unit tests are in a separate patch.
Attachment #8694568 - Flags: superreview?(martin.thomson)
Attachment #8694568 - Flags: review?(wtc)
Attachment #8694568 - Flags: review+
Attachment #8694568 - Flags: review?(ekr)
Comment on attachment 8694747 [details] [diff] [review] unit tests, CR pass 1 Review of attachment 8694747 [details] [diff] [review]: ----------------------------------------------------------------- ::: external_tests/ssl_gtest/databuffer.h @@ +52,5 @@ > Assign(other.data(), other.len()); > } > void Assign(const uint8_t* data, size_t len) { > Allocate(len); > + if (data) { This seems wrong. It leaves us in a state with uninitilized memory of type len. @@ +169,5 @@ > } > > +inline bool operator==(const DataBuffer& a, const DataBuffer& b) { > + return > + (a.empty() && b.empty()) || (0 == memcmp(a.data(), b.data(), a.len())); What happens if a.len() != b.len()? ::: external_tests/ssl_gtest/ssl_extension_unittest.cc @@ +618,5 @@ > +class SignedCertificateTimestampsExtractor { > + public: > + SignedCertificateTimestampsExtractor(TlsAgent& client) { > + client.SetAuthCertificateCallback( > + [&](TlsAgent& agent, PRBool checksig, PRBool isServer) { should we be capturing anything here? Why not []? @@ +625,5 @@ > + auth_timestamps.reset(new DataBuffer(scts->data, scts->len)); > + } > + ); > + client.SetHandshakeCallback( > + [&](TlsAgent& agent) { []? @@ +642,5 @@ > + ASSERT_EQ(timestamps, *handshake_timestamps); > + } > + > + std::unique_ptr<DataBuffer> auth_timestamps; > + std::unique_ptr<DataBuffer> handshake_timestamps; These should be private and end in _ @@ +647,5 @@ > +}; > + > +// Test timestamps extraction during a successful handshake. > +TEST_P(TlsExtensionTestGeneric, SignedCertificateTimestampsHandshake) { > + uint8_t val[] = { 0x01, 0x23, 0x45, 0x67, 0x89 }; Can you make this global const and name it kSomething @@ +669,5 @@ > +} > + > +// Test SSL_PeerSignedCertTimestamps returning zero-length SECItem > +// when the client / the server / both have not enabled the feature. > + Extra blank line @@ +684,5 @@ > + > + SignedCertificateTimestampsExtractor timestamps_extractor(*client_); > + Handshake(); > + CheckConnected(); > + timestamps_extractor.assertTimestamps(DataBuffer()); So what happens if someone feeds you an empty SCT extension? How is this distinguishable from no SCT extension? ::: external_tests/ssl_gtest/tls_agent.h @@ +10,5 @@ > #include "prio.h" > #include "ssl.h" > > #include <iostream> > +#include <functional> Sergei, we should probably verify that functional works on our builders. @@ +270,5 @@ > size_t send_ctr_; > size_t recv_ctr_; > bool expected_read_error_; > + HandshakeCallbackFunction handshake_callback_; > + AuthCertificateCallbackFunction auth_certificate_callback_; Please explicitly initialize these as () in the ctor. That's the convention here.
Attachment #8694747 - Flags: review?(ekr)
Comment on attachment 8694568 [details] [diff] [review] server side without unit tests, CR pass 4.1 (fixed indentation) Review of attachment 8694568 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/ssl/ssl.def @@ +188,5 @@ > ;+*; > ;+}; > +;+NSS_3.22 { # NSS 3.22 release > +;+ global: > +SSL_PeerSignedCertTimestamps; Nit: you probably shouldn't have this in this patch because it only implements the server side. Ignore this comment if you merge the patches before landing. ::: lib/ssl/ssl3ext.c @@ +1001,2 @@ > return 0; > + } I did not review this kea_type code. MT has extensive experience with it. @@ +2777,5 @@ > + ss->ssl3.hs.kea_def->kea == kea_dhe_rsa) { > + effectiveExchKeyType = ssl_kea_rsa; > + } else { > + effectiveExchKeyType = ss->ssl3.hs.kea_def->exchKeyType; > + } I also did not review this code. @@ +2800,5 @@ > + /* extension_type */ > + rv = ssl3_AppendHandshakeNumber(ss, > + ssl_signed_cert_timestamp_xtn, > + 2); > + if (rv != SECSuccess) goto loser; For better or worse, this code seems to generally break after the conditional ::: lib/ssl/sslsock.c @@ +1943,5 @@ > + if (sm->signedCertTimestamps[i].data) { > + if (ss->signedCertTimestamps[i].data) { > + SECITEM_FreeItem(&ss->signedCertTimestamps[i], PR_FALSE); > + } > + if (SECSuccess != SECITEM_CopyItem(NULL, Please put the function call first. @@ +2567,5 @@ > + return SECFailure; > + } > + > + if (kea <= 0 || kea >= kt_kea_size) { > + SSL_DBG(("%d: SSL[%d]: invalid key in SSL_SetSignedCertTimestamps", Nit: invalid key type @@ +2573,5 @@ > + return SECFailure; > + } > + > + if (ss->signedCertTimestamps[kea].data) { > + SECITEM_FreeItem(&ss->signedCertTimestamps[kea], PR_FALSE); It seems that you can unconditionally call SECITEM_FreeItem() whether .data is NULL or not: void SECITEM_FreeItem(SECItem *zap, PRBool freeit) { if (zap) { PORT_Free(zap->data); zap->data = 0; zap->len = 0; if (freeit) { PORT_Free(zap); } } } And PORT_Free also checks for a NULL ptr: void PORT_Free(void *ptr) { if (ptr) { PR_Free(ptr); } } I would suggest replacing all the conditionals around SECITEM_FreeItem.data. WTC, wdyt?
Attachment #8694568 - Flags: review?(ekr)
Attachment #8694568 - Flags: superreview?(martin.thomson) → superreview+
Hi Eric, An updated file is attached. Some comments: > This seems wrong. It leaves us in a state with uninitilized memory of type len. My intention was only to make sure memcpy is not called with a null pointer. The updated code in the attached file is more strict than the original, but it's probably a good thing (the tests still pass). > should we be capturing anything here? Why not []? "this" is implicitly captured so we can update the relevant member variable with the received timestamp. I think it is more clear in the updated code. > Can you make this global const and name it kSomething Using byte arrays on the stack is a common pattern in this tests file. A byte array here is used to initialize a SECItem so it can't be made const without copying or const-casting which I think is redundant just for making the original array const. > what happens if someone feeds you an empty SCT extension? Empty contents are not allowed with this extension. SSL_PeerSignedCertTimestamps returns an empty SECItem in case no extension was received, which is the result we expect here. > we should probably verify that functional works on our builders How can we verify this? <functional> is needed here for std::function. Alternatively, we can go back to the old code which uses a class for the callbacks. Let me know what you think.
Attachment #8694747 - Attachment is obsolete: true
Attachment #8695248 - Flags: review?(ekr)
Comment on attachment 8695248 [details] [diff] [review] unit tests, CR pass 2 Review of attachment 8695248 [details] [diff] [review]: ----------------------------------------------------------------- LGTM with nits below. ::: external_tests/ssl_gtest/databuffer.h @@ +174,5 @@ > } > > +inline bool operator==(const DataBuffer& a, const DataBuffer& b) { > + return (a.empty() && b.empty()) || > + (a.len() == b.len() && 0 == memcmp(a.data(), b.data(), a.len())); !memcmp ::: external_tests/ssl_gtest/ssl_extension_unittest.cc @@ +615,5 @@ > + > +// Helper class - stores signed certificate timestamps as provided > +// by the relevant callbacks on the client. > +class SignedCertificateTimestampsExtractor { > + public: Need to initialize members in ctor.
Attachment #8695248 - Flags: review?(ekr) → review+
It looks like all the main patches have been approved here. Wan-Teh, what is the status of your patch from 12/01? Does that need review? Is it part of this?
Attachment #8694394 - Attachment is obsolete: true
Attachment #8339605 - Attachment is obsolete: true
I spoke to WTC and we resolved the remaining issues. I am now going to attempt to land this.
Comment on attachment 8694568 [details] [diff] [review] server side without unit tests, CR pass 4.1 (fixed indentation) Review of attachment 8694568 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/ssl/ssl.def @@ +188,5 @@ > ;+*; > ;+}; > +;+NSS_3.22 { # NSS 3.22 release > +;+ global: > +SSL_PeerSignedCertTimestamps; Eric: I believe you figured out what was wrong. Patch #1 (client side) incorrectly added SSL_PeerSignedCertTimestamps to the NSS_3.21 section. It would have been nice to fix that before landing the patches, but I guess you wanted to land the reviewed patches faithfully. ::: lib/ssl/ssl3ext.c @@ +2800,5 @@ > + /* extension_type */ > + rv = ssl3_AppendHandshakeNumber(ss, > + ssl_signed_cert_timestamp_xtn, > + 2); > + if (rv != SECSuccess) goto loser; I agree. I didn't comment on this because we're going to reformat the entire file using clang-format soon. UPDATE: I found that both styles are heavily used in this file (ssl3ext.c). So I'll just let clang-format reformat this. ::: lib/ssl/sslsock.c @@ +1943,5 @@ > + if (sm->signedCertTimestamps[i].data) { > + if (ss->signedCertTimestamps[i].data) { > + SECITEM_FreeItem(&ss->signedCertTimestamps[i], PR_FALSE); > + } > + if (SECSuccess != SECITEM_CopyItem(NULL, I will fix this. @@ +2567,5 @@ > + return SECFailure; > + } > + > + if (kea <= 0 || kea >= kt_kea_size) { > + SSL_DBG(("%d: SSL[%d]: invalid key in SSL_SetSignedCertTimestamps", I will fix this. @@ +2573,5 @@ > + return SECFailure; > + } > + > + if (ss->signedCertTimestamps[kea].data) { > + SECITEM_FreeItem(&ss->signedCertTimestamps[kea], PR_FALSE); Both forms are common in NSS. (You can search for PORT_Free. There are a lot of null pointer checks before PORT_Free calls.)
Comment on attachment 8697055 [details] [diff] [review] Fix nits in conditional expression formatting and error messages Review of attachment 8697055 [details] [diff] [review]: ----------------------------------------------------------------- LGTM
Attachment #8697055 - Flags: review?(ekr) → review+
Comment on attachment 8697055 [details] [diff] [review] Fix nits in conditional expression formatting and error messages https://hg.mozilla.org/projects/nss/rev/d2f278bf0c8b
Attachment #8697055 - Flags: checked-in+
This is the first set of the base files for CT implementation in Firefox. This commit only contains the base CT definitions and parsing of TLS data, including unit tests. Validation code and similar will follow later. Since the code set is not complete, I think it's best at this stage to only focus on the general issues such as code conventions, base classes chosen, error handling, and so on. Review commit: https://reviewboard.mozilla.org/r/31809/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/31809/
Attachment #8710621 - Flags: review?(dkeeler)
Comment on attachment 8710621 [details] MozReview Request: Bug 944175 - preliminary review for a subset of Certificate Transparency auxiliary classes; r?keeler https://reviewboard.mozilla.org/r/31809/#review28777 In general, the code style looks good. Our guidelines are here for reference: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style One issue to address is the placement of return types and braces for function implementations - see the second to last comment here in particular. The error checking/handling looks good. The names of classes seems fine. ::: security/certverifier/CTSerialization.h:18 (Diff revision 1) > +pkix::Result EncodeDigitallySigned( Generally we format long lines like so: pkix::Result EncodeDigitallySigned(const DigitallySigned& data, Buffer& output); ::: security/certverifier/CTSerialization.h:38 (Diff revision 1) > + pkix::Input serialized_log_entry, Generally we use camelCase instead of underscores. ::: security/certverifier/CTSerialization.h:64 (Diff revision 1) > +pkix::Result EncodeSCTListForTesting(pkix::Input sct, Buffer& output); Let's keep testing code separate from implementation code if possible. ::: security/certverifier/CTSerialization.cpp:39 (Diff revision 1) > +// Length of sha256_root_hash buffer of SignedTreeHead Watch out for trailing whitespace like at the end of this line. ::: security/certverifier/CTSerialization.cpp:47 (Diff revision 1) > +Result BufferToInput(const Buffer& buffer, Input& input) { Generally we would write this like so: Result BufferToInput(const Buffer& buffer, Input& input) { return input.Init(buffer.begin(), buffer.length()); } ::: security/certverifier/CTSerialization.cpp:342 (Diff revision 1) > + Result rv; Generally we declare and use variables like this at the same time (as in EncodeLogEntry, for example).
Attachment #8710621 - Flags: review?(dkeeler)
Blocks: 1281469
If I'm not mistaken, this bug is concerned with implementing some level of CT support in NSS. It looks like this is done and committed. FWIW, CT support in PSM/Firefox is being tracked via Bug 1281469.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
See Also: → 1281469
Attachment #8710621 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: