lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CALcwBGBPaYh98d+3_3k8o+8WCbYU8cNPoOHaqhUduKZYz7Ntow@mail.gmail.com>
Date:   Tue, 12 Sep 2023 11:00:07 +0200
From:   Alfred Piccioni <alpic@...gle.com>
To:     Stephen Smalley <stephen.smalley.work@...il.com>
Cc:     kernel test robot <lkp@...el.com>,
        Paul Moore <paul@...l-moore.com>,
        Eric Paris <eparis@...isplace.org>,
        oe-kbuild-all@...ts.linux.dev, stable@...r.kernel.org,
        selinux@...r.kernel.org, linux-kernel@...r.kernel.org,
        LSM List <linux-security-module@...r.kernel.org>
Subject: Re: [PATCH V2] SELinux: Check correct permissions for FS_IOC32_*

On Mon, Sep 11, 2023 at 3:49 PM Stephen Smalley
<stephen.smalley.work@...il.com> wrote:
>
> On Mon, Sep 11, 2023 at 9:19 AM Stephen Smalley
> <stephen.smalley.work@...il.com> wrote:
> >
> > On Fri, Sep 8, 2023 at 6:54 PM kernel test robot <lkp@...el.com> wrote:
> > >
> > > Hi Alfred,
> > >
> > > kernel test robot noticed the following build errors:
> > >
> > > [auto build test ERROR on 50a510a78287c15cee644f345ef8bac8977986a7]
> > >
> > > url:    https://github.com/intel-lab-lkp/linux/commits/Alfred-Piccioni/SELinux-Check-correct-permissions-for-FS_IOC32_/20230906-200131
> > > base:   50a510a78287c15cee644f345ef8bac8977986a7
> > > patch link:    https://lore.kernel.org/r/20230906115928.3749928-1-alpic%40google.com
> > > patch subject: [PATCH V2] SELinux: Check correct permissions for FS_IOC32_*
> > > config: i386-debian-10.3 (https://download.01.org/0day-ci/archive/20230909/202309090600.NSyo7d2q-lkp@intel.com/config)
> > > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> > > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230909/202309090600.NSyo7d2q-lkp@intel.com/reproduce)
> > >
> > > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > > the same patch/commit), kindly add following tags
> > > | Reported-by: kernel test robot <lkp@...el.com>
> > > | Closes: https://lore.kernel.org/oe-kbuild-all/202309090600.NSyo7d2q-lkp@intel.com/
> > >
> > > All errors (new ones prefixed by >>):
> > >
> > >    security/selinux/hooks.c: In function 'selinux_file_ioctl':
> > > >> security/selinux/hooks.c:3647:9: error: duplicate case value
> > >     3647 |         case FS_IOC32_GETFLAGS:
> > >          |         ^~~~
> > >    security/selinux/hooks.c:3645:9: note: previously used here
> > >     3645 |         case FS_IOC_GETFLAGS:
> > >          |         ^~~~
> > >    security/selinux/hooks.c:3648:9: error: duplicate case value
> > >     3648 |         case FS_IOC32_GETVERSION:
> > >          |         ^~~~
> > >    security/selinux/hooks.c:3646:9: note: previously used here
> > >     3646 |         case FS_IOC_GETVERSION:
> > >          |         ^~~~
> > >    security/selinux/hooks.c:3654:9: error: duplicate case value
> > >     3654 |         case FS_IOC32_SETFLAGS:
> > >          |         ^~~~
> > >    security/selinux/hooks.c:3652:9: note: previously used here
> > >     3652 |         case FS_IOC_SETFLAGS:
> > >          |         ^~~~
> > >    security/selinux/hooks.c:3655:9: error: duplicate case value
> > >     3655 |         case FS_IOC32_SETVERSION:
> > >          |         ^~~~
> > >    security/selinux/hooks.c:3653:9: note: previously used here
> > >     3653 |         case FS_IOC_SETVERSION:
> > >          |         ^~~~
> >
> > Not sure of the right way to fix this while addressing the original
> > issue that this patch was intended to fix. Looking in fs/ioctl.c, I
> > see that the some FS_IOC32 values are remapped to the corresponding
> > FS_IOC values by the compat ioctl syscall entrypoint. Also notice this
> > comment there:
> >
> >         /* RED-PEN how should LSM module know it's handling 32bit? */
> >         error = security_file_ioctl(f.file, cmd, arg);
> >         if (error)
> >                 goto out;
> >
> > So perhaps this is a defect in LSM that needs to be addressed?
>
> Note btw that some of the 32-bit ioctl commands are only handled in
> the fs-specific compat_ioctl routines, e.g. ext4_compat_ioctl()
> handles EXT4_IOC32_GETVERSION == FS_IOC32_GETVERSION and ditto for
> _SETVERSION.
>
> >
> >
> > >
> > >
> > > vim +3647 security/selinux/hooks.c
> > >
> > >   3634
> > >   3635  static int selinux_file_ioctl(struct file *file, unsigned int cmd,
> > >   3636                                unsigned long arg)
> > >   3637  {
> > >   3638          const struct cred *cred = current_cred();
> > >   3639          int error = 0;
> > >   3640
> > >   3641          switch (cmd) {
> > >   3642          case FIONREAD:
> > >   3643          case FIBMAP:
> > >   3644          case FIGETBSZ:
> > >   3645          case FS_IOC_GETFLAGS:
> > >   3646          case FS_IOC_GETVERSION:
> > > > 3647          case FS_IOC32_GETFLAGS:
> > >   3648          case FS_IOC32_GETVERSION:
> > >   3649                  error = file_has_perm(cred, file, FILE__GETATTR);
> > >   3650                  break;
> > >   3651
> > >   3652          case FS_IOC_SETFLAGS:
> > >   3653          case FS_IOC_SETVERSION:
> > >   3654          case FS_IOC32_SETFLAGS:
> > >   3655          case FS_IOC32_SETVERSION:
> > >   3656                  error = file_has_perm(cred, file, FILE__SETATTR);
> > >   3657                  break;
> > >   3658
> > >   3659          /* sys_ioctl() checks */
> > >   3660          case FIONBIO:
> > >   3661          case FIOASYNC:
> > >   3662                  error = file_has_perm(cred, file, 0);
> > >   3663                  break;
> > >   3664
> > >   3665          case KDSKBENT:
> > >   3666          case KDSKBSENT:
> > >   3667                  error = cred_has_capability(cred, CAP_SYS_TTY_CONFIG,
> > >   3668                                              CAP_OPT_NONE, true);
> > >   3669                  break;
> > >   3670
> > >   3671          case FIOCLEX:
> > >   3672          case FIONCLEX:
> > >   3673                  if (!selinux_policycap_ioctl_skip_cloexec())
> > >   3674                          error = ioctl_has_perm(cred, file, FILE__IOCTL, (u16) cmd);
> > >   3675                  break;
> > >   3676
> > >   3677          /* default case assumes that the command will go
> > >   3678           * to the file's ioctl() function.
> > >   3679           */
> > >   3680          default:
> > >   3681                  error = ioctl_has_perm(cred, file, FILE__IOCTL, (u16) cmd);
> > >   3682          }
> > >   3683          return error;
> > >   3684  }
> > >   3685

Hey Stephen,

Thanks for looking into it a bit deeper! This seems a bit of a pickle.
I can think of a few somewhat hacky ways to fix this.

I can just set the flags to check `if FS_IOC32_*; set FS_IOC_*;`,
which is the quickest but kinda hacky.

I can go with the other plan of dropping the irrelevant bytes from the
cmd code, so all codes will be read as u16. This effectively does the
same thing, but may be unclear.

I can also look into whether this can be solved at the LSM or a higher
level. Perhaps the filesystems setting `if FS_IOC32_*; set FS_IOC_*;`
is a hint that something else interesting is going wrong.

I'll spend a little time thinking and investigating and get back with
a more concrete solution. I'll also need to do a bit more robust
testing; it built on my machine!

Thanks!

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ