All of lore.kernel.org
 help / color / mirror / Atom feed
From: Paul Moore <paul@paul-moore.com>
To: Aaron Goidel <acgoide@tycho.nsa.gov>
Cc: selinux@vger.kernel.org, linux-security-module@vger.kernel.org,
	luto@amacapital.net, James Morris <jmorris@namei.org>,
	Stephen Smalley <sds@tycho.nsa.gov>,
	keescook@chromium.org, Serge Hallyn <serge@hallyn.com>,
	john.johansen@canonical.com, casey@schaufler-ca.com,
	mortonm@chromium.org, rgb@redhat.com,
	Nicholas Franck <nhfran2@tycho.nsa.gov>,
	linux-audit@redhat.com
Subject: Re: [RFC PATCH v2] security,capability: pass object information to security_capable
Date: Thu, 8 Aug 2019 12:30:28 -0400	[thread overview]
Message-ID: <CAHC9VhTSWiz45vh+M+sgu+ePwgFPZ4Mr8GmRZQjsGWQSzkjbLg@mail.gmail.com> (raw)
In-Reply-To: <20190801144313.1014-1-acgoide@tycho.nsa.gov>

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;
>

-- 
paul moore
www.paul-moore.com

WARNING: multiple messages have this Message-ID (diff)
From: Paul Moore <paul@paul-moore.com>
To: Aaron Goidel <acgoide@tycho.nsa.gov>
Cc: mortonm@chromium.org, john.johansen@canonical.com,
	selinux@vger.kernel.org, James Morris <jmorris@namei.org>,
	luto@amacapital.net, rgb@redhat.com,
	linux-security-module@vger.kernel.org, linux-audit@redhat.com,
	Nicholas Franck <nhfran2@tycho.nsa.gov>,
	Stephen Smalley <sds@tycho.nsa.gov>,
	Serge Hallyn <serge@hallyn.com>
Subject: Re: [RFC PATCH v2] security, capability: pass object information to security_capable
Date: Thu, 8 Aug 2019 12:30:28 -0400	[thread overview]
Message-ID: <CAHC9VhTSWiz45vh+M+sgu+ePwgFPZ4Mr8GmRZQjsGWQSzkjbLg@mail.gmail.com> (raw)
In-Reply-To: <20190801144313.1014-1-acgoide@tycho.nsa.gov>

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;
>

-- 
paul moore
www.paul-moore.com

  reply	other threads:[~2019-08-08 16:30 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-01 14:43 [RFC PATCH v2] security,capability: pass object information to security_capable Aaron Goidel
2019-08-08 16:30 ` Paul Moore [this message]
2019-08-08 16:30   ` [RFC PATCH v2] security, capability: " Paul Moore
2019-08-13 15:01   ` [Non-DoD Source] " Aaron Goidel
2019-08-13 15:01     ` Aaron Goidel
2019-08-13 21:27     ` Richard Guy Briggs
2019-08-13 21:27       ` Richard Guy Briggs
2019-08-14 19:59       ` Paul Moore
2019-08-14 19:59         ` Paul Moore
2019-08-14 21:08         ` Stephen Smalley
2019-08-14 21:08           ` Stephen Smalley
2019-08-14 21:27           ` Paul Moore
2019-08-14 21:27             ` Paul Moore
2019-08-15 13:10             ` [Non-DoD Source] " Aaron Goidel
2019-08-15 13:10               ` Aaron Goidel
2019-08-16 16:29               ` Paul Moore
2019-08-16 16:29                 ` Paul Moore

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAHC9VhTSWiz45vh+M+sgu+ePwgFPZ4Mr8GmRZQjsGWQSzkjbLg@mail.gmail.com \
    --to=paul@paul-moore.com \
    --cc=acgoide@tycho.nsa.gov \
    --cc=casey@schaufler-ca.com \
    --cc=jmorris@namei.org \
    --cc=john.johansen@canonical.com \
    --cc=keescook@chromium.org \
    --cc=linux-audit@redhat.com \
    --cc=linux-security-module@vger.kernel.org \
    --cc=luto@amacapital.net \
    --cc=mortonm@chromium.org \
    --cc=nhfran2@tycho.nsa.gov \
    --cc=rgb@redhat.com \
    --cc=sds@tycho.nsa.gov \
    --cc=selinux@vger.kernel.org \
    --cc=serge@hallyn.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.