Completely quote AppPath and CustomConf paths (#12955)

* Completely quote AppPath and CustomConf paths

Properly handle spaces in AppPath and CustomConf within hooks and
authorized_keys. Unfortunately here we don't seem to be able to get away
with using go-shellquote as it appears that Windows doesn't play too
well with singlequote quoting - therefore we will avoid singlequote
quoting unless we absolutely cannot get away without it, e.g. \n or !.

Fix #10813

Signed-off-by: Andrew Thornton <art27@cantab.net>

* missing change

Signed-off-by: Andrew Thornton <art27@cantab.net>

* fix Test_CmdKeys

Signed-off-by: Andrew Thornton <art27@cantab.net>

integrations/cmd_keys_test.go

@@ -10,11 +10,11 @@ import (
 	"io"
 	"net/url"
 	"os"
-	"strconv"
 	"testing"
 
 	"code.gitea.io/gitea/cmd"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 
 	"github.com/urfave/cli"
 )
@@ -32,7 +32,7 @@ func Test_CmdKeys(t *testing.T) {
 			{"with_key",
 				[]string{"keys", "-e", "git", "-u", "git", "-t", "ssh-rsa", "-k", "AAAAB3NzaC1yc2EAAAADAQABAAABgQDWVj0fQ5N8wNc0LVNA41wDLYJ89ZIbejrPfg/avyj3u/ZohAKsQclxG4Ju0VirduBFF9EOiuxoiFBRr3xRpqzpsZtnMPkWVWb+akZwBFAx8p+jKdy4QXR/SZqbVobrGwip2UjSrri1CtBxpJikojRIZfCnDaMOyd9Jp6KkujvniFzUWdLmCPxUE9zhTaPu0JsEP7MW0m6yx7ZUhHyfss+NtqmFTaDO+QlMR7L2QkDliN2Jl3Xa3PhuWnKJfWhdAq1Cw4oraKUOmIgXLkuiuxVQ6mD3AiFupkmfqdHq6h+uHHmyQqv3gU+/sD8GbGAhf6ftqhTsXjnv1Aj4R8NoDf9BS6KRkzkeun5UisSzgtfQzjOMEiJtmrep2ZQrMGahrXa+q4VKr0aKJfm+KlLfwm/JztfsBcqQWNcTURiCFqz+fgZw0Ey/de0eyMzldYTdXXNRYCKjs9bvBK+6SSXRM7AhftfQ0ZuoW5+gtinPrnmoOaSCEJbAiEiTO/BzOHgowiM="},
 				false,
-				"# gitea public key\ncommand=\"" + setting.AppPath + " --config=" + strconv.Quote(strconv.Quote(setting.CustomConf))[1:len(strconv.Quote(strconv.Quote(setting.CustomConf)))-1] + " serv key-1\",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQDWVj0fQ5N8wNc0LVNA41wDLYJ89ZIbejrPfg/avyj3u/ZohAKsQclxG4Ju0VirduBFF9EOiuxoiFBRr3xRpqzpsZtnMPkWVWb+akZwBFAx8p+jKdy4QXR/SZqbVobrGwip2UjSrri1CtBxpJikojRIZfCnDaMOyd9Jp6KkujvniFzUWdLmCPxUE9zhTaPu0JsEP7MW0m6yx7ZUhHyfss+NtqmFTaDO+QlMR7L2QkDliN2Jl3Xa3PhuWnKJfWhdAq1Cw4oraKUOmIgXLkuiuxVQ6mD3AiFupkmfqdHq6h+uHHmyQqv3gU+/sD8GbGAhf6ftqhTsXjnv1Aj4R8NoDf9BS6KRkzkeun5UisSzgtfQzjOMEiJtmrep2ZQrMGahrXa+q4VKr0aKJfm+KlLfwm/JztfsBcqQWNcTURiCFqz+fgZw0Ey/de0eyMzldYTdXXNRYCKjs9bvBK+6SSXRM7AhftfQ0ZuoW5+gtinPrnmoOaSCEJbAiEiTO/BzOHgowiM= user2@localhost\n",
+				"# gitea public key\ncommand=\"" + setting.AppPath + " --config=" + util.ShellEscape(setting.CustomConf) + " serv key-1\",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQDWVj0fQ5N8wNc0LVNA41wDLYJ89ZIbejrPfg/avyj3u/ZohAKsQclxG4Ju0VirduBFF9EOiuxoiFBRr3xRpqzpsZtnMPkWVWb+akZwBFAx8p+jKdy4QXR/SZqbVobrGwip2UjSrri1CtBxpJikojRIZfCnDaMOyd9Jp6KkujvniFzUWdLmCPxUE9zhTaPu0JsEP7MW0m6yx7ZUhHyfss+NtqmFTaDO+QlMR7L2QkDliN2Jl3Xa3PhuWnKJfWhdAq1Cw4oraKUOmIgXLkuiuxVQ6mD3AiFupkmfqdHq6h+uHHmyQqv3gU+/sD8GbGAhf6ftqhTsXjnv1Aj4R8NoDf9BS6KRkzkeun5UisSzgtfQzjOMEiJtmrep2ZQrMGahrXa+q4VKr0aKJfm+KlLfwm/JztfsBcqQWNcTURiCFqz+fgZw0Ey/de0eyMzldYTdXXNRYCKjs9bvBK+6SSXRM7AhftfQ0ZuoW5+gtinPrnmoOaSCEJbAiEiTO/BzOHgowiM= user2@localhost\n",
 			},
 			{"invalid", []string{"keys", "--not-a-flag=git"}, true, "Incorrect Usage: flag provided but not defined: -not-a-flag\n\n"},
 		}

models/ssh_key.go

@@ -38,8 +38,8 @@ import (
 
 const (
 	tplCommentPrefix = `# gitea public key`
-	tplCommand       = "%s --config=%q serv key-%d"
-	tplPublicKey     = tplCommentPrefix + "\n" + `command=%q,no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty %s` + "\n"
+	tplCommand       = "%s --config=%s serv key-%d"
+	tplPublicKey     = tplCommentPrefix + "\n" + `command=%s,no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty %s` + "\n"
 )
 
 var sshOpLocker sync.Mutex
@@ -84,7 +84,7 @@ func (key *PublicKey) OmitEmail() string {
 
 // AuthorizedString returns formatted public key string for authorized_keys file.
 func (key *PublicKey) AuthorizedString() string {
-	return fmt.Sprintf(tplPublicKey, fmt.Sprintf(tplCommand, setting.AppPath, setting.CustomConf, key.ID), key.Content)
+	return fmt.Sprintf(tplPublicKey, util.ShellEscape(fmt.Sprintf(tplCommand, util.ShellEscape(setting.AppPath), util.ShellEscape(setting.CustomConf), key.ID)), key.Content)
 }
 
 func extractTypeFromBase64Key(key string) (string, error) {

modules/repository/hooks.go

@@ -28,9 +28,9 @@ func getHookTemplates() (hookNames, hookTpls, giteaHookTpls []string) {
 		fmt.Sprintf("#!/usr/bin/env %s\ndata=$(cat)\nexitcodes=\"\"\nhookname=$(basename $0)\nGIT_DIR=${GIT_DIR:-$(dirname $0)}\n\nfor hook in ${GIT_DIR}/hooks/${hookname}.d/*; do\ntest -x \"${hook}\" && test -f \"${hook}\" || continue\necho \"${data}\" | \"${hook}\"\nexitcodes=\"${exitcodes} $?\"\ndone\n\nfor i in ${exitcodes}; do\n[ ${i} -eq 0 ] || exit ${i}\ndone\n", setting.ScriptType),
 	}
 	giteaHookTpls = []string{
-		fmt.Sprintf("#!/usr/bin/env %s\n\"%s\" hook --config='%s' pre-receive\n", setting.ScriptType, setting.AppPath, setting.CustomConf),
-		fmt.Sprintf("#!/usr/bin/env %s\n\"%s\" hook --config='%s' update $1 $2 $3\n", setting.ScriptType, setting.AppPath, setting.CustomConf),
-		fmt.Sprintf("#!/usr/bin/env %s\n\"%s\" hook --config='%s' post-receive\n", setting.ScriptType, setting.AppPath, setting.CustomConf),
+		fmt.Sprintf("#!/usr/bin/env %s\n%s hook --config=%s pre-receive\n", setting.ScriptType, util.ShellEscape(setting.AppPath), util.ShellEscape(setting.CustomConf)),
+		fmt.Sprintf("#!/usr/bin/env %s\n%s hook --config=%s update $1 $2 $3\n", setting.ScriptType, util.ShellEscape(setting.AppPath), util.ShellEscape(setting.CustomConf)),
+		fmt.Sprintf("#!/usr/bin/env %s\n%s hook --config=%s post-receive\n", setting.ScriptType, util.ShellEscape(setting.AppPath), util.ShellEscape(setting.CustomConf)),
 	}
 	return
 }

modules/util/shellquote.go

@@ -0,0 +1,100 @@
+// Copyright 2020 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package util
+
+import "strings"
+
+// Bash has the definition of a metacharacter:
+// * A character that, when unquoted, separates words.
+//   A metacharacter is one of: " \t\n|&;()<>"
+//
+// The following characters also have addition special meaning when unescaped:
+// * ‘${[*?!"'`\’
+//
+// Double Quotes preserve the literal value of all characters with then quotes
+// excepting: ‘$’, ‘`’, ‘\’, and, when history expansion is enabled, ‘!’.
+// The backslash retains its special meaning only when followed by one of the
+// following characters: ‘$’, ‘`’, ‘"’, ‘\’, or newline.
+// Backslashes preceding characters without a special meaning are left
+// unmodified. A double quote may be quoted within double quotes by preceding
+// it with a backslash. If enabled, history expansion will be performed unless
+// an ‘!’ appearing in double quotes is escaped using a backslash. The
+// backslash preceding the ‘!’ is not removed.
+//
+// -> This means that `!\n` cannot be safely expressed in `"`.
+//
+// Looking at the man page for Dash and ash the situation is similar.
+//
+// Now zsh requires that ‘}’, and ‘]’ are also enclosed in doublequotes or escaped
+//
+// Single quotes escape everything except a ‘'’
+//
+// There's one other gotcha - ‘~’ at the start of a string needs to be expanded
+// because people always expect that - of course if there is a special character before '/'
+// this is not going to work
+
+const (
+	tildePrefix      = '~'
+	needsEscape      = " \t\n|&;()<>${}[]*?!\"'`\\"
+	needsSingleQuote = "!\n"
+)
+
+var doubleQuoteEscaper = strings.NewReplacer(`$`, `\$`, "`", "\\`", `"`, `\"`, `\`, `\\`)
+var singleQuoteEscaper = strings.NewReplacer(`'`, `'\''`)
+var singleQuoteCoalescer = strings.NewReplacer(`''\'`, `\'`, `\'''`, `\'`)
+
+// ShellEscape will escape the provided string.
+// We can't just use go-shellquote here because our preferences for escaping differ from those in that we want:
+//
+// * If the string doesn't require any escaping just leave it as it is.
+// * If the string requires any escaping prefer double quote escaping
+// * If we have ! or newlines then we need to use single quote escaping
+func ShellEscape(toEscape string) string {
+	if len(toEscape) == 0 {
+		return toEscape
+	}
+
+	start := 0
+
+	if toEscape[0] == tildePrefix {
+		// We're in the forcibly non-escaped section...
+		idx := strings.IndexRune(toEscape, '/')
+		if idx < 0 {
+			idx = len(toEscape)
+		} else {
+			idx++
+		}
+		if !strings.ContainsAny(toEscape[:idx], needsEscape) {
+			// We'll assume that they intend ~ expansion to occur
+			start = idx
+		}
+	}
+
+	// Now for simplicity we'll look at the rest of the string
+	if !strings.ContainsAny(toEscape[start:], needsEscape) {
+		return toEscape
+	}
+
+	// OK we have to do some escaping
+	sb := &strings.Builder{}
+	_, _ = sb.WriteString(toEscape[:start])
+
+	// Do we have any characters which absolutely need to be within single quotes - that is simply ! or \n?
+	if strings.ContainsAny(toEscape[start:], needsSingleQuote) {
+		// We need to single quote escape.
+		sb2 := &strings.Builder{}
+		_, _ = sb2.WriteRune('\'')
+		_, _ = singleQuoteEscaper.WriteString(sb2, toEscape[start:])
+		_, _ = sb2.WriteRune('\'')
+		_, _ = singleQuoteCoalescer.WriteString(sb, sb2.String())
+		return sb.String()
+	}
+
+	// OK we can just use " just escape the things that need escaping
+	_, _ = sb.WriteRune('"')
+	_, _ = doubleQuoteEscaper.WriteString(sb, toEscape[start:])
+	_, _ = sb.WriteRune('"')
+	return sb.String()
+}

modules/util/shellquote_test.go

@@ -0,0 +1,92 @@
+// Copyright 2020 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package util
+
+import "testing"
+
+func TestShellEscape(t *testing.T) {
+	tests := []struct {
+		name     string
+		toEscape string
+		want     string
+	}{
+		{
+			"Simplest case - nothing to escape",
+			"a/b/c/d",
+			"a/b/c/d",
+		}, {
+			"Prefixed tilde - with normal stuff - should not escape",
+			"~/src/go/gitea/gitea",
+			"~/src/go/gitea/gitea",
+		}, {
+			"Typical windows path with spaces - should get doublequote escaped",
+			`C:\Program Files\Gitea v1.13 - I like lots of spaces\gitea`,
+			`"C:\\Program Files\\Gitea v1.13 - I like lots of spaces\\gitea"`,
+		}, {
+			"Forward-slashed windows path with spaces - should get doublequote escaped",
+			"C:/Program Files/Gitea v1.13 - I like lots of spaces/gitea",
+			`"C:/Program Files/Gitea v1.13 - I like lots of spaces/gitea"`,
+		}, {
+			"Prefixed tilde - but then a space filled path",
+			"~git/Gitea v1.13/gitea",
+			`~git/"Gitea v1.13/gitea"`,
+		}, {
+			"Bangs are unforutunately not predictable so need to be singlequoted",
+			"C:/Program Files/Gitea!/gitea",
+			`'C:/Program Files/Gitea!/gitea'`,
+		}, {
+			"Newlines are just irritating",
+			"/home/git/Gitea\n\nWHY-WOULD-YOU-DO-THIS\n\nGitea/gitea",
+			"'/home/git/Gitea\n\nWHY-WOULD-YOU-DO-THIS\n\nGitea/gitea'",
+		}, {
+			"Similarly we should nicely handle mutiple single quotes if we have to single-quote",
+			"'!''!'''!''!'!'",
+			`\''!'\'\''!'\'\'\''!'\'\''!'\''!'\'`,
+		}, {
+			"Double quote < ...",
+			"~/<gitea",
+			"~/\"<gitea\"",
+		}, {
+			"Double quote > ...",
+			"~/gitea>",
+			"~/\"gitea>\"",
+		}, {
+			"Double quote and escape $ ...",
+			"~/$gitea",
+			"~/\"\\$gitea\"",
+		}, {
+			"Double quote {...",
+			"~/{gitea",
+			"~/\"{gitea\"",
+		}, {
+			"Double quote }...",
+			"~/gitea}",
+			"~/\"gitea}\"",
+		}, {
+			"Double quote ()...",
+			"~/(gitea)",
+			"~/\"(gitea)\"",
+		}, {
+			"Double quote and escape `...",
+			"~/gitea`",
+			"~/\"gitea\\`\"",
+		}, {
+			"Double quotes can handle a number of things without having to escape them but not everything ...",
+			"~/<gitea> ${gitea} `gitea` [gitea] (gitea) \"gitea\" \\gitea\\ 'gitea'",
+			"~/\"<gitea> \\${gitea} \\`gitea\\` [gitea] (gitea) \\\"gitea\\\" \\\\gitea\\\\ 'gitea'\"",
+		}, {
+			"Single quotes don't need to escape except for '...",
+			"~/<gitea> ${gitea} `gitea` (gitea) !gitea! \"gitea\" \\gitea\\ 'gitea'",
+			"~/'<gitea> ${gitea} `gitea` (gitea) !gitea! \"gitea\" \\gitea\\ '\\''gitea'\\'",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := ShellEscape(tt.toEscape); got != tt.want {
+				t.Errorf("ShellEscape(%q):\nGot:    %s\nWanted: %s", tt.toEscape, got, tt.want)
+			}
+		})
+	}
+}