From f9b564be037fc2c7875c7b180ee6f002f8a9db53 Mon Sep 17 00:00:00 2001 From: deeplow Date: Tue, 1 Nov 2022 12:55:02 +0000 Subject: [PATCH] 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 | 42 ++++++++++++++++++++++++++++++++++++++++++ dangerzone/cli.py | 11 +++++++---- dangerzone/errors.py | 2 +- tests/test_cli.py | 39 +++++++++++++++++++++++++++++++++++++-- 4 files changed, 87 insertions(+), 7 deletions(-) diff --git a/dangerzone/args.py b/dangerzone/args.py index 120da65..8deb01d 100644 --- a/dangerzone/args.py +++ b/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] diff --git a/dangerzone/cli.py b/dangerzone/cli.py index a06283a..e06d891 100644 --- a/dangerzone/cli.py +++ b/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, Document +from .document import SAFE_EXTENSION 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" diff --git a/dangerzone/errors.py b/dangerzone/errors.py index 98b2759..a26d213 100644 --- a/dangerzone/errors.py +++ b/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 diff --git a/tests/test_cli.py b/tests/test_cli.py index 257388b..c31fc23 100644 --- a/tests/test_cli.py +++ b/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.