From 3c22a8ec164087beb1d292dc114f78f8b6382ae2 Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Sun, 7 Jan 2024 10:21:49 -0800 Subject: [PATCH] Prevent various shell injection attacks (#810). --- NEWS | 4 ++++ borgmatic/borg/borg.py | 3 ++- borgmatic/config/constants.py | 22 ++++++++++++++---- borgmatic/hooks/command.py | 3 ++- borgmatic/hooks/mongodb.py | 23 ++++++++++-------- borgmatic/hooks/postgresql.py | 26 +++++++++++++-------- borgmatic/hooks/sqlite.py | 5 ++-- setup.py | 2 +- tests/unit/borg/test_borg.py | 26 ++++++++++++++++++++- tests/unit/config/test_constants.py | 12 +++++++++- tests/unit/hooks/test_command.py | 10 ++++++++ tests/unit/hooks/test_mongodb.py | 8 +++++++ tests/unit/hooks/test_postgresql.py | 36 +++++++++++++++++++++++++++++ tests/unit/hooks/test_sqlite.py | 27 ++++++++++++++++++++++ 14 files changed, 178 insertions(+), 29 deletions(-) diff --git a/NEWS b/NEWS index 229df3d..a794485 100644 --- a/NEWS +++ b/NEWS @@ -1,3 +1,7 @@ +1.8.7.dev0 + * #810: SECURITY: Prevent shell injection attacks within the PostgreSQL hook, the MongoDB hook, the + SQLite hook, the "borgmatic borg" action, and command hook variable/constant interpolation. + 1.8.6 * #767: Add an "--ssh-command" flag to the "config bootstrap" action for setting a custom SSH command, as no configuration is available (including the "ssh_command" option) until diff --git a/borgmatic/borg/borg.py b/borgmatic/borg/borg.py index 1c0d6d1..2af0188 100644 --- a/borgmatic/borg/borg.py +++ b/borgmatic/borg/borg.py @@ -1,4 +1,5 @@ import logging +import shlex import borgmatic.commands.arguments import borgmatic.logger @@ -56,7 +57,7 @@ def run_arbitrary_borg( ) return execute_command( - full_command, + tuple(shlex.quote(part) for part in full_command), output_file=DO_NOT_CAPTURE, borg_local_path=local_path, shell=True, diff --git a/borgmatic/config/constants.py b/borgmatic/config/constants.py index d713d34..99f461b 100644 --- a/borgmatic/config/constants.py +++ b/borgmatic/config/constants.py @@ -1,3 +1,6 @@ +import shlex + + def coerce_scalar(value): ''' Given a configuration value, coerce it to an integer or a boolean as appropriate and return the @@ -16,7 +19,7 @@ def coerce_scalar(value): return value -def apply_constants(value, constants): +def apply_constants(value, constants, shell_escape=False): ''' Given a configuration value (bool, dict, int, list, or string) and a dict of named constants, replace any configuration string values of the form "{constant}" (or containing it) with the @@ -26,6 +29,8 @@ def apply_constants(value, constants): For instance, if a configuration value contains "{foo}", replace it with the value of the "foo" key found within the configuration's "constants". + If shell escape is True, then escape the constant's value before applying it. + Return the configuration value and modify the original. ''' if not value or not constants: @@ -33,15 +38,24 @@ def apply_constants(value, constants): if isinstance(value, str): for constant_name, constant_value in constants.items(): - value = value.replace('{' + constant_name + '}', str(constant_value)) + value = value.replace( + '{' + constant_name + '}', + shlex.quote(str(constant_value)) if shell_escape else str(constant_value), + ) # Support constants within non-string scalars by coercing the value to its appropriate type. value = coerce_scalar(value) elif isinstance(value, list): for index, list_value in enumerate(value): - value[index] = apply_constants(list_value, constants) + value[index] = apply_constants(list_value, constants, shell_escape) elif isinstance(value, dict): for option_name, option_value in value.items(): - value[option_name] = apply_constants(option_value, constants) + shell_escape = ( + shell_escape + or option_name.startswith('before_') + or option_name.startswith('after_') + or option_name == 'on_error' + ) + value[option_name] = apply_constants(option_value, constants, shell_escape) return value diff --git a/borgmatic/hooks/command.py b/borgmatic/hooks/command.py index 05f7d2f..299c1a1 100644 --- a/borgmatic/hooks/command.py +++ b/borgmatic/hooks/command.py @@ -1,6 +1,7 @@ import logging import os import re +import shlex from borgmatic import execute @@ -16,7 +17,7 @@ def interpolate_context(config_filename, hook_description, command, context): names/values, interpolate the values by "{name}" into the command and return the result. ''' for name, value in context.items(): - command = command.replace(f'{{{name}}}', str(value)) + command = command.replace(f'{{{name}}}', shlex.quote(str(value))) for unsupported_variable in re.findall(r'{\w+}', command): logger.warning( diff --git a/borgmatic/hooks/mongodb.py b/borgmatic/hooks/mongodb.py index 5a3a897..dbea768 100644 --- a/borgmatic/hooks/mongodb.py +++ b/borgmatic/hooks/mongodb.py @@ -1,4 +1,5 @@ import logging +import shlex from borgmatic.execute import execute_command, execute_command_with_processes from borgmatic.hooks import dump @@ -62,19 +63,23 @@ def build_dump_command(database, dump_filename, dump_format): return ( ('mongodump',) - + (('--out', dump_filename) if dump_format == 'directory' else ()) - + (('--host', database['hostname']) if 'hostname' in database else ()) - + (('--port', str(database['port'])) if 'port' in database else ()) - + (('--username', database['username']) if 'username' in database else ()) - + (('--password', database['password']) if 'password' in database else ()) + + (('--out', shlex.quote(dump_filename)) if dump_format == 'directory' else ()) + + (('--host', shlex.quote(database['hostname'])) if 'hostname' in database else ()) + + (('--port', shlex.quote(str(database['port']))) if 'port' in database else ()) + + (('--username', shlex.quote(database['username'])) if 'username' in database else ()) + + (('--password', shlex.quote(database['password'])) if 'password' in database else ()) + ( - ('--authenticationDatabase', database['authentication_database']) + ('--authenticationDatabase', shlex.quote(database['authentication_database'])) if 'authentication_database' in database else () ) - + (('--db', database['name']) if not all_databases else ()) - + (tuple(database['options'].split(' ')) if 'options' in database else ()) - + (('--archive', '>', dump_filename) if dump_format != 'directory' else ()) + + (('--db', shlex.quote(database['name'])) if not all_databases else ()) + + ( + tuple(shlex.quote(option) for option in database['options'].split(' ')) + if 'options' in database + else () + ) + + (('--archive', '>', shlex.quote(dump_filename)) if dump_format != 'directory' else ()) ) diff --git a/borgmatic/hooks/postgresql.py b/borgmatic/hooks/postgresql.py index c7fc440..d89ac81 100644 --- a/borgmatic/hooks/postgresql.py +++ b/borgmatic/hooks/postgresql.py @@ -137,23 +137,31 @@ def dump_data_sources(databases, config, log_prefix, dry_run): command = ( ( - dump_command, + shlex.quote(dump_command), '--no-password', '--clean', '--if-exists', ) - + (('--host', database['hostname']) if 'hostname' in database else ()) - + (('--port', str(database['port'])) if 'port' in database else ()) - + (('--username', database['username']) if 'username' in database else ()) + + (('--host', shlex.quote(database['hostname'])) if 'hostname' in database else ()) + + (('--port', shlex.quote(str(database['port']))) if 'port' in database else ()) + + ( + ('--username', shlex.quote(database['username'])) + if 'username' in database + else () + ) + (('--no-owner',) if database.get('no_owner', False) else ()) - + (('--format', dump_format) if dump_format else ()) - + (('--file', dump_filename) if dump_format == 'directory' else ()) - + (tuple(database['options'].split(' ')) if 'options' in database else ()) - + (() if database_name == 'all' else (database_name,)) + + (('--format', shlex.quote(dump_format)) if dump_format else ()) + + (('--file', shlex.quote(dump_filename)) if dump_format == 'directory' else ()) + + ( + tuple(shlex.quote(option) for option in database['options'].split(' ')) + if 'options' in database + else () + ) + + (() if database_name == 'all' else (shlex.quote(database_name),)) # Use shell redirection rather than the --file flag to sidestep synchronization issues # when pg_dump/pg_dumpall tries to write to a named pipe. But for the directory dump # format in a particular, a named destination is required, and redirection doesn't work. - + (('>', dump_filename) if dump_format != 'directory' else ()) + + (('>', shlex.quote(dump_filename)) if dump_format != 'directory' else ()) ) logger.debug( diff --git a/borgmatic/hooks/sqlite.py b/borgmatic/hooks/sqlite.py index 22f0001..5ac55fe 100644 --- a/borgmatic/hooks/sqlite.py +++ b/borgmatic/hooks/sqlite.py @@ -1,5 +1,6 @@ import logging import os +import shlex from borgmatic.execute import execute_command, execute_command_with_processes from borgmatic.hooks import dump @@ -51,10 +52,10 @@ def dump_data_sources(databases, config, log_prefix, dry_run): command = ( 'sqlite3', - database_path, + shlex.quote(database_path), '.dump', '>', - dump_filename, + shlex.quote(dump_filename), ) logger.debug( f'{log_prefix}: Dumping SQLite database at {database_path} to {dump_filename}{dry_run_label}' diff --git a/setup.py b/setup.py index cc59833..adea55c 100644 --- a/setup.py +++ b/setup.py @@ -1,6 +1,6 @@ from setuptools import find_packages, setup -VERSION = '1.8.6' +VERSION = '1.8.7.dev0' setup( diff --git a/tests/unit/borg/test_borg.py b/tests/unit/borg/test_borg.py index f38ec0e..a132b7a 100644 --- a/tests/unit/borg/test_borg.py +++ b/tests/unit/borg/test_borg.py @@ -102,7 +102,7 @@ def test_run_arbitrary_borg_with_archive_calls_borg_with_archive_flag(): flexmock(module.flags).should_receive('make_flags').and_return(()) flexmock(module.environment).should_receive('make_environment') flexmock(module).should_receive('execute_command').with_args( - ('borg', 'break-lock', '::$ARCHIVE'), + ('borg', 'break-lock', "'::$ARCHIVE'"), output_file=module.borgmatic.execute.DO_NOT_CAPTURE, borg_local_path='borg', shell=True, @@ -164,6 +164,30 @@ def test_run_arbitrary_borg_with_remote_path_calls_borg_with_remote_path_flags() ) +def test_run_arbitrary_borg_with_remote_path_injection_attack_gets_escaped(): + flexmock(module.borgmatic.logger).should_receive('add_custom_log_levels') + flexmock(module.logging).ANSWER = module.borgmatic.logger.ANSWER + flexmock(module.flags).should_receive('make_flags').and_return( + ('--remote-path', 'borg1; naughty-command') + ).and_return(()) + flexmock(module.environment).should_receive('make_environment') + flexmock(module).should_receive('execute_command').with_args( + ('borg', 'break-lock', '--remote-path', "'borg1; naughty-command'", '::'), + output_file=module.borgmatic.execute.DO_NOT_CAPTURE, + borg_local_path='borg', + shell=True, + extra_environment={'BORG_REPO': 'repo', 'ARCHIVE': ''}, + ) + + module.run_arbitrary_borg( + repository_path='repo', + config={}, + local_borg_version='1.2.3', + options=['break-lock', '::'], + remote_path='borg1', + ) + + def test_run_arbitrary_borg_passes_borg_specific_flags_to_borg(): flexmock(module.borgmatic.logger).should_receive('add_custom_log_levels') flexmock(module.logging).ANSWER = module.borgmatic.logger.ANSWER diff --git a/tests/unit/config/test_constants.py b/tests/unit/config/test_constants.py index 4383a09..dbb3b77 100644 --- a/tests/unit/config/test_constants.py +++ b/tests/unit/config/test_constants.py @@ -46,6 +46,10 @@ def test_apply_constants_with_empty_constants_passes_through_value(): (['{foo}', '{baz}'], ['bar', 'quux']), ({'key': 'value'}, {'key': 'value'}), ({'key': '{foo}'}, {'key': 'bar'}), + ({'key': '{inject}'}, {'key': 'echo hi; naughty-command'}), + ({'before_backup': '{inject}'}, {'before_backup': "'echo hi; naughty-command'"}), + ({'after_backup': '{inject}'}, {'after_backup': "'echo hi; naughty-command'"}), + ({'on_error': '{inject}'}, {'on_error': "'echo hi; naughty-command'"}), (3, 3), (True, True), (False, False), @@ -53,6 +57,12 @@ def test_apply_constants_with_empty_constants_passes_through_value(): ) def test_apply_constants_makes_string_substitutions(value, expected_value): flexmock(module).should_receive('coerce_scalar').replace_with(lambda value: value) - constants = {'foo': 'bar', 'baz': 'quux', 'int': 3, 'bool': True} + constants = { + 'foo': 'bar', + 'baz': 'quux', + 'int': 3, + 'bool': True, + 'inject': 'echo hi; naughty-command', + } assert module.apply_constants(value, constants) == expected_value diff --git a/tests/unit/hooks/test_command.py b/tests/unit/hooks/test_command.py index 3a657eb..2ba7ac1 100644 --- a/tests/unit/hooks/test_command.py +++ b/tests/unit/hooks/test_command.py @@ -25,6 +25,16 @@ def test_interpolate_context_interpolates_variables(): ) +def test_interpolate_context_escapes_interpolated_variables(): + command = 'ls {foo} {inject}' # noqa: FS003 + context = {'foo': 'bar', 'inject': 'hi; naughty-command'} + + assert ( + module.interpolate_context('test.yaml', 'pre-backup', command, context) + == "ls bar 'hi; naughty-command'" + ) + + def test_execute_hook_invokes_each_command(): flexmock(module).should_receive('interpolate_context').replace_with( lambda config_file, hook_description, command, context: command diff --git a/tests/unit/hooks/test_mongodb.py b/tests/unit/hooks/test_mongodb.py index d63845e..2fab040 100644 --- a/tests/unit/hooks/test_mongodb.py +++ b/tests/unit/hooks/test_mongodb.py @@ -164,6 +164,14 @@ def test_dump_data_sources_runs_mongodumpall_for_all_databases(): assert module.dump_data_sources(databases, {}, 'test.yaml', dry_run=False) == [process] +def test_build_dump_command_with_username_injection_attack_gets_escaped(): + database = {'name': 'test', 'username': 'bob; naughty-command'} + + command = module.build_dump_command(database, dump_filename='test', dump_format='archive') + + assert "'bob; naughty-command'" in command + + def test_restore_data_source_dump_runs_mongorestore(): hook_config = [{'name': 'foo', 'schemas': None}, {'name': 'bar'}] extract_process = flexmock(stdout=flexmock()) diff --git a/tests/unit/hooks/test_postgresql.py b/tests/unit/hooks/test_postgresql.py index c89733e..6ced44a 100644 --- a/tests/unit/hooks/test_postgresql.py +++ b/tests/unit/hooks/test_postgresql.py @@ -345,6 +345,42 @@ def test_dump_data_sources_runs_pg_dump_with_username_and_password(): assert module.dump_data_sources(databases, {}, 'test.yaml', dry_run=False) == [process] +def test_dump_data_sources_with_username_injection_attack_gets_escaped(): + databases = [{'name': 'foo', 'username': 'postgres; naughty-command', 'password': 'trustsome1'}] + process = flexmock() + flexmock(module).should_receive('make_extra_environment').and_return( + {'PGPASSWORD': 'trustsome1', 'PGSSLMODE': 'disable'} + ) + flexmock(module).should_receive('make_dump_path').and_return('') + flexmock(module).should_receive('database_names_to_dump').and_return(('foo',)) + flexmock(module.dump).should_receive('make_data_source_dump_filename').and_return( + 'databases/localhost/foo' + ) + flexmock(module.os.path).should_receive('exists').and_return(False) + flexmock(module.dump).should_receive('create_named_pipe_for_dump') + + flexmock(module).should_receive('execute_command').with_args( + ( + 'pg_dump', + '--no-password', + '--clean', + '--if-exists', + '--username', + "'postgres; naughty-command'", + '--format', + 'custom', + 'foo', + '>', + 'databases/localhost/foo', + ), + shell=True, + extra_environment={'PGPASSWORD': 'trustsome1', 'PGSSLMODE': 'disable'}, + run_to_completion=False, + ).and_return(process).once() + + assert module.dump_data_sources(databases, {}, 'test.yaml', dry_run=False) == [process] + + def test_dump_data_sources_runs_pg_dump_with_directory_format(): databases = [{'name': 'foo', 'format': 'directory'}] flexmock(module).should_receive('make_extra_environment').and_return({'PGSSLMODE': 'disable'}) diff --git a/tests/unit/hooks/test_sqlite.py b/tests/unit/hooks/test_sqlite.py index 06ae938..d74e56a 100644 --- a/tests/unit/hooks/test_sqlite.py +++ b/tests/unit/hooks/test_sqlite.py @@ -39,6 +39,33 @@ def test_dump_data_sources_dumps_each_database(): assert module.dump_data_sources(databases, {}, 'test.yaml', dry_run=False) == processes +def test_dump_data_sources_with_path_injection_attack_gets_escaped(): + databases = [ + {'path': '/path/to/database1; naughty-command', 'name': 'database1'}, + ] + processes = [flexmock()] + + flexmock(module).should_receive('make_dump_path').and_return('/path/to/dump') + flexmock(module.dump).should_receive('make_data_source_dump_filename').and_return( + '/path/to/dump/database' + ) + flexmock(module.os.path).should_receive('exists').and_return(False) + flexmock(module.dump).should_receive('create_named_pipe_for_dump') + flexmock(module).should_receive('execute_command').with_args( + ( + 'sqlite3', + "'/path/to/database1; naughty-command'", + '.dump', + '>', + '/path/to/dump/database', + ), + shell=True, + run_to_completion=False, + ).and_return(processes[0]) + + assert module.dump_data_sources(databases, {}, 'test.yaml', dry_run=False) == processes + + def test_dump_data_sources_with_non_existent_path_warns_and_dumps_database(): databases = [ {'path': '/path/to/database1', 'name': 'database1'},