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

Optionally allow following symlinks #26

Closed
ebkalderon opened this issue Nov 23, 2019 · 2 comments · Fixed by #28
Closed

Optionally allow following symlinks #26

ebkalderon opened this issue Nov 23, 2019 · 2 comments · Fixed by #28

Comments

@ebkalderon
Copy link
Contributor

It appears that Dir::sub_dir() always forces the O_NOFOLLOW flag when accessing files or subdirectories, which isn't always ideal. In my case, I was struggling to open the /proc/self subdirectory, until I eventually realized that I needed to perform the following dance to make it work:

let proc_dir = Dir::open("/proc")?;
// <snip>
let target = proc_dir.read_link("self")?;
let self_dir = proc_dir.sub_dir(&target)?;

This was surprising, as this was inconsistent with the default behavior of openat() and the IO error it produces was initially mystifying (it simply stated "Not a directory").

It would be nice if users could specify with a boolean flag on sub_dir() whether they would like to follow symlinks or not. At the very least, I think the documentation for sub_dir() should mention that symlinked subdirectories require the use of read_link() to resolve the real path before attempting to access it.

ebkalderon added a commit to ebkalderon/bastille that referenced this issue Nov 23, 2019
This commit fixes some issues with `write_uid_gid_maps()` which in turn
allows us to finally assign a proper user ID and group ID to our
sandboxed process, so now the `Sandbox::uid()` and `Sandbox::gid()`
builder methods actually do something. If the user doesn't specify them,
the sandbox defaults to inheriting the same user and group as the
parent process.

There were a handful of issues with coaxing the `openat` library to do
the right thing, first of which was described in tailhook/openat#26.
Another unsolved issue is that `writeln!()` and `write!()` sometimes
strangely fails when writing to files opened with
`openat::Dir::write_file()`, namely it throws an
`std::io::ErrorKind::InvalidInput` with the message "Invalid argument".
The panic itself appears to be coming from `std::io::Write::write_fmt()`
specfically when invoked on `/proc/self/{uid_map,gid_map}`.

Thankfully, when I use `<File as Write>::write_all()`, it works just
fine, and I can verify from within the sandboxed process that the UIDs
and GIDs are indeed set up correctly from the write. Strange, but at
least this works. :shrug:
ebkalderon added a commit to ebkalderon/bastille that referenced this issue Nov 23, 2019
This commit fixes some issues with `write_uid_gid_maps()` which in turn
allows us to finally assign a proper user ID and group ID to our
sandboxed process, so now the `Sandbox::uid()` and `Sandbox::gid()`
builder methods actually do something. If the user doesn't specify them,
the sandbox defaults to inheriting the same user and group as the
parent process.

There were a handful of issues with coaxing the `openat` library to do
the right thing, first of which was described in tailhook/openat#26.
Another unsolved issue is that `writeln!()` and `write!()` sometimes
strangely fails when writing to files opened with
`openat::Dir::write_file()`, namely it throws an
`std::io::ErrorKind::InvalidInput` with the message "Invalid argument".
The panic itself appears to be coming from `std::io::Write::write_fmt()`
specifically when invoked on `/proc/self/{uid_map,gid_map}`.

Thankfully, when I use `<File as Write>::write_all()`, it works just
fine, and I can verify from within the sandboxed process that the UIDs
and GIDs are indeed set up correctly from the write. Strange, but at
least this works. :shrug:
@tailhook
Copy link
Owner

I'm happy to accept PR for a documentation update.

For the flag: I think we should do that at some point. But we must do it considering #18 and #25 too. (i.e. some "open with options" method). So I'm not sure I can allocate time for it soon.

Just to know the context: any specific reason you're using openat for /proc/self ? I mean what's the benefit comparing to just opening /proc/self directly?

@ebkalderon
Copy link
Contributor Author

Sounds good! I'd be happy to open a PR with just the documentation update for now and close this issue in favor of #18, since I believe its scope encompasses this one.

In the sandboxing library I'm developing, I open the /proc directory with Dir::open() and keep the file descriptor open so that I can pass it into one or more child process, which needs access to various elements of /proc at different times. Sometimes I look at a specific PID under /proc, other times I look at /proc/self depending on what task the child needs to accomplish.

Because of this, I chose to keep a lone file descriptor for /proc open in the parent process and pass it to the child processes to use as they wish while setting up their environments, and it is easy to revoke privileges and drop inherited file descriptors before calling exec() on the desired process to sandbox because there is only one to manage.

With that said, I'm still polishing and simplifying this architecture, so it isn't final.

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