Make EV detection work in content processes

RESOLVED FIXED in mozilla37

Status

()

Core
Security: PSM
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: evilpie)

Tracking

(Blocks: 1 bug)

Trunk
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:-)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
(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.
You can inspire how mobile is doing this.  I believe it works well in Fennec.
(Reporter)

Comment 2

5 years ago
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).
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.
Summary: Make EV detection work in content processes → Make EV detection work in content processes in B2G
(Reporter)

Updated

5 years ago
Blocks: 763694
(Reporter)

Comment 4

5 years ago
We should make a blocking decision here.  Are we OK shipping without EV?  If not, we need to get a move on this one.
blocking-basecamp: --- → ?
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.
Based on Brian's comment and other discussion in triage, we're not going to block on this.
blocking-basecamp: ? → -
Created attachment 8534607 [details] [diff] [review]
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.
Assignee: nobody → evilpies
Created attachment 8535629 [details] [diff] [review]
v1 - immediately check and serialize EV status
Attachment #8534607 - Attachment is obsolete: true
Attachment #8535629 - Flags: review?(dkeeler)
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?
Attachment #8535629 - Flags: review?(dkeeler) → review-
Blocks: 1110690
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.
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.
Created attachment 8539614 [details] [diff] [review]
v2 - immediately check and serialize EV status
Attachment #8535629 - Attachment is obsolete: true
Attachment #8539614 - Flags: review?(dkeeler)
Created attachment 8539615 [details] [diff] [review]
v2 - immediately check and serialize EV status
Attachment #8539614 - Attachment is obsolete: true
Attachment #8539614 - Flags: review?(dkeeler)
Attachment #8539615 - Flags: review?(dkeeler)
Attachment #8539615 - Attachment is patch: true

Updated

3 years ago
Duplicate of this bug: 1114316
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.)
Attachment #8539615 - Flags: review?(dkeeler) → review+
prevcert->GetCert() gives me some CERTCertificate class. I probably need to QI prevcert tons NSSCertificate or something.
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.
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.
Attachment #8539615 - Flags: review?(kchen)
Attachment #8539615 - Flags: review?(kchen) → review+
Summary: Make EV detection work in content processes in B2G → Make EV detection work in content processes
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.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=40a77ad46a93
https://hg.mozilla.org/integration/mozilla-inbound/rev/876568e6c5cb
https://hg.mozilla.org/mozilla-central/rev/876568e6c5cb
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Duplicate of this bug: 820466
You need to log in before you can comment on or make changes to this bug.