| Message ID | AA7BADA2-06D5-4A20-A38F-57D374B6C58D@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 3 March 2017 at 15:55, G 3 <programmingkidx@gmail.com> wrote: > Here is the patch. I think we should let Mark or some else test it to see if > it does fix the problem before a real patch is submitted. > > --- > hw/9pfs/9p-util.h | 4 ++++ > 1 file changed, 4 insertions(+) We can't take any patch without a Signed-off-by: line, not even a three line one. > > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h > index 091f3ce..254d2a9 100644 > --- a/hw/9pfs/9p-util.h > +++ b/hw/9pfs/9p-util.h > @@ -13,6 +13,10 @@ > #ifndef QEMU_9P_UTIL_H > #define QEMU_9P_UTIL_H > > +#ifndef O_PATH > + #define O_PATH 0 > +#endif Could use a comment explaining why it's OK to define it in a way that means it's a no-op on hosts without it. > + > static inline void close_preserve_errno(int fd) > { > int serrno = errno; > -- > 2.10.2 thanks -- PMM
On Mar 3, 2017, at 10:58 AM, Peter Maydell wrote: > On 3 March 2017 at 15:55, G 3 <programmingkidx@gmail.com> wrote: >> Here is the patch. I think we should let Mark or some else test it >> to see if >> it does fix the problem before a real patch is submitted. >> >> --- >> hw/9pfs/9p-util.h | 4 ++++ >> 1 file changed, 4 insertions(+) > > We can't take any patch without a Signed-off-by: line, not > even a three line one. This was more of a RFC kind of patch. It is a pre-patch. I honestly don't know if it will work. > >> >> diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h >> index 091f3ce..254d2a9 100644 >> --- a/hw/9pfs/9p-util.h >> +++ b/hw/9pfs/9p-util.h >> @@ -13,6 +13,10 @@ >> #ifndef QEMU_9P_UTIL_H >> #define QEMU_9P_UTIL_H >> >> +#ifndef O_PATH >> + #define O_PATH 0 >> +#endif > > Could use a comment explaining why it's OK to define it in > a way that means it's a no-op on hosts without it. Ok. > >> + >> static inline void close_preserve_errno(int fd) >> { >> int serrno = errno; >> -- >> 2.10.2 > > thanks > -- PMM Thank you.
On Fri, 3 Mar 2017 15:58:13 +0000 Peter Maydell <peter.maydell@linaro.org> wrote: > On 3 March 2017 at 15:55, G 3 <programmingkidx@gmail.com> wrote: > > Here is the patch. I think we should let Mark or some else test it to see if > > it does fix the problem before a real patch is submitted. > > > > --- > > hw/9pfs/9p-util.h | 4 ++++ > > 1 file changed, 4 insertions(+) > > We can't take any patch without a Signed-off-by: line, not > even a three line one. > > > > > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h > > index 091f3ce..254d2a9 100644 > > --- a/hw/9pfs/9p-util.h > > +++ b/hw/9pfs/9p-util.h > > @@ -13,6 +13,10 @@ > > #ifndef QEMU_9P_UTIL_H > > #define QEMU_9P_UTIL_H > > > > +#ifndef O_PATH > > + #define O_PATH 0 > > +#endif > > Could use a comment explaining why it's OK to define it in > a way that means it's a no-op on hosts without it. > Right. I'll send a patch with an appropriate comment then. > > + > > static inline void close_preserve_errno(int fd) > > { > > int serrno = errno; > > -- > > 2.10.2 > > thanks > -- PMM
On Fri, Mar 03, 2017 at 10:55:01AM -0500, G 3 wrote: > > On Mar 3, 2017, at 10:44 AM, Greg Kurz wrote: > > > On Fri, 3 Mar 2017 10:28:00 -0500 > > G 3 <programmingkidx@gmail.com> wrote: > > > > > On Mar 3, 2017, at 9:59 AM, qemu-devel-request@nongnu.org wrote: > > > > On 02/03/17 17:40, Daniel P. Berrange wrote: > > > > > > > > > On Thu, Mar 02, 2017 at 05:28:24PM +0000, Mark Cave-Ayland wrote: > > > > > > Does anyone else see the following error when trying to build git > > > > > > master? > > > > > > > > > > > > cc -I/home/build/src/qemu/git/qemu/hw/9pfs -Ihw/9pfs > > > > > > -I/home/build/src/qemu/git/qemu/tcg > > > > > > -I/home/build/src/qemu/git/qemu/tcg/i386 > > > > > > -I/home/build/src/qemu/git/qemu/linux-headers > > > > > > -I/home/build/src/qemu/git/qemu/linux-headers -I. > > > > > > -I/home/build/src/qemu/git/qemu -I/home/build/src/qemu/git/qemu/ > > > > > > include > > > > > > -I/usr/include/pixman-1 > > > > > > -I/home/build/src/qemu/git/qemu/dtc/libfdt > > > > > > -Werror -pthread -I/usr/include/glib-2.0 > > > > > > -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -m64 -mcx16 - > > > > > > D_GNU_SOURCE > > > > > > -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes > > > > > > -Wredundant-decls -Wall -Wundef -Wwrite-strings > > > > > > -Wmissing-prototypes > > > > > > -fno-strict-aliasing -fno-common -fwrapv -Wendif-labels > > > > > > -Wno-missing-include-dirs -Wempty-body -Wnested-externs > > > > > > -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers > > > > > > -Wold-style-declaration -Wold-style-definition -Wtype-limits > > > > > > -fstack-protector-all -I/usr/include/p11-kit-1 > > > > > > -I/usr/include/libpng12 -I/home/build/src/qemu/git/qemu/tests - > > > > > > MMD -MP > > > > > > -MT hw/9pfs/9p-util.o -MF hw/9pfs/9p-util.d -O2 -U_FORTIFY_SOURCE > > > > > > -D_FORTIFY_SOURCE=2 -g -c -o hw/9pfs/9p-util.o hw/9pfs/9p-util.c > > > > > > In file included from hw/9pfs/9p-util.c:15:0: > > > > > > hw/9pfs/9p-util.h: In function ?openat_dir?: > > > > > > hw/9pfs/9p-util.h:25:57: error: ?O_PATH? undeclared (first use in > > > > > > this > > > > > > function) > > > > > > hw/9pfs/9p-util.h:25:57: note: each undeclared identifier is > > > > > > reported > > > > > > only once for each function it appears in > > > > > > hw/9pfs/9p-util.h:26:1: error: control reaches end of non-void > > > > > > function > > > > > > [-Werror=return-type] > > > > > > > > > > > > Build platform is Debian Wheezy on an x86_64 host. > > > > > > > > > > IIUC, O_PATH was introduced in glibc 2.14 and Wheezy only has 2.13. > > > > > > > > > > So unless we want to make this 9pfs code a configurable > > > > > option, this > > > > > means Debian Wheezy is no longer a supportable platform for QEMU. > > > > > > > > Oh sure, I appreciate that wheezy is getting towards then end of its > > > > lifetime - it's just a little bit inconvenient to break my > > > > development > > > > environment just as we enter 2.9 freeze ;) > > > > > > > > If everyone agrees that wheezy is no longer supported after 2.9 > > > > then I > > > > can look to upgrading, however my QEMU development is done on my > > > > laptop > > > > which is also setup for my day job so it's not a simple case of just > > > > switching the repository and running dist-upgrade to get me going > > > > again... > > > > > > I remember years ago something like O_PATH was not defined on Mac OS > > > X, > > > so the solution was to define the constant as zero. Something like > > > this: > > > > > > #ifndef O_PATH > > > #define O_PATH 0 > > > #endif > > > > > > Maybe this might work in 9p-util.h. > > > > > > > Yes. Please send a patch and I'll merge it. > > > > Cheers. > > > > -- > > Greg > > > Here is the patch. I think we should let Mark or some else test it to see if > it does fix the problem before a real patch is submitted. > > --- > hw/9pfs/9p-util.h | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h > index 091f3ce..254d2a9 100644 > --- a/hw/9pfs/9p-util.h > +++ b/hw/9pfs/9p-util.h > @@ -13,6 +13,10 @@ > #ifndef QEMU_9P_UTIL_H > #define QEMU_9P_UTIL_H > > +#ifndef O_PATH > + #define O_PATH 0 > +#endif Isn't the use of O_PATH required in order to fix the recent security vulnerability in 9p ? If so, then defining it to 0 means the QEMU is silently becoming vulnerable once again which I don't think is a good idea. Regards, Daniel
On Mar 3, 2017, at 11:21 AM, Daniel P. Berrange wrote: > On Fri, Mar 03, 2017 at 10:55:01AM -0500, G 3 wrote: >> >> On Mar 3, 2017, at 10:44 AM, Greg Kurz wrote: >> >>> On Fri, 3 Mar 2017 10:28:00 -0500 >>> G 3 <programmingkidx@gmail.com> wrote: >>> >>>> On Mar 3, 2017, at 9:59 AM, qemu-devel-request@nongnu.org wrote: >>>>> On 02/03/17 17:40, Daniel P. Berrange wrote: >>>>> >>>>>> On Thu, Mar 02, 2017 at 05:28:24PM +0000, Mark Cave-Ayland wrote: >>>>>>> Does anyone else see the following error when trying to build >>>>>>> git >>>>>>> master? >>>>>>> >>>>>>> cc -I/home/build/src/qemu/git/qemu/hw/9pfs -Ihw/9pfs >>>>>>> -I/home/build/src/qemu/git/qemu/tcg >>>>>>> -I/home/build/src/qemu/git/qemu/tcg/i386 >>>>>>> -I/home/build/src/qemu/git/qemu/linux-headers >>>>>>> -I/home/build/src/qemu/git/qemu/linux-headers -I. >>>>>>> -I/home/build/src/qemu/git/qemu -I/home/build/src/qemu/git/qemu/ >>>>>>> include >>>>>>> -I/usr/include/pixman-1 >>>>>>> -I/home/build/src/qemu/git/qemu/dtc/libfdt >>>>>>> -Werror -pthread -I/usr/include/glib-2.0 >>>>>>> -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -m64 -mcx16 - >>>>>>> D_GNU_SOURCE >>>>>>> -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes >>>>>>> -Wredundant-decls -Wall -Wundef -Wwrite-strings >>>>>>> -Wmissing-prototypes >>>>>>> -fno-strict-aliasing -fno-common -fwrapv -Wendif-labels >>>>>>> -Wno-missing-include-dirs -Wempty-body -Wnested-externs >>>>>>> -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers >>>>>>> -Wold-style-declaration -Wold-style-definition -Wtype-limits >>>>>>> -fstack-protector-all -I/usr/include/p11-kit-1 >>>>>>> -I/usr/include/libpng12 -I/home/build/src/qemu/git/qemu/ >>>>>>> tests - >>>>>>> MMD -MP >>>>>>> -MT hw/9pfs/9p-util.o -MF hw/9pfs/9p-util.d -O2 - >>>>>>> U_FORTIFY_SOURCE >>>>>>> -D_FORTIFY_SOURCE=2 -g -c -o hw/9pfs/9p-util.o hw/9pfs/9p- >>>>>>> util.c >>>>>>> In file included from hw/9pfs/9p-util.c:15:0: >>>>>>> hw/9pfs/9p-util.h: In function ?openat_dir?: >>>>>>> hw/9pfs/9p-util.h:25:57: error: ?O_PATH? undeclared (first >>>>>>> use in >>>>>>> this >>>>>>> function) >>>>>>> hw/9pfs/9p-util.h:25:57: note: each undeclared identifier is >>>>>>> reported >>>>>>> only once for each function it appears in >>>>>>> hw/9pfs/9p-util.h:26:1: error: control reaches end of non-void >>>>>>> function >>>>>>> [-Werror=return-type] >>>>>>> >>>>>>> Build platform is Debian Wheezy on an x86_64 host. >>>>>> >>>>>> IIUC, O_PATH was introduced in glibc 2.14 and Wheezy only has >>>>>> 2.13. >>>>>> >>>>>> So unless we want to make this 9pfs code a configurable >>>>>> option, this >>>>>> means Debian Wheezy is no longer a supportable platform for QEMU. >>>>> >>>>> Oh sure, I appreciate that wheezy is getting towards then end >>>>> of its >>>>> lifetime - it's just a little bit inconvenient to break my >>>>> development >>>>> environment just as we enter 2.9 freeze ;) >>>>> >>>>> If everyone agrees that wheezy is no longer supported after 2.9 >>>>> then I >>>>> can look to upgrading, however my QEMU development is done on my >>>>> laptop >>>>> which is also setup for my day job so it's not a simple case of >>>>> just >>>>> switching the repository and running dist-upgrade to get me going >>>>> again... >>>> >>>> I remember years ago something like O_PATH was not defined on >>>> Mac OS >>>> X, >>>> so the solution was to define the constant as zero. Something like >>>> this: >>>> >>>> #ifndef O_PATH >>>> #define O_PATH 0 >>>> #endif >>>> >>>> Maybe this might work in 9p-util.h. >>>> >>> >>> Yes. Please send a patch and I'll merge it. >>> >>> Cheers. >>> >>> -- >>> Greg >> >> >> Here is the patch. I think we should let Mark or some else test it >> to see if >> it does fix the problem before a real patch is submitted. >> >> --- >> hw/9pfs/9p-util.h | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h >> index 091f3ce..254d2a9 100644 >> --- a/hw/9pfs/9p-util.h >> +++ b/hw/9pfs/9p-util.h >> @@ -13,6 +13,10 @@ >> #ifndef QEMU_9P_UTIL_H >> #define QEMU_9P_UTIL_H >> >> +#ifndef O_PATH >> + #define O_PATH 0 >> +#endif > > Isn't the use of O_PATH required in order to fix the recent > security vulnerability in 9p ? If so, then defining it to > 0 means the QEMU is silently becoming vulnerable once again > which I don't think is a good idea. I haven't found any documentation that states O_PATH is required to keep things secure. https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg04231.html This post does talk about fixing security issues with the 9pfs protocol, but there is no mention of the 9p-util.h file. Greg Kurz - do you have an option on O_PATH and security?
On 03/03/2017 10:21 AM, Daniel P. Berrange wrote: >>>> I remember years ago something like O_PATH was not defined on Mac OS >>>> X, >>>> so the solution was to define the constant as zero. Something like >>>> this: >>>> >>>> #ifndef O_PATH >>>> #define O_PATH 0 >>>> #endif >>>> >>>> Maybe this might work in 9p-util.h. >>>> >> >> +#ifndef O_PATH >> + #define O_PATH 0 >> +#endif > > Isn't the use of O_PATH required in order to fix the recent > security vulnerability in 9p ? If so, then defining it to > 0 means the QEMU is silently becoming vulnerable once again > which I don't think is a good idea. My understanding is that O_PATH is an optimization. It lets openat() succeed in some places where it would ordinarily fail (for example, it can be used to open a dir with mode 0000) - the resulting fd is limited-use (it cannot be used to read() or write(), but CAN be used as the relative fd for a subsequent openat(), for example). If you define O_PATH to 0, then attempts to traverse paths will fail where the could have otherwise succeeded, but failure is okay (the CVE was that we were succeeding at opening through a guest-controlled symlink; whether we now fail or guarantee that we are not going through a symlink is a quality of implementation, but either way, we are at least immune from succeeding through a symlink).
On Fri, Mar 03, 2017 at 10:40:13AM -0600, Eric Blake wrote: > On 03/03/2017 10:21 AM, Daniel P. Berrange wrote: > > >> > >> +#ifndef O_PATH > >> + #define O_PATH 0 > >> +#endif > > > > Isn't the use of O_PATH required in order to fix the recent > > security vulnerability in 9p ? If so, then defining it to > > 0 means the QEMU is silently becoming vulnerable once again > > which I don't think is a good idea. > > My understanding is that O_PATH is an optimization. It lets openat() > succeed in some places where it would ordinarily fail (for example, it > can be used to open a dir with mode 0000) - the resulting fd is > limited-use (it cannot be used to read() or write(), but CAN be used as > the relative fd for a subsequent openat(), for example). If you define > O_PATH to 0, then attempts to traverse paths will fail where the could > have otherwise succeeded, but failure is okay (the CVE was that we were > succeeding at opening through a guest-controlled symlink; whether we now > fail or guarantee that we are not going through a symlink is a quality > of implementation, but either way, we are at least immune from > succeeding through a symlink). So we're not vulnerable, but we are breaking some valid guest usage. I don't much like the idea of doing that silently, but i guess there's no better alternative. Regards, Daniel
On Fri, 3 Mar 2017 16:21:28 +0000 "Daniel P. Berrange" <berrange@redhat.com> wrote: > On Fri, Mar 03, 2017 at 10:55:01AM -0500, G 3 wrote: > > > > On Mar 3, 2017, at 10:44 AM, Greg Kurz wrote: > > > > > On Fri, 3 Mar 2017 10:28:00 -0500 > > > G 3 <programmingkidx@gmail.com> wrote: > > > > > > > On Mar 3, 2017, at 9:59 AM, qemu-devel-request@nongnu.org wrote: > > > > > On 02/03/17 17:40, Daniel P. Berrange wrote: > > > > > > > > > > > On Thu, Mar 02, 2017 at 05:28:24PM +0000, Mark Cave-Ayland wrote: > > > > > > > Does anyone else see the following error when trying to build git > > > > > > > master? > > > > > > > > > > > > > > cc -I/home/build/src/qemu/git/qemu/hw/9pfs -Ihw/9pfs > > > > > > > -I/home/build/src/qemu/git/qemu/tcg > > > > > > > -I/home/build/src/qemu/git/qemu/tcg/i386 > > > > > > > -I/home/build/src/qemu/git/qemu/linux-headers > > > > > > > -I/home/build/src/qemu/git/qemu/linux-headers -I. > > > > > > > -I/home/build/src/qemu/git/qemu -I/home/build/src/qemu/git/qemu/ > > > > > > > include > > > > > > > -I/usr/include/pixman-1 > > > > > > > -I/home/build/src/qemu/git/qemu/dtc/libfdt > > > > > > > -Werror -pthread -I/usr/include/glib-2.0 > > > > > > > -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -m64 -mcx16 - > > > > > > > D_GNU_SOURCE > > > > > > > -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes > > > > > > > -Wredundant-decls -Wall -Wundef -Wwrite-strings > > > > > > > -Wmissing-prototypes > > > > > > > -fno-strict-aliasing -fno-common -fwrapv -Wendif-labels > > > > > > > -Wno-missing-include-dirs -Wempty-body -Wnested-externs > > > > > > > -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers > > > > > > > -Wold-style-declaration -Wold-style-definition -Wtype-limits > > > > > > > -fstack-protector-all -I/usr/include/p11-kit-1 > > > > > > > -I/usr/include/libpng12 -I/home/build/src/qemu/git/qemu/tests - > > > > > > > MMD -MP > > > > > > > -MT hw/9pfs/9p-util.o -MF hw/9pfs/9p-util.d -O2 -U_FORTIFY_SOURCE > > > > > > > -D_FORTIFY_SOURCE=2 -g -c -o hw/9pfs/9p-util.o hw/9pfs/9p-util.c > > > > > > > In file included from hw/9pfs/9p-util.c:15:0: > > > > > > > hw/9pfs/9p-util.h: In function ?openat_dir?: > > > > > > > hw/9pfs/9p-util.h:25:57: error: ?O_PATH? undeclared (first use in > > > > > > > this > > > > > > > function) > > > > > > > hw/9pfs/9p-util.h:25:57: note: each undeclared identifier is > > > > > > > reported > > > > > > > only once for each function it appears in > > > > > > > hw/9pfs/9p-util.h:26:1: error: control reaches end of non-void > > > > > > > function > > > > > > > [-Werror=return-type] > > > > > > > > > > > > > > Build platform is Debian Wheezy on an x86_64 host. > > > > > > > > > > > > IIUC, O_PATH was introduced in glibc 2.14 and Wheezy only has 2.13. > > > > > > > > > > > > So unless we want to make this 9pfs code a configurable > > > > > > option, this > > > > > > means Debian Wheezy is no longer a supportable platform for QEMU. > > > > > > > > > > Oh sure, I appreciate that wheezy is getting towards then end of its > > > > > lifetime - it's just a little bit inconvenient to break my > > > > > development > > > > > environment just as we enter 2.9 freeze ;) > > > > > > > > > > If everyone agrees that wheezy is no longer supported after 2.9 > > > > > then I > > > > > can look to upgrading, however my QEMU development is done on my > > > > > laptop > > > > > which is also setup for my day job so it's not a simple case of just > > > > > switching the repository and running dist-upgrade to get me going > > > > > again... > > > > > > > > I remember years ago something like O_PATH was not defined on Mac OS > > > > X, > > > > so the solution was to define the constant as zero. Something like > > > > this: > > > > > > > > #ifndef O_PATH > > > > #define O_PATH 0 > > > > #endif > > > > > > > > Maybe this might work in 9p-util.h. > > > > > > > > > > Yes. Please send a patch and I'll merge it. > > > > > > Cheers. > > > > > > -- > > > Greg > > > > > > Here is the patch. I think we should let Mark or some else test it to see if > > it does fix the problem before a real patch is submitted. > > > > --- > > hw/9pfs/9p-util.h | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h > > index 091f3ce..254d2a9 100644 > > --- a/hw/9pfs/9p-util.h > > +++ b/hw/9pfs/9p-util.h > > @@ -13,6 +13,10 @@ > > #ifndef QEMU_9P_UTIL_H > > #define QEMU_9P_UTIL_H > > > > +#ifndef O_PATH > > + #define O_PATH 0 > > +#endif > > Isn't the use of O_PATH required in order to fix the recent > security vulnerability in 9p ? If so, then defining it to > 0 means the QEMU is silently becoming vulnerable once again > which I don't think is a good idea. > O_PATH was supposed to be used as an optimization here, since fds returned by this function are only passed to openat()... but your comment makes me realize I inadvertently dropped O_NOFOLLOW between v1 and v2 of the patchset. And this IS an actual vulnerability issue :) And reading the openat() manpage, I see that O_PATH | O_NOFOLLOW doesn't cause openat() to fail, but to return a fd pointing to the symlink which is certainly not what I want :) I guess I'll just stop using O_PATH then. I'll send a patch. > > Regards, > Daniel
On 03/03/2017 10:40 AM, Eric Blake wrote: >> Isn't the use of O_PATH required in order to fix the recent >> security vulnerability in 9p ? If so, then defining it to >> 0 means the QEMU is silently becoming vulnerable once again >> which I don't think is a good idea. > > My understanding is that O_PATH is an optimization. It lets openat() > succeed in some places where it would ordinarily fail (for example, it > can be used to open a dir with mode 0000) - the resulting fd is > limited-use (it cannot be used to read() or write(), but CAN be used as > the relative fd for a subsequent openat(), for example). If you define > O_PATH to 0, then attempts to traverse paths will fail where the could > have otherwise succeeded, but failure is okay (the CVE was that we were > succeeding at opening through a guest-controlled symlink; whether we now > fail or guarantee that we are not going through a symlink is a quality > of implementation, but either way, we are at least immune from > succeeding through a symlink). [I hit send too soon] To put it in perspective, the 9p fixes included code for chmod() that falls back to fchmodat() - but Linux' fchmodat() is broken (it is not POSIX-compliant in that there is no race-free way to use AT_SYMLINK_NOFOLLOW, at least not until Greg gets his kernel patches approved that implement the fchmodat2() syscall [1]). The symptoms are that we now have cases where the guest will get failures where they could have otherwise succeeded if fchmodat() were not broken, but such cases are limited to corners where permissions are overly-tight; in the common case, the permissions will allow opening the file with O_RDONLY or O_WRONLY and fchmod() can be used. So a limited-use fix for the CVE that safely succeeds without symlinks in the common case but fails in the corner case of tight permissions (which is what defining O_PATH to 0 would do) is better than the pre-CVE state of code that succeeds but risks going through a user-controlled symlink. [1] https://lkml.org/lkml/2017/2/28/461
On 03/03/2017 10:43 AM, Greg Kurz wrote: >>> +#ifndef O_PATH >>> + #define O_PATH 0 >>> +#endif >> >> Isn't the use of O_PATH required in order to fix the recent >> security vulnerability in 9p ? If so, then defining it to >> 0 means the QEMU is silently becoming vulnerable once again >> which I don't think is a good idea. >> > > O_PATH was supposed to be used as an optimization here, since fds returned by > this function are only passed to openat()... but your comment makes me realize > I inadvertently dropped O_NOFOLLOW between v1 and v2 of the patchset. And this > IS an actual vulnerability issue :) And reading the openat() manpage, I see > that O_PATH | O_NOFOLLOW doesn't cause openat() to fail, but to return a fd > pointing to the symlink which is certainly not what I want :) Why not? It works, since openat(fd, ...) fails with EBADF if fd is a symlink rather than a directory. (Well, it SHOULD fail like that, according to the man page; I need to write a test program and find out for sure). So you don't have to do any additional syscalls, as your very next *at call will tell you if you actually got a directory or a symlink.
On Fri, 3 Mar 2017 12:11:36 -0600 Eric Blake <eblake@redhat.com> wrote: > On 03/03/2017 10:43 AM, Greg Kurz wrote: > > >>> +#ifndef O_PATH > >>> + #define O_PATH 0 > >>> +#endif > >> > >> Isn't the use of O_PATH required in order to fix the recent > >> security vulnerability in 9p ? If so, then defining it to > >> 0 means the QEMU is silently becoming vulnerable once again > >> which I don't think is a good idea. > >> > > > > O_PATH was supposed to be used as an optimization here, since fds returned by > > this function are only passed to openat()... but your comment makes me realize > > I inadvertently dropped O_NOFOLLOW between v1 and v2 of the patchset. And this > > IS an actual vulnerability issue :) And reading the openat() manpage, I see > > that O_PATH | O_NOFOLLOW doesn't cause openat() to fail, but to return a fd > > pointing to the symlink which is certainly not what I want :) > > Why not? It works, since openat(fd, ...) fails with EBADF if fd is a > symlink rather than a directory. (Well, it SHOULD fail like that, > according to the man page; I need to write a test program and find out > for sure). So you don't have to do any additional syscalls, as your > very next *at call will tell you if you actually got a directory or a > symlink. > O_PATH | O_NOFOLLOW is a special case as described in the last paragraph of O_PATH in the man page: If pathname is a symbolic link and the O_NOFOLLOW flag is also specified, then the call returns a file descriptor referring to the symbolic link. This file descriptor can be used as the dirfd argument in calls to fchownat(2), fstatat(2), linkat(2), and readlinkat(2) with an empty pathname to have the calls oper‐ ate on the symbolic link. Cheers. -- Greg
On 03/03/2017 12:15 PM, Greg Kurz wrote: > > O_PATH | O_NOFOLLOW is a special case as described in the last paragraph > of O_PATH in the man page: > > If pathname is a symbolic link and the O_NOFOLLOW flag is also > specified, then the call returns a file descriptor referring to > the symbolic link. This file descriptor can be used as the > dirfd argument in calls to fchownat(2), fstatat(2), linkat(2), > and readlinkat(2) with an empty pathname to have the calls oper‐ > ate on the symbolic link. Only when coupled with AT_EMPTY_PATHNAME. Without that additional flag, then it must be a directory.
On Fri, 3 Mar 2017 12:28:01 -0600 Eric Blake <eblake@redhat.com> wrote: > On 03/03/2017 12:15 PM, Greg Kurz wrote: > > > > > O_PATH | O_NOFOLLOW is a special case as described in the last paragraph > > of O_PATH in the man page: > > > > If pathname is a symbolic link and the O_NOFOLLOW flag is also > > specified, then the call returns a file descriptor referring to > > the symbolic link. This file descriptor can be used as the > > dirfd argument in calls to fchownat(2), fstatat(2), linkat(2), > > and readlinkat(2) with an empty pathname to have the calls oper‐ > > ate on the symbolic link. > > Only when coupled with AT_EMPTY_PATHNAME. Without that additional flag, > then it must be a directory. > And we don't use AT_EMPTY_PATHNAME, so this should work indeed.
diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h index 091f3ce..254d2a9 100644 --- a/hw/9pfs/9p-util.h +++ b/hw/9pfs/9p-util.h @@ -13,6 +13,10 @@ #ifndef QEMU_9P_UTIL_H #define QEMU_9P_UTIL_H +#ifndef O_PATH + #define O_PATH 0 +#endif + static inline void close_preserve_errno(int fd) { int serrno = errno;