1015314 - Change threshold for self-xss changes

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Manish Goregaokar [:manishearth]]

Bug 994134 has landed now.

There were discussions on restricting the change to Release and Beta. Should we go through with this?

Comment 1

[Manish Goregaokar [:manishearth]]

Attachment: Patch for branding changes

Comment 2

[Rob Campbell [:rc] (:robcee)]

I feel like 10 times is too high a bar for this.

Can we lower the pref to 3? There are valid reasons to copy and paste text into scratchpads and consoles that this is going to make annoying.

thanks!

Comment 3

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Rob Campbell [:rc] (:robcee) from comment #2)
> I feel like 10 times is too high a bar for this.

10 is what chrome is doing. I could get behind fewer, I agree it will be a bit annoying. IIRC 10 refers to the number of console history items?

> Can we lower the pref to 3? There are valid reasons to copy and paste text
> into scratchpads and consoles that this is going to make annoying.

Totally agree. or 5?

Comment 4

[Manish Goregaokar [:manishearth]]

> 10 is what chrome is doing. I could get behind fewer, I agree it will be a
> bit annoying. IIRC 10 refers to the number of console history items?

Currently, Chrome is doing nothing, they're waiting on a "Developer Mode" feature; opinions are divided on that. The original patch had 10, but it was rolled back later.
 
> Totally agree. or 5?

I like 5 too.

Comment 5

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Manish Goregaokar [:manishearth] from comment #4)
...
> Currently, Chrome is doing nothing, they're waiting on a "Developer Mode"
> feature; opinions are divided on that. The original patch had 10, but it was
> rolled back later.

Ah interesting, hadn't realized that. Still think this is worth doing.

Comment 6

[Joe Walker [:jwalker]]

Chromium bug https://code.google.com/p/chromium/issues/detail?id=345205

Comment 8

[Andrea Giammarchi]

re-commenting about this here:

This is ultra annoying specially on an about:blank page … I think the warning might even be OK as it is as long as this is shown only on https websites where eventually there could be a concrete problem.

Having this once (or 10 times) on about:blank on Nightly and then never again even in https or secured websites won't benefit anyone resulting instead just annoying and also pointless if security was a concern in real sites.

Last, but not least, I really don't think browsers should be responsible for this, 'cause you put yourself in a place where users might even end up blaming you if the code they copied and pasted from a site that told them to also write 'allow pasting' before pasting caused problems to their account.

I'd rather follow Safari approach, where developer tools must be enabled explicitly and probably that's the only moment you want to show a warning, letting users agree about the fact they can cause troubles to themselves and the browser or the devtools is not responsible for that.

Thanks David for filing the other bug, and thanks for listening Mozillians.

Best Regards

Comment 9

[Manish Goregaokar [:manishearth]]

Attachment: Updated patch, threshold lowered to 5

Updated the patch to lower the threshhold to 5.

I think this has simmered for long enough in Nightly without any bugs (except for the formatting one which I'll get around to), time to push this through.

Comment 10

[Manish Goregaokar [:manishearth]]

(In reply to Andrea Giammarchi from comment #8)
> re-commenting about this here:
> 
> This is ultra annoying specially on an about:blank page … I think the
> warning might even be OK as it is as long as this is shown only on https
> websites where eventually there could be a concrete problem.

I don't see where HTTPS comes in here. Self-XSS is a much easier exploit than MITMing HTTP; especially since self-xss is something that can be designed to propagate itself. MITM is a local attack, unless you control an ISP or something.

There *is* a concrete problem already, this has even affected banks in the past.

> Having this once (or 10 times) on about:blank on Nightly and then never
> again even in https or secured websites won't benefit anyone resulting
> instead just annoying and also pointless if security was a concern in real
> sites.

Agreed, that's why it's being removed from Nightly :)

> Last, but not least, I really don't think browsers should be responsible for
> this, 'cause you put yourself in a place where users might even end up
> blaming you if the code they copied and pasted from a site that told them to
> also write 'allow pasting' before pasting caused problems to their account.

The warning is clear enough, I would say, especially about the not understanding bit. I don't think that anyone can blame Firefox if they blindly follow instructions even after being presented with that warning.

> I'd rather follow Safari approach, where developer tools must be enabled
> explicitly and probably that's the only moment you want to show a warning,
> letting users agree about the fact they can cause troubles to themselves and
> the browser or the devtools is not responsible for that.

AFAICT Firefox is not going to go down the "Developer Mode" road (Chrome might), so this is ruled out.

Plus, this is a specific warning. "Developer Mode" can at most be a generic warning; less effective against the problem at hand. This is one of the major scams involving devtools (perhaps the only one), so a specific warning makes more sense to me. If someone thinks they are doing developerish things (as most of these scams lead them to think they are doing), they won't be deterred by a generic "only use this if you are a dev" warning for Developer Mode.

Comment 11

[Andrea Giammarchi]

> I don't see where HTTPS comes in here.

you already named banks, there it goes https ... other sites that are not even behind https or that not use CSP headers that mark inline JS not allowed should be OK for the console and no warning needed, IMO


Last, but not least, tell me how many self-xss I can do on about:blank which was my first screenshot :P

In about:blank or in absence of a protocol I'd say this makes no sense anyway, just annoying.

Comment 12

[Manish Goregaokar [:manishearth]]

(In reply to Andrea Giammarchi from comment #11)
> > I don't see where HTTPS comes in here.
> 
> you already named banks, there it goes https ... other sites that are not
> even behind https or that not use CSP headers that mark inline JS not
> allowed should be OK for the console and no warning needed, IMO

The existence of attacks on sites which use https does not necessarily mean that only https sites need the protection. This exploit existed for Facebook (and was a problem) before HTTPS was rolled out by default.

Lack of HTTPS doesn't mean a site doesn't care about security, either. StackExchange was on HTTP for quite a while, due to the sheer complexity of getting everything on HTTPS (it's not as easy as it sounds)

> Last, but not least, tell me how many self-xss I can do on about:blank which
> was my first screenshot :P
> 
> In about:blank or in absence of a protocol I'd say this makes no sense
> anyway, just annoying.

That makes sense, but it seems like an edge case.

Remember that most devs will never encounter this since it automatically turns off after light usage of the console. It will only occur if you paste stuff into the console before having used it a couple of times. I guess the fix can be made, not sure if we want to riddle the code with handling of about:blank and whatnot.

Comment 13

[Joe Walker [:jwalker]]

(In reply to Andrea Giammarchi from comment #11)
> In about:blank or in absence of a protocol I'd say this makes no sense
> anyway, just annoying.

It has been argued that pages from the browser, which have a more direct route to privilege escalation, are more of a concern than http pages.

Comment 14

[Paul Rouget [:paul]]

(In reply to Joe Walker [:jwalker] from comment #13)
> (In reply to Andrea Giammarchi from comment #11)
> > In about:blank or in absence of a protocol I'd say this makes no sense
> > anyway, just annoying.
> 
> It has been argued that pages from the browser, which have a more direct
> route to privilege escalation, are more of a concern than http pages.

Not even a "route to". JS code is already privileged code.

Comment 15

[Joe Walker [:jwalker]]

Comment on attachment 8462598 [details] [diff] [review]
Updated patch, threshold lowered to 5

Review of attachment 8462598 [details] [diff] [review]:
-----------------------------------------------------------------

Please could you split this patch in 2.

One patch moves the settings to branding, and sets all the values to 5. We commit this straight away. Then in a couple of weeks when we know that this hasn't caused problems, commit patch no.2 which takes away the test for nightly/aurora.

::: browser/branding/nightly/pref/firefox-branding.js
@@ +30,5 @@
>  pref("browser.search.param.yahoo-fr-metro", "");
>  #endif
> +
> +// Number of usages of the web console or scratchpad.
> +// If this is less than 10, then pasting code into the web console or scratchpad is disabled

s/10/5

Comment 16

[Manish Goregaokar [:manishearth]]

Attachment: Reduce threshhold to 5

Split patch

Comment 17

[Manish Goregaokar [:manishearth]]

Attachment: Reduce threshhold to 5

+comment

Comment 18

[Manish Goregaokar [:manishearth]]

Attachment: Branding changes

Branding changes, will r? later

Comment 19

[Manish Goregaokar [:manishearth]]

Marking as checkin-needed, filing a separate bug for the branding changes so this doesn't get marked as FIXED.

Comment 20

[Joe Walker [:jwalker]]

Thanks Manish!

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Try link? :)

Comment 22

[Manish Goregaokar [:manishearth]]

Gah, forgot: https://tbpl.mozilla.org/?tree=Try&rev=380203562143

Waiting for it to build.

Comment 23

[Manish Goregaokar [:manishearth]]

Attachment: Reduced threshhold, fixed test

Forgot to update the test *sheepish grin*

Laptop conked out, currently using a lab computer, need someone to try push for me.

Comment 24

[Manish Goregaokar [:manishearth]]

New trypush, courtesy capella: https://tbpl.mozilla.org/?tree=Try&rev=f243997b128e

Comment 25

[Manish Goregaokar [:manishearth]]

Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b1a84e51ecee

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/ef44ee66c51c

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ef44ee66c51c