[PATCH 1/7] audit: implement generic feature setting and retrieving

[PATCH 1/7] audit: implement generic feature setting and retrieving

[Eric Paris]

The audit_status structure was not designed with extensibility in mind.
Define a new AUDIT_SET_FEATURE message type which takes a new structure
of bits where things can be enabled/disabled/locked one at a time.  This
structure should be able to grow in the future while maintaining forward
and backward compatibility (based loosly on the ideas from capabilities
and prctl)

This does not actually add any features, but is just infrastructure to
allow new on/off types of audit system features.

Signed-off-by: Eric Paris <eparis@redhat.com>
---
 include/linux/audit.h      |   2 +
 include/uapi/linux/audit.h |  16 +++++++
 kernel/audit.c             | 110 ++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 127 insertions(+), 1 deletion(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 729a4d1..7b31bec 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -73,6 +73,8 @@ struct audit_field {
 	void				*lsm_rule;
 };
 
+extern int is_audit_feature_set(int which);
+
 extern int __init audit_register_class(int class, unsigned *list);
 extern int audit_classify_syscall(int abi, unsigned syscall);
 extern int audit_classify_arch(int arch);
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index b7cb978..a053243 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -68,6 +68,9 @@
 #define AUDIT_MAKE_EQUIV	1015	/* Append to watched tree */
 #define AUDIT_TTY_GET		1016	/* Get TTY auditing status */
 #define AUDIT_TTY_SET		1017	/* Set TTY auditing status */
+#define AUDIT_SET_FEATURE	1018	/* Turn an audit feature on or off */
+#define AUDIT_GET_FEATURE	1019	/* Get which features are enabled */
+#define AUDIT_FEATURE_CHANGE	1020	/* audit log listing feature changes */
 
 #define AUDIT_FIRST_USER_MSG	1100	/* Userspace messages mostly uninteresting to kernel */
 #define AUDIT_USER_AVC		1107	/* We filter this differently */
@@ -369,6 +372,19 @@ struct audit_status {
 	__u32		backlog;	/* messages waiting in queue */
 };
 
+struct audit_features {
+#define AUDIT_FEATURE_VERSION	1
+	__u32	vers;
+	__u32	mask;		/* which bits we are dealing with */
+	__u32	features;	/* which feature to enable/disable */
+	__u32	lock;		/* which features to lock */
+};
+
+#define AUDIT_LAST_FEATURE	-1
+
+#define audit_feature_valid(x)		((x) >= 0 && (x) <= AUDIT_LAST_FEATURE)
+#define AUDIT_FEATURE_TO_MASK(x)	(1 << ((x) & 31)) /* mask for __u32 */
+
 struct audit_tty_status {
 	__u32		enabled;	/* 1 = enabled, 0 = disabled */
 	__u32		log_passwd;	/* 1 = enabled, 0 = disabled */
diff --git a/kernel/audit.c b/kernel/audit.c
index f2f4666..3acbbc8 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -140,6 +140,15 @@ static struct task_struct *kauditd_task;
 static DECLARE_WAIT_QUEUE_HEAD(kauditd_wait);
 static DECLARE_WAIT_QUEUE_HEAD(audit_backlog_wait);
 
+static struct audit_features af = {.vers = AUDIT_FEATURE_VERSION,
+				   .mask = -1,
+				   .features = 0,
+				   .lock = 0,};
+
+static char *audit_feature_names[0] = {
+};
+
+
 /* Serialize requests from userspace. */
 DEFINE_MUTEX(audit_cmd_mutex);
 
@@ -584,6 +593,8 @@ static int audit_netlink_ok(struct sk_buff *skb, u16 msg_type)
 		return -EOPNOTSUPP;
 	case AUDIT_GET:
 	case AUDIT_SET:
+	case AUDIT_GET_FEATURE:
+	case AUDIT_SET_FEATURE:
 	case AUDIT_LIST_RULES:
 	case AUDIT_ADD_RULE:
 	case AUDIT_DEL_RULE:
@@ -628,6 +639,94 @@ static int audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
 	return rc;
 }
 
+int is_audit_feature_set(int i)
+{
+	return af.features & AUDIT_FEATURE_TO_MASK(i);
+}
+
+
+static int audit_get_feature(struct sk_buff *skb)
+{
+	u32 seq;
+
+	seq = nlmsg_hdr(skb)->nlmsg_seq;
+
+	audit_send_reply(NETLINK_CB(skb).portid, seq, AUDIT_GET, 0, 0,
+			 &af, sizeof(af));
+
+	return 0;
+}
+
+static void audit_log_feature_change(int which, u32 old_feature, u32 new_feature,
+				     u32 old_lock, u32 new_lock, int res)
+{
+	struct audit_buffer *ab;
+
+	ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE);
+	audit_log_format(ab, "feature=%s new=%d old=%d old_lock=%d new_lock=%d res=%d",
+			 audit_feature_names[which], !!old_feature, !!new_feature,
+			 !!old_lock, !!new_lock, res);
+	audit_log_end(ab);
+}
+
+static int audit_set_feature(struct sk_buff *skb)
+{
+	struct audit_features *uaf;
+	int i;
+
+	BUILD_BUG_ON(AUDIT_LAST_FEATURE + 1 > sizeof(audit_feature_names)/sizeof(audit_feature_names[0]));
+	uaf = nlmsg_data(nlmsg_hdr(skb));
+
+	/* if there is ever a version 2 we should handle that here */
+
+	for (i = 0; i <= AUDIT_LAST_FEATURE; i++) {
+		u32 feature = AUDIT_FEATURE_TO_MASK(i);
+		u32 old_feature, new_feature, old_lock, new_lock;
+
+		/* if we are not changing this feature, move along */
+		if (!(feature & uaf->mask))
+			continue;
+
+		old_feature = af.features & feature;
+		new_feature = uaf->features & feature;
+		new_lock = (uaf->lock | af.lock) & feature;
+		old_lock = af.lock & feature;
+
+		/* are we changing a locked feature? */
+		if ((af.lock & feature) && (new_feature != old_feature)) {
+			audit_log_feature_change(i, old_feature, new_feature,
+						 old_lock, new_lock, 0);
+			return -EPERM;
+		}
+	}
+	/* nothing invalid, do the changes */
+	for (i = 0; i <= AUDIT_LAST_FEATURE; i++) {
+		u32 feature = AUDIT_FEATURE_TO_MASK(i);
+		u32 old_feature, new_feature, old_lock, new_lock;
+
+		/* if we are not changing this feature, move along */
+		if (!(feature & uaf->mask))
+			continue;
+
+		old_feature = af.features & feature;
+		new_feature = uaf->features & feature;
+		old_lock = af.lock & feature;
+		new_lock = (uaf->lock | af.lock) & feature;
+
+		if (new_feature != old_feature)
+			audit_log_feature_change(i, old_feature, new_feature,
+						 old_lock, new_lock, 1);
+
+		if (new_feature)
+			af.features |= feature;
+		else
+			af.features &= ~feature;
+		af.lock |= new_lock;
+	}
+
+	return 0;
+}
+
 static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 {
 	u32			seq;
@@ -699,7 +798,16 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 		if (status_get->mask & AUDIT_STATUS_BACKLOG_LIMIT)
 			err = audit_set_backlog_limit(status_get->backlog_limit);
 		break;
-	case AUDIT_USER:
+	case AUDIT_GET_FEATURE:
+		err = audit_get_feature(skb);
+		if (err)
+			return err;
+		break;
+	case AUDIT_SET_FEATURE:
+		err = audit_set_feature(skb);
+		if (err)
+			return err;
+		break;
 	case AUDIT_FIRST_USER_MSG ... AUDIT_LAST_USER_MSG:
 	case AUDIT_FIRST_USER_MSG2 ... AUDIT_LAST_USER_MSG2:
 		if (!audit_enabled && msg_type != AUDIT_USER_AVC)
-- 
1.8.2.1

[PATCH 2/7] selinux: apply selinux checks on new audit message types

[Eric Paris]

We use the read check to get the feature set (like AUDIT_GET) and the
write check to set the features (like AUDIT_SET).

Signed-off-by: Eric Paris <eparis@redhat.com>
---
 security/selinux/nlmsgtab.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c
index 855e464..332ac8a 100644
--- a/security/selinux/nlmsgtab.c
+++ b/security/selinux/nlmsgtab.c
@@ -116,6 +116,8 @@ static struct nlmsg_perm nlmsg_audit_perms[] =
 	{ AUDIT_MAKE_EQUIV,	NETLINK_AUDIT_SOCKET__NLMSG_WRITE    },
 	{ AUDIT_TTY_GET,	NETLINK_AUDIT_SOCKET__NLMSG_READ     },
 	{ AUDIT_TTY_SET,	NETLINK_AUDIT_SOCKET__NLMSG_TTY_AUDIT	},
+	{ AUDIT_GET_FEATURE,	NETLINK_AUDIT_SOCKET__NLMSG_READ     },
+	{ AUDIT_SET_FEATURE,	NETLINK_AUDIT_SOCKET__NLMSG_WRITE    },
 };
 
 
-- 
1.8.2.1

[PATCH 3/7] audit: loginuid functions coding style

[Eric Paris]

This is just a code rework.  It makes things more readable.  It does not
make any functional changes.

It does change the log messages to include both the old session id as
well the new and it includes a new res field, which means we get
messages even when the user did not have permission to change the
loginuid.

Signed-off-by: Eric Paris <eparis@redhat.com>
---
 kernel/auditsc.c | 70 ++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 45 insertions(+), 25 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 9845cb3..f260281 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1964,6 +1964,39 @@ int auditsc_get_stamp(struct audit_context *ctx,
 /* global counter which is incremented every time something logs in */
 static atomic_t session_id = ATOMIC_INIT(0);
 
+static int audit_set_loginuid_perm(kuid_t loginuid)
+{
+#ifdef CONFIG_AUDIT_LOGINUID_IMMUTABLE
+	/* if we are unset, we don't need privs */
+	if (!audit_loginuid_set(current))
+		return 0;
+#else	/* CONFIG_AUDIT_LOGINUID_IMMUTABLE */
+	if (capable(CAP_AUDIT_CONTROL))
+		return 0;
+#endif /* CONFIG_AUDIT_LOGINUID_IMMUTABLE */
+	return -EPERM;
+}
+
+static void audit_log_set_loginuid(kuid_t koldloginuid, kuid_t kloginuid,
+				   unsigned int oldsessionid, unsigned int sessionid,
+				   int rc)
+{
+	struct audit_buffer *ab;
+	uid_t uid, ologinuid, nloginuid;
+
+	uid = from_kuid(&init_user_ns, task_uid(current));
+	ologinuid = from_kuid(&init_user_ns, koldloginuid);
+	nloginuid = from_kuid(&init_user_ns, kloginuid),
+
+	ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_LOGIN);
+	if (!ab)
+		return;
+	audit_log_format(ab, "pid=%d uid=%u old auid=%u new auid=%u old "
+			 "ses=%u new ses=%u res=%d", current->pid, uid, ologinuid,
+			 nloginuid, oldsessionid, sessionid, !rc);
+	audit_log_end(ab);
+}
+
 /**
  * audit_set_loginuid - set current task's audit_context loginuid
  * @loginuid: loginuid value
@@ -1975,37 +2008,24 @@ static atomic_t session_id = ATOMIC_INIT(0);
 int audit_set_loginuid(kuid_t loginuid)
 {
 	struct task_struct *task = current;
-	struct audit_context *context = task->audit_context;
-	unsigned int sessionid;
+	unsigned int sessionid = -1;
+	kuid_t oldloginuid, oldsessionid;
+	int rc;
 
-#ifdef CONFIG_AUDIT_LOGINUID_IMMUTABLE
-	if (audit_loginuid_set(task))
-		return -EPERM;
-#else /* CONFIG_AUDIT_LOGINUID_IMMUTABLE */
-	if (!capable(CAP_AUDIT_CONTROL))
-		return -EPERM;
-#endif  /* CONFIG_AUDIT_LOGINUID_IMMUTABLE */
+	oldloginuid = audit_get_loginuid(current);
+	oldsessionid = audit_get_sessionid(current);
+
+	rc = audit_set_loginuid_perm(loginuid);
+	if (rc)
+		goto out;
 
 	sessionid = atomic_inc_return(&session_id);
-	if (context && context->in_syscall) {
-		struct audit_buffer *ab;
 
-		ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_LOGIN);
-		if (ab) {
-			audit_log_format(ab, "login pid=%d uid=%u "
-				"old auid=%u new auid=%u"
-				" old ses=%u new ses=%u",
-				task->pid,
-				from_kuid(&init_user_ns, task_uid(task)),
-				from_kuid(&init_user_ns, task->loginuid),
-				from_kuid(&init_user_ns, loginuid),
-				task->sessionid, sessionid);
-			audit_log_end(ab);
-		}
-	}
 	task->sessionid = sessionid;
 	task->loginuid = loginuid;
-	return 0;
+out:
+	audit_log_set_loginuid(oldloginuid, loginuid, oldsessionid, sessionid, rc);
+	return rc;
 }
 
 /**
-- 
1.8.2.1

[PATCH 4/7] audit: remove CONFIG_AUDIT_LOGINUID_IMMUTABLE

[Eric Paris]

After trying to use this feature in Fedora we found the hard coding
policy like this into the kernel was a bad idea.  Surprise surprise.
We ran into these problems because it was impossible to launch a
container as a logged in user and run a login daemon inside that container.
This reverts back to the old behavior before this option was added.  The
option will be re-added in a userspace selectable manor such that
userspace can choose when it is and when it is not appropriate.

Signed-off-by: Eric Paris <eparis@redhat.com>
---
 init/Kconfig     | 14 --------------
 kernel/auditsc.c | 10 ++++------
 2 files changed, 4 insertions(+), 20 deletions(-)

diff --git a/init/Kconfig b/init/Kconfig
index 5341d72..2396945 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -272,20 +272,6 @@ config AUDIT_TREE
 	depends on AUDITSYSCALL
 	select FSNOTIFY
 
-config AUDIT_LOGINUID_IMMUTABLE
-	bool "Make audit loginuid immutable"
-	depends on AUDIT
-	help
-	  The config option toggles if a task setting its loginuid requires
-	  CAP_SYS_AUDITCONTROL or if that task should require no special permissions
-	  but should instead only allow setting its loginuid if it was never
-	  previously set.  On systems which use systemd or a similar central
-	  process to restart login services this should be set to true.  On older
-	  systems in which an admin would typically have to directly stop and
-	  start processes this should be set to false.  Setting this to true allows
-	  one to drop potentially dangerous capabilites from the login tasks,
-	  but may not be backwards compatible with older init systems.
-
 source "kernel/irq/Kconfig"
 source "kernel/time/Kconfig"
 
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index f260281..bbc5a4f 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1966,15 +1966,13 @@ static atomic_t session_id = ATOMIC_INIT(0);
 
 static int audit_set_loginuid_perm(kuid_t loginuid)
 {
-#ifdef CONFIG_AUDIT_LOGINUID_IMMUTABLE
 	/* if we are unset, we don't need privs */
 	if (!audit_loginuid_set(current))
 		return 0;
-#else	/* CONFIG_AUDIT_LOGINUID_IMMUTABLE */
-	if (capable(CAP_AUDIT_CONTROL))
-		return 0;
-#endif /* CONFIG_AUDIT_LOGINUID_IMMUTABLE */
-	return -EPERM;
+	/* it is set, you need permission */
+	if (!capable(CAP_AUDIT_CONTROL))
+		return -EPERM;
+	return 0;
 }
 
 static void audit_log_set_loginuid(kuid_t koldloginuid, kuid_t kloginuid,
-- 
1.8.2.1

[PATCH 5/7] audit: allow unsetting the loginuid (with priv)

[Eric Paris]

If a task has CAP_AUDIT_CONTROL allow that task to unset their loginuid.
This would allow a child of that task to set their loginuid without
CAP_AUDIT_CONTROL.  Thus when launching a new login daemon, a
priviledged helper would be able to unset the loginuid and then the
daemon, which may be malicious user facing, do not need priv to function
correctly.

Signed-off-by: Eric Paris <eparis@redhat.com>
---
 fs/proc/base.c   | 14 ++++++++++----
 kernel/auditsc.c |  4 +++-
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 69078c7..33d0d0b 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1119,10 +1119,16 @@ static ssize_t proc_loginuid_write(struct file * file, const char __user * buf,
 		goto out_free_page;
 
 	}
-	kloginuid = make_kuid(file->f_cred->user_ns, loginuid);
-	if (!uid_valid(kloginuid)) {
-		length = -EINVAL;
-		goto out_free_page;
+
+	/* is userspace tring to explicitly UNSET the loginuid? */
+	if (loginuid == AUDIT_UID_UNSET) {
+		kloginuid = INVALID_UID;
+	} else {
+		kloginuid = make_kuid(file->f_cred->user_ns, loginuid);
+		if (!uid_valid(kloginuid)) {
+			length = -EINVAL;
+			goto out_free_page;
+		}
 	}
 
 	length = audit_set_loginuid(kloginuid);
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index bbc5a4f..eea28c1 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2017,7 +2017,9 @@ int audit_set_loginuid(kuid_t loginuid)
 	if (rc)
 		goto out;
 
-	sessionid = atomic_inc_return(&session_id);
+	/* are we setting or clearing? */
+	if (uid_valid(loginuid))
+		sessionid = atomic_inc_return(&session_id);
 
 	task->sessionid = sessionid;
 	task->loginuid = loginuid;
-- 
1.8.2.1

[PATCH 6/7] audit: audit feature to only allow unsetting the loginuid

[Eric Paris]

This is a new audit feature which only grants processes with
CAP_AUDIT_CONTROL the ability to unset their loginuid.  They cannot
directly set it from a valid uid to another valid uid.  The ability to
unset the loginuid is nice because a priviledged task, like that of
container creation, can unset the loginuid and then priv is not needed
inside the container when a login daemon needs to set the loginuid.

Signed-off-by: Eric Paris <eparis@redhat.com>
---
 include/uapi/linux/audit.h | 3 ++-
 kernel/audit.c             | 3 ++-
 kernel/auditsc.c           | 3 +++
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index a053243..2963b5a 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -380,7 +380,8 @@ struct audit_features {
 	__u32	lock;		/* which features to lock */
 };
 
-#define AUDIT_LAST_FEATURE	-1
+#define AUDIT_FEATURE_ONLY_UNSET_LOGINUID	0
+#define AUDIT_LAST_FEATURE			AUDIT_FEATURE_ONLY_UNSET_LOGINUID
 
 #define audit_feature_valid(x)		((x) >= 0 && (x) <= AUDIT_LAST_FEATURE)
 #define AUDIT_FEATURE_TO_MASK(x)	(1 << ((x) & 31)) /* mask for __u32 */
diff --git a/kernel/audit.c b/kernel/audit.c
index 3acbbc8..a5c470b 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -145,7 +145,8 @@ static struct audit_features af = {.vers = AUDIT_FEATURE_VERSION,
 				   .features = 0,
 				   .lock = 0,};
 
-static char *audit_feature_names[0] = {
+static char *audit_feature_names[1] = {
+	"only_unset_loginuid",
 };
 
 
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index eea28c1..e5dbbc6 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1972,6 +1972,9 @@ static int audit_set_loginuid_perm(kuid_t loginuid)
 	/* it is set, you need permission */
 	if (!capable(CAP_AUDIT_CONTROL))
 		return -EPERM;
+	/* reject if this is not an unset and we don't allow that */
+	if (is_audit_feature_set(AUDIT_FEATURE_ONLY_UNSET_LOGINUID) && uid_valid(loginuid))
+		return -EPERM;
 	return 0;
 }
 
-- 
1.8.2.1

[PATCH 7/7] audit: audit feature to set loginuid immutable

[Eric Paris]

This adds a new 'audit_feature' bit which allows userspace to set it
such that the loginuid is absolutely immutable, even if you have
CAP_AUDIT_CONTROL.

Signed-off-by: Eric Paris <eparis@redhat.com>
---
 include/uapi/linux/audit.h | 3 ++-
 kernel/audit.c             | 3 ++-
 kernel/auditsc.c           | 3 +++
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 2963b5a..9539ea9 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -381,7 +381,8 @@ struct audit_features {
 };
 
 #define AUDIT_FEATURE_ONLY_UNSET_LOGINUID	0
-#define AUDIT_LAST_FEATURE			AUDIT_FEATURE_ONLY_UNSET_LOGINUID
+#define AUDIT_FEATURE_LOGINUID_IMMUTABLE	1
+#define AUDIT_LAST_FEATURE			AUDIT_FEATURE_LOGINUID_IMMUTABLE
 
 #define audit_feature_valid(x)		((x) >= 0 && (x) <= AUDIT_LAST_FEATURE)
 #define AUDIT_FEATURE_TO_MASK(x)	(1 << ((x) & 31)) /* mask for __u32 */
diff --git a/kernel/audit.c b/kernel/audit.c
index a5c470b..900d61d 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -145,8 +145,9 @@ static struct audit_features af = {.vers = AUDIT_FEATURE_VERSION,
 				   .features = 0,
 				   .lock = 0,};
 
-static char *audit_feature_names[1] = {
+static char *audit_feature_names[2] = {
 	"only_unset_loginuid",
+	"loginuid_immutable",
 };
 
 
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index e5dbbc6..aace3ac 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1969,6 +1969,9 @@ static int audit_set_loginuid_perm(kuid_t loginuid)
 	/* if we are unset, we don't need privs */
 	if (!audit_loginuid_set(current))
 		return 0;
+	/* if AUDIT_FEATURE_LOGINUID_IMMUTABLE means never ever allow a change*/
+	if (is_audit_feature_set(AUDIT_FEATURE_LOGINUID_IMMUTABLE))
+		return -EPERM;
 	/* it is set, you need permission */
 	if (!capable(CAP_AUDIT_CONTROL))
 		return -EPERM;
-- 
1.8.2.1

Re: [PATCH 1/7] audit: implement generic feature setting and retrieving

[Eric Paris]

[-- Attachment #1: Type: text/plain, Size: 1174 bytes --]

On Fri, 2013-05-24 at 12:11 -0400, Eric Paris wrote:
> The audit_status structure was not designed with extensibility in mind.
> Define a new AUDIT_SET_FEATURE message type which takes a new structure
> of bits where things can be enabled/disabled/locked one at a time.  This
> structure should be able to grow in the future while maintaining forward
> and backward compatibility (based loosly on the ideas from capabilities
> and prctl)
> 
> This does not actually add any features, but is just infrastructure to
> allow new on/off types of audit system features.
> 
> Signed-off-by: Eric Paris <eparis@redhat.com>

Attached you will find the test program I used to check that things were
working correctly.  It should give an idea to Steve how we can program
the features support in userspace.  I believe it fits very nicely to
have a new syntax in audit.rules to set (and lock if needed/wanted)
these features.

netlink.c is just some helper code I stole from the audit tree to get
some functions which weren't exposed externally.  The only part really
interesting is test.c.

You will also need the include/uapi/linux/audit.h file from this patch
to build test.c

-Eric

[-- Attachment #2: Makefile --]
[-- Type: text/x-makefile, Size: 71 bytes --]

all: test

test: test.c
	gcc -o test -Wall -W test.c netlink.c -laudit

[-- Attachment #3: netlink.c --]
[-- Type: text/x-csrc, Size: 2752 bytes --]

#include <libaudit.h>
#include <string.h>
#include <linux/audit.h>
#include <unistd.h>
#include <string.h>
#include <errno.h>
#include <fcntl.h>
#include <time.h>
#include <sys/poll.h>
#include <stdio.h>
#include "private.h"

int check_ack(int fd)
{
	int rc, retries = 80;
	struct audit_reply rep;
	struct pollfd pfd[1];

retry:
	pfd[0].fd = fd;
	pfd[0].events = POLLIN;
	do {
		rc = poll(pfd, 1, 500); /* .5 second */
	} while (rc < 0 && errno == EINTR);

	/* We don't look at rc from above as it doesn't matter. We are 
	 * going to try to read nonblocking just in case packet shows up. */

	/* NOTE: whatever is returned is treated as the errno */
	rc = audit_get_reply(fd, &rep, GET_REPLY_NONBLOCKING, MSG_PEEK);
	if (rc == -EAGAIN && retries) {
		retries--;
		goto retry;
	} else if (rc < 0)
		return rc;
	else if (rc == 0)
		return -EINVAL; /* This can't happen anymore */
	else if (rc > 0 && rep.type == NLMSG_ERROR) {
		int error = rep.error->error;
		/* Eat the message */
		(void)audit_get_reply(fd, &rep, GET_REPLY_NONBLOCKING, 0);

		/* NLMSG_ERROR can indicate success, only report nonzero */
		if (error) {
			errno = -error;
			return error;
		}
	}
	return 0;
}

int audit_send(int fd, int type, const void *data, unsigned int size)
{
	static int sequence = 0;
	struct audit_message req;
	int retval;
	struct sockaddr_nl addr;

	/* Due to user space library callbacks, there's a chance that
	   a -1 for the fd could be passed. Just check for and handle it. */
	if (fd < 0) {
		errno = EBADF;
		return -errno;
	}

	if (NLMSG_SPACE(size) > MAX_AUDIT_MESSAGE_LENGTH) {
		errno = EINVAL;
		return -errno;
	}

	if (++sequence < 0) 
		sequence = 1;

	memset(&req, 0, sizeof(req));
	req.nlh.nlmsg_len = NLMSG_SPACE(size);
	req.nlh.nlmsg_type = type;
	req.nlh.nlmsg_flags = NLM_F_REQUEST|NLM_F_ACK;
	req.nlh.nlmsg_seq = sequence;
	if (size && data)
		memcpy(NLMSG_DATA(&req.nlh), data, size);
	memset(&addr, 0, sizeof(addr));
	addr.nl_family = AF_NETLINK;
	addr.nl_pid = 0;
	addr.nl_groups = 0;

	do {
		retval = sendto(fd, &req, req.nlh.nlmsg_len, 0,
			(struct sockaddr*)&addr, sizeof(addr));
	} while (retval < 0 && errno == EINTR);
	if (retval == (int)req.nlh.nlmsg_len) {
		if ((retval = check_ack(fd)) == 0)
			return sequence;
		else
			return retval; 
	}
	if (retval < 0) 
		return -errno;

	return 0;
}

int get_reply(int fd, void *data, size_t data_len)
{
	int len;
	struct sockaddr_nl nladdr;
	socklen_t nladdrlen = sizeof(nladdr);

	if (fd < 0)
		return -EBADF;

retry:
	len = recvfrom(fd, data, data_len, 0,
		(struct sockaddr*)&nladdr, &nladdrlen);

	if (len < 0) {
		if (errno == EINTR)
			goto retry;
		return -errno;
	}
	if (nladdrlen != sizeof(nladdr))
		return -EPROTO;
	if (nladdr.nl_pid)
		return -EINVAL;

	return len;
}


[-- Attachment #4: private.h --]
[-- Type: text/x-chdr, Size: 146 bytes --]

int check_ack(int fd);
int audit_send(int fd, int type, const void *data, unsigned int size);
int get_reply(int fd, void *data, size_t data_len);

[-- Attachment #5: test.c --]
[-- Type: text/x-csrc, Size: 1984 bytes --]

#include <libaudit.h>
#include <string.h>
#include <linux/audit.h>
#include <unistd.h>
#include <string.h>
#include <errno.h>
#include <fcntl.h>
#include <time.h>
#include <sys/poll.h>
#include <stdio.h>
#include <stdlib.h>
#include "private.h"

#define UNSET_FEATURE_MASK	AUDIT_FEATURE_TO_MASK(AUDIT_FEATURE_ONLY_UNSET_LOGINUID)
#define IMMUTABLE_FEATURE_MASK	AUDIT_FEATURE_TO_MASK(AUDIT_FEATURE_LOGINUID_IMMUTABLE)

static void print_af(char *prefix, struct audit_features *af)
{
	fprintf(stdout, "%s vers=%d mask=%08x feature=%08x lock=%08x\n", prefix, af->vers, af->mask, af->features, af->lock);
}

static int get_and_print(int fd)
{
	struct audit_features af, *paf;
	char buf[4096];
	int rc;

	/* get the features */
	rc = audit_send(fd, AUDIT_GET_FEATURE, &af, sizeof(af));
	if (rc < 0)
		return rc;

	rc = get_reply(fd, buf, sizeof(buf));
	if (rc < 0)
		return rc;

	paf = NLMSG_DATA(buf);
	print_af("FROM:", paf);
	return 0;
}

static int set_features(int fd, struct audit_features *af)
{
	int rc;

	print_af("TO:", af);
	rc = audit_send(fd, AUDIT_SET_FEATURE, af, sizeof(*af));
	if (rc < 0) {
		perror("audit_send");
		return rc;
	}

	rc = get_and_print(fd);
	if (rc < 0)
		return rc;

	return 0;
}

int main(int argc, char *argv[])
{
	int fd;
	int rc;
	struct audit_features af;
	unsigned int mask = UNSET_FEATURE_MASK | IMMUTABLE_FEATURE_MASK;
	unsigned int features = 0;
	unsigned int lock = 0;

	if (argc < 4) {
		fprintf(stderr, "Dude, gets your args together, unset, immut, lock\n");
		return -EINVAL;
	}
	if (atoi(argv[1]))
		features |= UNSET_FEATURE_MASK;
	if (atoi(argv[2]))
		features |= IMMUTABLE_FEATURE_MASK;
	if (atoi(argv[3]))
		lock = mask;

	fd = audit_open();
	if (fd < 0)
		return fd;

	rc = get_and_print(fd);
	if (rc < 0)
		return rc;

	memset(&af, 0, sizeof(af));
	/* set new features */
	af.vers = AUDIT_FEATURE_VERSION;
	af.mask = mask;
	af.features = features;
	af.lock = lock;

	rc = set_features(fd, &af);
	if (rc < 0)
		return rc;

	return 0;
}

[-- Attachment #6: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/7] audit: implement generic feature setting and retrieving

[William Roberts]

[-- Attachment #1.1: Type: text/plain, Size: 2039 bytes --]

Looking through the patch, these are my thoughts:

I like that my "splitlog" patch shrunk a lot, way easier. I like that. It
also proves something like this is the correct direction.

Shouldn't audit_set_feature() check the version number, granted it doesn't
mater now, but shouldn't their be a:

 if(uaf->version <= AUDIT_FEATURE_SUPPORTED_VERSION)


This branchy code could be:
if (new_feature)
af.features |= feature;
else
af.features &= ~feature;

changed to:
af.features = (af.features & ~feature) | feature

Ie just do a clear bit than or in what you want.

Otherwise LGTM

Bill





On Fri, May 24, 2013 at 9:28 AM, Eric Paris <eparis@redhat.com> wrote:

> On Fri, 2013-05-24 at 12:11 -0400, Eric Paris wrote:
> > The audit_status structure was not designed with extensibility in mind.
> > Define a new AUDIT_SET_FEATURE message type which takes a new structure
> > of bits where things can be enabled/disabled/locked one at a time.  This
> > structure should be able to grow in the future while maintaining forward
> > and backward compatibility (based loosly on the ideas from capabilities
> > and prctl)
> >
> > This does not actually add any features, but is just infrastructure to
> > allow new on/off types of audit system features.
> >
> > Signed-off-by: Eric Paris <eparis@redhat.com>
>
> Attached you will find the test program I used to check that things were
> working correctly.  It should give an idea to Steve how we can program
> the features support in userspace.  I believe it fits very nicely to
> have a new syntax in audit.rules to set (and lock if needed/wanted)
> these features.
>
> netlink.c is just some helper code I stole from the audit tree to get
> some functions which weren't exposed externally.  The only part really
> interesting is test.c.
>
> You will also need the include/uapi/linux/audit.h file from this patch
> to build test.c
>
> -Eric
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
>



-- 
Respectfully,

William C Roberts

[-- Attachment #1.2: Type: text/html, Size: 5884 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/7] audit: implement generic feature setting and retrieving

[William Roberts]

[-- Attachment #1.1: Type: text/plain, Size: 2690 bytes --]

A couple other comments I missed before:

 #define AUDIT_FEATURE_TO_MASK(x)       (1 << ((x) & 31)) /* mask for __u32
*/

Why &31 --> if someone adds more than 5 things, they will git dropped out...

 #define AUDIT_FEATURE_TO_MASK(x)       (1 << ((x) & (~(__u32)0))) /* mask
for __u32 */

something like this perhaps?

Also
static char *audit_feature_names[2] = { ...

Drop the number, I had to inc this when I added mine...

Bill


On Fri, May 24, 2013 at 1:41 PM, William Roberts
<bill.c.roberts@gmail.com>wrote:

> Looking through the patch, these are my thoughts:
>
> I like that my "splitlog" patch shrunk a lot, way easier. I like that. It
> also proves something like this is the correct direction.
>
> Shouldn't audit_set_feature() check the version number, granted it doesn't
> mater now, but shouldn't their be a:
>
>  if(uaf->version <= AUDIT_FEATURE_SUPPORTED_VERSION)
>
>
> This branchy code could be:
> if (new_feature)
> af.features |= feature;
>  else
> af.features &= ~feature;
>
> changed to:
> af.features = (af.features & ~feature) | feature
>
> Ie just do a clear bit than or in what you want.
>
> Otherwise LGTM
>
> Bill
>
>
>
>
>
> On Fri, May 24, 2013 at 9:28 AM, Eric Paris <eparis@redhat.com> wrote:
>
>> On Fri, 2013-05-24 at 12:11 -0400, Eric Paris wrote:
>> > The audit_status structure was not designed with extensibility in mind.
>> > Define a new AUDIT_SET_FEATURE message type which takes a new structure
>> > of bits where things can be enabled/disabled/locked one at a time.  This
>> > structure should be able to grow in the future while maintaining forward
>> > and backward compatibility (based loosly on the ideas from capabilities
>> > and prctl)
>> >
>> > This does not actually add any features, but is just infrastructure to
>> > allow new on/off types of audit system features.
>> >
>> > Signed-off-by: Eric Paris <eparis@redhat.com>
>>
>> Attached you will find the test program I used to check that things were
>> working correctly.  It should give an idea to Steve how we can program
>> the features support in userspace.  I believe it fits very nicely to
>> have a new syntax in audit.rules to set (and lock if needed/wanted)
>> these features.
>>
>> netlink.c is just some helper code I stole from the audit tree to get
>> some functions which weren't exposed externally.  The only part really
>> interesting is test.c.
>>
>> You will also need the include/uapi/linux/audit.h file from this patch
>> to build test.c
>>
>> -Eric
>>
>> --
>> Linux-audit mailing list
>> Linux-audit@redhat.com
>> https://www.redhat.com/mailman/listinfo/linux-audit
>>
>
>
>
> --
> Respectfully,
>
> William C Roberts
>
>


-- 
Respectfully,

William C Roberts

[-- Attachment #1.2: Type: text/html, Size: 7072 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/7] audit: implement generic feature setting and retrieving

[Richard Guy Briggs]

On Fri, May 24, 2013 at 12:11:44PM -0400, Eric Paris wrote:
> The audit_status structure was not designed with extensibility in mind.
> Define a new AUDIT_SET_FEATURE message type which takes a new structure
> of bits where things can be enabled/disabled/locked one at a time.  This
> structure should be able to grow in the future while maintaining forward
> and backward compatibility (based loosly on the ideas from capabilities
> and prctl)
> 
> This does not actually add any features, but is just infrastructure to
> allow new on/off types of audit system features.

This is the sort of infrastructure that occured to me for the
audit_tty_status structure, when I implemented the password logging
switch...

> Signed-off-by: Eric Paris <eparis@redhat.com>

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: 1.647.777.2635
Internal: (81) 32635

Re: [PATCH 1/7] audit: implement generic feature setting and retrieving

[Steve Grubb]

On Friday, May 24, 2013 12:11:44 PM Eric Paris wrote:
> The audit_status structure was not designed with extensibility in mind.
> Define a new AUDIT_SET_FEATURE message type which takes a new structure
> of bits where things can be enabled/disabled/locked one at a time.

This changes how we have been doing things. The way that the audit system 
settings have been done is to use the AUDIT_SET and AUDIT_GET commands. It 
takes a bit map as the function to perform. We have only used 5 of the 32 
bits.

Do we really need another of the same thing?

-Steve


> This
> structure should be able to grow in the future while maintaining forward
> and backward compatibility (based loosly on the ideas from capabilities
> and prctl)
> 
> This does not actually add any features, but is just infrastructure to
> allow new on/off types of audit system features.
> 
> Signed-off-by: Eric Paris <eparis@redhat.com>
> ---
>  include/linux/audit.h      |   2 +
>  include/uapi/linux/audit.h |  16 +++++++
>  kernel/audit.c             | 110
> ++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 127
> insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 729a4d1..7b31bec 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -73,6 +73,8 @@ struct audit_field {
>  	void				*lsm_rule;
>  };
> 
> +extern int is_audit_feature_set(int which);
> +
>  extern int __init audit_register_class(int class, unsigned *list);
>  extern int audit_classify_syscall(int abi, unsigned syscall);
>  extern int audit_classify_arch(int arch);
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index b7cb978..a053243 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -68,6 +68,9 @@
>  #define AUDIT_MAKE_EQUIV	1015	/* Append to watched tree */
>  #define AUDIT_TTY_GET		1016	/* Get TTY auditing status */
>  #define AUDIT_TTY_SET		1017	/* Set TTY auditing status */
> +#define AUDIT_SET_FEATURE	1018	/* Turn an audit feature on or off */
> +#define AUDIT_GET_FEATURE	1019	/* Get which features are enabled */
> +#define AUDIT_FEATURE_CHANGE	1020	/* audit log listing feature changes 
*/
> 
>  #define AUDIT_FIRST_USER_MSG	1100	/* Userspace messages mostly
> uninteresting to kernel */ #define AUDIT_USER_AVC		1107	/* We filter 
this
> differently */
> @@ -369,6 +372,19 @@ struct audit_status {
>  	__u32		backlog;	/* messages waiting in queue */
>  };
> 
> +struct audit_features {
> +#define AUDIT_FEATURE_VERSION	1
> +	__u32	vers;
> +	__u32	mask;		/* which bits we are dealing with */
> +	__u32	features;	/* which feature to enable/disable */
> +	__u32	lock;		/* which features to lock */
> +};
> +
> +#define AUDIT_LAST_FEATURE	-1
> +
> +#define audit_feature_valid(x)		((x) >= 0 && (x) <= AUDIT_LAST_FEATURE)
> +#define AUDIT_FEATURE_TO_MASK(x)	(1 << ((x) & 31)) /* mask for __u32 */
> +
>  struct audit_tty_status {
>  	__u32		enabled;	/* 1 = enabled, 0 = disabled */
>  	__u32		log_passwd;	/* 1 = enabled, 0 = disabled */
> diff --git a/kernel/audit.c b/kernel/audit.c
> index f2f4666..3acbbc8 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -140,6 +140,15 @@ static struct task_struct *kauditd_task;
>  static DECLARE_WAIT_QUEUE_HEAD(kauditd_wait);
>  static DECLARE_WAIT_QUEUE_HEAD(audit_backlog_wait);
> 
> +static struct audit_features af = {.vers = AUDIT_FEATURE_VERSION,
> +				   .mask = -1,
> +				   .features = 0,
> +				   .lock = 0,};
> +
> +static char *audit_feature_names[0] = {
> +};
> +
> +
>  /* Serialize requests from userspace. */
>  DEFINE_MUTEX(audit_cmd_mutex);
> 
> @@ -584,6 +593,8 @@ static int audit_netlink_ok(struct sk_buff *skb, u16
> msg_type) return -EOPNOTSUPP;
>  	case AUDIT_GET:
>  	case AUDIT_SET:
> +	case AUDIT_GET_FEATURE:
> +	case AUDIT_SET_FEATURE:
>  	case AUDIT_LIST_RULES:
>  	case AUDIT_ADD_RULE:
>  	case AUDIT_DEL_RULE:
> @@ -628,6 +639,94 @@ static int audit_log_common_recv_msg(struct
> audit_buffer **ab, u16 msg_type) return rc;
>  }
> 
> +int is_audit_feature_set(int i)
> +{
> +	return af.features & AUDIT_FEATURE_TO_MASK(i);
> +}
> +
> +
> +static int audit_get_feature(struct sk_buff *skb)
> +{
> +	u32 seq;
> +
> +	seq = nlmsg_hdr(skb)->nlmsg_seq;
> +
> +	audit_send_reply(NETLINK_CB(skb).portid, seq, AUDIT_GET, 0, 0,
> +			 &af, sizeof(af));
> +
> +	return 0;
> +}
> +
> +static void audit_log_feature_change(int which, u32 old_feature, u32
> new_feature, +				     u32 old_lock, u32 new_lock, int res)
> +{
> +	struct audit_buffer *ab;
> +
> +	ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE);
> +	audit_log_format(ab, "feature=%s new=%d old=%d old_lock=%d new_lock=%d
> res=%d", +			 audit_feature_names[which], !!old_feature, 
!!new_feature,
> +			 !!old_lock, !!new_lock, res);
> +	audit_log_end(ab);
> +}
> +
> +static int audit_set_feature(struct sk_buff *skb)
> +{
> +	struct audit_features *uaf;
> +	int i;
> +
> +	BUILD_BUG_ON(AUDIT_LAST_FEATURE + 1 >
> sizeof(audit_feature_names)/sizeof(audit_feature_names[0])); +	uaf =
> nlmsg_data(nlmsg_hdr(skb));
> +
> +	/* if there is ever a version 2 we should handle that here */
> +
> +	for (i = 0; i <= AUDIT_LAST_FEATURE; i++) {
> +		u32 feature = AUDIT_FEATURE_TO_MASK(i);
> +		u32 old_feature, new_feature, old_lock, new_lock;
> +
> +		/* if we are not changing this feature, move along */
> +		if (!(feature & uaf->mask))
> +			continue;
> +
> +		old_feature = af.features & feature;
> +		new_feature = uaf->features & feature;
> +		new_lock = (uaf->lock | af.lock) & feature;
> +		old_lock = af.lock & feature;
> +
> +		/* are we changing a locked feature? */
> +		if ((af.lock & feature) && (new_feature != old_feature)) {
> +			audit_log_feature_change(i, old_feature, new_feature,
> +						 old_lock, new_lock, 0);
> +			return -EPERM;
> +		}
> +	}
> +	/* nothing invalid, do the changes */
> +	for (i = 0; i <= AUDIT_LAST_FEATURE; i++) {
> +		u32 feature = AUDIT_FEATURE_TO_MASK(i);
> +		u32 old_feature, new_feature, old_lock, new_lock;
> +
> +		/* if we are not changing this feature, move along */
> +		if (!(feature & uaf->mask))
> +			continue;
> +
> +		old_feature = af.features & feature;
> +		new_feature = uaf->features & feature;
> +		old_lock = af.lock & feature;
> +		new_lock = (uaf->lock | af.lock) & feature;
> +
> +		if (new_feature != old_feature)
> +			audit_log_feature_change(i, old_feature, new_feature,
> +						 old_lock, new_lock, 1);
> +
> +		if (new_feature)
> +			af.features |= feature;
> +		else
> +			af.features &= ~feature;
> +		af.lock |= new_lock;
> +	}
> +
> +	return 0;
> +}
> +
>  static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>  {
>  	u32			seq;
> @@ -699,7 +798,16 @@ static int audit_receive_msg(struct sk_buff *skb,
> struct nlmsghdr *nlh) if (status_get->mask & AUDIT_STATUS_BACKLOG_LIMIT)
>  			err = audit_set_backlog_limit(status_get->backlog_limit);
>  		break;
> -	case AUDIT_USER:
> +	case AUDIT_GET_FEATURE:
> +		err = audit_get_feature(skb);
> +		if (err)
> +			return err;
> +		break;
> +	case AUDIT_SET_FEATURE:
> +		err = audit_set_feature(skb);
> +		if (err)
> +			return err;
> +		break;
>  	case AUDIT_FIRST_USER_MSG ... AUDIT_LAST_USER_MSG:
>  	case AUDIT_FIRST_USER_MSG2 ... AUDIT_LAST_USER_MSG2:
>  		if (!audit_enabled && msg_type != AUDIT_USER_AVC)

Re: [PATCH 7/7] audit: audit feature to set loginuid immutable

[Steve Grubb]

On Friday, May 24, 2013 12:11:50 PM Eric Paris wrote:
> This adds a new 'audit_feature' bit which allows userspace to set it
> such that the loginuid is absolutely immutable, even if you have
> CAP_AUDIT_CONTROL.

I'm also not sure I like it done this way. What I was thinking about is that 
we should set this at boot so that no matter what happens during boot, the 
policy is for setting loginuid cannot be messed with. We really do not want 
this to be changeable after the system comes up. I'd much rather see this as 
audit=4 on the boot prompt (meaning enabled and immutable). This way its clear 
to everyone that it can only be changed by rebooting the system and the policy 
is in effect for the duration of the session.

-Steve


> Signed-off-by: Eric Paris <eparis@redhat.com>
> ---
>  include/uapi/linux/audit.h | 3 ++-
>  kernel/audit.c             | 3 ++-
>  kernel/auditsc.c           | 3 +++
>  3 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 2963b5a..9539ea9 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -381,7 +381,8 @@ struct audit_features {
>  };
> 
>  #define AUDIT_FEATURE_ONLY_UNSET_LOGINUID	0
> -#define AUDIT_LAST_FEATURE			AUDIT_FEATURE_ONLY_UNSET_LOGINUID
> +#define AUDIT_FEATURE_LOGINUID_IMMUTABLE	1
> +#define AUDIT_LAST_FEATURE			AUDIT_FEATURE_LOGINUID_IMMUTABLE
> 
>  #define audit_feature_valid(x)		((x) >= 0 && (x) <= AUDIT_LAST_FEATURE)
>  #define AUDIT_FEATURE_TO_MASK(x)	(1 << ((x) & 31)) /* mask for __u32 */
> diff --git a/kernel/audit.c b/kernel/audit.c
> index a5c470b..900d61d 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -145,8 +145,9 @@ static struct audit_features af = {.vers =
> AUDIT_FEATURE_VERSION, .features = 0,
>  				   .lock = 0,};
> 
> -static char *audit_feature_names[1] = {
> +static char *audit_feature_names[2] = {
>  	"only_unset_loginuid",
> +	"loginuid_immutable",
>  };
> 
> 
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index e5dbbc6..aace3ac 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1969,6 +1969,9 @@ static int audit_set_loginuid_perm(kuid_t loginuid)
>  	/* if we are unset, we don't need privs */
>  	if (!audit_loginuid_set(current))
>  		return 0;
> +	/* if AUDIT_FEATURE_LOGINUID_IMMUTABLE means never ever allow a change*/
> +	if (is_audit_feature_set(AUDIT_FEATURE_LOGINUID_IMMUTABLE))
> +		return -EPERM;
>  	/* it is set, you need permission */
>  	if (!capable(CAP_AUDIT_CONTROL))
>  		return -EPERM;

Re: [PATCH 7/7] audit: audit feature to set loginuid immutable

[Eric Paris]

On Mon, 2013-07-08 at 16:34 -0400, Steve Grubb wrote:
> On Friday, May 24, 2013 12:11:50 PM Eric Paris wrote:
> > This adds a new 'audit_feature' bit which allows userspace to set it
> > such that the loginuid is absolutely immutable, even if you have
> > CAP_AUDIT_CONTROL.
> 
> I'm also not sure I like it done this way. What I was thinking about is that 
> we should set this at boot so that no matter what happens during boot, the 
> policy is for setting loginuid cannot be messed with. We really do not want 
> this to be changeable after the system comes up. I'd much rather see this as 
> audit=4 on the boot prompt (meaning enabled and immutable). This way its clear 
> to everyone that it can only be changed by rebooting the system and the policy 
> is in effect for the duration of the session.

Linus has explicitly said the kernel command line options are only
acceptable if they are required for kernel functionality before they can
be set by userspace.  If we don't trust the audit system initialization
we already lost and no amount of audit= is going to change that.  Since
there is absolutely no benefit to setting this on the kernel command
line, before we can parse and use the audit.rules, I can not make this
yet another archaic command line option.  I'm sorry, but this just
cannot happen...

Re: [PATCH 7/7] audit: audit feature to set loginuid immutable

[Steve Grubb]

On Monday, July 08, 2013 04:51:20 PM Eric Paris wrote:
> On Mon, 2013-07-08 at 16:34 -0400, Steve Grubb wrote:
> > On Friday, May 24, 2013 12:11:50 PM Eric Paris wrote:
> > > This adds a new 'audit_feature' bit which allows userspace to set it
> > > such that the loginuid is absolutely immutable, even if you have
> > > CAP_AUDIT_CONTROL.
> > 
> > I'm also not sure I like it done this way. What I was thinking about is
> > that we should set this at boot so that no matter what happens during
> > boot, the policy is for setting loginuid cannot be messed with. We really
> > do not want this to be changeable after the system comes up. I'd much
> > rather see this as audit=4 on the boot prompt (meaning enabled and
> > immutable). This way its clear to everyone that it can only be changed by
> > rebooting the system and the policy is in effect for the duration of the
> > session.
> 
> Linus has explicitly said the kernel command line options are only
> acceptable if they are required for kernel functionality before they can
> be set by userspace.

I'd say that is the case. If we are going to have immutable loginuid, we don't 
want some processes running under one policy and then others later under 
another policy. Besides, this isn't adding a boot command. Its already there.


> If we don't trust the audit system initialization we already lost and no
> amount of audit= is going to change that.  

I'm thinking more about High Assurance cases where the boot 
partition/environment is removed from an attacker's reach. Consider use cases 
where you want to allow root, but you do not want them to make certain kinds 
of changes to the system by taking away certain capabilities in the initramfs 
which is outside of the control of anyone with root.

-Steve

Re: [PATCH 7/7] audit: audit feature to set loginuid immutable

[Eric Paris]

On Mon, 2013-07-08 at 17:26 -0400, Steve Grubb wrote:
> On Monday, July 08, 2013 04:51:20 PM Eric Paris wrote:

> > If we don't trust the audit system initialization we already lost and no
> > amount of audit= is going to change that.  
> 
> I'm thinking more about High Assurance cases where the boot 
> partition/environment is removed from an attacker's reach. Consider use cases 
> where you want to allow root, but you do not want them to make certain kinds 
> of changes to the system by taking away certain capabilities in the initramfs 
> which is outside of the control of anyone with root.

If that's the case, you must be loading the audit policy inside the
initramfs, and thus, you can set this inside the initrd.  We MUST have
absolute trust until the audit.rules are processed.  To get a boot
option, we have to show how this has value before the audit.rules are
loaded.  And it doesn't...  Not in any system I can imagine.  Nor in the
description you gave above...  What am I missing?

Re: [PATCH 1/7] audit: implement generic feature setting and retrieving

[Eric Paris]

On Mon, 2013-07-08 at 16:28 -0400, Steve Grubb wrote:
> On Friday, May 24, 2013 12:11:44 PM Eric Paris wrote:
> > The audit_status structure was not designed with extensibility in mind.
> > Define a new AUDIT_SET_FEATURE message type which takes a new structure
> > of bits where things can be enabled/disabled/locked one at a time.
> 
> This changes how we have been doing things. The way that the audit system 
> settings have been done is to use the AUDIT_SET and AUDIT_GET commands. It 
> takes a bit map as the function to perform. We have only used 5 of the 32 
> bits.
> 
> Do we really need another of the same thing?

It's not the same thing.  This is an interface designed for options
which have 4 states.  On/Off and Locked/Unlocked.  It is certainly the
right solution for that problem if we want to solve it generically.
(look at what it did to the other code who wanted an on/off option)

AUDIT_SET/GET was designed around setting a kernel variable to a single
value.  It does an ok job at this (although I'd argue that there could
be a better design here as well, but we have this, so we live with it.)
It certainly does not form naturally to the 4 states of the new
interface.

I can certainly shoehorn a 4 state interface into AUDIT_SET/GET.  But I
believe it will be less obvious and less efficient code.  Maybe that
doesn't matter too much since only you have to deal with it and it isn't
run particularly often but I don't see a reason to solve a problem
inefficiently.  No matter what we are going to have a new design
pattern, so why not a natural pattern rather than one we are forcing to
needlessly conform?

Let's say we do want to force ourselves to use the AUDIT_SET/GET netlink
message.  What do you think the interface should look like?

I can just do the exact same thing inside AUDIT_SET/GET where I
implement a usable bitmask of on/off/lockable options.  It'd be
basically the exact same thing as I have now, just more multiplexing
over the GET/SET message.  Certainly no better.

I could add two new audit status bits, AUDIT_STATUS_LOGINUID_IMMUTABLE
and AUDIT_STATUS_LOGINUID_IMMUTABLE_LOCKED, along with two __u8's to
struct audit_status.  One __u8 could be loginuid_immutable and the other
loginuid_immutable_locked.  I guess that keeps us in line with the
GET/SET mentality.  I don't see a benefit, but I am biased towards my
own code.

I could also do it as one __u8 and have magic meaning to the bits inside
the __u8, aka, 
0 == off unlocked
1 == on unlocked
2 == off locked
3 == on locked
but that seems to needlessly make the code more indecipherable to
humans.  So I won't do it (I see audit_panic and audit_enabled as
examples of this poor design pattern)

What do you think it should look like and why is it better than mine
which has proven quite nice for multiple people?

-Eric

Re: [PATCH 1/7] audit: implement generic feature setting and retrieving

[William Roberts]

[-- Attachment #1.1: Type: text/plain, Size: 3726 bytes --]

On Mon, Jul 8, 2013 at 2:55 PM, Eric Paris <eparis@redhat.com> wrote:

> On Mon, 2013-07-08 at 16:28 -0400, Steve Grubb wrote:
> > On Friday, May 24, 2013 12:11:44 PM Eric Paris wrote:
> > > The audit_status structure was not designed with extensibility in mind.
> > > Define a new AUDIT_SET_FEATURE message type which takes a new structure
> > > of bits where things can be enabled/disabled/locked one at a time.
> >
> > This changes how we have been doing things. The way that the audit system
> > settings have been done is to use the AUDIT_SET and AUDIT_GET commands.
> It
> > takes a bit map as the function to perform. We have only used 5 of the 32
> > bits.
> >
> > Do we really need another of the same thing?
>
> It's not the same thing.  This is an interface designed for options
> which have 4 states.  On/Off and Locked/Unlocked.  It is certainly the
> right solution for that problem if we want to solve it generically.
> (look at what it did to the other code who wanted an on/off option)
>
> AUDIT_SET/GET was designed around setting a kernel variable to a single
> value.  It does an ok job at this (although I'd argue that there could
> be a better design here as well, but we have this, so we live with it.)
> It certainly does not form naturally to the 4 states of the new
> interface.
>
> I can certainly shoehorn a 4 state interface into AUDIT_SET/GET.  But I
> believe it will be less obvious and less efficient code.  Maybe that
> doesn't matter too much since only you have to deal with it and it isn't
> run particularly often but I don't see a reason to solve a problem
> inefficiently.  No matter what we are going to have a new design
> pattern, so why not a natural pattern rather than one we are forcing to
> needlessly conform?
>
> Let's say we do want to force ourselves to use the AUDIT_SET/GET netlink
> message.  What do you think the interface should look like?
>
> I can just do the exact same thing inside AUDIT_SET/GET where I
> implement a usable bitmask of on/off/lockable options.  It'd be
> basically the exact same thing as I have now, just more multiplexing
> over the GET/SET message.  Certainly no better.
>
> I could add two new audit status bits, AUDIT_STATUS_LOGINUID_IMMUTABLE
> and AUDIT_STATUS_LOGINUID_IMMUTABLE_LOCKED, along with two __u8's to
> struct audit_status.  One __u8 could be loginuid_immutable and the other
> loginuid_immutable_locked.  I guess that keeps us in line with the
> GET/SET mentality.  I don't see a benefit, but I am biased towards my
> own code.
>
> I could also do it as one __u8 and have magic meaning to the bits inside
> the __u8, aka,
> 0 == off unlocked
> 1 == on unlocked
> 2 == off locked
> 3 == on locked
> but that seems to needlessly make the code more indecipherable to
> humans.  So I won't do it (I see audit_panic and audit_enabled as
> examples of this poor design pattern)
>
> What do you think it should look like and why is it better than mine
> which has proven quite nice for multiple people?
>
> <snip>

I had some code that I submitted to Eric on top of his generic feature
set/get patch, and reduced my change set to 8 lines. The nicest parts of it
is that it is bitpacked, so we wont really need to bump the revision of the
struct up for anything in the near future. AUDIT_SET/GET are unversioned
(if i remember right) so their is no way to add capability to it in a
portable sense. No matter what, it would be nice to get everything new
versioned, their are a bazillion flags in the switch, and its annoying.
Also, in my case, if I can avoid having to add a new flag, it just makes
enhancing the audit subsystem way easier. It felt dirty adding a new case
to the switch...

Bill



-- 
Respectfully,

William C Roberts

[-- Attachment #1.2: Type: text/html, Size: 4427 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/7] audit: implement generic feature setting and retrieving

[Steve Grubb]

On Monday, July 08, 2013 05:55:07 PM Eric Paris wrote:
> On Mon, 2013-07-08 at 16:28 -0400, Steve Grubb wrote:
> > On Friday, May 24, 2013 12:11:44 PM Eric Paris wrote:
> > > The audit_status structure was not designed with extensibility in mind.
> > > Define a new AUDIT_SET_FEATURE message type which takes a new structure
> > > of bits where things can be enabled/disabled/locked one at a time.
> > 
> > This changes how we have been doing things. The way that the audit system
> > settings have been done is to use the AUDIT_SET and AUDIT_GET commands. It
> > takes a bit map as the function to perform. We have only used 5 of the 32
> > bits.
> > 
> > Do we really need another of the same thing?
> 
> It's not the same thing.  This is an interface designed for options
> which have 4 states.  On/Off and Locked/Unlocked.  It is certainly the
> right solution for that problem if we want to solve it generically.
> (look at what it did to the other code who wanted an on/off option)
> 
> AUDIT_SET/GET was designed around setting a kernel variable to a single
> value.  It does an ok job at this (although I'd argue that there could
> be a better design here as well, but we have this, so we live with it.)
> It certainly does not form naturally to the 4 states of the new
> interface.

I did some more digging. I guess the GET/SET interface is limited. Setting 
values could be done by reusing one of the places in the struct, but then 
getting the values would be a problem.

So, how is user space supposed to detect that the kernel supports this 
interface? What I have needed for years is a way to ask the kernel what 
features it currently contains. For example, if you try to use interfield 
comparisons and the kernel doesn't support it, I get an EINVAL and bounce that 
to the user. What would be better is if I could ask the kernel what features 
it contains and then I can not send the interfield comparison but output a 
message saying the current kernel does not support this feature.


> I can certainly shoehorn a 4 state interface into AUDIT_SET/GET. 

Does the new interface support more than 4 a state variable? Suppose we need 
to set a number value like 8192, will it do that?

-Steve

Re: [PATCH 1/7] audit: implement generic feature setting and retrieving

[Eric Paris]

On Tue, 2013-07-09 at 14:30 -0400, Steve Grubb wrote:
> > I can certainly shoehorn a 4 state interface into AUDIT_SET/GET. 
> 
> Does the new interface support more than 4 a state variable? Suppose
> we need 
> to set a number value like 8192, will it do that?

No.  The new interface is written to be on/off locked/unlock

The get/set interface could be extended to allow for this.  We'd have to
grow the size of struct audit_status with a new __u32.  Kernel space
would have to 0 out the struct and overwrite it with what it got from
userspace.  Userspace would just have to ignore the additional info from
a read...

I agree, a version field is useful. 

Re: [PATCH 1/7] audit: implement generic feature setting and retrieving

[Steve Grubb]

On Friday, May 24, 2013 12:11:44 PM Eric Paris wrote:
> +static void audit_log_feature_change(int which, u32 old_feature, u32
> new_feature, +				     u32 old_lock, u32 new_lock, int res)
> +{
> +	struct audit_buffer *ab;
> +
> +	ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE);
> +	audit_log_format(ab, "feature=%s new=%d old=%d old_lock=%d new_lock=%d
> res=%d", +			 audit_feature_names[which], !!old_feature, 
!!new_feature,
> +			 !!old_lock, !!new_lock, res);
> +	audit_log_end(ab);
> +}

Shouldn't we be recording all the subjecting information? The above would be 
the object and results. But we need the "who" part.

-Steve

Re: [PATCH 7/7] audit: audit feature to set loginuid immutable

[Steve Grubb]

On Monday, July 08, 2013 05:32:31 PM Eric Paris wrote:
> On Mon, 2013-07-08 at 17:26 -0400, Steve Grubb wrote:
> > On Monday, July 08, 2013 04:51:20 PM Eric Paris wrote:
> > > If we don't trust the audit system initialization we already lost and no
> > > amount of audit= is going to change that.
> > 
> > I'm thinking more about High Assurance cases where the boot
> > partition/environment is removed from an attacker's reach. Consider use
> > cases where you want to allow root, but you do not want them to make
> > certain kinds of changes to the system by taking away certain
> > capabilities in the initramfs which is outside of the control of anyone
> > with root.
> 
> If that's the case, you must be loading the audit policy inside the
> initramfs, and thus, you can set this inside the initrd.

I don't think anyone has plans to write those tools at the moment. That would 
be ideal. But even in the case where audit rules don't get loaded, there are 
audit events generated by the MAC systems and some hard coded kernel events 
and user space events. It would be nice to know they are not tampered with.


> We MUST have absolute trust until the audit.rules are processed.  To get a
> boot option, we have to show how this has value before the audit.rules are
> loaded.  And it doesn't...  Not in any system I can imagine.  Nor in the
> description you gave above...  What am I missing?

I know you want to treat this like SE Linux where audit rules = selinux 
policy. SE Linux policy is tightly integrated to the boot process. I'd bet 
that if selinux was enabled and loading failed, the system halts. It also 
happens very early before other daemons run. The audit system isn't designed 
this way. Rules can fail to load and the system still comes up. I'd like to at 
least have a way to prevent tampering with loginuid should someone discover a 
system like this.

-Steve

Re: [PATCH 7/7] audit: audit feature to set loginuid immutable

[LC Bruzenak]

On 07/09/2013 05:24 PM, Steve Grubb wrote

...
I don't think anyone has plans to write those tools at the moment. That would 
be ideal. But even in the case where audit rules don't get loaded, there are 
audit events generated by the MAC systems and some hard coded kernel events 
and user space events. It would be nice to know they are not tampered with.
...


Question - from the title I had thought this was a good thing. But wasn't loginuid (and subsequently auid) already immutable?
Sorry; just not certain what this change does for the average guy...

Thx,
LCB

-- 
LC (Lenny) Bruzenak
lenny@magitekltd.com

Re: [PATCH 7/7] audit: audit feature to set loginuid immutable

[Steve Grubb]

On Tuesday, July 09, 2013 06:51:43 PM LC Bruzenak wrote:
> On 07/09/2013 05:24 PM, Steve Grubb wrote
> 
> ...
> I don't think anyone has plans to write those tools at the moment. That
> would be ideal. But even in the case where audit rules don't get loaded,
> there are audit events generated by the MAC systems and some hard coded
> kernel events and user space events. It would be nice to know they are not
> tampered with. ...
> 
> 
> Question - from the title I had thought this was a good thing. But wasn't
> loginuid (and subsequently auid) already immutable? Sorry; just not certain
> what this change does for the average guy...

Currently its a compile time option. This means when its selected the auid is 
immutable and you have a strong assurance argument that any action by the 
subject really is the subject's account. Strong assurance may be required for 
high assurance deployments. It would be more solid standing up in court as 
well because the argument can be made that whatever action occurred can be 
attributed to the subject because there is no way to change it. Its tamper-
proof.

The change means the default policy will now allow process with 
CAP_AUDIT_CONTROL to change the auid to anything at anytime and then perform 
actions which would be attributed to another user. There is an event logged on 
setting the loginuid, so it could be considered tamper-evident. At least as 
long as the event's not filtered or erased.

My preference is that we have a way that we can put the system into the 
immutable mode in a way that leaves no doubts about whether the system has 
operated under the same policy from beginning to end.

-Steve

Re: [PATCH 7/7] audit: audit feature to set loginuid immutable

[LC Bruzenak]

On 07/10/2013 08:46 AM, Steve Grubb wrote:
>
> Currently its a compile time option. This means when its selected the auid is 
> immutable and you have a strong assurance argument that any action by the 
> subject really is the subject's account. Strong assurance may be required for 
> high assurance deployments. It would be more solid standing up in court as 
> well because the argument can be made that whatever action occurred can be 
> attributed to the subject because there is no way to change it. Its tamper-
> proof.
That was my understanding.
>
> The change means the default policy will now allow process with 
> CAP_AUDIT_CONTROL to change the auid to anything at anytime and then perform 
> actions which would be attributed to another user. There is an event logged on 
> setting the loginuid, so it could be considered tamper-evident. At least as 
> long as the event's not filtered or erased.
This sounds dangerous. Why would we want to allow this?
>
> My preference is that we have a way that we can put the system into the 
> immutable mode in a way that leaves no doubts about whether the system has 
> operated under the same policy from beginning to end.
That is a better way.

>From an end user perspective I can tell you that although we strive to
be diligent, the reality of reduced budgets and multi-tasking security
managers means that tamper-proof (at least as a near-to-mid-term goal)
is desired over tamper-evident.
Even if the event is not erased or filtered it means another requirement
levied on a person which I do not believe existed before.

Thanks for the info, Steve. I appreciate it!
LCB

-- 
LC (Lenny) Bruzenak
lenny@magitekltd.com

Re: [PATCH 7/7] audit: audit feature to set loginuid immutable

[Eric Paris]

On Wed, 2013-07-10 at 09:32 -0500, LC Bruzenak wrote:
> On 07/10/2013 08:46 AM, Steve Grubb wrote:
> >
> > Currently its a compile time option. This means when its selected the auid is 
> > immutable and you have a strong assurance argument that any action by the 
> > subject really is the subject's account. Strong assurance may be required for 
> > high assurance deployments. It would be more solid standing up in court as 
> > well because the argument can be made that whatever action occurred can be 
> > attributed to the subject because there is no way to change it. Its tamper-
> > proof.
> That was my understanding.
> >
> > The change means the default policy will now allow process with 
> > CAP_AUDIT_CONTROL to change the auid to anything at anytime and then perform 
> > actions which would be attributed to another user. There is an event logged on 
> > setting the loginuid, so it could be considered tamper-evident. At least as 
> > long as the event's not filtered or erased.
> This sounds dangerous. Why would we want to allow this?

Immutability was first introduced in kernel 3.3.  It wasn't enabled in
the kernel config for Fedora until some time much later.  It is not
present in any enterprise distro that I know of.

Before systemd immutability was not possible.  If an admin logged in and
restarted the sshd daemon the daemon would be running as the admin's
loginuid.  When a new user tried to log in via ssh it would need to
change the loginuid from the admin loginuid to their new loginuid.  We
had to give sshd CAP_AUDIT_CONTROL in order to switch the loginuid.
(ALL loginuid changes required CAP_AUDIT_CONTROL)

When systemd came out I added immutability.  Since restarting sshd in a
systemd world is done by init, which has no loginuid, and thus the new
sshd would have no loginuid.  Thus a user logging in would be able to
set a new loginuid without any permissions.

We learned that admins, for various reasons, really do want to be able
to launch daemons by hand and not via init/systemd.  In particular, we
know of many people who launch containers by hand which allows some form
of login (usually sshd).  With the current loginuid immutable code those
people are UNABLE to log in inside the container.

This patch series allows a privileged task (one with CAP_AUDIT_CONTROL)
the ability to unset their own loginuid.  I allows behavior similar to
the pre 3.3 kernels.  Big different being that the privilege is needed
in a helper to UNSET the loginuid, not in the network facing daemon
(ssh) to SET the loginuid.  The series also allows the 'unsetting'
feature to be disabled and locked so it cannot ever be enabled.

Whew, so now we have background.  Now we can talk about the tamper-proof
vs tamper-evident discussion.  I do not buy tamper-proof at all.  If the
user has CAP_AUDIT_CONTROL claiming tamper-proof-ness is a lie.  They
can auditctl -e0 and win.  Ah, but one might retort, what is someone ran
auditctl -e2.  And I would retort, but good sir, if you ran auditctl -e2
to stop CAP_AUDIT_CONTROL from completely owning the box, why didn't you
set the loginuid immutable first?

Thus far every statement that I have seen about loginuid_immutable being
necessary on the kernel command line does not stand up to scrutiny...

-Eric

Re: [PATCH 7/7] audit: audit feature to set loginuid immutable

[LC Bruzenak]

On 07/10/2013 01:16 PM, Eric Paris wrote:
>> > This sounds dangerous. Why would we want to allow this?
> Immutability was first introduced in kernel 3.3.  It wasn't enabled in
> the kernel config for Fedora until some time much later.  It is not
> present in any enterprise distro that I know of.
>
> Before systemd immutability was not possible.  If an admin logged in and
> restarted the sshd daemon the daemon would be running as the admin's
> loginuid.  When a new user tried to log in via ssh it would need to
> change the loginuid from the admin loginuid to their new loginuid.  We
> had to give sshd CAP_AUDIT_CONTROL in order to switch the loginuid.
> (ALL loginuid changes required CAP_AUDIT_CONTROL)
>
> When systemd came out I added immutability.  Since restarting sshd in a
> systemd world is done by init, which has no loginuid, and thus the new
> sshd would have no loginuid.  Thus a user logging in would be able to
> set a new loginuid without any permissions.
>
> We learned that admins, for various reasons, really do want to be able
> to launch daemons by hand and not via init/systemd.  In particular, we
> know of many people who launch containers by hand which allows some form
> of login (usually sshd).  With the current loginuid immutable code those
> people are UNABLE to log in inside the container.
>
> This patch series allows a privileged task (one with CAP_AUDIT_CONTROL)
> the ability to unset their own loginuid.  I allows behavior similar to
> the pre 3.3 kernels.  Big different being that the privilege is needed
> in a helper to UNSET the loginuid, not in the network facing daemon
> (ssh) to SET the loginuid.  The series also allows the 'unsetting'
> feature to be disabled and locked so it cannot ever be enabled.

Thank you for these details; I really appreciate it.

As far as restarting daemons - I guess in theory this obviates the
"run_init" command?
Or only the uid part of the context?

And by breaking apart the unsetting (privileged helper) with the setting
(daemon itself) this is more securely accomplished?

Chewing on this a bit...regardless, thanks again for the details; it
helps tremendously.

LCB

-- 
LC (Lenny) Bruzenak
lenny@magitekltd.com

Re: [PATCH 7/7] audit: audit feature to set loginuid immutable

[LC Bruzenak]

Eric,

NVM on this part.
I forget not everyone lives in a SElinux world...
> As far as restarting daemons - I guess in theory this obviates the
> "run_init" command?
> Or only the uid part of the context?
Thx,
LCB

-- 
LC (Lenny) Bruzenak
lenny@magitekltd.com

Re: [PATCH 7/7] audit: audit feature to set loginuid immutable

[Eric Paris]

On Wed, 2013-07-10 at 13:51 -0500, LC Bruzenak wrote:
> On 07/10/2013 01:16 PM, Eric Paris wrote:
> >> > This sounds dangerous. Why would we want to allow this?
> > Immutability was first introduced in kernel 3.3.  It wasn't enabled in
> > the kernel config for Fedora until some time much later.  It is not
> > present in any enterprise distro that I know of.
> >
> > Before systemd immutability was not possible.  If an admin logged in and
> > restarted the sshd daemon the daemon would be running as the admin's
> > loginuid.  When a new user tried to log in via ssh it would need to
> > change the loginuid from the admin loginuid to their new loginuid.  We
> > had to give sshd CAP_AUDIT_CONTROL in order to switch the loginuid.
> > (ALL loginuid changes required CAP_AUDIT_CONTROL)
> >
> > When systemd came out I added immutability.  Since restarting sshd in a
> > systemd world is done by init, which has no loginuid, and thus the new
> > sshd would have no loginuid.  Thus a user logging in would be able to
> > set a new loginuid without any permissions.
> >
> > We learned that admins, for various reasons, really do want to be able
> > to launch daemons by hand and not via init/systemd.  In particular, we
> > know of many people who launch containers by hand which allows some form
> > of login (usually sshd).  With the current loginuid immutable code those
> > people are UNABLE to log in inside the container.
> >
> > This patch series allows a privileged task (one with CAP_AUDIT_CONTROL)
> > the ability to unset their own loginuid.  I allows behavior similar to
> > the pre 3.3 kernels.  Big different being that the privilege is needed
> > in a helper to UNSET the loginuid, not in the network facing daemon
> > (ssh) to SET the loginuid.  The series also allows the 'unsetting'
> > feature to be disabled and locked so it cannot ever be enabled.
> 
> Thank you for these details; I really appreciate it.
> 
> As far as restarting daemons - I guess in theory this obviates the
> "run_init" command?
> Or only the uid part of the context?

Correct.  "systemctl start httpd.service" or "service httpd
start" (which just maps to the systemctl command) no longer need
run_init to deal with the SELinux context of init vs sysadm/unconfined.

> And by breaking apart the unsetting (privileged helper) with the setting
> (daemon itself) this is more securely accomplished?

I believe so.  In the RHEL6 way of handling loginuid, we would have to
give CAP_AUDIT_CONTROL inside the container, since sshd inside the
container would need it to allow users to log in (mind you RHEL6 doesn't
have containers, but still)

In this way the helper which creates the container could be given the
priv and CAP_AUDIT_CONTROL would never be granted inside the container
at all.  I believe it's a better design any time network facing daemons
have less privilege.

> Chewing on this a bit...regardless, thanks again for the details; it
> helps tremendously.

Re: [PATCH 1/7] audit: implement generic feature setting and retrieving

[Richard Guy Briggs]

On Fri, May 24, 2013 at 12:11:44PM -0400, Eric Paris wrote:
> The audit_status structure was not designed with extensibility in mind.
> Define a new AUDIT_SET_FEATURE message type which takes a new structure
> of bits where things can be enabled/disabled/locked one at a time.  This
> structure should be able to grow in the future while maintaining forward
> and backward compatibility (based loosly on the ideas from capabilities
> and prctl)
> 
> This does not actually add any features, but is just infrastructure to
> allow new on/off types of audit system features.

However, it does surprisingly disable one!

> diff --git a/kernel/audit.c b/kernel/audit.c
> index f2f4666..3acbbc8 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -699,7 +798,16 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>  		if (status_get->mask & AUDIT_STATUS_BACKLOG_LIMIT)
>  			err = audit_set_backlog_limit(status_get->backlog_limit);
>  		break;
> -	case AUDIT_USER:
> +	case AUDIT_GET_FEATURE:
> +		err = audit_get_feature(skb);
> +		if (err)
> +			return err;
> +		break;
> +	case AUDIT_SET_FEATURE:
> +		err = audit_set_feature(skb);
> +		if (err)
> +			return err;
> +		break;
>  	case AUDIT_FIRST_USER_MSG ... AUDIT_LAST_USER_MSG:
>  	case AUDIT_FIRST_USER_MSG2 ... AUDIT_LAST_USER_MSG2:
>  		if (!audit_enabled && msg_type != AUDIT_USER_AVC)

Can I assume that the removal of the AUDIT_USER case line was
accidental?  It has broken USER type AUDIT messages.


- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

Re: [PATCH 1/7] audit: implement generic feature setting and retrieving

[Eric Paris]

On Sat, 2013-11-02 at 03:26 -0400, Richard Guy Briggs wrote:
> On Fri, May 24, 2013 at 12:11:44PM -0400, Eric Paris wrote:
> > The audit_status structure was not designed with extensibility in mind.
> > Define a new AUDIT_SET_FEATURE message type which takes a new structure
> > of bits where things can be enabled/disabled/locked one at a time.  This
> > structure should be able to grow in the future while maintaining forward
> > and backward compatibility (based loosly on the ideas from capabilities
> > and prctl)
> > 
> > This does not actually add any features, but is just infrastructure to
> > allow new on/off types of audit system features.
> 
> However, it does surprisingly disable one!
> 
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index f2f4666..3acbbc8 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -699,7 +798,16 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> >  		if (status_get->mask & AUDIT_STATUS_BACKLOG_LIMIT)
> >  			err = audit_set_backlog_limit(status_get->backlog_limit);
> >  		break;
> > -	case AUDIT_USER:
> > +	case AUDIT_GET_FEATURE:
> > +		err = audit_get_feature(skb);
> > +		if (err)
> > +			return err;
> > +		break;
> > +	case AUDIT_SET_FEATURE:
> > +		err = audit_set_feature(skb);
> > +		if (err)
> > +			return err;
> > +		break;
> >  	case AUDIT_FIRST_USER_MSG ... AUDIT_LAST_USER_MSG:
> >  	case AUDIT_FIRST_USER_MSG2 ... AUDIT_LAST_USER_MSG2:
> >  		if (!audit_enabled && msg_type != AUDIT_USER_AVC)
> 
> Can I assume that the removal of the AUDIT_USER case line was
> accidental?  It has broken USER type AUDIT messages.

Wow, Bad Eric.  Bad.  Please fix!

Re: [PATCH 1/7] audit: implement generic feature setting and retrieving

[Steve Grubb]

Hello,

Just spent some time debugging auditctl, it was doing something I thought was 
weird. I tracked it down to this patch, see below for commentary...

On Friday, May 24, 2013 12:11:44 PM Eric Paris wrote:
> The audit_status structure was not designed with extensibility in mind.
> Define a new AUDIT_SET_FEATURE message type which takes a new structure
> of bits where things can be enabled/disabled/locked one at a time.  This
> structure should be able to grow in the future while maintaining forward
> and backward compatibility (based loosly on the ideas from capabilities
> and prctl)
> 
> This does not actually add any features, but is just infrastructure to
> allow new on/off types of audit system features.
> 
> Signed-off-by: Eric Paris <eparis@redhat.com>
> ---
>  include/linux/audit.h      |   2 +
>  include/uapi/linux/audit.h |  16 +++++++
>  kernel/audit.c             | 110
> ++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 127
> insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 729a4d1..7b31bec 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -73,6 +73,8 @@ struct audit_field {
>  	void				*lsm_rule;
>  };
> 
> +extern int is_audit_feature_set(int which);
> +
>  extern int __init audit_register_class(int class, unsigned *list);
>  extern int audit_classify_syscall(int abi, unsigned syscall);
>  extern int audit_classify_arch(int arch);
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index b7cb978..a053243 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -68,6 +68,9 @@
>  #define AUDIT_MAKE_EQUIV	1015	/* Append to watched tree */
>  #define AUDIT_TTY_GET		1016	/* Get TTY auditing status */
>  #define AUDIT_TTY_SET		1017	/* Set TTY auditing status */
> +#define AUDIT_SET_FEATURE	1018	/* Turn an audit feature on or off */
> +#define AUDIT_GET_FEATURE	1019	/* Get which features are enabled */
> +#define AUDIT_FEATURE_CHANGE	1020	/* audit log listing feature changes 
*/
> 
>  #define AUDIT_FIRST_USER_MSG	1100	/* Userspace messages mostly
> uninteresting to kernel */ #define AUDIT_USER_AVC		1107	/* We 
filter this
> differently */
> @@ -369,6 +372,19 @@ struct audit_status {
>  	__u32		backlog;	/* messages waiting in queue */
>  };
> 
> +struct audit_features {
> +#define AUDIT_FEATURE_VERSION	1
> +	__u32	vers;
> +	__u32	mask;		/* which bits we are dealing with */
> +	__u32	features;	/* which feature to enable/disable */
> +	__u32	lock;		/* which features to lock */
> +};
> +
> +#define AUDIT_LAST_FEATURE	-1
> +
> +#define audit_feature_valid(x)		((x) >= 0 && (x) <= 
AUDIT_LAST_FEATURE)
> +#define AUDIT_FEATURE_TO_MASK(x)	(1 << ((x) & 31)) /* mask for __u32 */
> +
>  struct audit_tty_status {
>  	__u32		enabled;	/* 1 = enabled, 0 = disabled */
>  	__u32		log_passwd;	/* 1 = enabled, 0 = disabled */
> diff --git a/kernel/audit.c b/kernel/audit.c
> index f2f4666..3acbbc8 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -140,6 +140,15 @@ static struct task_struct *kauditd_task;
>  static DECLARE_WAIT_QUEUE_HEAD(kauditd_wait);
>  static DECLARE_WAIT_QUEUE_HEAD(audit_backlog_wait);
> 
> +static struct audit_features af = {.vers = AUDIT_FEATURE_VERSION,
> +				   .mask = -1,
> +				   .features = 0,
> +				   .lock = 0,};
> +
> +static char *audit_feature_names[0] = {
> +};
> +
> +
>  /* Serialize requests from userspace. */
>  DEFINE_MUTEX(audit_cmd_mutex);
> 
> @@ -584,6 +593,8 @@ static int audit_netlink_ok(struct sk_buff *skb, u16
> msg_type) return -EOPNOTSUPP;
>  	case AUDIT_GET:
>  	case AUDIT_SET:
> +	case AUDIT_GET_FEATURE:
> +	case AUDIT_SET_FEATURE:
>  	case AUDIT_LIST_RULES:
>  	case AUDIT_ADD_RULE:
>  	case AUDIT_DEL_RULE:
> @@ -628,6 +639,94 @@ static int audit_log_common_recv_msg(struct
> audit_buffer **ab, u16 msg_type) return rc;
>  }
> 
> +int is_audit_feature_set(int i)
> +{
> +	return af.features & AUDIT_FEATURE_TO_MASK(i);
> +}
> +
> +
> +static int audit_get_feature(struct sk_buff *skb)
> +{
> +	u32 seq;
> +
> +	seq = nlmsg_hdr(skb)->nlmsg_seq;
> +
> +	audit_send_reply(NETLINK_CB(skb).portid, seq, AUDIT_GET, 0, 0,
> +			 &af, sizeof(af));
> +
> +	return 0;
> +}

Isn't this broke? This returns the status (AUDIT_GET) instead of all the bits 
that got set via the set_feature command. It needs to build a struct 
audit_features and send it back using AUDIT_GET_FEATURE as the netlink msg 
type.

-Steve