Crash with HTTPS Everywhere

RESOLVED FIXED

Status

()

--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jag.alves, Unassigned)

Tracking

({crash})

Trunk
x86_64
All
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(crash signature, URL)

(Reporter)

Description

5 years ago
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
(Reporter)

Comment 1

5 years ago
Note: doesn't crash in safe mode.

Comment 2

5 years ago
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)
(Reporter)

Comment 6

5 years ago
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)

Comment 7

5 years ago
Crashes with 26.0b5 Linux-x86_64 too, but without a crash report (segfault).
OS: Windows 8.1 → All

Comment 8

5 years ago
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)

Updated

5 years ago
Severity: normal → critical
Version: 28 Branch → Trunk

Comment 9

5 years ago
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"...

Comment 11

5 years ago
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...

Comment 13

5 years ago
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.

Comment 15

5 years ago
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)

Comment 19

5 years ago
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)

Comment 20

5 years ago
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

Comment 22

5 years ago
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 :-)
(Reporter)

Comment 24

5 years ago
Verified fixed in my test cases as well.

Comment 25

5 years ago
Grouse.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.