Security: cli wildcard injection mitigation

Wildcard arguments like `*` can lead to security vulnerabilities
if files are maliciously named as would-be parameters. In the following
scenario if a file in the current directory was named '--help', running
the following command would show the help.

  $ dangerzone-cli *

By checking if parameters also happen to be files, we mitigate this
risk and have a chance to warn the user.

dangerzone/args.py

@@ -1,3 +1,5 @@
+import functools
+import os
 from typing import List, Optional, Tuple
 
 import click
@@ -64,3 +66,43 @@ def validate_output_filename(
     ctx: click.Context, param: str, value: Optional[str]
 ) -> Optional[str]:
     return _validate_output_filename(ctx, param, value)
+
+
+def check_suspicious_options(args: List[str]) -> None:
+    options = set([arg for arg in args if arg.startswith("-")])
+    try:
+        files = set(os.listdir())
+    except Exception:
+        # If we can list files in the current working directory, this means that
+        # we're probably in an unlinked directory. Dangerzone should still work in
+        # this case, so we should return here.
+        return
+
+    intersection = options & files
+    if intersection:
+        filenames_str = ", ".join(intersection)
+        msg = (
+            f"Security: Detected CLI options that are also present as files in the"
+            f" current working directory: {filenames_str}"
+        )
+        click.echo(msg)
+        exit(1)
+
+
+def override_parser_and_check_suspicious_options(click_main: click.Command) -> None:
+    """Override the argument parsing logic of Click.
+
+    Click does not allow us to have access to the raw arguments that it receives (either
+    from sys.argv or from its testing module). To circumvent this, we can override its
+    `Command.parse_args()` method, which is public and should be safe to do so.
+
+    We can use it to check for any suspicious options prior to arg parsing.
+    """
+    orig_parse_fn = click_main.parse_args
+
+    @functools.wraps(orig_parse_fn)
+    def custom_parse_fn(ctx: click.Context, args: List[str]) -> List[str]:
+        check_suspicious_options(args)
+        return orig_parse_fn(ctx, args)
+
+    click_main.parse_args = custom_parse_fn  # type: ignore [assignment]

dangerzone/cli.py

@@ -1,18 +1,18 @@
 import json
 import logging
-import os
 import sys
-from typing import List, Optional
+from typing import Any, Callable, List, Optional, TypeVar
 
 import click
 from colorama import Back, Fore, Style
 
 from . import args, container, errors
-from .container import convert
+from .document import SAFE_EXTENSION
-from .document import SAFE_EXTENSION, Document
 from .logic import DangerzoneCore
 from .util import get_version
 
+F = TypeVar("F", bound=Callable[..., Any])
+
 
 def print_header(s: str) -> None:
     click.echo("")
@@ -98,6 +98,9 @@ def cli_main(
         exit(0)
 
 
+args.override_parser_and_check_suspicious_options(cli_main)
+
+
 def setup_logging() -> None:
     if getattr(sys, "dangerzone_dev", True):
         fmt = "%(message)s"

dangerzone/errors.py

@@ -1,7 +1,7 @@
 import functools
 import logging
 import sys
-from typing import Any, Callable, TypeVar, cast
+from typing import Any, Callable, Sequence, TypeVar, cast
 
 import click
 

tests/test_cli.py

@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+import contextlib
 import copy
 import os
 import shutil
@@ -106,7 +107,9 @@ class CLIResult(Result):
 
 
 class TestCli(TestBase):
-    def run_cli(self, args: Sequence[str] | str = ()) -> CLIResult:
+    def run_cli(
+        self, args: Sequence[str] | str = (), tmp_path: Path = None
+    ) -> CLIResult:
         """Run the CLI with the provided arguments.
 
         Callers can either provide a list of arguments (iterable), or a single
@@ -120,7 +123,20 @@ class TestCli(TestBase):
             # Convert the single argument to a tuple, else Click will attempt
             # to tokenize it.
             args = (args,)
-        result = CliRunner().invoke(cli_main, args)
+
+        # TODO: Replace this with `contextlib.chdir()` [1], which was added in
+        # Python 3.11.
+        #
+        # [1]: # https://docs.python.org/3/library/contextlib.html#contextlib.chdir
+        try:
+            if tmp_path is not None:
+                cwd = os.getcwd()
+                os.chdir(tmp_path)
+            result = CliRunner().invoke(cli_main, args)
+        finally:
+            if tmp_path is not None:
+                os.chdir(cwd)
+
         return CLIResult.reclass_click_result(result, args)
 
 
@@ -197,3 +213,22 @@ class TestCliConversion(TestCliBasic):
         result = self.run_cli(doc_path)
         result.assert_success()
         assert len(os.listdir(tmp_path)) == 2
+
+
+class TestSecurity(TestCli):
+    def test_suspicious_double_dash_file(self, tmp_path: Path) -> None:
+        """Protection against "dangeronze-cli *" and files named --option."""
+        file_path = tmp_path / "--ocr-lang"
+        file_path.touch()
+        result = self.run_cli(["--ocr-lang", "eng"], tmp_path)
+        result.assert_failure(message="Security: Detected CLI options that are also")
+
+    def test_suspicious_double_dash_and_equals_file(self, tmp_path: Path) -> None:
+        """Protection against "dangeronze-cli *" and files named --option=value."""
+        file_path = tmp_path / "--output-filename=bad"
+        file_path.touch()
+        result = self.run_cli(["--output-filename=bad", "eng"], tmp_path)
+        result.assert_failure(message="Security: Detected CLI options that are also")
+
+        # TODO: Check that this applies for single dash arguments, and concatenated
+        # single dash arguments, once Dangerzone supports them.