Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

mkdir_all() implementation #10

Closed
cyphar opened this issue Dec 23, 2019 · 1 comment · Fixed by #51
Closed

mkdir_all() implementation #10

cyphar opened this issue Dec 23, 2019 · 1 comment · Fixed by #51
Labels
target/runc Requirement to port runc to libpathrs. target/umoci Requirement to port umoci to libpathrs.

Comments

@cyphar
Copy link
Member

cyphar commented Dec 23, 2019

We need to have a Go-like os.MkdirAll implementation, since a lot of Go programs depend on this behaviour. In principle users could implement this manually but it's really suboptimal if done manually. Instead if we:

  • "Partially resolve" the path (resolve as many components as possible).
  • mkdirat each of the following components (openat-ing the next one).

Then we should have a much more efficient implementation of the above.

@cyphar cyphar added target/umoci Requirement to port umoci to libpathrs. target/runc Requirement to port runc to libpathrs. labels Dec 23, 2019
@cyphar
Copy link
Member Author

cyphar commented Jul 5, 2024

cyphar/filepath-securejoin#13 is a Go implementation of this, for reference. There were a couple of snags I didn't anticipate:

  1. To get feature parity between openat2(RESOLVE_IN_ROOT) and our userspace emulated version, we need to emulate the handling of dangling symlinks correctly. The current implementation expands all symlinks completely and operates on them as though they were part of the original path, but openat2 treats them as atomic objects. A safe MkdirAll implementation cyphar/filepath-securejoin#13 implements the necessary emulation using a "symlink stack".
  2. Because you cannot currently get a handle directly from mkdir, it is possible for the directory to be swapped with another path.
    Currently there are some protections in A safe MkdirAll implementation cyphar/filepath-securejoin#13 to detect this case, but it might be worthwhile to consider whether this is something that is a useful attack or if we should just allow it. The argument against adding protections is that because mkdir_all doesn't care if the directory existed already, we are already comfortable with the directory being an attacker-created directory anyway.

dims pushed a commit to dims/libcontainer that referenced this issue Oct 19, 2024
While we use SecureJoin to try to make all of our target paths inside
the container safe, SecureJoin is not safe against an attacker than can
change the path after we "resolve" it.

os.MkdirAll can inadvertently follow symlinks and thus an attacker could
end up tricking runc into creating empty directories on the host (note
that the container doesn't get access to these directories, and the host
just sees empty directories). However, this could potentially cause DoS
issues by (for instance) creating a directory in a conf.d directory for
a daemon that doesn't handle subdirectories properly.

In addition, the handling for creating file bind-mounts did a plain
open(O_CREAT) on the SecureJoin'd path, which is even more obviously
unsafe (luckily we didn't use O_TRUNC, or this bug could've allowed an
attacker to cause data loss...). Regardless of the symlink issue,
opening an untrusted file could result in a DoS if the file is a hung
tty or some other "nasty" file. We can use mknodat to safely create a
regular file without opening anything anyway (O_CREAT|O_EXCL would also
work but it makes the logic a bit more complicated, and we don't want to
open the file for any particular reason anyway).

libpathrs[1] is the long-term solution for these kinds of problems, but
for now we can patch this particular issue by creating a more restricted
MkdirAll that refuses to resolve symlinks and does the creation using
file descriptors. This is loosely based on a more secure version that
filepath-securejoin now has[2] and will be added to libpathrs soon[3].

[1]: https://github.com/openSUSE/libpathrs
[2]: https://github.com/cyphar/filepath-securejoin/releases/tag/v0.3.0
[3]: openSUSE/libpathrs#10

Fixes: CVE-2024-45310
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
dims pushed a commit to dims/libcontainer that referenced this issue Oct 19, 2024
While we use SecureJoin to try to make all of our target paths inside
the container safe, SecureJoin is not safe against an attacker than can
change the path after we "resolve" it.

os.MkdirAll can inadvertently follow symlinks and thus an attacker could
end up tricking runc into creating empty directories on the host (note
that the container doesn't get access to these directories, and the host
just sees empty directories). However, this could potentially cause DoS
issues by (for instance) creating a directory in a conf.d directory for
a daemon that doesn't handle subdirectories properly.

In addition, the handling for creating file bind-mounts did a plain
open(O_CREAT) on the SecureJoin'd path, which is even more obviously
unsafe (luckily we didn't use O_TRUNC, or this bug could've allowed an
attacker to cause data loss...). Regardless of the symlink issue,
opening an untrusted file could result in a DoS if the file is a hung
tty or some other "nasty" file. We can use mknodat to safely create a
regular file without opening anything anyway (O_CREAT|O_EXCL would also
work but it makes the logic a bit more complicated, and we don't want to
open the file for any particular reason anyway).

libpathrs[1] is the long-term solution for these kinds of problems, but
for now we can patch this particular issue by creating a more restricted
MkdirAll that refuses to resolve symlinks and does the creation using
file descriptors. This is loosely based on a more secure version that
filepath-securejoin now has[2] and will be added to libpathrs soon[3].

[1]: https://github.com/openSUSE/libpathrs
[2]: https://github.com/cyphar/filepath-securejoin/releases/tag/v0.3.0
[3]: openSUSE/libpathrs#10

Fixes: CVE-2024-45310
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
dims pushed a commit to dims/libcontainer that referenced this issue Oct 19, 2024
While we use SecureJoin to try to make all of our target paths inside
the container safe, SecureJoin is not safe against an attacker than can
change the path after we "resolve" it.

os.MkdirAll can inadvertently follow symlinks and thus an attacker could
end up tricking runc into creating empty directories on the host (note
that the container doesn't get access to these directories, and the host
just sees empty directories). However, this could potentially cause DoS
issues by (for instance) creating a directory in a conf.d directory for
a daemon that doesn't handle subdirectories properly.

In addition, the handling for creating file bind-mounts did a plain
open(O_CREAT) on the SecureJoin'd path, which is even more obviously
unsafe (luckily we didn't use O_TRUNC, or this bug could've allowed an
attacker to cause data loss...). Regardless of the symlink issue,
opening an untrusted file could result in a DoS if the file is a hung
tty or some other "nasty" file. We can use mknodat to safely create a
regular file without opening anything anyway (O_CREAT|O_EXCL would also
work but it makes the logic a bit more complicated, and we don't want to
open the file for any particular reason anyway).

libpathrs[1] is the long-term solution for these kinds of problems, but
for now we can patch this particular issue by creating a more restricted
MkdirAll that refuses to resolve symlinks and does the creation using
file descriptors. This is loosely based on a more secure version that
filepath-securejoin now has[2] and will be added to libpathrs soon[3].

[1]: https://github.com/openSUSE/libpathrs
[2]: https://github.com/cyphar/filepath-securejoin/releases/tag/v0.3.0
[3]: openSUSE/libpathrs#10

Fixes: CVE-2024-45310
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
target/runc Requirement to port runc to libpathrs. target/umoci Requirement to port umoci to libpathrs.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant