Merge pull request #1767 from fractal-analytics-platform/1646-escape-…

…or-reject-special-characters-in-user-provided-strings

Escape or reject special characters in commands to execute

CHANGELOG.md

@@ -1,6 +1,13 @@
 **Note**: Numbers like (\#1234) point to closed Pull Requests on the fractal-server repository.
 
 
+# 2.5.1 (Unreleased)
+
+* API:
+    * Add validation for user-provided strings that execute commands with subprocess or remote-shell (\#1767).
+* Runner and task collection:
+    * Validate commands before running them via `subprocess` or `fabric` (\#1767).
+
 # 2.5.0
 
 This release removes support for including V1 tasks in V2 workflows. This comes

fractal_server/app/routes/api/v2/task_collection_custom.py

@@ -20,12 +20,12 @@
 from ....schemas.v2 import TaskReadV2
 from fractal_server.app.models import UserOAuth
 from fractal_server.app.routes.auth import current_active_verified_user
+from fractal_server.string_tools import validate_cmd
 from fractal_server.tasks.v2.background_operations import _insert_tasks
 from fractal_server.tasks.v2.background_operations import (
     _prepare_tasks_metadata,
 )
 
-
 router = APIRouter()
 
 logger = set_logger(__name__)
@@ -74,6 +74,7 @@ async def collect_task_custom(
         package_name_underscore = task_collect.package_name.replace("-", "_")
         # Note that python_command is then used as part of a subprocess.run
         # statement: be careful with mixing `'` and `"`.
+        validate_cmd(package_name_underscore)
         python_command = (
             "import importlib.util; "
             "from pathlib import Path; "

fractal_server/app/runner/compress_folder.py

@@ -48,7 +48,7 @@ def create_tar_archive(
         "."
     )
     logger.debug(f"cmd tar:\n{cmd_tar}")
-    run_subprocess(cmd=cmd_tar, logger_name=logger_name)
+    run_subprocess(cmd=cmd_tar, logger_name=logger_name, allow_char="*")
 
 
 def remove_temp_subfolder(subfolder_path_tmp_copy: Path, logger_name: str):

fractal_server/app/runner/executors/slurm/sudo/_subprocess_run_as_user.py

@@ -20,6 +20,7 @@
 from typing import Optional
 
 from ......logger import set_logger
+from fractal_server.string_tools import validate_cmd
 
 logger = set_logger(__name__)
 
@@ -47,6 +48,7 @@ def _run_command_as_user(
     Returns:
         res: The return value from `subprocess.run`.
     """
+    validate_cmd(cmd)
     logger.debug(f'[_run_command_as_user] {user=}, cmd="{cmd}"')
     if user:
         new_cmd = f"sudo --set-home --non-interactive -u {user} {cmd}"

fractal_server/app/runner/executors/slurm/sudo/executor.py

@@ -47,6 +47,7 @@
 from ._subprocess_run_as_user import _run_command_as_user
 from fractal_server import __VERSION__
 from fractal_server.app.runner.components import _COMPONENT_KEY_
+from fractal_server.string_tools import validate_cmd
 
 
 logger = set_logger(__name__)
@@ -65,6 +66,7 @@ def _subprocess_run_or_raise(full_command: str) -> Optional[CompletedProcess]:
     Returns:
         The actual `CompletedProcess` output of `subprocess.run`.
     """
+    validate_cmd(full_command)
     try:
         output = subprocess.run(  # nosec
             shlex.split(full_command),
@@ -1266,6 +1268,7 @@ def shutdown(self, wait=True, *, cancel_futures=False):
             pre_command = f"sudo --non-interactive -u {self.slurm_user}"
             submit_command = f"scancel {scancel_string}"
             full_command = f"{pre_command} {submit_command}"
+            validate_cmd(full_command)
             logger.debug(f"Now execute `{full_command}`")
             try:
                 subprocess.run(  # nosec

fractal_server/app/runner/run_subprocess.py

@@ -3,11 +3,15 @@
 from typing import Optional
 
 from fractal_server.logger import get_logger
+from fractal_server.string_tools import validate_cmd
 
 
 def run_subprocess(
-    cmd: str, logger_name: Optional[str] = None
+    cmd: str,
+    allow_char: Optional[str] = None,
+    logger_name: Optional[str] = None,
 ) -> subprocess.CompletedProcess:
+    validate_cmd(cmd, allow_char=allow_char)
     logger = get_logger(logger_name)
     try:
         res = subprocess.run(  # nosec

fractal_server/app/runner/v1/_common.py

@@ -31,6 +31,7 @@
 from fractal_server.app.runner.filenames import HISTORY_FILENAME
 from fractal_server.app.runner.filenames import METADATA_FILENAME
 from fractal_server.app.runner.task_files import get_task_file_paths
+from fractal_server.string_tools import validate_cmd
 
 
 def no_op_submit_setup_call(
@@ -77,6 +78,7 @@ def _call_command_wrapper(cmd: str, stdout: Path, stderr: Path) -> None:
                             TERM or KILL signal)
     """
 
+    validate_cmd(cmd)
     # Verify that task command is executable
     if shutil.which(shlex_split(cmd)[0]) is None:
         msg = (

fractal_server/app/runner/v2/runner_functions_low_level.py

@@ -12,6 +12,7 @@
 from ..exceptions import TaskExecutionError
 from fractal_server.app.models.v2 import WorkflowTaskV2
 from fractal_server.app.runner.task_files import get_task_file_paths
+from fractal_server.string_tools import validate_cmd
 
 
 def _call_command_wrapper(cmd: str, log_path: Path) -> None:
@@ -25,6 +26,10 @@ def _call_command_wrapper(cmd: str, log_path: Path) -> None:
                             exit code (e.g. due to the subprocess receiving a
                             TERM or KILL signal)
     """
+    try:
+        validate_cmd(cmd)
+    except ValueError as e:
+        raise TaskExecutionError(f"Invalid command. Original error: {str(e)}")
 
     # Verify that task command is executable
     if shutil.which(shlex_split(cmd)[0]) is None:

fractal_server/app/schemas/user.py

@@ -10,7 +10,7 @@
 from ._validators import val_absolute_path
 from ._validators import val_unique_list
 from ._validators import valstr
-
+from fractal_server.string_tools import validate_cmd
 
 __all__ = (
     "UserRead",
@@ -77,14 +77,16 @@ class UserUpdate(schemas.BaseUserUpdate):
         valstr("slurm_user")
     )
     _username = validator("username", allow_reuse=True)(valstr("username"))
-    _cache_dir = validator("cache_dir", allow_reuse=True)(
-        val_absolute_path("cache_dir")
-    )
 
     _slurm_accounts = validator("slurm_accounts", allow_reuse=True)(
         val_unique_list("slurm_accounts")
     )
 
+    @validator("cache_dir")
+    def cache_dir_validator(cls, value):
+        validate_cmd(value)
+        return val_absolute_path("cache_dir")(value)
+
     @validator(
         "is_active",
         "is_verified",
@@ -115,9 +117,10 @@ class UserUpdateStrict(BaseModel, extra=Extra.forbid):
         val_unique_list("slurm_accounts")
     )
 
-    _cache_dir = validator("cache_dir", allow_reuse=True)(
-        val_absolute_path("cache_dir")
-    )
+    @validator("cache_dir")
+    def cache_dir_validator(cls, value):
+        validate_cmd(value)
+        return val_absolute_path("cache_dir")(value)
 
 
 class UserUpdateWithNewGroupIds(UserUpdate):
@@ -157,6 +160,8 @@ def slurm_accounts_validator(cls, value):
         valstr("slurm_user")
     )
     _username = validator("username", allow_reuse=True)(valstr("username"))
-    _cache_dir = validator("cache_dir", allow_reuse=True)(
-        val_absolute_path("cache_dir")
-    )
+
+    @validator("cache_dir")
+    def cache_dir_validator(cls, value):
+        validate_cmd(value)
+        return val_absolute_path("cache_dir")(value)

fractal_server/app/schemas/v2/task.py

@@ -11,6 +11,7 @@
 
 from .._validators import valdictkeys
 from .._validators import valstr
+from fractal_server.string_tools import validate_cmd
 
 
 class TaskCreateV2(BaseModel, extra=Extra.forbid):
@@ -43,6 +44,11 @@ def validate_commands(cls, values):
                 "Task must have at least one valid command "
                 "(parallel and/or non_parallel)"
             )
+        if command_parallel is not None:
+            validate_cmd(command_parallel)
+        if command_non_parallel is not None:
+            validate_cmd(command_non_parallel)
+
         return values
 
     _name = validator("name", allow_reuse=True)(valstr("name"))

fractal_server/ssh/_fabric.py

@@ -17,6 +17,7 @@
 from ..logger import get_logger
 from ..logger import set_logger
 from fractal_server.config import get_settings
+from fractal_server.string_tools import validate_cmd
 from fractal_server.syringe import Inject
 
 
@@ -143,6 +144,7 @@ def run_command(
         self,
         *,
         cmd: str,
+        allow_char: Optional[str] = None,
         max_attempts: Optional[int] = None,
         base_interval: Optional[int] = None,
         lock_timeout: Optional[int] = None,
@@ -152,13 +154,17 @@ def run_command(
 
         Args:
             cmd: Command to be run
+            allow_char: Forbidden chars to allow for this command
             max_attempts:
             base_interval:
             lock_timeout:
 
         Returns:
             Standard output of the command, if successful.
         """
+
+        validate_cmd(cmd, allow_char=allow_char)
+
         actual_max_attempts = self.default_max_attempts
         if max_attempts is not None:
             actual_max_attempts = max_attempts

fractal_server/string_tools.py

@@ -1,7 +1,12 @@
 import string
+from typing import Optional
 
 __SPECIAL_CHARACTERS__ = f"{string.punctuation}{string.whitespace}"
 
+# List of invalid characters discussed here:
+# https://github.com/fractal-analytics-platform/fractal-server/issues/1647
+__NOT_ALLOWED_FOR_COMMANDS__ = r"`#$&*()\|[]{};<>?!"
+
 
 def sanitize_string(value: str) -> str:
     """
@@ -43,3 +48,23 @@ def slugify_task_name_for_source(task_name: str) -> str:
         Slug-ified task name.
     """
     return task_name.replace(" ", "_").lower()
+
+
+def validate_cmd(command: str, allow_char: Optional[str] = None):
+    """
+    Assert that the provided `command` does not contain any of the forbidden
+    characters for commands
+    (fractal_server.string_tools.__NOT_ALLOWED_FOR_COMMANDS__)
+
+    Args:
+        command: command to validate.
+        allow_char: chars to accept among the forbidden ones
+    """
+    forbidden = set(__NOT_ALLOWED_FOR_COMMANDS__)
+    if allow_char is not None:
+        forbidden = forbidden - set(allow_char)
+    if not forbidden.isdisjoint(set(command)):
+        raise ValueError(
+            f"Command must not contain any of this characters: '{forbidden}'\n"
+            f"Provided command: '{command}'."
+        )

tests/no_version/test_string_tools.py

@@ -1,5 +1,9 @@
+import pytest
+
+from fractal_server.string_tools import __NOT_ALLOWED_FOR_COMMANDS__
 from fractal_server.string_tools import __SPECIAL_CHARACTERS__
 from fractal_server.string_tools import sanitize_string
+from fractal_server.string_tools import validate_cmd
 
 
 def test_unit_sanitize_string():
@@ -10,3 +14,11 @@ def test_unit_sanitize_string():
     value = "/some (rm) \t path *!"
     expected_value = "_some__rm____path___"
     assert sanitize_string(value) == expected_value
+
+
+def test_unit_validate_cmd():
+    for char in __NOT_ALLOWED_FOR_COMMANDS__:
+        cmd = f"abc{char}def"
+        with pytest.raises(ValueError):
+            validate_cmd(cmd)
+        validate_cmd(cmd, allow_char=f"xy{char}z")

tests/no_version/test_unit_schemas_no_version.py

@@ -73,14 +73,21 @@ def test_user_create():
     assert u.cache_dir == CACHE_DIR
     # With invalid cache_dir attribute
     with pytest.raises(ValidationError) as e:
-        u = UserCreate(email="a@b.c", password="asd", cache_dir="  ")
+        UserCreate(email="a@b.c", password="asd", cache_dir="  ")
     debug(e.value)
     assert "cannot be empty" in e.value.errors()[0]["msg"]
     # With invalid cache_dir attribute
     with pytest.raises(ValidationError) as e:
-        u = UserCreate(email="a@b.c", password="asd", cache_dir="xxx")
+        UserCreate(email="a@b.c", password="asd", cache_dir="xxx")
     debug(e.value)
     assert "must be an absolute path" in e.value.errors()[0]["msg"]
+    # With invalid cache_dir attribute
+    with pytest.raises(
+        ValidationError, match="must not contain any of this characters"
+    ) as e:
+        UserCreate(email="a@b.c", password="asd", cache_dir=f"{CACHE_DIR}*;")
+    debug(e.value)
+
     # With all attributes
     u = UserCreate(
         email="a@b.c",
@@ -111,6 +118,12 @@ def test_user_update_strict():
     UserUpdateStrict(slurm_accounts=None)
     UserUpdateStrict(slurm_accounts=["a", "b", "c"])
 
+    UserUpdateStrict(cache_dir="/path")
+    with pytest.raises(
+        ValidationError, match="must not contain any of this characters"
+    ):
+        UserUpdateStrict(cache_dir="/path*;")
+
 
 def test_user_group_create():
     UserGroupCreate(name="group1")

tests/v2/00_ssh/test_setup.py

@@ -33,6 +33,10 @@ def test_versions(fractal_ssh: FractalSSH, current_py_version: str):
     command = f"{python_bin} -c '{python_command}'"
 
     print(f"COMMAND:\n{command}")
-    stdout = fractal_ssh.run_command(cmd=command)
+
+    with pytest.raises(ValueError):
+        fractal_ssh.run_command(cmd=command)
+    stdout = fractal_ssh.run_command(cmd=command, allow_char=";'()")
+
     print(f"STDOUT:\n{stdout}")
     assert stdout.strip() == str(fractal_server.__VERSION__)

tests/v2/04_runner/test_unit_lowlevel_functions.py

@@ -0,0 +1,25 @@
+import pytest
+
+from fractal_server.app.runner.exceptions import TaskExecutionError
+from fractal_server.app.runner.v2._local.executor import (
+    FractalThreadPoolExecutor,
+)
+from fractal_server.app.runner.v2.runner_functions_low_level import (
+    _call_command_wrapper,
+)
+
+
+async def test_command_wrapper(tmp_path):
+    with FractalThreadPoolExecutor() as executor:
+        future1 = executor.submit(
+            _call_command_wrapper, "ls -al .", log_path="/tmp/fractal_log"
+        )
+        future2 = executor.submit(
+            _call_command_wrapper, "ls -al ./*", log_path="/tmp/fractal_log"
+        )
+
+    future1.result()
+    with pytest.raises(
+        TaskExecutionError, match="must not contain any of this characters"
+    ):
+        future2.result()