SELinux Archive on lore.kernel.org
 help / color / 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
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 index

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

Reply instructions:

You may reply publically 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

SELinux Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/selinux/0 selinux/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 selinux selinux/ https://lore.kernel.org/selinux \
		selinux@vger.kernel.org selinux@archiver.kernel.org
	public-inbox-index selinux

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.selinux


AGPL code for this site: git clone https://public-inbox.org/ public-inbox