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

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 cloudnative-pg#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 cloudnative-pg#10515

Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
Signed-off-by: Leonardo Cecchi <leonardo.cecchi@enterprisedb.com>
Signed-off-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com>
Co-authored-by: Leonardo Cecchi <leonardo.cecchi@enterprisedb.com>
Co-authored-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com>

Author: 3 people

pkg/management/postgres/restore.go

@@ -47,6 +47,7 @@ import (
 	"github.com/cloudnative-pg/machinery/pkg/fileutils"
 	"github.com/cloudnative-pg/machinery/pkg/log"
 	"github.com/cloudnative-pg/machinery/pkg/stringset"
+	"github.com/kballard/go-shellquote"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/util/retry"
@@ -145,6 +146,10 @@ func (info InitInfo) RestoreSnapshot(ctx context.Context, cli client.Client, imm
 	}
 
 	var envs []string
+	// Operator-controlled command: LogPath and LogFileName are operator
+	// constants, so no shell-quoting layer is needed here. The config-file
+	// literal escaping is still applied for consistency with the WAL-restore
+	// path in getRestoreWalConfig.
 	restoreCmd := fmt.Sprintf(
 		"/controller/manager wal-restore --log-destination %s/%s.json %%f %%p",
 		postgresSpec.LogPath, postgresSpec.LogFileName)
@@ -641,7 +646,7 @@ func getRestoreWalConfig(ctx context.Context, backup *apiv1.Backup) (string, err
 
 	return configfile.RenderPostgresConfiguration(map[string]string{
 		"recovery_target_action": "promote",
-		"restore_command":        strings.Join(cmd, " "),
+		"restore_command":        shellquote.Join(cmd...),
 	}), nil
 }
 

pkg/management/postgres/restore_test.go

@@ -209,7 +209,7 @@ var _ = Describe("testing restore InitInfo methods", func() {
 })
 
 var _ = Describe("getRestoreWalConfig", func() {
-	It("escapes quotes and backslashes so user-controlled backup fields cannot corrupt custom.conf",
+	It("escapes quotes and backslashes across both shell and config layers",
 		func(ctx SpecContext) {
 			backup := &apiv1.Backup{
 				Status: apiv1.BackupStatus{
@@ -219,12 +219,59 @@ var _ = Describe("getRestoreWalConfig", func() {
 			}
 			out, err := getRestoreWalConfig(ctx, backup)
 			Expect(err).ToNot(HaveOccurred())
-			// Embedded single quote must be doubled; backslash must be doubled.
-			Expect(out).To(ContainSubstring("s3://bucket/has''quote"))
-			Expect(out).To(ContainSubstring(`server\\name`))
-			// And the raw unescaped form must NOT appear anywhere in the output.
+
+			// shellQuote wraps the value as has\'quote (escape).
+			// configfile.EscapePostgresConfLiteral then doubles every ' and \,
+			// yielding the exact form below at the config-file layer.
+			Expect(out).To(ContainSubstring(`s3://bucket/has\\''quote`))
+			// shellQuote quotes `\` to `\\` — config layer doubles the
+			// backslash.
+			Expect(out).To(ContainSubstring(`server\\\\name`))
+
+			// Raw unescaped forms must not appear — either would let the user
+			// break out of the config-file string literal.
 			Expect(out).NotTo(ContainSubstring("has'quote"))
 			Expect(out).NotTo(MatchRegexp(`server\\[^\\]name`),
 				"backslash in server name must be doubled")
 		})
+
+	It("shell-quotes cmd args so whitespace in user-controlled fields does not word-split",
+		func(ctx SpecContext) {
+			backup := &apiv1.Backup{
+				Status: apiv1.BackupStatus{
+					DestinationPath: "s3://my bucket/wal",
+					ServerName:      "server-a",
+				},
+			}
+			out, err := getRestoreWalConfig(ctx, backup)
+			Expect(err).ToNot(HaveOccurred())
+			// After shell-quoting, the DestinationPath is wrapped in single quotes.
+			// The outer configfile.EscapePostgresConfLiteral layer then doubles
+			// each of those single quotes, so the value appears as ''...'' in
+			// the config file.
+			Expect(out).To(ContainSubstring("''s3://my bucket/wal''"))
+
+			// PostgreSQL will replace %f %p with proper quoting when needed.
+			Expect(out).To(ContainSubstring("%f"))
+			Expect(out).To(ContainSubstring("%p"))
+		})
+
+	It("nests an obvious shell-injection payload inside both quoting layers",
+		func(ctx SpecContext) {
+			backup := &apiv1.Backup{
+				Status: apiv1.BackupStatus{
+					DestinationPath: `s3://bucket"; rm -rf /; "`,
+					ServerName:      "server-a",
+				},
+			}
+			out, err := getRestoreWalConfig(ctx, backup)
+			Expect(err).ToNot(HaveOccurred())
+
+			// shellquote.Join wraps the whole arg in single quotes (because
+			// of the embedded `;` and spaces); EscapePostgresConfLiteral
+			// then doubles each of those quotes. The payload must arrive at
+			// the shell as one argument, not as a `;`-separated command.
+			Expect(out).To(ContainSubstring(
+				`''s3://bucket"; rm -rf /; "''`))
+		})
 })