Last Comment Bug 686248 - SSL certificate EV status not calculated correctly in e10s systems
: SSL certificate EV status not calculated correctly in e10s systems
Status: RESOLVED FIXED
[sg:moderate?] [e10s]
:
Product: Core
Classification: Components
Component: Security: UI (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla10
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on: 698984 698983 820466
Blocks: 697781 575950 674147
  Show dependency treegraph
 
Reported: 2011-09-11 18:05 PDT by Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
Modified: 2013-08-19 23:57 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
Add isExtendedValidation to nsISSLStatus so it can be used by mobile (29.65 KB, patch)
2011-10-25 17:17 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
honzab.moz: review+
kaie: superreview+
Details | Diff | Review

Description Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-09-11 18:05:08 PDT
+++ 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) 2011-09-13 12:52:28 PDT
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?) 2011-09-18 23:10:08 PDT
(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 [:mwargers] (gone per 2016-05-31 :-( ) 2011-09-19 09:45:16 PDT
Can this problem be seen on certain https:// sites?
Comment 4 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-07 11:56:22 PDT
> 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?) 2011-10-25 17:17:39 PDT
Created attachment 569557 [details] [diff] [review]
Add isExtendedValidation to nsISSLStatus so it can be used by mobile
Comment 6 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-25 19:07:11 PDT
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) 2011-10-27 11:09:32 PDT
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?) 2011-10-27 11:31:24 PDT
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?) 2011-10-27 11:33:36 PDT
P1 because it is blocking bug 674147 which is blocking SPDY.
Comment 10 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-27 12:04:28 PDT
(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?) 2011-10-27 12:07:15 PDT
(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?) 2011-10-30 17:45:38 PDT
https://hg.mozilla.org/mozilla-central/rev/b38db925f437

Note You need to log in before you can comment on or make changes to this bug.