Re: [RFC PATCH] selinux: randomize layout of key structures

[Kees Cook <keescook@chromium.org>]

On Tue, Dec 17, 2019 at 08:02:58PM -0500, Wenhui Zhang wrote:
> EVM protects while security attributes data in memory and disk.
> structrand protects security attributes data in memory.
> 
> It seem like EVM introduces higher overhead than trampoline based
> structrand in general.
> 
> So, if we know our disks are reliable,  does it make sense if we add some
> ifdef options for disabling EVM please?
> 
> However this really depends on which one is faster,  the trampoline
> function or the encryption hash function.

There is no trampoline. It's just a per-compile re-ordering of the
structures. It makes attack that depend on knowing structure layouts
less reliable. EVM does not check these at memory access times, so they
are complimentary.

-Kees

> 
> 
> On Tue, Dec 17, 2019 at 7:47 PM Kees Cook <keescook@chromium.org> wrote:
> 
> > On Tue, Dec 17, 2019 at 07:21:49PM -0500, Wenhui Zhang wrote:
> > > In my understanding, evm is called whenever a security related attribute
> > is
> > > committed to VFS.
> > > It is set as enabled  by default.
> > > (ref.
> > >
> > https://github.com/torvalds/linux/blob/8b68150883ca466a23e90902dd4113b22e692f04/security/integrity/evm/evm_main.c
> > > )
> >
> > Okay, so yes, complimentary. randstruct means attackers need use bugs to
> > expose enough memory from the kernel to determine the order of structure
> > members before they can manipulate them with a kernel bug.
> >
> > -Kees
> >
> > >
> > > On Tue, Dec 17, 2019 at 7:16 PM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > > On Tue, Dec 17, 2019 at 07:12:28PM -0500, Wenhui Zhang wrote:
> > > > > Hi, Sorry for bothering you again.
> > > > >
> > > > > However I came into some *evm code* in LSM, which calculates HMAC for
> > > > data
> > > > > structures.
> > > > > It looks like HMAC could protect integrity of LSM data structures.
> > > > >
> > > > > IMHO, __randomize_layout and evm might duplicate the work, any
> > > > instructions
> > > > > on this please?
> > > >
> > > > When does the EVM code perform the checking? I would assume these are
> > > > complimentary features rather than duplicate.
> > > >
> > > > -Kees
> > > >
> > > > >
> > > > >
> > > > > On Tue, Dec 17, 2019 at 6:50 PM Kees Cook <keescook@chromium.org>
> > wrote:
> > > > >
> > > > > > On Fri, Dec 13, 2019 at 03:28:38PM -0500, Stephen Smalley wrote:
> > > > > > > Randomize the layout of key selinux data structures.
> > > > > > > Initially this is applied to the selinux_state, selinux_ss,
> > > > > > > policydb, and task_security_struct data structures.
> > > > > > >
> > > > > > > NB To test/use this mechanism, one must install the
> > > > > > > necessary build-time dependencies, e.g. gcc-plugin-devel on
> > Fedora,
> > > > > > > and enable CONFIG_GCC_PLUGIN_RANDSTRUCT in the kernel
> > configuration.
> > > > > > >
> > > > > > > Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
> > > > > >
> > > > > > Reviewed-by: Kees Cook <keescook@chromium.org>
> > > > > >
> > > > > > -Kees
> > > > > >
> > > > > > > ---
> > > > > > > I would have expected that two kernels built with the same config
> > > > > > > with this enabled would have yielded different struct layouts in
> > > > > > > pahole vmlinux output, but that doesn't appear to be the case.
> > They
> > > > > > > do have different seeds.  Am I doing something wrong?
> > > > > > > Also, does DEBUG_INFO_BTF effectively undermine/negate the
> > benefits
> > > > of
> > > > > > this
> > > > > > > change if enabled?
> > > > > > >
> > > > > > >  security/selinux/include/objsec.h   | 2 +-
> > > > > > >  security/selinux/include/security.h | 2 +-
> > > > > > >  security/selinux/ss/policydb.h      | 2 +-
> > > > > > >  security/selinux/ss/services.h      | 2 +-
> > > > > > >  4 files changed, 4 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/security/selinux/include/objsec.h
> > > > > > b/security/selinux/include/objsec.h
> > > > > > > index a4a86cbcfb0a..330b7b6d44e0 100644
> > > > > > > --- a/security/selinux/include/objsec.h
> > > > > > > +++ b/security/selinux/include/objsec.h
> > > > > > > @@ -35,7 +35,7 @@ struct task_security_struct {
> > > > > > >       u32 create_sid;         /* fscreate SID */
> > > > > > >       u32 keycreate_sid;      /* keycreate SID */
> > > > > > >       u32 sockcreate_sid;     /* fscreate SID */
> > > > > > > -};
> > > > > > > +} __randomize_layout;
> > > > > > >
> > > > > > >  enum label_initialized {
> > > > > > >       LABEL_INVALID,          /* invalid or not initialized */
> > > > > > > diff --git a/security/selinux/include/security.h
> > > > > > b/security/selinux/include/security.h
> > > > > > > index 49737087ad33..3ea406ad91b6 100644
> > > > > > > --- a/security/selinux/include/security.h
> > > > > > > +++ b/security/selinux/include/security.h
> > > > > > > @@ -110,7 +110,7 @@ struct selinux_state {
> > > > > > >       bool policycap[__POLICYDB_CAPABILITY_MAX];
> > > > > > >       struct selinux_avc *avc;
> > > > > > >       struct selinux_ss *ss;
> > > > > > > -};
> > > > > > > +} __randomize_layout;
> > > > > > >
> > > > > > >  void selinux_ss_init(struct selinux_ss **ss);
> > > > > > >  void selinux_avc_init(struct selinux_avc **avc);
> > > > > > > diff --git a/security/selinux/ss/policydb.h
> > > > > > b/security/selinux/ss/policydb.h
> > > > > > > index bc56b14e2216..98afe52a3d19 100644
> > > > > > > --- a/security/selinux/ss/policydb.h
> > > > > > > +++ b/security/selinux/ss/policydb.h
> > > > > > > @@ -307,7 +307,7 @@ struct policydb {
> > > > > > >
> > > > > > >       u16 process_class;
> > > > > > >       u32 process_trans_perms;
> > > > > > > -};
> > > > > > > +} __randomize_layout;;
> > > > > > >
> > > > > > >  extern void policydb_destroy(struct policydb *p);
> > > > > > >  extern int policydb_load_isids(struct policydb *p, struct sidtab
> > > > *s);
> > > > > > > diff --git a/security/selinux/ss/services.h
> > > > > > b/security/selinux/ss/services.h
> > > > > > > index fc40640a9725..c5896f39e8f6 100644
> > > > > > > --- a/security/selinux/ss/services.h
> > > > > > > +++ b/security/selinux/ss/services.h
> > > > > > > @@ -31,7 +31,7 @@ struct selinux_ss {
> > > > > > >       struct selinux_map map;
> > > > > > >       struct page *status_page;
> > > > > > >       struct mutex status_lock;
> > > > > > > -};
> > > > > > > +} __randomize_layout;
> > > > > > >
> > > > > > >  void services_compute_xperms_drivers(struct extended_perms
> > *xperms,
> > > > > > >                               struct avtab_node *node);
> > > > > > > --
> > > > > > > 2.23.0
> > > > > > >
> > > > > >
> > > > > > --
> > > > > > Kees Cook
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > V/R,
> > > > >
> > > > > Wenhui Zhang
> > > > >
> > > > > Email: wenhui@gwmail.gwu.edu
> > > > >            Telephone: 1-(703) 424 3193
> > > >
> > > > --
> > > > Kees Cook
> > > >
> > >
> > >
> > > --
> > > V/R,
> > >
> > > Wenhui Zhang
> > >
> > > Email: wenhui@gwmail.gwu.edu
> > >            Telephone: 1-(703) 424 3193
> >
> > --
> > Kees Cook
> >
> 
> 
> -- 
> V/R,
> 
> Wenhui Zhang
> 
> Email: wenhui@gwmail.gwu.edu
>            Telephone: 1-(703) 424 3193

-- 
Kees Cook