Prevent various shell injection attacks (#810).

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

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,

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

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(

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 ())
     )
 
 

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(

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}'

setup.py

@@ -1,6 +1,6 @@
 from setuptools import find_packages, setup
 
-VERSION = '1.8.6'
+VERSION = '1.8.7.dev0'
 
 
 setup(

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

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

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

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())

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'})

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'},