Skip to content

fix(restore): shell-quote restore_command arguments - #10518

Merged
mnencia merged 3 commits into
mainfrom
dev/shell-quote-barman
Apr 30, 2026
Merged

mnencia merged 3 commits into
mainfrom
dev/shell-quote-barman

Conversation

@armru

@armru armru commented Apr 22, 2026 •

Copy link
Copy Markdown
Member

getRestoreWalConfig joined user-controlled backup status fields (EndpointURL, DestinationPath, ServerName) with spaces into a single restore_command string. PostgreSQL runs restore_command via system(), and the POSIX shell word-splits unquoted whitespace, so a DestinationPath like "s3://my bucket/wal" would be passed to barman-cloud-wal-restore as separate arguments.

This bug predates #10506 but was unreachable for inputs containing single quotes or backslashes because such values corrupted the config file first. Now that the config file is well-formed, the shell layer needs its own quoting.

Wrap each cmd element via github.com/kballard/go-shellquote before passing the joined value through configfile.RenderPostgresConfiguration. The result is two layered quoting passes: shell-safe single quotes around any argument with whitespace or shell metacharacters, and the standard config-file literal escape applied by the helper.

Related #10515

@armru
armru requested a review from a team April 22, 2026 16:14
@dosubot dosubot Bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Apr 22, 2026
@cnpg-bot cnpg-bot added backport-requested ◀️ This pull request should be backported to all supported releases release-1.25 release-1.28 labels Apr 22, 2026
@github-actions

Copy link
Copy Markdown
Contributor

❗ By default, the pull request is configured to backport to all release branches.

  • To stop backporting this pr, remove the label: backport-requested ◀️ or add the label 'do not backport'
  • To stop backporting this pr to a certain release branch, remove the specific branch label: release-x.y

@dosubot dosubot Bot added bug 🐛 Something isn't working security 👮 labels Apr 22, 2026
@armru
armru force-pushed the dev/shell-quote-barman branch from fee3056 to c3c5fed Compare April 22, 2026 16:15
@armru
armru changed the base branch from main to dev/10506 April 23, 2026 11:50
@dosubot dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Apr 23, 2026
@armru armru added the no-issue label Apr 23, 2026
@armru

armru commented Apr 24, 2026

Copy link
Copy Markdown
Member Author

/test

@github-actions

Copy link
Copy Markdown
Contributor

@dosubot dosubot Bot added size:XS This PR changes 0-9 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Apr 24, 2026
@cnpg-bot cnpg-bot added the ok to merge 👌 This PR can be merged label Apr 24, 2026
@dosubot dosubot Bot added lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Apr 27, 2026
@mnencia
mnencia force-pushed the dev/shell-quote-barman branch from 66b3883 to 7b0443f Compare April 27, 2026 17:35
@dosubot dosubot Bot removed the size:L This PR changes 100-499 lines, ignoring generated files. label Apr 27, 2026
@dosubot dosubot Bot added size:XS This PR changes 0-9 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Apr 28, 2026
@mnencia mnencia changed the title fix: tighten recovery target validation fix(restore): shell-quote restore_command arguments Apr 28, 2026
@mnencia
mnencia force-pushed the dev/shell-quote-barman branch from d8dee34 to c36015f Compare April 28, 2026 16:14
mnencia added a commit that referenced this pull request Apr 29, 2026
…#10515)

Values written into custom.conf were only escaped for single quotes.
Backslashes and control characters (LF, CR, TAB, BS, FF) passed through
unescaped, which either corrupted the configuration (e.g. a
user-supplied recovery_target_name containing ' or a newline) or
silently mangled the runtime value (a \ in log_line_prefix stripped by
PostgreSQL's config lexer).

Introduce configfile.RenderPostgresConfiguration as the single canonical
helper that emits a postgresql.conf-style fragment from a
map[string]string, with values escaped according to the guc-file.l rules
(\, ', \n, \r, \t, \b, \f). Migrate every config-file-emitting site to
it: pkg/configfile.UpdateConfigurationContents (replacing
pq.QuoteLiteral, which emits an E'...' SQL-style literal that the config
lexer rejects when the value contains a backslash),
pkg/postgres.CreatePostgresqlConfFile,
api/v1.(*RecoveryTarget).BuildPostgresOptions (previously did no
escaping at all for recovery_target_name/_xid/_lsn/_timeline/_time), and
pkg/management/postgres.getRestoreWalConfig and RestoreSnapshot's
restore_command construction. After this change the inner
escapePostgresConfLiteral is private to pkg/configfile.

Upgrade impact: clusters whose PostgreSQL parameters contain \ or a
control character (most commonly a literal tab in log_line_prefix) will
see one pg_reload_conf() on the first reconcile after upgrade because
the config-file SHA256 changes. No restart, no connection drops.
Clusters with only default or quote-only special values are unaffected.

Related #10518
Closes #10506

Reported-by: Koda Reef <[email protected]>
Signed-off-by: Armando Ruocco <[email protected]>
Signed-off-by: Marco Nenciarini <[email protected]>
Signed-off-by: Niccolò Fei <[email protected]>
Co-authored-by: Marco Nenciarini <[email protected]>
Co-authored-by: Niccolò Fei <[email protected]>
Base automatically changed from dev/10506 to main April 29, 2026 17:48
@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Apr 29, 2026
cnpg-bot pushed a commit that referenced this pull request Apr 29, 2026
…#10515)

Values written into custom.conf were only escaped for single quotes.
Backslashes and control characters (LF, CR, TAB, BS, FF) passed through
unescaped, which either corrupted the configuration (e.g. a
user-supplied recovery_target_name containing ' or a newline) or
silently mangled the runtime value (a \ in log_line_prefix stripped by
PostgreSQL's config lexer).

Introduce configfile.RenderPostgresConfiguration as the single canonical
helper that emits a postgresql.conf-style fragment from a
map[string]string, with values escaped according to the guc-file.l rules
(\, ', \n, \r, \t, \b, \f). Migrate every config-file-emitting site to
it: pkg/configfile.UpdateConfigurationContents (replacing
pq.QuoteLiteral, which emits an E'...' SQL-style literal that the config
lexer rejects when the value contains a backslash),
pkg/postgres.CreatePostgresqlConfFile,
api/v1.(*RecoveryTarget).BuildPostgresOptions (previously did no
escaping at all for recovery_target_name/_xid/_lsn/_timeline/_time), and
pkg/management/postgres.getRestoreWalConfig and RestoreSnapshot's
restore_command construction. After this change the inner
escapePostgresConfLiteral is private to pkg/configfile.

Upgrade impact: clusters whose PostgreSQL parameters contain \ or a
control character (most commonly a literal tab in log_line_prefix) will
see one pg_reload_conf() on the first reconcile after upgrade because
the config-file SHA256 changes. No restart, no connection drops.
Clusters with only default or quote-only special values are unaffected.

Related #10518
Closes #10506

Reported-by: Koda Reef <[email protected]>
Signed-off-by: Armando Ruocco <[email protected]>
Signed-off-by: Marco Nenciarini <[email protected]>
Signed-off-by: Niccolò Fei <[email protected]>
Co-authored-by: Marco Nenciarini <[email protected]>
Co-authored-by: Niccolò Fei <[email protected]>
(cherry picked from commit d65da47)
@mnencia
mnencia force-pushed the dev/shell-quote-barman branch from c36015f to fa5dec1 Compare April 29, 2026 17:53
@dosubot dosubot Bot added size:XS This PR changes 0-9 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Apr 29, 2026
mnencia added a commit that referenced this pull request Apr 30, 2026
…#10515)

Values written into custom.conf were only escaped for single quotes.
Backslashes and control characters (LF, CR, TAB, BS, FF) passed through
unescaped, which either corrupted the configuration (e.g. a
user-supplied recovery_target_name containing ' or a newline) or
silently mangled the runtime value (a \ in log_line_prefix stripped by
PostgreSQL's config lexer).

Introduce configfile.RenderPostgresConfiguration as the single canonical
helper that emits a postgresql.conf-style fragment from a
map[string]string, with values escaped according to the guc-file.l rules
(\, ', \n, \r, \t, \b, \f). Migrate every config-file-emitting site to
it: pkg/configfile.UpdateConfigurationContents (replacing
pq.QuoteLiteral, which emits an E'...' SQL-style literal that the config
lexer rejects when the value contains a backslash),
pkg/postgres.CreatePostgresqlConfFile,
api/v1.(*RecoveryTarget).BuildPostgresOptions (previously did no
escaping at all for recovery_target_name/_xid/_lsn/_timeline/_time), and
pkg/management/postgres.getRestoreWalConfig and RestoreSnapshot's
restore_command construction. After this change the inner
escapePostgresConfLiteral is private to pkg/configfile.

Upgrade impact: clusters whose PostgreSQL parameters contain \ or a
control character (most commonly a literal tab in log_line_prefix) will
see one pg_reload_conf() on the first reconcile after upgrade because
the config-file SHA256 changes. No restart, no connection drops.
Clusters with only default or quote-only special values are unaffected.

Related #10518
Closes #10506

Reported-by: Koda Reef <[email protected]>
Signed-off-by: Armando Ruocco <[email protected]>
Signed-off-by: Marco Nenciarini <[email protected]>
Signed-off-by: Niccolò Fei <[email protected]>
Co-authored-by: Marco Nenciarini <[email protected]>
Co-authored-by: Niccolò Fei <[email protected]>
(cherry picked from commit d65da47)
mnencia added a commit that referenced this pull request Apr 30, 2026
…#10515)

Values written into custom.conf were only escaped for single quotes.
Backslashes and control characters (LF, CR, TAB, BS, FF) passed through
unescaped, which either corrupted the configuration (e.g. a
user-supplied recovery_target_name containing ' or a newline) or
silently mangled the runtime value (a \ in log_line_prefix stripped by
PostgreSQL's config lexer).

Introduce configfile.RenderPostgresConfiguration as the single canonical
helper that emits a postgresql.conf-style fragment from a
map[string]string, with values escaped according to the guc-file.l rules
(\, ', \n, \r, \t, \b, \f). Migrate every config-file-emitting site to
it: pkg/configfile.UpdateConfigurationContents (replacing
pq.QuoteLiteral, which emits an E'...' SQL-style literal that the config
lexer rejects when the value contains a backslash),
pkg/postgres.CreatePostgresqlConfFile,
api/v1.(*RecoveryTarget).BuildPostgresOptions (previously did no
escaping at all for recovery_target_name/_xid/_lsn/_timeline/_time), and
pkg/management/postgres.getRestoreWalConfig and RestoreSnapshot's
restore_command construction. After this change the inner
escapePostgresConfLiteral is private to pkg/configfile.

Upgrade impact: clusters whose PostgreSQL parameters contain \ or a
control character (most commonly a literal tab in log_line_prefix) will
see one pg_reload_conf() on the first reconcile after upgrade because
the config-file SHA256 changes. No restart, no connection drops.
Clusters with only default or quote-only special values are unaffected.

Related #10518
Closes #10506

Reported-by: Koda Reef <[email protected]>
Signed-off-by: Armando Ruocco <[email protected]>
Signed-off-by: Marco Nenciarini <[email protected]>
Signed-off-by: Niccolò Fei <[email protected]>
Co-authored-by: Marco Nenciarini <[email protected]>
Co-authored-by: Niccolò Fei <[email protected]>
(cherry picked from commit d65da47)
armru and others added 3 commits April 30, 2026 15:44
getRestoreWalConfig joined user-controlled backup status fields
(EndpointURL, DestinationPath, ServerName) with spaces into a
single-string restore_command. PostgreSQL runs restore_command via
system(), and the POSIX shell word-splits unquoted whitespace, so a
DestinationPath like "s3://my bucket/wal" would be passed to
barman-cloud-wal-restore as separate arguments.

This bug predates #10506 but was unreachable for inputs containing
single quotes or backslashes because such values corrupted the
config file first. Now that the config file is well-formed, the
shell layer needs its own quoting.

Wrap each cmd element in single quotes (with embedded quotes
escaped as '\'') before joining.

Signed-off-by: Armando Ruocco <[email protected]>
Signed-off-by: Leonardo Cecchi <[email protected]>
Add an explicit shell-injection regression test for getRestoreWalConfig
and document why RestoreSnapshot's restore_command does not need a
shell-quoting layer.

Signed-off-by: Marco Nenciarini <[email protected]>
@mnencia
mnencia force-pushed the dev/shell-quote-barman branch from fa5dec1 to 5188d3a Compare April 30, 2026 13:44
@mnencia
mnencia merged commit ed882c2 into main Apr 30, 2026
42 checks passed
@mnencia
mnencia deleted the dev/shell-quote-barman branch April 30, 2026 13:58
@github-project-automation github-project-automation Bot moved this from Todo to Done in Security Map Apr 30, 2026
cnpg-bot pushed a commit that referenced this pull request Apr 30, 2026
getRestoreWalConfig joined user-controlled backup status fields
(EndpointURL, DestinationPath, ServerName) with spaces into a single
restore_command string. PostgreSQL runs restore_command via system(),
and the POSIX shell word-splits unquoted whitespace, so a
DestinationPath like "s3://my bucket/wal" would be passed to
barman-cloud-wal-restore as separate arguments.

This bug predates #10506 but was unreachable for inputs containing
single quotes or backslashes because such values corrupted the config
file first. Now that the config file is well-formed, the shell layer
needs its own quoting.

Wrap each cmd element via github.com/kballard/go-shellquote before
passing the joined value through configfile.RenderPostgresConfiguration.
The result is two layered quoting passes: shell-safe single quotes
around any argument with whitespace or shell metacharacters, and the
standard config-file literal escape applied by the helper.

Related #10515

Signed-off-by: Armando Ruocco <[email protected]>
Signed-off-by: Leonardo Cecchi <[email protected]>
Signed-off-by: Marco Nenciarini <[email protected]>
Co-authored-by: Leonardo Cecchi <[email protected]>
Co-authored-by: Marco Nenciarini <[email protected]>
(cherry picked from commit ed882c2)
cnpg-bot pushed a commit that referenced this pull request Apr 30, 2026
getRestoreWalConfig joined user-controlled backup status fields
(EndpointURL, DestinationPath, ServerName) with spaces into a single
restore_command string. PostgreSQL runs restore_command via system(),
and the POSIX shell word-splits unquoted whitespace, so a
DestinationPath like "s3://my bucket/wal" would be passed to
barman-cloud-wal-restore as separate arguments.

This bug predates #10506 but was unreachable for inputs containing
single quotes or backslashes because such values corrupted the config
file first. Now that the config file is well-formed, the shell layer
needs its own quoting.

Wrap each cmd element via github.com/kballard/go-shellquote before
passing the joined value through configfile.RenderPostgresConfiguration.
The result is two layered quoting passes: shell-safe single quotes
around any argument with whitespace or shell metacharacters, and the
standard config-file literal escape applied by the helper.

Related #10515

Signed-off-by: Armando Ruocco <[email protected]>
Signed-off-by: Leonardo Cecchi <[email protected]>
Signed-off-by: Marco Nenciarini <[email protected]>
Co-authored-by: Leonardo Cecchi <[email protected]>
Co-authored-by: Marco Nenciarini <[email protected]>
(cherry picked from commit ed882c2)
mnencia added a commit that referenced this pull request May 5, 2026
getRestoreWalConfig joined user-controlled backup status fields
(EndpointURL, DestinationPath, ServerName) with spaces into a single
restore_command string. PostgreSQL runs restore_command via system(),
and the POSIX shell word-splits unquoted whitespace, so a
DestinationPath like "s3://my bucket/wal" would be passed to
barman-cloud-wal-restore as separate arguments.

This bug predates #10506 but was unreachable for inputs containing
single quotes or backslashes because such values corrupted the config
file first. Now that the config file is well-formed, the shell layer
needs its own quoting.

Wrap each cmd element via github.com/kballard/go-shellquote before
passing the joined value through configfile.RenderPostgresConfiguration.
The result is two layered quoting passes: shell-safe single quotes
around any argument with whitespace or shell metacharacters, and the
standard config-file literal escape applied by the helper.

Related #10515

Signed-off-by: Armando Ruocco <[email protected]>
Signed-off-by: Leonardo Cecchi <[email protected]>
Signed-off-by: Marco Nenciarini <[email protected]>
Co-authored-by: Leonardo Cecchi <[email protected]>
Co-authored-by: Marco Nenciarini <[email protected]>
(cherry picked from commit ed882c2)
sdwilsh pushed a commit to sdwilsh/ansible-playbooks that referenced this pull request May 11, 2026
##### [\`v1.29.1\`](https://github.com/cloudnative-pg/cloudnative-pg/releases/tag/v1.29.1)

**Release date:** May 8, 2026

##### Security and Supply Chain

- **`CVE-2026-44477` / `GHSA-423p-g724-fr39`: metrics exporter privilege escalation**: the metrics exporter no longer authenticates as the `postgres` superuser. It now uses a dedicated `cnpg_metrics_exporter` role with `pg_monitor` privileges only, closing a chain that let a low-privilege database user gain PostgreSQL superuser. ([`GHSA-423p-g724-fr39`](GHSA-423p-g724-fr39)) <!-- 1.29 1.28 1.25 -->

  Upgrade impact: custom monitoring queries that read user-owned tables, or use `target_databases: '*'` against databases where `PUBLIC CONNECT` has been revoked, need explicit `GRANT` statements to `cnpg_metrics_exporter`. See ["Custom query privileges and safety"](../monitoring.md#custom-query-privileges-and-safety) and ["Manually creating the metrics exporter role"](../monitoring.md#manually-creating-the-metrics-exporter-role) in the monitoring documentation.

  For replica clusters, upgrade the source primary cluster before any replica clusters that consume from it. The `cnpg_metrics_exporter` role is created on the source primary and replicates downstream; a replica cluster upgraded first will scrape against a missing role until the source primary upgrades. The manual-recovery section linked above also covers replica clusters.

- **Schema-qualified catalog references in default monitoring queries**: hardened the shipped monitoring configuration and documentation samples by qualifying every `pg_catalog` object explicitly. Unqualified references resolve through `search_path`, which a database user can manipulate to shadow built-in objects. ([#10576](cloudnative-pg/cloudnative-pg#10576)) <!-- 1.29 1.28 1.25 -->

- **Discoverable SBOM and provenance attestations**: SBOM and SLSA provenance attached to operator container images now follow the OCI 1.1 Referrers spec, so standard registry tooling and supply-chain scanners can discover them automatically. ([#10601](cloudnative-pg/cloudnative-pg#10601)) <!-- 1.29 1.28 1.25 -->

- **CVE remediation in `github.com/jackc/pgx/v5`**: bumped to v5.9.2 to pick up upstream fixes for `CVE-2026-33816` (memory-safety in `pgproto3`) and `GHSA-j88v-2chj-qfwx` (SQL injection via simple-protocol dollar-quoted string handling). ([#10437](cloudnative-pg/cloudnative-pg#10437), [#10499](cloudnative-pg/cloudnative-pg#10499))

- **CVE remediation in the Go runtime**: built with Go 1.26.3 to pick up upstream fixes in `crypto/x509`, `crypto/tls`, `net/http`, and `net` (CVE-2026-32280, CVE-2026-32281, CVE-2026-33810, CVE-2026-33814, CVE-2026-33811, CVE-2026-39825). ([#10463](cloudnative-pg/cloudnative-pg#10463), [#10647](cloudnative-pg/cloudnative-pg#10647)) <!-- 1.29 1.28 1.25 -->

- **Build pipeline hardening**: the Go 1.26.3 bump also addresses CVE-2026-42501 (`cmd/go` module-checksum validation), reducing supply-chain exposure during release builds. The affected code paths are not reachable from the running operator. ([#10647](cloudnative-pg/cloudnative-pg#10647)) <!-- 1.29 1.28 1.25 -->

##### Changes

- Switched TLS peer verification from `VerifyPeerCertificate` to `VerifyConnection`, which runs on every completed handshake (the former is skipped on resumed TLS 1.3 sessions). Session resumption is not enabled in CloudNativePG today, so this has no observable effect, but it future-proofs verification if session caching is introduced later. ([#10478](cloudnative-pg/cloudnative-pg#10478)) <!-- 1.29 1.28 1.25 -->

##### Fixes

- Fixed a failover window where the former primary kept its primary label. If it returned during failover (for example, after a transient network partition), the `-rw` service kept routing to it, replicas could reconnect, and committed writes were lost to `pg_rewind`. The old primary is now labeled `unhealthy` to isolate it from service traffic during failover. ([#10409](cloudnative-pg/cloudnative-pg#10409)) <!-- 1.29 1.28 1.25 -->

- Fixed failover not being triggered when the node hosting the primary becomes unreachable. The operator now reads the pod's `Ready` condition (flipped to `False` by the node controller when the kubelet stops reporting) instead of `ContainersReady`, which stays stale as `True` in that scenario. Combined with the spurious-failover guard ([#10445](cloudnative-pg/cloudnative-pg#10445)), failover triggers only when Kubernetes itself marks the pod not Ready. ([#10448](cloudnative-pg/cloudnative-pg#10448)) <!-- 1.29 1.28 1.25 -->

- Fixed spurious failovers caused by transient failures on the primary's HTTP status endpoint. ([#10445](cloudnative-pg/cloudnative-pg#10445)) <!-- 1.29 1.28 1.25 -->

- Fixed escaping of backslashes and control characters in PostgreSQL configuration values. Previously, such characters in parameters like `log_line_prefix` could corrupt the configuration file or be silently stripped at runtime. ([#10515](cloudnative-pg/cloudnative-pg#10515)) <!-- 1.29 1.28 1.25 -->

- Fixed `restore_command` construction to shell-quote each argument. Values such as a `destinationPath` containing whitespace (for example, `s3://my bucket/wal`) were word-split by the POSIX shell and passed to the WAL restore tool as separate arguments. ([#10518](cloudnative-pg/cloudnative-pg#10518)) <!-- 1.29 1.28 1.25 -->

- Tightened `recoveryTarget` validation in the admission webhook: `targetXID` must now be a non-negative 32-bit integer, and `targetName` must be shorter than 64 bytes and free of ASCII control characters. Malformed values are rejected at admission instead of failing later during PostgreSQL recovery. ([#10565](cloudnative-pg/cloudnative-pg#10565)) <!-- 1.29 1.28 1.25 -->

- Fixed snapshot restores failing when leftover `pgsql_tmp*` directories were present in the data directory. ([#10447](cloudnative-pg/cloudnative-pg#10447)) <!-- 1.29 1.28 1.25 -->

- Fixed a deadlock occurring when PVC storage size and resource requests are changed simultaneously. ([#10427](cloudnative-pg/cloudnative-pg#10427)) <!-- 1.29 1.28 1.25 -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-requested ◀️ This pull request should be backported to all supported releases bug 🐛 Something isn't working lgtm This PR has been approved by a maintainer no-issue ok to merge 👌 This PR can be merged release-1.28 size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants