Error message may leak credentials used for mounting about go-dockerclient

[shoeffner]

It would be possible to filter all errors/methods, but you will never know in which places users will have credentials, so that will become tricky. In this particular instance you (@fsouza) already pinned down the error handling in moby, so maybe it's good to patch it right there? They already resolve addr= options and replace them for the error messages:

https://github.com/moby/moby/blob/7c69b6dc08c7ce128c3015e08076641c2c5c40e5/volume/local/local_unix.go#L137-L143

We could adapt that to mask the password, maybe something along these lines:

From e7f5e23da0382ef7091b7473ea87d1ef5b8e91a4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sebastian=20H=C3=B6ffner?=
 <[email protected]>
Date: Thu, 12 May 2022 16:53:02 +0200
Subject: [PATCH] volume: mask password in cifs mount error messages
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Sebastian Höffner <[email protected]>
---
 volume/local/local.go      | 12 ++++++++++++
 volume/local/local_test.go | 19 +++++++++++++++++++
 volume/local/local_unix.go |  3 +++
 3 files changed, 34 insertions(+)

diff --git a/volume/local/local.go b/volume/local/local.go
index 29e3cc9a54..ec03327a4e 100644
--- a/volume/local/local.go
+++ b/volume/local/local.go
@@ -366,3 +366,15 @@ func getAddress(opts string) string {
 	}
 	return ""
 }
+
+// getPassword finds out a password from options
+func getPassword(opts string) string {
+	optsList := strings.Split(opts, ",")
+	for i := 0; i < len(optsList); i++ {
+		if strings.HasPrefix(optsList[i], "password=") {
+			passwd := strings.SplitN(optsList[i], "=", 2)[1]
+			return passwd
+		}
+	}
+	return ""
+}
diff --git a/volume/local/local_test.go b/volume/local/local_test.go
index 180ad09380..8aea918941 100644
--- a/volume/local/local_test.go
+++ b/volume/local/local_test.go
@@ -29,6 +29,25 @@ func TestGetAddress(t *testing.T) {
 
 }
 
+func TestGetPassword(t *testing.T) {
+	cases := map[string]string{
+		"password=secret":                       "secret",
+		" ":                                     "",
+		"password=":                             "",
+		"password=Tr0ub4dor&3":                  "Tr0ub4dor&3",
+		"password=correcthorsebatterystaple":    "correcthorsebatterystaple",
+		"username=moby,password=secret":         "secret",
+		"username=moby,password=secret,addr=11": "secret",
+		"username=moby,addr=11":                 "",
+	}
+	for optsstring, success := range cases {
+		v := getPassword(optsstring)
+		if v != success {
+			t.Errorf("Test case failed for %s actual: %s expected : %s", optsstring, v, success)
+		}
+	}
+}
+
 func TestRemove(t *testing.T) {
 	skip.If(t, runtime.GOOS == "windows", "FIXME: investigate why this test fails on CI")
 	rootDir, err := os.MkdirTemp("", "local-volume-test")
diff --git a/volume/local/local_unix.go b/volume/local/local_unix.go
index 4fdd182544..7cbb74dca4 100644
--- a/volume/local/local_unix.go
+++ b/volume/local/local_unix.go
@@ -141,6 +141,9 @@ func (v *localVolume) mount() error {
 			}
 			mountOpts = strings.Replace(mountOpts, "addr="+addrValue, "addr="+ipAddr.String(), 1)
 		}
+		if password := getPassword(v.opts.MountOpts); password != "" {
+			mountOpts = strings.Replace(mountOpts, "password="+password, "password=********", 1)
+		}
 	}
 	err := mount.Mount(v.opts.MountDevice, v.path, v.opts.MountType, mountOpts)
 	return errors.Wrap(err, "failed to mount local volume")
-- 
2.36.1

I will try to setup some local test to see if the error output is suppressed this way; if so, I will create an issue/PR with moby and see what they say about it.
Or do you have other ideas?

[fsouza]

Yeah confirmed it's a matter of upgrading Docker itself and there's no action needed on the client.

@hmeine thank you very much for pushing this through!

[fsouza]

Hey @hmeine, thanks for opening this issue! I believe the error comes from the Docker daemon (sorry to forward it once again), it comes as the response body for the request we send. One way we could fix this in our side is by not including the response body in the error message in some cases, but arguably we don't want to omit all responses from the Docker API?

Using a file wouldn't really work with the way Docker is designed to work: it runs as a daemon that could be in a different host (though in practice people don't run it like that 🤔)

@DerekStrickland do you have any thoughts on this?

[DerekStrickland]

@fsouza We discussed it internally and thought of using a credentials file or a credentials helper. The question then becomes does a failure to still end up including the secrets in the error message, and I suspect even then it might/will.

We also discussed parsing the error message for these well known strings and then obfuscating the message before logging. That's pretty brittle, because what other secrets might leak that we aren't handling?

If you don't have control over this error message then, probably we should look at the docker daemon code and see if we can identify all possible secrets (at this time) and handle obfuscating them, or submit a PR to the upstream. Seems like there isn't a great option presenting itself, but that's what we've thought of so far. I did a short search through the daemon code but didn't find the code in question. Have you already found that code, and if so can you share a permalink? If not, I'll keep digging.

[fsouza]

Yeah, the communication betwen go-dockerclient and the Docker daemon happens via HTTP over a tcp or unix socket (usually a unix socket). So all we have is the response body from the daemon.

Within the daemon, that error comes from here: https://github.com/moby/moby/blob/d5d5f258dfc95c46ed1e62953a754b7cf3edecd3/volume/local/local_unix.go#L145-L146, which if I'm not mistaken goes all the say to this: https://github.com/moby/sys/blob/03355939d693724ebf3cbda75aa5f1b6539f5eb1/mount/mounter_linux.go#L30

So the problem is that the secret is in the source? Like, if that mount was specified in an fstab file, it'd contain the secrets in the source too? Perhaps we could flag an issue to the Docker team to see if they have ideas on how we could signal that a field may contain a secret?

[DerekStrickland]

I was afraid the rabbit whole would keep going 😄

I supposed it can't hurt to raise an issue. @hmeine what do you think?

[hmeine]

Yes, it looks as if we're not at the bottom of the rabbit hole yet. On the other hand, as I wrote in my original report, the error might even be caused by the kernel's lack of an API that takes secrets separately from mount options.

I do agree that it makes sense to take the discussion more upstream, but the process might take a while to "converge".

I wonder if it would still be a good idea to search & replace the known password in the logs? I think that could be a much faster and fairly safe solution, depending on potential downsides, of which a few come to my mind:

• If the code taking the log would be "distant" from the mount handling code, this might require an undesirable interdependency.
• If the secret needs to be stored longer for this purpose, or handled in some less safe way, that could also be undesirable. (Maybe this could be mitigated by storing a hash and extracting the password with a regular expression instead of just search & replacing.)
• If search & replacing was somehow unsafe, e.g., if it was possible that encoding issues make the masking of the log message fail, that would also be ugly.

Having written all that, I wonder: Wouldn't it be quite easy to just clear out any password with an appropriate regular expression? After all, the mount options string is composed downstream, right? (Not sure if this is in go-dockerclient.) One could argue that the code composing the mount options is also where one should filter the reply, because that code knows that / where a password would appear, right? Wouldn't a regexp such as ,password=[^,]+ allow to do the censoring in an arguably clean way? (Yes, I am also wondering what would happen with a password containing a comma…)

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[hmeine]

May I ping you again, asking if my relatively simple idea of censoring any password at the location where the password=.. string is also composed is not good?

[fsouza]

May I ping you again, asking if my relatively simple idea of censoring any password at the location where the password=.. string is also composed is not good?

Ops completely missed that, sorry. Would we filter that in all errors/methods?

[fsouza]

I will try to setup some local test to see if the error output is suppressed this way; if so, I will create an issue/PR with moby and see what they say about it.
Or do you have other ideas?

I think this is a reasonable approach!

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[hmeine]

For public record: This issue has been fixed upstream in moby; I don't know if go-dockerclient has already benefitted from that.

[fsouza]

For public record: This issue has been fixed upstream in moby; I don't know if go-dockerclient has already benefitted from that.

Thanks for the update! I believe that fix is server side, right? So go-dockerclient should get it "for free" (users need to upgrade the docker daemon though).