| Message ID | 20220615152623.311223-8-cgzones@googlemail.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [v3,1/8] capability: add any wrapper to test for multiple caps with exactly one audit message | expand |
On Wed, Jun 15, 2022 at 05:26:23PM +0200, Christian Göttsche wrote: > Add the interfaces `capable_any()` and `ns_capable_any()` as an > alternative to multiple `capable()`/`ns_capable()` calls, like > `capable_any(CAP_SYS_NICE, CAP_SYS_ADMIN)` instead of > `capable(CAP_SYS_NICE) || capable(CAP_SYS_ADMIN)`. > > `capable_any()`/`ns_capable_any()` will in particular generate exactly > one audit message, either for the left most capability in effect or, if > the task has none, the first one. > > This is especially helpful with regard to SELinux, where each audit > message about a not allowed capability will create an AVC denial. > Using this function with the least invasive capability as left most > argument (e.g. CAP_SYS_NICE before CAP_SYS_ADMIN) enables policy writers > to only allow the least invasive one and SELinux domains pass this check > with only capability:sys_nice or capability:sys_admin allowed without > any AVC denial message. > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> Reviewed-by: Serge Hallyn <serge@hallyn.com> > > --- > v3: > - rename to capable_any() > - fix typo in function documentation > - add ns_capable_any() > v2: > avoid varargs and fix to two capabilities; capable_or3() can be added > later if needed > --- > include/linux/capability.h | 10 +++++++ > kernel/capability.c | 53 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 63 insertions(+) > > diff --git a/include/linux/capability.h b/include/linux/capability.h > index 65efb74c3585..7316d5339a6e 100644 > --- a/include/linux/capability.h > +++ b/include/linux/capability.h > @@ -208,7 +208,9 @@ extern bool has_capability_noaudit(struct task_struct *t, int cap); > extern bool has_ns_capability_noaudit(struct task_struct *t, > struct user_namespace *ns, int cap); > extern bool capable(int cap); > +extern bool capable_any(int cap1, int cap2); > extern bool ns_capable(struct user_namespace *ns, int cap); > +extern bool ns_capable_any(struct user_namespace *ns, int cap1, int cap2); > extern bool ns_capable_noaudit(struct user_namespace *ns, int cap); > extern bool ns_capable_setid(struct user_namespace *ns, int cap); > #else > @@ -234,10 +236,18 @@ static inline bool capable(int cap) > { > return true; > } > +static inline bool capable_any(int cap1, int cap2) > +{ > + return true; > +} > static inline bool ns_capable(struct user_namespace *ns, int cap) > { > return true; > } > +static inline bool ns_capable_any(struct user_namespace *ns, int cap1, int cap2) > +{ > + return true; > +} > static inline bool ns_capable_noaudit(struct user_namespace *ns, int cap) > { > return true; > diff --git a/kernel/capability.c b/kernel/capability.c > index 765194f5d678..ab9b889c3f4d 100644 > --- a/kernel/capability.c > +++ b/kernel/capability.c > @@ -435,6 +435,59 @@ bool ns_capable_setid(struct user_namespace *ns, int cap) > } > EXPORT_SYMBOL(ns_capable_setid); > > +/** > + * ns_capable_any - Determine if the current task has one of two superior capabilities in effect > + * @ns: The usernamespace we want the capability in > + * @cap1: The capabilities to be tested for first > + * @cap2: The capabilities to be tested for secondly > + * > + * Return true if the current task has at least one of the two given superior > + * capabilities currently available for use, false if not. > + * > + * In contrast to or'ing capable() this call will create exactly one audit > + * message, either for @cap1, if it is granted or both are not permitted, > + * or @cap2, if it is granted while the other one is not. > + * > + * The capabilities should be ordered from least to most invasive, i.e. CAP_SYS_ADMIN last. > + * > + * This sets PF_SUPERPRIV on the task if the capability is available on the > + * assumption that it's about to be used. > + */ > +bool ns_capable_any(struct user_namespace *ns, int cap1, int cap2) > +{ > + if (ns_capable_noaudit(ns, cap1)) > + return ns_capable(ns, cap1); > + > + if (ns_capable_noaudit(ns, cap2)) > + return ns_capable(ns, cap2); > + > + return ns_capable(ns, cap1); > +} > +EXPORT_SYMBOL(ns_capable_any); > + > +/** > + * capable_any - Determine if the current task has one of two superior capabilities in effect > + * @cap1: The capabilities to be tested for first > + * @cap2: The capabilities to be tested for secondly > + * > + * Return true if the current task has at least one of the two given superior > + * capabilities currently available for use, false if not. > + * > + * In contrast to or'ing capable() this call will create exactly one audit > + * message, either for @cap1, if it is granted or both are not permitted, > + * or @cap2, if it is granted while the other one is not. > + * > + * The capabilities should be ordered from least to most invasive, i.e. CAP_SYS_ADMIN last. > + * > + * This sets PF_SUPERPRIV on the task if the capability is available on the > + * assumption that it's about to be used. > + */ > +bool capable_any(int cap1, int cap2) > +{ > + return ns_capable_any(&init_user_ns, cap1, cap2); > +} > +EXPORT_SYMBOL(capable_any); > + > /** > * capable - Determine if the current task has a superior capability in effect > * @cap: The capability to be tested for > -- > 2.36.1
On Mon, 27 Jun 2022 at 00:34, Serge E. Hallyn <serge@hallyn.com> wrote: > > On Wed, Jun 15, 2022 at 05:26:23PM +0200, Christian Göttsche wrote: > > Add the interfaces `capable_any()` and `ns_capable_any()` as an > > alternative to multiple `capable()`/`ns_capable()` calls, like > > `capable_any(CAP_SYS_NICE, CAP_SYS_ADMIN)` instead of > > `capable(CAP_SYS_NICE) || capable(CAP_SYS_ADMIN)`. > > > > `capable_any()`/`ns_capable_any()` will in particular generate exactly > > one audit message, either for the left most capability in effect or, if > > the task has none, the first one. > > > > This is especially helpful with regard to SELinux, where each audit > > message about a not allowed capability will create an AVC denial. > > Using this function with the least invasive capability as left most > > argument (e.g. CAP_SYS_NICE before CAP_SYS_ADMIN) enables policy writers > > to only allow the least invasive one and SELinux domains pass this check > > with only capability:sys_nice or capability:sys_admin allowed without > > any AVC denial message. > > > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > Reviewed-by: Serge Hallyn <serge@hallyn.com> > Kindly ping. So far patch 3 was reviewed [1] and patch 4 was reviewed [2,3] and partially acked [4]. Currently this series trivially rebases on top of 6.0-rc1. Should I send a rebased v4 or what is the best way to move forward? [1]: https://lore.kernel.org/all/7fd6f544-0bd2-62fe-bddd-869364f351e8@acm.org/ [2]: https://lore.kernel.org/all/Yqn+sCXTHeTH5v+R@pendragon.ideasonboard.com/ [3]: https://lore.kernel.org/all/09374557-8c8d-1925-340c-784f29630ec5@kernel.org/ [4]: https://lore.kernel.org/all/73a603a2-5e5e-1b45-8e19-ab0795027336@xs4all.nl/
On Tue, Aug 30, 2022 at 11:05 AM Christian Göttsche <cgzones@googlemail.com> wrote: > > On Mon, 27 Jun 2022 at 00:34, Serge E. Hallyn <serge@hallyn.com> wrote: > > > > On Wed, Jun 15, 2022 at 05:26:23PM +0200, Christian Göttsche wrote: > > > Add the interfaces `capable_any()` and `ns_capable_any()` as an > > > alternative to multiple `capable()`/`ns_capable()` calls, like > > > `capable_any(CAP_SYS_NICE, CAP_SYS_ADMIN)` instead of > > > `capable(CAP_SYS_NICE) || capable(CAP_SYS_ADMIN)`. > > > > > > `capable_any()`/`ns_capable_any()` will in particular generate exactly > > > one audit message, either for the left most capability in effect or, if > > > the task has none, the first one. > > > > > > This is especially helpful with regard to SELinux, where each audit > > > message about a not allowed capability will create an AVC denial. > > > Using this function with the least invasive capability as left most > > > argument (e.g. CAP_SYS_NICE before CAP_SYS_ADMIN) enables policy writers > > > to only allow the least invasive one and SELinux domains pass this check > > > with only capability:sys_nice or capability:sys_admin allowed without > > > any AVC denial message. > > > > > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > > > Reviewed-by: Serge Hallyn <serge@hallyn.com> > > Kindly ping. > > So far patch 3 was reviewed [1] and patch 4 was reviewed [2,3] and > partially acked [4]. > > Currently this series trivially rebases on top of 6.0-rc1. > Should I send a rebased v4 or what is the best way to move forward? Hi Christian, Sorry for the delay, this is one of those things that was stalled a bit during the maintainer hand-off. It's on my list of things to look at, it is just unfortunate that we have had a lot of things going on at the LSM layer lately; don't respin it just yet, let me take a quick look first ...
On Wed, Jun 15, 2022 at 11:27 AM Christian Göttsche <cgzones@googlemail.com> wrote: > > Add the interfaces `capable_any()` and `ns_capable_any()` as an > alternative to multiple `capable()`/`ns_capable()` calls, like > `capable_any(CAP_SYS_NICE, CAP_SYS_ADMIN)` instead of > `capable(CAP_SYS_NICE) || capable(CAP_SYS_ADMIN)`. > > `capable_any()`/`ns_capable_any()` will in particular generate exactly > one audit message, either for the left most capability in effect or, if > the task has none, the first one. > > This is especially helpful with regard to SELinux, where each audit > message about a not allowed capability will create an AVC denial. > Using this function with the least invasive capability as left most > argument (e.g. CAP_SYS_NICE before CAP_SYS_ADMIN) enables policy writers > to only allow the least invasive one and SELinux domains pass this check > with only capability:sys_nice or capability:sys_admin allowed without > any AVC denial message. > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > --- > v3: > - rename to capable_any() > - fix typo in function documentation > - add ns_capable_any() > v2: > avoid varargs and fix to two capabilities; capable_or3() can be added > later if needed > --- > include/linux/capability.h | 10 +++++++ > kernel/capability.c | 53 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 63 insertions(+) ... > diff --git a/kernel/capability.c b/kernel/capability.c > index 765194f5d678..ab9b889c3f4d 100644 > --- a/kernel/capability.c > +++ b/kernel/capability.c > @@ -435,6 +435,59 @@ bool ns_capable_setid(struct user_namespace *ns, int cap) > } > EXPORT_SYMBOL(ns_capable_setid); > > +/** > + * ns_capable_any - Determine if the current task has one of two superior capabilities in effect > + * @ns: The usernamespace we want the capability in > + * @cap1: The capabilities to be tested for first > + * @cap2: The capabilities to be tested for secondly > + * > + * Return true if the current task has at least one of the two given superior > + * capabilities currently available for use, false if not. > + * > + * In contrast to or'ing capable() this call will create exactly one audit > + * message, either for @cap1, if it is granted or both are not permitted, > + * or @cap2, if it is granted while the other one is not. > + * > + * The capabilities should be ordered from least to most invasive, i.e. CAP_SYS_ADMIN last. > + * > + * This sets PF_SUPERPRIV on the task if the capability is available on the > + * assumption that it's about to be used. > + */ > +bool ns_capable_any(struct user_namespace *ns, int cap1, int cap2) > +{ > + if (ns_capable_noaudit(ns, cap1)) > + return ns_capable(ns, cap1); > + > + if (ns_capable_noaudit(ns, cap2)) > + return ns_capable(ns, cap2); > + > + return ns_capable(ns, cap1); I'm slightly concerned that some people are going to be upset about making an additional call into the capabilities code with this function. I think we need to be a bit more clever here to take out some of the extra work. I wonder if we create a new capability function, call it ns_capable_audittrue(...) or something like that, that only generates an audit record if the current task has the requested capability; if the current task does not have the requested capability no audit record is generated. With this new function I think we could rewrite ns_capable_any(...) like this: bool ns_capable_any(ns, cap1, cap2) { if (ns_capable_audittrue(ns, cap1)) return true; if (ns_capable_audittrue(ns, cap2)) return true; return ns_capable(ns, cap1); } ... we would still have an extra capability check in the failure case, but that's an error case anyway and not likely to draw much concern. Of course this would require some additional work, meaning a new CAP_OPT_XXX flag (CAP_OPT_AUDITTRUE?), and updates to the individual LSMs. However, the good news here is that it appears only SELinux and AppArmor would need modification (the others don't care about capabilities or audit) and in each case the modification to support the new CAP_OPT_AUDITTRUE flag look pretty simple. Thoughts? > +} > +EXPORT_SYMBOL(ns_capable_any);
On Thu, Sep 1, 2022 at 8:56 PM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Jun 15, 2022 at 11:27 AM Christian Göttsche > <cgzones@googlemail.com> wrote: > > > > Add the interfaces `capable_any()` and `ns_capable_any()` as an > > alternative to multiple `capable()`/`ns_capable()` calls, like > > `capable_any(CAP_SYS_NICE, CAP_SYS_ADMIN)` instead of > > `capable(CAP_SYS_NICE) || capable(CAP_SYS_ADMIN)`. > > > > `capable_any()`/`ns_capable_any()` will in particular generate exactly > > one audit message, either for the left most capability in effect or, if > > the task has none, the first one. > > > > This is especially helpful with regard to SELinux, where each audit > > message about a not allowed capability will create an AVC denial. > > Using this function with the least invasive capability as left most > > argument (e.g. CAP_SYS_NICE before CAP_SYS_ADMIN) enables policy writers > > to only allow the least invasive one and SELinux domains pass this check > > with only capability:sys_nice or capability:sys_admin allowed without > > any AVC denial message. > > > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > > > --- > > v3: > > - rename to capable_any() > > - fix typo in function documentation > > - add ns_capable_any() > > v2: > > avoid varargs and fix to two capabilities; capable_or3() can be added > > later if needed > > --- > > include/linux/capability.h | 10 +++++++ > > kernel/capability.c | 53 ++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 63 insertions(+) > > ... > > > diff --git a/kernel/capability.c b/kernel/capability.c > > index 765194f5d678..ab9b889c3f4d 100644 > > --- a/kernel/capability.c > > +++ b/kernel/capability.c > > @@ -435,6 +435,59 @@ bool ns_capable_setid(struct user_namespace *ns, int cap) > > } > > EXPORT_SYMBOL(ns_capable_setid); > > > > +/** > > + * ns_capable_any - Determine if the current task has one of two superior capabilities in effect > > + * @ns: The usernamespace we want the capability in > > + * @cap1: The capabilities to be tested for first > > + * @cap2: The capabilities to be tested for secondly > > + * > > + * Return true if the current task has at least one of the two given superior > > + * capabilities currently available for use, false if not. > > + * > > + * In contrast to or'ing capable() this call will create exactly one audit > > + * message, either for @cap1, if it is granted or both are not permitted, > > + * or @cap2, if it is granted while the other one is not. > > + * > > + * The capabilities should be ordered from least to most invasive, i.e. CAP_SYS_ADMIN last. > > + * > > + * This sets PF_SUPERPRIV on the task if the capability is available on the > > + * assumption that it's about to be used. > > + */ > > +bool ns_capable_any(struct user_namespace *ns, int cap1, int cap2) > > +{ > > + if (ns_capable_noaudit(ns, cap1)) > > + return ns_capable(ns, cap1); > > + > > + if (ns_capable_noaudit(ns, cap2)) > > + return ns_capable(ns, cap2); > > + > > + return ns_capable(ns, cap1); > > I'm slightly concerned that some people are going to be upset about > making an additional call into the capabilities code with this > function. I think we need to be a bit more clever here to take out > some of the extra work. > > I wonder if we create a new capability function, call it > ns_capable_audittrue(...) or something like that, that only generates > an audit record if the current task has the requested capability ... To be clear, when I mean by generating an audit record when true is that the LSMs implementing the security_capable() hook would only call their audit related code when the capability requirement was met, in many cases that will *not* likely generate an audit record, but you get the basic idea I hope. For SELinux this would likely mean modifying cred_has_capability() something like this ... diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 03bca97c8b29..c1b7f0582d16 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -1581,6 +1581,7 @@ static int cred_has_capability(const struct cred *cred, u16 sclass; u32 sid = cred_sid(cred); u32 av = CAP_TO_MASK(cap); + bool audit; int rc; ad.type = LSM_AUDIT_DATA_CAP; @@ -1601,7 +1602,10 @@ static int cred_has_capability(const struct cred *cred, rc = avc_has_perm_noaudit(&selinux_state, sid, sid, sclass, av, 0, &avd); - if (!(opts & CAP_OPT_NOAUDIT)) { + audit = !(opts & CAP_OPT_NOAUDIT); + if (opts & CAP_OPT_AUDITTRUE) + audit = !rc; + if (audit) { int rc2 = avc_audit(&selinux_state, sid, sid, sclass, av, &avd, rc, &ad); if (rc2) [There is likely a cleaner patch than the above, this was just mean as a demonstration] > ... if > the current task does not have the requested capability no audit > record is generated. With this new function I think we could rewrite > ns_capable_any(...) like this: > > bool ns_capable_any(ns, cap1, cap2) > { > if (ns_capable_audittrue(ns, cap1)) > return true; > if (ns_capable_audittrue(ns, cap2)) > return true; > return ns_capable(ns, cap1); > } > > ... we would still have an extra capability check in the failure case, > but that's an error case anyway and not likely to draw much concern. > > Of course this would require some additional work, meaning a new > CAP_OPT_XXX flag (CAP_OPT_AUDITTRUE?), and updates to the individual > LSMs. However, the good news here is that it appears only SELinux and > AppArmor would need modification (the others don't care about > capabilities or audit) and in each case the modification to support > the new CAP_OPT_AUDITTRUE flag look pretty simple. > > Thoughts? > > > +} > > +EXPORT_SYMBOL(ns_capable_any);
diff --git a/include/linux/capability.h b/include/linux/capability.h index 65efb74c3585..7316d5339a6e 100644 --- a/include/linux/capability.h +++ b/include/linux/capability.h @@ -208,7 +208,9 @@ extern bool has_capability_noaudit(struct task_struct *t, int cap); extern bool has_ns_capability_noaudit(struct task_struct *t, struct user_namespace *ns, int cap); extern bool capable(int cap); +extern bool capable_any(int cap1, int cap2); extern bool ns_capable(struct user_namespace *ns, int cap); +extern bool ns_capable_any(struct user_namespace *ns, int cap1, int cap2); extern bool ns_capable_noaudit(struct user_namespace *ns, int cap); extern bool ns_capable_setid(struct user_namespace *ns, int cap); #else @@ -234,10 +236,18 @@ static inline bool capable(int cap) { return true; } +static inline bool capable_any(int cap1, int cap2) +{ + return true; +} static inline bool ns_capable(struct user_namespace *ns, int cap) { return true; } +static inline bool ns_capable_any(struct user_namespace *ns, int cap1, int cap2) +{ + return true; +} static inline bool ns_capable_noaudit(struct user_namespace *ns, int cap) { return true; diff --git a/kernel/capability.c b/kernel/capability.c index 765194f5d678..ab9b889c3f4d 100644 --- a/kernel/capability.c +++ b/kernel/capability.c @@ -435,6 +435,59 @@ bool ns_capable_setid(struct user_namespace *ns, int cap) } EXPORT_SYMBOL(ns_capable_setid); +/** + * ns_capable_any - Determine if the current task has one of two superior capabilities in effect + * @ns: The usernamespace we want the capability in + * @cap1: The capabilities to be tested for first + * @cap2: The capabilities to be tested for secondly + * + * Return true if the current task has at least one of the two given superior + * capabilities currently available for use, false if not. + * + * In contrast to or'ing capable() this call will create exactly one audit + * message, either for @cap1, if it is granted or both are not permitted, + * or @cap2, if it is granted while the other one is not. + * + * The capabilities should be ordered from least to most invasive, i.e. CAP_SYS_ADMIN last. + * + * This sets PF_SUPERPRIV on the task if the capability is available on the + * assumption that it's about to be used. + */ +bool ns_capable_any(struct user_namespace *ns, int cap1, int cap2) +{ + if (ns_capable_noaudit(ns, cap1)) + return ns_capable(ns, cap1); + + if (ns_capable_noaudit(ns, cap2)) + return ns_capable(ns, cap2); + + return ns_capable(ns, cap1); +} +EXPORT_SYMBOL(ns_capable_any); + +/** + * capable_any - Determine if the current task has one of two superior capabilities in effect + * @cap1: The capabilities to be tested for first + * @cap2: The capabilities to be tested for secondly + * + * Return true if the current task has at least one of the two given superior + * capabilities currently available for use, false if not. + * + * In contrast to or'ing capable() this call will create exactly one audit + * message, either for @cap1, if it is granted or both are not permitted, + * or @cap2, if it is granted while the other one is not. + * + * The capabilities should be ordered from least to most invasive, i.e. CAP_SYS_ADMIN last. + * + * This sets PF_SUPERPRIV on the task if the capability is available on the + * assumption that it's about to be used. + */ +bool capable_any(int cap1, int cap2) +{ + return ns_capable_any(&init_user_ns, cap1, cap2); +} +EXPORT_SYMBOL(capable_any); + /** * capable - Determine if the current task has a superior capability in effect * @cap: The capability to be tested for
Add the interfaces `capable_any()` and `ns_capable_any()` as an alternative to multiple `capable()`/`ns_capable()` calls, like `capable_any(CAP_SYS_NICE, CAP_SYS_ADMIN)` instead of `capable(CAP_SYS_NICE) || capable(CAP_SYS_ADMIN)`. `capable_any()`/`ns_capable_any()` will in particular generate exactly one audit message, either for the left most capability in effect or, if the task has none, the first one. This is especially helpful with regard to SELinux, where each audit message about a not allowed capability will create an AVC denial. Using this function with the least invasive capability as left most argument (e.g. CAP_SYS_NICE before CAP_SYS_ADMIN) enables policy writers to only allow the least invasive one and SELinux domains pass this check with only capability:sys_nice or capability:sys_admin allowed without any AVC denial message. Signed-off-by: Christian Göttsche <cgzones@googlemail.com> --- v3: - rename to capable_any() - fix typo in function documentation - add ns_capable_any() v2: avoid varargs and fix to two capabilities; capable_or3() can be added later if needed --- include/linux/capability.h | 10 +++++++ kernel/capability.c | 53 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+)