Change threshold for self-xss changes

RESOLVED FIXED in Firefox 34

Status

()

Firefox
Developer Tools: Console
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: manishearth, Assigned: manishearth)

Tracking

Trunk
Firefox 34
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

3 years ago
Bug 994134 has landed now.

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

Comment 1

3 years ago
Created attachment 8427862 [details] [diff] [review]
Patch for branding changes
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
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!
(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?
(Assignee)

Comment 4

3 years ago
> 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.
(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.
Chromium bug https://code.google.com/p/chromium/issues/detail?id=345205
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1043981

Comment 8

3 years ago
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
(Assignee)

Comment 9

3 years ago
Created attachment 8462598 [details] [diff] [review]
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.
Attachment #8427862 - Attachment is obsolete: true
Attachment #8462598 - Flags: review?(jwalker)
(Assignee)

Comment 10

3 years ago
(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

3 years ago
> 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.
(Assignee)

Comment 12

3 years ago
(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.
(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

3 years ago
(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 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
Attachment #8462598 - Flags: review?(jwalker) → review+
(Assignee)

Comment 16

3 years ago
Created attachment 8465251 [details] [diff] [review]
Reduce threshhold to 5

Split patch
Attachment #8462598 - Attachment is obsolete: true
Attachment #8465251 - Flags: review?(jwalker)
(Assignee)

Comment 17

3 years ago
Created attachment 8465253 [details] [diff] [review]
Reduce threshhold to 5

+comment
Attachment #8465251 - Attachment is obsolete: true
Attachment #8465251 - Flags: review?(jwalker)
Attachment #8465253 - Flags: review?(jwalker)
(Assignee)

Comment 18

3 years ago
Created attachment 8465255 [details] [diff] [review]
Branding changes

Branding changes, will r? later
Attachment #8465255 - Flags: feedback?(jwalker)
Attachment #8465255 - Flags: feedback?(jwalker) → feedback+
Attachment #8465253 - Flags: review?(jwalker) → review+
(Assignee)

Comment 19

3 years ago
Marking as checkin-needed, filing a separate bug for the branding changes so this doesn't get marked as FIXED.
Keywords: checkin-needed
Summary: Restrict self-xss changes to Release/beta channels → Change threshold for self-xss changes
(Assignee)

Updated

3 years ago
See Also: → bug 1046672
(Assignee)

Updated

3 years ago
Attachment #8465255 - Attachment is obsolete: true
Thanks Manish!
(Assignee)

Updated

3 years ago
Blocks: 1046672
Try link? :)
Keywords: checkin-needed
(Assignee)

Comment 22

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

Waiting for it to build.
(Assignee)

Comment 23

3 years ago
Created attachment 8465957 [details] [diff] [review]
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.
Attachment #8465253 - Attachment is obsolete: true
Attachment #8465957 - Flags: review?(jwalker)
(Assignee)

Comment 24

3 years ago
New trypush, courtesy capella: https://tbpl.mozilla.org/?tree=Try&rev=f243997b128e
Attachment #8465957 - Flags: review?(jwalker) → review+
(Assignee)

Comment 25

3 years ago
Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b1a84e51ecee
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/ef44ee66c51c
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/ef44ee66c51c
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
You need to log in before you can comment on or make changes to this bug.