CVE-2021-41146: Improper Neutralization of Special Elements used in a Command ('Command Injection') (CWE-77)
value: qutebrowser is an open source keyboard-focused browser with a minimal GUI. Starting with qutebrowser v1.7.0, the Windows installer for qutebrowser registers a `qutebrowserurl:` URL handler. With certain applications, opening a specially crafted `qutebrowserurl:...` URL can lead to execution of qutebrowser commands, which in turn allows arbitrary code execution via commands such as `:spawn` or `:debug-pyeval`. Only Windows installs where qutebrowser is registered as URL handler are affected. The issue has been fixed in qutebrowser v2.4.0. The fix also adds additional hardening for potential similar issues on Linux (by adding the new --untrusted-args flag to the .desktop file), though no such vulnerabilities are known.
CWE description: The product constructs all or part of a command using externally-influenced input from an upstream component, but it does not neutralize or incorrectly neutralizes special elements that could modify the intended command when it is sent to a downstream component.
CVSS v3 base score: 8.8
Published: 2021-10-21T18:15Z
Repository: https://github.com/qutebrowser/qutebrowser
Language: Python
Files changed: misc/nsis/install.nsh, misc/org.qutebrowser.qutebrowser.desktop, qutebrowser/qutebrowser.py, tests/unit/test_qutebrowser.py
Commit date: 2021-10-21 16:01:04 +0200
Commit message: CVE-2021-41146: Add --untrusted-args to avoid argument injection

On Windows, if an application is registered as an URL handler like this:

    HKEY_CLASSES_ROOT
        https
            URL Protocol = ""
            [...]
            shell
                open
                    command
                    (Default) = ".../qutebrowser.exe" "%1"

one would think that Windows takes care of making sure URLs can't inject
arguments by containing a quote. However, this is not the case, as
stated by the Microsoft docs:
https://docs.microsoft.com/en-us/previous-versions/windows/internet-explorer/ie-developer/platform-apis/aa767914(v=vs.85)

    Security Warning: Applications that handle URI schemes must consider how to
    respond to malicious data. Because handler applications can receive data
    from untrusted sources, the URI and other parameter values passed to the
    application may contain malicious data that attempts to exploit the handling
    application.

and

    As noted above, the string that is passed to a pluggable protocol handler
    might be broken across multiple parameters. Malicious parties could use
    additional quote or backslash characters to pass additional command line
    parameters. For this reason, pluggable protocol handlers should assume that
    any parameters on the command line could come from malicious parties, and
    carefully validate them. Applications that could initiate dangerous actions
    based on external data must first confirm those actions with the user. In
    addition, handling applications should be tested with URIs that are overly
    long or contain unexpected (or undesirable) character sequences.

Indeed it's trivial to pass a command to qutebrowser this way - given how
trivial the exploit is to recreate given the information above, here's a PoC:

    https:x" ":spawn calc

(or qutebrowserurl: instead of https: if qutebrowser isn't registered as a
default browser)

Some applications do escape the quote characters before calling
qutebrowser - but others, like Outlook Desktop or .url files, do not.

As a fix, we add an --untrusted-args flag and some early validation of the raw
sys.argv, before parsing any arguments or e.g. creating a QApplication (which
might already allow injecting Qt flags there).

We assume that there's no way for an attacker to inject flags *before* the %1
placeholder in the registry, and add --untrusted-args as the last argument of
the registry entry. This way, it'd still be possible for users to customize
their invocation flags without having to remove --untrusted-args.

After --untrusted-args, however, we have some rather strict checks:

- There should be zero or one arguments, but not two (or more)
- Any argument may not start with - (flag) or : (qutebrowser command)

We also add the --untrusted-args flag to the Linux .desktop file, though it
should not be needed there, as the specification there is sane:

https://specifications.freedesktop.org/desktop-entry-spec/desktop-entry-spec-latest.html#exec-variables

    Implementations must take care not to expand field codes into multiple
    arguments unless explicitly instructed by this specification. This means
    that name fields, filenames and other replacements that can contain spaces
    must be passed as a single argument to the executable program after
    expansion.

There is no comparable mechanism on macOS, which opens the application without
arguments and then sends an "open" event to it:
https://doc.qt.io/qt-5/qfileopenevent.html

This issue was introduced in qutebrowser v1.7.0 which started registering it as
URL handler: baee2888907b260881d5831c68500941937261a0 / #4086

This is by no means an issue isolated to qutebrowser. Many other projects have
had similar trouble with Windows' rather unexpected behavior:

Electron / Exodus Bitcoin wallet:
- http://web.archive.org/web/20190702112128/https://medium.com/0xcc/electrons-bug-shellexecute-to-blame-cacb433d0d62
- https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2018-1000006
- https://medium.com/hackernoon/exploiting-electron-rce-in-exodus-wallet-d9e6db13c374

IE/Firefox:

- https://bugzilla.mozilla.org/show_bug.cgi?id=384384
- https://bugzilla.mozilla.org/show_bug.cgi?id=1572838

Others:
- http://web.archive.org/web/20210930203632/https://www.vdoo.com/blog/exploiting-custom-protocol-handlers-in-windows
- https://parsiya.net/blog/2021-03-17-attack-surface-analysis-part-2-custom-protocol-handlers/
- etc. etc.

See CVE-2021-41146 / GHSA-vw27-fwjf-5qxm:
https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2021-41146
https://github.com/qutebrowser/qutebrowser/security/advisories/GHSA-vw27-fwjf-5qxm

Thanks to Ping Fan (Zetta) Ke of Valkyrie-X Security Research Group
(VXRL/@vxresearch) for finding and responsibly disclosing this issue.

Diff: diff --git a/misc/nsis/install.nsh b/misc/nsis/install.nsh
index f29a0a9a80..362bf9c184 100755
--- a/misc/nsis/install.nsh
+++ b/misc/nsis/install.nsh
@@ -349,11 +349,11 @@ Section "Register with Windows" SectionWindowsRegister
     !insertmacro UpdateRegStr SHCTX "SOFTWARE\Classes\$2" "" "$3"
     !insertmacro UpdateRegStr SHCTX "SOFTWARE\Classes\$2" "FriendlyTypeName" "$3"
     !insertmacro UpdateRegDWORD SHCTX "SOFTWARE\Classes\$2" "EditFlags" 0x00000002
     !insertmacro UpdateRegStr SHCTX "SOFTWARE\Classes\$2\DefaultIcon" "" "$1,0"
     !insertmacro UpdateRegStr SHCTX "SOFTWARE\Classes\$2\shell" "" "open"
-    !insertmacro UpdateRegStr SHCTX "SOFTWARE\Classes\$2\shell\open\command" "" "$\"$1$\" $\"%1$\""
+    !insertmacro UpdateRegStr SHCTX "SOFTWARE\Classes\$2\shell\open\command" "" "$\"$1$\" --untrusted-args $\"%1$\""
     !insertmacro UpdateRegStr SHCTX "SOFTWARE\Classes\$2\shell\open\ddeexec" "" ""
     StrCmp $2 "${PRODUCT_NAME}HTML" 0 +4
     StrCpy $2 "${PRODUCT_NAME}URL"
     StrCpy $3 "${PRODUCT_NAME} URL"
     Goto WriteRegHandler
diff --git a/misc/org.qutebrowser.qutebrowser.desktop b/misc/org.qutebrowser.qutebrowser.desktop
index 52144b3c59..d999496ee7 100644
--- a/misc/org.qutebrowser.qutebrowser.desktop
+++ b/misc/org.qutebrowser.qutebrowser.desktop
@@ -43,11 +43,11 @@ Comment=A keyboard-driven, vim-like browser based on PyQt5
 Comment[de]=Ein Tastatur-gesteuerter, vim-ähnlicher Browser basierend auf PyQt5
 Comment[it]= Un browser web vim-like utilizzabile da tastiera basato su PyQt5
 Icon=qutebrowser
 Type=Application
 Categories=Network;WebBrowser;
-Exec=qutebrowser %u
+Exec=qutebrowser --untrusted-args %u
 Terminal=false
 StartupNotify=true
 MimeType=text/html;text/xml;application/xhtml+xml;application/xml;application/rdf+xml;image/gif;image/jpeg;image/png;x-scheme-handler/http;x-scheme-handler/https;x-scheme-handler/qute;
 Keywords=Browser
 Actions=new-window;preferences;
diff --git a/qutebrowser/qutebrowser.py b/qutebrowser/qutebrowser.py
index d0819f8328..c576c4a06e 100644
--- a/qutebrowser/qutebrowser.py
+++ b/qutebrowser/qutebrowser.py
@@ -85,10 +85,15 @@ def get_argparser():
     parser.add_argument('--desktop-file-name',
                         default="org.qutebrowser.qutebrowser",
                         help="Set the base name of the desktop entry for this "
                         "application. Used to set the app_id under Wayland. See "
                         "https://doc.qt.io/qt-5/qguiapplication.html#desktopFileName-prop")
+    parser.add_argument('--untrusted-args',
+                        action='store_true',
+                        help="Mark all following arguments as untrusted, which "
+                        "enforces that they are URLs/search terms (and not flags or "
+                        "commands)")
 
     parser.add_argument('--json-args', help=argparse.SUPPRESS)
     parser.add_argument('--temp-basedir-restarted',
                         help=argparse.SUPPRESS,
                         action='store_true')
@@ -205,11 +210,31 @@ def _unpack_json_args(args):
     data = json.loads(args.json_args)
     new_args.update(data)
     return argparse.Namespace(**new_args)
 
 
+def _validate_untrusted_args(argv):
+    # NOTE: Do not use f-strings here, as this should run with older Python
+    # versions (so that a proper error can be displayed)
+    try:
+        untrusted_idx = argv.index('--untrusted-args')
+    except ValueError:
+        return
+
+    rest = argv[untrusted_idx + 1:]
+    if len(rest) > 1:
+        sys.exit(
+            "Found multiple arguments ({}) after --untrusted-args, "
+            "aborting.".format(' '.join(rest)))
+
+    for arg in rest:
+        if arg.startswith(('-', ':')):
+            sys.exit("Found {} after --untrusted-args, aborting.".format(arg))
+
+
 def main():
+    _validate_untrusted_args(sys.argv)
     parser = get_argparser()
     argv = sys.argv[1:]
     args = parser.parse_args(argv)
     if args.json_args is not None:
         args = _unpack_json_args(args)
diff --git a/tests/unit/test_qutebrowser.py b/tests/unit/test_qutebrowser.py
index d9275631d0..36b4065a1d 100644
--- a/tests/unit/test_qutebrowser.py
+++ b/tests/unit/test_qutebrowser.py
@@ -20,10 +20,12 @@
 """Tests for qutebrowser.qutebrowser.
 
 (Mainly commandline flag parsing)
 """
 
+import re
+
 import pytest
 
 from qutebrowser import qutebrowser
 
 
@@ -73,5 +75,63 @@ class TestJsonArgs:
         args = parser.parse_args(['--json-args', '{"debug": true}'])
         args = qutebrowser._unpack_json_args(args)
         # pylint: disable=no-member
         assert args.debug
         assert not args.temp_basedir
+
+
+class TestValidateUntrustedArgs:
+
+    @pytest.mark.parametrize('args', [
+        [],
+        [':nop'],
+        [':nop', '--untrusted-args'],
+        [':nop', '--debug', '--untrusted-args'],
+        [':nop', '--untrusted-args', 'foo'],
+        ['--debug', '--untrusted-args', 'foo'],
+        ['foo', '--untrusted-args', 'bar'],
+    ])
+    def test_valid(self, args):
+        qutebrowser._validate_untrusted_args(args)
+
+    @pytest.mark.parametrize('args, message', [
+        (
+            ['--untrusted-args', '--debug'],
+            "Found --debug after --untrusted-args, aborting.",
+        ),
+        (
+            ['--untrusted-args', ':nop'],
+            "Found :nop after --untrusted-args, aborting.",
+        ),
+        (
+            ['--debug', '--untrusted-args', '--debug'],
+            "Found --debug after --untrusted-args, aborting.",
+        ),
+        (
+            [':nop', '--untrusted-args', '--debug'],
+            "Found --debug after --untrusted-args, aborting.",
+        ),
+        (
+            [':nop', '--untrusted-args', ':nop'],
+            "Found :nop after --untrusted-args, aborting.",
+        ),
+        (
+            [
+                ':nop',
+                '--untrusted-args',
+                ':nop',
+                '--untrusted-args',
+                'https://www.example.org',
+            ],
+            (
+                "Found multiple arguments (:nop --untrusted-args "
+                "https://www.example.org) after --untrusted-args, aborting."
+            )
+        ),
+        (
+            ['--untrusted-args', 'okay1', 'okay2'],
+            "Found multiple arguments (okay1 okay2) after --untrusted-args, aborting.",
+        ),
+    ])
+    def test_invalid(self, args, message):
+        with pytest.raises(SystemExit, match=re.escape(message)):
+            qutebrowser._validate_untrusted_args(args)

Vulnerable code:     !insertmacro UpdateRegStr SHCTX "SOFTWARE\Classes\$2\shell\open\command" "" "$\"$1$\" $\"%1$\""
Exec=qutebrowser %u
Fixed code:     !insertmacro UpdateRegStr SHCTX "SOFTWARE\Classes\$2\shell\open\command" "" "$\"$1$\" --untrusted-args $\"%1$\""
Exec=qutebrowser --untrusted-args %u
    parser.add_argument('--untrusted-args',
                        action='store_true',
                        help="Mark all following arguments as untrusted, which "
                        "enforces that they are URLs/search terms (and not flags or "
                        "commands)")
def _validate_untrusted_args(argv):
    # NOTE: Do not use f-strings here, as this should run with older Python
    # versions (so that a proper error can be displayed)
    try:
        untrusted_idx = argv.index('--untrusted-args')
    except ValueError:
        return

    rest = argv[untrusted_idx + 1:]
    if len(rest) > 1:
        sys.exit(
            "Found multiple arguments ({}) after --untrusted-args, "
            "aborting.".format(' '.join(rest)))

    for arg in rest:
        if arg.startswith(('-', ':')):
            sys.exit("Found {} after --untrusted-args, aborting.".format(arg))


    _validate_untrusted_args(sys.argv)
import re



class TestValidateUntrustedArgs:

    @pytest.mark.parametrize('args', [
        [],
        [':nop'],
        [':nop', '--untrusted-args'],
        [':nop', '--debug', '--untrusted-args'],
        [':nop', '--untrusted-args', 'foo'],
        ['--debug', '--untrusted-args', 'foo'],
        ['foo', '--untrusted-args', 'bar'],
    ])
    def test_valid(self, args):
        qutebrowser._validate_untrusted_args(args)

    @pytest.mark.parametrize('args, message', [
        (
            ['--untrusted-args', '--debug'],
            "Found --debug after --untrusted-args, aborting.",
        ),
        (
            ['--untrusted-args', ':nop'],
            "Found :nop after --untrusted-args, aborting.",
        ),
        (
            ['--debug', '--untrusted-args', '--debug'],
            "Found --debug after --untrusted-args, aborting.",
        ),
        (
            [':nop', '--untrusted-args', '--debug'],
            "Found --debug after --untrusted-args, aborting.",
        ),
        (
            [':nop', '--untrusted-args', ':nop'],
            "Found :nop after --untrusted-args, aborting.",
        ),
        (
            [
                ':nop',
                '--untrusted-args',
                ':nop',
                '--untrusted-args',
                'https://www.example.org',
            ],
            (
                "Found multiple arguments (:nop --untrusted-args "
                "https://www.example.org) after --untrusted-args, aborting."
            )
        ),
        (
            ['--untrusted-args', 'okay1', 'okay2'],
            "Found multiple arguments (okay1 okay2) after --untrusted-args, aborting.",
        ),
    ])
    def test_invalid(self, args, message):
        with pytest.raises(SystemExit, match=re.escape(message)):
            qutebrowser._validate_untrusted_args(args)