Testing for allowed bind paths should be more strict

[jirislav]

In the following chunk of code:

https://github.com/buildkite/sockguard/blob/1d44251770df5916f8d74c1300d15f996b1d2906/director.go#L323-L335

I suggest to use the built-in string comparison operator == instead of strings.HasPrefix(hostSrc, allowedPath), because you are currently allowing to bind paths potentially different than those allowed.

For example, imagine I allow path /etc/sh. Current implementation will allow me to mount /etc/shadow, so attacker could manipulate existing users on Docker host.

But I see one culprit in my suggestion:

• you won't be able to bind subdirectories of the allowed path, so I actually suggest to perform the check as follows:

  if allowedPath == hostSrc || strings.HasPrefix(hostSrc, allowedPath + "/") {
  return true 
  }

• this implementation will allow only the provided path (it may be a file) plus the potential subdirectories

What do you think?

[jirislav]

I've tested this fix and it works :) .. going to send PR soon.

[lox]

Closed via #45!

[jirislav]

Now, when I look at the current implementation, I think that it would be still possible to mount denied directory by path traversal.

For example, let's say that only /etc/nginx is allowed. But what if the attacker tries to attach /etc/nginx/../shadow ? By only looking at the code itself, I can imagine this mounting attempt would pass.

But note, that I haven't tested this potential vulnerability. Let's just make this as something to think about for now and to find out how could we solve this.