On Wed, 27 Apr 2022 18:18:31 +0200 Christian Schoenebeck <qemu_...@crudebyte.com> wrote:
> On Mittwoch, 27. April 2022 15:31:42 CEST Greg Kurz wrote: > > On Wed, 27 Apr 2022 14:32:53 +0200 > > > > Christian Schoenebeck <qemu_...@crudebyte.com> wrote: > > > On Mittwoch, 27. April 2022 12:18:10 CEST Greg Kurz wrote: > > > > On Wed, 27 Apr 2022 11:27:28 +0900 > > > > > > > > Akihiko Odaki <akihiko.od...@gmail.com> wrote: > > > > > On 2022/04/26 21:38, Greg Kurz wrote: > [...] > > > > > Considering the transient states are tolerated in 9pfs, we need to > > > > > design this function to be tolerant with transient states as well. The > > > > > use of chmod() is not safe when we consider about transient states. A > > > > > malicious actor may replace the file at the path with a symlink which > > > > > may escape the shared directory and chmod() will naively follow it. > > > > > > > > You get a point here. Thanks for your tenacity ! :-) > > > > > > Yep, I send a v4 with fchmodat_nofollow() instead of chmod(), thanks! > > > > > > BTW, why is it actually allowed for client to create a symlink pointing > > > outside exported directory tree with security_model=passthrough/none? Did > > > anybody want that? > > > > The target argument to symlink() is merely a string that is stored in > > the inode. It is only evaluated as a path at the time an action is > > made on the link. Checking at symlink() time is thus useless. > > > > Anyway, we're safe on this side since it's the client's job to > > resolve links and we explicitly don't follow them in the server. > > I wouldn't call it useless, because it is easier to keep exactly 1 hole > stuffed instead of being forced to continuously stuff N holes as new ones may > popup up over time, as this case shows (mea culpa). > > So you are trading (probably minor) performance for security. > > But my question was actually meant seriously: whether there was anybody in the > past who probably actually wanted to create relative symlinks outside the > exported tree. For instance for another service with wider host filesystem > access. > I took the question seriously :-), the problem is that even if you do a check on the target at symlink() time, you don't know how it will be evaluated in the end. Quick demonstration: $ cd /var/tmp/ $ mkdir foo $ cd foo/ $ # Suppose foo is the jail $ mkdir bar $ ln -sf .. bar/link $ realpath bar/link /var/tmp/foo $ # Good, we're still under foo $ mv bar/link . $ realpath link /var/tmp $ # Ouch we've escaped So in the end, the only real fix is to ban path based syscalls and pass AT_SYMLINK_NOFOLLOW everywhere. This was the justification for rewriting nearly all 9p local in order to fix CVE-2016-9602. https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg06225.html > [...] > > > > This brings up a new problem I hadn't realized before : the > > > > fchmodat_nofollow() implementation in 9p-local.c is really > > > > a linux only thing to cope with AT_SYMLINK_NOFOLLOW not being > > > > supported with fchmodat(). It looks that this should move to > > > > 9p-util-linux.c and a proper version should be added for macOS > > > > in 9p-util-darwin.c > > > > > > Like already agreed on the other thread, yes, that makes sense. But I > > > think > > > this can be handled with a follow-up, separate from this series. > > > > Fair enough if you want to handle fchmodat_nofollow() later but you > > must at least use fchmodat(AT_SYMLINK_NOFOLLOW) in this patch > > instead of chmod(). > > Sounds like a quick and easy workaround. However looking at 'man fchmodat' on > macOS, this probably does not exactly do what you wanted it to, at least not > in this particular case: > > AT_SYMLINK_NOFOLLOW > If path names a symbolic link, then the mode of the symbolic > link is changed. > > AT_SYMLINK_NOFOLLOW_ANY > If path names a symbolic link, then the mode of the symbolic > link is changed and > if if the path has any other symbolic links, an error is > returned. > > So if somebody replaced the socket file by a 1st order symlink, you would > adjust the symlink's permissions with both AT_SYMLINK_NOFOLLOW as well as > with > AT_SYMLINK_NOFOLLOW_ANY. I mean it's better than chmod(), but acceptable? > As long as the link is not followed outside, we're good : it will change the symlink mode and then what ? > Using our existing fchmodat_nofollow() instead is no viable alternative > either, as it uses operations that are not supported on socket files on macOS > (tested). > > Best regards, > Christian Schoenebeck > >
