Adding less idiomatic, but more accurate wrappers?

[cyphar]

I'm bringing this up because of a discussion with @cgwalters about this crate (see cyphar/libpathrs#1). libpathrs is a library which helps correctly handle path resolution in the face of potential attackers. To this end, I make use of a lot of different *at syscalls -- but because it is a security-sensitive project I am very wary of how Rust crates use different syscalls (to the point where I audited the key methods in the Rust standard library I used, to ensure they did the right thing -- see rust-lang/rust#62425).

@cgwalters asked me whether I could use this library for libpathrs in order to help consolidate Rust *at-using programs to make use of a single library. I took a look, and unfortunately several aspects of the current API concern me a little bit (at least, for the use-case I have). There are three main problems I saw:

1. The API is designed around a Dir object, with everything being a method of such an object. This is probably the most obvious way of doing it, given how all of the *at(2) documentation is written -- but it's not the most useful. AT_EMPTY_PATH (or various other /proc/self/fd tricks) allows you to operate on non-directories, and there are many use-cases for wanting to do that. Another example of this problem is that the renameat2 wrappers can only be used to a rename between two paths with the same Dir -- this is simply not useable for my usecase with libpathrs (I must be able to do all *at(2) operations without any /s in the paths, because that leads to serious attack vectors -- and so I need to use two different Dirs).

2. The flags for all the *at(2) syscalls are not exposed by this crate. While this might make the API much simpler, it seriously restricts the usability of the *at(2) wrappers provided -- as a simple example, if you cannot explicitly specify O_NOFOLLOW or the non-openat(2) equivalent then you simply cannot be secure against certain attacks (in the libpathrs usecase). To be fair, I do agree with your splitting of the functionality of renameat2(2) -- it does three different things (though you didn't expose RENAME_NOREPLACE). Not to mention the lack of O_PATH support -- which is ridiculously critical for libpathrs (the entire library depends on very specific semantics that O_PATH file descriptors provide).

3. While there is some documentation for the methods, I would love to have much more detailed documentation about the security properties (even something as basic as showing what syscalls are called by a given method). For security-critical projects i

My main question is, would you be interested in working together to create an API that can accommodate my requirements while also providing more idiomatic helper wrappers that less-insane users can make use of? Currently libpathrs has its own wrappers (which I trust and understand because I wrote them), but it makes little sense for every project to write their own wrappers for a dozen syscalls.

There is also some hardening work within libpathrs (such as getting a handle to /proc when the library is first loaded, checking that it really is procfs and then using it for all /proc-related shenanigans). It would be nice if things like that were put into this project as well.

Thanks.

[tailhook]

My main question is, would you be interested in working together to create an API that can accommodate my requirements while also providing more idiomatic helper wrappers that less-insane users can make use of?

Yes. Except I'm not sure I have a lot of time to collaborate on this. Also, I want to keep functionality cross-platform where possible. This is a reason it does not expose specific flags.

A couple of points more:

• AT_EMPTY_PATH is not used mostly because it requires a capability, and I don't want to users to have different code paths for root and non-root. But yes we can structure code somehow so it's possible to use (not sure what is the API, though)
• O_PATH is used by default for dirs. The complex issue it that it's linux-specific
• NOFOLLOW is also used by default for most functions (but may need extra review)
• There is a rename function moving between directories, and also hardlink. So there is no issue in using multiple Dir objects. local_rename is just an easy helper
• RENAME_NOREPLACE should be added (perhaps as a different function), I had no need for it myself and nobody contributed yet.

such as getting a handle to /proc when the library is first loaded, checking that it really is procfs and then using it for all /proc-related shenanigans

The issue with this is that I often use openat in containers and /proc can change after pivot_root. I'm not sure it would be a problem in my specific use cases, but I feel we need to be careful with such things. Probably having an explicit handle to /proc for such shenanigans.

And yes, security against some attacks were also in mind when designing the library. There are a couple of things to keep in mind when using the library though (like always using a single component in path). I agree the documentation should be improved.

The simplest way to expose all the functionality is to have openat::linux with all the system calls exposed as is with just fd parameters replaced with Dir. And then gradually improving methods and helper functions for most common operations. What do you think?

[cyphar]

@tailhook

Except I'm not sure I have a lot of time to collaborate on this.

That's okay, I can look into it and throw patches over the wall.

Also, I want to keep functionality cross-platform where possible. This is a reason it does not expose specific flags.

Ah, okay -- I do vaguely recall that some other Unices have openat but it's a somewhat "stunted" (or less bloated, depending on your PoV) version compared to the one we have on Linux.

A couple of points more:

• AT_EMPTY_PATH is not used mostly because it requires a capability, and I don't want to users to have different code paths for root and non-root. But yes we can structure code somehow so it's possible to use (not sure what is the API, though)

It doesn't necessarily require capabilities -- you're probably thinking of linkat (which does) but there are quite a few other syscalls (readlinkat and fstatat come to mind) where there is no such requirement. In the case of readlinkat you actually need AT_EMPTY_PATH because obviously if you try to readlink a procfs magic-link you won't get the contents of the symlink you have a handle to. (EDIT: I forgot that readlinkat inexplicably doesn't take a flag argument.)

In fact, I think the only example of AT_EMPTY_PATH which requires capabilities is linkat.

• O_PATH is used by default for dirs. The complex issue it that it's linux-specific

I guess my point was that the actual open wrapper doesn't provide the ability (even if Linux-specific and gated by #[cfg(linux)]) to create an O_PATH descriptor.

• NOFOLLOW is also used by default for most functions (but may need extra review)

Ah okay, it might be the case that this should be documented better.

• There is a rename function moving between directories, and also hardlink. So there is no issue in using multiple Dir objects. local_rename is just an easy helper

I completely missed this, sorry about that.

• RENAME_NOREPLACE should be added (perhaps as a different function), I had no need for it myself and nobody contributed yet.

Fair enough (though RENAME_NOREPLACE can be used with all three renameat2 modes, maybe it's better as a boolean argument).

The issue with this is that I often use openat in containers and /proc can change after pivot_root. I'm not sure it would be a problem in my specific use cases, but I feel we need to be careful with such things.

Since we only care about /proc/self this isn't as much of a correctness issue as you might think. For the current process, /proc/self will always be valid because pidns applies to children -- and if you fork+exec then the /proc handle will be reloaded from the container one. Even if it didn't though, /proc/self using a host /proc handle always works even inside a child pidns (it's host processes trying to open /proc/self in a container that becomes an issue -- in fact, this might be an even stronger argument to grab a handle),

However, there is a security counter-argument -- an open O_PATH to a host directory is usually a bit of a worry because these descriptors can be used by an attacker in the container (assuming they get around the privilege problems) to escalate to the host's mount namespace. There are several workarounds for this (we use some of them in runc), which boil down to:

1. prctl(PR_SET_DUMPABLE, 0) to disallow ptrace-unprivileged users from touching /proc/$pid/fd.
2. Setting all file descriptors to O_CLOEXEC.
3. Forcibly closing the O_CLOEXEC file descriptors before exec (this was required because of a race condition in the kernel -- which I fixed a few years back, but it doesn't hurt to be safe).
4. Using user namespaces for containers (this provides a ridiculous protections against racing-join attacks).

All of that being said, there's no need to do this right now -- I just wanted to mention it as something you might find interesting to look into. Or we could just get users to provide a handle explicitly as you suggested.

And yes, security against some attacks were also in mind when designing the library. There are a couple of things to keep in mind when using the library though (like always using a single component in path). I agree the documentation should be improved.

That's great to hear. As an aside (in case you're interested) I'm currently posting patches for openat2(2) (and libpathrs makes use of it if available).

The simplest way to expose all the functionality is to have openat::linux with all the system calls exposed as is with just fd parameters replaced with Dir. And then gradually improving methods and helper functions for most common operations. What do you think?

Yeah this seems like a good idea (though maybe we should pass &Files or AsRawFds instead of raw file descriptors).

[cgwalters]

O_PATH is used by default for dirs. The complex issue it that it's linux-specific

The cross-platform aspect is tricky. I feel like since the Rust libstd covers a fully cross-platform API for generic filesystem stuff, our role here should be to expose more operating-system specific features.

It's clear we want a standard crate that has the low-level wrappers. And probably large chunks of that is going to be #[cfg(target_os = linux)].

That said, on the flip side even Rust libstd should probably be using openat() for remove_dir_all_recursive(). Having some of the code in libstd that doesn't use fd-relative APIs call into this crate should probably be a consideration too.

BTW, another random note on the API topic; this crate hardcoding O_PATH didn't quite work for one use case I had: https://github.com/coreos/rpm-ostree/blob/c8113bde32a6235704603e4cc562af8437331b59/rust/src/coreos_rootfs.rs#L21

[tailhook]

The cross-platform aspect is tricky. I feel like since the Rust libstd covers a fully cross-platform API for generic filesystem stuff, our role here should be to expose more operating-system specific features.

Rust stdlib doesn't provide same level of security (preventing symlink attacks, and similar things).

On the other hand if we just reexport libc wrappers over AsRawFd as proposed in previous comment, it doesn't add anything valuable.

It often not clear how to combine all that flags and open modes in linux kernel to make simple file management tool. And the idea to make a lot of safe use cases easier and obvious to write.

[cgwalters]

OK a year later...I was looking at this again because I'd like to make use of openat2()'s functionality in some of my projects - specifically I want to create "rooted" directory file descriptors via RESOLVE_BENEATH | RESOLVE_NOMAGICLINKS.

(A huge trap with openat() in general is one can be super careful to use relative paths but if you accidentally add a leading / in privileged code you can end up writing the file where you didn't expect)

[cgwalters]

So I think what I want is just the code from https://github.com/openSUSE/libpathrs/blob/ad7e8c2ebccdcd6dbb682bb3cec31650ab1c55e1/src/syscalls.rs#L664 in this library - in this case I'd be fine with a "use if available" semantics, otherwise fall back to a regular openat().