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

Use os.Root for filesystem access #33572

Open
delvh opened this issue Feb 12, 2025 · 4 comments
Open

Use os.Root for filesystem access #33572

delvh opened this issue Feb 12, 2025 · 4 comments
Labels
proposal/accepted We have reviewed the proposal and agree that it should be implemented like that/at all. type/proposal The new feature has not been accepted yet but needs to be discussed first. type/refactoring Existing code has been cleaned up. There should be no new functionality.

Comments

@delvh
Copy link
Member

delvh commented Feb 12, 2025

At the moment, there are a couple of places, where we need to query the filesystem - especially for git data, customizations, templates.
Oftentimes, the places we need to query are user-supplied and must thus be sanitized.
Through the new os.Root in 1.24 we can now ensure that access is only possible in directories we want to access.
As such, we should migrate all filesystem access to use os.Root wherever possible as a security measure.

@delvh delvh added type/proposal The new feature has not been accepted yet but needs to be discussed first. type/refactoring Existing code has been cleaned up. There should be no new functionality. labels Feb 12, 2025
@silverwind
Copy link
Member

silverwind commented Feb 12, 2025

Seems good to avoid future path traversal exploits. I doubt whether all fs access can be migrated. Ideally we should forbid functions like os.ReadFile via lint rule so it will require a // nolint comment to circumvent.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Feb 12, 2025

We already have this and have used it as much as possible. These functions guarantee that the root (first) elem won't be bypassed.

Image

Image

Image

Image

@delvh
Copy link
Member Author

delvh commented Feb 12, 2025

Well… to some extent.
Currently, we ensure it with semantics (we have called this function at runtime beforehand).
However, with this change, we can ensure even at compile time that no out-of-bounds access can occur.
Additionally, it has one other benefit:
Right now, we silently ignore the invalid parts of parameters.
If we used os.Root, those ignored parts would become error cases instead, which sounds a lot cleaner in my perspective.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Feb 12, 2025

Yes, we could refactor some "PathJoinXxx" caller functions to use os.Root. I think they are the only places need to be taken care of the directory bypass at the moment. IIRC there is no other function directly accessing the filesystem with user input (correct me if I was wrong or I missed anything)

@lunny lunny added the proposal/accepted We have reviewed the proposal and agree that it should be implemented like that/at all. label Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal/accepted We have reviewed the proposal and agree that it should be implemented like that/at all. type/proposal The new feature has not been accepted yet but needs to be discussed first. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

No branches or pull requests

4 participants