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

Implement secure join #346

Closed
yihuaf opened this issue Sep 29, 2021 · 8 comments
Closed

Implement secure join #346

yihuaf opened this issue Sep 29, 2021 · 8 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@yihuaf
Copy link
Collaborator

yihuaf commented Sep 29, 2021

We need a secure_join implementation that's similar to https://github.com/cyphar/filepath-securejoin. In short, this function guarantee secure_join(rootfs, path) the path is within rootfs. When path is ../../ or other malicious symbolic links, it can cause youki to make changes outside of rootfs, creating security issue. This can be a follow up to #342

An example of usage in runc:

https://github.com/opencontainers/runc/blob/51beb5c436b159ae2d483b219c37ecfde13b006a/libcontainer/utils/utils.go#L119

@yihuaf yihuaf added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed labels Sep 29, 2021
@Ian-Yy
Copy link
Contributor

Ian-Yy commented Sep 30, 2021

Hi, I'm quite new to youki. Can I try to work on this?

@yihuaf
Copy link
Collaborator Author

yihuaf commented Sep 30, 2021

@Ian-Yy Sure. Do you know where to start? Please don't hesitate to ask us on how we can help you.

@Ian-Yy
Copy link
Contributor

Ian-Yy commented Oct 1, 2021

@yihuaf I will try going through the code first. Thanks.

@Ian-Yy
Copy link
Contributor

Ian-Yy commented Oct 1, 2021

Hi @yihuaf , correct me if I am wrong. This task require me to implement a new function secure_join in the trait PathBufExt? Is that right?

@yihuaf
Copy link
Collaborator Author

yihuaf commented Oct 1, 2021

Hi @yihuaf , correct me if I am wrong. This task require me to implement a new function secure_join in the trait PathBufExt? Is that right?

Yes that's a very good idea. Or you can just implement a function secure_join(rootfs: Path, path: Path) as a single function.

@Ian-Yy
Copy link
Contributor

Ian-Yy commented Oct 1, 2021

Sorry but there's one thing I'm quite unsure of now. The implementation of secure_join in https://github.com/cyphar/filepath-securejoin uses a lot of filepath.Clean and we do not have it in Rust. I saw there's a crate path_clean that does exactly the same as Go filepath.Clean but is quite old. Should I use this crate or code it?

@yihuaf
Copy link
Collaborator Author

yihuaf commented Oct 1, 2021

Let's use it to at least prototype.

@Ian-Yy
Copy link
Contributor

Ian-Yy commented Oct 2, 2021

Hi @yihuaf , I've done with a prototype with some tests for it. After working on it for some time, I realized I don't need to use the crate path_clean as mentioned above. Please let me know if there's anything wrong with the PR.

@yihuaf yihuaf closed this as completed Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants