Closed Bug 1422643 (CVE-2018-5143) Opened 3 years ago Closed 3 years ago
Self-XSS protection bypass with tab characters
Status: UNCONFIRMED → NEW
Component: Untriaged → Address Bar
Ever confirmed: true
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attachment #8934151 - Flags: review?(mak77)
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, 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.
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.
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).
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 email@example.com: https://hg.mozilla.org/integration/autoland/rev/6ad5ec88a898 deal with tabs in the protocol in js paste detection code, r=florian,valentin
Backed out for failing browser/base/content/test/urlbar/browser_removeUnsafeProtocolsFromURLBarPaste.js Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6ad5ec88a8982d83b8097fd76a2383aae94711c6 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=154245034&repo=autoland&lineNumber=2041 Backout: https://hg.mozilla.org/integration/autoland/rev/ea1837bc4e1fb9eae72fae99cfb205c5b662d432
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
Green on try ( https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ae1a7a6e5fd ) so relanding...
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/ffd3fd96a76a deal with tabs in the protocol in js paste detection code, r=florian,valentin
You need to log in before you can comment on or make changes to this bug.