Skip to content

join: return an error if root is unclean path#43

Merged
cyphar merged 1 commit intomainfrom
unclean-root
Jan 10, 2025
Merged

join: return an error if root is unclean path#43
cyphar merged 1 commit intomainfrom
unclean-root

Conversation

@cyphar
Copy link
Copy Markdown
Owner

@cyphar cyphar commented Jan 9, 2025

If a user provides an unclean root path, we will implicitly clean it at
the end of SecureJoin (which may result in a path that doesn't exist or
has "escaped" the root). Such usage is fundamentally unsafe so we should
just return an error.

Fixes #42
Reported-by: Erik Sjölund erik.sjolund@gmail.com
Signed-off-by: Aleksa Sarai cyphar@cyphar.com

If a user provides an unclean root path, we will implicitly clean it at
the end of SecureJoin (which may result in a path that doesn't exist or
has "escaped" the root). Such usage is fundamentally unsafe so we should
just return an error.

Reported-by: Erik Sjölund <erik.sjolund@gmail.com>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
@kolyshkin
Copy link
Copy Markdown
Contributor

This breaks the following use case: https://github.com/opencontainers/runc/blob/610aa88ab201f289c05c2e262912d0630f46eb35/tests/cmd/seccompagent/seccompagent.go#L143-L154
because root contains an ending slash. See opencontainers/runc#4590 (comment) for some more info.

I was thinking about more relaxed checking rules (such as, merely checking that root does not contain .. elements) but this would be more complicated to implement.

@kolyshkin
Copy link
Copy Markdown
Contributor

I was thinking about more relaxed checking rules (such as, merely checking that root does not contain .. elements) but this would be more complicated to implement.

Or maybe it's easy. A naive approach is to do something like this:

if strings.Contains("/"+root+"/", "/../") {
	return "", errUncleanRoot
}

This should allow for harmless stuff like ending slash, extra slashes, or . elements, but error out for actually bad root paths.

@cyphar
Copy link
Copy Markdown
Owner Author

cyphar commented Jan 17, 2025

The problem is Windows paths, though I think I can come up with a solution for that... (In theory we just need to strip the volume first but Windows has some wacky features like C:..\ though I guess blocking all of the crap is the best solution).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

How to handle root with ..?

2 participants