764496 - Make EV detection work in content processes

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Justin Lebar (not reading bugmail)]

(Please feel free to make the bug summary more correct.)

As I understand the problem: SSL status is serialized down to the content process, but EV status is not.  Thus bug 763694 hits an assertion when it tries to read EV status from a content process (bug 763694 comment 3).

If instead the EV status were passed down to the content process, this assertion would go away and, I think, <iframe mozbrowser> would be able to detect EV certs.

Comment 1

[Honza Bambas (:mayhemer)]

You can inspire how mobile is doing this.  I believe it works well in Fennec.

Comment 2

[Justin Lebar (not reading bugmail)]

Right, bsmith and I talked about this and read through how it works in XUL Fennec.

It's pretty ugly.  The state is sent down to the content process, then the content process re-serializes it and sends it up to the chrome process, which reads the EV state.

Fennec-specific code has to do this dance, which is why it doesn't work in B2G.

The idea is to make this work transparently for the consumer (the nsIWebProgressListener in the child process).

Comment 3

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

Honza, if we separate the serialization logic used for the cache from the serialization logic used for e10s, then for the e10s stuff we can just serialize/deserialize the isExtendedValidation bit.

I want to separate the serialization into cache and non-cache cases anyway to help with the issue in bug 697781. It turns out that the approach of lazily calculating the data in that bug won't work well with these e10s cases.

Comment 4

[Justin Lebar (not reading bugmail)]

We should make a blocking decision here.  Are we OK shipping without EV?  If not, we need to get a move on this one.

Comment 5

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

IMO, we shouldn't block on whether EV certs have a blue lock instead of a green lock icon in the address bar, which AFAICT is the only difference the user would be able to perceive.

Comment 6

[Andrew Overholt [:overholt]]

Based on Brian's comment and other discussion in triage, we're not going to block on this.

Comment 7

[Tom S [:evilpie]]

Attachment: wip

Apparently this bug also affects firefox.html. My idea is to always set the EV flag immediately after assigning the server-cert, which should presumably only happen in the parent.

Comment 8

[Tom S [:evilpie]]

Attachment: v1 - immediately check and serialize EV status

Comment 9

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

Comment on attachment 8535629 [details] [diff] [review]
v1 - immediately check and serialize EV status

Review of attachment 8535629 [details] [diff] [review]:
-----------------------------------------------------------------

This should work for our immediate purposes. Long term, we want to re-do the communication of this kind of security information between parent/child, but that's a much larger project. I am a bit concerned about how this will interact with the cache and showing the EV indicator, but after testing out the patch, it seemed to work (and, again, it should do the right thing, since we're saving all the information we need...). The other issue is in the past it seems two threads would race on determining if a certificate was EV, and with this change, if the wrong thread wins, we might not show the indicator when we should. We'll just have to keep an eye on that.
I've asked for a few additional features on this, so I should probably have another look. r- for now. Thanks for working on this!

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +436,5 @@
>    }
>  
>    // Before checking the server certificate we need to make sure the
>    // handshake has completed.
> +  if (!mHandshakeCompleted || !SSLStatus() || !SSLStatus()->HasServerCert())

nit: as long as we're touching this line, let's add the {} around the conditional body

@@ +457,5 @@
>  
>    ScopedCERTCertificate nssCert;
>  
> +  nsCOMPtr<nsIX509Cert> cert;
> +  (void) SSLStatus()->GetServerCert(getter_AddRefs(cert));

Check the return value here (i.e. if (NS_FAILED(rv) {\n return rv;\n})

::: security/manager/ssl/src/nsSSLStatus.cpp
@@ +286,5 @@
>  nsSSLStatus::~nsSSLStatus()
>  {
>  }
> +
> +void

This should probably return an nsresult so we know when/if it fails.

@@ +287,5 @@
>  {
>  }
> +
> +void
> +nsSSLStatus::SetServerCert(nsIX509Cert* aServerCert)

This should also take a parameter like the enum { ev_status_invalid = 0, ev_status_valid = 1, ev_status_unknown = 2 } in nsNSSCertificate.h, because in most cases when we're setting the server cert, we already know its EV status (in AuthCertificate, we know if it verified successfully or not and if it's EV, in TransportSecurityInfo::SetStatusErrorBits, we know it's not EV (since there were verification errors), etc.). That way, we don't do more work than we have to (and certificate path building is a bit expensive).

@@ +293,5 @@
> +  mServerCert = aServerCert;
> +
> +#ifndef MOZ_NO_EV_CERTS
> +  nsCOMPtr<nsIIdentityInfo> idinfo = do_QueryInterface(mServerCert);
> +  nsresult rv = idinfo->GetIsExtendedValidation(&mIsExtendedValidation);

null-check idinfo before using it (basically, use the NS_ERROR that was removed from GetIsExtendedValidation)

@@ +294,5 @@
> +
> +#ifndef MOZ_NO_EV_CERTS
> +  nsCOMPtr<nsIIdentityInfo> idinfo = do_QueryInterface(mServerCert);
> +  nsresult rv = idinfo->GetIsExtendedValidation(&mIsExtendedValidation);
> +  if (NS_WARN_IF(NS_FAILED(rv)))

nit: braces around conditional body (also, we probably don't need the NS_WARN_IF)

::: security/manager/ssl/src/nsSSLStatus.h
@@ +29,5 @@
>  
>    nsSSLStatus();
>  
> +  void
> +  SetServerCert(nsIX509Cert* aServerCert);

nit: return value goes on the same line in header files

@@ +32,5 @@
> +  void
> +  SetServerCert(nsIX509Cert* aServerCert);
> +
> +  bool
> +  HasServerCert() {

nit: return value goes on the same line, but the '{' goes on the next line

@@ +58,5 @@
>  };
>  
>  #define NS_SSLSTATUS_CID \
> +{ 0xe2f14826, 0x9e70, 0x4647, \
> +  {0xb2, 0x3f, 0x10, 0x10, 0xf5, 0x12, 0x46, 0x28 } }

nit: seems like we should have a space after the '{' to be consistent (or is it the original line that is wrong?)

::: toolkit/modules/RemoteWebProgress.jsm
@@ -105,5 @@
>      }
>  
> -    // We must check the Extended Validation (EV) state here, on the chrome
> -    // process, because NSS is needed for that determination.
> -    if (deserialized && deserialized.isExtendedValidation)

Why should this be removed?

Comment 10

[Tom S [:evilpie]]

Comment on attachment 8535629 [details] [diff] [review]
v1 - immediately check and serialize EV status

Review of attachment 8535629 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/src/nsSSLStatus.cpp
@@ +287,5 @@
>  {
>  }
> +
> +void
> +nsSSLStatus::SetServerCert(nsIX509Cert* aServerCert)

Good idea!

@@ +293,5 @@
> +  mServerCert = aServerCert;
> +
> +#ifndef MOZ_NO_EV_CERTS
> +  nsCOMPtr<nsIIdentityInfo> idinfo = do_QueryInterface(mServerCert);
> +  nsresult rv = idinfo->GetIsExtendedValidation(&mIsExtendedValidation);

" and mServerCert will implement nsIIdentityInfo in the chrome process." So I assumed this was unnecessary, because this function should only be called in the chrome process.

::: toolkit/modules/RemoteWebProgress.jsm
@@ -105,5 @@
>      }
>  
> -    // We must check the Extended Validation (EV) state here, on the chrome
> -    // process, because NSS is needed for that determination.
> -    if (deserialized && deserialized.isExtendedValidation)

Well this is exactly the case we are fixing, right? We would send a cert to the content-process, but that process can't query the EV bit, so STATE_IDENTITY_EV_TOPLEVEL isn't set.

Comment 11

[Tom S [:evilpie]]

Mhm It's not really clear to me how to actually obtain the ev-status in those functions. Actually having a return type for SetServerCert is rather pointless as well, most of the callers are not prepared to handle failures.

Comment 12

[Tom S [:evilpie]]

Attachment: v2 - immediately check and serialize EV status

Comment 13

[Tom S [:evilpie]]

Attachment: v2 - immediately check and serialize EV status

Comment 15

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

Comment on attachment 8539615 [details] [diff] [review]
v2 - immediately check and serialize EV status

Review of attachment 8539615 [details] [diff] [review]:
-----------------------------------------------------------------

Great! r=me with comments addressed. You should also get review from a dom/toolkit peer.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +1142,5 @@
>      }
>  
> +    if (status && !status->HasServerCert()) {
> +      nsNSSCertificate::EVStatus evStatus;
> +      // XX should we do this rv != SECSuccess?

This code is correct as-is; if verification failed, evStatus should be ev_status_invalid.

::: security/manager/ssl/src/nsNSSCallbacks.cpp
@@ +1287,5 @@
>  
>    if (equals_previous) {
>      PR_LOG(gPIPNSSLog, PR_LOG_DEBUG,
>              ("HandshakeCallback using PREV cert %p\n", prevcert.get()));
> +    status->SetServerCert(prevcert, nsNSSCertificate::ev_status_unknown);

We can get the cached EV status from prevcert, right? (it isn't exposed currently, but I would add 'GetCachedEVStatus' or something to nsNSSCertificate and do prevcert->GetCert()->GetCachedEVStatus())

::: security/manager/ssl/src/nsSSLStatus.cpp
@@ +274,5 @@
>  , mIsDomainMismatch(false)
>  , mIsNotValidAtThisTime(false)
>  , mIsUntrusted(false)
> +, mIsExtendedValidation(false)
> +, mHasExtendedValidation(false)

I was thinking 'mHasExtendedValidation' might be a bit confusing - should we call this 'mExtendedValidationStatusKnown'? (And, honestly, we can probably shorten these to 'mIsEV' and 'mEVStatusKnown' or whatever.)

Comment 16

[Tom S [:evilpie]]

prevcert->GetCert() gives me some CERTCertificate class. I probably need to QI prevcert tons NSSCertificate or something.

Comment 17

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

Oh, right. Hmmm - iirc, it's not a great idea to go from an XPCOM object to what the code thinks is its underlying implementation (i.e. nsIX509Cert -> nsNSSCertificate), because it might be wrong (unless there's a safe way of doing that that I don't know about?)
In any case, I think the whole "try to use the previous cert" business is a bit dubious in its implementation and perhaps even motivation. If we implement something like a certificate verification cache, it won't be necessary. In short, we can probably just use "ev_status_unknown" in that case for now.

Comment 18

[Tom S [:evilpie]]

Okay. I just pushed the patch to try, hopefully it doesn't break b2g. If not I am going to ask kanru, who owns the browser webapi, for review.

Comment 19

[Tom S [:evilpie]]

I don't really test B2G, but I would curious to know if it actually works on that platform. I mainly wrote this for firefox.html, where EV status is now showing up correctly.

Comment 20

[Tom S [:evilpie]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=40a77ad46a93
https://hg.mozilla.org/integration/mozilla-inbound/rev/876568e6c5cb

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/876568e6c5cb