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

root: add mkdir_all() implementation #51

Merged
merged 9 commits into from
Aug 12, 2024
Merged

Conversation

cyphar
Copy link
Member

@cyphar cyphar commented Aug 9, 2024

This is primarily necessary because Go users expect to be able to use
os.MkdirAll (which is not safe to use with an active attacker). We could
try to implement this just for Go within the bindings code, but it turns
out that you need to have a fairly intricate resolve_partial()
implementation for this to be efficient (especially for the opath
resolver).

This implementation is based on filepath-securejoin's safe MkdirAll
implementation.

Fixes #10
Signed-off-by: Aleksa Sarai [email protected]


This still needs:

  • C API and binding updates.
  • The extra "is this the directory we expect" code we have when doing the mkdir-openat loop. Still not entirely sure how useful it is but it wouldn't hurt (unfortunately we need to do readdir which isn't nicely supported by the Rust stdlib...)
    • uid/gid/mode checks.
    • "is the directory empty" checks.
    • dead inode checks?
  • Maybe some tests to make sure bad modes aren't silently accepted?

@cyphar cyphar force-pushed the mkdir-all branch 9 times, most recently from cc6796c to dc28595 Compare August 12, 2024 08:32
This is a revert of 7ea5c47.

It seems very unlikely the overhead of Rc is going to be a problem in
practice, and the SymlinkStack we need for Root::mkdir_all requires us
to convert RcFile::Original to RcFile::Ref which eliminates the major
benefit of RcFile (this is only relevant for partial lookups but the
code is kind of ugly regardless).

Signed-off-by: Aleksa Sarai <[email protected]>
We need to be able to easily iterate over ancestor paths (as well as the
associated "remaining" component) in order to implement mkdirall. This
is effectively an iterative version path_split() (so we can switch
path_split to using it very easily).

Signed-off-by: Aleksa Sarai <[email protected]>
(This is a separate commit so that git-blame is happy.)

Signed-off-by: Aleksa Sarai <[email protected]>
This is needed in order to implement Root::mkdir_all() efficiently.
Unfortunately, for the opath resolver we need to create a "symlink
stack" data structure in order to make sure that we have the same
behaviour as the openat2 resolver when dealing with dangling symlinks.

This logic is mostly taken from filepath-securejoin. However, we treat
all errors (including -ELOOP) as though they are any other kind of
lookup error and return a partial lookup result in that case. Due to
some downsides of the Go implementation of the symlink stack,
filepath-securejoin only does this for -ENOENT and -ENOTDIR.

Signed-off-by: Aleksa Sarai <[email protected]>
For the most part, these tests are identical to the resolve() tests
except that we have to keep track of what the trailing path is.

These tests are based on the same tests in filepath-securejoin.

Signed-off-by: Aleksa Sarai <[email protected]>
Getting the current umask is a little tricky for us because umask is
shared setting and the default mechanism of getting the current umask
(umask(2)) requires changing it temporarily, so as a library we can't be
sure that umask(2) won't break some other thread or process.

Signed-off-by: Aleksa Sarai <[email protected]>
This is primarily necessary because Go users expect to be able to use
os.MkdirAll (which is not safe to use with an active attacker). We could
try to implement this just for Go within the bindings code, but it turns
out that you need to have a fairly intricate resolve_partial()
implementation for this to be efficient (especially for the opath
resolver).

This implementation is based on filepath-securejoin's safe MkdirAll
implementation.

Signed-off-by: Aleksa Sarai <[email protected]>
These tests are mostly based on the filepath-securejoin tests. Most of
the tests are similar to the resolve_partial() tests.

Signed-off-by: Aleksa Sarai <[email protected]>
@cyphar cyphar merged commit 46f7094 into openSUSE:main Aug 12, 2024
14 checks passed
@cyphar cyphar deleted the mkdir-all branch August 12, 2024 10:21
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 this pull request may close these issues.

mkdir_all() implementation
1 participant