Closed Bug 1696685 (CVE-2021-43541) Opened 2 years ago Closed 2 years ago

URIs passed to external application protocol / URL handlers should have spaces escaped

Categories

(Core :: DOM: Navigation, defect)

Desktop
All
defect

Tracking

()

RESOLVED FIXED
95 Branch
Tracking Status
firefox-esr78 --- wontfix
firefox-esr91 95+ fixed
firefox93 --- wontfix
firefox94 --- wontfix
firefox95 + fixed

People

(Reporter: chriscla, Assigned: pbz)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression, sec-moderate, sec-vector, Whiteboard: [reporter-external] [client-bounty-form][adv-main95+][adv-ESR91.4.0+])

Attachments

(2 files, 1 obsolete file)

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.

Flags: sec-bounty?

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.

Group: firefox-core-security → network-core-security
Status: UNCONFIRMED → NEW
Type: task → defect
Component: Security → Networking
Depends on: 1389251, CVE-2007-3845
Ever confirmed: true
Flags: needinfo?(valentin.gosu)
OS: Unspecified → All
Product: Firefox → Core
Hardware: Unspecified → Desktop
Summary: URI Escaping Windows URL handlers → URIs passed to external application protocol / URL handlers should have spaces escaped
  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.

Component: Networking → DOM: Navigation

(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 _.

Group: network-core-security → dom-core-security

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?

Flags: needinfo?(agakhokidze)

Yes, sounds interesting!

Assignee: nobody → agakhokidze
Flags: needinfo?(agakhokidze)

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.

Flags: needinfo?(valentin.gosu)

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

Assignee: agakhokidze → nobody

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?

Flags: needinfo?(pbz)
Flags: needinfo?(jhofmann)

I'll take a look. Thanks!

Assignee: nobody → pbz
Status: NEW → ASSIGNED
Flags: needinfo?(pbz)
Flags: needinfo?(jhofmann)

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

Severity: -- → S2

Depends on D128630

Blocks: 1736543

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

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

Attachment #9246199 - Attachment is obsolete: true

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.
Flags: needinfo?(pbz)
Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 95 Branch
Flags: sec-bounty?
Regressed by: 1389251
Has Regression Range: --- → yes
Keywords: regression

Cleared the bounty flag per comment 0

QA Whiteboard: [post-critsmash-triage]
Flags: qe-verify-

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

Flags: needinfo?(pbz)

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:
Flags: needinfo?(pbz)
Attachment #9246198 - Flags: approval-mozilla-esr91?

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

Approved for 91.4esr.

Attachment #9246198 - Flags: approval-mozilla-esr91? → approval-mozilla-esr91+
Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form][adv-main95+][adv-ESR91.4.0+]
Alias: CVE-2021-43541
Group: core-security-release
Blocks: 1781264
You need to log in before you can comment on or make changes to this bug.