[RFC,v2] security,capability: pass object information to security_capable
diff mbox series

Message ID 20190801144313.1014-1-acgoide@tycho.nsa.gov
State Superseded
Headers show
Series
  • [RFC,v2] security,capability: pass object information to security_capable
Related show

Commit Message

Aaron Goidel Aug. 1, 2019, 2:43 p.m. UTC
From: Nicholas Franck <nhfran2@tycho.nsa.gov>

At present security_capable does not pass any object information
and therefore can neither audit the particular object nor take it
into account. Augment the security_capable interface to support
passing supplementary data. Use this facility initially to convey
the inode for capability checks relevant to inodes. This only
addresses capable_wrt_inode_uidgid calls; other capability checks
relevant to inodes will be addressed in subsequent changes. In the
future, this will be further extended to pass object information for
other capability checks such as the target task for CAP_KILL.

In SELinux this new information is leveraged here to include the inode
in the audit message. In the future, it could also be used to perform
a per inode capability checks.

It would be possible to fold the existing opts argument into this new
supplementary data structure. This was omitted from this change to
minimize changes.

Signed-off-by: Nicholas Franck <nhfran2@tycho.nsa.gov>
Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov>
---
v2:
- Changed order of audit prints so optional information comes second
---
 include/linux/capability.h             |  7 ++++++
 include/linux/lsm_audit.h              |  5 +++-
 include/linux/lsm_hooks.h              |  3 ++-
 include/linux/security.h               | 23 +++++++++++++-----
 kernel/capability.c                    | 33 ++++++++++++++++++--------
 kernel/seccomp.c                       |  2 +-
 security/apparmor/capability.c         |  8 ++++---
 security/apparmor/include/capability.h |  4 +++-
 security/apparmor/ipc.c                |  2 +-
 security/apparmor/lsm.c                |  5 ++--
 security/apparmor/resource.c           |  2 +-
 security/commoncap.c                   | 11 +++++----
 security/lsm_audit.c                   | 21 ++++++++++++++--
 security/safesetid/lsm.c               |  3 ++-
 security/security.c                    |  5 ++--
 security/selinux/hooks.c               | 20 +++++++++-------
 security/smack/smack_access.c          |  2 +-
 17 files changed, 110 insertions(+), 46 deletions(-)

Comments

Paul Moore Aug. 8, 2019, 4:30 p.m. UTC | #1
On Thu, Aug 1, 2019 at 10:43 AM Aaron Goidel <acgoide@tycho.nsa.gov> wrote:
> From: Nicholas Franck <nhfran2@tycho.nsa.gov>
>
> At present security_capable does not pass any object information
> and therefore can neither audit the particular object nor take it
> into account. Augment the security_capable interface to support
> passing supplementary data. Use this facility initially to convey
> the inode for capability checks relevant to inodes. This only
> addresses capable_wrt_inode_uidgid calls; other capability checks
> relevant to inodes will be addressed in subsequent changes. In the
> future, this will be further extended to pass object information for
> other capability checks such as the target task for CAP_KILL.
>
> In SELinux this new information is leveraged here to include the inode
> in the audit message. In the future, it could also be used to perform
> a per inode capability checks.
>
> It would be possible to fold the existing opts argument into this new
> supplementary data structure. This was omitted from this change to
> minimize changes.
>
> Signed-off-by: Nicholas Franck <nhfran2@tycho.nsa.gov>
> Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov>
> ---
> v2:
> - Changed order of audit prints so optional information comes second
> ---
>  include/linux/capability.h             |  7 ++++++
>  include/linux/lsm_audit.h              |  5 +++-
>  include/linux/lsm_hooks.h              |  3 ++-
>  include/linux/security.h               | 23 +++++++++++++-----
>  kernel/capability.c                    | 33 ++++++++++++++++++--------
>  kernel/seccomp.c                       |  2 +-
>  security/apparmor/capability.c         |  8 ++++---
>  security/apparmor/include/capability.h |  4 +++-
>  security/apparmor/ipc.c                |  2 +-
>  security/apparmor/lsm.c                |  5 ++--
>  security/apparmor/resource.c           |  2 +-
>  security/commoncap.c                   | 11 +++++----
>  security/lsm_audit.c                   | 21 ++++++++++++++--
>  security/safesetid/lsm.c               |  3 ++-
>  security/security.c                    |  5 ++--
>  security/selinux/hooks.c               | 20 +++++++++-------
>  security/smack/smack_access.c          |  2 +-
>  17 files changed, 110 insertions(+), 46 deletions(-)

You should CC the linux-audit list, I've added them on this mail.

I had hoped to see some thought put into the idea of dynamically
emitting the proper audit records as I mentioned in the previous patch
set, but regardless there are some comments on this code as written
...

> diff --git a/security/lsm_audit.c b/security/lsm_audit.c
> index 33028c098ef3..18cc7c956b69 100644
> --- a/security/lsm_audit.c
> +++ b/security/lsm_audit.c
> @@ -229,9 +229,26 @@ static void dump_common_audit_data(struct audit_buffer *ab,
>         case LSM_AUDIT_DATA_IPC:
>                 audit_log_format(ab, " key=%d ", a->u.ipc_id);
>                 break;
> -       case LSM_AUDIT_DATA_CAP:
> -               audit_log_format(ab, " capability=%d ", a->u.cap);
> +       case LSM_AUDIT_DATA_CAP: {
> +               const struct inode *inode;
> +
> +               audit_log_format(ab, " capability=%d ", a->u.cap_struct.cap);
> +               if (a->u.cap_struct.cad) {
> +                       switch (a->u.cap_struct.cad->type) {
> +                       case CAP_AUX_DATA_INODE: {
> +                               inode = a->u.cap_struct.cad->u.inode;
> +
> +                               audit_log_format(ab, " dev=");
> +                               audit_log_untrustedstring(ab,
> +                                       inode->i_sb->s_id);
> +                               audit_log_format(ab, " ino=%lu",
> +                                       inode->i_ino);
> +                               break;
> +                       }

Since you are declaring "inode" further up, there doesn't appear to be
any need for the CAP_AUX_DATA_INODE braces, please remove them.

The general recommended practice when it comes to "sometimes" fields
in an audit record, is to always record them in the record, but use a
value of "?" when there is nothing relevant to record.  For example,
when *not* recording inode information you would do something like the
following:

  audit_log_format(ab, " dev=? ino=?");

> +                       }
> +               }
>                 break;
> +       }
>         case LSM_AUDIT_DATA_PATH: {
>                 struct inode *inode;
>
Aaron Goidel Aug. 13, 2019, 3:01 p.m. UTC | #2
On 8/8/19 12:30 PM, Paul Moore wrote:
> On Thu, Aug 1, 2019 at 10:43 AM Aaron Goidel <acgoide@tycho.nsa.gov> wrote:
>> From: Nicholas Franck <nhfran2@tycho.nsa.gov>
>>
>> At present security_capable does not pass any object information
>> and therefore can neither audit the particular object nor take it
>> into account. Augment the security_capable interface to support
>> passing supplementary data. Use this facility initially to convey
>> the inode for capability checks relevant to inodes. This only
>> addresses capable_wrt_inode_uidgid calls; other capability checks
>> relevant to inodes will be addressed in subsequent changes. In the
>> future, this will be further extended to pass object information for
>> other capability checks such as the target task for CAP_KILL.
>>
>> In SELinux this new information is leveraged here to include the inode
>> in the audit message. In the future, it could also be used to perform
>> a per inode capability checks.
>>
>> It would be possible to fold the existing opts argument into this new
>> supplementary data structure. This was omitted from this change to
>> minimize changes.
>>
>> Signed-off-by: Nicholas Franck <nhfran2@tycho.nsa.gov>
>> Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov>
>> ---
>> v2:
>> - Changed order of audit prints so optional information comes second
>> ---
>>   include/linux/capability.h             |  7 ++++++
>>   include/linux/lsm_audit.h              |  5 +++-
>>   include/linux/lsm_hooks.h              |  3 ++-
>>   include/linux/security.h               | 23 +++++++++++++-----
>>   kernel/capability.c                    | 33 ++++++++++++++++++--------
>>   kernel/seccomp.c                       |  2 +-
>>   security/apparmor/capability.c         |  8 ++++---
>>   security/apparmor/include/capability.h |  4 +++-
>>   security/apparmor/ipc.c                |  2 +-
>>   security/apparmor/lsm.c                |  5 ++--
>>   security/apparmor/resource.c           |  2 +-
>>   security/commoncap.c                   | 11 +++++----
>>   security/lsm_audit.c                   | 21 ++++++++++++++--
>>   security/safesetid/lsm.c               |  3 ++-
>>   security/security.c                    |  5 ++--
>>   security/selinux/hooks.c               | 20 +++++++++-------
>>   security/smack/smack_access.c          |  2 +-
>>   17 files changed, 110 insertions(+), 46 deletions(-)
> 
> You should CC the linux-audit list, I've added them on this mail.
> 
> I had hoped to see some thought put into the idea of dynamically
> emitting the proper audit records as I mentioned in the previous patch
> set, but regardless there are some comments on this code as written
> ...
> 
>> diff --git a/security/lsm_audit.c b/security/lsm_audit.c
>> index 33028c098ef3..18cc7c956b69 100644
>> --- a/security/lsm_audit.c
>> +++ b/security/lsm_audit.c
>> @@ -229,9 +229,26 @@ static void dump_common_audit_data(struct audit_buffer *ab,
>>          case LSM_AUDIT_DATA_IPC:
>>                  audit_log_format(ab, " key=%d ", a->u.ipc_id);
>>                  break;
>> -       case LSM_AUDIT_DATA_CAP:
>> -               audit_log_format(ab, " capability=%d ", a->u.cap);
>> +       case LSM_AUDIT_DATA_CAP: {
>> +               const struct inode *inode;
>> +
>> +               audit_log_format(ab, " capability=%d ", a->u.cap_struct.cap);
>> +               if (a->u.cap_struct.cad) {
>> +                       switch (a->u.cap_struct.cad->type) {
>> +                       case CAP_AUX_DATA_INODE: {
>> +                               inode = a->u.cap_struct.cad->u.inode;
>> +
>> +                               audit_log_format(ab, " dev=");
>> +                               audit_log_untrustedstring(ab,
>> +                                       inode->i_sb->s_id);
>> +                               audit_log_format(ab, " ino=%lu",
>> +                                       inode->i_ino);
>> +                               break;
>> +                       }
> 
> Since you are declaring "inode" further up, there doesn't appear to be
> any need for the CAP_AUX_DATA_INODE braces, please remove them.
> 
> The general recommended practice when it comes to "sometimes" fields
> in an audit record, is to always record them in the record, but use a
> value of "?" when there is nothing relevant to record.  For example,
> when *not* recording inode information you would do something like the
> following:
> 
>    audit_log_format(ab, " dev=? ino=?");
> 
The issue this brings up is what happens when this is expanded to more 
cases? Assuming there will be a case here for logging audit data for 
task based capabilities (CAP_AUX_DATA_TASK), what do we want to have 
happen if we are recording *neither* inode information nor task 
information (say a PID)? If we log something in the inode case, we 
presumably don't want to call audit_log_format(ab, " dev=?, pid=?") as 
well. (And vice versa for when we log a pid and no inode).
>> +                       }
>> +               }
>>                  break;
>> +       }
>>          case LSM_AUDIT_DATA_PATH: {
>>                  struct inode *inode;
>>
>
Richard Guy Briggs Aug. 13, 2019, 9:27 p.m. UTC | #3
On 2019-08-13 11:01, Aaron Goidel wrote:
> On 8/8/19 12:30 PM, Paul Moore wrote:
> > On Thu, Aug 1, 2019 at 10:43 AM Aaron Goidel <acgoide@tycho.nsa.gov> wrote:
> > > From: Nicholas Franck <nhfran2@tycho.nsa.gov>
> > > 
> > > At present security_capable does not pass any object information
> > > and therefore can neither audit the particular object nor take it
> > > into account. Augment the security_capable interface to support
> > > passing supplementary data. Use this facility initially to convey
> > > the inode for capability checks relevant to inodes. This only
> > > addresses capable_wrt_inode_uidgid calls; other capability checks
> > > relevant to inodes will be addressed in subsequent changes. In the
> > > future, this will be further extended to pass object information for
> > > other capability checks such as the target task for CAP_KILL.
> > > 
> > > In SELinux this new information is leveraged here to include the inode
> > > in the audit message. In the future, it could also be used to perform
> > > a per inode capability checks.
> > > 
> > > It would be possible to fold the existing opts argument into this new
> > > supplementary data structure. This was omitted from this change to
> > > minimize changes.
> > > 
> > > Signed-off-by: Nicholas Franck <nhfran2@tycho.nsa.gov>
> > > Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov>
> > > ---
> > > v2:
> > > - Changed order of audit prints so optional information comes second
> > > ---
> > >   include/linux/capability.h             |  7 ++++++
> > >   include/linux/lsm_audit.h              |  5 +++-
> > >   include/linux/lsm_hooks.h              |  3 ++-
> > >   include/linux/security.h               | 23 +++++++++++++-----
> > >   kernel/capability.c                    | 33 ++++++++++++++++++--------
> > >   kernel/seccomp.c                       |  2 +-
> > >   security/apparmor/capability.c         |  8 ++++---
> > >   security/apparmor/include/capability.h |  4 +++-
> > >   security/apparmor/ipc.c                |  2 +-
> > >   security/apparmor/lsm.c                |  5 ++--
> > >   security/apparmor/resource.c           |  2 +-
> > >   security/commoncap.c                   | 11 +++++----
> > >   security/lsm_audit.c                   | 21 ++++++++++++++--
> > >   security/safesetid/lsm.c               |  3 ++-
> > >   security/security.c                    |  5 ++--
> > >   security/selinux/hooks.c               | 20 +++++++++-------
> > >   security/smack/smack_access.c          |  2 +-
> > >   17 files changed, 110 insertions(+), 46 deletions(-)
> > 
> > You should CC the linux-audit list, I've added them on this mail.
> > 
> > I had hoped to see some thought put into the idea of dynamically
> > emitting the proper audit records as I mentioned in the previous patch
> > set, but regardless there are some comments on this code as written
> > ...
> > 
> > > diff --git a/security/lsm_audit.c b/security/lsm_audit.c
> > > index 33028c098ef3..18cc7c956b69 100644
> > > --- a/security/lsm_audit.c
> > > +++ b/security/lsm_audit.c
> > > @@ -229,9 +229,26 @@ static void dump_common_audit_data(struct audit_buffer *ab,
> > >          case LSM_AUDIT_DATA_IPC:
> > >                  audit_log_format(ab, " key=%d ", a->u.ipc_id);
> > >                  break;
> > > -       case LSM_AUDIT_DATA_CAP:
> > > -               audit_log_format(ab, " capability=%d ", a->u.cap);
> > > +       case LSM_AUDIT_DATA_CAP: {
> > > +               const struct inode *inode;
> > > +
> > > +               audit_log_format(ab, " capability=%d ", a->u.cap_struct.cap);
> > > +               if (a->u.cap_struct.cad) {
> > > +                       switch (a->u.cap_struct.cad->type) {
> > > +                       case CAP_AUX_DATA_INODE: {
> > > +                               inode = a->u.cap_struct.cad->u.inode;
> > > +
> > > +                               audit_log_format(ab, " dev=");
> > > +                               audit_log_untrustedstring(ab,
> > > +                                       inode->i_sb->s_id);
> > > +                               audit_log_format(ab, " ino=%lu",
> > > +                                       inode->i_ino);
> > > +                               break;
> > > +                       }
> > 
> > Since you are declaring "inode" further up, there doesn't appear to be
> > any need for the CAP_AUX_DATA_INODE braces, please remove them.
> > 
> > The general recommended practice when it comes to "sometimes" fields
> > in an audit record, is to always record them in the record, but use a
> > value of "?" when there is nothing relevant to record.  For example,
> > when *not* recording inode information you would do something like the
> > following:
> > 
> >    audit_log_format(ab, " dev=? ino=?");
> > 
> The issue this brings up is what happens when this is expanded to more
> cases? Assuming there will be a case here for logging audit data for task
> based capabilities (CAP_AUX_DATA_TASK), what do we want to have happen if we
> are recording *neither* inode information nor task information (say a PID)?
> If we log something in the inode case, we presumably don't want to call
> audit_log_format(ab, " dev=?, pid=?") as well. (And vice versa for when we
> log a pid and no inode).

Yup.  This record is already a mess due to that.

The general solution is to either use a new record type for each
variant, or to use an auxiliary record for each additional piece of
information.  The long term solution that has been suggested it to move
away from a text message format.

> > > +                       }
> > > +               }
> > >                  break;
> > > +       }
> > >          case LSM_AUDIT_DATA_PATH: {
> > >                  struct inode *inode;
> 
> Aaron

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
Paul Moore Aug. 14, 2019, 7:59 p.m. UTC | #4
On Tue, Aug 13, 2019 at 5:27 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2019-08-13 11:01, Aaron Goidel wrote:
> > On 8/8/19 12:30 PM, Paul Moore wrote:
> > > On Thu, Aug 1, 2019 at 10:43 AM Aaron Goidel <acgoide@tycho.nsa.gov> wrote:
> > > > From: Nicholas Franck <nhfran2@tycho.nsa.gov>
> > > >
> > > > At present security_capable does not pass any object information
> > > > and therefore can neither audit the particular object nor take it
> > > > into account. Augment the security_capable interface to support
> > > > passing supplementary data. Use this facility initially to convey
> > > > the inode for capability checks relevant to inodes. This only
> > > > addresses capable_wrt_inode_uidgid calls; other capability checks
> > > > relevant to inodes will be addressed in subsequent changes. In the
> > > > future, this will be further extended to pass object information for
> > > > other capability checks such as the target task for CAP_KILL.
> > > >
> > > > In SELinux this new information is leveraged here to include the inode
> > > > in the audit message. In the future, it could also be used to perform
> > > > a per inode capability checks.
> > > >
> > > > It would be possible to fold the existing opts argument into this new
> > > > supplementary data structure. This was omitted from this change to
> > > > minimize changes.
> > > >
> > > > Signed-off-by: Nicholas Franck <nhfran2@tycho.nsa.gov>
> > > > Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov>
> > > > ---
> > > > v2:
> > > > - Changed order of audit prints so optional information comes second
> > > > ---
> > > >   include/linux/capability.h             |  7 ++++++
> > > >   include/linux/lsm_audit.h              |  5 +++-
> > > >   include/linux/lsm_hooks.h              |  3 ++-
> > > >   include/linux/security.h               | 23 +++++++++++++-----
> > > >   kernel/capability.c                    | 33 ++++++++++++++++++--------
> > > >   kernel/seccomp.c                       |  2 +-
> > > >   security/apparmor/capability.c         |  8 ++++---
> > > >   security/apparmor/include/capability.h |  4 +++-
> > > >   security/apparmor/ipc.c                |  2 +-
> > > >   security/apparmor/lsm.c                |  5 ++--
> > > >   security/apparmor/resource.c           |  2 +-
> > > >   security/commoncap.c                   | 11 +++++----
> > > >   security/lsm_audit.c                   | 21 ++++++++++++++--
> > > >   security/safesetid/lsm.c               |  3 ++-
> > > >   security/security.c                    |  5 ++--
> > > >   security/selinux/hooks.c               | 20 +++++++++-------
> > > >   security/smack/smack_access.c          |  2 +-
> > > >   17 files changed, 110 insertions(+), 46 deletions(-)
> > >
> > > You should CC the linux-audit list, I've added them on this mail.
> > >
> > > I had hoped to see some thought put into the idea of dynamically
> > > emitting the proper audit records as I mentioned in the previous patch
> > > set, but regardless there are some comments on this code as written
> > > ...
> > >
> > > > diff --git a/security/lsm_audit.c b/security/lsm_audit.c
> > > > index 33028c098ef3..18cc7c956b69 100644
> > > > --- a/security/lsm_audit.c
> > > > +++ b/security/lsm_audit.c
> > > > @@ -229,9 +229,26 @@ static void dump_common_audit_data(struct audit_buffer *ab,
> > > >          case LSM_AUDIT_DATA_IPC:
> > > >                  audit_log_format(ab, " key=%d ", a->u.ipc_id);
> > > >                  break;
> > > > -       case LSM_AUDIT_DATA_CAP:
> > > > -               audit_log_format(ab, " capability=%d ", a->u.cap);
> > > > +       case LSM_AUDIT_DATA_CAP: {
> > > > +               const struct inode *inode;
> > > > +
> > > > +               audit_log_format(ab, " capability=%d ", a->u.cap_struct.cap);
> > > > +               if (a->u.cap_struct.cad) {
> > > > +                       switch (a->u.cap_struct.cad->type) {
> > > > +                       case CAP_AUX_DATA_INODE: {
> > > > +                               inode = a->u.cap_struct.cad->u.inode;
> > > > +
> > > > +                               audit_log_format(ab, " dev=");
> > > > +                               audit_log_untrustedstring(ab,
> > > > +                                       inode->i_sb->s_id);
> > > > +                               audit_log_format(ab, " ino=%lu",
> > > > +                                       inode->i_ino);
> > > > +                               break;
> > > > +                       }
> > >
> > > Since you are declaring "inode" further up, there doesn't appear to be
> > > any need for the CAP_AUX_DATA_INODE braces, please remove them.
> > >
> > > The general recommended practice when it comes to "sometimes" fields
> > > in an audit record, is to always record them in the record, but use a
> > > value of "?" when there is nothing relevant to record.  For example,
> > > when *not* recording inode information you would do something like the
> > > following:
> > >
> > >    audit_log_format(ab, " dev=? ino=?");
> > >
> > The issue this brings up is what happens when this is expanded to more
> > cases? Assuming there will be a case here for logging audit data for task
> > based capabilities (CAP_AUX_DATA_TASK), what do we want to have happen if we
> > are recording *neither* inode information nor task information (say a PID)?
> > If we log something in the inode case, we presumably don't want to call
> > audit_log_format(ab, " dev=?, pid=?") as well. (And vice versa for when we
> > log a pid and no inode).
>
> Yup.  This record is already a mess due to that.
>
> The general solution is to either use a new record type for each
> variant, or to use an auxiliary record for each additional piece of
> information.  The long term solution that has been suggested it to move
> away from a text message format.

This is why I was suggesting that Aaron look into allowing the LSMs to
request generation of audit records in the earlier thread (and
mentioned it again at the top of my comments in this thread).
Stephen Smalley Aug. 14, 2019, 9:08 p.m. UTC | #5
On 8/14/19 3:59 PM, Paul Moore wrote:
> On Tue, Aug 13, 2019 at 5:27 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>> On 2019-08-13 11:01, Aaron Goidel wrote:
>>> On 8/8/19 12:30 PM, Paul Moore wrote:
>>>> On Thu, Aug 1, 2019 at 10:43 AM Aaron Goidel <acgoide@tycho.nsa.gov> wrote:
>>>>> From: Nicholas Franck <nhfran2@tycho.nsa.gov>
>>>>>
>>>>> At present security_capable does not pass any object information
>>>>> and therefore can neither audit the particular object nor take it
>>>>> into account. Augment the security_capable interface to support
>>>>> passing supplementary data. Use this facility initially to convey
>>>>> the inode for capability checks relevant to inodes. This only
>>>>> addresses capable_wrt_inode_uidgid calls; other capability checks
>>>>> relevant to inodes will be addressed in subsequent changes. In the
>>>>> future, this will be further extended to pass object information for
>>>>> other capability checks such as the target task for CAP_KILL.
>>>>>
>>>>> In SELinux this new information is leveraged here to include the inode
>>>>> in the audit message. In the future, it could also be used to perform
>>>>> a per inode capability checks.
>>>>>
>>>>> It would be possible to fold the existing opts argument into this new
>>>>> supplementary data structure. This was omitted from this change to
>>>>> minimize changes.
>>>>>
>>>>> Signed-off-by: Nicholas Franck <nhfran2@tycho.nsa.gov>
>>>>> Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov>
>>>>> ---
>>>>> v2:
>>>>> - Changed order of audit prints so optional information comes second
>>>>> ---
>>>>>    include/linux/capability.h             |  7 ++++++
>>>>>    include/linux/lsm_audit.h              |  5 +++-
>>>>>    include/linux/lsm_hooks.h              |  3 ++-
>>>>>    include/linux/security.h               | 23 +++++++++++++-----
>>>>>    kernel/capability.c                    | 33 ++++++++++++++++++--------
>>>>>    kernel/seccomp.c                       |  2 +-
>>>>>    security/apparmor/capability.c         |  8 ++++---
>>>>>    security/apparmor/include/capability.h |  4 +++-
>>>>>    security/apparmor/ipc.c                |  2 +-
>>>>>    security/apparmor/lsm.c                |  5 ++--
>>>>>    security/apparmor/resource.c           |  2 +-
>>>>>    security/commoncap.c                   | 11 +++++----
>>>>>    security/lsm_audit.c                   | 21 ++++++++++++++--
>>>>>    security/safesetid/lsm.c               |  3 ++-
>>>>>    security/security.c                    |  5 ++--
>>>>>    security/selinux/hooks.c               | 20 +++++++++-------
>>>>>    security/smack/smack_access.c          |  2 +-
>>>>>    17 files changed, 110 insertions(+), 46 deletions(-)
>>>>
>>>> You should CC the linux-audit list, I've added them on this mail.
>>>>
>>>> I had hoped to see some thought put into the idea of dynamically
>>>> emitting the proper audit records as I mentioned in the previous patch
>>>> set, but regardless there are some comments on this code as written
>>>> ...
>>>>
>>>>> diff --git a/security/lsm_audit.c b/security/lsm_audit.c
>>>>> index 33028c098ef3..18cc7c956b69 100644
>>>>> --- a/security/lsm_audit.c
>>>>> +++ b/security/lsm_audit.c
>>>>> @@ -229,9 +229,26 @@ static void dump_common_audit_data(struct audit_buffer *ab,
>>>>>           case LSM_AUDIT_DATA_IPC:
>>>>>                   audit_log_format(ab, " key=%d ", a->u.ipc_id);
>>>>>                   break;
>>>>> -       case LSM_AUDIT_DATA_CAP:
>>>>> -               audit_log_format(ab, " capability=%d ", a->u.cap);
>>>>> +       case LSM_AUDIT_DATA_CAP: {
>>>>> +               const struct inode *inode;
>>>>> +
>>>>> +               audit_log_format(ab, " capability=%d ", a->u.cap_struct.cap);
>>>>> +               if (a->u.cap_struct.cad) {
>>>>> +                       switch (a->u.cap_struct.cad->type) {
>>>>> +                       case CAP_AUX_DATA_INODE: {
>>>>> +                               inode = a->u.cap_struct.cad->u.inode;
>>>>> +
>>>>> +                               audit_log_format(ab, " dev=");
>>>>> +                               audit_log_untrustedstring(ab,
>>>>> +                                       inode->i_sb->s_id);
>>>>> +                               audit_log_format(ab, " ino=%lu",
>>>>> +                                       inode->i_ino);
>>>>> +                               break;
>>>>> +                       }
>>>>
>>>> Since you are declaring "inode" further up, there doesn't appear to be
>>>> any need for the CAP_AUX_DATA_INODE braces, please remove them.
>>>>
>>>> The general recommended practice when it comes to "sometimes" fields
>>>> in an audit record, is to always record them in the record, but use a
>>>> value of "?" when there is nothing relevant to record.  For example,
>>>> when *not* recording inode information you would do something like the
>>>> following:
>>>>
>>>>     audit_log_format(ab, " dev=? ino=?");
>>>>
>>> The issue this brings up is what happens when this is expanded to more
>>> cases? Assuming there will be a case here for logging audit data for task
>>> based capabilities (CAP_AUX_DATA_TASK), what do we want to have happen if we
>>> are recording *neither* inode information nor task information (say a PID)?
>>> If we log something in the inode case, we presumably don't want to call
>>> audit_log_format(ab, " dev=?, pid=?") as well. (And vice versa for when we
>>> log a pid and no inode).
>>
>> Yup.  This record is already a mess due to that.
>>
>> The general solution is to either use a new record type for each
>> variant, or to use an auxiliary record for each additional piece of
>> information.  The long term solution that has been suggested it to move
>> away from a text message format.
> 
> This is why I was suggesting that Aaron look into allowing the LSMs to
> request generation of audit records in the earlier thread (and
> mentioned it again at the top of my comments in this thread).

How would that work? The behavior we want is to capture some information 
about the inode whenever there is a capability denial upon an attempted 
access to that inode.  Allowing the LSM to enable audit collection on a 
per-process basis doesn't appear to help with that goal, because this is 
something we want for all processes.  Further, we don't really want the 
rest of the audit collection machinery engaged here; we just want the 
inode information for this specific inode, and we have the inode readily 
accessible in the caller of the LSM hook already, so we don't need to do 
it earlier.

Further, in the future we want to be able to take the inode security 
label into consideration as part of the capability checking, which is 
independent of audit entirely.  So the rest of the patch will still be 
required even if the audit solution ends up being different.
Paul Moore Aug. 14, 2019, 9:27 p.m. UTC | #6
On Wed, Aug 14, 2019 at 5:08 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 8/14/19 3:59 PM, Paul Moore wrote:
> > On Tue, Aug 13, 2019 at 5:27 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> >> On 2019-08-13 11:01, Aaron Goidel wrote:
> >>> On 8/8/19 12:30 PM, Paul Moore wrote:
> >>>> On Thu, Aug 1, 2019 at 10:43 AM Aaron Goidel <acgoide@tycho.nsa.gov> wrote:
> >>>>> From: Nicholas Franck <nhfran2@tycho.nsa.gov>
> >>>>>
> >>>>> At present security_capable does not pass any object information
> >>>>> and therefore can neither audit the particular object nor take it
> >>>>> into account. Augment the security_capable interface to support
> >>>>> passing supplementary data. Use this facility initially to convey
> >>>>> the inode for capability checks relevant to inodes. This only
> >>>>> addresses capable_wrt_inode_uidgid calls; other capability checks
> >>>>> relevant to inodes will be addressed in subsequent changes. In the
> >>>>> future, this will be further extended to pass object information for
> >>>>> other capability checks such as the target task for CAP_KILL.
> >>>>>
> >>>>> In SELinux this new information is leveraged here to include the inode
> >>>>> in the audit message. In the future, it could also be used to perform
> >>>>> a per inode capability checks.
> >>>>>
> >>>>> It would be possible to fold the existing opts argument into this new
> >>>>> supplementary data structure. This was omitted from this change to
> >>>>> minimize changes.
> >>>>>
> >>>>> Signed-off-by: Nicholas Franck <nhfran2@tycho.nsa.gov>
> >>>>> Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov>
> >>>>> ---
> >>>>> v2:
> >>>>> - Changed order of audit prints so optional information comes second
> >>>>> ---
> >>>>>    include/linux/capability.h             |  7 ++++++
> >>>>>    include/linux/lsm_audit.h              |  5 +++-
> >>>>>    include/linux/lsm_hooks.h              |  3 ++-
> >>>>>    include/linux/security.h               | 23 +++++++++++++-----
> >>>>>    kernel/capability.c                    | 33 ++++++++++++++++++--------
> >>>>>    kernel/seccomp.c                       |  2 +-
> >>>>>    security/apparmor/capability.c         |  8 ++++---
> >>>>>    security/apparmor/include/capability.h |  4 +++-
> >>>>>    security/apparmor/ipc.c                |  2 +-
> >>>>>    security/apparmor/lsm.c                |  5 ++--
> >>>>>    security/apparmor/resource.c           |  2 +-
> >>>>>    security/commoncap.c                   | 11 +++++----
> >>>>>    security/lsm_audit.c                   | 21 ++++++++++++++--
> >>>>>    security/safesetid/lsm.c               |  3 ++-
> >>>>>    security/security.c                    |  5 ++--
> >>>>>    security/selinux/hooks.c               | 20 +++++++++-------
> >>>>>    security/smack/smack_access.c          |  2 +-
> >>>>>    17 files changed, 110 insertions(+), 46 deletions(-)
> >>>>
> >>>> You should CC the linux-audit list, I've added them on this mail.
> >>>>
> >>>> I had hoped to see some thought put into the idea of dynamically
> >>>> emitting the proper audit records as I mentioned in the previous patch
> >>>> set, but regardless there are some comments on this code as written
> >>>> ...
> >>>>
> >>>>> diff --git a/security/lsm_audit.c b/security/lsm_audit.c
> >>>>> index 33028c098ef3..18cc7c956b69 100644
> >>>>> --- a/security/lsm_audit.c
> >>>>> +++ b/security/lsm_audit.c
> >>>>> @@ -229,9 +229,26 @@ static void dump_common_audit_data(struct audit_buffer *ab,
> >>>>>           case LSM_AUDIT_DATA_IPC:
> >>>>>                   audit_log_format(ab, " key=%d ", a->u.ipc_id);
> >>>>>                   break;
> >>>>> -       case LSM_AUDIT_DATA_CAP:
> >>>>> -               audit_log_format(ab, " capability=%d ", a->u.cap);
> >>>>> +       case LSM_AUDIT_DATA_CAP: {
> >>>>> +               const struct inode *inode;
> >>>>> +
> >>>>> +               audit_log_format(ab, " capability=%d ", a->u.cap_struct.cap);
> >>>>> +               if (a->u.cap_struct.cad) {
> >>>>> +                       switch (a->u.cap_struct.cad->type) {
> >>>>> +                       case CAP_AUX_DATA_INODE: {
> >>>>> +                               inode = a->u.cap_struct.cad->u.inode;
> >>>>> +
> >>>>> +                               audit_log_format(ab, " dev=");
> >>>>> +                               audit_log_untrustedstring(ab,
> >>>>> +                                       inode->i_sb->s_id);
> >>>>> +                               audit_log_format(ab, " ino=%lu",
> >>>>> +                                       inode->i_ino);
> >>>>> +                               break;
> >>>>> +                       }
> >>>>
> >>>> Since you are declaring "inode" further up, there doesn't appear to be
> >>>> any need for the CAP_AUX_DATA_INODE braces, please remove them.
> >>>>
> >>>> The general recommended practice when it comes to "sometimes" fields
> >>>> in an audit record, is to always record them in the record, but use a
> >>>> value of "?" when there is nothing relevant to record.  For example,
> >>>> when *not* recording inode information you would do something like the
> >>>> following:
> >>>>
> >>>>     audit_log_format(ab, " dev=? ino=?");
> >>>>
> >>> The issue this brings up is what happens when this is expanded to more
> >>> cases? Assuming there will be a case here for logging audit data for task
> >>> based capabilities (CAP_AUX_DATA_TASK), what do we want to have happen if we
> >>> are recording *neither* inode information nor task information (say a PID)?
> >>> If we log something in the inode case, we presumably don't want to call
> >>> audit_log_format(ab, " dev=?, pid=?") as well. (And vice versa for when we
> >>> log a pid and no inode).
> >>
> >> Yup.  This record is already a mess due to that.
> >>
> >> The general solution is to either use a new record type for each
> >> variant, or to use an auxiliary record for each additional piece of
> >> information.  The long term solution that has been suggested it to move
> >> away from a text message format.
> >
> > This is why I was suggesting that Aaron look into allowing the LSMs to
> > request generation of audit records in the earlier thread (and
> > mentioned it again at the top of my comments in this thread).
>
> How would that work? The behavior we want is to capture some information
> about the inode whenever there is a capability denial upon an attempted
> access to that inode.  Allowing the LSM to enable audit collection on a
> per-process basis doesn't appear to help with that goal, because this is
> something we want for all processes.  Further, we don't really want the
> rest of the audit collection machinery engaged here ...

I read this as "we want to audit this information, but we don't to
turn on the very thing which is designed to do this".  At some point,
if you want to audit things, you actually need to turn on auditing.

> ... we just want the
> inode information for this specific inode, and we have the inode readily
> accessible in the caller of the LSM hook already, so we don't need to do
> it earlier.

Aaron appeared to be complaining that if we stuck to the current best
practices regarding record formatting for the LSM generated audit
record, the record was going to get complicated when people started
adding additional information.  FWIW, I don't disagree.  The only real
alternative I've seen thus far is to look into having the LSM enable
certain records, if anyone has another idea I'm all ears/eyes.

> Further, in the future we want to be able to take the inode security
> label into consideration as part of the capability checking, which is
> independent of audit entirely.  So the rest of the patch will still be
> required even if the audit solution ends up being different.

That's a different topic, I don't think there are any remaining
objections to passing the inode information here.
Aaron Goidel Aug. 15, 2019, 1:10 p.m. UTC | #7
On 8/14/19 5:27 PM, Paul Moore wrote:
> On Wed, Aug 14, 2019 at 5:08 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On 8/14/19 3:59 PM, Paul Moore wrote:
>>> On Tue, Aug 13, 2019 at 5:27 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>>>> On 2019-08-13 11:01, Aaron Goidel wrote:
>>>>> On 8/8/19 12:30 PM, Paul Moore wrote:
>>>>>> On Thu, Aug 1, 2019 at 10:43 AM Aaron Goidel <acgoide@tycho.nsa.gov> wrote:
>>>>>>> From: Nicholas Franck <nhfran2@tycho.nsa.gov>
>>>>>>>
>>>>>>> At present security_capable does not pass any object information
>>>>>>> and therefore can neither audit the particular object nor take it
>>>>>>> into account. Augment the security_capable interface to support
>>>>>>> passing supplementary data. Use this facility initially to convey
>>>>>>> the inode for capability checks relevant to inodes. This only
>>>>>>> addresses capable_wrt_inode_uidgid calls; other capability checks
>>>>>>> relevant to inodes will be addressed in subsequent changes. In the
>>>>>>> future, this will be further extended to pass object information for
>>>>>>> other capability checks such as the target task for CAP_KILL.
>>>>>>>
>>>>>>> In SELinux this new information is leveraged here to include the inode
>>>>>>> in the audit message. In the future, it could also be used to perform
>>>>>>> a per inode capability checks.
>>>>>>>
>>>>>>> It would be possible to fold the existing opts argument into this new
>>>>>>> supplementary data structure. This was omitted from this change to
>>>>>>> minimize changes.
>>>>>>>
>>>>>>> Signed-off-by: Nicholas Franck <nhfran2@tycho.nsa.gov>
>>>>>>> Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov>
>>>>>>> ---
>>>>>>> v2:
>>>>>>> - Changed order of audit prints so optional information comes second
>>>>>>> ---
>>>>>>>     include/linux/capability.h             |  7 ++++++
>>>>>>>     include/linux/lsm_audit.h              |  5 +++-
>>>>>>>     include/linux/lsm_hooks.h              |  3 ++-
>>>>>>>     include/linux/security.h               | 23 +++++++++++++-----
>>>>>>>     kernel/capability.c                    | 33 ++++++++++++++++++--------
>>>>>>>     kernel/seccomp.c                       |  2 +-
>>>>>>>     security/apparmor/capability.c         |  8 ++++---
>>>>>>>     security/apparmor/include/capability.h |  4 +++-
>>>>>>>     security/apparmor/ipc.c                |  2 +-
>>>>>>>     security/apparmor/lsm.c                |  5 ++--
>>>>>>>     security/apparmor/resource.c           |  2 +-
>>>>>>>     security/commoncap.c                   | 11 +++++----
>>>>>>>     security/lsm_audit.c                   | 21 ++++++++++++++--
>>>>>>>     security/safesetid/lsm.c               |  3 ++-
>>>>>>>     security/security.c                    |  5 ++--
>>>>>>>     security/selinux/hooks.c               | 20 +++++++++-------
>>>>>>>     security/smack/smack_access.c          |  2 +-
>>>>>>>     17 files changed, 110 insertions(+), 46 deletions(-)
>>>>>>
>>>>>> You should CC the linux-audit list, I've added them on this mail.
>>>>>>
>>>>>> I had hoped to see some thought put into the idea of dynamically
>>>>>> emitting the proper audit records as I mentioned in the previous patch
>>>>>> set, but regardless there are some comments on this code as written
>>>>>> ...
>>>>>>
>>>>>>> diff --git a/security/lsm_audit.c b/security/lsm_audit.c
>>>>>>> index 33028c098ef3..18cc7c956b69 100644
>>>>>>> --- a/security/lsm_audit.c
>>>>>>> +++ b/security/lsm_audit.c
>>>>>>> @@ -229,9 +229,26 @@ static void dump_common_audit_data(struct audit_buffer *ab,
>>>>>>>            case LSM_AUDIT_DATA_IPC:
>>>>>>>                    audit_log_format(ab, " key=%d ", a->u.ipc_id);
>>>>>>>                    break;
>>>>>>> -       case LSM_AUDIT_DATA_CAP:
>>>>>>> -               audit_log_format(ab, " capability=%d ", a->u.cap);
>>>>>>> +       case LSM_AUDIT_DATA_CAP: {
>>>>>>> +               const struct inode *inode;
>>>>>>> +
>>>>>>> +               audit_log_format(ab, " capability=%d ", a->u.cap_struct.cap);
>>>>>>> +               if (a->u.cap_struct.cad) {
>>>>>>> +                       switch (a->u.cap_struct.cad->type) {
>>>>>>> +                       case CAP_AUX_DATA_INODE: {
>>>>>>> +                               inode = a->u.cap_struct.cad->u.inode;
>>>>>>> +
>>>>>>> +                               audit_log_format(ab, " dev=");
>>>>>>> +                               audit_log_untrustedstring(ab,
>>>>>>> +                                       inode->i_sb->s_id);
>>>>>>> +                               audit_log_format(ab, " ino=%lu",
>>>>>>> +                                       inode->i_ino);
>>>>>>> +                               break;
>>>>>>> +                       }
>>>>>>
>>>>>> Since you are declaring "inode" further up, there doesn't appear to be
>>>>>> any need for the CAP_AUX_DATA_INODE braces, please remove them.
>>>>>>
>>>>>> The general recommended practice when it comes to "sometimes" fields
>>>>>> in an audit record, is to always record them in the record, but use a
>>>>>> value of "?" when there is nothing relevant to record.  For example,
>>>>>> when *not* recording inode information you would do something like the
>>>>>> following:
>>>>>>
>>>>>>      audit_log_format(ab, " dev=? ino=?");
>>>>>>
>>>>> The issue this brings up is what happens when this is expanded to more
>>>>> cases? Assuming there will be a case here for logging audit data for task
>>>>> based capabilities (CAP_AUX_DATA_TASK), what do we want to have happen if we
>>>>> are recording *neither* inode information nor task information (say a PID)?
>>>>> If we log something in the inode case, we presumably don't want to call
>>>>> audit_log_format(ab, " dev=?, pid=?") as well. (And vice versa for when we
>>>>> log a pid and no inode).
>>>>
>>>> Yup.  This record is already a mess due to that.
>>>>
>>>> The general solution is to either use a new record type for each
>>>> variant, or to use an auxiliary record for each additional piece of
>>>> information.  The long term solution that has been suggested it to move
>>>> away from a text message format.
>>>
>>> This is why I was suggesting that Aaron look into allowing the LSMs to
>>> request generation of audit records in the earlier thread (and
>>> mentioned it again at the top of my comments in this thread).
>>
>> How would that work? The behavior we want is to capture some information
>> about the inode whenever there is a capability denial upon an attempted
>> access to that inode.  Allowing the LSM to enable audit collection on a
>> per-process basis doesn't appear to help with that goal, because this is
>> something we want for all processes.  Further, we don't really want the
>> rest of the audit collection machinery engaged here ...
> 
> I read this as "we want to audit this information, but we don't to
> turn on the very thing which is designed to do this".  At some point,
> if you want to audit things, you actually need to turn on auditing.
> 
>> ... we just want the
>> inode information for this specific inode, and we have the inode readily
>> accessible in the caller of the LSM hook already, so we don't need to do
>> it earlier.
> 
> Aaron appeared to be complaining that if we stuck to the current best
> practices regarding record formatting for the LSM generated audit
> record, the record was going to get complicated when people started
> adding additional information.  FWIW, I don't disagree.  The only real
> alternative I've seen thus far is to look into having the LSM enable
> certain records, if anyone has another idea I'm all ears/eyes.
> 
>> Further, in the future we want to be able to take the inode security
>> label into consideration as part of the capability checking, which is
>> independent of audit entirely.  So the rest of the patch will still be
>> required even if the audit solution ends up being different.
> 
> That's a different topic, I don't think there are any remaining
> objections to passing the inode information here.
> 

I'm looking at how to enable LSMs to selectively turn on audit 
collection. So there seems to be two key points: audit_alloc() and 
__audit_syscall_entry(). Would it suffice to define a single boolean 
hook that takes the task and call it from both functions, to decide 
whether to override an AUDIT_DISABLED state in audit_alloc() and to 
override a 0 audit_n_rules in __audit_syscall_entry(). In audit_alloc() 
if audit_filter_task() returned AUDIT_DISABLED and the hook returned 
true, we would change the state to AUDIT_BUILD_CONTEXT. In 
__audit_syscall_entry(), if the hook returned true, we would set dummy 
to 0. Obviously, we could have a more general hook which lets us return 
arbitrary audit states, but, it isn't clear how we would reconcile 
conflicting results from audit_filter_task() and the hook for any 
situation other than AUDIT_DISABLED. We could also potentially use a 
different hook in __audit_syscall_entry(), though I don't think that we 
want the LSMs trying to interpret the syscall number or arguments.

Do you think that is sufficiently general or would you suggest something 
different?
Paul Moore Aug. 16, 2019, 4:29 p.m. UTC | #8
On Thu, Aug 15, 2019 at 9:11 AM Aaron Goidel <acgoide@tycho.nsa.gov> wrote:
> I'm looking at how to enable LSMs to selectively turn on audit
> collection. So there seems to be two key points: audit_alloc() and
> __audit_syscall_entry(). Would it suffice to define a single boolean
> hook that takes the task and call it from both functions, to decide
> whether to override an AUDIT_DISABLED state in audit_alloc() and to
> override a 0 audit_n_rules in __audit_syscall_entry(). In audit_alloc()
> if audit_filter_task() returned AUDIT_DISABLED and the hook returned
> true, we would change the state to AUDIT_BUILD_CONTEXT. In
> __audit_syscall_entry(), if the hook returned true, we would set dummy
> to 0. Obviously, we could have a more general hook which lets us return
> arbitrary audit states, but, it isn't clear how we would reconcile
> conflicting results from audit_filter_task() and the hook for any
> situation other than AUDIT_DISABLED. We could also potentially use a
> different hook in __audit_syscall_entry(), though I don't think that we
> want the LSMs trying to interpret the syscall number or arguments.
>
> Do you think that is sufficiently general or would you suggest something
> different?

FWIW, I think treating the per-task audit switch as a boolean is fine;
I don't think we want other in-kernel callers to have to worry about
the different audit states.  From their perspective it is either "on"
or "off".

However, I think there are two parts of the greater LSM-enables-audit
discussion, and we're only discussing the first part: collection.  The
second part is the actual audit record generation, and I think this
part is going to be less clear.  While the changes to audit_alloc(),
etc. are necessary to be able to do any meaningful audit later on, I'm
thinking introducing some granularity and LSM control to what gets
generated in audit_log_exit() might be very welcome both from a
performance and log cleanliness perspective.

Some random thoughts on this (some may be way off, but I want to start
with some expectations):
* The LSM should never be able to block collection/generation of audit
records, just enable additional records.
* The LSM controls should only affect what we call the "syscall
auditing" bits, e.g. the stuff in auditsc.c.  Audit records that
happen outside of this should be untouched, the AVC records are an
example of a record that exists independent of syscall auditing.
* We should be able to have the LSM set a per-syscall audit enable
flag which would be checked in audit_log_exit() (or
audit_free()/__audit_syscall_exit()).
* It's not clear to me if we want to provide some granularity to the
LSM regarding what records get generated in audit_log_exit(), for
example do we allow the LSM to request just PATH records?  I'm
guessing we wouldn't want to specify record types directly, but
perhaps record "classes", e.g. "file".

I'm not sure if any of this is going to be a good idea, but I think we
need to discuss it a bit before we start duplicating things in
lsm_audit.c.

Patch
diff mbox series

diff --git a/include/linux/capability.h b/include/linux/capability.h
index ecce0f43c73a..f72de64c179d 100644
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -211,6 +211,8 @@  extern bool capable(int cap);
 extern bool ns_capable(struct user_namespace *ns, int cap);
 extern bool ns_capable_noaudit(struct user_namespace *ns, int cap);
 extern bool ns_capable_setid(struct user_namespace *ns, int cap);
+extern bool ns_capable_inode(struct user_namespace *ns, int cap,
+			     const struct inode *inode);
 #else
 static inline bool has_capability(struct task_struct *t, int cap)
 {
@@ -246,6 +248,11 @@  static inline bool ns_capable_setid(struct user_namespace *ns, int cap)
 {
 	return true;
 }
+static bool ns_capable_inode(struct user_namespace *ns, int cap,
+			     const struct inode *inode)
+{
+	return true;
+}
 #endif /* CONFIG_MULTIUSER */
 extern bool privileged_wrt_inode_uidgid(struct user_namespace *ns, const struct inode *inode);
 extern bool capable_wrt_inode_uidgid(const struct inode *inode, int cap);
diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h
index 915330abf6e5..15d2a62639f0 100644
--- a/include/linux/lsm_audit.h
+++ b/include/linux/lsm_audit.h
@@ -79,7 +79,10 @@  struct common_audit_data {
 		struct dentry *dentry;
 		struct inode *inode;
 		struct lsm_network_audit *net;
-		int cap;
+		struct  {
+			int cap;
+			struct cap_aux_data *cad;
+		} cap_struct;
 		int ipc_id;
 		struct task_struct *tsk;
 #ifdef CONFIG_KEYS
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index ead98af9c602..3270b8af3498 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1472,7 +1472,8 @@  union security_list_options {
 	int (*capable)(const struct cred *cred,
 			struct user_namespace *ns,
 			int cap,
-			unsigned int opts);
+			unsigned int opts,
+			struct cap_aux_data *cad);
 	int (*quotactl)(int cmds, int type, int id, struct super_block *sb);
 	int (*quota_on)(struct dentry *dentry);
 	int (*syslog)(int type);
diff --git a/include/linux/security.h b/include/linux/security.h
index 7d9c1da1f659..a37377065401 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -77,9 +77,18 @@  enum lsm_event {
 	LSM_POLICY_CHANGE,
 };
 
+
+struct cap_aux_data {
+	char type;
+#define CAP_AUX_DATA_INODE	1
+	union	{
+		const struct inode *inode;
+	} u;
+};
+
 /* These functions are in security/commoncap.c */
 extern int cap_capable(const struct cred *cred, struct user_namespace *ns,
-		       int cap, unsigned int opts);
+		       int cap, unsigned int opts, struct cap_aux_data *cad);
 extern int cap_settime(const struct timespec64 *ts, const struct timezone *tz);
 extern int cap_ptrace_access_check(struct task_struct *child, unsigned int mode);
 extern int cap_ptrace_traceme(struct task_struct *parent);
@@ -215,9 +224,10 @@  int security_capset(struct cred *new, const struct cred *old,
 		    const kernel_cap_t *inheritable,
 		    const kernel_cap_t *permitted);
 int security_capable(const struct cred *cred,
-		       struct user_namespace *ns,
-		       int cap,
-		       unsigned int opts);
+		     struct user_namespace *ns,
+		     int cap,
+		     unsigned int opts,
+		     struct cap_aux_data *cad);
 int security_quotactl(int cmds, int type, int id, struct super_block *sb);
 int security_quota_on(struct dentry *dentry);
 int security_syslog(int type);
@@ -478,9 +488,10 @@  static inline int security_capset(struct cred *new,
 static inline int security_capable(const struct cred *cred,
 				   struct user_namespace *ns,
 				   int cap,
-				   unsigned int opts)
+				   unsigned int opts,
+				   struct cap_aux_data *cad)
 {
-	return cap_capable(cred, ns, cap, opts);
+	return cap_capable(cred, ns, cap, opts, NULL);
 }
 
 static inline int security_quotactl(int cmds, int type, int id,
diff --git a/kernel/capability.c b/kernel/capability.c
index 1444f3954d75..c3723443904a 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -297,7 +297,7 @@  bool has_ns_capability(struct task_struct *t,
 	int ret;
 
 	rcu_read_lock();
-	ret = security_capable(__task_cred(t), ns, cap, CAP_OPT_NONE);
+	ret = security_capable(__task_cred(t), ns, cap, CAP_OPT_NONE, NULL);
 	rcu_read_unlock();
 
 	return (ret == 0);
@@ -338,7 +338,7 @@  bool has_ns_capability_noaudit(struct task_struct *t,
 	int ret;
 
 	rcu_read_lock();
-	ret = security_capable(__task_cred(t), ns, cap, CAP_OPT_NOAUDIT);
+	ret = security_capable(__task_cred(t), ns, cap, CAP_OPT_NOAUDIT, NULL);
 	rcu_read_unlock();
 
 	return (ret == 0);
@@ -363,7 +363,8 @@  bool has_capability_noaudit(struct task_struct *t, int cap)
 
 static bool ns_capable_common(struct user_namespace *ns,
 			      int cap,
-			      unsigned int opts)
+			      unsigned int opts,
+			      struct cap_aux_data *cad)
 {
 	int capable;
 
@@ -372,7 +373,7 @@  static bool ns_capable_common(struct user_namespace *ns,
 		BUG();
 	}
 
-	capable = security_capable(current_cred(), ns, cap, opts);
+	capable = security_capable(current_cred(), ns, cap, opts, cad);
 	if (capable == 0) {
 		current->flags |= PF_SUPERPRIV;
 		return true;
@@ -393,7 +394,7 @@  static bool ns_capable_common(struct user_namespace *ns,
  */
 bool ns_capable(struct user_namespace *ns, int cap)
 {
-	return ns_capable_common(ns, cap, CAP_OPT_NONE);
+	return ns_capable_common(ns, cap, CAP_OPT_NONE, NULL);
 }
 EXPORT_SYMBOL(ns_capable);
 
@@ -411,7 +412,7 @@  EXPORT_SYMBOL(ns_capable);
  */
 bool ns_capable_noaudit(struct user_namespace *ns, int cap)
 {
-	return ns_capable_common(ns, cap, CAP_OPT_NOAUDIT);
+	return ns_capable_common(ns, cap, CAP_OPT_NOAUDIT, NULL);
 }
 EXPORT_SYMBOL(ns_capable_noaudit);
 
@@ -430,7 +431,7 @@  EXPORT_SYMBOL(ns_capable_noaudit);
  */
 bool ns_capable_setid(struct user_namespace *ns, int cap)
 {
-	return ns_capable_common(ns, cap, CAP_OPT_INSETID);
+	return ns_capable_common(ns, cap, CAP_OPT_INSETID, NULL);
 }
 EXPORT_SYMBOL(ns_capable_setid);
 
@@ -470,7 +471,7 @@  bool file_ns_capable(const struct file *file, struct user_namespace *ns,
 	if (WARN_ON_ONCE(!cap_valid(cap)))
 		return false;
 
-	if (security_capable(file->f_cred, ns, cap, CAP_OPT_NONE) == 0)
+	if (security_capable(file->f_cred, ns, cap, CAP_OPT_NONE, NULL) == 0)
 		return true;
 
 	return false;
@@ -503,7 +504,8 @@  bool capable_wrt_inode_uidgid(const struct inode *inode, int cap)
 {
 	struct user_namespace *ns = current_user_ns();
 
-	return ns_capable(ns, cap) && privileged_wrt_inode_uidgid(ns, inode);
+	return ns_capable_inode(ns, cap, inode) &&
+		privileged_wrt_inode_uidgid(ns, inode);
 }
 EXPORT_SYMBOL(capable_wrt_inode_uidgid);
 
@@ -524,7 +526,18 @@  bool ptracer_capable(struct task_struct *tsk, struct user_namespace *ns)
 	cred = rcu_dereference(tsk->ptracer_cred);
 	if (cred)
 		ret = security_capable(cred, ns, CAP_SYS_PTRACE,
-				       CAP_OPT_NOAUDIT);
+				       CAP_OPT_NOAUDIT, NULL);
 	rcu_read_unlock();
 	return (ret == 0);
 }
+
+bool ns_capable_inode(struct user_namespace *ns, int cap,
+			const struct inode *inode)
+{
+	struct cap_aux_data cad;
+
+	cad.type = CAP_AUX_DATA_INODE;
+	cad.u.inode = inode;
+	return ns_capable_common(ns, cap, CAP_OPT_NONE, &cad);
+}
+EXPORT_SYMBOL(ns_capable_inode);
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 811b4a86cdf6..d59dd7079ece 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -446,7 +446,7 @@  static struct seccomp_filter *seccomp_prepare_filter(struct sock_fprog *fprog)
 	 */
 	if (!task_no_new_privs(current) &&
 	    security_capable(current_cred(), current_user_ns(),
-				     CAP_SYS_ADMIN, CAP_OPT_NOAUDIT) != 0)
+			     CAP_SYS_ADMIN, CAP_OPT_NOAUDIT, NULL) != 0)
 		return ERR_PTR(-EACCES);
 
 	/* Allocate a new seccomp_filter */
diff --git a/security/apparmor/capability.c b/security/apparmor/capability.c
index 752f73980e30..c45192a16733 100644
--- a/security/apparmor/capability.c
+++ b/security/apparmor/capability.c
@@ -50,7 +50,7 @@  static void audit_cb(struct audit_buffer *ab, void *va)
 	struct common_audit_data *sa = va;
 
 	audit_log_format(ab, " capname=");
-	audit_log_untrustedstring(ab, capability_names[sa->u.cap]);
+	audit_log_untrustedstring(ab, capability_names[sa->u.cap_struct.cap]);
 }
 
 /**
@@ -148,13 +148,15 @@  static int profile_capable(struct aa_profile *profile, int cap,
  *
  * Returns: 0 on success, or else an error code.
  */
-int aa_capable(struct aa_label *label, int cap, unsigned int opts)
+int aa_capable(struct aa_label *label, int cap, unsigned int opts,
+	       struct cap_aux_data *cad)
 {
 	struct aa_profile *profile;
 	int error = 0;
 	DEFINE_AUDIT_DATA(sa, LSM_AUDIT_DATA_CAP, OP_CAPABLE);
 
-	sa.u.cap = cap;
+	sa.u.cap_struct.cap = cap;
+	sa.u.cap_struct.cad = cad;
 	error = fn_for_each_confined(label, profile,
 			profile_capable(profile, cap, opts, &sa));
 
diff --git a/security/apparmor/include/capability.h b/security/apparmor/include/capability.h
index 1b3663b6ab12..d888f09d76d1 100644
--- a/security/apparmor/include/capability.h
+++ b/security/apparmor/include/capability.h
@@ -20,6 +20,7 @@ 
 #include "apparmorfs.h"
 
 struct aa_label;
+struct cap_aux_data;
 
 /* aa_caps - confinement data for capabilities
  * @allowed: capabilities mask
@@ -40,7 +41,8 @@  struct aa_caps {
 
 extern struct aa_sfs_entry aa_sfs_entry_caps[];
 
-int aa_capable(struct aa_label *label, int cap, unsigned int opts);
+int aa_capable(struct aa_label *label, int cap, unsigned int opts,
+	       struct cap_aux_data *cad);
 
 static inline void aa_free_cap_rules(struct aa_caps *caps)
 {
diff --git a/security/apparmor/ipc.c b/security/apparmor/ipc.c
index aacd1e95cb59..deb5267ca695 100644
--- a/security/apparmor/ipc.c
+++ b/security/apparmor/ipc.c
@@ -108,7 +108,7 @@  static int profile_tracer_perm(struct aa_profile *tracer,
 	aad(sa)->peer = tracee;
 	aad(sa)->request = 0;
 	aad(sa)->error = aa_capable(&tracer->label, CAP_SYS_PTRACE,
-				    CAP_OPT_NONE);
+				    CAP_OPT_NONE, NULL);
 
 	return aa_audit(AUDIT_APPARMOR_AUTO, tracer, sa, audit_ptrace_cb);
 }
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 87500bde5a92..82790accb679 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -172,14 +172,15 @@  static int apparmor_capget(struct task_struct *target, kernel_cap_t *effective,
 }
 
 static int apparmor_capable(const struct cred *cred, struct user_namespace *ns,
-			    int cap, unsigned int opts)
+			    int cap, unsigned int opts,
+			    struct cap_aux_data *cad)
 {
 	struct aa_label *label;
 	int error = 0;
 
 	label = aa_get_newest_cred_label(cred);
 	if (!unconfined(label))
-		error = aa_capable(label, cap, opts);
+		error = aa_capable(label, cap, opts, cad);
 	aa_put_label(label);
 
 	return error;
diff --git a/security/apparmor/resource.c b/security/apparmor/resource.c
index 552ed09cb47e..9b3d4b4437f2 100644
--- a/security/apparmor/resource.c
+++ b/security/apparmor/resource.c
@@ -124,7 +124,7 @@  int aa_task_setrlimit(struct aa_label *label, struct task_struct *task,
 	 */
 
 	if (label != peer &&
-	    aa_capable(label, CAP_SYS_RESOURCE, CAP_OPT_NOAUDIT) != 0)
+	    aa_capable(label, CAP_SYS_RESOURCE, CAP_OPT_NOAUDIT, NULL) != 0)
 		error = fn_for_each(label, profile,
 				audit_resource(profile, resource,
 					       new_rlim->rlim_max, peer,
diff --git a/security/commoncap.c b/security/commoncap.c
index c477fb673701..1860ea50f473 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -68,7 +68,7 @@  static void warn_setuid_and_fcaps_mixed(const char *fname)
  * kernel's capable() and has_capability() returns 1 for this case.
  */
 int cap_capable(const struct cred *cred, struct user_namespace *targ_ns,
-		int cap, unsigned int opts)
+		int cap, unsigned int opts, struct cap_aux_data *cad)
 {
 	struct user_namespace *ns = targ_ns;
 
@@ -226,7 +226,7 @@  static inline int cap_inh_is_capped(void)
 	 * capability
 	 */
 	if (cap_capable(current_cred(), current_cred()->user_ns,
-			CAP_SETPCAP, CAP_OPT_NONE) == 0)
+			CAP_SETPCAP, CAP_OPT_NONE, NULL) == 0)
 		return 0;
 	return 1;
 }
@@ -1211,7 +1211,8 @@  int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
 		    || (cap_capable(current_cred(),
 				    current_cred()->user_ns,
 				    CAP_SETPCAP,
-				    CAP_OPT_NONE) != 0)			/*[4]*/
+				    CAP_OPT_NONE,
+				    NULL) != 0)				/*[4]*/
 			/*
 			 * [1] no changing of bits that are locked
 			 * [2] no unlocking of locks
@@ -1307,7 +1308,7 @@  int cap_vm_enough_memory(struct mm_struct *mm, long pages)
 	int cap_sys_admin = 0;
 
 	if (cap_capable(current_cred(), &init_user_ns,
-				CAP_SYS_ADMIN, CAP_OPT_NOAUDIT) == 0)
+				CAP_SYS_ADMIN, CAP_OPT_NOAUDIT, NULL) == 0)
 		cap_sys_admin = 1;
 
 	return cap_sys_admin;
@@ -1328,7 +1329,7 @@  int cap_mmap_addr(unsigned long addr)
 
 	if (addr < dac_mmap_min_addr) {
 		ret = cap_capable(current_cred(), &init_user_ns, CAP_SYS_RAWIO,
-				  CAP_OPT_NONE);
+				  CAP_OPT_NONE, NULL);
 		/* set PF_SUPERPRIV if it turns out we allow the low mmap */
 		if (ret == 0)
 			current->flags |= PF_SUPERPRIV;
diff --git a/security/lsm_audit.c b/security/lsm_audit.c
index 33028c098ef3..18cc7c956b69 100644
--- a/security/lsm_audit.c
+++ b/security/lsm_audit.c
@@ -229,9 +229,26 @@  static void dump_common_audit_data(struct audit_buffer *ab,
 	case LSM_AUDIT_DATA_IPC:
 		audit_log_format(ab, " key=%d ", a->u.ipc_id);
 		break;
-	case LSM_AUDIT_DATA_CAP:
-		audit_log_format(ab, " capability=%d ", a->u.cap);
+	case LSM_AUDIT_DATA_CAP: {
+		const struct inode *inode;
+
+		audit_log_format(ab, " capability=%d ", a->u.cap_struct.cap);
+		if (a->u.cap_struct.cad) {
+			switch (a->u.cap_struct.cad->type) {
+			case CAP_AUX_DATA_INODE: {
+				inode = a->u.cap_struct.cad->u.inode;
+
+				audit_log_format(ab, " dev=");
+				audit_log_untrustedstring(ab,
+					inode->i_sb->s_id);
+				audit_log_format(ab, " ino=%lu",
+					inode->i_ino);
+				break;
+			}
+			}
+		}
 		break;
+	}
 	case LSM_AUDIT_DATA_PATH: {
 		struct inode *inode;
 
diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
index cecd38e2ac80..c74ed83e9501 100644
--- a/security/safesetid/lsm.c
+++ b/security/safesetid/lsm.c
@@ -80,7 +80,8 @@  static bool check_setuid_policy_hashtable_key_value(kuid_t parent,
 static int safesetid_security_capable(const struct cred *cred,
 				      struct user_namespace *ns,
 				      int cap,
-				      unsigned int opts)
+				      unsigned int opts,
+				      struct cap_aux_data *cad)
 {
 	if (cap == CAP_SETUID &&
 	    check_setuid_policy_hashtable_key(cred->uid)) {
diff --git a/security/security.c b/security/security.c
index 30687e1366b7..e5e0637f43ac 100644
--- a/security/security.c
+++ b/security/security.c
@@ -691,9 +691,10 @@  int security_capset(struct cred *new, const struct cred *old,
 int security_capable(const struct cred *cred,
 		     struct user_namespace *ns,
 		     int cap,
-		     unsigned int opts)
+		     unsigned int opts,
+		     struct cap_aux_data *cad)
 {
-	return call_int_hook(capable, 0, cred, ns, cap, opts);
+	return call_int_hook(capable, 0, cred, ns, cap, opts, cad);
 }
 
 int security_quotactl(int cmds, int type, int id, struct super_block *sb)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index a1aaf79421df..1816402a6b8a 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1620,7 +1620,10 @@  static inline u32 signal_to_av(int sig)
 
 /* Check whether a task is allowed to use a capability. */
 static int cred_has_capability(const struct cred *cred,
-			       int cap, unsigned int opts, bool initns)
+				int cap,
+				unsigned int opts,
+				bool initns,
+				struct cap_aux_data *cad)
 {
 	struct common_audit_data ad;
 	struct av_decision avd;
@@ -1630,7 +1633,8 @@  static int cred_has_capability(const struct cred *cred,
 	int rc;
 
 	ad.type = LSM_AUDIT_DATA_CAP;
-	ad.u.cap = cap;
+	ad.u.cap_struct.cad = cad;
+	ad.u.cap_struct.cap = cap;
 
 	switch (CAP_TO_INDEX(cap)) {
 	case 0:
@@ -2167,9 +2171,9 @@  static int selinux_capset(struct cred *new, const struct cred *old,
  */
 
 static int selinux_capable(const struct cred *cred, struct user_namespace *ns,
-			   int cap, unsigned int opts)
+			   int cap, unsigned int opts, struct cap_aux_data *cad)
 {
-	return cred_has_capability(cred, cap, opts, ns == &init_user_ns);
+	return cred_has_capability(cred, cap, opts, ns == &init_user_ns, cad);
 }
 
 static int selinux_quotactl(int cmds, int type, int id, struct super_block *sb)
@@ -2243,7 +2247,7 @@  static int selinux_vm_enough_memory(struct mm_struct *mm, long pages)
 	int rc, cap_sys_admin = 0;
 
 	rc = cred_has_capability(current_cred(), CAP_SYS_ADMIN,
-				 CAP_OPT_NOAUDIT, true);
+				 CAP_OPT_NOAUDIT, true, NULL);
 	if (rc == 0)
 		cap_sys_admin = 1;
 
@@ -3103,9 +3107,9 @@  static bool has_cap_mac_admin(bool audit)
 	const struct cred *cred = current_cred();
 	unsigned int opts = audit ? CAP_OPT_NONE : CAP_OPT_NOAUDIT;
 
-	if (cap_capable(cred, &init_user_ns, CAP_MAC_ADMIN, opts))
+	if (cap_capable(cred, &init_user_ns, CAP_MAC_ADMIN, opts, NULL))
 		return false;
-	if (cred_has_capability(cred, CAP_MAC_ADMIN, opts, true))
+	if (cred_has_capability(cred, CAP_MAC_ADMIN, opts, true, NULL))
 		return false;
 	return true;
 }
@@ -3609,7 +3613,7 @@  static int selinux_file_ioctl(struct file *file, unsigned int cmd,
 	case KDSKBENT:
 	case KDSKBSENT:
 		error = cred_has_capability(cred, CAP_SYS_TTY_CONFIG,
-					    CAP_OPT_NONE, true);
+					    CAP_OPT_NONE, true, NULL);
 		break;
 
 	/* default case assumes that the command will go
diff --git a/security/smack/smack_access.c b/security/smack/smack_access.c
index fe2ce3a65822..e961bfe8f00a 100644
--- a/security/smack/smack_access.c
+++ b/security/smack/smack_access.c
@@ -640,7 +640,7 @@  bool smack_privileged_cred(int cap, const struct cred *cred)
 	struct smack_known_list_elem *sklep;
 	int rc;
 
-	rc = cap_capable(cred, &init_user_ns, cap, CAP_OPT_NONE);
+	rc = cap_capable(cred, &init_user_ns, cap, CAP_OPT_NONE, NULL);
 	if (rc)
 		return false;