Closed Bug 939180 Opened 11 years ago Closed 11 years ago

Crash with HTTPS Everywhere

Categories

(Core :: General, defect)

Product:
Core
Component:
General
Platform:
x86_64
All
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: jag.alves, Unassigned)

References

(
URL
)

Details

(Keywords: crash)

Crash Data

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20131115030203

Steps to reproduce:

I have HTTPS 4.0development.13 installed.

Went to http://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/windows8.html



Actual results:

Crash: https://crash-stats.mozilla.com/report/index/cb565345-a68c-4bf9-ada3-180312131114


Expected results:

no crash
Note: doesn't crash in safe mode.
URL: http://people.mozilla.org/~shorlander...
Crashes with stable release 3.4.2 too.
In a debug Nightly there this assertion:

###!!! ASSERTION: You can't dereference a NULL nsRefPtr with operator->().: 'mRawPtr != 0', file c:\builds\moz2_slave\m-cen-w32-d-000000000000000000\build\obj-firefox\dist\include\nsAutoPtr.h, line 1035


(In reply to jorge alves from comment #0)
> I have HTTPS 4.0development.13 installed.

For reference, it's HTTPS Everywhere on https://www.eff.org/https-everywhere
Crash Signature: [@ nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::Length() | nsExternalResourceMap::AddExternalResource(nsIURI*, nsIContentViewer*, nsILoadGroup*, nsIDocument*) ]
Component: Untriaged → General
Keywords: crash, regressionwindow-wanted
Product: Firefox → Core
Summary: Crash with HttpsEverywhere → Crash with HTTPS Everywhere
HTTPS everywhere is modifying a the URI of an HTTP channel in a ... bizarre way.  The URL we initially open is:

  https://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/effects.svg 

but by the time the channel sends OnStartRequest that same URI object we had before has gotten mutated to:

  https://people.mozilla.com/~shorlander/mockups-interactive/australis-interactive-mockups/effects.svg

Note the "org" we used to have and the "com" we have now.

This is NOT the channel getting redirected; this is the exact URI object that got passed to newChannel getting mutated under the caller.  That sort of thing is not ok, and in this case it violates totally reasonable assumptions about being able to use the URI as a hashtable key.

Jorge, could we find out what exactly this addon is doing here and why?  :(

It's not clear that this is a regression, by the way; the relevant code on our side certainly hasn't changed in a long time.  This might be the first time someone running the broken addon has loaded a page with external SVG resources, though.
Flags: needinfo?(jorge)
Adding our HTTPS Everywhere contacts. Jorge, can you confirm that this is a development version and not a release version causing the crash?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(jorge) → needinfo?(jag.alves)
The original report is with HTTPS Everywhere 4.0development.13 and Nightly.

I just tested v3.4.2 (stable) with Firefox 25.0.1 and it also crashed. Error report:
https://crash-stats.mozilla.com/report/index/874199ff-39cc-4429-9ed7-a903d2131118
Flags: needinfo?(jag.alves)
Crashes with 26.0b5 Linux-x86_64 too, but without a crash report (segfault).
OS: Windows 8.1 → All
crashes with firefox-3.7a1pre.2009-09-01-03.en-US.linux-x86_64 (2009-07-02-06 and earlier ≈3.5 builds don't work here; minVersion is 3.5)
Severity: normal → critical
Version: 28 Branch → Trunk
Thanks for the heads up.  I probably won't be able to get to this until Thursday or Friday this week.  We have a few weeks to land a fix, right?
Well, current stable HTTPS Everywhere (3.4.2) crashes with current stable Firefox (25.0.1).  So I'm not sure about "a few weeks"...
OK, that's a first-rate emergency.  I'll cancel some plans and start looking at it tonight.
The good news is that it's a null-deref crash, so at least safe.  I suspect Thursday or Friday this week would in fact be fine.  Just would rather not drag it out for many weeks...
OK.  Doing some bisecting, it seems that the underlying bug exists in HTTPS E at least as far back as 2.2.3 (although in our 2.x releases the rulesets didn't trigger jorge's example).  Maximum verbosity logs from within HTTPS E show these crash circumstances in 2.2.3 (but the modern Mozilla ruleset):

Potentially applicable rules for people.mozilla.org:
  Mozilla
Rewrote https://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/images-win8/bookmarkPanel-icon-bookmarksToolbar.png
active rule Mozilla in https://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/windows8.html -> https://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/windows8.html serial 16
https://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/images-win8/bookmarkPanel-icon-bookmarksToolbar.png: Redirection limit is 19
Found nsIHttpChannel.redirectTo. Using it.
Segmentation fault (core dumped)

4.0development.13 crash circumstances look like this:

Processing https://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/images-win8/tabSeparator.png
Potentially applicable rules for people.mozilla.org:
  Mozilla
Rewrote https://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/images-win8/tabSeparator.png
active rule Mozilla in https://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/windows8.html -> https://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/windows8.html serial 19
https://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/images-win8/tabSeparator.png: Redirection limit is 19
Using nsIHttpChannel.redirectTo: https://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/images-win8/tabSeparator.png -> https://people.mozilla.com/~shorlander/mockups-interactive/australis-interactive-mockups/images-win8/tabSeparator.png
SSL Observatory: SHA-256 hash of cert chain for people.mozilla.com is 91CB7FE304471F9946485C2ED5811F146EDECC2B2CFB1E13F80EBE07F3DFF381
SSL Observatory: Cert chain is NOT whitelisted. Proceeding with submission.
SSL Observatory: Got root cert at position: 2
SSL Observatory: Already submitted cert for people.mozilla.com. Ignoring
Got http-on-examine-response @ https://people.mozilla.com/~shorlander/mockups-interactive/australis-interactive-mockups/images-win8/toolbarButton-customize.png
SSL Observatory: SHA-256 hash of cert chain for people.mozilla.com is 91CB7FE304471F9946485C2ED5811F146EDECC2B2CFB1E13F80EBE07F3DFF381
SSL Observatory: Cert chain is NOT whitelisted. Proceeding with submission.
SSL Observatory: Got root cert at position: 2
SSL Observatory: Already submitted cert for people.mozilla.com. Ignoring
Got http-on-examine-response @ https://people.mozilla.com/~shorlander/mockups-interactive/australis-interactive-mockups/images-win8/bookmarkEditPanel-icon-header.png
SSL Observatory: SHA-256 hash of cert chain for people.mozilla.com is 91CB7FE304471F9946485C2ED5811F146EDECC2B2CFB1E13F80EBE07F3DFF381
SSL Observatory: Cert chain is NOT whitelisted. Proceeding with submission.
SSL Observatory: Got root cert at position: 2
SSL Observatory: Already submitted cert for people.mozilla.com. Ignoring
Got http-on-examine-response @ https://people.mozilla.com/~shorlander/mockups-interactive/australis-interactive-mockups/images-win7/menuPanel-arrowUp.png
SSL Observatory: SHA-256 hash of cert chain for people.mozilla.com is 91CB7FE304471F9946485C2ED5811F146EDECC2B2CFB1E13F80EBE07F3DFF381
SSL Observatory: Cert chain is NOT whitelisted. Proceeding with submission.
SSL Observatory: Got root cert at position: 2
SSL Observatory: Already submitted cert for people.mozilla.com. Ignoring
Got http-on-examine-response @ https://people.mozilla.com/~shorlander/mockups-interactive/australis-interactive-mockups/images-win8/dropdownArrow.png
Segmentation fault (core dumped)

or like this, when I disable the observatory:

Potentially applicable rules for people.mozilla.org:
  Mozilla
Rewrote https://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/effects.svg
https://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/effects.svg: Redirection limit is 19
Using nsIHttpChannel.redirectTo: https://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/effects.svg -> https://people.mozilla.com/~shorlander/mockups-interactive/australis-interactive-mockups/effects.svg
Segmentation fault (core dumped)
So is it possible that HTTPS E just gets the channel's .URI, then modifies it and passes the result to redirectTo?

If so, the fix should be just cloning the URI, then modifying the clone instead.
More bisection data: I was able to reproduce this in Firefox 24 and Firefox 20, with https-e 4.0development.13.
> I was able to reproduce this in Firefox 24 and Firefox 20

Right; the SVG external resource code has been around for a long while.

Let me see if I can get a stack to where the URI is getting modified.
Ah, so the mutation is done from the content policy callback, before the HTTP channel is even created.  Unfortunately, it's done _after_ some metadata about the load is added to a hashtable using the URI as key.

The stack to the mutation when using 4.0development13 is:

0 anonymous(old_uri = [xpconnect wrapped nsIURI @ 0x114fdf240 (native @ 0x11cbc5308)], new_uri = [xpconnect wrapped (nsISupports, nsIStandardURL, nsIURI) @ 0x114fdf2b0 (native @ 0x114c41d08)]) ["chrome://https-everywhere/content/code/HTTPS.js":101]
    this = [object Object]
1 anonymous(uri = [xpconnect wrapped nsIURI @ 0x114fdf240 (native @ 0x11cbc5308)], fallback = null, ctx = [object XrayWrapper [object HTMLDivElement]]) ["chrome://https-everywhere/content/code/HTTPS.js":149]
    this = [object Object]
2 anonymous(aContentType = 1, aContentLocation = [xpconnect wrapped nsIURI @ 0x114fdf240 (native @ 0x11cbc5308)], aRequestOrigin = [xpconnect wrapped nsIURI @ 0x1152855c0 (native @ 0x11d46b008)], aContext = [object XrayWrapper [object HTMLDivElement]], aMimeTypeGuess = "", aInternalCall = null, unwrappedLocation = [xpconnect wrapped (nsISupports, nsIPrincipal, nsISerializable) @ 0x115285da0 (native @ 0x110a80180)]) ["file:///tmp/polj/extensions/https-everywhere@eff.org/components/https-everywhere.js":618]
    this = [object Object]

which is the rewriteInPlace function being called from content policy.  I assume this is purposeful behavior?

I suppose on our end we could mark the URI we're using immutable.  But then rewriteInPlace would throw, forceURI would catch, and then what would happen?

Or we could store a clone of the URI in mURI, making sure we clone it before calling the content policy code....

Adding ourselves to the pending load table with the post-rewrite hostname would be wrong, unfortunately, so we can't just do that.

By the way, why exactly is this rewriting https://people.mozilla.org to https://people.mozilla.com anyway?
Flags: needinfo?(pde-lists)
Probably the rewriteInPlace code that tries to mutate the URI from nsIContentPolicy can be deprecated in versions of Firefox that have nsIHTTPChannel::RedirecTo, which is Firefox >= 20.  That was on our "to do one day" list.

We're going to need to do some testing to make sure that our code is functioning correctly after we turn that off, though.  I think Seth Schoen has a test infrastructure for that...
Flags: needinfo?(pde-lists)
OK, I can confirm that disabling all of our nsIContentPolicy bindings fixes the bug and doesn't obviously leak any HTTP requests that should be rewritten.  (this is a working fix: https://gitweb.torproject.org/pde/https-everywhere.git/commitdiff/724aed1f13aa7c384c6069535a62a27c401e7b62)

Seth, can you run an extensive test on that with your wireshark framework?  

We will also need to decide whether to flag HTTPS E as incompatible with firefox < 20, or whether to keep the Content Policy code around behind some conditional tests for those older browser versions.
Per https://www.eff.org/files/Changelog.txt this is now fixed with HTTPS Everywhere 4.0development.14 and 3.4.3.

I can confirm this for the 4.0development.14 case against

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131204030203 CSet: 9688476c1544 and
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20131202182626 CSet: f8d0c1b35b42

Didn't try other combinations/versions.
Keywords: regressionwindow-wanted
That's not good at all.  With the changes I made in 3.4.3 and 4.0development.14, I can no longer reproduce the crash via
http://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/windows8.html

Do you see the crash at that URL?  Or have another way to trigger it?
Ummm, I meant in my comment that I can confirm the changelog, i.e. the Issue/Crash being fixed and *not* meaning the issue still happening despite the changelog.

sorry for not making this more clear :-)
Verified fixed in my test cases as well.
Grouse.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.