Closed Bug 782654 Opened 12 years ago Closed 12 years ago

Implement Mixed Content Blocker New Icon - Backend Changes

Categories

(Firefox :: Security, defect)

Product:
Firefox
Component:
Security
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 20

People

(Reporter: tanvi, Assigned: tanvi)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 32 obsolete files)

Use a whitelist instead of blacklist approach in nsMixedContentBlocker.cpp v5
11.58 KB, patch
tanvi
: review+
geekboy
: checkin+
Details | Diff | Splinter Review
Mixed active content flag added to the document v7
11.98 KB, patch
smaug
: review+
geekboy
: checkin+
Details | Diff | Splinter Review
Add logic to check expandedPrincipal for addon content scripts v9
4.89 KB, patch
tanvi
: review+
tanvi
: review+
geekboy
: checkin+
Details | Diff | Splinter Review
Bug 62178 implements a mechanism to block http script/active or http display content on https pages. This bug is to actually implement the blocking UI and enhance the patch in bug 62178 to make this feasible.
Now that bug 62178 has landed, we can move on to the blocking UI. The design for this can be found here: http://people.mozilla.com/~lco/ProjectSPF/Mixed_Content/Mixed%20Content%20Spec%20v3.pdf. There may be some visual design tweaks to this from Stephen.
See Also: → 747090, 781018, 776278
Having only crossed and grayed out the "https" for mixed-content pages does not seem very visible to me. Please use an appropriate and distinguishable icon (like the warning sign in the spec) and/or use a thick diagonal black or red (not gray!) bar to cross out the "https".
Depends on: 62178
Open questions / reminders to me: * Should we land 776278 as part of this bug (upgrade http request to https request) * Should we remove the pref security.mixed_content.block_active_content pref from about:config so that users don't have a way to disable blocking? * Should we work to finish bug 747090 (change the icon for mixed script content) first and try and land it in FF18 before we proceed to implement the blocking UI?
(In reply to Florian Bender from comment #2) > Having only crossed and grayed out the "https" for mixed-content pages does > not seem very visible to me. Please use an appropriate and distinguishable > icon (like the warning sign in the spec) and/or use a thick diagonal black > or red (not gray!) bar to cross out the "https". We decided to cross it out in grey (and not red) because we are also using a different icon for it - a yellow triangle with the exclamation point that you can see in page 8 of the spec.
(In reply to Tanvi Vyas from comment #3) my opinions, for what they are worth : > * Should we land 776278 as part of this bug (upgrade http request to https > request) no > * Should we remove the pref security.mixed_content.block_active_content pref > from about:config so that users don't have a way to disable blocking? no > * Should we work to finish bug 747090 (change the icon for mixed script > content) first and try and land it in FF18 before we proceed to implement > the blocking UI? yes
(In reply to Tanvi Vyas from comment #4) > We decided to cross it out in grey (and not red) because we are also using a > different icon for it - a yellow triangle with the exclamation point that > you can see in page 8 of the spec. Alright, I thought the use of a different icon was questioned. However, gray + alarming icon is sufficient.
Change the icon for Mixed Active Content to a yellow triangle with an exclamation mark. Created a new identity mode and security state. Took bsmith's suggestion to implement everything outside of PSM. You can try this out by visiting https://eventbrite.com, which loads external fonts (making it mixed active content). I've got a bug somewhere, so for https://people.mozilla.com/~bsterne/tests/62178/test.html the grey globe shows up instead of the yellow triangle, even though there is mixed script on the page. Will work that out and resubmit. This is only a small portion of the coded needed to complete this bug.
Comment on attachment 666882 [details] [diff] [review] Change LockIcon for Mixed Active Content > var gIdentityHandler = { > // Mode strings used to control CSS display > IDENTITY_MODE_IDENTIFIED : "verifiedIdentity", // High-quality identity information > IDENTITY_MODE_DOMAIN_VERIFIED : "verifiedDomain", // Minimal SSL CA-signed domain verification > IDENTITY_MODE_UNKNOWN : "unknownIdentity", // No trusted identity information > IDENTITY_MODE_MIXED_CONTENT : "unknownIdentity mixedContent", // SSL with unauthenticated content >+ IDENTITY_MODE_MIXED_ACTIVE_CONTENT : "unknownIdentity mixedActiveContent", // SSL with unauthenticated content Since we don't want a special style for IDENTITY_MODE_MIXED_CONTENT, we should just use "unknownIdentity" for it and re-purpose "mixedContent" IDENTITY_MODE_MIXED_ACTIVE_CONTENT. Alternatively, I guess we could get rid of IDENTITY_MODE_MIXED_CONTENT altogether (or just change its meaning to be limited to active content).
(In reply to Dão Gottwald [:dao] from comment #8) > Since we don't want a special style for IDENTITY_MODE_MIXED_CONTENT, we > should just use "unknownIdentity" for it and re-purpose "mixedContent" > IDENTITY_MODE_MIXED_ACTIVE_CONTENT. Alternatively, I guess we could get rid > of IDENTITY_MODE_MIXED_CONTENT altogether (or just change its meaning to be > limited to active content). We don't have a special style for IDENTITY_MODE_MIXED_CONTENT (either for both display and active content, or for just display content - which should arguably be called IDENTITY_MODE_MIXED_DISPLAY CONTENT). However, we may in the future, so I'd rather leave it in. And be specific with the word "active" for active content, to avoid confusion. Is that okay?
(In reply to Tanvi Vyas from comment #9) > However, we may in the future, I don't see this. As I understand it, we didn't do that because it would too often send a false message of insecurity. Why would this change?
(In reply to Dão Gottwald [:dao] from comment #10) > (In reply to Tanvi Vyas from comment #9) > > However, we may in the future, > > I don't see this. As I understand it, we didn't do that because it would too > often send a false message of insecurity. Why would this change? We currently have no plans to change this right now. Just though it would be better to keep it in if some situation in the future came up that would make us want to do something different for mixed display content. We could also get rid of it for now. I do think we should be careful to call all mixed active content "Mixed Active Content", so that it is clearly distinguished form mixed content in general when developers look at the code.
I've figured out the odd bug that I'm experiencing, and it is because of code that is called in PSM, so I'm not going to be able to avoid that code as bsmith suggested. With the attached patch applied, go to https://people.mozilla.com/~tvyas/mixedcontent.html. The Mixed Active Content Icon is displayed and an alert box pops up. After the alert box is clicked, the Mixed Active Content icon turns into the Globe. This is because functions in nsSecureBrowserUIImpl.cpp are called by nsDocLoader.cpp. nsSecureBrowerUIImpl.cpp is doesn't know about the Mixed Active state (since, per bsmith, I've been trying to avoid that code). Here is the backtrace: http://pastebin.mozilla.org/1856548 I'll have to edit nsSecureBrowerUIImpl.cpp after all. Not sure if it should be a quick fix bandaid to solve the problem, or if nsSecureBrowerUIImpl.cpp should be fully aware of the STATE_IS_MIXED_ACTIVE state.
(In reply to Tanvi Vyas from comment #12) > I'll have to edit nsSecureBrowerUIImpl.cpp after all. Not sure if it should > be a quick fix bandaid to solve the problem, or if nsSecureBrowerUIImpl.cpp > should be fully aware of the STATE_IS_MIXED_ACTIVE state. After talking to Kai and dveditz, I decided to apply a bandaid to fix the bug, instead of touching nsSecureBrowserUIImpl.cpp. I've added an attribute allowMixedActiveContent in nsIHttpChannel.idl that is true if mixed active content is loaded on the page. This patch now displays an orangish warning triangle with an exclamation point when mixed active content is on a page. The next step to this bug is to try and put a strike through the https in the url bar.
Attachment #666882 - Attachment is obsolete: true
(In reply to Tanvi Vyas from comment #13) > After talking to Kai and dveditz, I decided to apply a bandaid to fix the > bug, instead of touching nsSecureBrowserUIImpl.cpp. I've added an attribute > allowMixedActiveContent in nsIHttpChannel.idl that is true if mixed active > content is loaded on the page. This seems very wrong to me. Necko shouldn't maintain any state related to mixed content. Why should a channel know anything about mixed content when that is a document-level concept? If nsSecureBrowserUIImpl is broken, then we should either fix it or remove it and do all the mixed-content detection outside of nsSecureBrowserUIImpl. > nsSecureBrowerUIImpl.cpp is doesn't know about the Mixed Active > state (since, per bsmith, I've been trying to avoid that code). My original recommendation was to completely replace the mixed content stuff in nsSecureBrowserUIImpl so that it doesn't calculate any mixed content state any longer. I didn't mean to imply you shouldn't touch nsSecureBrowserUIImpl at all. Ideally, your patch would include: hg rm security/manager/boot/src/nsSecureBrowserUIImpl.h hg rm security/manager/boot/src/nsSecureBrowserUIImpl.cpp and no changes to Necko.
Comment on attachment 668636 [details] [diff] [review] Change LockIcon for Mixed Active Content Review of attachment 668636 [details] [diff] [review]: ----------------------------------------------------------------- Keep in mind that you must serialize the mixed content state somewhere to disk to properly accommodate session restore. Currently, we do this in a very horrible way, serializing the nsIAssociatedContentSecurity from the channel into the HTTP cache. However, that causes many problems, so trying to do anything similar to that is a step backwards. Honza Bambas and I have both independently been thinking a lot about how to fix (replace) the security indicator state tracking in nsISecureBrowserUIImpl. Honza probably has additional ideas of what to do, as he's been thinking about these problems for a very long time. My previous comment about your code being able to easily and completely replace nsISecureBrowserUIImpl is somewhat of an exaggeration; however, it is not that far from the truth, especially after bug 799009 and bug 799007. ::: browser/base/content/browser.js @@ +4224,3 @@ > wpl.STATE_SECURE_HIGH | > wpl.STATE_SECURE_MED | > wpl.STATE_SECURE_LOW; See bug 799007 about HIGH/MED/LOW. ::: content/base/src/nsMixedContentBlocker.cpp @@ +56,5 @@ > + > + //Set allMixedActiveContent flag to true. > + nsCOMPtr<nsIContentViewer> mContentViewer; > + docShell->GetContentViewer(getter_AddRefs(mContentViewer)); > + nsIDocument *doc = mContentViewer->GetDocument(); You already get the document here. Why don't you set the AllowMixedActiveContent property on the document, instead of on the channel? Wouldn't that also make back/forward work correctly? It seems like it would also pave the way for a correct implementation of session restore, since session restore already requires serializing state from the document. Like I said in my previous comment, it doesn't seem to make sense to set this property on the channel, because the channel shouldn't have to deal with the mixed content concept. IMO, instead of setAllowMixedContent(bool), it would be better to add (on nsIDocument): // implemented as: mMixedContentTypes |= types; return NS_OK; void addMixedContent(MixedContentTypes types); // implemented as: *aResult = mMixedContentTypes; return NS_OK; readonly attribute MixedContentTypes mixedContentTypes; Then, AFAICT, you could totally ignore (better: remove) the mixed content status reported by nsISecureBrowserUI, and then remove nsIAssociatedContentSecurity, which would probably also fix bug 748809 and probably bug 509409, and probably many other bugs. ::: uriloader/base/nsIWebProgressListener.idl @@ +172,5 @@ > */ > const unsigned long STATE_IS_INSECURE = 0x00000004; > const unsigned long STATE_IS_BROKEN = 0x00000001; > const unsigned long STATE_IS_SECURE = 0x00000002; > + const unsigned long STATE_IS_MIXED_ACTIVE = 0x00000008; Not sure it is necessary or good to add a new state to nsIWebProgressListener. If you take my suggestion above, then the web progress listener can distinguish between passive/active mixed content by asking the nsIDocument.
Thanks Brian for your help here. I think you are right, and we should use the Document instead of the Channel. In the meantime, a bunch of other things have come up: * We need to account for more content-types in nsMixedContentBlocker.cpp and block by default for content-types that are not specified. Brian has a proposed patch here: https://hg.mozilla.org/try/rev/5d208e51ff76 * If you apply the attached patch to nightly, and go to https://tbpl.mozilla.org/?tree=Try, you will get a "Loading failed: error". This is because nsMixedContentBlocker.cpp recognizes an http XHR as Mixed Active Content. But xhr mixed content isn't considered mixed content per bug https://bugzilla.mozilla.org/show_bug.cgi?id=305284. * Brian has added a patch to bug https://bugzilla.mozilla.org/show_bug.cgi?id=799007 that removes the high/medium/low security states from nsSecureBrowserUIImpl that simplifies the code a bit. * Honza is going to start work on bug https://bugzilla.mozilla.org/show_bug.cgi?id=370886 this week. Will talk to him to coordinate on where he is moving that code, and how it impacts this bug.
(In reply to Dão Gottwald [:dao] from comment #8) > Since we don't want a special style for IDENTITY_MODE_MIXED_CONTENT, we > should just use "unknownIdentity" for it and re-purpose "mixedContent" > IDENTITY_MODE_MIXED_ACTIVE_CONTENT. Alternatively, I guess we could get rid > of IDENTITY_MODE_MIXED_CONTENT altogether (or just change its meaning to be > limited to active content). Dão, I think it is good to allow at least *third-party* extensions and themes to style the address bar differently for active vs. passive mixed content. And, I would like developer tools (ours and thid-party tools) to also be able to style the icon differently when they are active, because that is what will help developers avoid mixed content issues in the first place. Finally, I don't think there is complete agreement on what to display for the passive mixed content case. I do share your concern of raising false alarms. But, I think it is better to discuss that in bug 747090. > IDENTITY_MODE_MIXED_CONTENT : "unknownIdentity mixedContent", // SSL with unauthenticated content >+ IDENTITY_MODE_MIXED_ACTIVE_CONTENT : "unknownIdentity mixedActiveContent", // SSL with unauthenticated content IMO, IDENTITY_MODE_MIXED_ACTIVE_CONTENT should be "unknownIdentity mixedContent mixedActiveContent" (i.e. a superset of IDENTITY_MODE_MIXED_CONTENT) so that stylesheets that don't know about the distinction between mixedContent and mixedActiveContent will be able to still show the right indicators.
> Thanks Brian for your help here. I think you are right, and we should use > the Document instead of the Channel. Complete and will post a new patch a little later today. The back and forth buttons aren't preserving icon state, so I'll have to work that out later. > In the meantime, a bunch of other things have come up: > * We need to account for more content-types in nsMixedContentBlocker.cpp and > block by default for content-types that are not specified. Brian has a > proposed patch here: https://hg.mozilla.org/try/rev/5d208e51ff76 Brian is working on this in bug https://bugzilla.mozilla.org/show_bug.cgi?id=799346
Depends on: 305284, 799346
Depends on: 308496
No longer depends on: 305284
See Also: → newlock, 799346
Updated the patch to use the document instead of the channel. Also fixed a few other things (now allow about:, javascript:, wss:, and data: loads in addition to https: loads). The UI persistence isn't great and is not maintained, though that may be a problem with our lock/globe icons regardless of this patch. I'm going to focus on the rest of the UI before I deal with the persistence issue.
Attachment #668636 - Attachment is obsolete: true
Since there are lot of things that have come up while working on this bug, I've listed all the things we need to do for the Mixed Content Blocker on Firefox Desktop: * First patch to bug 782654 that could change the UI from the globe to the yellow triangle for mixed active content. But doesn't (because we want the other changes to be completed first). This code could land as their is no change to user behavior, and it fixes some bugs. * Second Patch to bug 782654 that puts all UI in place - the triangle icon, the "mixed content protection icon" or shield on the left side of the url bar (where the plugin and geolocation icons show up), the door hanger, etc. * Create mdn/sumo page that has "Technical Information" on the feature if the user wants more info. * Order the content policies - If nsMixedContentBlocker.cpp finds http javascript on an https page, it will flag the page as mixed content, block the script and add the "mixed content icon" to the left side of the url bar. If the page has a CSP policy that blocks that script, then the "mixed content protection icon" shouldn't show up, since the page is blocking that content with CSP and doesn't need the mixed content blocker to block it. Hence, perhaps we should ensure that nsMixedContentBlocker.cpp is called after all other Content Policies are called. Input on this is welcome. * Persistence issue - assume the mixed script content triangle icon appears and the user switches tabs. when the user goes back to the original tab, the icon should be the triangle, but sometimes its a lock. There is a persistence issue here that I have to debug. * Fix bug https://bugzilla.mozilla.org/show_bug.cgi?id=418354 * Session Store Issues - > [bsmith] - Keep in mind that you must serialize the mixed content state somewhere to disk to properly accommodate session restore. Currently, we do this in a very horrible way, serializing the nsIAssociatedContentSecurity from the channel into the HTTP cache. However, that causes many problems, so trying to do anything similar to that is a step backwards. * Telemetry - bug 781018 * XHR load bug conflict with mixed content blocker bug -> After a discussion with the security and privacy engineering team, we decided http XHR + cors should be flagged as mixed content. In current stable versions of firefox, xhr + cors is not flagged as mixed content: https://bugzilla.mozilla.org/show_bug.cgi?id=308496 * Verity that the Mixed Content Blocker does not interfere with csp reports. * Do not allow users to unblock mixed active content on HSTS pages - https://bugzilla.mozilla.org/show_bug.cgi?id=800098. This could potentially be a followup. Or could potentially be a bad idea. * Expand mixed content blocking and make it a whitelist instead of a blacklist. We are missing some content types and need to make the implementation more robust. bsmith has done a first pass on this. We should decide if this is a followup bug or should land in the same release as the Mixed Content Blocker - https://bugzilla.mozilla.org/show_bug.cgi?id=799346 * And then of course, mochitests.
See Also: → 801945, 801947
And a couple more... * Webconsole should distinguish between Mixed Active Content and Mixed Display Content (followup bug) * Webconsole should show users and developers what is blocked when Mixed Active Content is blocked. (I'm not sure about this. It requires more thought and a discussion with the devtools team. Also, we were planning to add this into the Mixed Content Blocker itself in V2 of the feature.)
In browser.css, I had set the identity icon for Mixed Active Content to be identity-icons-https-mixed-active.png. However, the icon change is not ready to land (we have quite a bit of more UI work to do before that and icon persistence issues on going back and forward between pages). The rest of the patch could land sooner though. So I'm instead setting the identity icon for Mixed Active Content to be identity-icons-generic.png (the globe), so that the patch doesn't cause any UI changes. This patch adds logic to nsMixedContentBlocker.cpp and alerts the Browser of Mixed Active Content via nsIDocument. This patch is needed for the telemetry bug 781018. It also fixes a few bugs. Who should review this? Olli for the C++ and Jared for the UI changes?
Attachment #670172 - Attachment is obsolete: true
Attachment #671665 - Flags: review?(jaws)
Attachment #671665 - Flags: review?(bugs)
Push to try - https://tbpl.mozilla.org/?tree=Try&rev=298292a183a1 Next patch will be for the UI changes: (In reply to Tanvi Vyas from comment #20) > * Second Patch to bug 782654 that puts all UI in place - the triangle icon, > the "mixed content protection icon" or shield on the left side of the url > bar (where the plugin and geolocation icons show up), the door hanger, etc.
That looks like a patch that would be best split into two pieces, separating the core changes from the UI pieces.
Comment on attachment 671665 [details] [diff] [review] Add more plumbing to nsMixedContentBlocker and add a mixed content flag to nsIDocument Review of attachment 671665 [details] [diff] [review]: ----------------------------------------------------------------- > Bug 747090 - Change the icon for mixed content. > Added attribute for allowMixedActiveContent The changeset summary either needs to be updated to say "Bug 782654", or this patch should be attached to bug 747090. I agree with Gavin about splitting this patch up. ::: browser/base/content/browser.js @@ +4241,1 @@ > this._hostChanged = false; I'm not sure what changed in browser.js? Different EOL characters? ::: browser/locales/en-US/chrome/browser/browser.properties @@ +226,5 @@ > > identity.encrypted=Your connection to this website is encrypted to prevent eavesdropping. > identity.unencrypted=Your connection to this website is not encrypted. > identity.mixed_content=Your connection to this site is only partially encrypted, and does not prevent eavesdropping. > +identity.mixed_active_content=Your connection to this site is only partially encrypted, and does not prevent eavesdropping ACTIVE. This text looks like it is placeholder. What do you plan on changing it to? ::: browser/themes/gnomestripe/browser.css @@ +1008,5 @@ > .verifiedIdentity > #page-proxy-favicon[pageproxystate="valid"] { > list-style-image: url(chrome://browser/skin/identity-icons-https-ev.png); > } > > +.mixedActiveContent > #page-proxy-favicon[pageproxystate="valid"] { Can an element have the verifiedDomain class as well as the verifiedIdentity class? The reliance on rule ordering here seems too brittle. You should be able to update the above two rules like so, > .verifiedDomain:not(.mixedActiveContent) > #page-proxy-favicon[pageproxystate="valid"] { ... } and > .verifiedIdentity:not(.mixedActiveContent) > #page-proxy-favicon[pageproxystate="valid"] { ... }
Attachment #671665 - Flags: review?(jaws) → review-
(In reply to Jared Wein [:jaws] from comment #25) > > +.mixedActiveContent > #page-proxy-favicon[pageproxystate="valid"] { > > Can an element have the verifiedDomain class as well as the verifiedIdentity > class? No. > The reliance on rule ordering here seems too brittle. You should be > able to update the above two rules like so, > > .verifiedDomain:not(.mixedActiveContent) > #page-proxy-favicon[pageproxystate="valid"] { ... } > and > > .verifiedIdentity:not(.mixedActiveContent) > #page-proxy-favicon[pageproxystate="valid"] { ... } This doesn't make sense. mixedActiveContent and verifiedDomain/verifiedIdentity can't be present at the same time.
(In reply to Dão Gottwald [:dao] from comment #26) > (In reply to Jared Wein [:jaws] from comment #25) > > > +.mixedActiveContent > #page-proxy-favicon[pageproxystate="valid"] { > > > > Can an element have the verifiedDomain class as well as the verifiedIdentity > > class? > > No. Dangit, I had meant to say verifiedDomain coupled with mixedActiveContent. Either way, if the two can't be present at the same time, then I don't see a need to have special rules for .mixedActiveContent since the icons will fall back to the generic icons anyways.
Depends on: 799346
We are updating the spec to match this design: http://people.mozilla.com/~lco/ProjectSPF/Mixed_Content/Mixed%20Content%20concepts%20v3.pdf The Mixed Content icon will now be in the same area we find things like the plug-in icon, location sharing, saved password for this site. This new location brings the Mixed Content icon closer to other security and privacy indicators such as the site identity indicators, as well as the icons mentioned above. I will upload the full revised spec soon. For now, let me know if you have any questions!
(In reply to Larissa Co from comment #28) > We are updating the spec to match this design: > http://people.mozilla.com/~lco/ProjectSPF/Mixed_Content/Mixed%20Content%20concepts%20v3.pdf I don't think the popup should appear automatically, unless you really want the user to interact with it. The popup disappearing automatically after two seconds seems like a rather irritating user experience.
> I don't think the popup should appear automatically, unless you really want > the user to interact with it. The popup disappearing automatically after two > seconds seems like a rather irritating user experience. It's just to introduce the user to what the icon means. After 3 instances, we don't show a popup at all. Also, the behavior of having the popup disappear after a few seconds (if 2 secs is not the right amount of time, I'm flexible about that) is consistent with how our other popups behave.
The proposed behavior is actually not consistent with other popups' behavior, as far as I can tell.
Depends on: 802833
Bug 799346 was originally filed to use a whitelist instead of blacklist approach to nsMixedContentBlocker.cpp and include more content-types. I've marked that bug a duplicate of this one and will work on that as part of this bug. This patch should be applied on top of the plumbing in nsMixedContentBlocker (which I will separate out from the UI changes).
Depends on: 802905
No longer depends on: 802833
See Also: → 802833
Depends on: 803225
Updated Spec to v4: http://people.mozilla.com/~lco/ProjectSPF/Mixed_Content/Mixed_Content_Spec/ (And from now on, this link will always take you to the most up-to-date spec. yay!) * Changed the design so that the Mixed Content icon is now in the permissions bar, closer to the site identity indicator. * Added a button for disabling Mixed Content to the site identity door hanger when Mixed Content is enabled. We are working to revise the site identity doorhanger messages to better distinguish between mixed script and mixed display. Stay tuned for that text. I will also file visD bugs soon.
Based on conversation in #security, it seems like we should be switching to use the principal instead of aRequestLocation, and we should account for blob: URIs too. <devd> What happens if an https page is: <iframe src='data:text/html,<script src=http://www.google.com/test.js></script>'> <devd> Also, similar to data: URIs, blob: URI iframes also inherit, right? Why are they blocked right now by MixedContentBlocker ?
Comment on attachment 671665 [details] [diff] [review] Add more plumbing to nsMixedContentBlocker and add a mixed content flag to nsIDocument >+ /** >+ * Get the allow mixed active content flag for this document. >+ */ >+ bool GetAllowMixedActiveContent() >+ { >+ return mAllowMixedActiveContent; >+ } >+ >+ /** >+ * Set the allow mixed active content flag for this document. >+ */ >+ void SetAllowMixedActiveContent(bool allowMixedActiveContent) >+ { >+ mAllowMixedActiveContent = allowMixedActiveContent; >+ } >+ > > /** > * Get the sandbox flags for this document. > * @see nsSandboxFlags.h for the possible flags > */ > uint32_t GetSandboxFlags() const > { > return mSandboxFlags; >@@ -1887,16 +1903,19 @@ protected: > bool mNeedLayoutFlush; > > // True if a style flush might not be a no-op > bool mNeedStyleFlush; > > // True if a DOMMutationObserver is perhaps attached to a node in the document. > bool mMayHaveDOMMutationObservers; > >+ // True if a document has loaded Mixed Active Script (see nsMixedContentBlocker.cpp) What is Mixed Active Script? >+ bool mAllowMixedActiveContent; >+ > // Fired at the document that attempted to load mixed content. The UI could > // handle this event, for example, by displaying an info bar that offers the > // choice to reload the page with mixed content permitted. >-// >-// Disabled for now until bug 782654 is fixed >-/* >+ > class nsMixedContentBlockedEvent : public nsRunnable > { > public: > nsMixedContentBlockedEvent(nsISupports *aContext, unsigned short aType) > : mContext(aContext), mType(aType) > {} > > NS_IMETHOD Run() > { > NS_ASSERTION(mContext, > "You can't call this runnable without a requesting context"); > > // To update the security UI in the tab with the blocked mixed content, call > // nsISecurityEventSink::OnSecurityChange. You can get to the event sink by > // calling NS_CP_GetDocShellFromContext on the context, and QI'ing to > // nsISecurityEventSink. >+ if(mType == eBlockedMixedScript) { >+ >+ // mixed script was allowed and we need to alert checkIdentity() in browser.js sentences start with capital letter. Also, don't mention browser.js. That it Firefox specific, but this code is in Gecko. >+ // Update the security UI in the tab with the blocked mixed content >+ nsCOMPtr<nsIDocShell> docShell = NS_CP_GetDocShellFromContext(mContext); >+ >+ nsCOMPtr<nsIContentViewer> mContentViewer; >+ docShell->GetContentViewer(getter_AddRefs(mContentViewer)); >+ nsIDocument *doc = mContentViewer->GetDocument(); nsIDocument* doc >+ doc->SetAllowMixedActiveContent(true); >+ >+ nsCOMPtr<nsISecurityEventSink> eventSink = do_QueryInterface(docShell); >+ if (eventSink) { >+ eventSink->OnSecurityChange(mContext, nsIWebProgressListener::STATE_IS_MIXED_ACTIVE); >+ } >+ >+ } else { >+ if(mType == eBlockedMixedDisplay) { >+ //Do Nothing for now; state will already be set STATE_IS_BROKEN >+ } >+ } > > return NS_OK; > } > private: > // The requesting context for the content load. Generally, a DOM node from > // the document that caused the load. > nsCOMPtr<nsISupports> mContext; > > // The type of mixed content that was blocked, i.e. active or display > unsigned short mType; > }; >-*/ > >- bool isHttps; >- if (NS_FAILED(aContentLocation->SchemeIs("https", &isHttps)) || isHttps) { >+ bool isSchemeSafe; >+ //Note - Open to suggestions on the proper way of formatting this >+ if ( NS_FAILED(aContentLocation->SchemeIs("https", &isSchemeSafe)) || isSchemeSafe || >+ NS_FAILED(aContentLocation->SchemeIs("about", &isSchemeSafe)) || isSchemeSafe || >+ NS_FAILED(aContentLocation->SchemeIs("data:", &isSchemeSafe)) || isSchemeSafe || >+ NS_FAILED(aContentLocation->SchemeIs("javascript:", &isSchemeSafe)) || isSchemeSafe || >+ NS_FAILED(aContentLocation->SchemeIs("wss:", &isSchemeSafe)) || isSchemeSafe ) { > return NS_OK; > } This code is being changed in some other bug too. > >+ > // If we are here we have mixed content. > > // Decide whether or not to allow the mixed content based on what type of > // content it is and if the user permitted it. > switch (aContentType) { > case nsIContentPolicy::TYPE_FONT: > case nsIContentPolicy::TYPE_OBJECT: > case nsIContentPolicy::TYPE_SCRIPT: >@@ -143,42 +166,41 @@ nsMixedContentBlocker::ShouldLoad(uint32 > case nsIContentPolicy::TYPE_SUBDOCUMENT: > case nsIContentPolicy::TYPE_WEBSOCKET: > case nsIContentPolicy::TYPE_XMLHTTPREQUEST: > // fonts, plugin content, scripts, stylesheets, iframes, websockets and > // XHRs are considered high risk for mixed content so these are blocked > // per the mixed script preference > if (sBlockMixedScript) { > *aDecision = nsIContentPolicy::REJECT_REQUEST; >- >+ } else { > // Fire the event from a script runner as it is unsafe to run script > // from within ShouldLoad >- // Disabled until bug 782654 is fixed. >- /* > nsContentUtils::AddScriptRunner( > new nsMixedContentBlockedEvent(aRequestingContext, eBlockedMixedScript)); >- */ >+ >+ return NS_OK; > } Oh, we're changing the meaning of nsMixedContentBlockedEvent. It is not about blocking anymore, since if (sBlockMixedScript) should catch that case. The name of nsMixedContentBlockedEvent need to change then >+++ b/docshell/base/nsIDocShell.idl >@@ -457,16 +457,24 @@ interface nsIDocShell : nsISupports You need to update uuid
Attachment #671665 - Flags: review?(bugs) → review-
Comment on attachment 671665 [details] [diff] [review] Add more plumbing to nsMixedContentBlocker and add a mixed content flag to nsIDocument Review of attachment 671665 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsMixedContentBlocker.cpp @@ +148,5 @@ > + if ( NS_FAILED(aContentLocation->SchemeIs("https", &isSchemeSafe)) || isSchemeSafe || > + NS_FAILED(aContentLocation->SchemeIs("about", &isSchemeSafe)) || isSchemeSafe || > + NS_FAILED(aContentLocation->SchemeIs("data:", &isSchemeSafe)) || isSchemeSafe || > + NS_FAILED(aContentLocation->SchemeIs("javascript:", &isSchemeSafe)) || isSchemeSafe || > + NS_FAILED(aContentLocation->SchemeIs("wss:", &isSchemeSafe)) || isSchemeSafe ) { You can check for nsIProtocolHandler::URI_IS_LOCAL_RESOURCE or nsIProtocolHandler::URI_INHERITS_SECURITY_CONTEXT. Similar check is (was?) in secure browser UI. Checking by scheme is always bad....
mayhemer, the checks are being changed in bug 803225.
(In reply to Honza Bambas (:mayhemer) from comment #37) > Checking by scheme is always bad.... Hi Honza, In bug 803225 I have switched to using protocol flags. The patches on this bug are now out of date, as I decided to fix bug 803225 and bug 802905 first. I'll get back to this bug afterwards, adding a flag for mixed content to nsIDocument. I have not worked out how to handle redirects yet (https://bugzilla.mozilla.org/show_bug.cgi?id=418354).
Broke patch 671665 into two pieces (per Gavin). This one is for the UI changes.
Attachment #671665 - Attachment is obsolete: true
Broke patch 671665 into two pieces (per Gavin). This one is for the UI changes. Addressed Jared's comments.
Attachment #683358 - Attachment is obsolete: true
Broke patch 671665 into two pieces. This is the patch that sets a flag in the document when mixed active content is loaded on the page.
Attachment #683358 - Attachment is obsolete: false
Attachment #683359 - Attachment is obsolete: true
Separated out the whitelist patch for content types (so that unknown content types are rejected instead of accepted by default). I have not addressed Olli's comments yet.
Attachment #672602 - Attachment is obsolete: true
Attachment #683362 - Attachment is patch: true
Changed the flag for loaded and allowed mixed active content to something that makes more sense (hasMixedActiveContentLoaded) and addressed Olli's comments. Still a problem with mixed content in embedded iframes. I might resolve that in a separate patch so that we can start to get things working (even though they aren't perfect yet).
Attachment #683358 - Attachment is obsolete: true
Changed the flag for loaded and allowed mixed active content to something that makes more sense (hasMixedActiveContentLoaded) and addressed Olli's comments. Still a problem with mixed content in embedded iframes. I might resolve that in a separate patch so that we can start to get things working (even though they aren't perfect yet).
Attachment #683362 - Attachment is obsolete: true
Changed the flag for loaded and allowed mixed active content to something that makes more sense (hasMixedActiveContentLoaded) and addressed Olli's comments. Still a problem with mixed content in embedded iframes. I might resolve that in a separate patch so that we can start to get things working (even though they aren't perfect yet).
(In reply to Tanvi Vyas from comment #45) > Still a problem with mixed content in embedded iframes. I might resolve > that in a separate patch so that we can start to get things working (even > though they aren't perfect yet). Fixed this issue in this updated patch. Marking patches for review.
Attachment #684201 - Attachment is obsolete: true
Attachment #684270 - Flags: review?(bugs)
Attachment #683366 - Attachment is obsolete: true
Attachment #684202 - Attachment is patch: true
Attachment #684202 - Flags: review?(bugs)
Attachment #684200 - Flags: review?(bugs)
Attachment #684200 - Flags: review?(bugs) → review?(dao)
Comment on attachment 684270 [details] [diff] [review] Mixed active content flag added to the document Update IID of nsIDocument. (Not sure if it is technically absolutely necessary in this case, but better be safe.) >+ /** >+ * Get the has mixed active content loaded flag for this document. >+ */ >+ bool GetHasMixedActiveContentLoaded() >+ { >+ return mHasMixedActiveContentLoaded; >+ } >+ >+ /** >+ * Set the has mixed active content loaded flag for this document. >+ */ >+ void SetHasMixedActiveContentLoaded(bool hasMixedActiveContentLoaded) s/hasMixedActiveContentLoaded/aHasMixedActiveContentLoaded/ >+ // Mixed content was allowed and is about to load; get the document and >+ // set the approriate flag to true if we are about to load Mixed Active >+ // Content. >+ nsCOMPtr<nsIDocShell> docShell = NS_CP_GetDocShellFromContext(mContext); >+ nsCOMPtr<nsIDocShellTreeItem> currentDocShellTreeItem(do_QueryInterface(docShell)); >+ NS_ASSERTION(currentDocShellTreeItem, "No document shell tree item from docshell!"); >+ nsCOMPtr<nsIDocShellTreeItem> sameTypeRoot; >+ currentDocShellTreeItem->GetSameTypeRootTreeItem(getter_AddRefs(sameTypeRoot)); >+ NS_ASSERTION(sameTypeRoot, "No document shell root tree item from document shell tree item!"); >+ >+ //now get the document now from sameTypeRoot Space after // and fix the comment. I guess the latter 'now' is extra. >+ nsCOMPtr<nsIDocShell> rootDocShell(do_QueryInterface(sameTypeRoot)); >+ nsCOMPtr<nsIContentViewer> mContentViewer; >+ rootDocShell->GetContentViewer(getter_AddRefs(mContentViewer)); >+ nsIDocument* rootDoc = mContentViewer->GetDocument(); nsCOMPt<nsiDocument> rootDoc = do_GetInterface(sameTypeRoot); should work. (And null check the result) >+ >+ if(mType == eBlockedMixedScript) { >+ >+ rootDoc->SetHasMixedActiveContentLoaded(true); So, hmm, what does the flag actually mean? Is it more like has-mixed-active-content-loaded-and-blocked or what? > // Cache the pref for mixed display blocking > Preferences::AddBoolVarCache(&sBlockMixedDisplay, > "security.mixed_content.block_display_content"); >+ if (!sMixedContentBlockerLog) >+ sMixedContentBlockerLog = PR_NewLogModule("MixedContentBlocker"); >+ Looks like all this prlog stuff is unused. Remove it. Also from higher up in the file. >+nsDocShell::GetHasMixedActiveContentLoaded(bool *aHasMixedActiveContentLoaded) >+{ >+ *aHasMixedActiveContentLoaded = false; >+ nsCOMPtr<nsIDocument> doc(do_GetInterface(GetAsSupports(this))); >+ if (!doc) { >+ return NS_OK; //Is this the right thing to return??? >+ } >+ *aHasMixedActiveContentLoaded = doc->GetHasMixedActiveContentLoaded(); >+ return NS_OK; >+} Maybe like this: nsCOMPtr<nsIDocument> doc(do_GetInterface(GetAsSupports(this))); *aHasMixedActiveContentLoaded = doc && doc->GetHasMixedActiveContentLoaded(); return NS_OK; >+ * This attribute determines whether Mixed Active Content is loaded on the >+ * httpChannel. Set to true in nsMixedContentBlocker.cpp if Mixed Active Content >+ * is allowed (either explicitly on the page by the user or through the about:config >+ * setting security.mixed_content.block_active_content) and is about to load. >+ */ >+ readonly attribute boolean hasMixedActiveContentLoaded; You need to update uuid when adding something to an interface. Could take a look at a new patch.
Attachment #684270 - Flags: review?(bugs) → review-
Comment on attachment 684202 [details] [diff] [review] Use a whitelist instead of blacklist approach in nsMixedContentBlocker.cpp >@@ -115,54 +115,120 @@ nsMixedContentBlocker::ShouldLoad(uint32 > nsIURI* aContentLocation, > nsIURI* aRequestingLocation, > nsISupports* aRequestingContext, > const nsACString& aMimeGuess, > nsISupports* aExtra, > nsIPrincipal* aRequestPrincipal, > int16_t* aDecision) > { >- // Default policy: allow the load if we find no reason to block it. >- *aDecision = nsIContentPolicy::ACCEPT; >+ // We access sBlockMixedScript and sBlockMixedDisplay without synchronization What does the comment mean? What synchronization ? >+ MOZ_ASSERT(NS_IsMainThread()); > >- // If mixed script blocking and mixed display blocking are turned off >- // we can return early >- if (!sBlockMixedScript && !sBlockMixedDisplay) { >- return NS_OK; >- } I would not remove this early return. >+ // Notes on non-obvious decisions: This is very nice. >+ // TYPE_WEBSOCKET: The W3C Websockets recommendation requires browsers to s/W3C Websockets recommendation/Websocket API/ Mainly because we actually try to implement whatever is specified in WhatWG spec, not W3C. >+ // TYPE_XMLHTTPREQUEST: XHR requires either same origin or CORS, so most >+ // mixed-content XHR will already be blocked by that check. We suspect that >+ // HTTPS-to-HTTP XHR with CORS is relatively uncommon. Odd comment to say anything about our "suspection", unless you have some data about it.
Attachment #684202 - Flags: review?(bugs) → review+
Though, I guess I could take still a look at a new patch.
(In reply to Olli Pettay [:smaug] from comment #48) > Comment on attachment 684270 [details] [diff] [review] > Mixed active content flag added to the document > >+ > >+ if(mType == eBlockedMixedScript) { > >+ > >+ rootDoc->SetHasMixedActiveContentLoaded(true); > So, hmm, what does the flag actually mean? > Is it more like has-mixed-active-content-loaded-and-blocked or what? This is actually a very good question. This property is going to be persisted in session store. Say the flag is *not* set when we block all loads of insecured subcontent using the CSP mixed content blocker. State of the document is then set to (left at) "secured" and thus is also stored to session store. When after session restore some mechanism turns off or bypasses CSP mixed content blocker and we somehow load also the insecured content, will state of the document correctly change to "mixed"? I think no. And this question is even more general, since if we from any reason don't have the CSP mixed content blocker we have no way to determine mixed content. And since CSP is IMO (=needs to be verified) not designed for such critical decisions like document security state, we may get to that situation one day.
(In reply to Honza Bambas (:mayhemer) from comment #51) > (In reply to Olli Pettay [:smaug] from comment #48) > > >+ rootDoc->SetHasMixedActiveContentLoaded(true); > > So, hmm, what does the flag actually mean? > > Is it more like has-mixed-active-content-loaded-and-blocked or what? > This flag means that there is mixed active on the page that has been loaded by the browser (not blocked). It is allowed because a) the about:config setting security.mixed_content.block_active_content is set to false (which is still the current default), or b) the about:config setting security.mixed_content.block_active_content is set to true and the user bypasses the blocked active content through a User Interface. (Note that the User Interface is one of the next steps of this bug and has yet to be implemented; the design is described in comment 34). > This is actually a very good question. This property is going to be > persisted in session store. > > Say the flag is *not* set when we block all loads of insecured subcontent > using the CSP mixed content blocker. State of the document is then set to > (left at) "secured" and thus is also stored to session store. > > When after session restore some mechanism turns off or bypasses CSP mixed > content blocker and we somehow load also the insecured content, will state > of the document correctly change to "mixed"? I think no. > I'm not sure how this can happen. Content Security Policy (CSP) and the Mixed Content Blocker are two very different Content Policies. From my knowledge, there are 6 content policies (CSPService, nsContentBlocker nsNoDataProtocolContentPolicy, nsMixedContentBlocker, nsWebBrowserContentPolicy, nsDataDocumentContentPolicy) that should each be applied on every load (with the exception of redirects - see bug https://bugzilla.mozilla.org/show_bug.cgi?id=418354). We do have an issue in the order of content policies, which is listed as one of the items to address for this bug in comment 20: (In reply to Tanvi Vyas from comment #20) > * Order the content policies - If nsMixedContentBlocker.cpp finds http > javascript on an https page, it will flag the page as mixed content, block > the script and add the "mixed content icon" to the left side of the url bar. > If the page has a CSP policy that blocks that script, then the "mixed > content protection icon" shouldn't show up, since the page is blocking that > content with CSP and doesn't need the mixed content blocker to block it. > Hence, perhaps we should ensure that nsMixedContentBlocker.cpp is called > after all other Content Policies are called. Input on this is welcome. >
(In reply to Tanvi Vyas from comment #52) > This flag means that there is mixed active on the page that has been loaded > by the browser (not blocked). s/there is mixed active content/there is *or was* mixed active content/. In particular, there might have been a mixed script element that executed on the page and then was removed. We need to keep track of this because the unsafe script has tainted everything the page does from that point forward. > > Say the flag is *not* set when we block all loads of insecured subcontent > > using the CSP mixed content blocker. State of the document is then set to > > (left at) "secured" and thus is also stored to session store. > > > > When after session restore some mechanism turns off or bypasses CSP mixed > > content blocker and we somehow load also the insecured content, will state > > of the document correctly change to "mixed"? I think no. > > > I'm not sure how this can happen. Content Security Policy (CSP) and the > Mixed Content Blocker are two very different Content Policies. From my > knowledge, there are 6 content policies (CSPService, nsContentBlocker > nsNoDataProtocolContentPolicy, nsMixedContentBlocker, > nsWebBrowserContentPolicy, nsDataDocumentContentPolicy) that should each be > applied on every load (with the exception of redirects - see bug > https://bugzilla.mozilla.org/show_bug.cgi?id=418354). I think Honza is using "CSP" as a shorthand for the more general "content policy" term. The question is basically whether the mixed content blocker works correctly during session restore. My interpretation of Honza's comment is that during session restore, we load the content without checking with the content policies. I don't know if that is true or not, and I don't know if that is OK or not with respect to the mixed content blocker. > > content with CSP and doesn't need the mixed content blocker to block it. > > Hence, perhaps we should ensure that nsMixedContentBlocker.cpp is called > > after all other Content Policies are called. Input on this is welcome. Instead of making the mixed content blocker an just another implementation of nsIContentPolicy, we can hard-code the call to nsMixedContentBlocker::shouldLoad in the functions that loop through all the registered nsIContentPolicy implementations, so that the nsMixedContentPolicy call occurs after the loop.
Blocks: MixedContentBlocker
(In reply to Brian Smith (:bsmith) from comment #53) > > This flag means that there is mixed active on the page that has been loaded > > by the browser (not blocked). > > s/there is mixed active content/there is *or was* mixed active content/. In > particular, there might have been a mixed script element that executed on > the page and then was removed. We need to keep track of this because the > unsafe script has tainted everything the page does from that point forward. > Sure. If there is a load of mixed active content at any time, the flag will be set. If that script is later removed, the flag shouldn't get unset. Nothing unsets the flag once it is set in the code for a document. > The question is basically whether the mixed content blocker works correctly > during session restore. My interpretation of Honza's comment is that during > session restore, we load the content without checking with the content > policies. I don't know if that is true or not, and I don't know if that is > OK or not with respect to the mixed content blocker. > Okay. We should file a followup bug to investigate whether or not session store goes through the content policies and make it a blocker for bug 815321. > > > content with CSP and doesn't need the mixed content blocker to block it. > > > Hence, perhaps we should ensure that nsMixedContentBlocker.cpp is called > > > after all other Content Policies are called. Input on this is welcome. > > Instead of making the mixed content blocker an just another implementation > of nsIContentPolicy, we can hard-code the call to > nsMixedContentBlocker::shouldLoad in the functions that loop through all the > registered nsIContentPolicy implementations, so that the > nsMixedContentPolicy call occurs after the loop. Thanks for the input! This is a good idea.
Doing some bug management. (In reply to Tanvi Vyas from comment #20) > Since there are lot of things that have come up while working on this bug, > I've listed all the things we need to do for the Mixed Content Blocker on > Firefox Desktop: > > * First patch to bug 782654 that could change the UI from the globe to the > yellow triangle for mixed active content. But doesn't (because we want the > other changes to be completed first). This code could land as their is no > change to user behavior, and it fixes some bugs. > Will be part of this bug (patch Change Site Identity Icon for Mixed Active Content) and we actually will go ahead and change the icon, and just make sure we land the UI part before this gets to beta or stable. Aurora might be okay, but we'll see when we get to that point. > * Second Patch to bug 782654 that puts all UI in place - the triangle icon, > the "mixed content protection icon" or shield on the left side of the url > bar (where the plugin and geolocation icons show up), the door hanger, etc. > To be completed as part of this bug or a bug that blocks this bug. > * Create mdn/sumo page that has "Technical Information" on the feature if > the user wants more info. > To be completed as part of this bug or a bug that blocks this bug. > * Expand mixed content blocking and make it a whitelist instead of a > blacklist. We are missing some content types and need to make the > implementation more robust. bsmith has done a first pass on this. We > should decide if this is a followup bug or should land in the same release > as the Mixed Content Blocker - > https://bugzilla.mozilla.org/show_bug.cgi?id=799346 > This is being done as part of this bug. See patch " Use a whitelist instead of blacklist approach in nsMixedContentBlocker.cpp" ---------------------------- The rest of the items are not blockers for this bug, but should be blockers for the master Mixed Content Blocker bug 815321: > * Order the content policies - If nsMixedContentBlocker.cpp finds http > javascript on an https page, it will flag the page as mixed content, block > the script and add the "mixed content icon" to the left side of the url bar. > If the page has a CSP policy that blocks that script, then the "mixed > content protection icon" shouldn't show up, since the page is blocking that > content with CSP and doesn't need the mixed content blocker to block it. > Hence, perhaps we should ensure that nsMixedContentBlocker.cpp is called > after all other Content Policies are called. Input on this is welcome. > See bug 815329. > * Persistence issue - assume the mixed script content triangle icon appears > and the user switches tabs. when the user goes back to the original tab, > the icon should be the triangle, but sometimes its a lock. There is a > persistence issue here that I have to debug. > See bug 815334. > * Fix bug https://bugzilla.mozilla.org/show_bug.cgi?id=418354 > Redirect issue in bug 418354. > * Session Store Issues - > [bsmith] - Keep in mind that you must serialize > the mixed content state somewhere to disk to properly accommodate session > restore. Currently, we do this in a very horrible way, serializing the > nsIAssociatedContentSecurity from the channel into the HTTP cache. However, > that causes many problems, so trying to do anything similar to that is a > step backwards. > See bug 815345 (just filed). > * Telemetry - bug 781018 > See bug 781018 > * XHR load bug conflict with mixed content blocker bug -> After a discussion > with the security and privacy engineering team, we decided http XHR + cors > should be flagged as mixed content. In current stable versions of firefox, > xhr + cors is not flagged as mixed content: > https://bugzilla.mozilla.org/show_bug.cgi?id=308496 > This will change because of this bug (we will mark xhr + cors as Mixed Active Content and display the triangle icon). There is probably more followup work to be done in bug 308496 though. > * Verity that the Mixed Content Blocker does not interfere with csp reports. > This should no longer be an issue, as we observed as bug 802905 landed and the csp test that checks that reports are successfully sent passed (http://mxr.mozilla.org/mozilla-central/source/content/base/test/test_bug548193.html?force=1) > * Do not allow users to unblock mixed active content on HSTS pages - > https://bugzilla.mozilla.org/show_bug.cgi?id=800098. This could potentially > be a followup. Or could potentially be a bad idea. > See bug 800098 > * And then of course, mochitests. Necessary mochitest will be done on a bug by bug basis.
No longer depends on: 308496, 800098
No longer depends on: 799346
See Also: newlock, 418354, 438760, 747090, 776278, 781018, 799346, 801945, 801947, 802833 →
(In reply to Tanvi Vyas from comment #54) > > Instead of making the mixed content blocker an just another implementation > > of nsIContentPolicy, we can hard-code the call to > > nsMixedContentBlocker::shouldLoad in the functions that loop through all the > > registered nsIContentPolicy implementations, so that the > > nsMixedContentPolicy call occurs after the loop. > > Thanks for the input! This is a good idea. I like this too. And also add it (hard-code it) to the redirect callback?
(In reply to Olli Pettay [:smaug] from comment #48) > Comment on attachment 684270 [details] [diff] [review] > Mixed active content flag added to the document > > Update IID of nsIDocument. > (Not sure if it is technically absolutely necessary in this case, but better > be safe.) > Done > >+ /** > >+ * Get the has mixed active content loaded flag for this document. > >+ */ > >+ bool GetHasMixedActiveContentLoaded() > >+ { > >+ return mHasMixedActiveContentLoaded; > >+ } > >+ > >+ /** > >+ * Set the has mixed active content loaded flag for this document. > >+ */ > >+ void SetHasMixedActiveContentLoaded(bool hasMixedActiveContentLoaded) > s/hasMixedActiveContentLoaded/aHasMixedActiveContentLoaded/ > Done > >+ // Mixed content was allowed and is about to load; get the document and > >+ // set the approriate flag to true if we are about to load Mixed Active > >+ // Content. > >+ nsCOMPtr<nsIDocShell> docShell = NS_CP_GetDocShellFromContext(mContext); > >+ nsCOMPtr<nsIDocShellTreeItem> currentDocShellTreeItem(do_QueryInterface(docShell)); > >+ NS_ASSERTION(currentDocShellTreeItem, "No document shell tree item from docshell!"); > >+ nsCOMPtr<nsIDocShellTreeItem> sameTypeRoot; > >+ currentDocShellTreeItem->GetSameTypeRootTreeItem(getter_AddRefs(sameTypeRoot)); > >+ NS_ASSERTION(sameTypeRoot, "No document shell root tree item from document shell tree item!"); > >+ > >+ //now get the document now from sameTypeRoot > Space after //> and fix the comment. I guess the latter 'now' is extra. > Done > >+ nsCOMPtr<nsIDocShell> rootDocShell(do_QueryInterface(sameTypeRoot)); > >+ nsCOMPtr<nsIContentViewer> mContentViewer; > >+ rootDocShell->GetContentViewer(getter_AddRefs(mContentViewer)); > >+ nsIDocument* rootDoc = mContentViewer->GetDocument(); > nsCOMPt<nsiDocument> rootDoc = do_GetInterface(sameTypeRoot); > should work. > (And null check the result) > Ah, that is easier. > >+ > >+ if(mType == eBlockedMixedScript) { > >+ > >+ rootDoc->SetHasMixedActiveContentLoaded(true); > So, hmm, what does the flag actually mean? > Is it more like has-mixed-active-content-loaded-and-blocked or what? > I talked about this in comment 52. It means what it says, mixed active content is on the page and has loaded (it has not been blocked). > > // Cache the pref for mixed display blocking > > Preferences::AddBoolVarCache(&sBlockMixedDisplay, > > "security.mixed_content.block_display_content"); > >+ if (!sMixedContentBlockerLog) > >+ sMixedContentBlockerLog = PR_NewLogModule("MixedContentBlocker"); > >+ > Looks like all this prlog stuff is unused. Remove it. > Also from higher up in the file. > Done. > >+nsDocShell::GetHasMixedActiveContentLoaded(bool *aHasMixedActiveContentLoaded) > >+{ > >+ *aHasMixedActiveContentLoaded = false; > >+ nsCOMPtr<nsIDocument> doc(do_GetInterface(GetAsSupports(this))); > >+ if (!doc) { > >+ return NS_OK; //Is this the right thing to return??? > >+ } > >+ *aHasMixedActiveContentLoaded = doc->GetHasMixedActiveContentLoaded(); > >+ return NS_OK; > >+} > Maybe like this: > nsCOMPtr<nsIDocument> doc(do_GetInterface(GetAsSupports(this))); > *aHasMixedActiveContentLoaded = doc && doc->GetHasMixedActiveContentLoaded(); > return NS_OK; > Thanks! Done. > >+ * This attribute determines whether Mixed Active Content is loaded on the > >+ * httpChannel. Set to true in nsMixedContentBlocker.cpp if Mixed Active Content > >+ * is allowed (either explicitly on the page by the user or through the about:config > >+ * setting security.mixed_content.block_active_content) and is about to load. > >+ */ > >+ readonly attribute boolean hasMixedActiveContentLoaded; > You need to update uuid when adding something to an interface. > Done.
Attachment #684270 - Attachment is obsolete: true
Attachment #685728 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #49) > Comment on attachment 684202 [details] [diff] [review] > Use a whitelist instead of blacklist approach in nsMixedContentBlocker.cpp > > >@@ -115,54 +115,120 @@ nsMixedContentBlocker::ShouldLoad(uint32 > > nsIURI* aContentLocation, > > nsIURI* aRequestingLocation, > > nsISupports* aRequestingContext, > > const nsACString& aMimeGuess, > > nsISupports* aExtra, > > nsIPrincipal* aRequestPrincipal, > > int16_t* aDecision) > > { > >- // Default policy: allow the load if we find no reason to block it. > >- *aDecision = nsIContentPolicy::ACCEPT; > >+ // We access sBlockMixedScript and sBlockMixedDisplay without synchronization > What does the comment mean? What synchronization ? > This was actually added by bsmith. I've updated the comments with some more details to make it more clear. > >+ MOZ_ASSERT(NS_IsMainThread()); > > > >- // If mixed script blocking and mixed display blocking are turned off > >- // we can return early > >- if (!sBlockMixedScript && !sBlockMixedDisplay) { > >- return NS_OK; > >- } > I would not remove this early return. > We need to do more before we return. We have to see if we have a mixed content case (is this an https page with http content?), determine the content-type of that content (is it mixed display or mixed script?) and then call the Run method to set the hasMixedActiveContentLoaded flag and call OnSecurityChange if we have mixed active content. > > >+ // Notes on non-obvious decisions: > This is very nice. > The credit goes to bsmith, as the first version of this patch was in bug 799346. > >+ // TYPE_WEBSOCKET: The W3C Websockets recommendation requires browsers to > s/W3C Websockets recommendation/Websocket API/ > Mainly because we actually try to implement whatever is specified in WhatWG > spec, not > W3C. > Done. > >+ // TYPE_XMLHTTPREQUEST: XHR requires either same origin or CORS, so most > >+ // mixed-content XHR will already be blocked by that check. We suspect that > >+ // HTTPS-to-HTTP XHR with CORS is relatively uncommon. > Odd comment to say anything about our "suspection", unless you have some > data about it. Okay, that is true since we don't have telemetry on that yet. I changed the note.
Attachment #684202 - Attachment is obsolete: true
Attachment #685862 - Flags: review?(bugs)
(In reply to Tanvi Vyas from comment #58) > > >+ // TYPE_XMLHTTPREQUEST: XHR requires either same origin or CORS, so most > > >+ // mixed-content XHR will already be blocked by that check. We suspect that > > >+ // HTTPS-to-HTTP XHR with CORS is relatively uncommon. > > Odd comment to say anything about our "suspection", unless you have some > > data about it. > > Okay, that is true since we don't have telemetry on that yet. I changed the > note. Nit: It's good to also note that Chrome allows HTTPS -> HTTP XHR and IE doesn't, if that's what our investigation ultimately revealed. The final decision of whether to agree with IE or Chrome still needs to be made. But, if we agree with Chrome now then we're basically making it impossible to make a different decision later.
A push to try shows that we are breaking xpcshell test https://mxr.mozilla.org/mozilla-central/source/js/xpconnect/tests/unit/test_allowedDomainsXHR.js https://tbpl.mozilla.org/?tree=Try&rev=6db7b35b7de9 At first I thought this might because in this patch XHR+CORS is no longer allowed if the request is mixed content. But that is not the case (the test uses http everywhere). The test is failing on attempt to xhr.open("GET", "http://localhost:4444/simple"). The open request goes through nsMixedContentBlocker.cpp and is blocked by this code: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#106 There is no RequestingLocation for the xhr open because the test is creating it in a Sandbox. From bug https://bugzilla.mozilla.org/show_bug.cgi?id=734891 it sounds like the Sandbox has an expanded Principal of the following: ["http://www.example.com", "http://localhost:4444/simple"], and hence we can't get a single Principal with: nsCOMPtr<nsIURI> principalUri; node->NodePrincipal()->GetURI(getter_AddRefs(principalUri)); aRequestingLocation = principalUri; What should I do here? Look for an expandedPrincipal and then check if any of the urls in the list start with https? But just because one starts with https doesn't mean that the origin is an https origin, it just means it might be? Seeking suggestions. Thanks! (Why did it work before -> We have reorganized nsMixedContentBlocker.cpp a bit in the patches to this bug, so now the code at line 106 is called when previously it wasn't because security.mixed_content.block_active_content was set to false, and we checked that before we proceeded with other computations)
I got a little more context on #jetpack. Content scripts are called by add-ons to execute in a sandbox on a page (with no or limited DOM access). These scripts can make XHR calls to a list of predefined locations that are defined in the expandedPrincipal (so it's like CORS for content scripts). The expandedPrincipal does not give us data about the principal of the page the content script is attached to. I'm not sure if there is a way to get the principal/origin from a content script. The nsMixedContentBlocker should block XHR calls to http pages from content scripts attached to https pages. Without knowing whether the content script is attached to an https page, we cannot determine whether or not to allow the XHR call. Right now (both in the code that has already landed and in the code attached to this bug), we are rejecting the XHR call since we cannot get the requesting location.
Gabor, bholley, Mossop - Which parts of the DOM are jetpack content scripts intended to have access to (both read and write?). If they can affect the DOM of an https page, then XHR requests to http pages should be blocked by the nsMixedContentBlocker. If they can't, then they should be allowed. (More context in comments 60 and 61). Thanks!
(In reply to Tanvi Vyas from comment #62) > Which parts of the DOM are jetpack content scripts intended to have access > to (both read and write?). According to my knowledge it can have access to any part of the dom of the content. So the concept is that by default the content-script is a sandbox with the same principal as the content, and the content window is hooked up into the prototype of the sandbox. By default the content-script is accessing the dom through xray wrapper, but it can easily unwrap it and access it directly in practice. > > If they can affect the DOM of an https page, then XHR requests to http pages > should be blocked by the nsMixedContentBlocker. If they can't, then they > should be allowed. > This seems to be tricky since from an ExpandedPrincipal you cannot safely determine which of the principals is the principal of the document the content-script was attached to... Once we knew that part it would be easy to decide when to block if I understand you correctly (if one of the principals is not https AND the site it was attached to is https then block it). If that is the case we might be able store this information in the ExpandedPrincipal at creation time somehow (at content-script sandbox creation time there is a reference to the content window, we can store its principal on the ExpandedPrincipal and reuse it later for this particular check, and if that extra info is not set on the ExpandedPrincipal we can just fall back to a trivial check like: are all the principals https, or all http, or mixed, and block the mixed case) I'd like to understand the risk here. Normally it's ok that a reviewed addon have more privilege than a regular website, but if I understand it correctly the risk here is that the addon might send out some data accidentally unencrypted (I can see this something a reviewer can easily overlook). So maybe then this should be handled at addon-sdk level too if we make a restriction, and communicated toward addon developers to make this clear.
Gabor, thank you for your detailed reply! (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #63) > This seems to be tricky since from an ExpandedPrincipal you cannot safely > determine which of the principals is the principal of the document the > content-script was attached to... Once we knew that part it would be easy to > decide when to block if I understand you correctly (if one of the principals > is not https AND the site it was attached to is https then block it). > Yes, if one or more of the principals is not https AND the site is attached to an https page, then we should block xhr requests to those http principals. We will block when the about:config setting security.mixed_content.block_active_content is set to true (which it eventually will be by default) and the user doe not over-ride the blocking through the Mixed Content UI. If the user does override the blocking, then technically we could unblock the xhr request by the add-on. However, since the user does not depend on the add-on to make the page work, maybe we don't have to worry about adding in that logic. Perhaps we can just make a rule that add-ons shouldn't make http requests from https pages. I'm not sure about the right call here, so we should discuss this further. > If that is the case we might be able store this information in the > ExpandedPrincipal at creation time somehow > Will the content window definitely be in the Expanded Principal? If so, maybe you can make the it the first entry in the ExpandedPrincipal array? > (at content-script sandbox > creation time there is a reference to the content window, we can store its > principal on the ExpandedPrincipal and reuse it later for this particular > check, and if that extra info is not set on the ExpandedPrincipal we can > just fall back to a trivial check like: are all the principals https, or all > http, or mixed, and block the mixed case) > Yes, this trivial check could work. I think the logic would be: * if all principals are http, then the content window is http (since it is one of the principals) so allow them all. * if all principals are https, allow them * if some principals are http and some are https, then block the http principals. > I'd like to understand the risk here. Normally it's ok that a reviewed addon > have more privilege than a regular website, but if I understand it correctly > the risk here is that the addon might send out some data accidentally > unencrypted (I can see this something a reviewer can easily overlook). > If a user visits an https page, they expect their connection with that page to be secure and safe from eavesdroppers and man-in-the-middle attackers. If an add-on makes an xhr request to an http page, that leaks data about the user (their cookies, the referrer, etc) to eavesdroppers in cleartext. It also allows a MITM the intercept and change the response. An eavesdropper might change the xhr response to malicious script that then runs on the https page. The script could change the page (breaking the integrity of the page) or it could steal data from the page (breaking the confidentially of the page). > So maybe then this should be handled at addon-sdk level too if we make a > restriction, and communicated toward addon developers to make this clear. > Making this clearer in the addon-sdk and to developers would be great, since many times the developers don't know that their is a risk. If it's restricted at the SDK level, that would solve the problem all together and we could just allow xhr calls from ExpandedPrincipals. Other than add-ons, is there anything else that uses/requires an ExpandedPrincipal? Can we assume that if we find an ExpandedPrincipal, the request is being made by an add-on?
Comment on attachment 685728 [details] [diff] [review] Mixed active content flag added to the document >+ >+ // Mixed content was allowed and is about to load; get the document and >+ // set the approriate flag to true if we are about to load Mixed Active >+ // Content. >+ nsCOMPtr<nsIDocShell> docShell = NS_CP_GetDocShellFromContext(mContext); >+ nsCOMPtr<nsIDocShellTreeItem> currentDocShellTreeItem(do_QueryInterface(docShell)); >+ NS_ASSERTION(currentDocShellTreeItem, "No document shell tree item from docshell!"); Not quite right assertion. It could be NS_CP_GetDocShellFromContext which is returning null docshell. Also, since it is ok for NS_CP_GetDocShellFromContext to return null, add null check for currentDocShellTreeItem and return early. >+ if(mType == eBlockedMixedScript) { >+ >+ rootDoc->SetHasMixedActiveContentLoaded(true); >+ >+ // Update the security UI in the tab with the blocked mixed content >+ nsCOMPtr<nsISecurityEventSink> eventSink = do_QueryInterface(docShell); >+ if (eventSink) { >+ eventSink->OnSecurityChange(mContext, nsIWebProgressListener::STATE_IS_MIXED_ACTIVE); >+ } This is still misleading. If type is eBlockedMixedScript, that sounds like we have blocked something, but yet it apparently seems to mean that we allow mixed loading. > /** >+ * This attribute determines whether Mixed Active Content is loaded on the >+ * httpChannel. Set to true in nsMixedContentBlocker.cpp if Mixed Active Content >+ * is allowed (either explicitly on the page by the user or through the about:config >+ * setting security.mixed_content.block_active_content) and is about to load. >+ */ >+ readonly attribute boolean hasMixedActiveContentLoaded; security.mixed_content.block_active_content obviously should block hasMixedActiveContentLoaded, not set it true. Fix the comment.
Attachment #685728 - Flags: review?(bugs) → review-
Attachment #685862 - Flags: review?(bugs) → review+
Addressed Olli's comments.
Attachment #685728 - Attachment is obsolete: true
Attachment #687572 - Flags: review?(bugs)
Had to change some function and variable names to remove the word "Blocked" when content wasn't necessarily blocked. Carrying over r+ from Olli. Here is a new push to try. The xpcshell tests will still fail due to the issues we are working out with ExpandedPrincipals and add-ons. https://tbpl.mozilla.org/?tree=Try&rev=07e03d8df7aa
Attachment #685862 - Attachment is obsolete: true
Attachment #687574 - Flags: review+
Removing whitespace. Carrying over r+.
Attachment #687574 - Attachment is obsolete: true
Attachment #687575 - Flags: review+
Attachment #687572 - Flags: review?(bugs) → review+
(In reply to Tanvi Vyas [:tanvi] from comment #64) > Gabor, thank you for your detailed reply! > However, since the user does not depend on the add-on to make the page work, > maybe we don't have to worry about adding in that logic. Perhaps we can > just make a rule that add-ons shouldn't make http requests from https pages. > I'm not sure about the right call here, so we should discuss this further. Not sure if necessary... It's trivial to add on the one hand, not sure if it's the expected result or not. I would prefer add another flag for it, that would make it explicit, or not implement it. > Will the content window definitely be in the Expanded Principal? If so, > maybe you can make the it the first entry in the ExpandedPrincipal array? Yes it will, and we (the Addon-SDK) are the only user of it so we can do that yes. It is very simple solution but feels a bit sneaky, so if we do it this way some explicit comments needed in the code to make it clear (both in SDK and Gecko). This can be checked btw. at sandbox creation time, since we have a reference there to the content window, so it's easy to check its principal against the first element of the array. > Yes, this trivial check could work. I think the logic would be: > * if all principals are http, then the content window is http (since it is > one of the principals) so allow them all. > * if all principals are https, allow them > * if some principals are http and some are https, then block the http > principals. I would like to extend that logic with if the content window is http and some of the extended principals are https allow them... Unless you have a good reason not to enable it... So in case an addon wants to support web sites with https, and let's say want to communicate with its own server, he can choose https to make it work for every website. Or else I cannot see how could an addon support both https and http sites... > Making this clearer in the addon-sdk and to developers would be great, since > many times the developers don't know that their is a risk. If it's > restricted at the SDK level, that would solve the problem all together and > we could just allow xhr calls from ExpandedPrincipals. Currently only AddonSDK is using ExpandedPrincipals. So making this restriction at SDK level is possible (and a lot easier to do probably). My only concern is that someone at some point starts using them and opens a security leak without realizing it. So at least some asserts would be nice in ExpandedPrincipals ctor (while we make sure on the addon side to pass in the content principal as the first element of the array...) > > Other than add-ons, is there anything else that uses/requires an > ExpandedPrincipal? Can we assume that if we find an ExpandedPrincipal, the > request is being made by an add-on? We can assume it for now. For the future... who knows?
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #69) > > Will the content window definitely be in the Expanded Principal? If so, > > maybe you can make the it the first entry in the ExpandedPrincipal array? > > Yes it will, and we (the Addon-SDK) are the only user of it so we can do > that yes. It is very simple solution but feels a bit sneaky, so if we do it > this way some explicit comments needed in the code to make it clear (both in > SDK and Gecko). This can be checked btw. at sandbox creation time, since we > have a reference there to the content window, so it's easy to check its > principal against the first element of the array. > Currently only AddonSDK is using ExpandedPrincipals. So making this > restriction at SDK level is possible (and a lot easier to do probably). My > only concern is that someone at some point starts using them and opens a > security leak without realizing it. So at least some asserts would be nice > in ExpandedPrincipals ctor (while we make sure on the addon side to pass in > the content principal as the first element of the array...) > * We could make the real principal the first element in the ExpandedPrincipal array, and include explicit comments about this in the code. Perhaps we can even rename the ExpandedPrincipal to something else (CORSAllowedOrigins, JetpackExpandedPrincipal, …) in case someone else decides to use it down the road. * If we make this change in the Addon-SDK, it will only affect add ons using the new SDK versions (correct?). Addons using old SDKs won't have this check implemented, and hence we have to decide whether we want to allow them to make the XHR call anyway, or break them in the mixed content case. * We could also make changes to the ExpandedPrincipal constructor to ensure that the first element is the real principal, but wouldn't that break add ons on old SDK versions that don't set their real principal as the first array element in the ExpandedPrincipal? I will be joining tomorrows Jetpack meeting and discuss this further with Gabor and the jetpack team.
The good news is that the related Addon-SDK feature is not yet released (it's close to it actually but as far as I know the related patch is not yet landed) so we can make any changes required, without having to worry about breaking any existing Add-ons. Anyway, see you soon on the meeting and we'll discuss the rest.
Talked to Gabor and the jetpack team today. We decided that the content principal will be the first element in the array of ExpandedPrincipals. If a load does not have a principal, I will check if it has an expanded principal and check if the first element in the expanded principal is https. If it is, then the logic in nsMixedContentBlocker will continue and block the load if the pref to block is enabled. If it is not, then we will return NS_OK and proceed. There are no existing addons that use ExpandedPrincipal yet (the bug that would allow this hasn't landed yet) so we don't have to worry about breaking any existing addons. I will write a patch that does this, attach it here, and ask Gabor for review. Thanks!
Attachment #684200 - Flags: review?(dao) → review?(jaws)
I've written a patch for the expandedPrincipal, but I'm not fully confident with it. The xpcshell test test_allowedDomainsXHR.js is now passing and all the mixed content mochitests are succeeding. I've made changes beyond just adding the expandedPrincipal. I added 2 new ways to get the aRequestingLocation (one is try getting a window instead of a node, and the other uses aRequestPrincipal). I added comments to the reviewer about whether or not these changes make sense. Flagging this for review for Gabor (for the 5 lines of code under if(expanded) and the comments relating to them). And an additional review from smaug for the rest. Here is a push to try - https://tbpl.mozilla.org/?tree=Try&rev=235a34468bea
Attachment #688519 - Flags: review?(gkrizsanits)
Flags: needinfo?(bugs)
Attachment #688519 - Flags: review?(bugs)
Flags: needinfo?(bugs)
Comment on attachment 684200 [details] [diff] [review] Change Site Identity Icon for Mixed Active Content >+ } else if (state & nsIWebProgressListener.STATE_IS_BROKEN) { >+ if(gBrowser.docShell.hasMixedActiveContentLoaded) { if ( >+ this.setMode(this.IDENTITY_MODE_MIXED_ACTIVE_CONTENT); >+ } else { >+ this.setMode(this.IDENTITY_MODE_MIXED_CONTENT); >+ } >+ } else if (state & nsIWebProgressListener.STATE_IS_MIXED_ACTIVE) { >+ this.setMode(this.IDENTITY_MODE_MIXED_ACTIVE_CONTENT); >+ } else { Why are there two paths that would cause us to set IDENTITY_MODE_MIXED_ACTIVE_CONTENT? >+identity.mixed_active_content=This site contains content that isn't secure. Your connection to this site is only partially encrypted and does not prevent eavesdropping. Has this string been signed off on by a UX person? "contains content that isn't secure" seems to be just another way of saying "is only partially encrypted". >--- a/browser/themes/winstripe/browser.css >+++ b/browser/themes/winstripe/browser.css >-#identity-box:hover > #page-proxy-favicon { >+.mixedActiveContent > #page-proxy-favicon[pageproxystate="valid"] { >+ list-style-image: url(chrome://browser/skin/identity-icons-https-mixed-active.png); >+} >+ >+identity-box:hover > #page-proxy-favicon { > -moz-image-region: rect(0, 32px, 16px, 16px); > } You broke this selector. >--- a/browser/themes/winstripe/jar.mn >+++ b/browser/themes/winstripe/jar.mn You forgot to add identity-icons-https-mixed-active.png to the aero section.
Attachment #684200 - Flags: review?(jaws) → review-
(In reply to Brian Smith (:bsmith) from comment #15) > > const unsigned long STATE_IS_INSECURE = 0x00000004; > > const unsigned long STATE_IS_BROKEN = 0x00000001; > > const unsigned long STATE_IS_SECURE = 0x00000002; > > + const unsigned long STATE_IS_MIXED_ACTIVE = 0x00000008; > > Not sure it is necessary or good to add a new state to > nsIWebProgressListener. If you take my suggestion above, then the web > progress listener can distinguish between passive/active mixed content by > asking the nsIDocument. (In reply to Brian Smith (:bsmith) from comment #17) > > IDENTITY_MODE_MIXED_CONTENT : "unknownIdentity mixedContent", // SSL with unauthenticated content > >+ IDENTITY_MODE_MIXED_ACTIVE_CONTENT : "unknownIdentity mixedActiveContent", // SSL with unauthenticated content > > IMO, IDENTITY_MODE_MIXED_ACTIVE_CONTENT should be "unknownIdentity > mixedContent mixedActiveContent" (i.e. a superset of > IDENTITY_MODE_MIXED_CONTENT) so that stylesheets that don't know about the > distinction between mixedContent and mixedActiveContent will be able to > still show the right indicators. What happened to these suggestions?
Comment on attachment 688519 [details] [diff] [review] Add logic to check expandedPrincipal for addon content scripts v1 I don't like this approach. If we want to support having a primary principal for expanded principals, we should just fully implement nsIPrincipal, instead of having callers munge the whitelist. Note that if the first principal of the starts becoming the 'main' principal, we'll need to document that somehow (loudly), because it's a significant change in the current behavior AFAICT.
Attachment #688519 - Flags: review-
Thinking about this a bit more, I really don't like this notion of having a primary principal on the nsEP. How about this: If we find ourselves doing an XHR to an http:// domain with an nsEP, we check if our whitelist contains any https:// domains. If so, we deny access. This solves the general problem of leaking encrypted stuff onto insecure channels without handicapping our infrastructure so much.
(In reply to Bobby Holley (:bholley) (on vacation though Dec 4) from comment #76) > Comment on attachment 688519 [details] [diff] [review] > Add logic to check expandedPrincipal for addon content scripts v1 > > I don't like this approach. If we want to support having a primary principal > for expanded principals, we should just fully implement nsIPrincipal, > instead of having callers munge the whitelist. I'm not a big fan of it either, but didn't have a much better idea yet. Fully implementing nsIPrincpal would be very confusing and error prone imo. If someone is not aware of their existence can easily write code that just checks the URI of a principal and if that does not return null one will never detect that it is an nsEP. I would prefer a ContentPrincipal or something similar on the nsIEP interface for this. Eh... I'm really trying to find a better solution for this. > > Note that if the first principal of the starts becoming the 'main' > principal, we'll need to document that somehow (loudly), because it's a > significant change in the current behavior AFAICT. Yes. (In reply to Bobby Holley (:bholley) (on vacation though Dec 4) from comment #77) > Thinking about this a bit more, I really don't like this notion of having a > primary principal on the nsEP. > > How about this: If we find ourselves doing an XHR to an http:// domain with > an nsEP, we check if our whitelist contains any https:// domains. If so, we > deny access. This solves the general problem of leaking encrypted stuff onto > insecure channels without handicapping our infrastructure so much. It does, except it comes with a huge restriction I would not like to try to explain to add-on developers to be honest, becase if I forget about the implementation details for a second is very weird. Because that would mean that you cannot specify an http and an https origin for an addon. Add-on developer already complaining a lot about all the restrictions... I'm not sure that we really need any restrictions for add-ons here. Or maybe we should do the restrictions on the SDK side. Like not allowing a content-script with non https origins to be attached to an https site maybe?
(In reply to Dão Gottwald [:dao] from comment #74) > Comment on attachment 684200 [details] [diff] [review] > Change Site Identity Icon for Mixed Active Content > > >+ } else if (state & nsIWebProgressListener.STATE_IS_BROKEN) { > >+ if(gBrowser.docShell.hasMixedActiveContentLoaded) { > > if ( > > >+ this.setMode(this.IDENTITY_MODE_MIXED_ACTIVE_CONTENT); > >+ } else { > >+ this.setMode(this.IDENTITY_MODE_MIXED_CONTENT); > >+ } > >+ } else if (state & nsIWebProgressListener.STATE_IS_MIXED_ACTIVE) { > >+ this.setMode(this.IDENTITY_MODE_MIXED_ACTIVE_CONTENT); > >+ } else { > > Why are there two paths that would cause us to set > IDENTITY_MODE_MIXED_ACTIVE_CONTENT? > If a website has mixed active content that has loaded, we set the state to IDENTITY_MODE_MIXED_ACTIVE_CONTENT in nsMixedContentBlocker and we set hasMixedActiveContentLoaded to true. The yellow triangle will show up as the Site Identity indicator. If the website then has a subsequent load of mixed display content, the state will get set to STATE_IS_BROKEN by nsSecureBrowserUIImpl.cpp. nsSecureBrowserUIImpl.cpp is PSM code that is fragile and will eventually go away or be re-written (per Kai, bsmith, and Honza). When STATE_IS_BROKEN AND we know that hasMixedActiveContentLoaded is true, then we have a Mixed Active state (as opposed to a Mixed Display state) and we should use the yellow triangle. If I don't use two paths, then the second case (mixed active content is loaded then mixed display content is loaded) would result in a globe instead of a yellow triangle. > >+identity.mixed_active_content=This site contains content that isn't secure. Your connection to this site is only partially encrypted and does not prevent eavesdropping. > > Has this string been signed off on by a UX person? "contains content that > isn't secure" seems to be just another way of saying "is only partially > encrypted". > I have been working with Larissa from UX on the text. This text is similar to the current text for identity.mixed_content. We are working on making both of these better, and have some ideas on an etherpad, but have not finalized the text yet. In the meantime, we are going with this. I will double-check with Larissa to see if that is okay with her. The other two comments were addressed and I will post an updated patch shortly.
(In reply to Dão Gottwald [:dao] from comment #75) > (In reply to Brian Smith (:bsmith) from comment #15) > > > const unsigned long STATE_IS_INSECURE = 0x00000004; > > > const unsigned long STATE_IS_BROKEN = 0x00000001; > > > const unsigned long STATE_IS_SECURE = 0x00000002; > > > + const unsigned long STATE_IS_MIXED_ACTIVE = 0x00000008; > > > > Not sure it is necessary or good to add a new state to > > nsIWebProgressListener. If you take my suggestion above, then the web > > progress listener can distinguish between passive/active mixed content by > > asking the nsIDocument. > I am adding the hasMixedActiveContentLoaded flag to the document. Why is it bad to add a new state to nsIWebProgressListnener? We can later change the nsIWebProgressListner appropriately after we decided what to do with and rewrite nsSecureBrowserUIImpl.cpp. > (In reply to Brian Smith (:bsmith) from comment #17) > > > IDENTITY_MODE_MIXED_CONTENT : "unknownIdentity mixedContent", // SSL with unauthenticated content > > >+ IDENTITY_MODE_MIXED_ACTIVE_CONTENT : "unknownIdentity mixedActiveContent", // SSL with unauthenticated content > > > > IMO, IDENTITY_MODE_MIXED_ACTIVE_CONTENT should be "unknownIdentity > > mixedContent mixedActiveContent" (i.e. a superset of > > IDENTITY_MODE_MIXED_CONTENT) so that stylesheets that don't know about the > > distinction between mixedContent and mixedActiveContent will be able to > > still show the right indicators. > > What happened to these suggestions? I made this change; although it doesn't look like any stylesheets are using mixedContent yet - http://mxr.mozilla.org/mozilla-central/search?string=mixedContent&case=on&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
Updated patch for review.
Attachment #684200 - Attachment is obsolete: true
Attachment #689061 - Flags: review?(dao)
Comment on attachment 688519 [details] [diff] [review] Add logic to check expandedPrincipal for addon content scripts v1 Removing r? for smaug and gabor until I have an updated patch.
Attachment #688519 - Flags: review?(gkrizsanits)
Attachment #688519 - Flags: review?(bugs)
Comment on attachment 689061 [details] [diff] [review] Change Site Identity Icon for Mixed Active Content v2 (In reply to Tanvi Vyas [:tanvi] from comment #79) > If a website has mixed active content that has loaded, we set the state to > IDENTITY_MODE_MIXED_ACTIVE_CONTENT in nsMixedContentBlocker and we set > hasMixedActiveContentLoaded to true. The yellow triangle will show up as > the Site Identity indicator. If the website then has a subsequent load of > mixed display content, the state will get set to STATE_IS_BROKEN by > nsSecureBrowserUIImpl.cpp. nsSecureBrowserUIImpl.cpp is PSM code that is > fragile and will eventually go away or be re-written (per Kai, bsmith, and > Honza). When STATE_IS_BROKEN AND we know that hasMixedActiveContentLoaded > is true, then we have a Mixed Active state (as opposed to a Mixed Display > state) and we should use the yellow triangle. If I don't use two paths, > then the second case (mixed active content is loaded then mixed display > content is loaded) would result in a globe instead of a yellow triangle. Please document this in the code accordingly. > > >+identity.mixed_active_content=This site contains content that isn't secure. Your connection to this site is only partially encrypted and does not prevent eavesdropping. > > > > Has this string been signed off on by a UX person? "contains content that > > isn't secure" seems to be just another way of saying "is only partially > > encrypted". > > > I have been working with Larissa from UX on the text. This text is similar > to the current text for identity.mixed_content. We are working on making > both of these better, and have some ideas on an etherpad, but have not > finalized the text yet. In the meantime, we are going with this. I will > double-check with Larissa to see if that is okay with her. It's similar to the current text, but with a redundant sentence added, which doesn't make much sense. Please get this sorted out now or just re-use identity.mixed_content directly rather than adding a new string. We shouldn't land a string that we know we're going to change, as it pointlessly generates work for localizers. (In reply to Tanvi Vyas [:tanvi] from comment #80) > > > Not sure it is necessary or good to add a new state to > > > nsIWebProgressListener. If you take my suggestion above, then the web > > > progress listener can distinguish between passive/active mixed content by > > > asking the nsIDocument. > > I am adding the hasMixedActiveContentLoaded flag to the document. Why is it > bad to add a new state to nsIWebProgressListnener? We can later change the > nsIWebProgressListner appropriately after we decided what to do with and > rewrite nsSecureBrowserUIImpl.cpp. No, once we've shipped this state, we can't take it back. > switch (this._state & wpl_security_bits) { > case wpl.STATE_IS_SECURE: > level = "high"; > break; >- case wpl.STATE_IS_BROKEN: >+ case wpl.STATE_IS_BROKEN | wpl.STATE_IS_MIXED_ACTIVE: > level = "broken"; > break; > } case wpl.STATE_IS_BROKEN: case wpl.STATE_IS_MIXED_ACTIVE: level = "broken"; break; >+ } else if (state & nsIWebProgressListener.STATE_IS_BROKEN) { >+ if(gBrowser.docShell.hasMixedActiveContentLoaded) { if (
Attachment #689061 - Flags: review?(dao) → review-
First part of the patch is for Olli (I check if aRequestingContext is a window in cases where it's not a node). Second part of the patch is for Gabor to review (expandedPrincipal)
Attachment #688519 - Attachment is obsolete: true
Attachment #689362 - Flags: review?(gkrizsanits)
Attachment #689362 - Flags: review?(bugs)
Flagging Needs-info for Larissa. She is still working on finalizing the Site Identity text. In the meantime, we will keep it the same as it is today (per Dao's suggestion not too change it and then change it again).
Flags: needinfo?(lco)
Flagging Stephen for Mixed Content Blocker Icon design (the icon that will show up when mixed content has been blocked - next to the plugin and geolocation icons).
Flags: needinfo?(shorlander)
Address Dao's review comments. Got rid of the STATE_IS_MIXED_ACTIVE state on the WebProgressListener and instead am just checking the document flag hasMixedActiveContentLoaded.
Attachment #689061 - Attachment is obsolete: true
Attachment #689478 - Flags: review?(dao)
Addressing the change requested by Dao and bsmith to remove STATE_IS_MIXED_ACTIVE from nsIWebProgressListner, caused a few changes in this patch (mostly just removing some code). Asking for review from smaug again since this patch has slightly changed.
Attachment #687572 - Attachment is obsolete: true
Attachment #689479 - Flags: review?(bugs)
Missed on change.
Attachment #689479 - Attachment is obsolete: true
Attachment #689479 - Flags: review?(bugs)
Attachment #689482 - Flags: review?(bugs)
Attachment #689362 - Attachment is patch: true
Comment on attachment 689478 [details] [diff] [review] Change Site Identity Icon for Mixed Active Content v3 > this._encryptionLabel[this.IDENTITY_MODE_MIXED_CONTENT] = > gNavigatorBundle.getString("identity.mixed_content"); >+ this._encryptionLabel[this.IDENTITY_MODE_MIXED_ACTIVE_CONTENT] = >+ gNavigatorBundle.getString("identity.mixed_active_content"); You should re-use identity.mixed_content rather than adding a new string with the same content. >+ if(gBrowser.docShell.hasMixedActiveContentLoaded) { again: 'if (', not 'if('
Attachment #689478 - Flags: review?(dao) → review-
Comment on attachment 689482 [details] [diff] [review] Mixed active content flag added to the document v7 Some patch somewhere stopped adding a new flag to nsIWebProgressListener, but I don't see now which one. This looks good too.
Attachment #689482 - Flags: review?(bugs) → review+
Comment on attachment 689362 [details] [diff] [review] Add logic to check expandedPrincipal for addon content scripts v2 >+ } else { >+ // Try window if its not a node. >+ // Question for reviewer - in which cases would it be a window instead of a node? >+ // nsDataDocumentContentPolicy checks for a window if a node doesn't exist. >+ // Does it make sense to the same thing here, in nsMixedContentBlocker? I didn't find cases when it would be window, but nothing in the API prevents passing window as context. >+ nsCOMPtr<nsIDOMWindow> window = do_QueryInterface(aRequestingContext); >+ if (window) { >+ nsCOMPtr<nsIScriptObjectPrincipal> scriptObjPrin = do_QueryInterface(window); >+ if(scriptObjPrin) { >+ >+ nsIPrincipal* prin = scriptObjPrin->GetPrincipal(); >+ if(prin) { >+ >+ nsCOMPtr<nsIURI> principalUri; Extra newline after if(scriptObjPrin) { and if(prin) { >+ prin->GetURI(getter_AddRefs(principalUri)); >+ aRequestingLocation = principalUri; >+ } >+ } >+ } >+ // If content scripts from an addon are causing this load, they have an >+ // ExpandedPrincipal instead of a Principal. This is privileged code, so allow >+ // the load. >+ if (nsCOMPtr<nsIExpandedPrincipal> expanded = do_QueryInterface(aRequestPrincipal)) { >+ *aDecision = ACCEPT; >+ return NS_OK; >+ } else { >+ // We still don't have a requesting location and there is no Expanded Principal. >+ // We can't tell if this is a mixed content load. Deny to be safe. >+ *aDecision = REJECT_REQUEST; >+ return NS_OK; >+ } This feels a bit unsafe. And if this stuff is for some new addon sdk feature, we should start from more restrictive, and remove restrictions if they are too tight. I guess I'm leaning to the approach where extended principal must have only https: stuff.
Attachment #689362 - Flags: review?(bugs)
Attachment #689362 - Flags: review?(bobbyholley+bmo)
Attachment #689362 - Flags: review-
(In reply to Dão Gottwald [:dao] from comment #90) > Comment on attachment 689478 [details] [diff] [review] > Change Site Identity Icon for Mixed Active Content v3 > > > this._encryptionLabel[this.IDENTITY_MODE_MIXED_CONTENT] = > > gNavigatorBundle.getString("identity.mixed_content"); > >+ this._encryptionLabel[this.IDENTITY_MODE_MIXED_ACTIVE_CONTENT] = > >+ gNavigatorBundle.getString("identity.mixed_active_content"); > > You should re-use identity.mixed_content rather than adding a new string > with the same content. > Can we leave this as is for now, even though it is redundant? That way, when Larissa has the new string, we can just make a text change instead of a code change? I expect to have the new string before this would move to Aurora (January 6th week), so the old string and the redundancy would only be in nightly for a short period. > >+ if(gBrowser.docShell.hasMixedActiveContentLoaded) { > > again: 'if (', not 'if(' Fixed.
Attachment #689810 - Flags: review?(dao)
Attachment #689478 - Attachment is obsolete: true
Removed the whitespace per Olli's comment. Gabor, bholley - r? to you for the ExpandedPrincipal change.
Attachment #689362 - Attachment is obsolete: true
Attachment #689362 - Flags: review?(gkrizsanits)
Attachment #689362 - Flags: review?(bobbyholley+bmo)
Attachment #689813 - Flags: review?(gkrizsanits)
Attachment #689813 - Flags: review?(bugs)
Attachment #689813 - Flags: review?(bobbyholley+bmo)
Comment on attachment 689813 [details] [diff] [review] Add logic to check expandedPrincipal for addon content scripts v3 I this is really what addon sdk wants, ok.
Attachment #689813 - Flags: review?(bugs) → review+
Try push from yesterday looks pretty good. The only changes since have been whitespace changes: https://tbpl.mozilla.org/?tree=Try&rev=2c2ef63a668a
Comment on attachment 689810 [details] [diff] [review] Change Site Identity Icon for Mixed Active Content v4 > > > this._encryptionLabel[this.IDENTITY_MODE_MIXED_CONTENT] = > > > gNavigatorBundle.getString("identity.mixed_content"); > > >+ this._encryptionLabel[this.IDENTITY_MODE_MIXED_ACTIVE_CONTENT] = > > >+ gNavigatorBundle.getString("identity.mixed_active_content"); > > > > You should re-use identity.mixed_content rather than adding a new string > > with the same content. > > > Can we leave this as is for now, even though it is redundant? That way, > when Larissa has the new string, we can just make a text change instead of a > code change? I expect to have the new string before this would move to > Aurora (January 6th week), so the old string and the redundancy would only > be in nightly for a short period. No, we'd have to still change the string name such that localizers (who may have translated the old string while it was on mozilla-central) would pick up the changed semantics. The cleanest and simplest way is to just add the new string when it's ready.
Attachment #689810 - Flags: review?(dao) → review-
Removed the string for identity.mixed_active_content until we have a final version from Larissa.
Attachment #689810 - Attachment is obsolete: true
Attachment #689834 - Flags: review?(dao)
Comment on attachment 689813 [details] [diff] [review] Add logic to check expandedPrincipal for addon content scripts v3 I don't understand why we're using a potentially-different principal in the first check and the second check. Unless we have a good, well-understood, well-documented reason, we should first compute the nsIPrincipal we're using for the entire trap, and then handle the various cases.
Attachment #689813 - Flags: review?(bobbyholley+bmo) → review-
Attachment #689834 - Flags: review?(dao) → review+
Sorry for not responding. I'm working on the text for the Site Identity description. We know it's not great, but it's fairly complex to change. For now keep what we have (although we should not ship with it because it's confusing). Also, for reference, the associated VisD bugs are (linking them to this bug too): https://bugzilla.mozilla.org/show_bug.cgi?id=803626 https://bugzilla.mozilla.org/show_bug.cgi?id=803620
Depends on: 803626, 803620
Flags: needinfo?(lco)
Addressed bholley's comments. We use the aRequestPrincipal to get the aRequestingLocation if one is not passed into the function. If we still don't have aRequestingLocation (possibly because aRequestPrincipal is not passed in yet - it was a recently added parameter in bug 767134 and bug 803765, and hasn't been added everywhere it should be), we try getting aRequestingLocation from aRequestingContext. If that doesn't work, we check if an ExpandedPrincipal exists. If an ExpandedPrincipal exists this is privileged code, and we allow the load. If it does not exist, then we deny the load since we've done all we can to get the aRequestingLocation but we still don't have it.
Attachment #689813 - Attachment is obsolete: true
Attachment #689813 - Flags: review?(gkrizsanits)
Attachment #689991 - Flags: review?(bugs)
Attachment #689991 - Flags: review?(bobbyholley+bmo)
Comment on attachment 689991 [details] [diff] [review] Add logic to check expandedPrincipal for addon content scripts v4 Review of attachment 689991 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsMixedContentBlocker.cpp @@ +273,3 @@ > if (!aRequestingLocation) { > + nsCOMPtr<nsIURI> principalUri; > + aRequestPrincipal->GetURI(getter_AddRefs(principalUri)); If aRequestPrincipal isn't passed, this will cause a null deref, right? @@ +276,5 @@ > + aRequestingLocation = principalUri; > + > + // If we can't get the aRequestingLocation from the aRequestingPrincipal, > + // try getting it from the DOM node. > + if(!aRequestingLocation) { nit: if ( @@ +279,5 @@ > + // try getting it from the DOM node. > + if(!aRequestingLocation) { > + nsCOMPtr<nsINode> node = do_QueryInterface(aRequestingContext); > + if (node) { > + node->NodePrincipal()->GetURI(getter_AddRefs(principalUri)); My original concern still stands. If this code is to have any effect, and we're assuming that aRequestingPrincipal was passed (above), then we have two inherently different principals: aRequestingPrincipal and node->NodePrincipal(). This code implies that they are different, and I want to know why. I'm suggesting something along the lines of the following pseudocode. if (!aRequestingLocation) { if (!aRequestingPrincipal) { // Compute aRequestingPrincipal via NodePrincipal or SOP. If we // still don't have it, bail. } // Now we have aRequestingPrincipal. Try to get its URI. If we can't, then // handle the other cases (system principal, nsExpandedPrincipal, null principal). }
Attachment #689991 - Flags: review?(bobbyholley+bmo) → review-
Comment on attachment 689991 [details] [diff] [review] Add logic to check expandedPrincipal for addon content scripts v4 Uh, I hate content policy API. But to be safe, better to do what bholley suggests. only try to get principal from node or window if principal isn't passed. Sorry that I missed that earlier.
Attachment #689991 - Flags: review?(bugs)
Would be good to go through all the content policy API usage and check how it is being used.
Tried to address bholley and smaug's comments. Now, we check if aRequestPrincipal exists. If it does, we try and get aRequestingLocation from it. If it doesn't, we check if aRequestingContext exists. We try and get aRequestingLocation from it. If that fails and we still do not have aRequestingLocation, we check if an ExpandedPrincipal exists. If an ExpandedPrincipal exists this is privileged code, and we allow the load. If it does not exist, then we deny the load since we've done all we can to get the aRequestingLocation but we still don't have it. Push to try: https://tbpl.mozilla.org/?tree=Try&rev=5384f28aede5 I asked on bug 803765 about which cases non-gecko code would be the caller and not pass in aRequetingPrincipal, and the response was extensions/addons. But they weren't sure if there would be other cases as well. As follow up to bug 767134 and bug 803765, there probably should have been a sweep through the code to add in aRequestPrincipal for all gecko callers. I'm not sure if there is a bug filed for this or if Dev plans to tackle that later.
Attachment #689991 - Attachment is obsolete: true
Attachment #690114 - Flags: review?(bugs)
Attachment #690114 - Flags: review?(bobbyholley+bmo)
Comment on attachment 690114 [details] [diff] [review] Add logic to check expandedPrincipal for addon content scripts v5 >+ // Try window if its not a node. >+ nsCOMPtr<nsIDOMWindow> window = do_QueryInterface(aRequestingContext); >+ if (window) { >+ nsCOMPtr<nsIScriptObjectPrincipal> scriptObjPrin = do_QueryInterface(window); You could skip the QI to nsIDOMWindow and just QI to nsIScriptObjectPrincipal > if (!aRequestingLocation) { >- *aDecision = REJECT_REQUEST; >- return NS_OK; >+ // If content scripts from an addon are causing this load, they have an >+ // ExpandedPrincipal instead of a Principal. This is privileged code, so allow >+ // the load. >+ if (nsCOMPtr<nsIExpandedPrincipal> expanded = do_QueryInterface(aRequestPrincipal)) { To be super safe, could you add a check if aRequestPrincipal is system principal. That should be able load everything. Usually we don't call this method with system principal, but addons may end up doing that. So if (nsCOMPtr<nsIExpandedPrincipal> expanded = do_QueryInterface(aRequestPrincipal) || aRequestPrincipal && nsContentUtils::IsSystemPrincipal(aRequestPrincipal)) (This way too complicated. We should force principal usage for content policy API and always use that, or something like that. But no such changes in this bug, please.)
Attachment #690114 - Flags: review?(bugs) → review+
Made the changes suggested by Olli and carrying over the r+. New push to try: https://tbpl.mozilla.org/?tree=Try&rev=e610fa365fa6 Will request additional review from bholley.
Attachment #690114 - Attachment is obsolete: true
Attachment #690114 - Flags: review?(bobbyholley+bmo)
Attachment #690136 - Flags: review+
Attachment #690136 - Flags: review?(bobbyholley+bmo)
> To be super safe, could you add a check if aRequestPrincipal is system > principal. That should be I think shouldLoad will never get called if the requestor is a system principal. None of the other gecko nsicontentpolicy implementations (e.g., CSP ) check whether principal causing the load is a system principal. If not checking for system principal causes breakage, other places would be breaking too.
NS_CheckContentLoadPolicy explicitly filters out system principal. I don't see how it would be filtered out if addon is calling the API.
You are right, it won't be. But, IIRC, addons directly call the API inside their own shouldload functions, forwarding the original arguments they got. Since their own shouldLoad function will not be called for system principal, the addon would not call the API with a system principal. Now it is possible for an addon to call the API directly with the system principal as argument, but I am just saying I am not aware of any that do. The fact that other nsicontentpolicy listeners inside Gecko do not make a special case for systemprincipal makes me think mixedcontentblocker shouldn't either. It seems weird that mixedcontentblocker is the only in-gecko contentpolicy that checks for system principal.
Comment on attachment 690136 [details] [diff] [review] Add logic to check expandedPrincipal for addon content scripts v6 I still think this is much more confusing and verbose than it needs to be. This patch calls GetURI three times, checks aRequestingLocation twice, and sets aRequestingLocation three times. I'd still like to see this simplified along the lines of comment 102. 1 - Check for aRequestingLocation. If we have it, we're done. 2 - If we don't have it, figure out the requesting principal. This is aRequestingPrincipal if we have it, otherwise we compute it from the Node or SOP. 3 - Once we have the principal, call GetURI on it. If we get a URI, we're done. 4 - Check for EP and system principal. If that's the case, allow. 5 - Deny.
Attachment #690136 - Flags: review?(bobbyholley+bmo) → review-
I followed bholley's suggestions from comments 102 and 111. Here is a new version of the code that uses the following logic: * If we have aRequestingLocation, we are done. * If not, we check if we don't have aRequestPrincipal (and if we don't we try getting it from the node or window) * We check again if we have aRequestPrincipal (either because it was passed in or because we got it from the node or window). If we do, we get the uri associated with the prinicpal and set aRequestingLocation. * If we still don't have aRequestingLocation, we check if there is an expanded principal. If there is, then we accept. If we don't, then we deny. Carrying over the r+ from smaug and r? to bholley.
Attachment #690136 - Attachment is obsolete: true
Attachment #690517 - Flags: review?(bobbyholley+bmo)
Comment on attachment 690517 [details] [diff] [review] Add logic to check expandedPrincipal for addon content scripts v7 Carrying over r+ from Olli.
Attachment #690517 - Flags: review+
Comment on attachment 690517 [details] [diff] [review] Add logic to check expandedPrincipal for addon content scripts v7 Review of attachment 690517 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsMixedContentBlocker.cpp @@ +274,5 @@ > + // If we don't have aRequestPrincipal, try getting it from the > + // DOM node using aRequestingContext > + nsCOMPtr<nsINode> node = do_QueryInterface(aRequestingContext); > + if (node) { > + nsIPrincipal* aRequestPrincipal = node->NodePrincipal(); This redeclares and thus shadows aRequestPrincipal. It should give a compile warning I think? @@ +279,5 @@ > + } else { > + // Try using the windows script object principal if its not a node. > + nsCOMPtr<nsIScriptObjectPrincipal> scriptObjPrin = do_QueryInterface(aRequestingContext); > + if (scriptObjPrin) { > + nsIPrincipal* aRequestPrincipal = scriptObjPrin->GetPrincipal(); Same here.
Attachment #690517 - Flags: review?(bobbyholley+bmo) → review-
Removed the extra declaration for nsRequestPrincipal. Push to try: https://tbpl.mozilla.org/?tree=Try&rev=29e6e4cedeea
Attachment #690517 - Attachment is obsolete: true
Attachment #690575 - Flags: review?(bobbyholley+bmo)
Comment on attachment 690575 [details] [diff] [review] Add logic to check expandedPrincipal for addon content scripts v8 Carrying over smaug r+.
Attachment #690575 - Flags: review+
Comment on attachment 690575 [details] [diff] [review] Add logic to check expandedPrincipal for addon content scripts v8 Review of attachment 690575 [details] [diff] [review]: ----------------------------------------------------------------- r=bholley with comments addressed. :-) ::: content/base/src/nsMixedContentBlocker.cpp @@ +276,5 @@ > + nsCOMPtr<nsINode> node = do_QueryInterface(aRequestingContext); > + if (node) { > + aRequestPrincipal = node->NodePrincipal(); > + } else { > + // Try using the windows script object principal if its not a node. Nit: |window's| and |it's|. @@ +290,1 @@ > aRequestingLocation = principalUri; Please check the rv for the GetURI call and only do the assignment if NS_SUCCEEDED(rv). @@ +293,3 @@ > if (!aRequestingLocation) { > + // If content scripts from an addon are causing this load, they have an > + // ExpandedPrincipal instead of a Principal. This is privileged code, so allow "pseudo-privileged"
Attachment #690575 - Flags: review?(bobbyholley+bmo) → review+
Nits addressed. Thanks bholley and smaug!!! New push to try: https://tbpl.mozilla.org/?tree=Try&rev=517276e3b5ce
Attachment #690575 - Attachment is obsolete: true
Attachment #690608 - Flags: review+
Comment on attachment 690608 [details] [diff] [review] Add logic to check expandedPrincipal for addon content scripts v9 r+ from bholley and smaug.
Attachment #690608 - Flags: review+
I would like to land 3/4 patches in this bug (everything but "Change Site Identity Icon for Mixed Active Content v5"). These will hence all be plumbing changes and not affect the UI. I'm working on a 5th patch that will add a dropdown (something like http://cl.ly/image/1E2c300w1f3M), and I don't want to land "Change Site Identity Icon for Mixed Active Content v5" until that is done. I've pushed the other 3 patches to try. If the results are good, then we can land these: https://tbpl.mozilla.org/?tree=Try&rev=2c13509c6c08 Mixed active content flag added to the document v7 Use a whitelist instead of blacklist approach in nsMixedContentBlocker.cpp v5 Add logic to check expandedPrincipal for addon content scripts v9
Try looks good and I haven't heard any objections to landing, so I'm going to add checkin-needed flags to the appropriate attachments. https://tbpl.mozilla.org/?tree=Try&rev=2c13509c6c08
Attachment #690608 - Flags: checkin?(sstamm)
Attachment #687575 - Flags: checkin?(sstamm)
Attachment #689482 - Flags: checkin?(sstamm)
Comment on attachment 690608 [details] [diff] [review] Add logic to check expandedPrincipal for addon content scripts v9 https://hg.mozilla.org/integration/mozilla-inbound/rev/1c5601022840
Attachment #690608 - Flags: checkin?(sstamm) → checkin+
Comment on attachment 687575 [details] [diff] [review] Use a whitelist instead of blacklist approach in nsMixedContentBlocker.cpp v5 https://hg.mozilla.org/integration/mozilla-inbound/rev/eef87ef15814
Attachment #687575 - Flags: checkin?(sstamm) → checkin+
Comment on attachment 689482 [details] [diff] [review] Mixed active content flag added to the document v7 https://hg.mozilla.org/integration/mozilla-inbound/rev/3cb5f5c8fdc0
Attachment #689482 - Flags: checkin?(sstamm) → checkin+
Reopening... there is still some work to do here and a reviewed patch attached to this bug (though, not ready to land). Also adding the [leaveopen] to the whiteboard. Sorry, should have done that earlier.
Status: RESOLVED → REOPENED
Flags: needinfo?(shorlander)
Resolution: FIXED → ---
Whiteboard: [leaveopen]
Flags: needinfo?(shorlander)
Can you file a new bug for the additional work? Tracking multiple patch landings in one bug is generally a bad practice, it leads to very confusing bug history. It's also generally a good idea to split out bugs for front-end and back-end, and mark them as dependent - this bug has patches both to Firefox and to Gecko, which can also be hard to follow.
This bug was intended for the Mixed Content Blocker UI, which has both backend and frontend components. Neither is quite done with the patches that have landed. I'd like to keep this one open and perhaps file bugs that this bug depends on for the rest of the patches? Though separating the javascript and C++ changes into different bugs feels odd to me. I have 2 new WIP patches that I'm going to attach here for now, as I'm stuck on one part and would like input.
This is intended to look something like http://cl.ly/image/1E2c300w1f3M, but right now, it looks more like http://people.mozilla.com/~tvyas/MixedUI3B.png. This patch needs some CSS work.
This patch looks like it should work, and allow mixed active content when the user overrides the blocking on a particular page. However, I can't seem to set gBrowser.docShell.userOverrideBlockedMixedActiveContent to true in browser.js. Every time I dump the value it is false (even if I change it's default state to true in nsDocShell.cpp). Not sure what I'm missing here.
(In reply to Tanvi Vyas [:tanvi] from comment #128) > This bug was intended for the Mixed Content Blocker UI, which has both > backend and frontend components. Neither is quite done with the patches > that have landed. I'd like to keep this one open and perhaps file bugs that > this bug depends on for the rest of the patches? Though separating the > javascript and C++ changes into different bugs feels odd to me. Splitting the work up by code involved is generally exactly what you should do, because reviewers and interested parties tend to be different for changes to different modules. You can have a single tracking bug that covers the larger effort, if you find that useful, and indeed it looks like you're already doing that with bug 815321. So really, all I'm saying is that this particular bug's scope is too broad and we should segment the work further into UI back-end work (Core), and actual UI work (Firefox). Turning a bug with landed patches into a tracking bug after the fact is not ideal, so I recommend closing this one as FIXED (perhaps after changing its summary to describe what was implemented here), filing a new "Mixed Content UI" general tracker bug, which you can make this one block, and then filing additional separate dependent bugs off of that one. (I'm sorry if this seems overly pedantic. Good practices in this area can prove to be really important, particularly when it comes to someone later trying to figure out what we implemented, when, and for what reason. Looking back through poorly organized bug history when trying to deal with e.g. a firedrill situation 1 year down the line can be very frustrating.)
Flags: needinfo?(shorlander)
Per Gavin, splitting this bug out into multiple sub bugs that block 815321.
Summary: Implement Mixed Content Blocker UI → Implement Mixed Content Blocker New Icon - Backend Changes
Blocks: 822373
This bug now only covers the backend changes needed to add the Mixed Active Content icon in place of the globe. Plus some plumbing changes (whitelist instead of blacklist, expanded principal). All this has already landed. The rest of the tasks that were originally going to be part of this bug (per comment 55) have been split out into the following: Bug 822366 - Implement Mixed Content Blocker New Icon - Frontend Changes Bug 822367 - Implement Mixed Content Blocker Doorhanger - Backend Changes Bug 822371 - Implement Mixed Content Blocker Doorhanger - Frontend Changes Bug 822373 - Learn More pages for Mixed Content Blocker I'll merge all the remaining patches to those bugs and obsolete them here.
No longer blocks: 822373
Comment on attachment 689834 [details] [diff] [review] Change Site Identity Icon for Mixed Active Content v5 Carried over to bug 822366.
Attachment #689834 - Attachment is obsolete: true
Comment on attachment 692567 [details] [diff] [review] Mixed Content Blocker Doorhanger v1 Carried over to bug 822371.
Attachment #692567 - Attachment is obsolete: true
Comment on attachment 692569 [details] [diff] [review] Plumbing for Mixed Content Blocker Doorhanger v1 Carrying over to bug 822367
Attachment #692569 - Attachment is obsolete: true
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Whiteboard: [leaveopen]
Tanvi and I worked on improving the site identity doorhanger messages for mixed content here: https://firefox-ux.etherpad.mozilla.org/MixedContent-messages I know this isn't really the bug to file this in now, but since there was some conversation about it here, I wanted to provide the link to it. I don't think we have a separate bug yet for the site identity doorhanger messages. Will have to check with Tanvi.
https://people.mozilla.com/~tvyas/WrongSiteIdentityMessage.png I found a bug in the code that landed on nightly. Suppose you visit a page that is http and it has an https iframe within the page. If that https iframe has mixed content, we set the identity state to STATE_IS_BROKEN when it should be unknown (http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#77). We should instead check if the load is within an iframe or if the rootDoc is https. If the rootDoc is not https, then we should allow the load and return, without calling eventSink and changing the security state.
Blocks: 824871
Blocks: 838394
Blocks: 418354
No longer blocks: 418354
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: