linux-kernel - Re: [PATCH -next] cred: conditionally declare groups-related functions

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] [day] [month] [year] [list]

Message-ID: <CAHC9VhTO7QQt-ZduEZj2y54zbYefCQiZwEpmXaM82_5MqjYoOA@mail.gmail.com>
Date:   Fri, 22 Jun 2018 15:19:08 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     omosnace@...hat.com
Cc:     akpm@...ux-foundation.org, rdunlap@...radead.org,
        sfr@...b.auug.org.au, Eric Paris <eparis@...hat.com>,
        linux-kernel@...r.kernel.org, linux-next@...r.kernel.org,
        linux-audit@...hat.com
Subject: Re: [PATCH -next] cred: conditionally declare groups-related functions

On Fri, Jun 22, 2018 at 3:47 AM Ondrej Mosnacek <omosnace@...hat.com> wrote:
> št 21. 6. 2018 o 23:17 Paul Moore <paul@...l-moore.com> napísal(a):
> > On Thu, Jun 21, 2018 at 4:33 AM Ondrej Mosnacek <omosnace@...hat.com> wrote:
> > > The groups-related functions declared in include/linux/cred.h are
> > > defined in kernel/groups.c, which is compiled only when
> > > CONFIG_MULTIUSER=y. Move all these function declarations under #ifdef
> > > CONFIG_MULTIUSER to help avoid accidental usage in contexts where
> > > CONFIG_MULTIUSER might be disabled.
> > >
> > > This patch also adds a fallback for groups_search(). Currently this
> > > function is only called from kernel/groups.c itself and
> > > keys/permissions.c, which depends on CONFIG_MULTIUSER. However, the
> > > audit subsystem (which does not depend on CONFIG_MULTIUSER) calls this
> > > function in -next, so the fallback will be needed to avoid compilation
> > > errors or ugly workarounds.
> > >
> > > See also:
> > > https://lkml.org/lkml/2018/6/20/670
> > > https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/audit.git/commit/?h=next&id=af85d1772e31fed34165a1b3decef340cf4080c0
> > >
> > > Reported-by: Randy Dunlap <rdunlap@...radead.org>
> > > Signed-off-by: Ondrej Mosnacek <omosnace@...hat.com>
> > > ---
> > >  include/linux/cred.h | 16 +++++++++++-----
> > >  1 file changed, 11 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/include/linux/cred.h b/include/linux/cred.h
> > > index 631286535d0f..8917768453cc 100644
> > > --- a/include/linux/cred.h
> > > +++ b/include/linux/cred.h
> > > @@ -65,6 +65,12 @@ extern void groups_free(struct group_info *);
> > >
> > >  extern int in_group_p(kgid_t);
> > >  extern int in_egroup_p(kgid_t);
> > > +
> > > +extern int set_current_groups(struct group_info *);
> > > +extern void set_groups(struct cred *, struct group_info *);
> > > +extern int groups_search(const struct group_info *, kgid_t);
> > > +extern bool may_setgroups(void);
> > > +extern void groups_sort(struct group_info *);
> > >  #else
> > >  static inline void groups_free(struct group_info *group_info)
> > >  {
> > > @@ -78,12 +84,12 @@ static inline int in_egroup_p(kgid_t grp)
> > >  {
> > >          return 1;
> > >  }
> > > +
> > > +static inline int groups_search(const struct group_info *group_info, kgid_t grp)
> > > +{
> > > +       return 0;
> >
> > Is this the right fallback value?  If CONFIG_MULTIUSER is disabled,
> > wouldn't we always want to indicate a group match?  The in_group_p()
> > and in_egroup_p() dummy functions would seem to indicate that is the
> > correct behavior ...
>
> Hm, indeed this is a bit tricky and I'm guilty of not noticing this...
>
> The way I see it (now that I though about it a little), there are
> basically two possible semantics of groups_search():
> 1. as an (auxiliary) permissions checking function (like
> in_[e]group_p()) -- in this case we would expect the same return value
> as in_group_p(), i.e. 1.
> 2. as a function that simply checks if a group is contained in a list
> of groups (taken from a cred struct) -- in this case we would expect
> it to return 0 in single-user mode, since there will be always no
> supplemental groups set for any task (if I understand it right).
>
> I guess no matter which semantic we pick, we might confuse someone
> expecting the other one, so I would suggest dropping this patch (or at
> least the fallbacks for groups_search) and explicitly handle the
> single-user case in audit.
>
> We should probably default to 1 in audit anyway, because the original
> code used in_[e]group_p(). Even though 0 would seem more logical to
> me, comparing GIDs doesn't really make sense in single-user mode
> anyway, so keeping the legacy behavior will be safer. (In fact now
> that I think of it, having audit enabled (or even compiled) in
> single-user mode does not make much sense either... maybe we should
> just make CONFIG_AUDIT depend on CONFIG_MULTIUSER...).

If CONFIG_MULTIUSER is unset then basically everything runs as
root:root, there is no user separation anymore.  With that in mind it
seems the proper behavior is to always return 1 when groups_search()
is called.

Please make the adjustment and resubmit your patch.

-- 
paul moore
www.paul-moore.com