Use os.Root for filesystem access #14127

New Issue

GiteaMirror · 2025-11-02T11:03:45-06:00

GiteaMirror commented

2025-11-02 11:03:45 -06:00

Originally created by @delvh on GitHub (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.

Originally created by @delvh on GitHub (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.

GiteaMirror added the proposal/accepted type/proposal type/refactoring labels 2025-11-02 11:03:45 -06:00

GiteaMirror commented

2025-11-02 11:03:46 -06:00

@silverwind commented on GitHub (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.

@silverwind commented on GitHub (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.

GiteaMirror commented

2025-11-02 11:03:46 -06:00

@wxiaoguang commented on GitHub (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.

@wxiaoguang commented on GitHub (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. <details> ![Image](https://github.com/user-attachments/assets/2b75b875-ad12-4039-a50f-e30eb5cad723) ![Image](https://github.com/user-attachments/assets/2b030ec2-ad8e-4488-a457-3083e035b7cb) ![Image](https://github.com/user-attachments/assets/8bff1033-b3aa-4af5-bf98-43f49d5f3833) ![Image](https://github.com/user-attachments/assets/c47a6f85-5305-4c10-bb86-afec4313791b) </details>

GiteaMirror commented

2025-11-02 11:03:46 -06:00

@delvh commented on GitHub (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.

@delvh commented on GitHub (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.

GiteaMirror commented

2025-11-02 11:03:46 -06:00

@wxiaoguang commented on GitHub (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)

@wxiaoguang commented on GitHub (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)

Sign in to join this conversation.

Branches Tags

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: github-starred/gitea#14127

Use os.Root for filesystem access #14127

Use `os.Root` for filesystem access #14127