From e0360ac97a13bddc9204b2cd7276b5c2563f40f3 Mon Sep 17 00:00:00 2001 From: Ev Kontsevoy Date: Wed, 7 Jun 2017 23:10:23 -0700 Subject: [PATCH] Fixed #1048 (scp error with OpenSSH client) When this command is executed: ``` $ scp host:path/with/wildcards/* . ``` Teleport would launch "SSH exec" request on the sever side, which in turn would execute the following: ``` /bin/bash -c /usr/bin/teleport scp --remote-addr=127.0.0.1:44226 --local-addr=127.0.0.1:3022 -r -f path/with/wildcards/* ``` The problem is that bash will attempt to "expand" the wildcard, sending a bunch of files as an input into -f, but `teleport scp` needs to see the _exact_ string as passed via scp client. The proposed solution is to detect shell wildcard characters and wrap them in single quotes preventing them from being expanded. Another potential solution is to NOT use shell to execute SCP commands. --- lib/auth/clt.go | 5 ++++- lib/config/configuration.go | 1 + lib/srv/exec.go | 25 ++++++++++++++++++++++--- lib/srv/exec_test.go | 13 +++++++++++++ lib/utils/websocketwriter.go | 1 - tool/teleport/common/teleport.go | 2 +- 6 files changed, 41 insertions(+), 6 deletions(-) diff --git a/lib/auth/clt.go b/lib/auth/clt.go index 032ce1326992e..e752edfff0175 100644 --- a/lib/auth/clt.go +++ b/lib/auth/clt.go @@ -81,7 +81,10 @@ func NewClient(addr string, dialer Dialer, params ...roundtrip.ClientParam) (*Cl roundtrip.HTTPClient(&http.Client{ Transport: transport, }), - roundtrip.Tracer(NewTracer), + // TODO (ekontsevoy) this tracer pollutes the logs making it harder to work + // on issues that have nothing to do with the auth API, consider activating it + // via special environment variable? + // roundtrip.Tracer(NewTracer), ) c, err := roundtrip.NewClient(addr, CurrentVersion, params...) diff --git a/lib/config/configuration.go b/lib/config/configuration.go index 0b9159d7b1856..b6dde49c76a86 100644 --- a/lib/config/configuration.go +++ b/lib/config/configuration.go @@ -210,6 +210,7 @@ func ApplyFileConfig(fc *FileConfig, cfg *service.Config) error { cfg.CachePolicy = *cachePolicy // TODO(klizhentas): Removed on sasha/ha? + // TODO(ekontsevoy): I would advise against it. syslog is the only logger which works with scp if strings.ToLower(fc.Logger.Output) == "syslog" { utils.SwitchLoggingtoSyslog() } diff --git a/lib/srv/exec.go b/lib/srv/exec.go index 6411b469b66c7..5908bdfe0b5c0 100644 --- a/lib/srv/exec.go +++ b/lib/srv/exec.go @@ -71,6 +71,25 @@ type execResponse struct { ctx *ctx } +// quoteShellWildcards takes a slice of strings and wraps some of them +// into single quotes if they contain bash wildcard expansion characters like +// '*' or '?' +func quoteShellWildcards(args []string) []string { + retval := make([]string, len(args)) + for i := range args { + a := args[i] + // check each arg for 2 things: + // - contains wildcards? + // - already wrapped in "'"? + if strings.ContainsAny(a, "*?") && !(strings.HasPrefix(a, "'") && strings.HasSuffix(a, "'")) { + retval[i] = fmt.Sprintf("'%s'", a) + } else { + retval[i] = a + } + } + return retval +} + // parseExecRequest parses SSH exec request func parseExecRequest(req *ssh.Request, ctx *ctx) (*execResponse, error) { var e execReq @@ -86,7 +105,7 @@ func parseExecRequest(req *ssh.Request, ctx *ctx) (*execResponse, error) { // is this scp request? if f == "scp" { - // for 'scp' requests, we'll fork ourselves with scp parameters: + // for 'scp' requests, we'll launch ourselves with scp parameters: teleportBin, err := osext.Executable() if err != nil { return nil, trace.Wrap(err) @@ -95,7 +114,7 @@ func parseExecRequest(req *ssh.Request, ctx *ctx) (*execResponse, error) { teleportBin, ctx.conn.RemoteAddr().String(), ctx.conn.LocalAddr().String(), - strings.Join(args[1:], " ")) + strings.Join(quoteShellWildcards(args[1:]), " ")) } } ctx.exec = &execResponse{ @@ -194,7 +213,7 @@ func prepareCommand(ctx *ctx) (*exec.Cmd, error) { } } - // execute command using user's shell like openssh does: + // by default, execute command using user's shell like openssh does: // https://github.com/openssh/openssh-portable/blob/master/session.c c := exec.Command(shell, "-c", ctx.exec.cmdName) diff --git a/lib/srv/exec_test.go b/lib/srv/exec_test.go index 112839fcb3f58..40de1aea0b433 100644 --- a/lib/srv/exec_test.go +++ b/lib/srv/exec_test.go @@ -23,6 +23,7 @@ import ( "os/user" "path" "path/filepath" + "strings" "gopkg.in/check.v1" @@ -119,6 +120,18 @@ func (s *ExecSuite) TestLoginDefsParser(c *check.C) { c.Assert(getDefaultEnvPath("bad/file"), check.Equals, "PATH="+defaultPath) } +func (s *ExecSuite) TestShellEscaping(c *check.C) { + c.Assert(strings.Join(quoteShellWildcards([]string{"one", "two"}), " "), + check.Equals, + "one two") + c.Assert(strings.Join(quoteShellWildcards([]string{"o*ne", "two?"}), " "), + check.Equals, + "'o*ne' 'two?'") + c.Assert(strings.Join(quoteShellWildcards([]string{"'o*ne'", "'two?"}), " "), + check.Equals, + "'o*ne' ''two?'") +} + // implementation of ssh.Conn interface func (s *ExecSuite) User() string { return s.usr.Username } func (s *ExecSuite) SessionID() []byte { return []byte{1, 2, 3} } diff --git a/lib/utils/websocketwriter.go b/lib/utils/websocketwriter.go index 803c8f807b35d..cc59732e0d3ca 100644 --- a/lib/utils/websocketwriter.go +++ b/lib/utils/websocketwriter.go @@ -84,7 +84,6 @@ func (w *WebSockWrapper) Write(data []byte) (n int, err error) { err = websocket.Message.Send(w.ws, utf8) } if err != nil { - log.Error(err) n = 0 } return n, err diff --git a/tool/teleport/common/teleport.go b/tool/teleport/common/teleport.go index 26a4aafb0c9ac..39369df9c6d20 100644 --- a/tool/teleport/common/teleport.go +++ b/tool/teleport/common/teleport.go @@ -50,7 +50,7 @@ func Run(cmdlineArgs []string, testRun bool) (executedCommand string, conf *serv // configure logger for a typical CLI scenario until configuration file is // parsed utils.InitLogger(utils.LoggingForDaemon, log.WarnLevel) - app := utils.InitCLIParser("teleport", "Clustered SSH service. Learn more at http://teleport.gravitational.com") + app := utils.InitCLIParser("teleport", "Clustered SSH service. Learn more at https://gravitational.com/teleport") // define global flags: var ccf config.CommandLineFlags