Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide examples how to do os.OpenFile securely after SecureJoin() #12

Closed
alban opened this issue May 17, 2024 · 3 comments · Fixed by #15
Closed

Provide examples how to do os.OpenFile securely after SecureJoin() #12

alban opened this issue May 17, 2024 · 3 comments · Fixed by #15

Comments

@alban
Copy link

alban commented May 17, 2024

Thanks for this package!

As you noted in the README, using SecureJoin() before os.OpenFile() is not always enough if the path components are modified between the 2 functions.

Do you know of Go projects doing this safely?

@cyphar
Copy link
Owner

cyphar commented Jun 26, 2024

There is no safe way to do this with SecureJoin because the API requires you to return a string, but the only safe API is to return an *os.File. libpathrs implements this correctly and the Go bindings are safe in this respect, though the project is not yet ready for wide adoption.

To see a pure Go implemention, the implementation of partialOpen in #13 is basically what is needed to do this safely in Go (though of course you don't want to return partial opens outside of MkdirAll).

It might be worth exposing the safer implementation of partialOpen as a public API in the future (though probably without the "partial" feature), though I think moving entirely to libpathrs is a better choice (getting a safe handle to a file is just one part of safely operating on an untrusted filesystem tree, and the Go stdlib is not designed to be used that way).

@cyphar
Copy link
Owner

cyphar commented Jun 26, 2024

Sorry, it's not correct to say "there is no safe way", it's just that it's inadvisable to try to patch around SecureJoin to be safer in this respect.

You could do SecureJoin, do an OpenFile of that path, and then verify that the path matches the readlink(/proc/self/fd/...). If it matches then you can be sure that the handle matches the path you expect.

However:

  • If the path doesn't exist, this won't help you. What you should do with a non-existent path depends on the operation you want to do, but in general you want to open the (hopefully existing) parent path in order to create new inodes. This requires rewriting callers to use file handles where they currently just use string paths.
  • There is the possibility of information disclosure of the host filesystem's tree based on error messages or timing information to figure out whether certain paths exist on the host. You wouldn't be able to read them, but this could lead to a serious security issue.
  • It may be possible to DoS the program using SecureJoin if there is a disconnected NFS mount or otherwise suspended filesystem on the host, and the attacker tricks you into trying to look up paths from underneath said filesystem. If this happens, the program will go into an uninterruptible sleep, and will not be killable.

Given the last two issues, I'm not sure whether you could say that is a "safe way" of doing it...

@cyphar
Copy link
Owner

cyphar commented Jul 10, 2024

#15 is an implementation of OpenInRoot, which is what users should use if they can. PTAL.

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 a pull request may close this issue.

2 participants