Repository navigation
check the base name for . and .. in Client.ReadDirContext - #703
likitha011 wants to merge 2 commits into
Conversation
| // Check the name after path.Base, so that a server cannot | ||
| // smuggle "." or ".." through as e.g. "x/..". | ||
| name := path.Base(filename) | ||
| if name == "." || name == ".." || name == "/" { | ||
| continue | ||
| } |
There was a problem hiding this comment.
https://github.com/openssh/openssh-portable/blob/master/sftp-client.c#L789-L797
/*
* Directory entries should never contain '/'
* These can be used to attack recursive ops
* (e.g. send '../../../../etc/passwd')
*/
if (*filename == '\0' ||
strpbrk(filename, SFTP_DIRECTORY_CHARS) != NULL) {
error("Server sent suspect path \"%s\" "
"during readdir of \"%s\"", filename, path);Where:
/* Directory separator characters */
#ifdef HAVE_CYGWIN
# define SFTP_DIRECTORY_CHARS "/\\"
#else /* HAVE_CYGWIN */
# define SFTP_DIRECTORY_CHARS "/"
#endif /* HAVE_CYGWIN */So, OpenSSH rejects any filename that contains a /, which as per the standards, should never happen, which is why we have never guarded against it.
It also does not appear to guard against .. and . being returned from the server.
Also, as this is a client traversing a server, the server is really only footgunning itself here by returning a “suspect path.” I suppose there is nothing wrong with defending against it, but again, a malicious server would only be hurting itself by being non-compliant here.
There was a problem hiding this comment.
Agreed the server is the one being non-compliant, but the fallout lands on the client: RemoveAll, Walk and anything joining Name() onto a local directory will act outside the directory it thinks it's listing. I hit this reading the code around the existing path.Base call, which already tolerates a / in names, so this just moves the ./.. check after it rather than adding a new rule. Happy to drop any name containing / outright like OpenSSH does instead if you'd prefer that.
| for _, e := range entries { | ||
| got = append(got, e.Name()) | ||
| } | ||
| t.Fatalf("entries = %q, want [file1]", got) |
There was a problem hiding this comment.
The want part of this test failure report fails to format the way that the test value would report.
| t.Fatalf("entries = %q, want [file1]", got) | |
| t.Fatalf(`entries = %q, want ["file1"]`, got) |
|
|
||
| entries, err := c.ReadDir("/") | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) |
There was a problem hiding this comment.
%v is the default verb. If we’re not doing any formatting, then just use the non-formatting versions:
| t.Fatalf("unexpected error: %v", err) | |
| t.Fatal("unexpected error:", err) |
| names := []string{".", "..", "x/..", "x/.", "../..", "/", "", "file1"} | ||
| body := marshalUint32(nil, id) | ||
| body = marshalUint32(body, uint32(len(names))) | ||
| for _, name := range names { | ||
| body = dirEntry(body, name) | ||
| } |
There was a problem hiding this comment.
One should generally use paragraphs to group code that has a related purpose into blocks that are easier to abstract on that a combined purpose. And we should not be interleaving two unrelated processes into one block:
| names := []string{".", "..", "x/..", "x/.", "../..", "/", "", "file1"} | |
| body := marshalUint32(nil, id) | |
| body = marshalUint32(body, uint32(len(names))) | |
| for _, name := range names { | |
| body = dirEntry(body, name) | |
| } | |
| body := marshalUint32(nil, id) | |
| names := []string{".", "..", "x/..", "x/.", "../..", "/", "", "file1"} | |
| body = marshalUint32(body, uint32(len(names))) | |
| for _, name := range names { | |
| body = dirEntry(body, name) | |
| } | |
There was a problem hiding this comment.
Done, applied all three suggestions and pushed.
Client.ReadDirContext skips "." and ".." by comparing the raw filename from the server, then applies path.Base afterwards, so a server that sends "x/..", "../.." or "x/." gets entries named ".." or "." (or "/" for a name of only slashes) into the result. Anything that builds paths from FileInfo.Name(), such as RemoveAll, Walk or a recursive download joining names onto a local directory, then walks out of the listed directory, which is the malicious-server filename problem OpenSSH's sftp client guards against. The check now runs on the path.Base result, with a regression test that fails before the change.