748809 - nsIAssociatedContentSecurity is unnecessary and can be removed

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Honza Bambas (:mayhemer)]

During work on bug 745254 I discovered we share nsIAssociatedContentSecurity implemented by nsNSSSocketInfo between different top level requests.

It means we may just overwrite the sub requests counters and get the wrong mixed content state on bfcaching.  It's random, depends on the socket shared for top channel loads.

It's funny I discovered this that late...

STR:
- have a new tab
- navigate to a fully secured page with a link to a mixed content page
- navigate to that mixed content page (by clicking the link)
- see mixed content
- navigate back
- sometimes it happens you see mixed content indicators
- if not, refresh and navigate forward, you should see a full security state on the mixed page
- if not, refresh and go back, you should see mixed indication on the fully secure page
- if still no luck, just wait for all KA connections to close and try jumping and refreshing here and there
- sooner or later, you'll see this is broken

(I'm using local IIS 7.5 server with default settings and a custom CA/DV certs)

This is broken across all current supported releases I believe.


To have a quick fix, we may let nsHttpChannel implement nsIAssociatedContentSecurity it self.  I'll have a patch by today.

Fixing bug 370886 is a next step...

Comment 1

[Daniel Veditz [:dveditz]]

In all of your descriptions you appear to have a secure site that may or may not be showing the right "mixed" state. If it's limited to that I'm inclined to call this "moderate", but if the issue can lead to a completely insecure site appearing secure (which we've seen in other bugs) then I'd lean more toward "high".

Comment 2

[Honza Bambas (:mayhemer)]

(In reply to Daniel Veditz [:dveditz] from comment #1)
> but if the issue can lead to a completely insecure
> site appearing secure

No it doesn't.

Comment 3

[Josh Aas]

Honza - do you have an ETA for a fix here? How difficult would you say fixing this bug will be?

Comment 4

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

(In reply to Honza Bambas (:mayhemer) from comment #0)
> During work on bug 745254 I discovered we share nsIAssociatedContentSecurity
> implemented by nsNSSSocketInfo between different top level requests.

Simply, nsNSSSocketInfo should not implement nsIAssociatedContentSecurity because nsNSSSocketInfo is information about a connection, not about a transaction (request/response).

> To have a quick fix, we may let nsHttpChannel implement
> nsIAssociatedContentSecurity it self.

There's a similar problem with this approach--for a given channel, there is never any "associated content," right? Only documents have associated content.

nsHttpChannel should not implement nsIAssociatedContentSecurity directly; otherwise we may keep the nsHttpChannel instance alive in memory longer than necessary.

Perhaps a good way to fix this would be to have TransportSecurityInfo stop implementing nsIAssociatedContentSecurity and nsISerializable, and then have a new subclass of TransportSecurityInfo that looks like this:
     
  
     class DocumentSecurityInfo : public nsIAssociatedContentSecurity
                                , public nsISerializable
                                , public nsITransportSecurityInfo
                                , public nsISSLStatusProvider
                                , ...
     {
         DocumentSecurityInfo(nsISupports * transportSecurityInfo)
              : mTransportSecurityInfo(transportSecurityInfo)
         {
         }

         // Move the implementations from TransportSecurityInfo to this class
         NS_DECL_NSIASSOCIATEDCONTENTSECURITY
         NS_DECL_NSISERIALIZABLE

         NS_FORWARD_NSITRANSPORTSECURITYINF(*mTransportSecurityInfo);
         NS_FORWARD_NSISSLSTATUSPROVIDER(*mTransportSecurityInfo);
         ...

     };

Then, whenever we basically whenever we copy the security-info from the transport  channel to the document, we would need to make a DocumentSecurityInfo wrapper for the transport's security info.

Comment 5

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

(In reply to Brian Smith (:bsmith) from comment #4)
> a new subclass of TransportSecurityInfo

I should have written "a new class" instead of "a new subclass of TransportSecurityInfo". (When I was editing my comment I realized subclassing TransportSecurityInfo would be bad and that we should use NS_FORWARD instead.)

Comment 6

[Honza Bambas (:mayhemer)]

@Josh: I don't think we can get this in for the next release (15).  This will need more changes that can be sensitive.  Depends on what priority this gets, I can work on this the next month.

@Brian: I have a similar idea - let sec ui change the sec info on doc channels.  We only need to make sure it gets persisted in cache.  That may be the tricky part.

Comment 7

[Josh Aas]

Moving to sec-low after discussion with Honza and Brian.

Comment 8

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

Besides the issues noted above, nsIAssociatedContentSecurity also doesn't work well with e10s. We should just remove nsIAssociatedContentSecurity. We already removed the distinction between low/medium/high security and Tanvi has re-implemented mixed content detection.

Comment 9

[Honza Bambas (:mayhemer)]

Yes, I need to overview how the work on mixed content detection based on CSP continues.  I'd love to remove secure browser ui completely (we are IMO not that far from it to happen, right?), then nsIACS can go as well.

Comment 10

[Tanvi Vyas[:tanvi]]

Is nsIAssociatedContentSecurity still used in NSS?

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to Tanvi Vyas [:tanvi] from comment #10)
> Is nsIAssociatedContentSecurity still used in NSS?

It definitely is used around the platform:
https://mxr.mozilla.org/mozilla-central/search?string=nsIAssociatedContentSecurity

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler]]

The implementation of the mixed content blocker made this essentially irrelevant. nsSecureBrowserUIImpl thought it still needed to use this, but it was wrong (see bug 832834). Now that that's been fixed, we can just remove this.

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: bug 748809 - remove nsIAssociatedContentSecurity and nsISecurityInfoProvider r?jrmuizel,mayhemer

nsIAssociatedContentSecurity and nsISecurityInfoProvider are unused as of
bug 832834, so this patch removes them.

Comment 14

[Honza Bambas (:mayhemer)]

Comment on attachment 9008496 [details]
bug 748809 - remove nsIAssociatedContentSecurity and nsISecurityInfoProvider r?jrmuizel,mayhemer

Honza Bambas (:mayhemer) has approved the revision.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 9008496 [details]
bug 748809 - remove nsIAssociatedContentSecurity and nsISecurityInfoProvider r?jrmuizel,mayhemer

Jeff Muizelaar [:jrmuizel] has approved the revision.

Comment 16

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Thanks for the reviews!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a50bcdc95db26b77e5dd53523d11ba7debb3c73

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/be7177f566cb
remove nsIAssociatedContentSecurity and nsISecurityInfoProvider r=mayhemer,jrmuizel

Comment 18

[Noemi Erli[:noemi_erli]]

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