807711 - unreachable code in SSLServerCertVerification -> AuthCertificate

Status: RESOLVED DUPLICATE of bug 1313491

(Core :: Security: PSM, defect)

Description

[Camilo Viecco (:cviecco)]

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.

Comment 1

[Honza Bambas (:mayhemer)]

(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

Comment 2

[Honza Bambas (:mayhemer)]

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?

Comment 3

[Honza Bambas (:mayhemer)]

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.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I already have a patch for this:

https://hg.mozilla.org/users/bsmith_mozilla.com/standalone-cert-validation/rev/f1378f6dbec6

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 7

[Garrett Robinson [:grobinson]]

Attachment: 807771_unreachable_code_in_auth_certificate.patch

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.

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(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 9

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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.

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(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 11

[Camilo Viecco (:cviecco)]

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.

Comment 12

[Garrett Robinson [:grobinson]]

(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?

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(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.

Comment 14

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

Comment 15

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(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.

Comment 16

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(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.

Comment 18

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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

Comment 20

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

This was fixed in bug 1313491.