Skip to content

Commit 88d1133

Browse files
thaJeztahthaJeztah
committed
cli/connhelper: quote ssh arguments to prevent shell injection
When connecting to a remote daemon through an ssh:// connection, the CLI connects with the remote host using ssh, executing the `docker system dial-stdio` command on the remote host to connect to the daemon API's unix socket. By default, the `docker system dial-stdio` command connects with the daemon using the default location (/var/run/docker.sock), or the location as configured on the remote host. Commit 25ebf0e (included in docker CLI v24.0.0-rc.2 and higher) introduced a feature to allow the location of the socket to be specified through the host connection string, for example: DOCKER_HOST='ssh://example.test/run/custom-docker.sock' The custom path is included as part of the ssh command executed from the client machine to connect with the remote host. THe example above would execute the following command from the client machine; ssh -o ConnectTimeout=30 -T -- example.test docker --host unix:///run/custom-docker.sock system dial-stdio ssh executes remote commands in a shell environment, and no quoting was in place, which allowed for a connection string to include additional content, which would be expanded / executed on the remote machine. For example, the following example would execute `echo hello > /hello.txt` on the remote machine; export DOCKER_HOST='ssh://example.test/var/run/docker.sock $(echo hello > /hello.txt)' docker info # (output of docker info from the remote machine) While this doesn't allow the user to do anything they're not already able to do so (by directly using the same SSH connection), the behavior is not expected, so this patch adds quoting to prevent such URLs from resulting in expansion. This patch updates the cli/connhelper and cli/connhelper/ssh package to quote parameters used in the ssh command to prevent code execution and expansion of variables on the remote machine. Quoting is also applied to other parameters that are obtained from the DOCKER_HOST url, such as username and hostname. - The existing `Spec.Args()` method inthe cli/connhelper/ssh package now quotes arguments, and returns a nil slice when failing to quote. Users of this package should therefore check the returned arguments before consuming. This method did not provide an error-return, and adding one would be a breaking change. - A new `Spec.Command` method is introduced, which (unlike the `Spec.Args()` method) provides an error return. Users are recommended to use this new method instead of the `Spec.Args()` method. Some minor additional changes in behavior are included in this patch; - Connection URLs with a trailing slash (e.g. `ssh://example.test/`) would previously result in `unix:///` being used as custom socket path. After this patch, the trailing slash is ignored, and no custom socket path is used. - Specifying a remote command is now required. When passing an empty remote command, `Spec.Args()` now results in a `nil` value to be returned (or an `no remote command specified` error when using `Spec.Comnmand()`. Signed-off-by: Sebastiaan van Stijn <[email protected]>
1 parent 82eda48 commit 88d1133

File tree

3 files changed

+288
-15
lines changed

3 files changed

+288
-15
lines changed

‎cli/connhelper/connhelper.go‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -47,14 +47,19 @@ func getConnectionHelper(daemonURL string, sshFlags []string) (*ConnectionHelper
4747
}
4848
sshFlags = addSSHTimeout(sshFlags)
4949
sshFlags = disablePseudoTerminalAllocation(sshFlags)
50+
51+
remoteCommand := []string{"docker", "system", "dial-stdio"}
52+
socketPath := sp.Path
53+
if strings.Trim(sp.Path, "/") != "" {
54+
remoteCommand = []string{"docker", "--host=unix://" + socketPath, "system", "dial-stdio"}
55+
}
56+
sshArgs, err := sp.Command(sshFlags, remoteCommand...)
57+
if err != nil {
58+
return nil, err
59+
}
5060
return &ConnectionHelper{
5161
Dialer: func(ctx context.Context, network, addr string) (net.Conn, error) {
52-
args := []string{"docker"}
53-
if sp.Path != "" {
54-
args = append(args, "--host", "unix://"+sp.Path)
55-
}
56-
args = append(args, "system", "dial-stdio")
57-
return commandconn.New(ctx, "ssh", append(sshFlags, sp.Args(args...)...)...)
62+
return commandconn.New(ctx, "ssh", sshArgs...)
5863
},
5964
Host: "http://docker.example.com",
6065
}, nil

‎cli/connhelper/ssh/ssh.go‎

Lines changed: 99 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ import (
55
"errors"
66
"fmt"
77
"net/url"
8+
9+
"github.com/docker/cli/cli/connhelper/internal/syntax"
810
)
911

1012
// ParseURL creates a [Spec] from the given ssh URL. It returns an error if
@@ -76,16 +78,106 @@ type Spec struct {
7678
Path string
7779
}
7880

79-
// Args returns args except "ssh" itself combined with optional additional command args
80-
func (sp *Spec) Args(add ...string) []string {
81+
// Args returns args except "ssh" itself combined with optional additional
82+
// command and args to be executed on the remote host. It attempts to quote
83+
// the given arguments to account for ssh executing the remote command in a
84+
// shell. It returns nil when unable to quote the remote command.
85+
func (sp *Spec) Args(remoteCommandAndArgs ...string) []string {
86+
// Format the remote command to run using the ssh connection, quoting
87+
// values where needed because ssh executes these in a POSIX shell.
88+
remoteCommand, err := quoteCommand(remoteCommandAndArgs...)
89+
if err != nil {
90+
return nil
91+
}
92+
93+
sshArgs, err := sp.args()
94+
if err != nil {
95+
return nil
96+
}
97+
if remoteCommand != "" {
98+
sshArgs = append(sshArgs, remoteCommand)
99+
}
100+
return sshArgs
101+
}
102+
103+
func (sp *Spec) args(sshFlags ...string) ([]string, error) {
81104
var args []string
105+
if sp.Host == "" {
106+
return nil, errors.New("no host specified")
107+
}
82108
if sp.User != "" {
83-
args = append(args, "-l", sp.User)
109+
// Quote user, as it's obtained from the URL.
110+
usr, err := syntax.Quote(sp.User, syntax.LangPOSIX)
111+
if err != nil {
112+
return nil, fmt.Errorf("invalid user: %w", err)
113+
}
114+
args = append(args, "-l", usr)
84115
}
85116
if sp.Port != "" {
86-
args = append(args, "-p", sp.Port)
117+
// Quote port, as it's obtained from the URL.
118+
port, err := syntax.Quote(sp.Port, syntax.LangPOSIX)
119+
if err != nil {
120+
return nil, fmt.Errorf("invalid port: %w", err)
121+
}
122+
args = append(args, "-p", port)
123+
}
124+
125+
// We consider "sshFlags" to be "trusted", and set from code only,
126+
// as they are not parsed from the DOCKER_HOST URL.
127+
args = append(args, sshFlags...)
128+
129+
host, err := syntax.Quote(sp.Host, syntax.LangPOSIX)
130+
if err != nil {
131+
return nil, fmt.Errorf("invalid host: %w", err)
132+
}
133+
134+
return append(args, "--", host), nil
135+
}
136+
137+
// Command returns the ssh flags and arguments to execute a command
138+
// (remoteCommandAndArgs) on the remote host. Where needed, it quotes
139+
// values passed in remoteCommandAndArgs to account for ssh executing
140+
// the remote command in a shell. It returns an error if no remote command
141+
// is passed, or when unable to quote the remote command.
142+
//
143+
// Important: to preserve backward-compatibility, Command does not currently
144+
// perform sanitization or quoting on the sshFlags and callers are expected
145+
// to sanitize this argument.
146+
func (sp *Spec) Command(sshFlags []string, remoteCommandAndArgs ...string) ([]string, error) {
147+
if len(remoteCommandAndArgs) == 0 {
148+
return nil, errors.New("no remote command specified")
149+
}
150+
sshArgs, err := sp.args(sshFlags...)
151+
if err != nil {
152+
return nil, err
153+
}
154+
remoteCommand, err := quoteCommand(remoteCommandAndArgs...)
155+
if err != nil {
156+
return nil, err
157+
}
158+
if remoteCommand != "" {
159+
sshArgs = append(sshArgs, remoteCommand)
160+
}
161+
return sshArgs, nil
162+
}
163+
164+
// quoteCommand returns the remote command to run using the ssh connection
165+
// as a single string, quoting values where needed because ssh executes
166+
// these in a POSIX shell.
167+
func quoteCommand(commandAndArgs ...string) (string, error) {
168+
var quotedCmd string
169+
for i, arg := range commandAndArgs {
170+
a, err := syntax.Quote(arg, syntax.LangPOSIX)
171+
if err != nil {
172+
return "", fmt.Errorf("invalid argument: %w", err)
173+
}
174+
if i == 0 {
175+
quotedCmd = a
176+
continue
177+
}
178+
quotedCmd += " " + a
87179
}
88-
args = append(args, "--", sp.Host)
89-
args = append(args, add...)
90-
return args
180+
// each part is quoted appropriately, so now we'll have a full
181+
// shell command to pass off to "ssh"
182+
return quotedCmd, nil
91183
}

‎cli/connhelper/ssh/ssh_test.go‎

Lines changed: 178 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package ssh
22

33
import (
4+
"strings"
45
"testing"
56

67
"gotest.tools/v3/assert"
@@ -26,6 +27,28 @@ func TestParseURL(t *testing.T) {
2627
Host: "example.com",
2728
},
2829
},
30+
{
31+
doc: "bare ssh URL with trailing slash",
32+
url: "ssh://example.com/",
33+
expectedArgs: []string{
34+
"--", "example.com",
35+
},
36+
expectedSpec: Spec{
37+
Host: "example.com",
38+
Path: "/",
39+
},
40+
},
41+
{
42+
doc: "bare ssh URL with trailing slashes",
43+
url: "ssh://example.com//",
44+
expectedArgs: []string{
45+
"--", "example.com",
46+
},
47+
expectedSpec: Spec{
48+
Host: "example.com",
49+
Path: "//",
50+
},
51+
},
2952
{
3053
doc: "bare ssh URL and remote command",
3154
url: "ssh://example.com",
@@ -34,7 +57,7 @@ func TestParseURL(t *testing.T) {
3457
},
3558
expectedArgs: []string{
3659
"--", "example.com",
37-
"docker", "system", "dial-stdio",
60+
`docker system dial-stdio`,
3861
},
3962
expectedSpec: Spec{
4063
Host: "example.com",
@@ -48,7 +71,7 @@ func TestParseURL(t *testing.T) {
4871
},
4972
expectedArgs: []string{
5073
"--", "example.com",
51-
"docker", "--host", "unix:///var/run/docker.sock", "system", "dial-stdio",
74+
`docker --host unix:///var/run/docker.sock system dial-stdio`,
5275
},
5376
expectedSpec: Spec{
5477
Host: "example.com",
@@ -84,6 +107,25 @@ func TestParseURL(t *testing.T) {
84107
Path: "/var/run/docker.sock",
85108
},
86109
},
110+
{
111+
// This test is only to verify the behavior of ParseURL to
112+
// pass through the Path as-is. Neither Spec.Args, nor
113+
// Spec.Command use the Path field directly, and it should
114+
// likely be deprecated.
115+
doc: "bad path",
116+
url: `ssh://example.com/var/run/docker.sock '$(echo hello > /hello.txt)'`,
117+
remoteCommand: []string{
118+
"docker", "--host", `unix:///var/run/docker.sock '$(echo hello > /hello.txt)'`, "system", "dial-stdio",
119+
},
120+
expectedArgs: []string{
121+
"--", "example.com",
122+
`docker --host "unix:///var/run/docker.sock '\$(echo hello > /hello.txt)'" system dial-stdio`,
123+
},
124+
expectedSpec: Spec{
125+
Host: "example.com",
126+
Path: `/var/run/docker.sock '$(echo hello > /hello.txt)'`,
127+
},
128+
},
87129
{
88130
doc: "malformed URL",
89131
url: "malformed %%url",
@@ -123,6 +165,21 @@ func TestParseURL(t *testing.T) {
123165
url: "https://example.com",
124166
expectedError: `invalid SSH URL: incorrect scheme: https`,
125167
},
168+
{
169+
doc: "invalid URL with NUL character",
170+
url: "ssh://example.com/var/run/\x00docker.sock",
171+
expectedError: `invalid SSH URL: net/url: invalid control character in URL`,
172+
},
173+
{
174+
doc: "invalid URL with newline character",
175+
url: "ssh://example.com/var/run/docker.sock\n",
176+
expectedError: `invalid SSH URL: net/url: invalid control character in URL`,
177+
},
178+
{
179+
doc: "invalid URL with control character",
180+
url: "ssh://example.com/var/run/\x1bdocker.sock",
181+
expectedError: `invalid SSH URL: net/url: invalid control character in URL`,
182+
},
126183
}
127184
for _, tc := range testCases {
128185
t.Run(tc.doc, func(t *testing.T) {
@@ -139,3 +196,122 @@ func TestParseURL(t *testing.T) {
139196
})
140197
}
141198
}
199+
200+
func TestCommand(t *testing.T) {
201+
testCases := []struct {
202+
doc string
203+
url string
204+
sshFlags []string
205+
customCmd []string
206+
expectedCmd []string
207+
expectedError string
208+
}{
209+
{
210+
doc: "bare ssh URL",
211+
url: "ssh://example.com",
212+
expectedCmd: []string{
213+
"--", "example.com",
214+
"docker system dial-stdio",
215+
},
216+
},
217+
{
218+
doc: "bare ssh URL with trailing slash",
219+
url: "ssh://example.com/",
220+
expectedCmd: []string{
221+
"--", "example.com",
222+
"docker system dial-stdio",
223+
},
224+
},
225+
{
226+
doc: "bare ssh URL with custom ssh flags",
227+
url: "ssh://example.com",
228+
sshFlags: []string{"-T", "-o", "ConnectTimeout=30", "-oStrictHostKeyChecking=no"},
229+
expectedCmd: []string{
230+
"-T",
231+
"-o", "ConnectTimeout=30",
232+
"-oStrictHostKeyChecking=no",
233+
"--", "example.com",
234+
"docker system dial-stdio",
235+
},
236+
},
237+
{
238+
doc: "ssh URL with all options",
239+
url: "ssh://[email protected]:10022/var/run/docker.sock",
240+
sshFlags: []string{"-T", "-o ConnectTimeout=30"},
241+
expectedCmd: []string{
242+
"-l", "me",
243+
"-p", "10022",
244+
"-T",
245+
"-o ConnectTimeout=30",
246+
"--", "example.com",
247+
"docker '--host=unix:///var/run/docker.sock' system dial-stdio",
248+
},
249+
},
250+
{
251+
doc: "bad ssh flags",
252+
url: "ssh://example.com",
253+
sshFlags: []string{"-T", "-o", `ConnectTimeout=30 $(echo hi > /hi.txt)`},
254+
expectedCmd: []string{
255+
"-T",
256+
"-o", `ConnectTimeout=30 $(echo hi > /hi.txt)`,
257+
"--", "example.com",
258+
"docker system dial-stdio",
259+
},
260+
},
261+
{
262+
doc: "bad username",
263+
url: `ssh://$(shutdown)[email protected]`,
264+
expectedCmd: []string{
265+
"-l", `'$(shutdown)me'`,
266+
"--", "example.com",
267+
"docker system dial-stdio",
268+
},
269+
},
270+
{
271+
doc: "bad hostname",
272+
url: `ssh://$(shutdown)example.com`,
273+
expectedCmd: []string{
274+
"--", `'$(shutdown)example.com'`,
275+
"docker system dial-stdio",
276+
},
277+
},
278+
{
279+
doc: "bad path",
280+
url: `ssh://example.com/var/run/docker.sock '$(echo hello > /hello.txt)'`,
281+
expectedCmd: []string{
282+
"--", "example.com",
283+
`docker "--host=unix:///var/run/docker.sock '\$(echo hello > /hello.txt)'" system dial-stdio`,
284+
},
285+
},
286+
{
287+
doc: "missing command",
288+
url: "ssh://example.com",
289+
customCmd: []string{},
290+
expectedError: "no remote command specified",
291+
},
292+
}
293+
for _, tc := range testCases {
294+
t.Run(tc.doc, func(t *testing.T) {
295+
sp, err := ParseURL(tc.url)
296+
assert.NilError(t, err)
297+
298+
var commandAndArgs []string
299+
if tc.customCmd == nil {
300+
socketPath := sp.Path
301+
commandAndArgs = []string{"docker", "system", "dial-stdio"}
302+
if strings.Trim(socketPath, "/") != "" {
303+
commandAndArgs = []string{"docker", "--host=unix://" + socketPath, "system", "dial-stdio"}
304+
}
305+
}
306+
307+
actualCmd, err := sp.Command(tc.sshFlags, commandAndArgs...)
308+
if tc.expectedError == "" {
309+
assert.NilError(t, err)
310+
assert.Check(t, is.DeepEqual(actualCmd, tc.expectedCmd), "%+#v", actualCmd)
311+
} else {
312+
assert.Check(t, is.Error(err, tc.expectedError))
313+
assert.Check(t, is.Nil(actualCmd))
314+
}
315+
})
316+
}
317+
}

0 commit comments

Comments
 (0)