Only modify HTTP requests #10839

ghost · 2017-07-01T15:42:44Z

I don't think we have any HTTPS to HTTPS requests and downgrades are disabled so let's only add a HTTP request handler.

Bisaloo · 2017-07-01T16:46:55Z

Actually, there are rewrites https -> https. Now the question is: do we want to keep them?

grep 'from="^https' *.xml | wc -l

72

ghost · 2017-07-01T17:03:33Z

@Bisaloo Please send a list of files which have them. I may rewrite these rules as a part of this PR.

Bisaloo · 2017-07-01T17:29:27Z

7Headlines.com.xml
AdultShopping.com.xml
Aeriagames.xml
Agol.dk.xml
AJC.com.xml
Austin_Fit_Magazine.com.xml
basekit.com.xml
Blip.xml
BME.hu_incomplete.xml
BME.hu.xml
CafePress.xml
Caltech.xml
cambridgeshire.gov.uk.xml
Cashback.co.uk.xml
Cato-Institute.xml
Comic-Con-Intl.xml
Cru.org.xml
DailyDot.xml
Dell.xml
Demonoid.xml
Deutsche-Rentenversicherung.de.xml
Epic-Systems.xml
Epson.xml
Fasthosts.xml
First_Class_Magazine.se.xml
Ghostery_metrics_optout.xml
GoogleMaps.xml
Google.xml
InkFrog.com.xml
Intelliworks_Chat.com.xml
Kettering.gov.uk.xml
Khalsa-Credit-Union.xml
Kiva.org.xml
KKH.de.xml
Knappschaft-Bahn-See.xml
Linuxaria.com.xml
LiveJournal.xml
Minijob-zentrale.de.xml
MoveOn.xml
Mutterschaftsgeld.de.xml
National_Geographic.xml
Necuhb.xml
Ntop.org.xml
Numergy.xml
Pair-Networks.xml
Parkerpen.com.xml
Perfect_Audience.xml
Plymouth-University.xml
Sandbag.xml
Sberbank_of_Russia.xml
SPajIT.xml
spectator.co.uk.xml
Stack-Exchange.xml
Stallman.org.xml
Telus.xml
Tesco.xml
Unblocked.xml
Unity.xml
US-Dept-of-Veterans-Affairs.xml
VEVO.xml
Virus_Bulletin.xml
Yandex.xml
Yimg.com.xml
yWorks.com.xml
ZeniMax-Media.xml

Note that there are sometimes good reasons for these rewrites. Hopefully, it will documented more often than not.

As explained in the issue I linked earlier, these rules sometimes prevent breakages (such as use of protocole relative links //example.com). It's more a matter of policy whether we want to keep them or not.

ghost · 2017-07-01T17:30:59Z

Let's ask @Hainish and others' opinions.

jeremyn · 2017-07-01T17:38:29Z

I don't like rewriting HTTPS domains because that goes beyond HTTPS Everywhere's mission. At the same time there may be some edge cases I'm not thinking of where rewriting HTTPS is a good idea.

Probably the first step is to update each of the relevant rulesets, in separate PRs, like in #9842 (comment) for default_off rulesets. Then we can see what we're left with and what changes are appropriate or possible. Note that I'm not committing to reviewing these PRs myself, so it may be a while before someone looks at them.

ghost · 2017-07-01T17:59:25Z

Closing until #10843 is done.

cschanaj · 2017-07-02T01:36:29Z

I agree this is a policy decision, noted that we don't have any downgrade rule anymore, see #9621 (comment). IMHO, it is unneccessary to remove the legacy rewrite since the users are not exposed to plain-text connection because of this anyway. Let's see @Hainish opinion on this.

Besides, @koops76 why do you need to modify the code base for this? I think that the same goal can be achieved by updating the ruleset in #10843 .

P.S. I am rather conservative to changes in the extension code base because this extension is installed on the tor-browser by default.

ghost · 2017-07-02T10:26:14Z

@cschanaj
background.js:
https://github.com/EFForg/https-everywhere/pull/10839/files#diff-81d05cf2685349d7bbbe9f394bd74506R573

manifest.json:
https://github.com/EFForg/https-everywhere/pull/10839/files#diff-a951caac7ef604fdff31e05ef8ca45d4R40

var -> const/let replacement probably should have been in its own pull request.

cschanaj · 2017-07-02T10:29:06Z

@koops76 I think it will be much better to have a separate PR for the replacement.

ghost · 2017-07-02T10:49:37Z

@cschanaj I cleaned out all these var to const/let changes from this PR.

ghost · 2017-07-02T10:54:15Z

Again, closing until #10843 is done.

ghost · 2017-07-02T14:06:05Z

@cschanaj I moved JS style changes into #10852.

jeremyn · 2017-07-02T14:09:45Z

@cschanaj If nothing but HTTP is needed then I'm okay with restricting the permissions to just HTTP. This improves security, for example it would make it significantly more difficult for a malicious contributor to sneak an unexpected HTTPS rewrite into a ruleset.

If we make this change to the extension permissions, then we should also add a check that the rule from attribute begins with ^http. Otherwise a mistake will manifest as a permission error instead of a value error. We can do that in the XML DTD as described here. Eventually we can make the ^http optional/implied.

ghost · 2017-07-02T14:11:53Z

@jeremyn A malicious contributor can do anything, this change doesn't prevent malicious code modification.

jeremyn · 2017-07-02T14:17:06Z

A malicious contributor has to get their changes past a reviewer first. A reviewer might overlook something like this

<rule from="https://good\.example\.com/" to="https://evil.example.org/" />

buried in a large PR, especially if the PR requires a lot of discussion or is rebased/squashed by the contributor, but the reviewer will almost certainly notice unexpected changes to .js or .json files.

cschanaj · 2017-07-02T14:40:46Z

@jeremyn This is also true for HTTP rewrite, for example

<rule from="http://good\.example\.com/" to="https://good.example.org.localhost/" />

will block all the connection to good.example.com over plain-text, see also Ghostery_metrics_optout.xml#L6

IMHO, a probable way to remedy this is to restrict rewrite such that from and to have to be over the same domain (TLD plus one).

We should allow rewrite like (same domain)

<rule from="http://good\.example\.com/" to="https://secure.example.com/" />

but not (cross domain)

<rule from="http://good\.example\.com/" to="https://good.example.org/" />

Though I guess a significant number of existing rulesets will fail this requirement.

ghost · 2017-07-02T14:51:41Z

@cschanaj That would break lots of rules that redirect from a custom domain to a canonical CDN domain to fix certificate mismatch errors.

jeremyn · 2017-07-02T15:04:02Z

@cschanaj I agree that a reviewer could also miss a malicious HTTP->HTTPS rewrite, however HTTPS->HTTPS rewrites are potentially more dangerous, because even sites with poor HTTPS support will usually make sure critical stuff like logins go through HTTPS. There is almost no chance that a POST request to login.example.com with a user's name and password will go through HTTP, which means that it's safe from any malicious HTTP rewrites by HTTPS Everywhere.

cschanaj · 2017-07-02T15:28:54Z

@koops76 Yes, but a CDN can be used to serve malicious content as well.

P.S. Personally, I am unwilling to see #10839 (comment) implemented, since it will break too many existing rulesets.

@jeremyn I see your point that rewriting HTTPS might have some security impacts to the users. I agree that we should audit/ update existing ruleset with HTTPS->HTTPS. Supposed that we can get ride of all HTTPS->HTTPS rewrites in #10843, I agree that https-everywhere should rewrite HTTP request only.

ghost · 2017-07-02T15:41:36Z

@cschanaj I'm talking only about the rulesets that redirect from HTTP-only custom domain to a canonical CDN domain with matching certificate name.

jeremyn · 2017-07-02T15:55:14Z

FYI we have (collectively) discussed cross-domain rewrites, including CDN-related, several times, for example see the links in #7253 (comment).

cschanaj · 2017-07-02T16:04:10Z

@koops76

Never mind. IMHO, #10839 (comment) is not a good idea in the short term because we have limited manpower to review pending PRs. Supposed that we restrict cross domain rewrite, we have to either update/ whitelist existing ruleset. The pending PRs might block new ruleset from getting merged. Also, I am really reluctant to see more whitelist are created.

For the moment, it should be much easier to focus on removing HTTPS->HTTPS rewrites.

ghost · 2017-07-09T21:01:21Z

#77

koops76 added 5 commits July 1, 2017 18:27

Update manifest.json

7a48d1f

Update background.js

26f9364

var -> let/const

fc7ef54

Update manifest.json

0674eda

Update background.js

7e9f4c3

ghost changed the title ~~Only modify HTTP requests.~~ Only modify HTTP requests Jul 1, 2017

ghost mentioned this pull request Jul 1, 2017

If possible, rewrite the rulesets in a way that does not require HTTPS->HTTPS rules. #10843

Closed

69 tasks

ghost closed this Jul 1, 2017

Undo var -> let/const

25a47da

ghost reopened this Jul 2, 2017

koops76 added 3 commits July 2, 2017 13:46

Update background.js

e0f785e

Update background.js

be30608

Update background.js

9793547

ghost closed this Jul 2, 2017

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only modify HTTP requests #10839

Only modify HTTP requests #10839

ghost commented Jul 1, 2017

Bisaloo commented Jul 1, 2017

ghost commented Jul 1, 2017

Bisaloo commented Jul 1, 2017

ghost commented Jul 1, 2017

jeremyn commented Jul 1, 2017

ghost commented Jul 1, 2017

cschanaj commented Jul 2, 2017 •

edited

Loading

ghost commented Jul 2, 2017

cschanaj commented Jul 2, 2017

ghost commented Jul 2, 2017

ghost commented Jul 2, 2017

ghost commented Jul 2, 2017

jeremyn commented Jul 2, 2017

ghost commented Jul 2, 2017 •

edited by ghost

Loading

jeremyn commented Jul 2, 2017 •

edited

Loading

cschanaj commented Jul 2, 2017 •

edited

Loading

ghost commented Jul 2, 2017

jeremyn commented Jul 2, 2017

cschanaj commented Jul 2, 2017

ghost commented Jul 2, 2017 •

edited by ghost

Loading

jeremyn commented Jul 2, 2017

cschanaj commented Jul 2, 2017 •

edited

Loading

ghost commented Jul 9, 2017

Only modify HTTP requests #10839

Only modify HTTP requests #10839

Conversation

ghost commented Jul 1, 2017

Bisaloo commented Jul 1, 2017

ghost commented Jul 1, 2017

Bisaloo commented Jul 1, 2017

ghost commented Jul 1, 2017

jeremyn commented Jul 1, 2017

ghost commented Jul 1, 2017

cschanaj commented Jul 2, 2017 • edited Loading

ghost commented Jul 2, 2017

cschanaj commented Jul 2, 2017

ghost commented Jul 2, 2017

ghost commented Jul 2, 2017

ghost commented Jul 2, 2017

jeremyn commented Jul 2, 2017

ghost commented Jul 2, 2017 • edited by ghost Loading

jeremyn commented Jul 2, 2017 • edited Loading

cschanaj commented Jul 2, 2017 • edited Loading

ghost commented Jul 2, 2017

jeremyn commented Jul 2, 2017

cschanaj commented Jul 2, 2017

ghost commented Jul 2, 2017 • edited by ghost Loading

jeremyn commented Jul 2, 2017

cschanaj commented Jul 2, 2017 • edited Loading

ghost commented Jul 9, 2017

cschanaj commented Jul 2, 2017 •

edited

Loading

ghost commented Jul 2, 2017 •

edited by ghost

Loading

jeremyn commented Jul 2, 2017 •

edited

Loading

cschanaj commented Jul 2, 2017 •

edited

Loading

ghost commented Jul 2, 2017 •

edited by ghost

Loading

cschanaj commented Jul 2, 2017 •

edited

Loading