1696685 - (CVE-2021-43541) URIs passed to external application protocol / URL handlers should have spaces escaped

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[chriscla]

Issue relevant to Firefox on Windows (Windows 10, Firefox 86.0). This is not a bug in Firefox directly. Rather it is behavior that could lead to other developer's security issues being exploitable.

On Firefox for Windows, Firefox will not escape parameters passed to Windows URI handlers. If a Windows URI handler doesn't quote it's parameters (as is documented here: https://docs.microsoft.com/en-us/windows/win32/shell/sec-shell) then it's possible for there to be command line injection when a URI is launched from Firefox.

Windows Chrome and Edge will URI encode parameters before launching the URI (including spaces). This would make it much harder for these issues to be exploitable.

For example:

Assume there is a handler, "fake_handler:" configured in windows as:

fake_handler.exe %1

Then the URI

fake_handler:Hello World

Would result in the command line: fake_handler Hello World. The app would treat World as a command line parameter.

In Edge/Chrome, this would result in the command line: fake_handler Hello%20World. which would all be treated as one parameter and not vulnerable to the command line injection.

Not looking for a bounty.

Comment 1

[:Gijs (he/him)]

Thanks for the report.

It looks like this is a reincarnation of bug 389106 (14 years ago!). I don't really follow what's happened there back in 2007/2008. It looks like a patch to escape spaces specifically for external URIs was discussed, and landed on all release branches except main/mozilla-central . I'm getting a bit confused around the archaeology here because this was around the time of the switch to mercurial (from CVS, back in the day), and mercurial has changes from bug 385065 which then don't include the space-escaping. There was also a change to nsEscape.cpp which escaped spaces everywhere, which landed on trunk (now mozilla-central). There nsEscape.cpp escaped spaces - until bug 1389251 changed that in Firefox 62. Although it says on that bug that Selena asked for a sec review there, I don't see evidence in the bug that that happened... I suspect if we don't want to escape spaces everywhere, we still need to do this in the external protocol handler (and, it would seem, adding a functional/integration style test that checks this happens wouldn't be a bad idea...).

Valentin, can you take a look?

Anne: I also don't know what, if anything, the URL spec says about passing non-web URLs around. Feels like if it doesn't already, being more explicit around expectations might not be a bad thing, but I don't know if that's in scope or not.

Comment 2

[Anne (:annevk)]

1. A scheme cannot contain a _ so that input would never parse as a URL as far as I can tell.
2. It's true that for non-special schemes spaces are not percent-encoded. This is what Chrome and Safari do as well: https://jsdom.github.io/whatwg-url/#url=ZmFrZStzY2hlbWU6aGVsbG8gd29ybGQ=&base=YWJvdXQ6Ymxhbms=.
3. I guess that means they do something special when handing such a URL to the OS, which can only happen during navigation. (If someone is going to investigate this, I would love some help with https://github.com/whatwg/html/issues/5375#issuecomment-776578002.)

Hope that helps.

Comment 3

[chriscla]

(In reply to Anne (:annevk) from comment #2)

1. A scheme cannot contain a _ so that input would never parse as a URL as far as I can tell.

Apologies, I typed that handler here as an example in the report and didn't realize that restriction. The real handler we experienced this with didn't have a _.

Comment 5

[Neha Kochar [:neha]]

Perhaps we can copy Chrome's escape behavior that Nika found: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/external_protocol/external_protocol_handler.cc;l=412;drc=6efee78914660d0249e01aa403579476d8dc6a20

Anny, would you like to work on this post-Fission?

Comment 6

[Anny Gakhokidze [:annyG]]

Yes, sounds interesting!

Comment 7

[Valentin Gosu [:valentin] (he/him)]

I completely missed this needinfo. Sorry about that.
Indeed, bug 1389251 started allowing spaces in URIs which made this hole possible.
I think calling NS_EscapeURL with esc_Spaces flag before creating the URI should be enough to cover this.
We should also check that the URI spec contains no invalid characters before we pass them off to the handler.

Comment 8

[Andrew McCreight [:mccr8]]

Anny is probably busy with parent process navigation stuff, so let's clear the assignee for now.

Comment 9

[Anne (:annevk)]

Johann, Paul, I know you are both busy, but as this sits in an area you have previously helped out with, is this something you could look into?

Comment 10

[Emma Zühlcke [:emz]]

I'll take a look. Thanks!

Comment 11

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Paul, feel free to let us know if severity S2 doesn't sound right to you. Thanks.

Comment 12

[Emma Zühlcke [:emz]]

Attachment: Bug 1696685 - r=#xpcom-reviewers

Comment 13

[Emma Zühlcke [:emz]]

Attachment: Bug 1696685 - Tests, r=#xpcom-reviewers

Depends on D128630

Comment 14

[Phabricator Automation]

Comment on attachment 9246199 [details]
Bug 1696685 - Tests, r=#xpcom-reviewers

Revision D128631 was moved to bug 1736543. Setting attachment 9246199 [details] to obsolete.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Landed: https://hg.mozilla.org/integration/autoland/rev/346bcc20a94e5e497150e103fa3e5e509d8327c5

Backed out for causing bustages on nsEscape.h:

https://hg.mozilla.org/integration/autoland/rev/b6163ae0c4c6a514a5b49704b7ffda7d7bd53e8e

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=usercancel%2Ctestfailed%2Cbusted%2Cexception%2Cretry&revision=346bcc20a94e5e497150e103fa3e5e509d8327c5
Failure log: https://treeherder.mozilla.org/logviewer?job_id=355263012&repo=autoland

[task 2021-10-19T09:45:24.451Z] 09:45:24     INFO -  In file included from /builds/worker/checkouts/gecko/chrome/nsChromeRegistry.cpp:14:
[task 2021-10-19T09:45:24.451Z] 09:45:24    ERROR -  /builds/worker/workspace/obj-build/dist/include/nsEscape.h:109:31: error: '/*' within block comment [-Werror,-Wcomment]
[task 2021-10-19T09:45:24.451Z] 09:45:24     INFO -                                /* Escapes everything except:
[task 2021-10-19T09:45:24.452Z] 09:45:24     INFO -                                ^
[task 2021-10-19T09:45:24.452Z] 09:45:24    ERROR -  /builds/worker/workspace/obj-build/dist/include/nsEscape.h:110:31: error: '/*' within block comment [-Werror,-Wcomment]
[task 2021-10-19T09:45:24.452Z] 09:45:24     INFO -                                /* a-z, 0-9 and !#$&'()*+,-./:;=?@[]_~ */
[task 2021-10-19T09:45:24.452Z] 09:45:24     INFO -                                ^
[task 2021-10-19T09:45:24.452Z] 09:45:24     INFO -  2 errors generated.

Comment 16

[Emma Zühlcke [:emz]]

https://hg.mozilla.org/integration/autoland/rev/4d6fb33656ec1d9dc24fe8def03458e69738c375

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4d6fb33656ec

Comment 18

[Daniel Veditz [:dveditz]]

Cleared the bounty flag per comment 0

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR91 approval when you get a chance.

Comment 20

[Emma Zühlcke [:emz]]

Comment on attachment 9246198 [details]
Bug 1696685 - r=#xpcom-reviewers

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Firefox will not escape spaces (among other characters) before passing them to external protocol handlers. This means it may be possible for websites to execute arbitrary commands on the target application or OS.
  See comment 0 for a detailed description.
• Fix Landed on Version: 95
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): Medium risk. Changing our escaping code could break some external protocol handlers. However, the new escape behavior aligns with Chromium which makes compatibility issues unlikely.
  Also the patch had bake time in Nightly and Beta.
• String or UUID changes made by this patch:

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9246198 [details]
Bug 1696685 - r=#xpcom-reviewers

Approved for 91.4esr.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/800cc8dc7639

Comment 23

[Frederik Braun [:freddy]]

Attachment: advisory.txt