1313491 - shift-refresh on a site with an EV certificate removes EV status

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

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

STR: visit a site with an EV certificate (e.g. bugzilla.mozilla.org) and then shift-refresh (clearing the cache).

Expected results: the extended green EV bar should still be displayed
Actual results: the EV indicator falls back to the DV indicator

I bisected this to changeset 4adb7daf5033 from bug 1264562.

Comment 1

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

Attachment: bug 1313491 - include more context when determining EV status

When doing TLS session resumption, the AuthCertificate hook is bypassed, which
means that the front-end doesn't know whether or not to show the EV indicator.
To deal with this, the platform attempts an EV verification. Before this patch,
this verification lacked much of the original context (e.g. stapled OCSP
responses, SCTs, the hostname, and in particular the first-party origin key).
Furthermore, it was unclear from a code architecture standpoint that a full
verification was even occurring. This patch brings the necessary context to the
verification and makes it much more clear that it is happening. It also takes
the opportunity to remove some unnecessary EV-related fields and information in
code and data structures that don't require it.

Review commit: https://reviewboard.mozilla.org/r/89868/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/89868/

Comment 2

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

Attachment: bug 1313491 - add basic tests that PSM sets the right security state during session resumption

Review commit: https://reviewboard.mozilla.org/r/89904/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/89904/

Comment 3

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8806508 [details]
bug 1313491 - add basic tests that PSM sets the right security state during session resumption

https://reviewboard.mozilla.org/r/89904/#review90138

::: security/manager/ssl/tests/unit/test_session_resumption.js:49
(Diff revision 1)
> +      ok(!sslStatus.isUntrusted,
> +         "expired.example.com should not have isUntrusted set");
> +    }
> +  );
> +
> +  // This test is similar, except with extended validation.

Could you maybe explain a little better exactly what the EV part of the test achieves? The two add_connection_test calls look basically the same. It's wasn't clear to me on reading this part of the test what was being tested.

Comment 4

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8806507 [details]
bug 1313491 - include more context when determining EV status

https://reviewboard.mozilla.org/r/89868/#review90166

Comment 5

[:Cykesiopka]

Comment on attachment 8806507 [details]
bug 1313491 - include more context when determining EV status

https://reviewboard.mozilla.org/r/89868/#review90404

Cool, much nicer!

::: security/manager/ssl/SSLServerCertVerification.cpp:1375
(Diff revision 1)
> +    RememberCertErrorsTable::GetInstance().RememberCertHasError(infoObject,
> +                                                                nullptr,
> +                                                                SECSuccess);

Follow-up material: Might be nice to create a new method dedicated to clearing cert errors, since IMO it's confusing that method named `RememberCertHasError()` can actually clear an existing error.

We should probably also rename `LookupCertErrorBits()` to `UpdateCertErrorBits()` or something.

::: security/manager/ssl/SSLServerCertVerification.cpp:1385
(Diff revision 1)
>                                             certificateTransparencyInfo);
>  
>      // The connection may get terminated, for example, if the server requires
>      // a client cert. Let's provide a minimal SSLStatus
>      // to the caller that contains at least the cert and its status.
> +    RefPtr<nsSSLStatus> status(infoObject->SSLStatus());

A prexisting issue, but it looks like this file is actually missing a few includes:
```cpp
#include "nsSSLStatus.h"
#include "nsNSSCertificate.h"
#include "mozilla/RefPtr.h"
#include "SharedCertVerifier.h"
#include "TransportSecurityInfo.h" // For RememberCertErrorsTable
```

::: security/manager/ssl/SSLServerCertVerification.cpp:1402
(Diff revision 1)
>        }
>  
> +      RefPtr<nsNSSCertificate> nsc = nsNSSCertificate::Create(cert.get());
>        status->SetServerCert(nsc, evStatus);
>        MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
> -             ("AuthCertificate setting NEW cert %p\n", nsc.get()));
> +              ("AuthCertificate setting NEW cert %p\n", nsc.get()));

Nit: Might as well get rid of the unnecessary \n while we're touching this line.

::: security/manager/ssl/nsNSSCallbacks.cpp:1106
(Diff revision 1)
> +
> +  if (!sslStatus || !fd || !infoObject) {
> +    return;
> +  }
> +
> +  UniqueCERTCertificate cert(SSL_PeerCertificate(fd));

Seems like this deserves an assert and null check.

::: security/manager/ssl/nsNSSCallbacks.cpp:1108
(Diff revision 1)
> +    return;
> +  }
> +
> +  UniqueCERTCertificate cert(SSL_PeerCertificate(fd));
> +
> +  RefPtr<SharedCertVerifier> certVerifier(GetDefaultCertVerifier());

A prexisting issue, but it looks like this file is actually missing a few includes:
```cpp
#include "mozilla/RefPtr.h"
#include "SharedCertVerifier.h"
#include "nsNSSCertificate.h"
```

::: security/manager/ssl/nsNSSCallbacks.cpp:1116
(Diff revision 1)
> +  if (!certVerifier) {
> +    return;
> +  }
> +
> +  // We don't own these pointers.
> +  const SECItemArray* csa = SSL_PeerStapledOCSPResponses(fd);

Nit: I guess this was copied from from AuthCertificateHook(), but maybe we could name `csa` something else? At least for me it's not a really intuitive name.

::: security/manager/ssl/nsNSSCallbacks.cpp:1156
(Diff revision 1)
> +                              &evOidPolicy);
> +
> +  RefPtr<nsNSSCertificate> nssc(nsNSSCertificate::Create(cert.get()));
> +  if (rv == Success && evOidPolicy != SEC_OID_UNKNOWN) {
> +    MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
> +            ("HandshakeCallback using NEW cert %p (is EV)\n", nssc.get()));

Nit: Get rid of the unnecessary \n. Same thing below.

::: security/manager/ssl/nsNSSCertificate.h:42
(Diff revision 1)
>  
>    friend class nsNSSCertificateFakeTransport;
>  
> -  explicit nsNSSCertificate(CERTCertificate* cert, SECOidTag* evOidPolicy = nullptr);
> +  explicit nsNSSCertificate(CERTCertificate* cert);
>    nsNSSCertificate();
> -  static nsNSSCertificate* Create(CERTCertificate*cert = nullptr,
> +  static nsNSSCertificate* Create(CERTCertificate*cert = nullptr);

Nit: Space after *.

::: security/manager/ssl/nsSSLStatus.cpp:13
(Diff revision 1)
>  #include "nsIClassInfoImpl.h"
> -#include "nsIObjectOutputStream.h"
>  #include "nsIObjectInputStream.h"
> +#include "nsIObjectOutputStream.h"
> +#include "nsNSSCertificate.h"
> +#include "plstr.h"

I think we can get rid of this include now.

Comment 6

[:Cykesiopka]

Comment on attachment 8806508 [details]
bug 1313491 - add basic tests that PSM sets the right security state during session resumption

https://reviewboard.mozilla.org/r/89904/#review90406

LGTM.

::: security/manager/ssl/tests/unit/bad_certs/ev-test-intermediate.pem.certspec:6
(Diff revision 1)
>  issuer:evroot
> -subject:anyPolicy-int-path-int
> +subject:ev-test-intermediate
>  issuerKey:ev
>  extension:basicConstraints:cA,
>  extension:keyUsage:cRLSign,keyCertSign
> -extension:authorityInformationAccess:http://www.example.com:8888/anyPolicy-int-path-int/
> +extension:authorityInformationAccess:http://localhost:8888/ev-test-intermediate/

Hmm, why the www.example.com -> localhost change? Is it to save us having to set the "network.dns.localDomains" pref?

::: security/manager/ssl/tests/unit/test_session_resumption.js:60
(Diff revision 1)
> +  add_connection_test("ev-test.example.com", PRErrorCodeSuccess, null,
> +    (transportSecurityInfo) => {
> +      ok(!(transportSecurityInfo.securityState &
> +           Ci.nsIWebProgressListener.STATE_CERT_USER_OVERRIDDEN),
> +         "ev-test.example.com should not have STATE_CERT_USER_OVERRIDDEN flag");
> +      let sslStatus = transportSecurityInfo
> +                        .QueryInterface(Ci.nsISSLStatusProvider)
> +                        .SSLStatus;
> +      ok(!sslStatus.isDomainMismatch,
> +         "ev-test.example.com should not have isDomainMismatch set");
> +      ok(!sslStatus.isNotValidAtThisTime,
> +         "ev-test.example.com should not have isNotValidAtThisTime set");
> +      ok(!sslStatus.isUntrusted,
> +         "ev-test.example.com should not have isUntrusted set");
> +      ok(!gEVExpected || sslStatus.isExtendedValidation,
> +         "ev-test.example.com should have isExtendedValidation set " +
> +         "(or this is a non-debug build)");
> +    }
> +  );

Nit: It might be clearer to dedupe this block of code and the one above to make it clearer we expect the exact same results.

Comment 7

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8806507 [details]
bug 1313491 - include more context when determining EV status

https://reviewboard.mozilla.org/r/89868/#review90940

This looks all right and proper to me.

Comment 8

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8806508 [details]
bug 1313491 - add basic tests that PSM sets the right security state during session resumption

https://reviewboard.mozilla.org/r/89904/#review91028

::: security/manager/ssl/tests/unit/test_session_resumption.js:49
(Diff revision 1)
> +      ok(!sslStatus.isUntrusted,
> +         "expired.example.com should not have isUntrusted set");
> +    }
> +  );
> +
> +  // This test is similar, except with extended validation.

Maybe it'd be clearer to split `run_test()` into two tests: `run_resume_non_ev_with_override()` and `run_resume_ev()` ?

Comment 9

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

Comment on attachment 8806507 [details]
bug 1313491 - include more context when determining EV status

https://reviewboard.mozilla.org/r/89868/#review90404

Thanks for the reviews, all!

> Follow-up material: Might be nice to create a new method dedicated to clearing cert errors, since IMO it's confusing that method named `RememberCertHasError()` can actually clear an existing error.
> 
> We should probably also rename `LookupCertErrorBits()` to `UpdateCertErrorBits()` or something.

Sure - filed bug 1316070.

> A prexisting issue, but it looks like this file is actually missing a few includes:
> ```cpp
> #include "nsSSLStatus.h"
> #include "nsNSSCertificate.h"
> #include "mozilla/RefPtr.h"
> #include "SharedCertVerifier.h"
> #include "TransportSecurityInfo.h" // For RememberCertErrorsTable
> ```

Ok - added.

> Nit: Might as well get rid of the unnecessary \n while we're touching this line.

Sounds good.

> Seems like this deserves an assert and null check.

Good call.

> A prexisting issue, but it looks like this file is actually missing a few includes:
> ```cpp
> #include "mozilla/RefPtr.h"
> #include "SharedCertVerifier.h"
> #include "nsNSSCertificate.h"
> ```

Ok - added (also sorted).

> Nit: I guess this was copied from from AuthCertificateHook(), but maybe we could name `csa` something else? At least for me it's not a really intuitive name.

I can't remember why I called it that, but yeah, it's not a great name.

> Nit: Get rid of the unnecessary \n. Same thing below.

Sounds good.

> Nit: Space after *.

Fixed.

> I think we can get rid of this include now.

Sounds good.

Comment 10

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

Comment on attachment 8806508 [details]
bug 1313491 - add basic tests that PSM sets the right security state during session resumption

https://reviewboard.mozilla.org/r/89904/#review90138

> Could you maybe explain a little better exactly what the EV part of the test achieves? The two add_connection_test calls look basically the same. It's wasn't clear to me on reading this part of the test what was being tested.

I added more documentation in addition to refactoring the code a bit.

Comment 11

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

Comment on attachment 8806508 [details]
bug 1313491 - add basic tests that PSM sets the right security state during session resumption

https://reviewboard.mozilla.org/r/89904/#review90406

> Hmm, why the www.example.com -> localhost change? Is it to save us having to set the "network.dns.localDomains" pref?

Yes - unfortunately add_connection_test *sets* "network.dns.localDomains" rather than appending to it. That needs fixing, but I was concerned about making too many changes with this patch, so I'd rather address that as a follow-up. I filed bug 1316076 for this.

> Nit: It might be clearer to dedupe this block of code and the one above to make it clearer we expect the exact same results.

Good call.

Comment 12

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

Comment on attachment 8806507 [details]
bug 1313491 - include more context when determining EV status

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89868/diff/1-2/

Comment 13

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

Comment on attachment 8806508 [details]
bug 1313491 - add basic tests that PSM sets the right security state during session resumption

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89904/diff/1-2/

Comment 14

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=372352e5ab0a

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1c76db819ee6
include more context when determining EV status r=Cykesiopka,jcj,mgoodwin
https://hg.mozilla.org/integration/autoland/rev/bd9fb3865606
add basic tests that PSM sets the right security state during session resumption r=Cykesiopka,jcj,mgoodwin

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1c76db819ee6
https://hg.mozilla.org/mozilla-central/rev/bd9fb3865606