Last Comment Bug 703834 - Factor out TransportSecurityInfo superclass from nsNSSSocketInfo
: Factor out TransportSecurityInfo superclass from nsNSSSocketInfo
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: mozilla15
Assigned To: Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
:
: David Keeler [:keeler] (use needinfo?)
Mentors:
: 733666 (view as bug list)
Depends on: 480745
Blocks: 615342 CVE-2011-0082
  Show dependency treegraph
 
Reported: 2011-11-18 22:56 PST by Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
Modified: 2012-04-30 08:05 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Split nsNSSSocketInfo into TransportSecurityInfo and nsNSSSocketInfo (160.76 KB, patch)
2012-03-26 10:32 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
no flags Details | Diff | Splinter Review
Make SSLServerCertVerification use TransportSecurityInfo instead of nsNSSSocketInfo (24.13 KB, patch)
2012-03-26 10:48 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
no flags Details | Diff | Splinter Review
Split nsNSSSocketInfo into TransportSecurityInfo and nsNSSSocketInfo (160.76 KB, patch)
2012-03-26 16:34 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
no flags Details | Diff | Splinter Review
Make SSLServerCertVerification use TransportSecurityInfo instead of nsNSSSocketInfo (25.21 KB, patch)
2012-03-26 16:35 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
no flags Details | Diff | Splinter Review
Split nsNSSSocketInfo into TransportSecurityInfo and nsNSSSocketInfo (167.42 KB, patch)
2012-03-28 02:09 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
honzab.moz: review+
Details | Diff | Splinter Review
Make SSLServerCertVerification use TransportSecurityInfo instead of nsNSSSocketInfo (25.39 KB, patch)
2012-03-28 02:10 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
honzab.moz: review+
Details | Diff | Splinter Review

Description Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-11-18 22:56:54 PST
Creating a TransportSecurityInfo class that implements nsITransportSecurityInfo and the other security-UI-related elements of nsNSSSocketInfo will make the code much easier to understand. Instead of passing an nsNSSSocketInfo to certificate validation, we could pass this TransportSecurityInfo instead. This would make it clearer that the certificate validation code isn't directly affecting anything on the socket transport thread.
Comment 1 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-03-26 10:32:26 PDT
Created attachment 609375 [details] [diff] [review]
Split nsNSSSocketInfo into TransportSecurityInfo and nsNSSSocketInfo

This patch splits nsNSSSocketInfo into two classes--a base class TransportSecurityInfo that handles all the certificate-related stuff, and nsNSSSocketInfo that handles all the SSL-connection stuff. The expectation is that we serialize/deserialize TransportSecurityInfo objects from the cache, and that we create nsNSSSocketInfo instances only when we are actually doing SSL connections.

The benefit of doing this split is that we can change SSLServerCertVerification.cpp to use only TransportSecurityInfo; this way, we can be sure that it is not dependent in any way on working on an actual SSL connection. This will make the patch for bug 660749 easier to review.

This is a very big patch. Other than splitting the class and fixing some whitespace issues, there are no significant code changes.

The part that should be reviewed closest is the changes to nsPSMRememberCertErrorsTable. Previously, it was embedded in nsNSSIOLayerHelpers, which belongs in nsNSSIOLayer.cpp. Besides the obvious refactoring to split it out, I attempted to make it thread-safe by protecting it with a mutex.
Comment 2 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-03-26 10:48:48 PDT
Created attachment 609377 [details] [diff] [review]
Make SSLServerCertVerification use TransportSecurityInfo instead of nsNSSSocketInfo

This patch is to be applied on top of the previous one. It consists mostly of s/nsNSSSocketInfo/TransportSecurityInfo/ and s/socketInfo/infoObject/ in SSLServerCertVerification.cpp.

It also moves the SPDY cert change check to a SSL-connection-specific code-path that won't be used by the patch for bug 660749.

It also makes the calling of the cert error listener conditional on the socketInfo implementing nsISSLSocketControl--nsNSSSocketInfo does, but TransportSecurityInfo on its own (which will be used for revalidating certs from the cache) doesn't.
Comment 3 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-03-26 10:58:09 PDT
*** Bug 733666 has been marked as a duplicate of this bug. ***
Comment 4 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-03-26 16:34:30 PDT
Note: When you read the patch in splinter, there are two sections for nsNSSIOLayer.cpp and two sections for nsNSSIOLayer.h. In each case, one section is for the parts that get copied into TransportSecurityInfo.cpp/h and the other section is for the parts that remain in nsNSSIOLayer.cpp/h.
Comment 5 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-03-26 16:34:59 PDT
Created attachment 609531 [details] [diff] [review]
Split nsNSSSocketInfo into TransportSecurityInfo and nsNSSSocketInfo
Comment 6 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-03-26 16:35:26 PDT
Created attachment 609532 [details] [diff] [review]
Make SSLServerCertVerification use TransportSecurityInfo instead of nsNSSSocketInfo
Comment 7 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-03-28 02:09:50 PDT
Created attachment 610040 [details] [diff] [review]
Split nsNSSSocketInfo into TransportSecurityInfo and nsNSSSocketInfo

Honza, please see the description of the patch in the previous comments.
Comment 8 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-03-28 02:10:46 PDT
Created attachment 610041 [details] [diff] [review]
Make SSLServerCertVerification use TransportSecurityInfo instead of nsNSSSocketInfo

Again, see the comments in the preceeding comments.
Comment 9 Honza Bambas (:mayhemer) 2012-03-29 17:48:21 PDT
Comment on attachment 610040 [details] [diff] [review]
Split nsNSSSocketInfo into TransportSecurityInfo and nsNSSSocketInfo

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

r=honzab with few comments addressed.

> // XXX: threadsafe?

It's inherited.  I.e. remove the comment.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +1115,2 @@
>  {
> +  MutexAutoLock lock(mMutex);

Please don't call any methods of the infoObject while holding this lock.  It is OK now but don't do that as a precaution when something changes.

The same in the Lookup method.  Just protect what you want to protect - members of this class.

::: security/manager/ssl/src/nsNSSIOLayer.h
@@ +144,3 @@
>  
>  public:
> +  RememberCertErrorsTable();

private?
Comment 10 Honza Bambas (:mayhemer) 2012-03-31 12:23:28 PDT
Comment on attachment 610041 [details] [diff] [review]
Make SSLServerCertVerification use TransportSecurityInfo instead of nsNSSSocketInfo

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

r=honzab

Sorry for the delay.
Comment 11 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-04-29 21:02:03 PDT
Updated patches due to bitrot.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a432c2d1b41
https://hg.mozilla.org/integration/mozilla-inbound/rev/57cc1577f951
Comment 12 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-04-29 21:04:46 PDT
(In reply to Brian Smith (:bsmith) from comment #11)
> Updated patches due to bitrot.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/4a432c2d1b41
> https://hg.mozilla.org/integration/mozilla-inbound/rev/57cc1577f951

... and addressed Honza's review comments.

Thank you for the review, Honza.
Comment 13 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-04-30 02:14:51 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/29914c0fb85e

When updating the patch, I accidentally left an unused copy of mSSLStatus in nsNSSSocketInfo, which was wrong and also didn't match what Honza reviewed. I corrected that in the checkin above.

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