812794 - Follow-ups for Bug 722979 - nsStrictTransportSecurityService uses the global Private Browsing service to decide where to set permissions

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

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

(In reply to Josh Matthews [:jdm] from Bug #722979 comment #45)
> > OK, I forgot about the splitting of TransportSecurityInfo and
> > nsNSSSocketinfo and what it means. We cannot put the flags in
> > TransportSecurityInfo because we need to be able to serialize/deserialize
> > TransportSecurityInfo from the cache, but we can't serialize/deserialize the
> > flags. Either we can put the flags accessor in nsISSLSocketControl, or we
> > can just use your original idea of checking if the current tab/window is a
> > private browsing tab/window. Probably your original idea is better.
> 
> I went with nsISSLSocketControl, otherwise I needed to add another
> static_cast to nsNSSSocketInfo in SSLServerCertVerification, since we need
> the flags there.

It is OK to keep the new member in nsISSLSocketControl. However:

::: docshell/base/nsDocShell.cpp
@@ +4180,5 @@
>                  bool isStsHost = false;
> +                uint32_t flags = 0;
> +                if (sslSocketControl) {
> +                    sslSocketControl->GetTransportFlags(&flags);
> +                }

This does not do the correct thing when in a private browsing mode, when !sslSocketControl. Similarly, the similar check in SSLServerCertVerification.cpp does not do the correct thing when !sslSocketControl. These are security-critical checks that should not be skipped.

Please fix this and then also remove the #include "nsISSLSocketControl.h".

AFIACT, the best way to fix this is to change the nsDocShell part to check "am I in a private browsing window" and then change the SSLServerCertVerification bit so that the flags are explicitly passed through CertErrorRunnable so that CheckCertOverrides can access them.

Also, the #include "nsISocketTransport.h" in TransportSecurityInfo.h and in nsNSSIOLayer.h should be removed.

Finally, since we did not merge nsISocketTransport flags and the nsISocketProvider flags into the same set/namespace, it is now very confusing about what functions take which flags. For example, does nsIStrictTransportSecurity::processStsHeader take provider flags or transport flags? This needs to be documented in the IDL. Avoiding this confusion is the reason I suggested that we should merge nsISocketProvider flags and nsISocketProvider flags.

Comment 1

[Josh Matthews [:jdm]]

Given that the nsIStrictTransportSecurityService documentation explicitly mentions nsISocketProvider, I don't think the final concern is actually valid.

Comment 2

[Josh Matthews [:jdm]]

I'm taking care of all of the SSLServerCertVerification changes in bug 769288.

Comment 3

[Josh Matthews [:jdm]]

Attachment: Make docshell STS usage use existing PB knowledge instead of digging in SSL internals.

This also corrects a Provider/Transport flag error that snuck in. I checked in MXR and no more of these should exist.

Comment 4

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

I found another issue:

(In reply to Christian :Biesinger in bug 722979 comment 49:
> Although I don't like the name
> IS_PRIVATE... maybe NO_PERSISTENT_STORAGE or something along those lines?

This flag isn't just about persistent storage. It is also needed, for example, in nsIRecentBadCertsService, even though that service doesn't implement any persistent storage. The new name is thus misleading and I think we should change back to IS_PRIVATE or to IS_PRIVATE_BROWSING OR PRIVATE_BROWSING. I believe that jdm used "isPrivate" and other variants in many different places in his patches.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 682832 [details] [diff] [review]
Make docshell STS usage use existing PB knowledge instead of digging in SSL internals.

r=me

Comment 6

[Josh Matthews [:jdm]]

Brian, I'm going to leave the flag renaming for a separate issue for now; I want to get this one landed post-haste and uplift it because of the flag error that exists.

Comment 7

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c0bd1afbf06

Comment 8

[Josh Matthews [:jdm]]

Comment on attachment 682832 [details] [diff] [review]
Make docshell STS usage use existing PB knowledge instead of digging in SSL internals.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 722979
User impact if declined: anonymous SSL connections can end up sending certificates to the server (regressing bug 466080)
Testing completed (on m-c, etc.): Landed on inbound.
Risk to taking this patch (and alternatives if risky): None. Restores previous behaviour.
String or UUID changes made by this patch: None.

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9c0bd1afbf06

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 682832 [details] [diff] [review]
Make docshell STS usage use existing PB knowledge instead of digging in SSL internals.

restores previous functionality, approving.

Comment 11

[Josh Matthews [:jdm]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/89a184e6ff24