1489882 - Beta and Nightly completely break Decentraleyes by preventing redirections to Web Accessible Resources

Status: RESOLVED WONTFIX

(WebExtensions :: Request Handling, defect)

Description

[Synzvato]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180712150008

Steps to reproduce:

I'm hereby reporting a severe issue that not only renders Decentraleyes fully ineffective, but also breaks any websites Decentraleyes attempts to interact with. This problem, at the time of writing, affects the latest Firefox Beta (63.0b4) and Nightly (64.0a1 2018-09-09). It also occurs on fresh profiles.

It's worth noting that the last Firefox Nightly build that worked was "2018-08-30-11-17-45" [1], and that the breaking changes seem to have been first introduced in Nightly "2018-08-30-22-01-10" [2]. The bug is easily reproducible, and should affect all users of recent Betas and Nightlies [3][4].

To reproduce the bug in a relatively quick and easy way:
  * install Decentraleyes on one of the aforementioned affected releases;
  * navigate to the Testing Utility [5] and view the test results.


Actual results:

Decentraleyes listens to the "webRequest.onBeforeRequest" listener [6], and returns a "BlockingResponse" object. So, it essentially instructs Firefox to internally redirect valid candidates to local "Web Accessible Resources" [7]. The returned redirect URL was valid, but Firefox ≥63 refused to act.


Expected results:

The valid "redirectURL" should have resulted in a local redirection by Firefox to the (registered) "Web Accessible Resource".

[1] https://ftp.mozilla.org/pub/firefox/nightly/2018/08/2018-08-30-11-17-45-mozilla-central/
[2] https://ftp.mozilla.org/pub/firefox/nightly/2018/08/2018-08-30-22-01-10-mozilla-central/
[3] https://git.synz.io/Synzvato/decentraleyes/issues/313
[4] https://www.reddit.com/r/firefox/comments/9ebo8c/the_decentraleyes_extension_has_stopped_working/
[5] https://decentraleyes.org/test/
[6] https://git.synz.io/Synzvato/decentraleyes/blob/master/core/state-manager.js#L112
[7] https://git.synz.io/Synzvato/decentraleyes/blob/master/core/interceptor.js#L112

Comment 1

[Johannes Pfrang [:johnp]]

I can mozregression in bug 1489875, duping to this one.

Comment 3

[Johannes Pfrang [:johnp]]

(/s/can/ran) & regressing changeset for completeness: https://hg.mozilla.org/integration/mozilla-inbound/rev/64696387b4b0

Comment 4

[:Gijs (he/him)]

(In reply to Synzvato from comment #0)
> Decentraleyes listens to the "webRequest.onBeforeRequest" listener [6], and
> returns a "BlockingResponse" object. So, it essentially instructs Firefox to
> internally redirect valid candidates to local "Web Accessible Resources"
> [7]. The returned redirect URL was valid, but Firefox ≥63 refused to act.

Can you provide some examples of the request URIs and the redirected-to-URIs? That is, what is the original request URI, and where does the add-on redirect the request to?

Comment 5

[:Gijs (he/him)]

In fact, maybe I've spotted the problem. Is it possible the filename of the targets of the redirects has the `.dec` extension like in the source tree of the add-on?

If so, that's the cause of your issue. If the filenames had the same extension as the URI you were redirecting, things would work.

Why do you need the magical '.dec' extension?

Comment 6

[Synzvato]

(In reply to :Gijs (he/him) from comment #5)

> Can you provide some examples of the request URIs and the redirected-to-URIs?

Example 1:

Original Request URI: https://ajax.aspnetcdn.com/ajax/jquery/jquery-1.12.3.min.js
Redirected-to-URI: moz-extension://<extension_uuid>/resources/jquery/1.12.3/jquery.min.js.dec

Example 2:

Original Request URI: https://code.jquery.com/jquery-1.12.4.min.js
Redirected-to-URI: moz-extension://<extension_uuid>/resources/jquery/1.12.4/jquery.min.js.dec

> Why do you need the magical '.dec' extension?

The ".dec" extension was adopted in order to work around "add-ons linter" limitations. Said linter aims to prevent add-ons from internally depending on vulnerable libraries. However, it only scans for presence, and not actual usage.

For instance, Decentraleyes does not internally depend on jQuery "v1.11.3", but does need to be able to inject it into any page that attempts to fetch that very same resource from a delivery network. The linter cannot tell the difference.

Apart from vulnerability checks, renaming the files kept the linter from attempting (and then silently failing) to analyze all resources [1]. The extremely high amount of bundled scripts caused the operation to time out consistently.

Changing the resource extensions to ".js" is not a viable solution, as it would leave me unable to publish the add-on.

[1] https://github.com/mozilla/addons-linter/blob/c0af4be081a394f254416f8d2bd047b7775ba661/src/linter.js#L590

Comment 7

[:Gijs (he/him)]

(In reply to Synzvato from comment #6)
> Changing the resource extensions to ".js" is not a viable solution, as it
> would leave me unable to publish the add-on.

Can you at least verify locally whether that fixes the issue you're having here?

Comment 8

[:Gijs (he/him)]

I'm fairly sure that just using the `.js` extension would unbreak things here. Given that, some comments. If I'm wrong, I can reconsider, but in the meantime:

(In reply to Synzvato from comment #6)
> (In reply to :Gijs (he/him) from comment #5)
> > Why do you need the magical '.dec' extension?
> 
> The ".dec" extension was adopted in order to work around "add-ons linter"
> limitations. 

Oh. Well, that's interesting...

> Said linter aims to prevent add-ons from internally depending
> on vulnerable libraries. However, it only scans for presence, and not actual
> usage.

Of course, because "usage" is pretty hard to 100%-accurately determine in JS without solving the halting problem, and also why would you ship files in your add-on that you don't use?

> For instance, Decentraleyes does not internally depend on jQuery "v1.11.3",
> but does need to be able to inject it into any page that attempts to fetch
> that very same resource from a delivery network.

I want to clearly point out that you're shifting goalposts here. The previous sentence talks about "usage", and now you say "internally depends". That's because obviously your add-on *does use* all these scripts - they get inserted into webpages the user browses, so they get run in that context much like page content scripts would, and that represents risk.

I know that you're saying "that very same resource", ie the page would have run that script anyway (but served from a CDN), but we have no way of knowing this without manual review as well as checking the checksums for all the scripts. And today, with the linter bypassed and the scripts minified, who knows whether the resources your add-on includes are true copies of the ones served by the CDN. That's a pretty scary point, because it means that anyone who installs the add-on effectively trusts it to run arbitrary, unlinted code instead of the CDN'd versions. Who would know if you (or a malicious clone of your add-on) inserted 'phone home' or password/credit-card scraping code in your add-on? Certainly not the linter, because you've just bypassed it, and to audit the code would mean going through hundreds of minified scripts.

Even if we assume that you and your add-on are so virtuous that we don't care about doing any checks here, the tightening of restrictions on the gecko side that caused this will also trip up more malicious add-ons that are using this loophole to bypass the linter, which feels like a good (or at least, not a bad) thing to do anyway.

> Apart from vulnerability checks, renaming the files kept the linter from
> attempting (and then silently failing) to analyze all resources [1]. The
> extremely high amount of bundled scripts caused the operation to time out
> consistently.

Did you report this to the add-ons team? I can't find a github issue about it on the linter repo you linked...


The "regressing" change here wasn't aimed at add-ons, but was addressing a different security issue. However, as far as I can see, making it harder to bypass the linter is a feature, not a bug, so I'm inclined to mark wontfix/invalid.

I'm not an AMO reviewer (and haven't been for a few years now), but I would suggest you talk to them about how to proceed. It seems to me that you have a number of different options, but I'm also pretty concerned you're willfully (and, based on the responses so far, unilaterally ie without discussion with AMO folks) bypassing review checks that are in place. I don't know what our policies are on that off-hand. Jorge, can you chime in please?

Comment 9

[Synzvato]

(In reply to :Gijs (he/him) from comment #7)

> Can you at least verify locally whether that fixes the issue you're having here?

I can confirm that locally changing the resource extensions from ".dec" to ".js" does resolve the issue. All tests were performed using (packed) XPIs, since the bug does not occur when the add-on is in an unpacked state (which is not surprising given that this involves "nsJARChannel"). The browser used was Nightly 64.0a1 (2018-09-09). I hope this helps.

Comment 10

[Jorge Villalobos [:jorgev] (he/him)]

-> ni TheOne for review perspective. It sounds like the '.dec' workaround was coordinated with reviewers. Note that he's out until Friday.

As for how to move forward, it'd be good to know if this is considered a bug in Firefox or not. If it isn't, then we'll need to figure out some other way to support Decentraleyes while they ship old libraries.

Comment 11

[Kris Maglione [:kmag]]

(In reply to Jorge Villalobos [:jorgev] from comment #10)
> As for how to move forward, it'd be good to know if this is considered a bug
> in Firefox or not. If it isn't, then we'll need to figure out some other way
> to support Decentraleyes while they ship old libraries.

I think the new behavior is correct, and I'd rather we not change it.

Comment 12

[Synzvato]

The extension has been reviewed extensively by various AMO reviewers. The resource bundle implementation has been discussed at great length over the past years. Said reviewers manually verify resource integrity using a dedicated script [1].

[1] https://git.synz.io/Synzvato/decentraleyes/tree/cbfeead5ce860084c864db7184ae10b8ca9328c4/audit

Comment 13

[:Gijs (he/him)]

(In reply to Kris Maglione [:kmag] from comment #11)
> (In reply to Jorge Villalobos [:jorgev] from comment #10)
> > As for how to move forward, it'd be good to know if this is considered a bug
> > in Firefox or not. If it isn't, then we'll need to figure out some other way
> > to support Decentraleyes while they ship old libraries.
> 
> I think the new behavior is correct, and I'd rather we not change it.

I would tend to agree. I'm also a little confused:

(In reply to Jorge Villalobos [:jorgev] from comment #10)
> -> ni TheOne for review perspective. It sounds like the '.dec' workaround
> was coordinated with reviewers. Note that he's out until Friday.

It's the 10th today, his bugzilla status says he's out until the 24th, 2 weeks from now, so I think we should try to move on this sooner.

Comment 14

[Kris Maglione [:kmag]]

(In reply to Synzvato from comment #12)
> The extension has been reviewed extensively by various AMO reviewers. The
> resource bundle implementation has been discussed at great length over the
> past years. Said reviewers manually verify resource integrity using a
> dedicated script [1].

If that's the case, then the linter shouldn't be a problem. Reviewers can override linter errors, and if a special process is required anyway, that shouldn't add significant extra overhead.

Comment 15

[Jorge Villalobos [:jorgev] (he/him)]

> It's the 10th today, his bugzilla status says he's out until the 24th, 2 weeks from now, so I think we should try to move on this sooner.

He's back on Friday and then back out. I'll make sure he reads this :). Judging by Kris's reply, it sounds like this bug should be closed either way.

> Reviewers can override linter errors, and if a special process is required anyway, that shouldn't add significant extra overhead.

The only process we could introduce now is for admins to upload all version updates for the extension, which isn't insignificant. Alternatively, we would need to introduce some exception for this particular extension, which also isn't great. We'll need to discuss how to do this.

Comment 16

[Synzvato]

(In reply to :Gijs (he/him) from comment #8)

> I know that you're saying "that very same resource", ie the page would have
> run that script anyway (but served from a CDN), but we have no way of
> knowing this without manual review as well as checking the checksums for all
> the scripts. And today, with the linter bypassed and the scripts minified,
> who knows whether the resources your add-on includes are true copies of the
> ones served by the CDN. That's a pretty scary point, because it means that
> anyone who installs the add-on effectively trusts it to run arbitrary,
> unlinted code instead of the CDN'd versions. Who would know if you (or a
> malicious clone of your add-on) inserted 'phone home' or
> password/credit-card scraping code in your add-on? Certainly not the linter,
> because you've just bypassed it, and to audit the code would mean going
> through hundreds of minified scripts.

> Even if we assume that you and your add-on are so virtuous that we don't care
> about doing any checks here [...].

Given that this has been discussed extensively with AMO reviewers, and the fact that they have been using a dedicated script to manually verify library integrity since the inception of Decentraleyes [1], could you please rectify these remarks?

[1] https://git.synz.io/Synzvato/decentraleyes/tree/cbfeead5ce860084c864db7184ae10b8ca9328c4/audit

Comment 17

[:Gijs (he/him)]

(In reply to Synzvato from comment #16)
> (In reply to :Gijs (he/him) from comment #8)
> 
> > I know that you're saying "that very same resource", ie the page would have
> > run that script anyway (but served from a CDN), but we have no way of
> > knowing this without manual review as well as checking the checksums for all
> > the scripts. And today, with the linter bypassed and the scripts minified,
> > who knows whether the resources your add-on includes are true copies of the
> > ones served by the CDN. That's a pretty scary point, because it means that
> > anyone who installs the add-on effectively trusts it to run arbitrary,
> > unlinted code instead of the CDN'd versions. Who would know if you (or a
> > malicious clone of your add-on) inserted 'phone home' or
> > password/credit-card scraping code in your add-on? Certainly not the linter,
> > because you've just bypassed it, and to audit the code would mean going
> > through hundreds of minified scripts.
> 
> > Even if we assume that you and your add-on are so virtuous that we don't care
> > about doing any checks here [...].
> 
> Given that this has been discussed extensively with AMO reviewers, and the
> fact that they have been using a dedicated script to manually verify library
> integrity since the inception of Decentraleyes [1], could you please rectify
> these remarks?

Unfortunately, bmo doesn't have comment editing functionality, so I can't change the comment, only collapse (not delete) it in its entirety (ie not just the bit you quoted) which I don't think makes sense here.

I'm happy if there are review processes in place to ensure this doesn't become a problem for the add-on you're maintaining - that there was an audit script or that the workaround was done after discussion with AMO folks hadn't been mentioned at the time I made the comment you cited.

That doesn't really alter the state of the world for other add-ons though. Per the earlier comments from Kris and Jorge, I'm going to close this as wontfix. I don't think we should make changes in Firefox to address this. I'm sure the add-ons team will work with you to come up with a workable solution to continue to publish Decentraleyes and have it work on 63 and later, and I'm happy to help you/them do so - but this bug isn't the right place as any changes won't be part of Firefox or its webextension implementation, but will affect the add-on and/or the linter.

Comment 18

[Synzvato]

(In reply to :Gijs (he/him) from comment #17)

Many thanks for the clarification. Much appreciated! Altering the verification procedure (instead of internal browser logic) sounds like a good idea. Thanks for offering your help, and I'll be sure to further discuss this with the add-ons team.

Comment 19

[Kris Maglione [:kmag]]

(In reply to Jorge Villalobos [:jorgev] from comment #15)
> > Reviewers can override linter errors, and if a special process is required anyway, that shouldn't add significant extra overhead.
> 
> The only process we could introduce now is for admins to upload all version
> updates for the extension, which isn't insignificant. Alternatively, we
> would need to introduce some exception for this particular extension, which
> also isn't great. We'll need to discuss how to do this.

All other things being equal, I would agree. But given that the review process for this extension seems to require special attention from an admin reviewer, anyway, this doesn't seem like something would add considerably to it.