security.mixed_content.block_active_content causes crash in nsMixedContentBlocker::ShouldLoad

RESOLVED FIXED in mozilla18

Status

()

Core
DOM
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: kenny.macdermid, Assigned: tanvi)

Tracking

({crash, regression, reproducible})

18 Branch
mozilla18
crash, regression, reproducible
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(crash signature, URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:18.0) Gecko/18.0 Firefox/18.0
Build ID: 20120918030553

Steps to reproduce:

Set the key 'security.mixed_content.block_active_content' to true. Browse to https://panopticlick.eff.org/ and click 'TEST ME'.

This was tested in safe mode, and with a completely new profile.


Actual results:

Browser crashed. See https://crash-stats.mozilla.com/report/index/bp-3c0ba554-7ce2-40c7-a068-5aee62120918 .


Expected results:

No crash.

Comment 2

6 years ago
No crash on Windows.
Severity: normal → critical
Status: UNCONFIRMED → NEW
Crash Signature: [@ nsMixedContentBlocker::ShouldLoad]
Component: Untriaged → DOM
Ever confirmed: true
Keywords: crash
Product: Firefox → Core
I experienced the crash on Mac OS X 10.7.  Last few lines from the command line...

WARNING: NS_ENSURE_TRUE(aURI) failed: file /Users/tvyas/dev/mozilla-central/caps/src/nsScriptSecurityManager.cpp, line 2048
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file /Users/tvyas/dev/mozilla-central/extensions/cookie/nsPermissionManager.cpp, line 905
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file /Users/tvyas/dev/mozilla-central/extensions/permissions/nsContentBlocker.cpp, line 249
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file /Users/tvyas/dev/mozilla-central/extensions/permissions/nsContentBlocker.cpp, line 211

I tried adding 
if (!aContentLocation)
  return NS_ERROR_FAILURE; 
to http://hg.mozilla.org/mozilla-central/annotate/e4757379b99a/content/base/src/nsMixedContentBlocker.cpp#l90 and rebuilt.  All the mixed content tests still pass and clicking "test me" on https://panopticlick.eff.org/ no longer causes the browser to crash.  The test finishes.

Updated

6 years ago
Keywords: reproducible
I can crash reliably while loading gmail (and the pref set to true): bp-71194fb1-e057-49f7-af98-1370c2120918

Updated

6 years ago
Crash Signature: [@ nsMixedContentBlocker::ShouldLoad] → [@ nsMixedContentBlocker::ShouldLoad] [@ nsMixedContentBlocker::ShouldLoad(unsigned int, nsIURI*, nsIURI*, nsISupports*, nsACString_internal const&, nsISupports*, nsIPrincipal*, short*)]
OS: Mac OS X → All
Hardware: x86 → All

Updated

6 years ago
Duplicate of this bug: 792336

Updated

6 years ago
Blocks: 62178
Keywords: regression

Comment 6

6 years ago
crashes on facebook, too.
https://crash-stats.mozilla.com/report/index/bp-4f435c1b-cc05-4c17-95d8-017a12120919
Tested with Mac OS X 10.8 and Windows 7.

Updated

6 years ago
Summary: security.mixed_content.block_active_content causes crash on panopticlick.eff.org → security.mixed_content.block_active_content causes crash in nsMixedContentBlocker::ShouldLoad
I'm working on fixing this.
Assignee: nobody → tanvi
Backtrace: http://pastebin.mozilla.org/1832008

Adding a null check will fix the issue:

+ if (!aContentLocation || NS_FAILED(aContentLocation->SchemeIs("https", &isHttps)) || isHttps) {
-   if(NS_FAILED(aContentLocation->SchemeIs("https", &isHttps)) || isHttps) {
to http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#130

But looking at the backtrace, I think there might be another issue elsewhere in the code (why is aContentLocation null?  Perhaps an issue in nsObjectLoadingContent.cpp).  If I don't figure this out by tomorrow, I'll update the bug with the simple patch to aContentLocation so that we can land that in Nightly and people can experiment with the feature without it crashing their browser.  And I can continue investigating.
Looks like there has been a change to panoptclick.eff.org, and hence I am longer able to reproduce the crash.  I tried facebook and gmail as well.  Is there a developer/older version of panopticlick.eff.org that I can use?
Tanvi, were you able to reproduce this when signed in on a Google+ profile page?
Created attachment 663487 [details] [diff] [review]
WIP Patch

I can reproduce using gmail on a Mac.  But not gmail on Linux.  I went through my backtrace and traced through the code that resulted in the null aContentLocation.  It is not obvious to me that aContentLocation (this->mURI) should have been set in any of the functions before it reached nsMixedContentBlocker.cpp.

I have attached a patch.  I'm still testing it out and will push to try.
(In reply to Tanvi Vyas from comment #11)
> Created attachment 663487 [details] [diff] [review]
> WIP Patch
> 
> I can reproduce using gmail on a Mac.  But not gmail on Linux.  I went
> through my backtrace and traced through the code that resulted in the null
> aContentLocation.  It is not obvious to me that aContentLocation
> (this->mURI) should have been set in any of the functions before it reached
> nsMixedContentBlocker.cpp.
> 
> I have attached a patch.  I'm still testing it out and will push to try.

Adding johns, since he recently refactored nsObjectLoadingContent.cpp, and perhaps he may know if this->mURI should have been set there.
Another backtrace for gmail.com - http://pastebin.mozilla.org/1834889

The opportunity to set this->mURI is in one of these functions -
* CheckProcessPolicy() in nsObjectLoadingContent.cpp:1150
* LoadObject in nsObjectLoadingContent.cpp:1624
* LoadObject in nsObjectLoadingContent.cpp:1490
* StartObjectLoad in nsHTMLSharedObjectElement.cpp:475
* StartObjectLoad nsHTMLSharedObjectElement.cpp:112

The relevant part of the backtrace:
#5  0x0000000101b967c3 in nsObjectLoadingContent::CheckProcessPolicy (this=0x1180c1fa8, aContentPolicy=0x7fff5fbe9b4e) at /Users/tvyas/dev/mozilla-central/content/base/src/nsObjectLoadingContent.cpp:1150
#6  0x0000000101b93e36 in nsObjectLoadingContent::LoadObject (this=0x1180c1fa8, aNotify=true, aForceLoad=false, aLoadingChannel=0x0) at /Users/tvyas/dev/mozilla-central/content/base/src/nsObjectLoadingContent.cpp:1624
#7  0x0000000101b93783 in nsObjectLoadingContent::LoadObject (this=0x1180c1fa8, aNotify=true, aForceLoad=false) at /Users/tvyas/dev/mozilla-central/content/base/src/nsObjectLoadingContent.cpp:1490
#8  0x0000000101ed7bfb in nsHTMLSharedObjectElement::StartObjectLoad (this=0x1180c1f30, aNotify=true) at /Users/tvyas/dev/mozilla-central/content/html/content/src/nsHTMLSharedObjectElement.cpp:475
#9  0x0000000101eda8da in nsHTMLSharedObjectElement::StartObjectLoad (this=0x1180c1f30) at /Users/tvyas/dev/mozilla-central/content/html/content/src/nsHTMLSharedObjectElement.cpp:112
  if (!aContentLocation || NS_FAILED(aContentLocation->SchemeIs("https", &isHttps)) || isHttps) {
    return NS_OK;
  }


Should we return NS_OK or return NS_ERROR_FAILURE?  If we return NS_OK, would we be allowing a mixed content object load?  If we return NS_ERROR_FAILURE, would we be rejecting https object loads? 

Could this failure be occurring where the plugin is loading and not the actual http/https resource?  If so, then a null aContentLocation makes sense.

Still investigating.
aContentLocation can legitimately be null for plugins, for instance:

> <object type="application/x-java-vm">
>   <param name="code" value="applet.class">
> <object>

Will load java, but will not open a channel or have a URI associated with it. aRequestingLocation and aRequestContext should still be set if this is in a normal web context.
Also - for URI-less plugins, we will only call shouldProcess() (as we never 'load' a channel), but nsMixedContentBlocker's shouldProcess() is just a |return shouldLoad(...)| at the moment.
Created attachment 663602 [details] [diff] [review]
Patch

Thanks John for your help!  I've updated the patch to return OK if we have a plugin with a null aContentLocation.  It returns a failure when aContentLocation is not set for any other type.

Push to try:
https://tbpl.mozilla.org/?tree=Try&rev=449d808b2c5c
Attachment #663487 - Attachment is obsolete: true
Attachment #663602 - Flags: feedback?
Attachment #663602 - Flags: feedback? → feedback?(jschoenick)
(Assignee)

Updated

6 years ago
Attachment #663602 - Flags: feedback?(jschoenick) → review?(jschoenick)
(Assignee)

Updated

6 years ago
Attachment #663602 - Attachment description: WIP Patch → Patch
Comment on attachment 663602 [details] [diff] [review]
Patch

This looks fine, though we might want to put this check in shouldProcess instead, since shouldLoad should always have a URI regardless of type. (There's also a few missing brackets for the outer |if|)

I'm going to pass this to bz for review however, as I'm not too familiar with the CSP stuff.
Attachment #663602 - Flags: review?(jschoenick) → review?(bzbarsky)
Created attachment 664205 [details] [diff] [review]
Patch

Sorry I missed the brackets; not sure why that still worked.  I've updated to patch and re-pushed to try (https://tbpl.mozilla.org/?tree=Try&rev=f0cdc9d33a5b ).  I can move this code to ShouldProcess() if preferred.

r? to Olli, since he did the reviews for bug 62178.
Attachment #664205 - Flags: review?(bugs)
Comment on attachment 664205 [details] [diff] [review]
Patch

Um, nsIContentPolicy.idl explicitly says that aContentLocation must not be null.
Comment on attachment 664205 [details] [diff] [review]
Patch

But ok, let's take this.
Could you file a followup to get the documentation right or
fix the callers.
Attachment #664205 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #21)
> Comment on attachment 664205 [details] [diff] [review]
> Patch
> 
> But ok, let's take this.
> Could you file a followup to get the documentation right or
> fix the callers.

I think moving to ShouldProcess() fixes this, since the ShouldProcess() doesn't require aContentLocation to be non-null: https://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIContentPolicy.idl#216

Would you like me to move the code to ShouldProcess or file the followup?
Created attachment 664228 [details] [diff] [review]
null check in ShouldProcess()

checking if aContentLocation is null in ShouldProcess(), before we call ShouldLoad().
Attachment #663602 - Attachment is obsolete: true
Attachment #663602 - Flags: review?(bzbarsky)
(In reply to Tanvi Vyas from comment #22)
> (In reply to Olli Pettay [:smaug] from comment #21)
> > Comment on attachment 664205 [details] [diff] [review]
> > Patch
> > 
> > But ok, let's take this.
> > Could you file a followup to get the documentation right or
> > fix the callers.
> 
> I think moving to ShouldProcess() fixes this, since the ShouldProcess()
> doesn't require aContentLocation to be non-null:
> https://mxr.mozilla.org/mozilla-central/source/content/base/public/
> nsIContentPolicy.idl#216
> 
> Would you like me to move the code to ShouldProcess or file the followup?
Ah, move the check to ShouldProcess then.
Comment on attachment 664228 [details] [diff] [review]
null check in ShouldProcess()

Carrying over r+
Attachment #664228 - Flags: review+
(Assignee)

Updated

6 years ago
Attachment #664205 - Attachment is obsolete: true
Try looks good.  Can we land this patch?
https://hg.mozilla.org/mozilla-central/rev/e0dbda5b3ac4
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.