chore(versioner): attempt to prevent blatantly unsafe external versioner commands (fixes #10721) (#10722)

While preparing the command, attempt to verify that the template
expansion happens in a way that will result in a non-shell-injection
command. I don't presume to say that this is a 100% prevention, and the
script itself can always do dumb shit with the file path later.
Nonetheless, we should make a best-effort attempt.

Equally, this could generate false positives for commands that are
strangely written but in fact safe. I think this is acceptable; external
versioning is currently used by approximately 0.02% of users, and
presumably most of them have a setup that is sane.

---------

Signed-off-by: Jakob Borg <jakob@kastelo.net>

lib/versioner/external.go

@@ -27,6 +27,8 @@ func init() {
 	factories["external"] = newExternal
 }
 
+const unixSpecialChars = "`" + `"'<>;!#$&*? `
+
 type external struct {
 	command    string
 	filesystem fs.Filesystem
@@ -68,36 +70,11 @@ func (v external) Archive(filePath string) error {
 		return errors.New("command is empty, please enter a valid command")
 	}
 
-	words, err := shellquote.Split(v.command)
+	cmd, err := v.prepareCommand(filePath)
 	if err != nil {
-		return fmt.Errorf("command is invalid: %w", err)
+		return err
 	}
 
-	context := map[string]string{
-		"%FOLDER_FILESYSTEM%": string(v.filesystem.Type()),
-		"%FOLDER_PATH%":       v.filesystem.URI(),
-		"%FILE_PATH%":         filePath,
-	}
-
-	for i, word := range words {
-		for key, val := range context {
-			word = strings.ReplaceAll(word, key, val)
-		}
-
-		words[i] = word
-	}
-
-	cmd := exec.Command(words[0], words[1:]...)
-	env := os.Environ()
-	// filter STGUIAUTH and STGUIAPIKEY from environment variables
-	var filteredEnv []string
-
-	for _, x := range env {
-		if !strings.HasPrefix(x, "STGUIAUTH=") && !strings.HasPrefix(x, "STGUIAPIKEY=") {
-			filteredEnv = append(filteredEnv, x)
-		}
-	}
-	cmd.Env = filteredEnv
 	combinedOutput, err := cmd.CombinedOutput()
 	l.Debugln("external command output:", string(combinedOutput))
 	if err != nil {
@@ -126,3 +103,54 @@ func (external) Restore(_ string, _ time.Time) error {
 func (external) Clean(_ context.Context) error {
 	return nil
 }
+
+// prepareCommand returns the command with environment for the given file
+// path.
+func (v external) prepareCommand(filePath string) (*exec.Cmd, error) {
+	words, err := shellquote.Split(v.command)
+	if err != nil {
+		return nil, fmt.Errorf("command is invalid: %w", err)
+	}
+
+	context := map[string]string{
+		"%FOLDER_FILESYSTEM%": string(v.filesystem.Type()),
+		"%FOLDER_PATH%":       v.filesystem.URI(),
+		"%FILE_PATH%":         filePath,
+	}
+
+	for i, word := range words {
+		unsafe := strings.ContainsAny(word, unixSpecialChars)
+		for key, val := range context {
+			// If the parameter contains both an unsafe character and a
+			// template placeholder, we consider it unsafe and reject it.
+			// Note that the shell splitting will have already removed outer
+			// quotes, so that a command like `foo "%FILE_PATH%"` is fine
+			// here, despite the double quote being one of our unsafe
+			// characters.
+			if unsafe && strings.Contains(word, key) {
+				return nil, errors.New("unsafe external versioning command; see https://docs.syncthing.net/users/versioning.html#external-file-versioning")
+			}
+			word = strings.ReplaceAll(word, key, val)
+		}
+
+		words[i] = word
+	}
+
+	// filter STGUIAUTH and STGUIAPIKEY from environment variables, and add
+	// our folder info.
+	env := os.Environ()
+	var filteredEnv []string
+	for _, x := range env {
+		if !strings.HasPrefix(x, "STGUIAUTH=") && !strings.HasPrefix(x, "STGUIAPIKEY=") {
+			filteredEnv = append(filteredEnv, x)
+		}
+	}
+	for k, v := range context {
+		k = strings.Trim(k, "%")
+		filteredEnv = append(filteredEnv, fmt.Sprintf("%s=%s", k, v))
+	}
+
+	cmd := exec.Command(words[0], words[1:]...) //nolint:gosec // execution with user tainted data, by design
+	cmd.Env = filteredEnv
+	return cmd, nil
+}

lib/versioner/external_test.go

@@ -76,6 +76,39 @@ func TestExternal(t *testing.T) {
 	}
 }
 
+func TestExternalCommandSplit(t *testing.T) {
+	e := external{
+		filesystem: fs.NewFilesystem(fs.FilesystemTypeFake, "TestExternalCommandSplit"),
+	}
+
+	cases := []struct {
+		cmd  string
+		safe bool
+	}{
+		{`echo %FOLDER_PATH% %FILE_PATH%`, true},
+		{`echo "%FOLDER_PATH% %FILE_PATH%"`, false},
+		{`echo %FOLDER_PATH%/%FILE_PATH%`, true},
+		{`echo "%FOLDER_PATH%/%FILE_PATH%"`, true},
+		{`echo '%FOLDER_PATH%/%FILE_PATH%'`, true},
+		{`echo "'%FOLDER_PATH%/%FILE_PATH%'"`, false},
+		{`sh -c "echo '%FOLDER_PATH%/%FILE_PATH%'"`, false},
+		{`sh -c "echo %FOLDER_PATH%/%FILE_PATH%"`, false},
+	}
+
+	for _, tc := range cases {
+		e.command = tc.cmd
+		res, err := e.prepareCommand("evil file name")
+		if tc.safe && err != nil {
+			t.Fatal(err)
+		}
+		if !tc.safe && err == nil {
+			t.Logf("%q", res.Path)
+			t.Logf("%q", res.Args)
+			t.Errorf("should be unsafe: %q", tc.cmd)
+		}
+	}
+}
+
 func prepForRemoval(t *testing.T, file string) {
 	if err := os.RemoveAll("testdata"); err != nil {
 		t.Fatal(err)