A CI failure:

container_inspect_test.go:34:
    expected a containerNotFound error, got Error response from daemon: Server error

//cc @cpuguy83

working on it , 15min and will run a new test

@thaJeztah the actual code change is a single line m.SkipClean(true)

@vdemeester , @thaJeztah - now it is split in 2 commits.

I think current IT and unit tests are ok as they already check the reply error and types.

ping @docker/security does this have any security implications?

Quite a while back I investigated Go's http routing and unless something has changed, Go will have already cleaned the URL before it ever reaches gorilla.

https://golang.org/pkg/net/http/#ServeMux

ServeMux also takes care of sanitizing the URL request path, redirecting any request containing . or .. elements or repeated slashes to an equivalent, cleaner URL.

It looks like this may indeed be problematic as gorilla looks to replace ServeMux entirely. I'm guessing a path traversal could result in serving data the requester should not have access to.

On a quick glance I don't see other places in the net/http package that clean the url outside of ServeMux and Redirect.

@endophage
the problem was that when you accept the container name in the endpoint like
/object/containerName/list

and containerName is ../
/object/..//list

gorilla tries to serve
/list and returns page not found

I looked through the code and the only way I found to solve it is to either encode and decode the container name for all endpoints or disable this redirect in the gorilla mux

I don't think there is any way to run container name check before the gorilla mux - page not found

I would be interested to hear more ideas.

@krasi-georgiev Is the issue that in the CLI, you need to inform the user that the container name is invalid?

Because if so you might be able to run filepath.Clean on the container name, and compare the result to the uncleaned name. The name would be invalid if they're not equal.

@cyli no the issue is that any api endpoint that takes a container name in the url gets redirected when you use ../ (for the container name)

and I don't think there is any way to run container name check before the gorilla mux returns page not found.

@cyli and I are coming at this from the security point of view and with that hat on, somebody entering .. in a URL should be assumed to be attempting some form of path traversal attack. We cannot condone enabling SkipClean for the global router as this PR does, and would still be very suspicious of enabling it for specific sub routers.

To make this remotely safe it would require auditing all code paths through which the name passes and ensuring it is never used as a file path without being cleaned, and even then it would only ensure it is safe at the point in time of the audit.

@endophage hm, yes, you are right. Should we have the API return a different Header (40x) instead of a 30x redirect? That would at least solve the "endpoint traversal"

@thaJeztah In a technical sense, normalizing the path and returning a 301 is the correct response. It's a permanent redirect to the resource the user actually asked for and a well design client is, on paper at least, meant to cache the 301 for any future requests to the incorrect path.

@endophage could you give me an example for a traversal attack as I can't see how this can be abused.

SkipClean(true) seems to disable any kind of redirects .. , ../ so not sure how disabling will open security issues.

@krasi-georgiev I can't give you a specific attack in Docker, one may not exist, but as I said, one would have to audit every single code path that uses the passed argument and I don't have the time to do that right now.

As a generic example though, imagine you have the username joe and resources in a system are labelled <username>/<resource name>. The code has a shortcut that says "if the first segment of the resource name matches the username of the requesting user, it's the owner so approve all operations on the resource". Now imagine joe sends a DELETE method request to joe/../bob/private. The framework would approve the request, but assuming it's backed by underlying files on disk, the system call to delete joe/../bob/private would translate the path to bob/private and joe would have deleted something he shouldn't have any permissions on.

If on the other hand the router implements path normalization as it does today, the actual business logic would never see joe/../bob/private because the routing would have turned it in to bob/private before it ever touches our authorization checks, and in that case, the request would not be authorized.

With that example in mind, every code path that would touch the de-normalized name would need to be audited to ensure these types of compromises do not exist.

I just tried few api requests with skipClean and I think it doesn't work as your example.

docker container inspect ../images

Calling GET /v1.27/containers/../images/json
Handler for GET /v1.27/containers/../images/json returned error: No such container: ../images

the way you describe is this should translate to /v1.27/images/json , bit it doesn't

sorry if I didn't understand your point correctly.

@krasi-georgiev The behaviour you've just described is exactly what would be expected with skipClean=true. Try it with skipClean=false (the default).

client
docker container inspect ../images

server
Calling GET /v1.27/images/json

client
json: cannot unmarshal array into Go value of type types.ContainerJSON

it takes ../ and redirect it one level up and runs the resulting command although it was passed as an argument.

isn't this what we want to avoid ?

I think this is the same result before updating gorilla

Exactly.

@endophage
I don't understand the point here, but haven't looked through the rest of the server code to give opinions

now the real bug I am trying to solve - this code in the client

resp, err := cli.get(ctx, "/containers/"+container+"/checkpoints", query, nil)
if err != nil {
if resp.statusCode == http.StatusNotFound {

https://github.com/docker/docker/blob/master/client/checkpoint_list.go#L23-L24

when you run
docker checkpoint ls ../
it translates to
/containers/..//checkpoints
and then
/checkpoints
and the server returns 404 url not found

now the client reads the 404 and assumes container not found , but the 404 is for page not found

is this the expected behaviour or the code needs fixing in the client ?

similar code is used for many other client commands that use container name in the api url
and some even just return the error from the server - Page not found - which is a bit confusing

Why are you using ../ as container name?

The correct solution here is probably to disallow names that involve path traversals so that you can never create a container with that name in the first place.

@endophage we could (and should) validate that on the cli side, that's true. The question is server/router side, should an http request on /v1.27/containers/../images/json return an error (invalid name or anything) or should it call the /v1.27/images/json ?

@vdemeester never trust a client to do the right thing (because not everyone will be using your client), the server requires the same validation. This PR currently doesn't move to return a different status, it moves to make the server accept /v1.27/containers/../images/json and pass through a container name that includes ../.

I don't have a strong opinion on the status and the exact response in this case is not security relevant as far as I can tell, so if consensus favours a 4XX I'm not going to argue against it.

@vdemeester do you know who could make a good decision on this ?

Apologies, just reiterating what I think @endophage is saying because I may have misinterpreted and I wanted to make sure we're all talking about the same thing.

The server accepting /v1.27/containers/../images/json and passing through a container named ../ means that every route that is parameterized must be checked for path traversal issues.

The container name is unvalidated user input that must be validated. Part of that validation includes path cleaning. I think we do do this for container names, but having the router automatically do this for you removes one possible bug in the input validation.

But this change does not just affect container names - it affects every value parameterized in a mux route. So it'd affect images, networks, volumes, swarm nodes, etc. Everywhere a resource ID or name is accepted, that value would have be cleaned the same way to make sure there's no path traversal issues.

We probably already do this correctly in most places, but because it's potentially a dangerous bug we'd have to go and audit every single place where this occurs, and ideally write tests for every single resource to ensure that no path traversals can occur, which is a lot of work. Not to mention that someone who adds a new set of routers would have to be very careful to verify the same thing.

So I don't think that we should add m.SkipClean(true), and propagate any ../ parameters to the individual resource handlers.

That said, there are other options. Client validation is one option. Possibly you could write a handler function that wraps the real server handler, and checks the entire path for any traversals at all and return a 4XX before passing the request onto the real server handler.

@cyli docker is not the only client using the api so the check should be on the server, I also prefer server checks for the parameters , but the current code implementations hands over the url directly to gorilla mux and any ../ in the url causes redirects before you can validate the container name or any other url parameter.

but the current code implementations hands over the url directly to gorilla mux and any ../ in the url causes redirects before you can validate the container name or any other url parameter.

@krasi-georgiev Would it be acceptable to intercept the requests before the server hands everything over to the mux, and just reject with a 4XX if any ../ is in the path? This doesn't l let you specify if the container name specifically is invalid, but I'd consider any ../ an invalid URL.

@cyli this is very close to the current behaviour - ../ redirects to a not existing path and returns page not found 404

/v1.27/containers/..//checkpoints => /v1.27/checkpoints => 404 page not found

the only other possible solution would be to enable UseEncodedPath
http://www.gorillatoolkit.org/pkg/mux#Router.UseEncodedPath

and change the client to encode the container names

this might be logical right? any variable in a url should be encoded right ?

this shouldn't affect the api at all.

@krasi-georgiev Ah apologies, I was thinking 403 instead of 404

@cyli wouldn't that be a confusing api reply ?
/v1.27/containers/..//checkpoints =>403 forbidden , why is this forbidden ?
I might be overthinking it.

gorilla/mux#226
I opened an issue in the gorilla repo to add the feature to run some the option to run a validation command for the variable before cleaning up the url and redirecting the ../

/v1.27/containers/..//checkpoints =>403 forbidden , why is this forbidden ?

My thought was that it's an invalid URL due to path traversal.

Also, please correct me if I've misunderstood the go 1.8 change - the change was in the behavior of the client, right? And the server is correctly still re-directing if it's a path traversal, it's just that the client redirection behavior is different?

This is, again, a suggestion for a fix to the client, but the check seemed to be on the client previously as well, if I understand how it worked previously (checking for 301s). Can we set a CheckRedirect function on the client so that it's not automatically followed?

@cyli this is interesting I didn't know that. I will let the maintainers reply on this one.

ping @thaJeztah

@krasi-georgiev Alternately, rather than 403 how about 400?

this is interesting I didn't know that. I will let the maintainers reply on this one.

I don't know either - I was asking because that's how I interpreted #29602 and golang/go#18347 (comment).

cc @cpuguy83 - would not automatically following the redirect on 301s return the client to previous behavior?

@cpuguy83, @vdemeester sorry for the reminder , but I would really appreciate some more ideas how to fix this.

We might be able to update the client to not follow redirects implicitly like this. The issue with go 1.8 I think is only client-side behavior.