nsIAssociatedContentSecurity is unnecessary and can be removed

RESOLVED FIXED in Firefox 64

Status

()

defect
P1
major
RESOLVED FIXED
7 years ago
9 months ago

People

(Reporter: mayhemer, Assigned: keeler)

Tracking

(Blocks 1 bug, {sec-low})

Trunk
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

(Whiteboard: [psm-assigned][adv-main64-])

Attachments

(1 attachment)

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...
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".
Whiteboard: [sg:moderate]
(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.
Status: NEW → ASSIGNED
Blocks: 749946
Assignee: nobody → honzab.moz
Honza - do you have an ETA for a fix here? How difficult would you say fixing this bug will be?
(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.
(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.)
@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.
Moving to sec-low after discussion with Honza and Brian.
Keywords: sec-moderatesec-low
Whiteboard: [sg:moderate]
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.
Depends on: 822367
Summary: Secure browser UI: Saving/restoring of subrequests state in nsIAssociatedContentSecurity fundamentally broken → nsIAssociatedContentSecurity fundamentally broken
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.
No longer depends on: 822367
Blocks: 822367
Group: crypto-core-security
Group: crypto-core-security
Group: core-security → crypto-core-security
Assignee: honzab.moz → nobody
Status: ASSIGNED → NEW
Is nsIAssociatedContentSecurity still used in NSS?
(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
Whiteboard: [psm-backlog]
Priority: P1 → P3
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.
Assignee: nobody → dkeeler
Group: crypto-core-security
Priority: P3 → P1
Summary: nsIAssociatedContentSecurity fundamentally broken → nsIAssociatedContentSecurity is unnecessary and can be removed
Whiteboard: [psm-backlog] → [psm-assigned]
nsIAssociatedContentSecurity and nsISecurityInfoProvider are unused as of
bug 832834, so this patch removes them.
Comment on attachment 9008496 [details]
bug 748809 - remove nsIAssociatedContentSecurity and nsISecurityInfoProvider r?jrmuizel,mayhemer

Honza Bambas (:mayhemer) has approved the revision.
Attachment #9008496 - Flags: review+
Comment on attachment 9008496 [details]
bug 748809 - remove nsIAssociatedContentSecurity and nsISecurityInfoProvider r?jrmuizel,mayhemer

Jeff Muizelaar [:jrmuizel] has approved the revision.
Attachment #9008496 - Flags: review+
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/be7177f566cb
remove nsIAssociatedContentSecurity and nsISecurityInfoProvider r=mayhemer,jrmuizel
https://hg.mozilla.org/mozilla-central/rev/be7177f566cb
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Whiteboard: [psm-assigned] → [psm-assigned][adv-main64-]
You need to log in before you can comment on or make changes to this bug.