| Message ID | E1tJWbk-007BH4-OB@kylie.crudebyte.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | 9pfs: fix regression regarding CVE-2023-2861 | expand |
On Fri, 6 Dec 2024 12:20:29 +0100 Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > The released fix for this CVE: > > f6b0de53fb8 ("9pfs: prevent opening special files (CVE-2023-2861)") > > caused a regression with security_model=passthrough. When handling a > 'Tmknod' request there was a side effect that 'Tmknod' request could fail > as 9p server was trying to adjust permissions: > > #6 close_if_special_file (fd=30) at ../hw/9pfs/9p-util.h:140 > #7 openat_file (mode=<optimized out>, flags=2228224, > name=<optimized out>, dirfd=<optimized out>) at > ../hw/9pfs/9p-util.h:181 > #8 fchmodat_nofollow (dirfd=dirfd@entry=31, > name=name@entry=0x5555577ea6e0 "mysocket", mode=493) at > ../hw/9pfs/9p-local.c:360 > #9 local_set_cred_passthrough (credp=0x7ffbbc4ace10, name=0x5555577ea6e0 > "mysocket", dirfd=31, fs_ctx=0x55555811f528) at > ../hw/9pfs/9p-local.c:457 > #10 local_mknod (fs_ctx=0x55555811f528, dir_path=<optimized out>, > name=0x5555577ea6e0 "mysocket", credp=0x7ffbbc4ace10) at > ../hw/9pfs/9p-local.c:702 > #11 v9fs_co_mknod (pdu=pdu@entry=0x555558121140, > fidp=fidp@entry=0x5555574c46c0, name=name@entry=0x7ffbbc4aced0, > uid=1000, gid=1000, dev=<optimized out>, mode=49645, > stbuf=0x7ffbbc4acef0) at ../hw/9pfs/cofs.c:205 > #12 v9fs_mknod (opaque=0x555558121140) at ../hw/9pfs/9p.c:3711 > > That's because server was opening the special file to adjust permissions, > however it was using O_PATH and it would have not returned the file > descriptor to guest. So the call to close_if_special_file() on that branch > was incorrect. > > Let's lift the restriction introduced by f6b0de53fb8 such that it would > allow to open special files on host if O_PATH flag is supplied, not only > for 9p server's own operations as described above, but also for any client > 'Topen' request. > > It is safe to allow opening special files with O_PATH on host, because > O_PATH only allows path based operations on the resulting file descriptor > and prevents I/O such as read() and write() on that file descriptor. > > Fixes: f6b0de53fb8 ("9pfs: prevent opening special files (CVE-2023-2861)") > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2337 > Reported-by: Dirk Herrendorfer <d.herrendoerfer@de.ibm.com> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > --- Reviewed-by: Greg Kurz <groug@kaod.org> > hw/9pfs/9p-util.h | 27 +++++++++++++++++---------- > 1 file changed, 17 insertions(+), 10 deletions(-) > > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h > index 51c94b0116..95ee4da9bd 100644 > --- a/hw/9pfs/9p-util.h > +++ b/hw/9pfs/9p-util.h > @@ -177,20 +177,27 @@ again: > return -1; > } > > - if (close_if_special_file(fd) < 0) { > - return -1; > - } > - > - serrno = errno; > - /* O_NONBLOCK was only needed to open the file. Let's drop it. We don't > - * do that with O_PATH since fcntl(F_SETFL) isn't supported, and openat() > - * ignored it anyway. > - */ > + /* Only if O_PATH is not set ... */ > if (!(flags & O_PATH_9P_UTIL)) { > + /* > + * Prevent I/O on special files (device files, etc.) on host side, > + * however it is safe and required to allow opening them with O_PATH, > + * as this is limited to (required) path based operations only. > + */ > + if (close_if_special_file(fd) < 0) { > + return -1; > + } > + > + serrno = errno; > + /* > + * O_NONBLOCK was only needed to open the file. Let's drop it. We don't > + * do that with O_PATH since fcntl(F_SETFL) isn't supported, and > + * openat() ignored it anyway. > + */ > ret = fcntl(fd, F_SETFL, flags); > assert(!ret); > + errno = serrno; > } > - errno = serrno; > return fd; > } >
On Friday, December 6, 2024 12:20:29 PM CET Christian Schoenebeck wrote: > The released fix for this CVE: > > f6b0de53fb8 ("9pfs: prevent opening special files (CVE-2023-2861)") > > caused a regression with security_model=passthrough. When handling a > 'Tmknod' request there was a side effect that 'Tmknod' request could fail > as 9p server was trying to adjust permissions: > > #6 close_if_special_file (fd=30) at ../hw/9pfs/9p-util.h:140 > #7 openat_file (mode=<optimized out>, flags=2228224, > name=<optimized out>, dirfd=<optimized out>) at > ../hw/9pfs/9p-util.h:181 > #8 fchmodat_nofollow (dirfd=dirfd@entry=31, > name=name@entry=0x5555577ea6e0 "mysocket", mode=493) at > ../hw/9pfs/9p-local.c:360 > #9 local_set_cred_passthrough (credp=0x7ffbbc4ace10, name=0x5555577ea6e0 > "mysocket", dirfd=31, fs_ctx=0x55555811f528) at > ../hw/9pfs/9p-local.c:457 > #10 local_mknod (fs_ctx=0x55555811f528, dir_path=<optimized out>, > name=0x5555577ea6e0 "mysocket", credp=0x7ffbbc4ace10) at > ../hw/9pfs/9p-local.c:702 > #11 v9fs_co_mknod (pdu=pdu@entry=0x555558121140, > fidp=fidp@entry=0x5555574c46c0, name=name@entry=0x7ffbbc4aced0, > uid=1000, gid=1000, dev=<optimized out>, mode=49645, > stbuf=0x7ffbbc4acef0) at ../hw/9pfs/cofs.c:205 > #12 v9fs_mknod (opaque=0x555558121140) at ../hw/9pfs/9p.c:3711 > > That's because server was opening the special file to adjust permissions, > however it was using O_PATH and it would have not returned the file > descriptor to guest. So the call to close_if_special_file() on that branch > was incorrect. > > Let's lift the restriction introduced by f6b0de53fb8 such that it would > allow to open special files on host if O_PATH flag is supplied, not only > for 9p server's own operations as described above, but also for any client > 'Topen' request. > > It is safe to allow opening special files with O_PATH on host, because > O_PATH only allows path based operations on the resulting file descriptor > and prevents I/O such as read() and write() on that file descriptor. > > Fixes: f6b0de53fb8 ("9pfs: prevent opening special files (CVE-2023-2861)") > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2337 > Reported-by: Dirk Herrendorfer <d.herrendoerfer@de.ibm.com> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > --- > hw/9pfs/9p-util.h | 27 +++++++++++++++++---------- > 1 file changed, 17 insertions(+), 10 deletions(-) Queued on 9p.next: https://github.com/cschoenebeck/qemu/commits/9p.next Let's see if we can still land this in 9.2. /Christian
On Tue, 10 Dec 2024 at 09:57, Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > > On Friday, December 6, 2024 12:20:29 PM CET Christian Schoenebeck wrote: > > The released fix for this CVE: > > > > f6b0de53fb8 ("9pfs: prevent opening special files (CVE-2023-2861)") > > > > caused a regression with security_model=passthrough. When handling a > > 'Tmknod' request there was a side effect that 'Tmknod' request could fail > > as 9p server was trying to adjust permissions: > > > > #6 close_if_special_file (fd=30) at ../hw/9pfs/9p-util.h:140 > > #7 openat_file (mode=<optimized out>, flags=2228224, > > name=<optimized out>, dirfd=<optimized out>) at > > ../hw/9pfs/9p-util.h:181 > > #8 fchmodat_nofollow (dirfd=dirfd@entry=31, > > name=name@entry=0x5555577ea6e0 "mysocket", mode=493) at > > ../hw/9pfs/9p-local.c:360 > > #9 local_set_cred_passthrough (credp=0x7ffbbc4ace10, name=0x5555577ea6e0 > > "mysocket", dirfd=31, fs_ctx=0x55555811f528) at > > ../hw/9pfs/9p-local.c:457 > > #10 local_mknod (fs_ctx=0x55555811f528, dir_path=<optimized out>, > > name=0x5555577ea6e0 "mysocket", credp=0x7ffbbc4ace10) at > > ../hw/9pfs/9p-local.c:702 > > #11 v9fs_co_mknod (pdu=pdu@entry=0x555558121140, > > fidp=fidp@entry=0x5555574c46c0, name=name@entry=0x7ffbbc4aced0, > > uid=1000, gid=1000, dev=<optimized out>, mode=49645, > > stbuf=0x7ffbbc4acef0) at ../hw/9pfs/cofs.c:205 > > #12 v9fs_mknod (opaque=0x555558121140) at ../hw/9pfs/9p.c:3711 > > > > That's because server was opening the special file to adjust permissions, > > however it was using O_PATH and it would have not returned the file > > descriptor to guest. So the call to close_if_special_file() on that branch > > was incorrect. > > > > Let's lift the restriction introduced by f6b0de53fb8 such that it would > > allow to open special files on host if O_PATH flag is supplied, not only > > for 9p server's own operations as described above, but also for any client > > 'Topen' request. > > > > It is safe to allow opening special files with O_PATH on host, because > > O_PATH only allows path based operations on the resulting file descriptor > > and prevents I/O such as read() and write() on that file descriptor. > > > > Fixes: f6b0de53fb8 ("9pfs: prevent opening special files (CVE-2023-2861)") > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2337 > > Reported-by: Dirk Herrendorfer <d.herrendoerfer@de.ibm.com> > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > > --- > > hw/9pfs/9p-util.h | 27 +++++++++++++++++---------- > > 1 file changed, 17 insertions(+), 10 deletions(-) > > Queued on 9p.next: > https://github.com/cschoenebeck/qemu/commits/9p.next > > Let's see if we can still land this in 9.2. 9.2 is due to be released today, so this would need to be: * a fix for a critical bug * the bug must be a regression since 9.1 * with a clear justification attached for why it's important enough to delay the release Commit f6b0de53fb8 was in 9.1, which suggests that this isn't a regression since 9.1 ? thanks -- PMM
On Tuesday, December 10, 2024 11:11:47 AM CET Peter Maydell wrote: > On Tue, 10 Dec 2024 at 09:57, Christian Schoenebeck > <qemu_oss@crudebyte.com> wrote: > > > > On Friday, December 6, 2024 12:20:29 PM CET Christian Schoenebeck wrote: > > > The released fix for this CVE: > > > > > > f6b0de53fb8 ("9pfs: prevent opening special files (CVE-2023-2861)") > > > > > > caused a regression with security_model=passthrough. When handling a > > > 'Tmknod' request there was a side effect that 'Tmknod' request could fail > > > as 9p server was trying to adjust permissions: > > > > > > #6 close_if_special_file (fd=30) at ../hw/9pfs/9p-util.h:140 > > > #7 openat_file (mode=<optimized out>, flags=2228224, > > > name=<optimized out>, dirfd=<optimized out>) at > > > ../hw/9pfs/9p-util.h:181 > > > #8 fchmodat_nofollow (dirfd=dirfd@entry=31, > > > name=name@entry=0x5555577ea6e0 "mysocket", mode=493) at > > > ../hw/9pfs/9p-local.c:360 > > > #9 local_set_cred_passthrough (credp=0x7ffbbc4ace10, name=0x5555577ea6e0 > > > "mysocket", dirfd=31, fs_ctx=0x55555811f528) at > > > ../hw/9pfs/9p-local.c:457 > > > #10 local_mknod (fs_ctx=0x55555811f528, dir_path=<optimized out>, > > > name=0x5555577ea6e0 "mysocket", credp=0x7ffbbc4ace10) at > > > ../hw/9pfs/9p-local.c:702 > > > #11 v9fs_co_mknod (pdu=pdu@entry=0x555558121140, > > > fidp=fidp@entry=0x5555574c46c0, name=name@entry=0x7ffbbc4aced0, > > > uid=1000, gid=1000, dev=<optimized out>, mode=49645, > > > stbuf=0x7ffbbc4acef0) at ../hw/9pfs/cofs.c:205 > > > #12 v9fs_mknod (opaque=0x555558121140) at ../hw/9pfs/9p.c:3711 > > > > > > That's because server was opening the special file to adjust permissions, > > > however it was using O_PATH and it would have not returned the file > > > descriptor to guest. So the call to close_if_special_file() on that branch > > > was incorrect. > > > > > > Let's lift the restriction introduced by f6b0de53fb8 such that it would > > > allow to open special files on host if O_PATH flag is supplied, not only > > > for 9p server's own operations as described above, but also for any client > > > 'Topen' request. > > > > > > It is safe to allow opening special files with O_PATH on host, because > > > O_PATH only allows path based operations on the resulting file descriptor > > > and prevents I/O such as read() and write() on that file descriptor. > > > > > > Fixes: f6b0de53fb8 ("9pfs: prevent opening special files (CVE-2023-2861)") > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2337 > > > Reported-by: Dirk Herrendorfer <d.herrendoerfer@de.ibm.com> > > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > > > --- > > > hw/9pfs/9p-util.h | 27 +++++++++++++++++---------- > > > 1 file changed, 17 insertions(+), 10 deletions(-) > > > > Queued on 9p.next: > > https://github.com/cschoenebeck/qemu/commits/9p.next > > > > Let's see if we can still land this in 9.2. > > 9.2 is due to be released today, so this would need to be: > * a fix for a critical bug > * the bug must be a regression since 9.1 > * with a clear justification attached for why it's important > enough to delay the release > > Commit f6b0de53fb8 was in 9.1, which suggests that this isn't > a regression since 9.1 ? Hi Peter, I just saw there was still one open milestone ticket for 9.2 and assumed the release to be delayed by one week anyway. I am not trying to convince you of a delay. So unless somebody objects, just disregard the PR for now. Thanks! /Christian
diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h index 51c94b0116..95ee4da9bd 100644 --- a/hw/9pfs/9p-util.h +++ b/hw/9pfs/9p-util.h @@ -177,20 +177,27 @@ again: return -1; } - if (close_if_special_file(fd) < 0) { - return -1; - } - - serrno = errno; - /* O_NONBLOCK was only needed to open the file. Let's drop it. We don't - * do that with O_PATH since fcntl(F_SETFL) isn't supported, and openat() - * ignored it anyway. - */ + /* Only if O_PATH is not set ... */ if (!(flags & O_PATH_9P_UTIL)) { + /* + * Prevent I/O on special files (device files, etc.) on host side, + * however it is safe and required to allow opening them with O_PATH, + * as this is limited to (required) path based operations only. + */ + if (close_if_special_file(fd) < 0) { + return -1; + } + + serrno = errno; + /* + * O_NONBLOCK was only needed to open the file. Let's drop it. We don't + * do that with O_PATH since fcntl(F_SETFL) isn't supported, and + * openat() ignored it anyway. + */ ret = fcntl(fd, F_SETFL, flags); assert(!ret); + errno = serrno; } - errno = serrno; return fd; }
The released fix for this CVE: f6b0de53fb8 ("9pfs: prevent opening special files (CVE-2023-2861)") caused a regression with security_model=passthrough. When handling a 'Tmknod' request there was a side effect that 'Tmknod' request could fail as 9p server was trying to adjust permissions: #6 close_if_special_file (fd=30) at ../hw/9pfs/9p-util.h:140 #7 openat_file (mode=<optimized out>, flags=2228224, name=<optimized out>, dirfd=<optimized out>) at ../hw/9pfs/9p-util.h:181 #8 fchmodat_nofollow (dirfd=dirfd@entry=31, name=name@entry=0x5555577ea6e0 "mysocket", mode=493) at ../hw/9pfs/9p-local.c:360 #9 local_set_cred_passthrough (credp=0x7ffbbc4ace10, name=0x5555577ea6e0 "mysocket", dirfd=31, fs_ctx=0x55555811f528) at ../hw/9pfs/9p-local.c:457 #10 local_mknod (fs_ctx=0x55555811f528, dir_path=<optimized out>, name=0x5555577ea6e0 "mysocket", credp=0x7ffbbc4ace10) at ../hw/9pfs/9p-local.c:702 #11 v9fs_co_mknod (pdu=pdu@entry=0x555558121140, fidp=fidp@entry=0x5555574c46c0, name=name@entry=0x7ffbbc4aced0, uid=1000, gid=1000, dev=<optimized out>, mode=49645, stbuf=0x7ffbbc4acef0) at ../hw/9pfs/cofs.c:205 #12 v9fs_mknod (opaque=0x555558121140) at ../hw/9pfs/9p.c:3711 That's because server was opening the special file to adjust permissions, however it was using O_PATH and it would have not returned the file descriptor to guest. So the call to close_if_special_file() on that branch was incorrect. Let's lift the restriction introduced by f6b0de53fb8 such that it would allow to open special files on host if O_PATH flag is supplied, not only for 9p server's own operations as described above, but also for any client 'Topen' request. It is safe to allow opening special files with O_PATH on host, because O_PATH only allows path based operations on the resulting file descriptor and prevents I/O such as read() and write() on that file descriptor. Fixes: f6b0de53fb8 ("9pfs: prevent opening special files (CVE-2023-2861)") Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2337 Reported-by: Dirk Herrendorfer <d.herrendoerfer@de.ibm.com> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> --- hw/9pfs/9p-util.h | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-)