Escape runtime variables in hooks #810

New Issue

kiwiz · 2024-01-05T17:35:20Z

kiwiz commented

2024-01-05 17:35:20 +00:00

What I'm trying to do and why

Fire off an alert with an error hook when the borg job fails. The interpolate_context function doesn't escape runtime variables (like output), so shell injection is possible.

Steps to reproduce

Create a hook configuration with any runtime variable that may contain special shell characters (ex: ").

Actual behavior

No response

Expected behavior

The variables are escaped so they can be safely passed into commands

Other notes / implementation ideas

Another option is to pass runtime variables in environment variables.

borgmatic version

1.8.5

borgmatic installation method

Official image

Borg version

1.2.7

Python version

3.11.5

Database version (if applicable)

No response

Operating system and version

No response

### What I'm trying to do and why Fire off an alert with an error hook when the borg job fails. The `interpolate_context` function doesn't escape runtime variables (like `output`), so shell injection is possible. ### Steps to reproduce Create a hook configuration with any runtime variable that may contain special shell characters (ex: `"`). ### Actual behavior _No response_ ### Expected behavior The variables are escaped so they can be safely passed into commands ### Other notes / implementation ideas Another option is to pass runtime variables in environment variables. ### borgmatic version 1.8.5 ### borgmatic installation method Official image ### Borg version 1.2.7 ### Python version 3.11.5 ### Database version (if applicable) _No response_ ### Operating system and version _No response_

witten commented

2024-01-05 18:10:51 +00:00

Thanks for taking the time to file this! Just to confirm, I assume you're talking about these runtime variables on the command hooks? I agree that escaping them does seem prudent from a security perspective, even if that might technically be a breaking change for some users.

Thanks for taking the time to file this! Just to confirm, I assume you're talking about [these runtime variables](https://torsion.org/borgmatic/docs/how-to/add-preparation-and-cleanup-steps-to-backups/#variable-interpolation) on the command hooks? I agree that escaping them does seem prudent from a security perspective, even if that might technically be a breaking change for some users.

witten added the

bug

security

labels 2024-01-05 18:11:01 +00:00

kiwiz commented

2024-01-05 18:15:51 +00:00

Yep! It should be sufficient to add a call to shlex.quote inside the interpolation logic.

Yep! It should be sufficient to add a call to [`shlex.quote`](https://docs.python.org/3/library/shlex.html#shlex.quote) inside the interpolation logic.

witten commented

2024-01-05 18:16:57 +00:00

Thanks, I totally forgot about that utility function even though it's already in use elsewhere!

witten referenced this issue from a commit

2024-01-07 18:23:56 +00:00

Prevent various shell injection attacks (#810).

witten commented

2024-01-07 18:24:37 +00:00

Fixed in main. Will be part of the next release. I also took the opportunity to fix similar shell injection attacks in other parts of the codebase (various database hooks, the borgmatic borg action, constant interpolation into command hooks, etc.).

Thanks again for bringing this to my attention!

Fixed in main. Will be part of the next release. I also took the opportunity to fix similar shell injection attacks in other parts of the codebase (various database hooks, the `borgmatic borg` action, constant interpolation into command hooks, etc.). Thanks again for bringing this to my attention!

witten closed this issue

2024-01-07 18:24:37 +00:00

witten commented

2024-01-21 22:57:18 +00:00

Released in borgmatic 1.8.7!

kiwiz commented

2024-01-22 05:19:36 +00:00

Thanks!

👍 1

vilhalmer commented

2024-01-24 00:34:56 +00:00

This was indeed a breaking change for people using the old provided method for running pg_dumpall within a container. I'm ok with changing to a wrapper script (no pg_dump on the host since postgres versioning is awful), but it might be good to leave some sort of reference to the old example in the docs for people who are wondering what happened.

witten commented

2024-01-24 05:12:00 +00:00

Thanks for weighing in, and I apologize for the breakage you encountered. What's the old provided method for running pg_dumpall within a container, and how exactly did it break? The generally recommend way for dumping PostgreSQL database with borgmatic is to use the built-in database hook, although I understand if that doesn't work for all use cases.

Thanks for weighing in, and I apologize for the breakage you encountered. What's the old provided method for running `pg_dumpall` within a container, and how exactly did it break? The generally recommend way for dumping PostgreSQL database with borgmatic is to use the [built-in database hook](https://torsion.org/borgmatic/docs/how-to/backup-your-databases/), although I understand if that doesn't work for all use cases.

vilhalmer commented

2024-01-29 00:52:05 +00:00

Sorry for the delay, this got lost in my inbox. I misremembered the docker exec example as being on the databases doc page, but it's actually on the configuration reference:

# Command to use instead of "pg_dump" or "pg_dumpall".
# This can be used to run a specific pg_dump version
# (e.g., one inside a running container). Defaults to
# "pg_dump" for single database dump or "pg_dumpall" to
# dump all databases.
# pg_dump_command: docker exec my_pg_container pg_dump

Sorry for the delay, this got lost in my inbox. I misremembered the docker exec example as being on the databases doc page, but it's actually on the [configuration reference](https://torsion.org/borgmatic/docs/reference/configuration/): # Command to use instead of "pg_dump" or "pg_dumpall". # This can be used to run a specific pg_dump version # (e.g., one inside a running container). Defaults to # "pg_dump" for single database dump or "pg_dumpall" to # dump all databases. # pg_dump_command: docker exec my_pg_container pg_dump

witten commented

2024-01-29 05:39:46 +00:00

Hmm, okay, so it looks like as part of this ticket's implementation, the pg_dump_command value is now escaped. Could you describe how that broke things for you and ideally provide the error message or logs for the breakage you're seeing? Thank you!

Hmm, okay, so it looks like as part of this ticket's implementation, the `pg_dump_command` value is now escaped. Could you describe how that broke things for you and ideally provide the error message or logs for the breakage you're seeing? Thank you!

vilhalmer commented

2024-01-29 21:00:42 +00:00

It ends up passing it as a single token to the shell (in my case it's podman, but it shouldn't be any different regardless of command):

Jan 29 20:57:12 compendium borgmatic[98093]: INFO borgbase-databases: Creating archive
Jan 29 20:57:12 compendium borgmatic[98093]: INFO ssh://<snip>.repo.borgbase.com/./repo: Dumping PostgreSQL databases
Jan 29 20:57:14 compendium borgmatic[98093]: INFO Creating archive at "ssh://<snip>.repo.borgbase.com/./repo::compendium-2024-01-29T20:57:12.842636"
Jan 29 20:57:14 compendium borgmatic[98093]: INFO Remote: Storage quota: 87.63 MB out of 10.00 GB used.
Jan 29 20:57:14 compendium borgmatic[98093]: INFO Remote: Storage quota: 87.63 MB out of 10.00 GB used.
Jan 29 20:57:15 compendium borgmatic[98093]: ERROR /bin/sh: line 1: podman exec postgresql pg_dumpall: command not found
Jan 29 20:57:15 compendium borgmatic[98093]: CRITICAL borgbase-databases: Error running actions for repository
Jan 29 20:57:15 compendium borgmatic[98093]: CRITICAL Command ''podman exec postgresql pg_dumpall' --no-password --clean --if-exists --username postgres > /root/.borgmatic/postgresql_databases/localhost/all' returned non-zero exit status 127.
Jan 29 20:57:15 compendium borgmatic[98093]: CRITICAL /etc/borgmatic.d/databases.yaml: An error occurred
Jan 29 20:57:15 compendium borgmatic[98093]: CRITICAL
Jan 29 20:57:15 compendium borgmatic[98093]: CRITICAL summary:
Jan 29 20:57:15 compendium borgmatic[98093]: CRITICAL /etc/borgmatic.d/databases.yaml: An error occurred
Jan 29 20:57:15 compendium borgmatic[98093]: CRITICAL borgbase-databases: Error running actions for repository
Jan 29 20:57:15 compendium borgmatic[98093]: CRITICAL /bin/sh: line 1: podman exec postgresql pg_dumpall: command not found
Jan 29 20:57:15 compendium borgmatic[98093]: CRITICAL Command ''podman exec postgresql pg_dumpall' --no-password --clean --if-exists --username postgres > /root/.borgmatic/postgresql_databases/localhost/all' returned non-zero exit status 127.
Jan 29 20:57:15 compendium borgmatic[98093]: CRITICAL
Jan 29 20:57:15 compendium borgmatic[98093]: CRITICAL Need some help? https://torsion.org/borgmatic/#issues

It ends up passing it as a single token to the shell (in my case it's podman, but it shouldn't be any different regardless of command): Jan 29 20:57:12 compendium borgmatic[98093]: INFO borgbase-databases: Creating archive Jan 29 20:57:12 compendium borgmatic[98093]: INFO ssh://<snip>.repo.borgbase.com/./repo: Dumping PostgreSQL databases Jan 29 20:57:14 compendium borgmatic[98093]: INFO Creating archive at "ssh://<snip>.repo.borgbase.com/./repo::compendium-2024-01-29T20:57:12.842636" Jan 29 20:57:14 compendium borgmatic[98093]: INFO Remote: Storage quota: 87.63 MB out of 10.00 GB used. Jan 29 20:57:14 compendium borgmatic[98093]: INFO Remote: Storage quota: 87.63 MB out of 10.00 GB used. Jan 29 20:57:15 compendium borgmatic[98093]: ERROR /bin/sh: line 1: podman exec postgresql pg_dumpall: command not found Jan 29 20:57:15 compendium borgmatic[98093]: CRITICAL borgbase-databases: Error running actions for repository Jan 29 20:57:15 compendium borgmatic[98093]: CRITICAL Command ''podman exec postgresql pg_dumpall' --no-password --clean --if-exists --username postgres > /root/.borgmatic/postgresql_databases/localhost/all' returned non-zero exit status 127. Jan 29 20:57:15 compendium borgmatic[98093]: CRITICAL /etc/borgmatic.d/databases.yaml: An error occurred Jan 29 20:57:15 compendium borgmatic[98093]: CRITICAL Jan 29 20:57:15 compendium borgmatic[98093]: CRITICAL summary: Jan 29 20:57:15 compendium borgmatic[98093]: CRITICAL /etc/borgmatic.d/databases.yaml: An error occurred Jan 29 20:57:15 compendium borgmatic[98093]: CRITICAL borgbase-databases: Error running actions for repository Jan 29 20:57:15 compendium borgmatic[98093]: CRITICAL /bin/sh: line 1: podman exec postgresql pg_dumpall: command not found Jan 29 20:57:15 compendium borgmatic[98093]: CRITICAL Command ''podman exec postgresql pg_dumpall' --no-password --clean --if-exists --username postgres > /root/.borgmatic/postgresql_databases/localhost/all' returned non-zero exit status 127. Jan 29 20:57:15 compendium borgmatic[98093]: CRITICAL Jan 29 20:57:15 compendium borgmatic[98093]: CRITICAL Need some help? https://torsion.org/borgmatic/#issues

witten referenced this issue

2024-01-30 19:03:05 +00:00

Escaping logic in pg_dump_command option breaks on spaces #822

witten commented

2024-01-30 19:03:30 +00:00

I don't think this is a documentation gap.. It's a straight-up bug! I've filed the issue as a new ticket: #822. Please feel free to follow along there.