686248 - SSL certificate EV status not calculated correctly in e10s systems

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect, P1)

Description

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

+++ This bug was initially created as a clone of Bug #575950 +++

The check for extended validation status of a connection in Fennec does not match the check for extended validation status for a connection in the desktop version, because Fennec uses nsNSSCertificate::GetIsExtendedValidation() instead of nsNSSSocketInfo::GetIsExtendedValidation(). The nsNSSCertificate version is missing some extra checks that are done in the nsNSSSocketInfo version. In particular, I suspect (but haven't verified) that these checks would cause Mobile Firefox to report a connection is EV incorrectly in circumstances where the desktop version would report an invalid SSL status.

I propose that we move nsNSSSocketInfo::GetIsExtendedValidation() to nsSSLStatus, making isExtendedValidation a member of nsISSLStatus.

Comment 1

[Honza Bambas (:mayhemer)]

Confirming the missing check and agree on the change, but you must confirm before that, we always have status where we now have nsIIdentityInfo.  nsSSLStatus has all the info needed to implement all methods of that interface.

Then we can change [1] to just |if (SSLStatus.isExtendedValidation)|

[1] http://hg.mozilla.org/mozilla-central/annotate/c9479e3f6c54/mobile/chrome/content/bindings/browser.xml#l244

Comment 2

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

(In reply to Honza Bambas (:mayhemer) from comment #1)
> Confirming the missing check and agree on the change, but you must confirm
> before that, we always have status where we now have nsIIdentityInfo. 
> nsSSLStatus has all the info needed to implement all methods of that
> interface.

We must assume non-EV if we don't have a status.

From looking at nsNSSCallbacks (the auth certificate callback and the handshake callback), it seems like we always set the status whenever we set the certificate. The code that manages the identity info and the SSL status is very unclear in general. Also, the serialization/deserialization of nsNSSSocketInfo and nsSSLStatus as used by the cache is done poorly (e.g. two copies of the cert are written to the cache, we cache the status of certificate validation when really we must re-validate the cert upon reading the entry from the cache, amongst other problems).

Also, the data we need to (de)serialize for e10s is different than the data we need to serialize for the cache, and that is different than "the entire object" which is (mostly) what we serialize now. So, we should avoid using nsISerializable for these things; instead we should have separate explicit ways of serializing nsSSLStatus for each usage. I have a rough idea of how all this code needs to be changed but I don't have time to implement all the changes right now.

Comment 3

[Martijn Wargers (dead)]

Can this problem be seen on certain https:// sites?

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

> I propose that we move nsNSSSocketInfo::GetIsExtendedValidation() to
> nsSSLStatus, making isExtendedValidation a member of nsISSLStatus.

I assume we need to wait for this to happen before moving forward.

Comment 5

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

Attachment: Add isExtendedValidation to nsISSLStatus so it can be used by mobile

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

I think this is more accurately an e10s bug, so I am updating the summary. I only mention this because Mobile is switching back to non-e10s in the Fennec Native (Android) releases. Those should start on phones for Fx11.

Desktop Firefox will likely need this when it moves to e10s too.

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 569557 [details] [diff] [review]
Add isExtendedValidation to nsISSLStatus so it can be used by mobile

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

r=honzab with the comments.

Needs sr from module owner or some s-reviewer familiar with usage of nsISSLStatusProvider.

::: security/manager/boot/public/nsISSLStatusProvider.idl
@@ +44,2 @@
>  interface nsISSLStatusProvider : nsISupports {
> +  readonly attribute nsISSLStatus SSLStatus;

Don't you want to do this rather as a separate bug/patch?  I think this needs a sr from Kai.

Personally I really am not against this change.  If you manage getting sr from him quickly then go forward.

::: security/manager/ssl/public/nsISSLStatus.idl
@@ +59,5 @@
>     *       query nsIX509Cert3::isSelfSigned 
>     */
>    readonly attribute boolean isUntrusted;
> +
> +  readonly attribute boolean isExtendedValidation;

You definitely need to add/copy some comment here.

::: security/manager/ssl/src/nsIdentityChecking.cpp
@@ +1093,3 @@
>    *aIsEV = false;
>  
> +  nsCOMPtr<nsIX509Cert> cert = mServerCert;

Why exactly are you doing this?

@@ +1096,4 @@
>  
> +  // Never allow missing or bad certs for EV,
> +  // regardless of overrides.
> +  NS_ENSURE_TRUE(cert && !mHaveCertErrorBits, NS_OK);

Do you really want to log this as an error?  Specially the !mHaveCertErrorBits is something that can happen and is not an error to log this way.  It is IMO a valid state that is indicated to user by broken EV status.  This is something that should more be logged to the error console then to the log/stderr.

Also this makes a developer looking at the code think this is some kind of internally illegal state.  

Better here is to use:
if (!mServerCert) {NS_ERROR("..."); return NS_OK;} 
if (mHaveCertErrorBits) {optional user level report; return NS_OK;}

::: security/manager/ssl/src/nsNSSCertificateFakeTransport.cpp
@@ -57,5 @@
>  /* nsNSSCertificateFakeTransport */
>  
> -NS_IMPL_THREADSAFE_ISUPPORTS5(nsNSSCertificateFakeTransport, nsIX509Cert,
> -                                                nsIX509Cert2,
> -                                                nsIX509Cert3,

I assume you checked there is no code that QI to Cert2/3 that could crash/misbehave when executed on the content process, right?

not sure either of these this could be invoked on the content process:
http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSCertificate.cpp#1670
http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSCertificate.cpp#1698
http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSCertificateDB.cpp#1016
http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSCertificateDB.cpp#1104

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +605,5 @@
>  NS_IMETHODIMP
>  nsNSSSocketInfo::Write(nsIObjectOutputStream* stream) {
>    stream->WriteID(kNSSSocketInfoMagic);
>  
> +  nsRefPtr<nsSSLStatus> status = mSSLStatus;

Why there is need to locally refer mSSLStatus?

@@ +628,5 @@
> +    NS_WARNING("Serializing nsNSSSocketInfo without mSSLStatus");
> +  }
> +
> +  // Store the flag if there is the certificate present
> +  stream->WriteBoolean(certSerializable);

!!certSerializable ?

One day I'd rather break the compatibility and not store the duplicate cert here, i.e. store |false| and be done.

Could you please file a bug on this to discuss if and when to do it?

Comment 8

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

Comment on attachment 569557 [details] [diff] [review]
Add isExtendedValidation to nsISSLStatus so it can be used by mobile

Kai, could you please sr+ the change of the type of nsISSLStatusProvider.SSLStatus from nsISupports to nsISSLStatus?

Comment 9

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

P1 because it is blocking bug 674147 which is blocking SPDY.

Comment 10

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

(In reply to Honza Bambas (:mayhemer) from comment #7)
> >  interface nsISSLStatusProvider : nsISupports {
> > +  readonly attribute nsISSLStatus SSLStatus;
> 
> Don't you want to do this rather as a separate bug/patch?  I think this
> needs a sr from Kai.

If Kai can't sr+ it I will revert it.

> > +  nsCOMPtr<nsIX509Cert> cert = mServerCert;
> 
> Why exactly are you doing this?

> > +  nsRefPtr<nsSSLStatus> status = mSSLStatus;
>
> Why there is need to locally refer mSSLStatus?

I will add a comment. Basically, it is there to protect against any concurrent thread modifying mServerCert or mSSLStatus to point to a different certificate while the current method is executing, without requiring locks.

> I assume you checked there is no code that QI to Cert2/3 that could
> crash/misbehave when executed on the content process, right?

I reviewed that before making the changes and it is safe. The only thing we do with certs in the content process is deserialize and serialize so that we can communicate them across content boundaries. 

> One day I'd rather break the compatibility and not store the duplicate cert
> here, i.e. store |false| and be done.
> 
> Could you please file a bug on this to discuss if and when to do it?

Bug 697781.

I will address the other comments.

Comment 11

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

(In reply to Honza Bambas (:mayhemer) from comment #7)
> > +  // Store the flag if there is the certificate present
> > +  stream->WriteBoolean(certSerializable);
> 
> !!certSerializable ?

We do not need to do this anymore because WriteBoolean take a bool instead of PRBool.

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/b38db925f437