From fc3aefb160527aa3960529f01db1593c096f5bd6 Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Wed, 20 Mar 2024 17:28:05 -0600 Subject: [PATCH 1/3] Quote user supplied inputs provided to scripts to avoid RCE This change introduces the func `utils.UnixShellQuote` which will quote any inputs which could potentially allow execution or script escape. This is utilized to ensure that scripts produced from a potential Phishing link could not contain code execution which may expose a user. --- lib/utils/cli.go | 19 +++++ lib/utils/cli_test.go | 137 ++++++++++++++++++++++++++++++++ lib/web/apiserver.go | 6 +- lib/web/integrations_awsoidc.go | 31 ++++---- lib/web/join_tokens.go | 24 +++--- 5 files changed, 187 insertions(+), 30 deletions(-) diff --git a/lib/utils/cli.go b/lib/utils/cli.go index e79c0bc2aa8f0..f75f2de4c0f3a 100644 --- a/lib/utils/cli.go +++ b/lib/utils/cli.go @@ -29,6 +29,7 @@ import ( stdlog "log" "log/slog" "os" + "regexp" "runtime" "strconv" "strings" @@ -430,6 +431,24 @@ func SplitIdentifiers(s string) []string { }) } +var unixShellQuoteCharacters = regexp.MustCompile( + "[^" + // Match any character that is NOT one of the following: + "\\w" + // Word characters (letter, number, underscore) + "@%+=:,./-" + // Safe symbols that don't typically have a special meaning in shells + "]") + +// UnixShellQuote returns the string in quotes if quoting is necessary to prevent possible execution or injection for +// UNIX-like systems. This is intended to be used when building shell scripts for Linux or macOS. +func UnixShellQuote(s string) string { + if unixShellQuoteCharacters.MatchString(s) { + s = strings.ReplaceAll(s, "\n", "\\n") + s = strings.ReplaceAll(s, "\r", "\\r") + return "'" + strings.ReplaceAll(s, "'", "'\"'\"'") + "'" + } + + return s +} + // EscapeControl escapes all ANSI escape sequences from string and returns a // string that is safe to print on the CLI. This is to ensure that malicious // servers can not hide output. For more details, see: diff --git a/lib/utils/cli_test.go b/lib/utils/cli_test.go index dcfccdeaab9cf..0a0fe236988eb 100644 --- a/lib/utils/cli_test.go +++ b/lib/utils/cli_test.go @@ -68,6 +68,143 @@ func TestUserMessageFromError(t *testing.T) { } } +func TestUnixShellQuote(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + in string + out string + }{ + { + name: "emptyString", + in: "", + out: "", + }, + { + name: "noQuote", + in: "foo", + out: "foo", + }, + { + name: "bang", + in: "foo!", + out: "'foo!'", + }, + { + name: "variable", + in: "foo$BAR", + out: "'foo$BAR'", + }, + { + name: "semicolon", + in: "foo;bar", + out: "'foo;bar'", + }, + { + name: "singleQuoteStart", + in: "'foo", + out: "''\"'\"'foo'", + }, + { + name: "singleQuoteMid", + in: "foo'bar", + out: "'foo'\"'\"'bar'", + }, + { + name: "singleQuoteEnd", + in: "foo'", + out: "'foo'\"'\"''", + }, + { + name: "singleQuotesSurrounding", + in: "'foo'", + out: "''\"'\"'foo'\"'\"''", + }, + { + name: "space", + in: "foo bar", + out: "'foo bar'", + }, + { + name: "path", + in: "/usr/local/bin", + out: "/usr/local/bin", + }, + { + name: "commandSubstitution", + in: "$(ls -la)", + out: "'$(ls -la)'", + }, + { + name: "backticks", + in: "`echo foo`", + out: "'`echo foo`'", + }, + { + name: "doubleQuotes", + in: "foo\"bar", + out: "'foo\"bar'", + }, + { + name: "brackets", + in: "[1,2,3]", + out: "'[1,2,3]'", + }, + { + name: "parentheses", + in: "(1+2)", + out: "'(1+2)'", + }, + { + name: "braceExpansion", + in: "{a,b}", + out: "'{a,b}'", + }, + { + name: "escapeCharacters", + in: "foo\\bar", + out: "'foo\\bar'", + }, + { + name: "wildcards", + in: "*", + out: "'*'", + }, + { + name: "pipe", + in: "foo | bar", + out: "'foo | bar'", + }, + { + name: "andOperator", + in: "foo && bar", + out: "'foo && bar'", + }, + { + name: "newline", + in: "foo\nbar", + out: "'foo\\nbar'", + }, + { + name: "carriageReturn", + in: "foo\rbar", + out: "'foo\\rbar'", + }, + { + name: "tab", + in: "foo\tbar", + out: "'foo\tbar'", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require.Equal(t, tt.out, UnixShellQuote(tt.in)) + }) + } +} + // TestEscapeControl tests escape control func TestEscapeControl(t *testing.T) { t.Parallel() diff --git a/lib/web/apiserver.go b/lib/web/apiserver.go index 8483b4519c0a0..13f52792373ea 100644 --- a/lib/web/apiserver.go +++ b/lib/web/apiserver.go @@ -1931,11 +1931,11 @@ func (h *Handler) installer(w http.ResponseWriter, r *http.Request, p httprouter tmpl := installers.Template{ PublicProxyAddr: h.PublicProxyAddr(), - MajorVersion: version, + MajorVersion: utils.UnixShellQuote(version), TeleportPackage: teleportPackage, - RepoChannel: repoChannel, + RepoChannel: utils.UnixShellQuote(repoChannel), AutomaticUpgrades: strconv.FormatBool(installUpdater), - AzureClientID: azureClientID, + AzureClientID: utils.UnixShellQuote(azureClientID), } err = instTmpl.Execute(w, tmpl) return nil, trace.Wrap(err) diff --git a/lib/web/integrations_awsoidc.go b/lib/web/integrations_awsoidc.go index afbd97d352b87..a97502a3ed465 100644 --- a/lib/web/integrations_awsoidc.go +++ b/lib/web/integrations_awsoidc.go @@ -45,6 +45,7 @@ import ( "github.com/gravitational/teleport/lib/integrations/awsoidc/deployserviceconfig" "github.com/gravitational/teleport/lib/reversetunnelclient" "github.com/gravitational/teleport/lib/services" + libutils "github.com/gravitational/teleport/lib/utils" "github.com/gravitational/teleport/lib/web/scripts/oneoff" "github.com/gravitational/teleport/lib/web/ui" ) @@ -266,11 +267,11 @@ func (h *Handler) awsOIDCConfigureDeployServiceIAM(w http.ResponseWriter, r *htt // teleport integration configure deployservice-iam argsList := []string{ "integration", "configure", "deployservice-iam", - fmt.Sprintf("--cluster=%s", clusterName), - fmt.Sprintf("--name=%s", integrationName), - fmt.Sprintf("--aws-region=%s", awsRegion), - fmt.Sprintf("--role=%s", role), - fmt.Sprintf("--task-role=%s", taskRole), + fmt.Sprintf("--cluster=%s", libutils.UnixShellQuote(clusterName)), + fmt.Sprintf("--name=%s", libutils.UnixShellQuote(integrationName)), + fmt.Sprintf("--aws-region=%s", libutils.UnixShellQuote(awsRegion)), + fmt.Sprintf("--role=%s", libutils.UnixShellQuote(role)), + fmt.Sprintf("--task-role=%s", libutils.UnixShellQuote(taskRole)), } script, err := oneoff.BuildScript(oneoff.OneOffScriptParams{ TeleportArgs: strings.Join(argsList, " "), @@ -305,8 +306,8 @@ func (h *Handler) awsOIDCConfigureEICEIAM(w http.ResponseWriter, r *http.Request // teleport integration configure eice-iam argsList := []string{ "integration", "configure", "eice-iam", - fmt.Sprintf("--aws-region=%s", awsRegion), - fmt.Sprintf("--role=%s", role), + fmt.Sprintf("--aws-region=%s", libutils.UnixShellQuote(awsRegion)), + fmt.Sprintf("--role=%s", libutils.UnixShellQuote(role)), } script, err := oneoff.BuildScript(oneoff.OneOffScriptParams{ TeleportArgs: strings.Join(argsList, " "), @@ -340,8 +341,8 @@ func (h *Handler) awsOIDCConfigureEKSIAM(w http.ResponseWriter, r *http.Request, // "teleport integration configure eks-iam" argsList := []string{ "integration", "configure", "eks-iam", - fmt.Sprintf("--aws-region=%s", awsRegion), - fmt.Sprintf("--role=%s", role), + fmt.Sprintf("--aws-region=%s", libutils.UnixShellQuote(awsRegion)), + fmt.Sprintf("--role=%s", libutils.UnixShellQuote(role)), } script, err := oneoff.BuildScript(oneoff.OneOffScriptParams{ TeleportArgs: strings.Join(argsList, " "), @@ -885,10 +886,10 @@ func (h *Handler) awsOIDCConfigureIdP(w http.ResponseWriter, r *http.Request, p // teleport integration configure awsoidc-idp argsList := []string{ "integration", "configure", "awsoidc-idp", - fmt.Sprintf("--cluster=%s", clusterName), - fmt.Sprintf("--name=%s", integrationName), - fmt.Sprintf("--role=%s", role), - fmt.Sprintf("--s3-bucket-uri=%s", s3URI.String()), + fmt.Sprintf("--cluster=%s", libutils.UnixShellQuote(clusterName)), + fmt.Sprintf("--name=%s", libutils.UnixShellQuote(integrationName)), + fmt.Sprintf("--role=%s", libutils.UnixShellQuote(role)), + fmt.Sprintf("--s3-bucket-uri=%s", libutils.UnixShellQuote(s3URI.String())), fmt.Sprintf("--s3-jwks-base64=%s", base64.StdEncoding.EncodeToString(jwksJSON)), } script, err := oneoff.BuildScript(oneoff.OneOffScriptParams{ @@ -923,8 +924,8 @@ func (h *Handler) awsOIDCConfigureListDatabasesIAM(w http.ResponseWriter, r *htt // teleport integration configure listdatabases-iam argsList := []string{ "integration", "configure", "listdatabases-iam", - fmt.Sprintf("--aws-region=%s", awsRegion), - fmt.Sprintf("--role=%s", role), + fmt.Sprintf("--aws-region=%s", libutils.UnixShellQuote(awsRegion)), + fmt.Sprintf("--role=%s", libutils.UnixShellQuote(role)), } script, err := oneoff.BuildScript(oneoff.OneOffScriptParams{ TeleportArgs: strings.Join(argsList, " "), diff --git a/lib/web/join_tokens.go b/lib/web/join_tokens.go index 86f2c73d82b60..146830ed7df6c 100644 --- a/lib/web/join_tokens.go +++ b/lib/web/join_tokens.go @@ -237,11 +237,11 @@ func (h *Handler) getNodeJoinScriptHandle(w http.ResponseWriter, r *http.Request } settings := scriptSettings{ - token: params.ByName("token"), + token: utils.UnixShellQuote(params.ByName("token")), appInstallMode: false, - joinMethod: r.URL.Query().Get("method"), + joinMethod: utils.UnixShellQuote(r.URL.Query().Get("method")), installUpdater: autoUpgrades, - automaticUpgradesVersion: autoUpgradesVersion, + automaticUpgradesVersion: utils.UnixShellQuote(autoUpgradesVersion), } script, err := getJoinScript(r.Context(), settings, h.GetProxyClient()) @@ -285,12 +285,12 @@ func (h *Handler) getAppJoinScriptHandle(w http.ResponseWriter, r *http.Request, } settings := scriptSettings{ - token: params.ByName("token"), + token: utils.UnixShellQuote(params.ByName("token")), appInstallMode: true, - appName: name, - appURI: uri, + appName: utils.UnixShellQuote(name), + appURI: utils.UnixShellQuote(uri), installUpdater: autoUpgrades, - automaticUpgradesVersion: autoUpgradesVersion, + automaticUpgradesVersion: utils.UnixShellQuote(autoUpgradesVersion), } script, err := getJoinScript(r.Context(), settings, h.GetProxyClient()) @@ -319,10 +319,10 @@ func (h *Handler) getDatabaseJoinScriptHandle(w http.ResponseWriter, r *http.Req } settings := scriptSettings{ - token: params.ByName("token"), + token: utils.UnixShellQuote(params.ByName("token")), databaseInstallMode: true, installUpdater: autoUpgrades, - automaticUpgradesVersion: autoUpgradesVersion, + automaticUpgradesVersion: utils.UnixShellQuote(autoUpgradesVersion), } script, err := getJoinScript(r.Context(), settings, h.GetProxyClient()) @@ -365,11 +365,11 @@ func (h *Handler) getDiscoveryJoinScriptHandle(w http.ResponseWriter, r *http.Re } settings := scriptSettings{ - token: params.ByName("token"), + token: utils.UnixShellQuote(params.ByName("token")), discoveryInstallMode: true, - discoveryGroup: discoveryGroup, + discoveryGroup: utils.UnixShellQuote(discoveryGroup), installUpdater: autoUpgrades, - automaticUpgradesVersion: autoUpgradesVersion, + automaticUpgradesVersion: utils.UnixShellQuote(autoUpgradesVersion), } script, err := getJoinScript(r.Context(), settings, h.GetProxyClient()) From 89f5d80eaf6ff62e22ad84794f95ab4cd24b00be Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Thu, 21 Mar 2024 10:57:02 -0600 Subject: [PATCH 2/3] awsAccessGraphOIDCSync: Ensure role parameter is quoted correctly --- lib/web/integrations_awsoidc.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/web/integrations_awsoidc.go b/lib/web/integrations_awsoidc.go index a97502a3ed465..4ebcbdd78bf28 100644 --- a/lib/web/integrations_awsoidc.go +++ b/lib/web/integrations_awsoidc.go @@ -965,7 +965,7 @@ func (h *Handler) awsAccessGraphOIDCSync(w http.ResponseWriter, r *http.Request, // "teleport integration configure access-graph aws-iam" argsList := []string{ "integration", "configure", "access-graph", "aws-iam", - fmt.Sprintf("--role=%s", role), + fmt.Sprintf("--role=%s", libutils.UnixShellQuote(role)), } script, err := oneoff.BuildScript(oneoff.OneOffScriptParams{ TeleportArgs: strings.Join(argsList, " "), From 6b49943dae8eb556588f897741a5696fa24f637c Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Mon, 25 Mar 2024 17:05:40 -0600 Subject: [PATCH 3/3] join_tokens: Move shell quote to `getJoinScript` rather than where parameters are extracted This will increase safety moving forward, but it requires a more conservative quoting strategy. --- lib/web/join_tokens.go | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/lib/web/join_tokens.go b/lib/web/join_tokens.go index 146830ed7df6c..331b7cd1a709f 100644 --- a/lib/web/join_tokens.go +++ b/lib/web/join_tokens.go @@ -237,11 +237,11 @@ func (h *Handler) getNodeJoinScriptHandle(w http.ResponseWriter, r *http.Request } settings := scriptSettings{ - token: utils.UnixShellQuote(params.ByName("token")), + token: params.ByName("token"), appInstallMode: false, - joinMethod: utils.UnixShellQuote(r.URL.Query().Get("method")), + joinMethod: r.URL.Query().Get("method"), installUpdater: autoUpgrades, - automaticUpgradesVersion: utils.UnixShellQuote(autoUpgradesVersion), + automaticUpgradesVersion: autoUpgradesVersion, } script, err := getJoinScript(r.Context(), settings, h.GetProxyClient()) @@ -285,12 +285,12 @@ func (h *Handler) getAppJoinScriptHandle(w http.ResponseWriter, r *http.Request, } settings := scriptSettings{ - token: utils.UnixShellQuote(params.ByName("token")), + token: params.ByName("token"), appInstallMode: true, - appName: utils.UnixShellQuote(name), - appURI: utils.UnixShellQuote(uri), + appName: name, + appURI: uri, installUpdater: autoUpgrades, - automaticUpgradesVersion: utils.UnixShellQuote(autoUpgradesVersion), + automaticUpgradesVersion: autoUpgradesVersion, } script, err := getJoinScript(r.Context(), settings, h.GetProxyClient()) @@ -319,10 +319,10 @@ func (h *Handler) getDatabaseJoinScriptHandle(w http.ResponseWriter, r *http.Req } settings := scriptSettings{ - token: utils.UnixShellQuote(params.ByName("token")), + token: params.ByName("token"), databaseInstallMode: true, installUpdater: autoUpgrades, - automaticUpgradesVersion: utils.UnixShellQuote(autoUpgradesVersion), + automaticUpgradesVersion: autoUpgradesVersion, } script, err := getJoinScript(r.Context(), settings, h.GetProxyClient()) @@ -365,11 +365,11 @@ func (h *Handler) getDiscoveryJoinScriptHandle(w http.ResponseWriter, r *http.Re } settings := scriptSettings{ - token: utils.UnixShellQuote(params.ByName("token")), + token: params.ByName("token"), discoveryInstallMode: true, - discoveryGroup: utils.UnixShellQuote(discoveryGroup), + discoveryGroup: discoveryGroup, installUpdater: autoUpgrades, - automaticUpgradesVersion: utils.UnixShellQuote(autoUpgradesVersion), + automaticUpgradesVersion: autoUpgradesVersion, } script, err := getJoinScript(r.Context(), settings, h.GetProxyClient()) @@ -510,16 +510,16 @@ func getJoinScript(ctx context.Context, settings scriptSettings, m nodeAPIGetter "packageName": packageName, "repoChannel": repoChannel, "installUpdater": strconv.FormatBool(settings.installUpdater), - "version": version, + "version": utils.UnixShellQuote(version), "appInstallMode": strconv.FormatBool(settings.appInstallMode), - "appName": settings.appName, - "appURI": settings.appURI, - "joinMethod": settings.joinMethod, + "appName": utils.UnixShellQuote(settings.appName), + "appURI": utils.UnixShellQuote(settings.appURI), + "joinMethod": utils.UnixShellQuote(settings.joinMethod), "labels": strings.Join(labelsList, ","), "databaseInstallMode": strconv.FormatBool(settings.databaseInstallMode), "db_service_resource_labels": dbServiceResourceLabels, "discoveryInstallMode": settings.discoveryInstallMode, - "discoveryGroup": settings.discoveryGroup, + "discoveryGroup": utils.UnixShellQuote(settings.discoveryGroup), }) if err != nil { return "", trace.Wrap(err)