Closed Bug 1422643 (CVE-2018-5143) Opened 6 years ago Closed 6 years ago

Self-XSS protection bypass with tab characters

Categories

(Firefox :: Address Bar, defect, P1)

57 Branch
defect

Tracking

()

RESOLVED FIXED
Firefox 59
Tracking Status
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: masatokinugawa, Assigned: Gijs)

References

Details

(Keywords: csectype-spoof, sec-low, Whiteboard: [fxsearch][adv-main59+])

Attachments

(2 files, 1 obsolete file)

Attached file poc_self-xss_tab.html
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20171203100121

Steps to reproduce:

Usually, to mitigate the self-XSS via the address bar, Firefox removes "javascript:" string when "javascript:[XSS_HERE]" URL is pasted to the address bar.
However, if there is a tab character between "javascript:" string, like "java[0x09]script:alert(1)", Firefox does not remove it although JavaScript is executed from that string by tapping the enter key.

Steps to Reproduce:
1. Open the attached PoC.
2. Copy the string in the textarea.
3. Paste it to the address bar.
4. When the enter key is pressed, JavaScript is executed.


Actual results:

The self-XSS protection does not work.


Expected results:

The self-XSS protection should work.
Status: UNCONFIRMED → NEW
Component: Untriaged → Address Bar
Ever confirmed: true
Priority: -- → P1
Whiteboard: [fxsearch]
Attached patch Patch v0.1 (obsolete) — Splinter Review
Given that we don't replace the URL unless we actually find it started with "javascript:", I think we can just aggressively strip all whitespace in that version of the string without causing undue alterations to "safe" strings. Yes, this will be annoying for "legitimate" uses that haven't been escaped. I'm... not sure I care about that? There are plenty of other, more sane ways to execute arbitrary JS. Marco, does that sound reasonable?

Per bug 1402896, I expect this is sec-low and can be opened up. Dan?
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(dveditz)
Attachment #8934151 - Flags: review?(mak77)
java[0x09]script: is an invalid scheme. Why is that executing as a javascript URL? Seems to me like that's the fundamental problem. 

For developers who really want to execute a javascript: URL stripping all the spaces will break their workflow completely, whereas before they simply had to restore the scheme. Maybe that's a smallish number of people and we could simply tell them "tough luck--use devtools instead", but I worry there might be other situations where magic scheme creation/concatenation will turn what was intended to be a search (because there's a space in it) into running something else.
Group: firefox-core-security
Flags: needinfo?(dveditz)
(In reply to Daniel Veditz [:dveditz] from comment #2)
> java[0x09]script: is an invalid scheme. Why is that executing as a
> javascript URL? Seems to me like that's the fundamental problem. 

https://url.spec.whatwg.org/#concept-basic-url-parser

Step 2 and 3 say:

> If input contains any ASCII tab or newline, validation error.
> 
> Remove all ASCII tab or newline from input.

Once again, validation errors do not cause parsing failure.

So no, I think this is valid URL input (unfortunately).

I would have preferred if the spec was less... accepting... of this type of input, but it isn't right now.
(In reply to Daniel Veditz [:dveditz] from comment #2)
> For developers who really want to execute a javascript: URL stripping all
> the spaces will break their workflow completely, whereas before they simply
> had to restore the scheme.

Can't they just URI-escape things?

> Maybe that's a smallish number of people and we
> could simply tell them "tough luck--use devtools instead", but I worry there
> might be other situations where magic scheme creation/concatenation will
> turn what was intended to be a search (because there's a space in it) into
> running something else.

We only do something if the post-space-concatenation starts with "javascript:" (possibly preceded by non-word characters). Seems unlikely that would happen for any legitimate non-search URL requests.
I would say that the fundamental problem is attempting to reimplement the URL parser in the address bar, but I think I've said that before and Gijs mentioned where were some problems with parsing first and then operating on the resulting structure.

Maybe we should start thinking about a different architecture here?
Is there a reason to not just strip spaces up to the first colon? That would solve both this problem and Daniel's concerns.
Well, it wouldn't match what the URL parser does. To match the URL parser you should not strip spaces. If you only care about the scheme you would need to remove leading CO control and space (U+0000 to U+0020, inclusive). Then split on the first ":" I suppose. Then remove any tab or newline (U+0009, U+000A, or U+000D) from the first segment returned. Then ASCII lowercase (i.e., not .toLowerCase(), unless you audit and are sure that no Unicode code point has a lowercase ASCII that's inside "javascript") the first segment. Then see if that first segment equals "javascript".

This is rather involved, which is why I think you should consider a better architecture.
well, we could probably try to make a URL from the string, and pick the scheme from there if that succeeds? Just throwing out thoughts, TBV.
Yeah, I think that works, but I also recall Gijs having concerns.
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8934151 [details] [diff] [review]
Patch v0.1

(In reply to Anne (:annevk) from comment #9)
> Yeah, I think that works, but I also recall Gijs having concerns.

I do have some concerns, but after more reflection, it's probably the best option right now.

Essentially, the ideal solution (IMO) is to check:
a) the pasted string
b) the result of the existing input value + pasted string once pasted (using selectionStart/End and manual concatenation)

and feed it through whatever process the URL bar would normally use to navigate to that URL.

Unfortunately right now, some of that process happens inside docshell.

For many reasons, it would be a Good Idea (tm) to switch to a situation where docshell never does any fixup for loads, and the only fixup that happens, must happen in JS land prior to handing stuff to docshell.

However, that's a project that would probably take months, require several very very old interface definitions to change plus the associated fallout, and has nasty corner cases because sometimes fixup currently originates from within docshell errors, and is done synchronously. Because the content process (where most web docshells live these days) doesn't have some of the information required for fixup (like search engine URLs), farming it out to the parent process requires sync IPC, which is terrible. Making it async would likely cause regressions all over the place because of assumptions we have about what happens when this type of situation occurs, when error pages load, etc. (also because error page loads are... weird). X-ref some of my comments in bug 1375244.

Similarly, fixup is currently in a docshell C++ component and desperately needs to be migrated to a JS (or maybe rust) equivalent because C++ is a terrible language to do that kind of string mashup in. If we use a jsm and then migrate fixup out of docshell, that'll also reduce the amount of xpcom goop we end up doing for this, but see above - this is hard.

That's all without going into what autocomplete code does, which also manually invokes some fixup in some circumstances, I think (to determine whether something is a search or not).

I'll put up a patch to just use the URL constructor here, which is a small change. This is kind of sad because it does a full parse which is clearly a superset of what we need here, but I don't think there's currently an easy way around that without running into some of the downsides that have already been mentioned here.
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8934151 - Flags: review?(mak77)
If we want to match what the URL parsers do, we can call nsURLHelper.cpp:net_ExtractURLScheme()
(In reply to Valentin Gosu [:valentin] from comment #11)
> If we want to match what the URL parsers do, we can call
> nsURLHelper.cpp:net_ExtractURLScheme()

Maybe, but this is JS code... is that method actually exposed anywhere?
Seems it is exposed as nsIIOService.extractScheme
Ugh. So it turns out, the other thing I forgot about when commenting here is that getting the scheme is all very well, but now we need the string without the scheme. Without processing all the other junk. Which constructing the URL doesn't help with, because it changes the rest of the input (e.g. by URI-escaping it).
Comment on attachment 8938146 [details]
Bug 1422643 - deal with tabs in the protocol in js paste detection code,

https://reviewboard.mozilla.org/r/208816/#review214592

::: browser/base/content/browser.js:6067
(Diff revision 1)
> -                                        () => {
> -                                                changed = true;
> -                                                return "";
> -                                              });
> -  return changed ? pasteDataNoJS : pasteData;
> +      scheme = Services.io.extractScheme(pasteData);
> +    } catch (ex) { }
> +    if (scheme != "javascript") {
> +      break;
> +    }
> +   

nit: trailing whitespace
Attachment #8938146 - Flags: review?(florian) → review+
Comment on attachment 8938146 [details]
Bug 1422643 - deal with tabs in the protocol in js paste detection code,

https://reviewboard.mozilla.org/r/208816/#review214724

This looks reasonable, but I'm not familiar enough with Services.io.extractScheme. Given the tests it seems like it should do the right thing though. You might want to test \r in the middle too and the full range of U+0000 through U+0020 as leading.
Attachment #8938146 - Flags: review?(annevk)
Comment on attachment 8938146 [details]
Bug 1422643 - deal with tabs in the protocol in js paste detection code,

https://reviewboard.mozilla.org/r/208816/#review215932

Looks good!
Attachment #8938146 - Flags: review?(valentin.gosu) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6ad5ec88a898
deal with tabs in the protocol in js paste detection code, r=florian,valentin
Last time I suggested someone should get backed out over TV failures I was told it's tier-2 and we don't do that... bit ironic. :-\

This is only failing on Linux and Windows, I guess there's a clipboard handling difference for those unicode bytes somewhere on OSX (which is where I tested locally...) vs the other OSes. I'll look later today.
(In reply to :Gijs from comment #23)
> Last time I suggested someone should get backed out over TV failures I was
> told it's tier-2 and we don't do that... bit ironic. :-\

Oh, my mistake - looks like this also failed on the "cl" tests on the 1 linux and 1 windows configuration it ran on, but not on the others because we don't run them on every push, and I missed this because everything was starred, the cl test abbreviations on Treeherder don't start with "bc", the failing log was from a TV job, and the tests view on treeherder has been disabled so I couldn't look for the test in any other straightforward manner...


> This is only failing on Linux and Windows, I guess there's a clipboard
> handling difference for those unicode bytes somewhere on OSX (which is where
> I tested locally...) vs the other OSes. I'll look later today.

Seems like both Windows and Linux don't like null bytes in their clipboard, and Windows also either doesn't like or transforms '\r' such that the clipboard test util code in the mochitest suite doesn't detect the copy as having succeeded, which then breaks the test. I'll modify those cases so we run it where we can but not, apparently, on Windows.
Attachment #8934151 - Attachment is obsolete: true
Flags: needinfo?(gijskruitbosch+bugs)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ffd3fd96a76a
deal with tabs in the protocol in js paste detection code, r=florian,valentin
https://hg.mozilla.org/mozilla-central/rev/ffd3fd96a76a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Whiteboard: [fxsearch] → [fxsearch][adv-main59+]
Alias: CVE-2018-5143
Depends on: 1439396
You need to log in before you can comment on or make changes to this bug.