The condition in https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/SSLServerCertVerification.cpp#960 is never false. This code was originally in http://hg.mozilla.org/mozilla-central/annotate/752810cce461/security/manager/ssl/src/nsNSSCallbacks.cpp but it was moved by: http://hg.mozilla.org/mozilla-central/rev/8576199c846c The code was originally intended to fix https://bugzilla.mozilla.org/show_bug.cgi?id=445871 Since I do not understand completeley the repercussions of this regression I am marking this bug as private.

(In reply to Camilo Viecco (:cviecco) from comment #0) > The condition in > > https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/ > SSLServerCertVerification.cpp#960 = http://hg.mozilla.org/mozilla-central/annotate/dd68409d7810/security/manager/ssl/src/SSLServerCertVerification.cpp#l960

Hmm.. not sure why needinfo dropped. Original has been introduced here: Diff: http://hg.mozilla.org/mozilla-central/diff/752810cce461/security/manager/ssl/src/nsNSSCallbacks.cpp File: http://hg.mozilla.org/mozilla-central/file/752810cce461/security/manager/ssl/src/nsNSSCallbacks.cpp#l1036 Modified here (bug 674147): Diff: http://hg.mozilla.org/mozilla-central/diff/8576199c846c/security/manager/ssl/src/SSLServerCertVerification.cpp File: http://hg.mozilla.org/mozilla-central/file/8576199c846c/security/manager/ssl/src/SSLServerCertVerification.cpp#l503 This whole block ... 498 if (!status) { 499 status = new nsSSLStatus(); 500 mSocketInfo->SetSSLStatus(status); 501 } 502 503 if (rv == SECSuccess) { 504 // Certificate verification succeeded delete any potential record 505 // of certificate error bits. 506 nsSSLIOLayerHelpers::mHostsWithCertErrors->RememberCertHasError( 507 mSocketInfo, nsnull, rv); 508 } 509 else { 510 // Certificate verification failed, update the status' bits. 511 nsSSLIOLayerHelpers::mHostsWithCertErrors->LookupCertErrorBits( 512 mSocketInfo, status); 513 } 514 515 if (status && !status->mServerCert) { 516 status->mServerCert = nsc; 517 PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, 518 ("AuthCertificate setting NEW cert %p\n", status->mServerCert.get())); 519 } ... should be moved out of (bellow) the ... 450 if (rv == SECSuccess) { ... 520 } ... block. Brian, does that seem ok to you?

BTW, I think there is a codepath where certList may not be freed. Thus I would move out also CERT_DestroyCertList(certList); snippet as well.

I already have a patch for this: https://hg.mozilla.org/users/bsmith_mozilla.com/standalone-cert-validation/rev/f1378f6dbec6

Cool. Brian can you sec-rate this one? (A best guess is fine)

AFAICT, if there is any problem at all, it only occurs when the user has added a cert error override for the cert anyway, so AFAICT this doesn't make the issue any worse.

This patch makes the unreachable code reachable, so RememberCertErrorsTable is updated even if cert verification fails. I also refactored slightly to clarify the flow of extracting the certificate, creating an nsSSLStatus if necessary, and setting fields on the nsSSLStatus. The main side effect of this change is that we now create an nsSSLStatus even if cert verification fails. This causes the following test failure: 0:05.04 TEST-PASS | /home/garrett/mozilla-central/testing/xpcshell/head.js | [do_check_eq : 762] 2153389990 == 2153389990 0:05.04 PR_Recv failed: SSL_ERROR_CERTIFICATE_UNKNOWN_ALERT 0:05.04 TEST-UNEXPECTED-FAIL | /home/garrett/mozilla-central/testing/xpcshell/head.js | [object XPCWrappedNative_NoHelper] == null - See following stack: 0:05.04 JS frame :: /home/garrett/mozilla-central/testing/xpcshell/head.js :: do_check_eq :: line 762 0:05.04 JS frame :: /home/garrett/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/security/manager/ssl/tests/unit/test_cert_overrides.js :: add_non_overridable_test/< :: line 45 0:05.04 JS frame :: /home/garrett/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/security/manager/ssl/tests/unit/head_psm.js :: add_connection_test/</< :: line 302 0:05.04 JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: Handler.prototype.process :: line 863 0:05.04 JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.walkerLoop :: line 742 0:05.04 JS frame :: /home/garrett/mozilla-central/testing/xpcshell/head.js :: _do_main :: line 191 0:05.04 JS frame :: /home/garrett/mozilla-central/testing/xpcshell/head.js :: _execute_test :: line 405 0:05.04 JS frame :: -e :: <TOP_LEVEL> :: line 1 0:05.04 native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0 0:05.04 TEST-INFO | (xpcshell/head.js) | exiting test However, I'm not sure if this is a legitimate failure, given bug 754369 (mentioned in the test on the line that's failing, which checks for the existence of nsISSLStatus). Perhaps this patch also fixes bug 754369, in which case the test needs to be updated.

(In reply to Garrett Robinson [:grobinson] from comment #7) > The main side effect of this change is that we now create an nsSSLStatus > even if cert verification fails. This causes the following test failure: The main concern is that there may be some code that assumes that the presence of an SSLStatus implies that certificate verification succeeded. Perhaps the safest way to go would be to create a new property on TransportSecurityInfo called BadSSLStatus, and set SSLStatus only if certificate validation succeeded (including cert error overrides) and set BadSSLStatus only if certificate validation failed. That would reduce risk. Otherwise, the author of the patch and the reviewer should carefully analyze all the uses of SSLStatus. Note in particular that there is no way, given an SSLStatus, to know whether certificate validation succeeded due to cert error override, or failed.

Comment on attachment 8446207 [details] [diff] [review] 807771_unreachable_code_in_auth_certificate.patch Review of attachment 8446207 [details] [diff] [review]: ----------------------------------------------------------------- I wasn't entirely joking when I said you should see if you can just remove RememberCertErrorsTable. As far as I can tell, it was added in bug 445871 to ensure that if a user added an exception for a certificate claiming to be EV, it would never show up as EV. However, the implementation is similar to the RecentBadCertsService and has some similar issues. In any case, I don't think we need it, because of the call to SetStatusErrorBits in CreateCertErrorRunnable: https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/SSLServerCertVerification.cpp#586 (which, interestingly enough, creates an nsSSLStatus on the TransportSecurityInfo object if one doesn't exist yet). As far as changing whether or not an nsSSLStatus gets created in AuthCertificate, I would be wary of that, like Brian says.

(In reply to David Keeler (:keeler) [use needinfo?] from comment #9) > In any case, I don't think we need it, because of the call to SetStatusErrorBits in > CreateCertErrorRunnable: > https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/ > SSLServerCertVerification.cpp#586 (which, interestingly enough, creates an > nsSSLStatus on the TransportSecurityInfo object if one doesn't exist yet). > As far as changing whether or not an nsSSLStatus gets created in > AuthCertificate, I would be wary of that, like Brian says. I remember now: Notice that SSLStatus is set when certificate verification succeeds w/o any error, or when certificate verification succeeds due to overridable error, but it isn't ever set (IIRC) when a non-overridable error occurs. IIRC, there is some code somewhere that works like this: If we don't have an SSLStatus then assume that something (e.g. certificate verification) failed and the load was blocked. Else, if we have an SSLStatus and any of the three override bits are set, then that means an overridable cert error occurred. Else, there must not have been any errors! Everything is A-OK, I think, for example, that the much-loved nsSecureBrowserUIImpl and/or the site identity block and/or larry drop-down may work that way.

Comment on attachment 8446207 [details] [diff] [review] 807771_unreachable_code_in_auth_certificate.patch Review of attachment 8446207 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/src/SSLServerCertVerification.cpp @@ +788,5 @@ > + > + if (!status->mServerCert) { > + status->mServerCert = nsc; > + PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, > + ("AuthCertificate setting NEW cert %p\n", status->mServerCert.get())); by the logic change you are now setting SSLstatus even in the case of failure, and I am very certain this indicates to some code success. Also, here you might be dererecing a null pointer.

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #10) > I remember now: Notice that SSLStatus is set when certificate verification > succeeds w/o any error, or when certificate verification succeeds due to > overridable error, but it isn't ever set (IIRC) when a non-overridable error > occurs. Ah, then the approach of stashing the peer certificate chain on the SSLStatus for error reporting is fundamentally flawed. Perhaps TransportSecurityInfo is a better place?

(In reply to Garrett Robinson [:grobinson] from comment #12) > (In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) > from comment #10) > > I remember now: Notice that SSLStatus is set when certificate verification > > succeeds w/o any error, or when certificate verification succeeds due to > > overridable error, but it isn't ever set (IIRC) when a non-overridable error > > occurs. > > Ah, then the approach of stashing the peer certificate chain on the > SSLStatus for error reporting is fundamentally flawed. Perhaps > TransportSecurityInfo is a better place? It isn't fundamentally flawed, it just means that more changes are needed. More changes are going to be needed regardless. Here is an example of some code that is going to have trouble if you don't make more changes: http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp?rev=37dc4a16c312#1152 In general, you'd need to audit the code for uses of "IsDomainMismatch" and friends (case insensitive because there are JS users) and SSLStatus. It may be OK to put the information directly into nsITransportSecurityInfo, especially if David and others thing it is a good idea to eventually remove nsISSLStatus and nsISSLStatusProvider in favor of putting everything into nsITransportSecurityInfo. Note: You also need to consider what to do when you serialize and deserialize the nsITransportSecurityInfo from the HTTP cache and across the e10s boundary. See TransportSecurityInfo::Read and TransportSecurityInfo::Write. In particular, since you are working on this for the SSL pinning violation reporting mechanism, you need to decide what you are going to do about loads from the cache that result in pinning violations, and makes sure you change TransportSecurityInfo::Read and TransportSecurityInfo::Write and/or nsSSLStatus::Read and nsSSLStatus::Write to accommodate that.

sec-low + annoyingly opaque bugmail -> not hidden.

(In reply to David Keeler (:keeler) [use needinfo?] from comment #9) > Comment on attachment 8446207 [details] [diff] [review] > 807771_unreachable_code_in_auth_certificate.patch > > Review of attachment 8446207 [details] [diff] [review]: > ----------------------------------------------------------------- > > I wasn't entirely joking when I said you should see if you can just remove > RememberCertErrorsTable. As far as I can tell, it was added in bug 445871 to > ensure that if a user added an exception for a certificate claiming to be > EV, it would never show up as EV. RememberCertErrorsTable exists because AuthCertificateHook is not called for TLS connections that are resuming a previous session. Without RememberCertErrorsTable, the UI would be able to discern from the nsITransportSecurityInfo/nsISSLStatus whether there was a cert error on resumed-session connections, so we would display the lock icon instead of the warning triangle. However, I agree that the way this code is calling RememberCertErrorsTable::GetInstance().LookupCertErrorBits doesn't seem to make any sense. RememberCertErrorsTable::GetInstance().LookupCertErrorBits should be called in HandshakeCallback for the session resumption case (only) in nsNSSCallbacks.cpp instead. In general RememberCertErrorsTable and friends are totally bogus and it would be great for them to be eventually removed. A while back I filed bug 695511 with the intent to fix this by adding an option to libssl to call AuthCertificateHook during session resumption too, but I never had time to implement it.

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #15) > RememberCertErrorsTable exists because AuthCertificateHook is not called for > TLS connections that are resuming a previous session. Without > RememberCertErrorsTable, the UI would be able to discern from the > nsITransportSecurityInfo/nsISSLStatus whether there was a cert error on > resumed-session connections, so we would display the lock icon instead of > the warning triangle. I'm confused. The only warning triangle I know of is for mixed content, which is tracked by nsSecureBrowserUIImpl, not the CertErrorsTable.

I'm assuming Garrett isn't actively working on this. It still would be nice to clean up, though.

This was fixed in bug 1313491.