linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Richard Guy Briggs <rgb@redhat.com>
To: linux-security-module@vger.kernel.org, linux-audit@redhat.com,
	linux-kernel@vger.kernel.org
Cc: Richard Guy Briggs <rgb@redhat.com>,
	Kees Cook <keescook@chromium.org>,
	Andy Lutomirski <luto@kernel.org>,
	"Serge E . Hallyn" <serge.hallyn@ubuntu.com>,
	James Morris <james.l.morris@oracle.com>,
	Paul Moore <pmoore@redhat.com>, Steve Grubb <sgrubb@redhat.com>,
	Eric Paris <eparis@redhat.com>
Subject: [PATCH GHAK16 V5 10/10] capabilities: audit log other surprising conditions
Date: Wed, 11 Oct 2017 20:57:14 -0400	[thread overview]
Message-ID: <5020cb3590120ad799db8324ede6a8122a3c9967.1507769413.git.rgb@redhat.com> (raw)
In-Reply-To: <cover.1507769413.git.rgb@redhat.com>
In-Reply-To: <cover.1507769413.git.rgb@redhat.com>

The existing condition tested for process effective capabilities set by
file attributes but intended to ignore the change if the result was
unsurprisingly an effective full set in the case root is special with a
setuid root executable file and we are root.

Stated again:
- When you execute a setuid root application, it is no surprise and
  expected that it got all capabilities, so we do not want capabilities
  recorded.
        if (pE_grew && !(pE_fullset && (eff_root || real_root) && root_priveleged) )

Now make sure we cover other cases:
- If something prevented a setuid root app getting all capabilities and
  it wound up with one capability only, then it is a surprise and should
  be logged.  When it is a setuid root file, we only want capabilities
  when the process does not get full capabilities..
        root_priveleged && setuid_root && !pE_fullset

- Similarly if a non-setuid program does pick up capabilities due to
  file system based capabilities, then we want to know what capabilities
  were picked up.  When it has file system based capabilities we want
  the capabilities.
        !is_setuid && (has_fcap && pP_gained)

- If it is a non-setuid file and it gets ambient capabilities, we want
  the capabilities.
        !is_setuid && pA_gained

- These last two are combined into one due to the common first parameter.

Related: https://github.com/linux-audit/audit-kernel/issues/16

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Acked-by: James Morris <james.l.morris@oracle.com>
Acked-by: Kees Cook <keescook@chromium.org>
Acked-by: Paul Moore <paul@paul-moore.com>
---
 security/commoncap.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index ad7536d..5fa839c 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -766,7 +766,7 @@ static inline bool __is_setgid(struct cred *new, const struct cred *old)
 { return !gid_eq(new->egid, old->gid); }
 
 /*
- * Audit candidate if current->cap_effective is set
+ * 1) Audit candidate if current->cap_effective is set
  *
  * We do not bother to audit if 3 things are true:
  *   1) cap_effective has all caps
@@ -776,16 +776,31 @@ static inline bool __is_setgid(struct cred *new, const struct cred *old)
  *
  * Number 1 above might fail if you don't have a full bset, but I think
  * that is interesting information to audit.
+ *
+ * A number of other conditions require logging:
+ * 2) something prevented setuid root getting all caps
+ * 3) non-setuid root gets fcaps
+ * 4) non-setuid root gets ambient
  */
-static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root)
+static inline bool nonroot_raised_pE(struct cred *new, const struct cred *old,
+				     kuid_t root, bool has_fcap)
 {
 	bool ret = false;
 
-	if (__cap_grew(effective, ambient, cred) &&
-	    !(__cap_full(effective, cred) &&
-	      (__is_eff(root, cred) || __is_real(root, cred)) &&
-	      root_privileged()))
+	if ((__cap_grew(effective, ambient, new) &&
+	     !(__cap_full(effective, new) &&
+	       (__is_eff(root, new) || __is_real(root, new)) &&
+	       root_privileged())) ||
+	    (root_privileged() &&
+	     __is_suid(root, new) &&
+	     !__cap_full(effective, new)) ||
+	    (!__is_setuid(new, old) &&
+	     ((has_fcap &&
+	       __cap_gained(permitted, new, old)) ||
+	      __cap_gained(ambient, new, old))))
+
 		ret = true;
+
 	return ret;
 }
 
@@ -865,7 +880,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 	if (WARN_ON(!cap_ambient_invariant_ok(new)))
 		return -EPERM;
 
-	if (nonroot_raised_pE(new, root_uid)) {
+	if (nonroot_raised_pE(new, old, root_uid, has_fcap)) {
 		ret = audit_log_bprm_fcaps(bprm, new, old);
 		if (ret < 0)
 			return ret;
-- 
1.8.3.1

  parent reply	other threads:[~2017-10-12  0:59 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-10-12  0:57 [PATCH GHAK16 V5 00/10] capabilities: do not audit log BPRM_FCAPS on set*id Richard Guy Briggs
2017-10-12  0:57 ` [PATCH GHAK16 V5 01/10] capabilities: factor out cap_bprm_set_creds privileged root Richard Guy Briggs
2017-10-12  0:57 ` [PATCH GHAK16 V5 02/10] capabilities: intuitive names for cap gain status Richard Guy Briggs
2017-10-12  0:57 ` [PATCH GHAK16 V5 03/10] capabilities: rename has_cap to has_fcap Richard Guy Briggs
2017-10-12  0:57 ` [PATCH GHAK16 V5 04/10] capabilities: use root_priveleged inline to clarify logic Richard Guy Briggs
2017-10-12  0:57 ` [PATCH GHAK16 V5 05/10] capabilities: use intuitive names for id changes Richard Guy Briggs
2017-10-12  0:57 ` [PATCH GHAK16 V5 06/10] capabilities: move audit log decision to function Richard Guy Briggs
2017-10-12  0:57 ` [PATCH GHAK16 V5 07/10] capabilities: remove a layer of conditional logic Richard Guy Briggs
2017-10-12  0:57 ` [PATCH GHAK16 V5 08/10] capabilities: invert logic for clarity Richard Guy Briggs
2017-10-12  0:57 ` [PATCH GHAK16 V5 09/10] capabilities: fix logic for effective root or real root Richard Guy Briggs
2017-10-12  0:57 ` Richard Guy Briggs [this message]
2017-10-19 13:08 ` [PATCH GHAK16 V5 00/10] capabilities: do not audit log BPRM_FCAPS on set*id Richard Guy Briggs
2017-10-20  1:29   ` James Morris
2017-10-20  2:34     ` Richard Guy Briggs
2017-10-20  5:15   ` James Morris

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=5020cb3590120ad799db8324ede6a8122a3c9967.1507769413.git.rgb@redhat.com \
    --to=rgb@redhat.com \
    --cc=eparis@redhat.com \
    --cc=james.l.morris@oracle.com \
    --cc=keescook@chromium.org \
    --cc=linux-audit@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=pmoore@redhat.com \
    --cc=serge.hallyn@ubuntu.com \
    --cc=sgrubb@redhat.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).