On Wed, Mar 9, 2022 at 7:36 PM John Johansen
<john.johansen@xxxxxxxxxxxxx> wrote:
> On 3/9/22 13:31, Paul Moore wrote:
> > On Tue, Mar 1, 2022 at 5:15 AM Mickaël Salaün <mic@xxxxxxxxxxx> wrote:
> >> On 01/03/2022 10:22, Christian Brauner wrote:
> >>> On Mon, Feb 28, 2022 at 10:59:35PM +0100, Mickaël Salaün wrote:
> >>>> From: Mickaël Salaün <mic@xxxxxxxxxxxxxxxxxxx>
> >>>>
> >>>> While transitionning to ACC_MODE() with commit 5300990c0370 ("Sanitize
> >>>> f_flags helpers") and then fixing it with commit 6d125529c6cb ("Fix
> >>>> ACC_MODE() for real"), we lost an open flags consistency check. Opening
> >>>> a file with O_WRONLY | O_RDWR leads to an f_flags containing MAY_READ |
> >>>> MAY_WRITE (thanks to the ACC_MODE() helper) and an empty f_mode.
> >>>> Indeed, the OPEN_FMODE() helper transforms 3 (an incorrect value) to 0.
> >>>>
> >>>> Fortunately, vfs_read() and vfs_write() both check for FMODE_READ, or
> >>>> respectively FMODE_WRITE, and return an EBADF error if it is absent.
> >>>> Before commit 5300990c0370 ("Sanitize f_flags helpers"), opening a file
> >>>> with O_WRONLY | O_RDWR returned an EINVAL error. Let's restore this safe
> >>>> behavior.
> >>>
> >>> That specific part seems a bit risky at first glance. Given that the
> >>> patch referenced is from 2009 this means we've been allowing O_WRONLY |
> >>> O_RDWR to succeed for almost 13 years now.
> >>
> >> Yeah, it's an old bug, but we should keep in mind that a file descriptor
> >> created with such flags cannot be used to read nor write. However,
> >> unfortunately, it can be used for things like ioctl, fstat, chdir… I
> >> don't know if there is any user of this trick.
> >>
> >> Either way, there is an inconsistency between those using ACC_MODE() and
> >> those using OPEN_FMODE(). If we decide to take a side for the behavior
> >> of one or the other, without denying to create such FD, it could also
> >> break security policies. We have to choose what to potentially break…
> >
> > I'm not really liking the idea that the empty/0 f_mode field leads to
> > SELinux doing an ioctl access check as opposed to the expected
> > read|write check. Yes, other parts of the code catch the problem, but
> > this is bad from a SELinux perspective. Looking quickly at the other
> > LSMs, it would appear that other LSMs are affected as well.
> >
> > If we're not going to fix file::f_mode, the LSMs probably need to
> > consider using file::f_flags directly in conjunction with a correct
> > OPEN_FMODE() macro (or better yet a small inline function that isn't
> > as ugly).
> >
> yeah, I have to agree
The silence on this has been deafening :/ No thoughts on fixing, or
not fixing OPEN_FMODE(), Al?
At this point I have to assume OPEN_FMODE() isn't changing so I'm
going to go ahead with moving SELinux over to file::f_flags. Once
I've got something working I'll CC the LSM list on the patches in case
the other LSMs want to do something similar. Full disclosure, that
might not happen until early-to-mid next week due to the weekend, new
kernel expected on Sunday, etc.
--
paul-moore.com
