* auditd.conf: flush set to DATA or SYNC does nothing on many kernels?
From: Cat @ 2015-10-05 21:43 UTC (permalink / raw)
To: linux-audit
[-- Attachment #1.1: Type: text/plain, Size: 732 bytes --]
Hello all,
I believe auditd's flush configuration can only be set to INCREMENTAL to
guarantee some form of log durability, while DATA or SYNC do nothing. Is
this is a known bug or did I misinterpret auditd.conf's man page?
In audit-event.c: in open_audit_log():
fcntl(F_SETFL, O_SYNC) is called on the already open log's file descriptor,
but O_SYNC (and O_DSYNC) are ignored by F_SETFL
You can check this in the kernel at
fs/fcntl.c:
#define SETFL_MASK (O_APPEND | O_NONBLOCK | O_NDELAY | O_DIRECT | O_NOATIME)
The fcntl() man page also indicates this expected behavior.
I checked both the kernel and audit source for CentOS 6.7 and Ubuntu
14.04.03 and I believe I've reproduced the problem on both distributions.
Thanks,
Cat
[-- Attachment #1.2: Type: text/html, Size: 981 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply
* [RFC PATCH v2 5/5] selinux: introduce kdbus access controls
From: Paul Moore @ 2015-10-05 20:41 UTC (permalink / raw)
To: linux-security-module; +Cc: linux-audit, selinux
In-Reply-To: <20151005203358.32023.88592.stgit@localhost>
Add the SELinux access control implementation for the new kdbus LSM
hooks using the new kdbus object class and the following permissions:
[NOTE: permissions below are based on kdbus code from Aug 2015]
* kdbus:impersonate
Send a different security label to kdbus peers.
* kdbus:fakecreds
Send different DAC credentials to kdbus peers.
* kdbus:fakepids
Send a different PID to kdbus peers.
* kdbus:owner
Act as a kdbus bus owner.
* kdbus:privileged
Act as a privileged endpoint.
* kdbus:activator
Act as a kdbus activator.
* kdbus:monitor
Act as a kdbus monitor.
* kdbus:policy_holder
Act as a kdbus policy holder.
* kdbus:connect
Create a new kdbus connection.
* kdbus:own
Own a kdbus service name.
* kdbus:talk
Talk between two kdbus endpoints.
* kdbus:see
See another kdbus endpoint.
* kdbus:see_name
See a kdbus service name.
* kdbus:see_notification
See a kdbus notification.
Signed-off-by: Paul Moore <pmoore@redhat.com>
---
ChangeLog:
- v2
* Add the selinux_kdbus_init_inode() hook
* Add some very basic info on the permissions to the description
* Add kdbus service name auditing in the AVC records
- v1
* Initial draft
---
security/selinux/hooks.c | 152 +++++++++++++++++++++++++++++++++++
security/selinux/include/classmap.h | 4 +
2 files changed, 154 insertions(+), 2 deletions(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 4d7e602..29341dd 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -9,8 +9,10 @@
* James Morris <jmorris@redhat.com>
*
* Copyright (C) 2001,2002 Networks Associates Technology, Inc.
- * Copyright (C) 2003-2008 Red Hat, Inc., James Morris <jmorris@redhat.com>
- * Eric Paris <eparis@redhat.com>
+ * Copyright (C) 2003-2008,2015 Red Hat, Inc.
+ * James Morris <jmorris@redhat.com>
+ * Eric Paris <eparis@redhat.com>
+ * Paul Moore <paul@paul-moore.com>
* Copyright (C) 2004-2005 Trusted Computer Solutions, Inc.
* <dgoeddel@trustedcs.com>
* Copyright (C) 2006, 2007, 2009 Hewlett-Packard Development Company, L.P.
@@ -1987,6 +1989,143 @@ static int selinux_binder_transfer_file(struct task_struct *from,
&ad);
}
+static int selinux_kdbus_conn_new(const struct cred *creds,
+ const struct kdbus_creds *fake_creds,
+ const struct kdbus_pids *fake_pids,
+ const char *fake_seclabel,
+ bool owner, bool privileged,
+ bool is_activator, bool is_monitor,
+ bool is_policy_holder)
+{
+ int rc;
+ u32 tsid = current_sid();
+ u32 av = KDBUS__CONNECT;
+
+ if (fake_creds)
+ av |= KDBUS__FAKECREDS;
+ if (fake_pids)
+ av |= KDBUS__FAKEPIDS;
+ if (owner)
+ av |= KDBUS__OWNER;
+ if (privileged)
+ av |= KDBUS__PRIVILEGED;
+ if (is_activator)
+ av |= KDBUS__ACTIVATOR;
+ if (is_monitor)
+ av |= KDBUS__MONITOR;
+ if (is_policy_holder)
+ av |= KDBUS__POLICY_HOLDER;
+
+ rc = avc_has_perm(tsid, cred_sid(creds), SECCLASS_KDBUS, av, NULL);
+ if (rc)
+ return rc;
+
+ if (fake_seclabel) {
+ u32 sid;
+ if (security_context_to_sid(fake_seclabel,
+ strlen(fake_seclabel),
+ &sid, GFP_KERNEL))
+ return -EINVAL;
+
+ rc = avc_has_perm(tsid, sid,
+ SECCLASS_KDBUS, KDBUS__IMPERSONATE, NULL);
+ }
+
+ return rc;
+}
+
+static int selinux_kdbus_own_name(const struct cred *creds, const char *name)
+{
+ int rc;
+ u32 name_sid;
+ struct common_audit_data ad;
+
+ rc = security_kdbus_sid(name, &name_sid);
+ if (rc)
+ return rc;
+
+ ad.type = LSM_AUDIT_DATA_KDBUS;
+ ad.u.kdbus_name = name;
+
+ return avc_has_perm(cred_sid(creds), name_sid,
+ SECCLASS_KDBUS, KDBUS__OWN, &ad);
+}
+
+static int selinux_kdbus_conn_talk(const struct cred *creds,
+ const struct cred *creds_to)
+{
+ return avc_has_perm(cred_sid(creds), cred_sid(creds_to),
+ SECCLASS_KDBUS, KDBUS__TALK, NULL);
+}
+
+static int selinux_kdbus_conn_see(const struct cred *creds,
+ const struct cred *creds_whom)
+{
+ return avc_has_perm(cred_sid(creds), cred_sid(creds_whom),
+ SECCLASS_KDBUS, KDBUS__SEE, NULL);
+}
+
+static int selinux_kdbus_conn_see_name(const struct cred *creds,
+ const char *name)
+{
+ int rc;
+ u32 name_sid;
+ struct common_audit_data ad;
+
+ rc = security_kdbus_sid(name, &name_sid);
+ if (rc)
+ return rc;
+
+ ad.type = LSM_AUDIT_DATA_KDBUS;
+ ad.u.kdbus_name = name;
+
+ return avc_has_perm(cred_sid(creds), name_sid,
+ SECCLASS_KDBUS, KDBUS__SEE_NAME, &ad);
+}
+
+static int selinux_kdbus_conn_see_notification(const struct cred *creds)
+{
+ return avc_has_perm(SECINITSID_KERNEL, cred_sid(creds),
+ SECCLASS_KDBUS, KDBUS__SEE_NOTIFICATION, NULL);
+}
+
+static int selinux_kdbus_proc_permission(const struct cred *creds,
+ struct pid *pid)
+{
+ int rc;
+ struct task_struct *task;
+
+ rcu_read_lock();
+ task = pid_task(pid, PIDTYPE_PID);
+ rc = avc_has_perm(cred_sid(creds), task_sid(task),
+ SECCLASS_PROCESS, PROCESS__GETATTR, NULL);
+ rcu_read_unlock();
+
+ return rc;
+}
+
+static int selinux_kdbus_init_inode(struct inode *inode,
+ const struct cred *creds)
+{
+ struct inode_security_struct *isec = inode->i_security;
+ u32 sid = cred_sid(creds);
+
+ /* XXX - this is very simple, e.g. no transitions, no special object
+ * class, etc. since this inode is basically an IPC socket ...
+ * however, is this too simple? do we want transitions? if we
+ * do, we should do the transition in kdbus_node_init() and not
+ * here so that endpoint is labeled correctly and not just this
+ * inode */
+
+ isec->inode = inode;
+ isec->task_sid = sid;
+ isec->sid = sid;
+ isec->sclass = SECCLASS_FILE;
+ isec->initialized = 1;
+
+ return 0;
+}
+
static int selinux_ptrace_access_check(struct task_struct *child,
unsigned int mode)
{
@@ -5888,6 +6027,15 @@ static struct security_operations selinux_ops = {
.binder_transfer_binder = selinux_binder_transfer_binder,
.binder_transfer_file = selinux_binder_transfer_file,
+ .kdbus_conn_new = selinux_kdbus_conn_new,
+ .kdbus_own_name = selinux_kdbus_own_name,
+ .kdbus_conn_talk = selinux_kdbus_conn_talk,
+ .kdbus_conn_see_name = selinux_kdbus_conn_see_name,
+ .kdbus_conn_see = selinux_kdbus_conn_see,
+ .kdbus_conn_see_notification = selinux_kdbus_conn_see_notification,
+ .kdbus_proc_permission = selinux_kdbus_proc_permission,
+ .kdbus_init_inode = selinux_kdbus_init_inode,
+
.ptrace_access_check = selinux_ptrace_access_check,
.ptrace_traceme = selinux_ptrace_traceme,
.capget = selinux_capget,
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index eccd61b..31e4435 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -153,5 +153,9 @@ struct security_class_mapping secclass_map[] = {
{ COMMON_SOCK_PERMS, "attach_queue", NULL } },
{ "binder", { "impersonate", "call", "set_context_mgr", "transfer",
NULL } },
+ { "kdbus", { "impersonate", "fakecreds", "fakepids", "owner",
+ "privileged", "activator", "monitor", "policy_holder",
+ "connect", "own", "talk", "see", "see_name",
+ "see_notification" } },
{ NULL }
};
^ permalink raw reply related
* [RFC PATCH v2 4/5] selinux: introduce kdbus names into the policy
From: Paul Moore @ 2015-10-05 20:41 UTC (permalink / raw)
To: linux-security-module; +Cc: linux-audit, selinux
In-Reply-To: <20151005203358.32023.88592.stgit@localhost>
SELinux treats kdbus service names as objects and therefore needs a
mechanism to map service names to security labels. This patch adds
support for loading kdbus name/label matches with the security policy.
The patch supports service name prefix matching to lessen the burden
on the policy developers and reduce the size of the resulting policy.
Signed-off-by: Paul Moore <pmoore@redhat.com>
---
ChangeLog:
- v2
* Porting needed to work with ioctl xperms
- v1
* Initial draft
---
security/selinux/include/security.h | 5 ++
security/selinux/ss/policydb.c | 88 +++++++++++++++++++++++++++++------
security/selinux/ss/policydb.h | 3 +
security/selinux/ss/services.c | 38 +++++++++++++++
4 files changed, 116 insertions(+), 18 deletions(-)
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index eb9a549..3219b5d 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -36,13 +36,14 @@
#define POLICYDB_VERSION_DEFAULT_TYPE 28
#define POLICYDB_VERSION_CONSTRAINT_NAMES 29
#define POLICYDB_VERSION_XPERMS_IOCTL 30
+#define POLICYDB_VERSION_KDBUS 31
/* Range of policy versions we understand*/
#define POLICYDB_VERSION_MIN POLICYDB_VERSION_BASE
#ifdef CONFIG_SECURITY_SELINUX_POLICYDB_VERSION_MAX
#define POLICYDB_VERSION_MAX CONFIG_SECURITY_SELINUX_POLICYDB_VERSION_MAX_VALUE
#else
-#define POLICYDB_VERSION_MAX POLICYDB_VERSION_XPERMS_IOCTL
+#define POLICYDB_VERSION_MAX POLICYDB_VERSION_KDBUS
#endif
/* Mask for just the mount related flags */
@@ -211,6 +212,8 @@ int security_fs_use(struct super_block *sb);
int security_genfs_sid(const char *fstype, char *name, u16 sclass,
u32 *sid);
+int security_kdbus_sid(const char *name, u32 *sid);
+
#ifdef CONFIG_NETLABEL
int security_netlbl_secattr_to_sid(struct netlbl_lsm_secattr *secattr,
u32 *sid);
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 992a315..9be2e6d 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -76,81 +76,86 @@ static struct policydb_compat_info policydb_compat[] = {
{
.version = POLICYDB_VERSION_BASE,
.sym_num = SYM_NUM - 3,
- .ocon_num = OCON_NUM - 1,
+ .ocon_num = OCON_NUM - 2,
},
{
.version = POLICYDB_VERSION_BOOL,
.sym_num = SYM_NUM - 2,
- .ocon_num = OCON_NUM - 1,
+ .ocon_num = OCON_NUM - 2,
},
{
.version = POLICYDB_VERSION_IPV6,
.sym_num = SYM_NUM - 2,
- .ocon_num = OCON_NUM,
+ .ocon_num = OCON_NUM - 1,
},
{
.version = POLICYDB_VERSION_NLCLASS,
.sym_num = SYM_NUM - 2,
- .ocon_num = OCON_NUM,
+ .ocon_num = OCON_NUM - 1,
},
{
.version = POLICYDB_VERSION_MLS,
.sym_num = SYM_NUM,
- .ocon_num = OCON_NUM,
+ .ocon_num = OCON_NUM - 1,
},
{
.version = POLICYDB_VERSION_AVTAB,
.sym_num = SYM_NUM,
- .ocon_num = OCON_NUM,
+ .ocon_num = OCON_NUM - 1,
},
{
.version = POLICYDB_VERSION_RANGETRANS,
.sym_num = SYM_NUM,
- .ocon_num = OCON_NUM,
+ .ocon_num = OCON_NUM - 1,
},
{
.version = POLICYDB_VERSION_POLCAP,
.sym_num = SYM_NUM,
- .ocon_num = OCON_NUM,
+ .ocon_num = OCON_NUM - 1,
},
{
.version = POLICYDB_VERSION_PERMISSIVE,
.sym_num = SYM_NUM,
- .ocon_num = OCON_NUM,
+ .ocon_num = OCON_NUM - 1,
},
{
.version = POLICYDB_VERSION_BOUNDARY,
.sym_num = SYM_NUM,
- .ocon_num = OCON_NUM,
+ .ocon_num = OCON_NUM - 1,
},
{
.version = POLICYDB_VERSION_FILENAME_TRANS,
.sym_num = SYM_NUM,
- .ocon_num = OCON_NUM,
+ .ocon_num = OCON_NUM - 1,
},
{
.version = POLICYDB_VERSION_ROLETRANS,
.sym_num = SYM_NUM,
- .ocon_num = OCON_NUM,
+ .ocon_num = OCON_NUM - 1,
},
{
.version = POLICYDB_VERSION_NEW_OBJECT_DEFAULTS,
.sym_num = SYM_NUM,
- .ocon_num = OCON_NUM,
+ .ocon_num = OCON_NUM - 1,
},
{
.version = POLICYDB_VERSION_DEFAULT_TYPE,
.sym_num = SYM_NUM,
- .ocon_num = OCON_NUM,
+ .ocon_num = OCON_NUM - 1,
},
{
.version = POLICYDB_VERSION_CONSTRAINT_NAMES,
.sym_num = SYM_NUM,
- .ocon_num = OCON_NUM,
+ .ocon_num = OCON_NUM - 1,
},
{
.version = POLICYDB_VERSION_XPERMS_IOCTL,
.sym_num = SYM_NUM,
+ .ocon_num = OCON_NUM - 1,
+ },
+ {
+ .version = POLICYDB_VERSION_KDBUS,
+ .sym_num = SYM_NUM,
.ocon_num = OCON_NUM,
},
};
@@ -2111,7 +2116,7 @@ static int ocontext_read(struct policydb *p, struct policydb_compat_info *info,
int i, j, rc;
u32 nel, len;
__le32 buf[3];
- struct ocontext *l, *c;
+ struct ocontext *l, *l2, *c;
u32 nodebuf[8];
for (i = 0; i < info->ocon_num; i++) {
@@ -2130,6 +2135,7 @@ static int ocontext_read(struct policydb *p, struct policydb_compat_info *info,
l->next = c;
else
p->ocontexts[i] = c;
+ l2 = l;
l = c;
switch (i) {
@@ -2219,6 +2225,43 @@ static int ocontext_read(struct policydb *p, struct policydb_compat_info *info,
goto out;
break;
}
+ case OCON_KDBUS: {
+ struct ocontext *iter, *last;
+ u32 len2;
+
+ rc = next_entry(buf, fp, sizeof(u32));
+ if (rc)
+ goto out;
+ len = le32_to_cpu(buf[0]);
+ rc = str_read(&c->u.name, GFP_KERNEL, fp, len);
+ if (rc)
+ goto out;
+ rc = context_read_and_validate(&c->context[0], p, fp);
+ if (rc) {
+ kfree(c->u.name);
+ goto out;
+ }
+
+ /* sort by ->u.name length, longest first */
+ last = NULL;
+ iter = p->ocontexts[OCON_KDBUS];
+ while (iter != c) {
+ len2 = strlen(iter->u.name);
+ if (len > len2) {
+ if (l2)
+ l2->next = NULL;
+ c->next = iter;
+ if (last == NULL)
+ p->ocontexts[i] = c;
+ else
+ last->next = c;
+ break;
+ }
+ last = iter;
+ iter = iter->next;
+ }
+ break;
+ }
}
}
}
@@ -3147,6 +3190,19 @@ static int ocontext_write(struct policydb *p, struct policydb_compat_info *info,
if (rc)
return rc;
break;
+ case OCON_KDBUS:
+ len = strlen(c->u.name);
+ buf[0] = cpu_to_le32(len);
+ rc = put_entry(buf, sizeof(u32), 1, fp);
+ if (rc)
+ return rc;
+ rc = put_entry(c->u.name, len, 1, fp);
+ if (rc)
+ return rc;
+ rc = context_write(p, &c->context[0], fp);
+ if (rc)
+ return rc;
+ break;
}
}
}
diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
index 725d594..ee9c120 100644
--- a/security/selinux/ss/policydb.h
+++ b/security/selinux/ss/policydb.h
@@ -222,7 +222,8 @@ struct genfs {
#define OCON_NODE 4 /* nodes */
#define OCON_FSUSE 5 /* fs_use */
#define OCON_NODE6 6 /* IPv6 nodes */
-#define OCON_NUM 7
+#define OCON_KDBUS 7 /* kdbus names */
+#define OCON_NUM 8
/* The policy database */
struct policydb {
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index b7df12b..ada2d28 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -2536,6 +2536,44 @@ int security_genfs_sid(const char *fstype,
}
/**
+ * security_kdbus_sid - Obtain a SID for a kdbus name
+ * @name: kdbus name
+ * @sid: SID for the kdbus name
+ *
+ * Obtain a SID for the given kdbus service name. Returns zero on success,
+ * negative values on error.
+ */
+int security_kdbus_sid(const char *name, u32 *sid)
+{
+ int rc = 0;
+ struct ocontext *c;
+
+ read_lock(&policy_rwlock);
+
+ c = policydb.ocontexts[OCON_KDBUS];
+ while (c) {
+ if (strncmp(c->u.name, name, strlen(c->u.name)) == 0)
+ break;
+ c = c->next;
+ }
+
+ if (c) {
+ if (!c->sid[0]) {
+ rc = sidtab_context_to_sid(&sidtab,
+ &c->context[0], &c->sid[0]);
+ if (rc)
+ goto out;
+ }
+ *sid = c->sid[0];
+ } else
+ *sid = SECINITSID_UNLABELED;
+
+out:
+ read_unlock(&policy_rwlock);
+ return rc;
+}
+
+/**
* security_fs_use - Determine how to handle labeling for a filesystem.
* @sb: superblock in question
*/
^ permalink raw reply related
* [RFC PATCH v2 3/5] lsm: add support for auditing kdbus service names
From: Paul Moore @ 2015-10-05 20:41 UTC (permalink / raw)
To: linux-security-module; +Cc: linux-audit, selinux
In-Reply-To: <20151005203358.32023.88592.stgit@localhost>
The kdbus service names will be recorded using 'service', similar to
the existing dbus audit records.
Signed-off-by: Paul Moore <pmoore@redhat.com>
---
ChangeLog:
- v2
* Initial draft
---
include/linux/lsm_audit.h | 2 ++
security/lsm_audit.c | 4 ++++
2 files changed, 6 insertions(+)
diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h
index ffb9c9d..d6a656f 100644
--- a/include/linux/lsm_audit.h
+++ b/include/linux/lsm_audit.h
@@ -59,6 +59,7 @@ struct common_audit_data {
#define LSM_AUDIT_DATA_INODE 9
#define LSM_AUDIT_DATA_DENTRY 10
#define LSM_AUDIT_DATA_IOCTL_OP 11
+#define LSM_AUDIT_DATA_KDBUS 12
union {
struct path path;
struct dentry *dentry;
@@ -75,6 +76,7 @@ struct common_audit_data {
#endif
char *kmod_name;
struct lsm_ioctlop_audit *op;
+ const char *kdbus_name;
} u;
/* this union contains LSM specific data */
union {
diff --git a/security/lsm_audit.c b/security/lsm_audit.c
index 9f6c649..d7af41d 100644
--- a/security/lsm_audit.c
+++ b/security/lsm_audit.c
@@ -397,6 +397,10 @@ static void dump_common_audit_data(struct audit_buffer *ab,
audit_log_format(ab, " kmod=");
audit_log_untrustedstring(ab, a->u.kmod_name);
break;
+ case LSM_AUDIT_DATA_KDBUS:
+ audit_log_format(ab, " service=");
+ audit_log_untrustedstring(ab, a->u.kdbus_name);
+ break;
} /* switch (a->type) */
}
^ permalink raw reply related
* [RFC PATCH v2 2/5] lsm: introduce hooks for kdbus
From: Paul Moore @ 2015-10-05 20:41 UTC (permalink / raw)
To: linux-security-module; +Cc: linux-audit, selinux
In-Reply-To: <20151005203358.32023.88592.stgit@localhost>
Add LSM access control hooks to kdbus; several new hooks are added and
the existing security_file_receive() hook is reused. The new hooks
are listed below:
* security_kdbus_conn_new
Check if the current task is allowed to create a new kdbus
connection.
* security_kdbus_own_name
Check if a connection is allowed to own a kdbus service name.
* security_kdbus_conn_talk
Check if a connection is allowed to talk to a kdbus peer.
* security_kdbus_conn_see
Check if a connection can see a kdbus peer.
* security_kdbus_conn_see_name
Check if a connection can see a kdbus service name.
* security_kdbus_conn_see_notification
Check if a connection can receive notifications.
* security_kdbus_proc_permission
Check if a connection can access another task's pid namespace info.
* security_kdbus_init_inode
Set the security label on a kdbusfs inode
Signed-off-by: Paul Moore <pmoore@redhat.com>
---
ChangeLog:
- v2
* Implemented suggestions by Stephen Smalley
* call security_kdbus_conn_new() sooner
* reworked hook inside kdbus_conn_policy_own_name()
* fixed if-conditional in kdbus_conn_policy_talk()
* reworked hook inside kdbus_conn_policy_see_name_unlocked()
* reworked hook inside kdbus_conn_policy_see()
* reworked hook inside kdbus_conn_policy_see_notification()
* added the security_kdbus_init_inode() hook
- v1
* Initial draft
---
include/linux/security.h | 126 ++++++++++++++++++++++++++++++++++++++++++++++
ipc/kdbus/connection.c | 73 +++++++++++++++++----------
ipc/kdbus/fs.c | 6 ++
ipc/kdbus/message.c | 19 +++++--
ipc/kdbus/metadata.c | 6 +-
security/security.c | 50 ++++++++++++++++++
6 files changed, 245 insertions(+), 35 deletions(-)
diff --git a/include/linux/security.h b/include/linux/security.h
index 18264ea..7992663 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -53,6 +53,9 @@ struct msg_queue;
struct xattr;
struct xfrm_sec_ctx;
struct mm_struct;
+struct kdbus_creds;
+struct kdbus_pids;
+struct pid;
/* Maximum number of letters for an LSM name string */
#define SECURITY_NAME_MAX 10
@@ -1300,6 +1303,45 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
* @file contains the struct file being transferred.
* @to contains the task_struct for the receiving task.
*
+ * @kdbus_conn_new
+ * Check if the current task is allowed to create a new kdbus connection.
+ * @creds credentials for the new connection
+ * @fake_creds kdbus faked credentials
+ * @fake_pids kdbus faked pids
+ * @fake_seclabel kdbus faked security label
+ * @owner kdbus owner
+ * @privileged kdbus privileged
+ * @is_activator kdbus activator boolean
+ * @is_monitor kdbus monitor boolean
+ * @is_policy_holder kdbus policy holder boolean
+ * @kdbus_own_name
+ * Check if a connection is allowed to own a kdbus service name.
+ * @creds requestor's credentials
+ * @name service name
+ * @kdbus_conn_talk
+ * Check if a connection is allowed to talk to a kdbus peer.
+ * @creds requestor's credentials
+ * @creds_peer peer credentials
+ * @kdbus_conn_see
+ * Check if a connection can see a kdbus peer.
+ * @creds requestor's credentials
+ * @creds_peer peer credentials
+ * @kdbus_conn_see_name
+ * Check if a connection can see a kdbus service name.
+ * @creds requestor's credentials
+ * @name service name
+ * @kdbus_conn_see_notification
+ * Check if a connection can receive notifications.
+ * @creds requestor's credentials
+ * @kdbus_proc_permission
+ * Check if a connection can access another task's pid namespace info.
+ * @cred requestor's credentials
+ * @pid target task's pid struct
+ * @kdbus_init_inode
+ * Set the security label on a kdbusfs inode
+ * @inode kdbusfs inode
+ * @creds inode owner credentials
+ *
* @ptrace_access_check:
* Check permission before allowing the current process to trace the
* @child process.
@@ -1468,6 +1510,22 @@ struct security_operations {
int (*binder_transfer_file) (struct task_struct *from,
struct task_struct *to, struct file *file);
+ int (*kdbus_conn_new)(const struct cred *creds,
+ const struct kdbus_creds *fake_creds,
+ const struct kdbus_pids *fake_pids,
+ const char *fake_seclabel,
+ bool owner, bool privileged, bool is_activator,
+ bool is_monitor, bool is_policy_holder);
+ int (*kdbus_own_name)(const struct cred *creds, const char *name);
+ int (*kdbus_conn_talk)(const struct cred *creds,
+ const struct cred *creds_peer);
+ int (*kdbus_conn_see)(const struct cred *creds,
+ const struct cred *creds_peer);
+ int (*kdbus_conn_see_name)(const struct cred *creds, const char *name);
+ int (*kdbus_conn_see_notification)(const struct cred *creds);
+ int (*kdbus_proc_permission)(const struct cred *creds, struct pid *pid);
+ int (*kdbus_init_inode)(struct inode *inode, const struct cred *creds);
+
int (*ptrace_access_check) (struct task_struct *child, unsigned int mode);
int (*ptrace_traceme) (struct task_struct *parent);
int (*capget) (struct task_struct *target,
@@ -1772,6 +1830,21 @@ int security_binder_transfer_binder(struct task_struct *from,
struct task_struct *to);
int security_binder_transfer_file(struct task_struct *from,
struct task_struct *to, struct file *file);
+int security_kdbus_conn_new(const struct cred *creds,
+ const struct kdbus_creds *fake_creds,
+ const struct kdbus_pids *fake_pids,
+ const char *fake_seclabel,
+ bool owner, bool privileged, bool is_activator,
+ bool is_monitor, bool is_policy_holder);
+int security_kdbus_own_name(const struct cred *creds, const char *name);
+int security_kdbus_conn_talk(const struct cred *creds,
+ const struct cred *creds_peer);
+int security_kdbus_conn_see(const struct cred *creds,
+ const struct cred *creds_peer);
+int security_kdbus_conn_see_name(const struct cred *creds, const char *name);
+int security_kdbus_conn_see_notification(const struct cred *creds);
+int security_kdbus_proc_permission(const struct cred *creds, struct pid *pid);
+int security_kdbus_init_inode(struct inode *inode, const struct cred *creds);
int security_ptrace_access_check(struct task_struct *child, unsigned int mode);
int security_ptrace_traceme(struct task_struct *parent);
int security_capget(struct task_struct *target,
@@ -1984,6 +2057,59 @@ static inline int security_binder_transfer_file(struct task_struct *from,
return 0;
}
+static inline int security_kdbus_conn_new(const struct cred *creds,
+ const struct kdbus_creds *fake_creds,
+ const struct kdbus_pids *fake_pids,
+ const char *fake_seclabel,
+ bool owner, bool privileged,
+ bool is_activator,
+ bool is_monitor,
+ bool is_policy_holder)
+{
+ return 0;
+}
+
+static inline int security_kdbus_own_name(const struct cred *creds,
+ const char *name)
+{
+ return 0;
+}
+
+static inline int security_kdbus_conn_talk(const struct cred *creds,
+ const struct cred *creds_peer)
+{
+ return 0;
+}
+
+static inline int security_kdbus_conn_see(const struct cred *creds,
+ const struct cred *creds_peer)
+{
+ return 0;
+}
+
+static inline int security_kdbus_conn_see_name(const struct cred *creds,
+ const char *name)
+{
+ return 0;
+}
+
+static inline int security_kdbus_conn_see_notification(const struct cred *creds)
+{
+ return 0;
+}
+
+static inline int security_kdbus_proc_permission)(const struct cred *creds,
+ struct pid *pid)
+{
+ return 0;
+}
+
+static inline int security_kdbus_init_inode(struct inode *inode,
+ const struct *creds)
+{
+ return 0;
+}
+
static inline int security_ptrace_access_check(struct task_struct *child,
unsigned int mode)
{
diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
index ef63d65..1cb87b3 100644
--- a/ipc/kdbus/connection.c
+++ b/ipc/kdbus/connection.c
@@ -26,6 +26,7 @@
#include <linux/path.h>
#include <linux/poll.h>
#include <linux/sched.h>
+#include <linux/security.h>
#include <linux/shmem_fs.h>
#include <linux/sizes.h>
#include <linux/slab.h>
@@ -108,6 +109,14 @@ static struct kdbus_conn *kdbus_conn_new(struct kdbus_ep *ep,
if (!owner && (creds || pids || seclabel))
return ERR_PTR(-EPERM);
+ ret = security_kdbus_conn_new(get_cred(file->f_cred),
+ creds, pids, seclabel,
+ owner, privileged,
+ is_activator, is_monitor,
+ is_policy_holder);
+ if (ret < 0)
+ return ERR_PTR(ret);
+
ret = kdbus_sanitize_attach_flags(hello->attach_flags_send,
&attach_flags_send);
if (ret < 0)
@@ -1435,12 +1444,12 @@ bool kdbus_conn_policy_own_name(struct kdbus_conn *conn,
return false;
}
- if (conn->owner)
- return true;
+ if (!conn->owner &&
+ kdbus_policy_query(&conn->ep->bus->policy_db, conn_creds, name,
+ hash) < KDBUS_POLICY_OWN)
+ return false;
- res = kdbus_policy_query(&conn->ep->bus->policy_db, conn_creds,
- name, hash);
- return res >= KDBUS_POLICY_OWN;
+ return (security_kdbus_own_name(conn_creds, name) == 0);
}
/**
@@ -1465,14 +1474,13 @@ bool kdbus_conn_policy_talk(struct kdbus_conn *conn,
to, KDBUS_POLICY_TALK))
return false;
- if (conn->owner)
- return true;
- if (uid_eq(conn_creds->euid, to->cred->uid))
- return true;
+ if (!conn->owner && !uid_eq(conn_creds->euid, to->cred->uid) &&
+ !kdbus_conn_policy_query_all(conn, conn_creds,
+ &conn->ep->bus->policy_db, to,
+ KDBUS_POLICY_TALK))
+ return false;
- return kdbus_conn_policy_query_all(conn, conn_creds,
- &conn->ep->bus->policy_db, to,
- KDBUS_POLICY_TALK);
+ return (security_kdbus_conn_talk(conn_creds, to->cred) == 0);
}
/**
@@ -1491,19 +1499,19 @@ bool kdbus_conn_policy_see_name_unlocked(struct kdbus_conn *conn,
const struct cred *conn_creds,
const char *name)
{
- int res;
+ if (!conn_creds)
+ conn_creds = conn->cred;
/*
* By default, all names are visible on a bus. SEE policies can only be
* installed on custom endpoints, where by default no name is visible.
*/
- if (!conn->ep->user)
- return true;
+ if (conn->ep->user &&
+ kdbus_policy_query_unlocked(&conn->ep->policy_db, conn_creds, name,
+ kdbus_strhash(name)) < KDBUS_POLICY_SEE)
+ return false;
- res = kdbus_policy_query_unlocked(&conn->ep->policy_db,
- conn_creds ? : conn->cred,
- name, kdbus_strhash(name));
- return res >= KDBUS_POLICY_SEE;
+ return (security_kdbus_conn_see_name(conn_creds, name) == 0);
}
static bool kdbus_conn_policy_see_name(struct kdbus_conn *conn,
@@ -1523,6 +1531,9 @@ static bool kdbus_conn_policy_see(struct kdbus_conn *conn,
const struct cred *conn_creds,
struct kdbus_conn *whom)
{
+ if (!conn_creds)
+ conn_creds = conn->cred;
+
/*
* By default, all names are visible on a bus, so a connection can
* always see other connections. SEE policies can only be installed on
@@ -1530,10 +1541,13 @@ static bool kdbus_conn_policy_see(struct kdbus_conn *conn,
* peers from each other, unless you see at least _one_ name of the
* peer.
*/
- return !conn->ep->user ||
- kdbus_conn_policy_query_all(conn, conn_creds,
- &conn->ep->policy_db, whom,
- KDBUS_POLICY_SEE);
+ if (conn->ep->user &&
+ !kdbus_conn_policy_query_all(conn, conn_creds,
+ &conn->ep->policy_db, whom,
+ KDBUS_POLICY_SEE))
+ return false;
+
+ return (security_kdbus_conn_see(conn_creds, whom->cred) == 0);
}
/**
@@ -1551,6 +1565,9 @@ bool kdbus_conn_policy_see_notification(struct kdbus_conn *conn,
const struct cred *conn_creds,
const struct kdbus_msg *msg)
{
+ if (!conn_creds)
+ conn_creds = conn->cred;
+
/*
* Depending on the notification type, broadcasted kernel notifications
* have to be filtered:
@@ -1567,18 +1584,22 @@ bool kdbus_conn_policy_see_notification(struct kdbus_conn *conn,
case KDBUS_ITEM_NAME_ADD:
case KDBUS_ITEM_NAME_REMOVE:
case KDBUS_ITEM_NAME_CHANGE:
- return kdbus_conn_policy_see_name(conn, conn_creds,
- msg->items[0].name_change.name);
+ if (!kdbus_conn_policy_see_name(conn, conn_creds,
+ msg->items[0].name_change.name))
+ return false;
case KDBUS_ITEM_ID_ADD:
case KDBUS_ITEM_ID_REMOVE:
- return true;
+ /* fall through for the LSM check */
+ break;
default:
WARN(1, "Invalid type for notification broadcast: %llu\n",
(unsigned long long)msg->items[0].type);
return false;
}
+
+ return (security_kdbus_conn_see_notification(conn_creds) == 0);
}
/**
diff --git a/ipc/kdbus/fs.c b/ipc/kdbus/fs.c
index 68818a8..4e84e89 100644
--- a/ipc/kdbus/fs.c
+++ b/ipc/kdbus/fs.c
@@ -23,6 +23,7 @@
#include <linux/namei.h>
#include <linux/pagemap.h>
#include <linux/sched.h>
+#include <linux/security.h>
#include <linux/slab.h>
#include "bus.h"
@@ -192,6 +193,7 @@ static const struct inode_operations fs_inode_iops = {
static struct inode *fs_inode_get(struct super_block *sb,
struct kdbus_node *node)
{
+ int ret;
struct inode *inode;
inode = iget_locked(sb, node->id);
@@ -200,6 +202,10 @@ static struct inode *fs_inode_get(struct super_block *sb,
if (!(inode->i_state & I_NEW))
return inode;
+ ret = security_kdbus_init_inode(inode, node->creds);
+ if (ret)
+ return ERR_PTR(ret);
+
inode->i_private = kdbus_node_ref(node);
inode->i_mapping->a_ops = &empty_aops;
inode->i_mode = node->mode & S_IALLUGO;
diff --git a/ipc/kdbus/message.c b/ipc/kdbus/message.c
index ae565cd..acbe981 100644
--- a/ipc/kdbus/message.c
+++ b/ipc/kdbus/message.c
@@ -150,12 +150,17 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice,
for (i = 0; i < gaps->n_fds; ++i) {
int fd;
+ ret = security_file_receive(gaps->fd_files[i]);
+ if (ret) {
+ incomplete_fds = true;
+ fds[n_fds++] = -1;
+ continue;
+ }
+
fd = get_unused_fd_flags(O_CLOEXEC);
if (fd < 0)
incomplete_fds = true;
- WARN_ON(!gaps->fd_files[i]);
-
fds[n_fds++] = fd < 0 ? -1 : fd;
}
@@ -178,6 +183,13 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice,
for (i = 0; i < gaps->n_memfds; ++i) {
int memfd;
+ ret = security_file_receive(gaps->memfd_files[i]);
+ if (ret) {
+ incomplete_fds = true;
+ fds[n_fds++] = -1;
+ continue;
+ }
+
memfd = get_unused_fd_flags(O_CLOEXEC);
if (memfd < 0) {
incomplete_fds = true;
@@ -194,9 +206,6 @@ int kdbus_gaps_install(struct kdbus_gaps *gaps, struct kdbus_pool_slice *slice,
* message.
*/
- WARN_ON(!gaps->memfd_offsets[i]);
- WARN_ON(!gaps->memfd_files[i]);
-
kvec.iov_base = &memfd;
kvec.iov_len = sizeof(memfd);
ret = kdbus_pool_slice_copy_kvec(slice, gaps->memfd_offsets[i],
diff --git a/ipc/kdbus/metadata.c b/ipc/kdbus/metadata.c
index 71ca475..07c45d7 100644
--- a/ipc/kdbus/metadata.c
+++ b/ipc/kdbus/metadata.c
@@ -1182,11 +1182,9 @@ static unsigned int kdbus_proc_permission(const struct pid_namespace *pid_ns,
const struct cred *cred,
struct pid *target)
{
- if (pid_ns->hide_pid < 1)
- return KDBUS_META_PROC_NORMAL;
-
/* XXX: we need groups_search() exported for aux-groups */
- if (gid_eq(cred->egid, pid_ns->pid_gid))
+ if ((pid_ns->hide_pid < 1 || gid_eq(cred->egid, pid_ns->pid_gid)) &&
+ security_kdbus_proc_permission(cred, target) == 0)
return KDBUS_META_PROC_NORMAL;
/*
diff --git a/security/security.c b/security/security.c
index 8e9b1f4..2e5ce04 100644
--- a/security/security.c
+++ b/security/security.c
@@ -158,6 +158,56 @@ int security_binder_transfer_file(struct task_struct *from,
return security_ops->binder_transfer_file(from, to, file);
}
+int security_kdbus_conn_new(const struct cred *creds,
+ const struct kdbus_creds *fake_creds,
+ const struct kdbus_pids *fake_pids,
+ const char *fake_seclabel,
+ bool owner, bool privileged, bool is_activator,
+ bool is_monitor, bool is_policy_holder)
+{
+ return security_ops->kdbus_conn_new(creds, fake_creds, fake_pids,
+ fake_seclabel, owner, privileged,
+ is_activator, is_monitor,
+ is_policy_holder);
+}
+
+int security_kdbus_own_name(const struct cred *creds, const char *name)
+{
+ return security_ops->kdbus_own_name(creds, name);
+}
+
+int security_kdbus_conn_talk(const struct cred *creds,
+ const struct cred *creds_peer)
+{
+ return security_ops->kdbus_conn_talk(creds, creds_peer);
+}
+
+int security_kdbus_conn_see(const struct cred *creds,
+ const struct cred *creds_peer)
+{
+ return security_ops->kdbus_conn_see(creds, creds_peer);
+}
+
+int security_kdbus_conn_see_name(const struct cred *creds, const char *name)
+{
+ return security_ops->kdbus_conn_see_name(creds, name);
+}
+
+int security_kdbus_conn_see_notification(const struct cred *creds)
+{
+ return security_ops->kdbus_conn_see_notification(creds);
+}
+
+int security_kdbus_proc_permission(const struct cred *creds, struct pid *pid)
+{
+ return security_ops->kdbus_proc_permission(creds, pid);
+}
+
+int security_kdbus_init_inode(struct inode *inode, const struct cred *creds)
+{
+ return security_ops->kdbus_init_inode(inode, creds);
+}
+
int security_ptrace_access_check(struct task_struct *child, unsigned int mode)
{
#ifdef CONFIG_SECURITY_YAMA_STACKED
^ permalink raw reply related
* [RFC PATCH v2 1/5] kdbus: add creator credentials to the endpoints
From: Paul Moore @ 2015-10-05 20:41 UTC (permalink / raw)
To: linux-security-module; +Cc: linux-audit, selinux
In-Reply-To: <20151005203358.32023.88592.stgit@localhost>
In order to effectively enforce LSM based access controls we need to
have more information about the kdbus endpoint creator than the
uid/gid currently stored in the kdbus_node_type struct. This patch
replaces the uid/gid values with a reference to the node creator's
credential struct which serves the needs of both the kdbus DAC access
controls as well as the LSM's access controls.
Two macros have also been created, kdbus_node_[uid,gid](), which can
be used to easily extract the euid/egid information from the new
credential reference. The effective uid/gid is used as it was used
in all areas of the previous kdbus code except for areas where the
uid/gid was never set beyond the basic initialization to zero/root;
I expect this was a bug that was never caught as the node creator in
these cases was always expect to be root.
Signed-off-by: Paul Moore <pmoore@redhat.com>
---
ChangeLog:
- v2
* Initial draft
---
ipc/kdbus/bus.c | 13 +++++--------
ipc/kdbus/endpoint.c | 14 ++++----------
ipc/kdbus/endpoint.h | 3 +--
ipc/kdbus/fs.c | 4 ++--
ipc/kdbus/node.c | 11 ++++-------
ipc/kdbus/node.h | 5 +++--
6 files changed, 19 insertions(+), 31 deletions(-)
diff --git a/ipc/kdbus/bus.c b/ipc/kdbus/bus.c
index a67f825..0cb9501 100644
--- a/ipc/kdbus/bus.c
+++ b/ipc/kdbus/bus.c
@@ -65,8 +65,7 @@ static void kdbus_bus_release(struct kdbus_node *node, bool was_active)
static struct kdbus_bus *kdbus_bus_new(struct kdbus_domain *domain,
const char *name,
struct kdbus_bloom_parameter *bloom,
- const u64 *pattach_owner,
- u64 flags, kuid_t uid, kgid_t gid)
+ const u64 *pattach_owner, u64 flags)
{
struct kdbus_bus *b;
u64 attach_owner;
@@ -81,7 +80,8 @@ static struct kdbus_bus *kdbus_bus_new(struct kdbus_domain *domain,
if (ret < 0)
return ERR_PTR(ret);
- ret = kdbus_verify_uid_prefix(name, domain->user_namespace, uid);
+ ret = kdbus_verify_uid_prefix(name, domain->user_namespace,
+ current_euid());
if (ret < 0)
return ERR_PTR(ret);
@@ -93,8 +93,6 @@ static struct kdbus_bus *kdbus_bus_new(struct kdbus_domain *domain,
b->node.free_cb = kdbus_bus_free;
b->node.release_cb = kdbus_bus_release;
- b->node.uid = uid;
- b->node.gid = gid;
b->node.mode = S_IRUSR | S_IXUSR;
if (flags & (KDBUS_MAKE_ACCESS_GROUP | KDBUS_MAKE_ACCESS_WORLD))
@@ -374,7 +372,7 @@ struct kdbus_bus *kdbus_cmd_bus_make(struct kdbus_domain *domain,
bus = kdbus_bus_new(domain,
argv[1].item->str, &argv[2].item->bloom_parameter,
argv[3].item ? argv[3].item->data64 : NULL,
- cmd->flags, current_euid(), current_egid());
+ cmd->flags);
if (IS_ERR(bus)) {
ret = PTR_ERR(bus);
bus = NULL;
@@ -393,8 +391,7 @@ struct kdbus_bus *kdbus_cmd_bus_make(struct kdbus_domain *domain,
goto exit;
}
- ep = kdbus_ep_new(bus, "bus", cmd->flags, bus->node.uid, bus->node.gid,
- false);
+ ep = kdbus_ep_new(bus, "bus", cmd->flags, false);
if (IS_ERR(ep)) {
ret = PTR_ERR(ep);
ep = NULL;
diff --git a/ipc/kdbus/endpoint.c b/ipc/kdbus/endpoint.c
index 44e7a20..1ba5d51 100644
--- a/ipc/kdbus/endpoint.c
+++ b/ipc/kdbus/endpoint.c
@@ -74,8 +74,6 @@ static void kdbus_ep_release(struct kdbus_node *node, bool was_active)
* @bus: The bus this endpoint will be created for
* @name: The name of the endpoint
* @access: The access flags for this node (KDBUS_MAKE_ACCESS_*)
- * @uid: The uid of the node
- * @gid: The gid of the node
* @is_custom: Whether this is a custom endpoint
*
* This function will create a new endpoint with the given
@@ -84,8 +82,7 @@ static void kdbus_ep_release(struct kdbus_node *node, bool was_active)
* Return: a new kdbus_ep on success, ERR_PTR on failure.
*/
struct kdbus_ep *kdbus_ep_new(struct kdbus_bus *bus, const char *name,
- unsigned int access, kuid_t uid, kgid_t gid,
- bool is_custom)
+ unsigned int access, bool is_custom)
{
struct kdbus_ep *e;
int ret;
@@ -96,7 +93,7 @@ struct kdbus_ep *kdbus_ep_new(struct kdbus_bus *bus, const char *name,
*/
if (is_custom) {
ret = kdbus_verify_uid_prefix(name, bus->domain->user_namespace,
- uid);
+ current_euid());
if (ret < 0)
return ERR_PTR(ret);
}
@@ -109,8 +106,6 @@ struct kdbus_ep *kdbus_ep_new(struct kdbus_bus *bus, const char *name,
e->node.free_cb = kdbus_ep_free;
e->node.release_cb = kdbus_ep_release;
- e->node.uid = uid;
- e->node.gid = gid;
e->node.mode = S_IRUSR | S_IWUSR;
if (access & (KDBUS_MAKE_ACCESS_GROUP | KDBUS_MAKE_ACCESS_WORLD))
e->node.mode |= S_IRGRP | S_IWGRP;
@@ -207,7 +202,7 @@ bool kdbus_ep_is_privileged(struct kdbus_ep *ep, struct file *file)
bool kdbus_ep_is_owner(struct kdbus_ep *ep, struct file *file)
{
return !ep->user &&
- (uid_eq(file->f_cred->euid, ep->bus->node.uid) ||
+ (uid_eq(file->f_cred->euid, kdbus_node_uid(&ep->bus->node)) ||
kdbus_ep_is_privileged(ep, file));
}
@@ -245,8 +240,7 @@ struct kdbus_ep *kdbus_cmd_ep_make(struct kdbus_bus *bus, void __user *argp)
item_make_name = argv[1].item->str;
- ep = kdbus_ep_new(bus, item_make_name, cmd->flags,
- current_euid(), current_egid(), true);
+ ep = kdbus_ep_new(bus, item_make_name, cmd->flags, true);
if (IS_ERR(ep)) {
ret = PTR_ERR(ep);
ep = NULL;
diff --git a/ipc/kdbus/endpoint.h b/ipc/kdbus/endpoint.h
index e0da59f..c537779 100644
--- a/ipc/kdbus/endpoint.h
+++ b/ipc/kdbus/endpoint.h
@@ -56,8 +56,7 @@ struct kdbus_ep {
container_of((_node), struct kdbus_ep, node)
struct kdbus_ep *kdbus_ep_new(struct kdbus_bus *bus, const char *name,
- unsigned int access, kuid_t uid, kgid_t gid,
- bool policy);
+ unsigned int access, bool policy);
struct kdbus_ep *kdbus_ep_ref(struct kdbus_ep *ep);
struct kdbus_ep *kdbus_ep_unref(struct kdbus_ep *ep);
diff --git a/ipc/kdbus/fs.c b/ipc/kdbus/fs.c
index 09c4809..68818a8 100644
--- a/ipc/kdbus/fs.c
+++ b/ipc/kdbus/fs.c
@@ -204,8 +204,8 @@ static struct inode *fs_inode_get(struct super_block *sb,
inode->i_mapping->a_ops = &empty_aops;
inode->i_mode = node->mode & S_IALLUGO;
inode->i_atime = inode->i_ctime = inode->i_mtime = CURRENT_TIME;
- inode->i_uid = node->uid;
- inode->i_gid = node->gid;
+ inode->i_uid = kdbus_node_uid(node);
+ inode->i_gid = kdbus_node_gid(node);
switch (node->type) {
case KDBUS_NODE_DOMAIN:
diff --git a/ipc/kdbus/node.c b/ipc/kdbus/node.c
index 89f58bc..cd0c1a0 100644
--- a/ipc/kdbus/node.c
+++ b/ipc/kdbus/node.c
@@ -12,6 +12,7 @@
*/
#include <linux/atomic.h>
+#include <linux/cred.h>
#include <linux/fs.h>
#include <linux/idr.h>
#include <linux/kdev_t.h>
@@ -170,13 +171,7 @@
* node initialization. They must remain constant. If
* NULL, they're skipped.
*
- * * node->mode: filesystem access modes
- * node->uid: filesystem owner uid
- * node->gid: filesystem owner gid
- * These fields must be set by the node owner during node
- * initialization. They must remain constant and may be
- * accessed by other callers to properly initialize
- * filesystem nodes.
+ * * node->creds: The credentials of the node's creator
*
* * node->id: This is an unsigned 32bit integer allocated by an IDA. It is
* always kept as small as possible during allocation and is
@@ -293,6 +288,7 @@ void kdbus_node_init(struct kdbus_node *node, unsigned int type)
mutex_init(&node->lock);
node->id = 0;
node->type = type;
+ node->creds = get_current_cred();
RB_CLEAR_NODE(&node->rb);
node->children = RB_ROOT;
init_waitqueue_head(&node->waitq);
@@ -433,6 +429,7 @@ struct kdbus_node *kdbus_node_unref(struct kdbus_node *node)
WARN_ON(atomic_read(&node->active) != KDBUS_NODE_DRAINED);
WARN_ON(!RB_EMPTY_NODE(&node->rb));
+ put_cred(node->creds);
if (node->free_cb)
node->free_cb(node);
if (safe.id > 0)
diff --git a/ipc/kdbus/node.h b/ipc/kdbus/node.h
index 970e02b..d52e1c2 100644
--- a/ipc/kdbus/node.h
+++ b/ipc/kdbus/node.h
@@ -41,8 +41,7 @@ struct kdbus_node {
kdbus_node_free_t free_cb;
kdbus_node_release_t release_cb;
umode_t mode;
- kuid_t uid;
- kgid_t gid;
+ const struct cred *creds;
/* valid once linked */
char *name;
@@ -57,6 +56,8 @@ struct kdbus_node {
};
#define kdbus_node_from_rb(_node) rb_entry((_node), struct kdbus_node, rb)
+#define kdbus_node_uid(_node) ((_node)->creds->euid)
+#define kdbus_node_gid(_node) ((_node)->creds->egid)
extern struct ida kdbus_node_ida;
^ permalink raw reply related
* [RFC PATCH v2 0/5] kdbus LSM/SELinux hooks
From: Paul Moore @ 2015-10-05 20:41 UTC (permalink / raw)
To: linux-security-module; +Cc: linux-audit, selinux
I got a little distracted with some other tasks so it has taken me a
bit longer than I had hoped to get v2 of this patchset out, but
better late than never they say ...
This revision incorporates all of Stephen's comments and adds support
for auditing kdbus service names; see each patch's changelog for more
details. Please take a look and comment, I'm especially interested
to hear what people have to say about the kdbusfs labeling, right now
it is very barebones, and I'm not sure if we want to support more
options (transitions, etc.).
Also, Smack folks, it would be great if I heard something from you
guys about the viability of these hooks for Smack.
---
Paul Moore (5):
kdbus: add creator credentials to the endpoints
lsm: introduce hooks for kdbus
lsm: add support for auditing kdbus service names
selinux: introduce kdbus names into the policy
selinux: introduce kdbus access controls
include/linux/lsm_audit.h | 2
include/linux/security.h | 126 +++++++++++++++++++++++++++++
ipc/kdbus/bus.c | 13 +--
ipc/kdbus/connection.c | 73 +++++++++++------
ipc/kdbus/endpoint.c | 14 +--
ipc/kdbus/endpoint.h | 3 -
ipc/kdbus/fs.c | 10 ++
ipc/kdbus/message.c | 19 +++-
ipc/kdbus/metadata.c | 6 -
ipc/kdbus/node.c | 11 +--
ipc/kdbus/node.h | 5 +
security/lsm_audit.c | 4 +
security/security.c | 50 ++++++++++++
security/selinux/hooks.c | 152 +++++++++++++++++++++++++++++++++++
security/selinux/include/classmap.h | 4 +
security/selinux/include/security.h | 5 +
security/selinux/ss/policydb.c | 88 +++++++++++++++++---
security/selinux/ss/policydb.h | 3 -
security/selinux/ss/services.c | 38 +++++++++
19 files changed, 540 insertions(+), 86 deletions(-)
^ permalink raw reply
* [PATCH] audit: constify parts of common_audit_data and lsm_network_audit
From: Paul Moore @ 2015-10-05 13:17 UTC (permalink / raw)
To: linux-audit; +Cc: linux-security-module
For the most part audit should never munge with any of the data in
these LSM common structs so constify as much as we can; this patch
handles some easy fields that don't require any real code changes.
Signed-off-by: Paul Moore <pmoore@redhat.com>
---
include/linux/lsm_audit.h | 8 ++++----
security/lsm_audit.c | 4 ++--
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h
index 1cc89e9..6c4c7ac 100644
--- a/include/linux/lsm_audit.h
+++ b/include/linux/lsm_audit.h
@@ -24,7 +24,7 @@
struct lsm_network_audit {
int netif;
- struct sock *sk;
+ const struct sock *sk;
u16 family;
__be16 dport;
__be16 sport;
@@ -55,7 +55,7 @@ struct common_audit_data {
#define LSM_AUDIT_DATA_DENTRY 10
union {
struct path path;
- struct dentry *dentry;
+ const struct dentry *dentry;
struct inode *inode;
struct lsm_network_audit *net;
int cap;
@@ -64,10 +64,10 @@ struct common_audit_data {
#ifdef CONFIG_KEYS
struct {
key_serial_t key;
- char *key_desc;
+ const char *key_desc;
} key_struct;
#endif
- char *kmod_name;
+ const char *kmod_name;
} u;
/* this union contains LSM specific data */
union {
diff --git a/security/lsm_audit.c b/security/lsm_audit.c
index 07fc997..bd61075 100644
--- a/security/lsm_audit.c
+++ b/security/lsm_audit.c
@@ -185,7 +185,7 @@ int ipv6_skb_to_auditdata(struct sk_buff *skb,
static inline void print_ipv6_addr(struct audit_buffer *ab,
- struct in6_addr *addr, __be16 port,
+ const struct in6_addr *addr, __be16 port,
char *name1, char *name2)
{
if (!ipv6_addr_any(addr))
@@ -288,7 +288,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
break;
case LSM_AUDIT_DATA_NET:
if (a->u.net->sk) {
- struct sock *sk = a->u.net->sk;
+ const struct sock *sk = a->u.net->sk;
struct unix_sock *u;
int len = 0;
char *p = NULL;
^ permalink raw reply related
* Two small errors in ausearch-parse.c
From: Emily Ratliff @ 2015-10-03 23:13 UTC (permalink / raw)
To: linux-audit
[-- Attachment #1.1: Type: text/plain, Size: 753 bytes --]
While testing usage of some tools, I stumbled across two small errors in
ausearch-parse.c. I have attached two patches
end-fix.patch fixes the segfaults which can be found by running
ausearch -m USER_AUTH,USER_ACCT --success no --if end-986-dump.log
and
ausearch -m USER_AUTH,USER_ACCT --success no --if error-ausearch.log
term-segfault.patch fixes the errors that can be found by running
aureport -if corrupt-log-for-aureport.log
and
ausearch -m USER_AUTH,USER_ACCT --success no --if
corrupt-log-for-ausearch.log
The erroneous log files were produced using zzuf. The corrupted log files
are also attached. It is unlikely that a user will encounter corrupted
audit log files in the wild, so these bugs aren't serious, but they are
easy to fix.
Emily
[-- Attachment #1.2: Type: text/html, Size: 922 bytes --]
[-- Attachment #2: end-fix.patch --]
[-- Type: text/x-patch, Size: 425 bytes --]
diff -Naurp audit-2.4.4.old/src/ausearch-parse.c audit-2.4.4/src/ausearch-parse.c
--- audit-2.4.4.old/src/ausearch-parse.c 2015-09-21 10:55:43.799602931 -0500
+++ audit-2.4.4/src/ausearch-parse.c 2015-09-21 11:08:15.842117361 -0500
@@ -983,7 +983,7 @@ skip:
char *end = ptr;
int legacy = 0;
- while (*end != ' ') {
+ while (*end != ' ' && *end != '\0') {
if (!isxdigit(*end))
legacy = 1;
end++;
[-- Attachment #3: term-segfault.patch --]
[-- Type: text/x-patch, Size: 431 bytes --]
diff -Naurp audit-2.4.4.old/src/ausearch-parse.c audit-2.4.4/src/ausearch-parse.c
--- audit-2.4.4.old/src/ausearch-parse.c 2015-09-21 08:12:22.554729726 -0500
+++ audit-2.4.4/src/ausearch-parse.c 2015-09-21 08:12:33.898513475 -0500
@@ -971,7 +971,7 @@ skip:
ptr = str + 5;
term = ptr + 1;
if (*ptr == '"') {
- while (*term != '"')
+ while (*term != '"' && *term != '\0')
term++;
saved = *term;
*term = 0;
[-- Attachment #4: corrupt-log-for-aureport.log --]
[-- Type: text/x-log, Size: 264 bytes --]
type=CRED_ACQ msg=audit(1442516413.108:311): pid=853 uid=0 auid=4294967295 ses=4294967295 subj=system_u:system_r:xdm_t:s0-s0:c0.c1023 msg='op=PAM:setcred grantors=pam_permit acct="gdm exe=/usr/libexec/gdm-session-worker hostname=? addr=? terminal=? res=success'
[-- Attachment #5: corrupt-log-for-ausearch.log --]
[-- Type: text/x-log, Size: 274 bytes --]
tYpe=CRED_DISP mqg=audt(\x11442\x1516416.256:441):(0kd=853 ukd=0 auid=4294967295 ses=4294967295 spbj=systmm_5:system_2:xdm_t:s0-s0>c°.c1023 }sG='kp=PAM:setcRed grandors=pam_permit acct="gdm\0(exe="/eóv/licexec¯wdm-cåssioo-worker" hnótnaeE<? a`tr=? terminal=/dev/tty1 res?suckecs&
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #6: end-986-dump.log --]
[-- Type: text/x-log; charset=US-ASCII; name="end-986-dump.log", Size: 273 bytes --]
tùpe=USER_AUTH msg=audit(1442516432.955:308): pid=853 uid50 awid=4294977295 weS=4294y67295 sq`j={y{tem_u:syótem_r:xdm_t:s0-s°;c0.c3023 msg='op=PAEºauthentication$crantors=pam_permit acct=\0g$m" exa="/uSr/lifexec/g`m-session-workar" hostname=? addr=?"terminal=? res=success'
[-- Attachment #7: error-ausearch.log --]
[-- Type: text/x-log, Size: 259 bytes --]
typu=GRP_MGMT mrg<audht(144\x12%17228.9"2:\x1535): pid=4418 uid<0 a5id=1020 3es=1 subj=unconfmned_u:unconfined_r:grotpa`e_t:s0-s0:c°.c1023 móg}/op=changinc-group(grq="rpc5ser" gid=\x129 acct=rrpquseR"\0exe="/usr/óãin/gBoupmod" hostname-? addr== terminal=? res=swccess'
[-- Attachment #8: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply
* Re: auditing kdbus service names
From: Paul Moore @ 2015-10-01 22:32 UTC (permalink / raw)
To: Steve Grubb; +Cc: Paul Osmialowski, linux-security-module, linux-audit, selinux
In-Reply-To: <2836805.Af3VO2DWPn@x2>
On Thursday, August 13, 2015 04:40:52 PM Steve Grubb wrote:
> On Wednesday, August 12, 2015 10:48:10 PM Paul Moore wrote:
> > On Wednesday, August 12, 2015 05:38:14 PM Steve Grubb wrote:
> > > On Wednesday, August 12, 2015 08:40:34 AM Paul Moore wrote:
> > > > Hello all,
> > > >
> > > > I'm currently working on a set of LSM hooks for the new kdbus IPC
> > > > mechanism and one of the things that I believe we will need to add is
> > > > a new audit field for the kdbus service name (very similar to the old
> > > > fashioned dbus service name). I was thinking "kdbus_svc" for the
> > > > field name, any objections?
> > >
> > > What was used on the old dbus events?
> >
> > The very generic "service" field name, see the "acquire_svc" example in
> > the URL below. I believe there is some value in picking a new field name
> > since 1) the field name is too generic in my opinion and 2) kdbus != dbus.
>
> In my book, they are the same. They are programs providing services on the
> bus. One thing I noticed in the dbus events is that there are a number of
> user controlled fields that are not escaped.
Following up on this ...
Decided to just reuse "service" since the rest of the audit record will make
it obvious (new obj class/perms) that the record if for a kdbus event and not
a dbus event. The next patchset will include the audit bits, I'll CC the
patchset here.
--
paul moore
security @ redhat
^ permalink raw reply
* Re: [PATCH 1/2] audit: stop an old auditd being starved out by a new auditd
From: Paul Moore @ 2015-09-29 22:24 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: linux-audit, linux-kernel, sgrubb, eparis, v.rathor, ctcard
In-Reply-To: <20150929043611.GC22712@madcap2.tricolour.ca>
On Tuesday, September 29, 2015 12:36:11 AM Richard Guy Briggs wrote:
> On 15/09/28, Paul Moore wrote:
> > On Monday, September 28, 2015 07:17:31 AM Richard Guy Briggs wrote:
...
> >
> > > So, I believ audit_make_reply() can be used just fine, setting portid,
> > > seq, done and multi to zero.
> >
> > The 'multi' flag should definitely be set to zero, 'seq' is fine at zero,
> > but I think we can do better with 'portid'; we know the 'portid' value so
> > just use it in the call to audit_make_reply().
>
> Most other audit_log_start() created messages set portid to zero except
> user messages, and those are set using the initiating process' portid
> and not the destination id.
Sorry, I was confusing the portid in sockaddr_nl with the portid in the
nlmsghdr ... anyway, yes, the portid in the netlink header should always be
zero since it references the sender and not the destination and the kernel has
portid 0.
> > I don't like that we are reusing audit_make_reply() for non-reply netlink
> > messages, but I'll get over that. This will likely get a revamp when we
> > get around to a proper fix of the queuing system.
>
> This could even be renamed audit_make_message() and possibly be
> generalized to be useful to audit_log_start(), or rather
> audit_buffer_alloc(). Later...
Not exactly what I was thinking, but as I said, not worth worrying about right
now.
> > > Ok, how about AUDIT_HIJACK_TEST, with a payload of the u32
> > > representation of the PID of the task attempting to replace it.
> >
> > Why add the TEST? It is a hijack attempt, or at least it is if the record
> > is emitted successfully :) I would go simply with AUDIT_HIJACK or maybe
> > AUDIT_REPLACE (or similar) if "hijack" is a bit too inflammatory (it
> > probably is ...).
>
> I had actually named it AUDIT_REPLACE_TEST, but your repeated use of the
> term "hijack" swayed me... I'd still lean towards *_TEST since it is
> testing to replace a stale socket and not a live one.
While we are using the record for a test, that is not its only purpose and we
might arrive at a future need to indicate a "hijack" that isn't a test. Keep
the "hijack" if you want, remove the "test".
--
paul moore
security @ redhat
^ permalink raw reply
* Re: [PATCH 3/3] kernel/audit: audit_tree_match can be boolean
From: Paul Moore @ 2015-09-29 18:47 UTC (permalink / raw)
To: Yaowei Bai; +Cc: eparis, linux-audit, linux-kernel
In-Reply-To: <1443536141-5134-3-git-send-email-bywxiaobai@163.com>
On Tuesday, September 29, 2015 10:15:41 PM Yaowei Bai wrote:
> This patch makes audit_tree_match return bool to improve readability
> due to this particular function only using either one or zero as its
> return value.
>
> No functional change.
>
> Signed-off-by: Yaowei Bai <bywxiaobai@163.com>
> ---
> kernel/audit.h | 2 +-
> kernel/audit_tree.c | 6 +++---
> 2 files changed, 4 insertions(+), 4 deletions(-)
Applied, thanks.
> diff --git a/kernel/audit.h b/kernel/audit.h
> index dadf86a..de6cbb7 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -301,7 +301,7 @@ extern int audit_exe_compare(struct task_struct *tsk,
> struct audit_fsnotify_mark #ifdef CONFIG_AUDIT_TREE
> extern struct audit_chunk *audit_tree_lookup(const struct inode *);
> extern void audit_put_chunk(struct audit_chunk *);
> -extern int audit_tree_match(struct audit_chunk *, struct audit_tree *);
> +extern bool audit_tree_match(struct audit_chunk *, struct audit_tree *);
> extern int audit_make_tree(struct audit_krule *, char *, u32);
> extern int audit_add_tree_rule(struct audit_krule *);
> extern int audit_remove_tree_rule(struct audit_krule *);
> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> index 94ecdab..5efe9b29 100644
> --- a/kernel/audit_tree.c
> +++ b/kernel/audit_tree.c
> @@ -197,13 +197,13 @@ struct audit_chunk *audit_tree_lookup(const struct
> inode *inode) return NULL;
> }
>
> -int audit_tree_match(struct audit_chunk *chunk, struct audit_tree *tree)
> +bool audit_tree_match(struct audit_chunk *chunk, struct audit_tree *tree)
> {
> int n;
> for (n = 0; n < chunk->count; n++)
> if (chunk->owners[n].owner == tree)
> - return 1;
> - return 0;
> + return true;
> + return false;
> }
>
> /* tagging and untagging inodes with trees */
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH 2/3] kernel/audit: audit_string_contains_control can be boolean
From: Paul Moore @ 2015-09-29 18:37 UTC (permalink / raw)
To: Yaowei Bai; +Cc: eparis, linux-audit, linux-kernel
In-Reply-To: <1443536141-5134-2-git-send-email-bywxiaobai@163.com>
On Tuesday, September 29, 2015 10:15:40 PM Yaowei Bai wrote:
> This patch makes audit_string_contains_control return bool to improve
> readability due to this particular function only using either one or
> zero as its return value.
>
> Signed-off-by: Yaowei Bai <bywxiaobai@163.com>
> ---
> include/linux/audit.h | 2 +-
> kernel/audit.c | 6 +++---
> 2 files changed, 4 insertions(+), 4 deletions(-)
Applied.
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 69844b6..20eba1e 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -457,7 +457,7 @@ extern struct audit_buffer *audit_log_start(struct
> audit_context *ctx, gfp_t gfp extern __printf(2, 3)
> void audit_log_format(struct audit_buffer *ab, const char *fmt, ...);
> extern void audit_log_end(struct audit_buffer *ab);
> -extern int audit_string_contains_control(const char *string,
> +extern bool audit_string_contains_control(const char *string,
> size_t len);
> extern void audit_log_n_hex(struct audit_buffer *ab,
> const unsigned char *buf,
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 662c007..86d423c 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1566,14 +1566,14 @@ void audit_log_n_string(struct audit_buffer *ab,
> const char *string, * @string: string to be checked
> * @len: max length of the string to check
> */
> -int audit_string_contains_control(const char *string, size_t len)
> +bool audit_string_contains_control(const char *string, size_t len)
> {
> const unsigned char *p;
> for (p = string; p < (const unsigned char *)string + len; p++) {
> if (*p == '"' || *p < 0x21 || *p > 0x7e)
> - return 1;
> + return true;
> }
> - return 0;
> + return false;
> }
>
> /**
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH 1/3] kernel/audit: audit_dummy_context can be boolean
From: Paul Moore @ 2015-09-29 18:23 UTC (permalink / raw)
To: Yaowei Bai; +Cc: eparis, linux-audit, linux-kernel
In-Reply-To: <1443536141-5134-1-git-send-email-bywxiaobai@163.com>
On Tuesday, September 29, 2015 10:15:39 PM Yaowei Bai wrote:
> This patch makes audit_dummy_context return bool due to this
> particular function only using either one or zero as its return
> value.
>
> No functional change.
>
> Signed-off-by: Yaowei Bai <bywxiaobai@163.com>
> ---
> include/linux/audit.h | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
Applied.
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index b2abc99..69844b6 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -143,7 +143,7 @@ extern void __audit_inode_child(const struct inode
> *parent, extern void __audit_seccomp(unsigned long syscall, long signr, int
> code); extern void __audit_ptrace(struct task_struct *t);
>
> -static inline int audit_dummy_context(void)
> +static inline bool audit_dummy_context(void)
> {
> void *p = current->audit_context;
> return !p || *(int *)p;
> @@ -345,9 +345,9 @@ static inline void audit_syscall_entry(int major,
> unsigned long a0, { }
> static inline void audit_syscall_exit(void *pt_regs)
> { }
> -static inline int audit_dummy_context(void)
> +static inline bool audit_dummy_context(void)
> {
> - return 1;
> + return true;
> }
> static inline struct filename *audit_reusename(const __user char *name)
> {
--
paul moore
www.paul-moore.com
^ permalink raw reply
* [PATCH 3/3] kernel/audit: audit_tree_match can be boolean
From: Yaowei Bai @ 2015-09-29 14:15 UTC (permalink / raw)
To: paul, eparis; +Cc: linux-audit, linux-kernel
In-Reply-To: <1443536141-5134-1-git-send-email-bywxiaobai@163.com>
This patch makes audit_tree_match return bool to improve readability
due to this particular function only using either one or zero as its
return value.
No functional change.
Signed-off-by: Yaowei Bai <bywxiaobai@163.com>
---
kernel/audit.h | 2 +-
kernel/audit_tree.c | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/kernel/audit.h b/kernel/audit.h
index dadf86a..de6cbb7 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -301,7 +301,7 @@ extern int audit_exe_compare(struct task_struct *tsk, struct audit_fsnotify_mark
#ifdef CONFIG_AUDIT_TREE
extern struct audit_chunk *audit_tree_lookup(const struct inode *);
extern void audit_put_chunk(struct audit_chunk *);
-extern int audit_tree_match(struct audit_chunk *, struct audit_tree *);
+extern bool audit_tree_match(struct audit_chunk *, struct audit_tree *);
extern int audit_make_tree(struct audit_krule *, char *, u32);
extern int audit_add_tree_rule(struct audit_krule *);
extern int audit_remove_tree_rule(struct audit_krule *);
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index 94ecdab..5efe9b29 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -197,13 +197,13 @@ struct audit_chunk *audit_tree_lookup(const struct inode *inode)
return NULL;
}
-int audit_tree_match(struct audit_chunk *chunk, struct audit_tree *tree)
+bool audit_tree_match(struct audit_chunk *chunk, struct audit_tree *tree)
{
int n;
for (n = 0; n < chunk->count; n++)
if (chunk->owners[n].owner == tree)
- return 1;
- return 0;
+ return true;
+ return false;
}
/* tagging and untagging inodes with trees */
--
1.9.1
^ permalink raw reply related
* [PATCH 2/3] kernel/audit: audit_string_contains_control can be boolean
From: Yaowei Bai @ 2015-09-29 14:15 UTC (permalink / raw)
To: paul, eparis; +Cc: linux-audit, linux-kernel
In-Reply-To: <1443536141-5134-1-git-send-email-bywxiaobai@163.com>
This patch makes audit_string_contains_control return bool to improve
readability due to this particular function only using either one or
zero as its return value.
Signed-off-by: Yaowei Bai <bywxiaobai@163.com>
---
include/linux/audit.h | 2 +-
kernel/audit.c | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 69844b6..20eba1e 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -457,7 +457,7 @@ extern struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp
extern __printf(2, 3)
void audit_log_format(struct audit_buffer *ab, const char *fmt, ...);
extern void audit_log_end(struct audit_buffer *ab);
-extern int audit_string_contains_control(const char *string,
+extern bool audit_string_contains_control(const char *string,
size_t len);
extern void audit_log_n_hex(struct audit_buffer *ab,
const unsigned char *buf,
diff --git a/kernel/audit.c b/kernel/audit.c
index 662c007..86d423c 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1566,14 +1566,14 @@ void audit_log_n_string(struct audit_buffer *ab, const char *string,
* @string: string to be checked
* @len: max length of the string to check
*/
-int audit_string_contains_control(const char *string, size_t len)
+bool audit_string_contains_control(const char *string, size_t len)
{
const unsigned char *p;
for (p = string; p < (const unsigned char *)string + len; p++) {
if (*p == '"' || *p < 0x21 || *p > 0x7e)
- return 1;
+ return true;
}
- return 0;
+ return false;
}
/**
--
1.9.1
^ permalink raw reply related
* [PATCH 1/3] kernel/audit: audit_dummy_context can be boolean
From: Yaowei Bai @ 2015-09-29 14:15 UTC (permalink / raw)
To: paul, eparis; +Cc: linux-audit, linux-kernel
This patch makes audit_dummy_context return bool due to this
particular function only using either one or zero as its return
value.
No functional change.
Signed-off-by: Yaowei Bai <bywxiaobai@163.com>
---
include/linux/audit.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/linux/audit.h b/include/linux/audit.h
index b2abc99..69844b6 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -143,7 +143,7 @@ extern void __audit_inode_child(const struct inode *parent,
extern void __audit_seccomp(unsigned long syscall, long signr, int code);
extern void __audit_ptrace(struct task_struct *t);
-static inline int audit_dummy_context(void)
+static inline bool audit_dummy_context(void)
{
void *p = current->audit_context;
return !p || *(int *)p;
@@ -345,9 +345,9 @@ static inline void audit_syscall_entry(int major, unsigned long a0,
{ }
static inline void audit_syscall_exit(void *pt_regs)
{ }
-static inline int audit_dummy_context(void)
+static inline bool audit_dummy_context(void)
{
- return 1;
+ return true;
}
static inline struct filename *audit_reusename(const __user char *name)
{
--
1.9.1
^ permalink raw reply related
* Re: [PATCH 1/2] audit: stop an old auditd being starved out by a new auditd
From: Richard Guy Briggs @ 2015-09-29 4:36 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit, linux-kernel, sgrubb, eparis, v.rathor, ctcard
In-Reply-To: <2360811.VVg2NkZJdF@sifl>
On 15/09/28, Paul Moore wrote:
> On Monday, September 28, 2015 07:17:31 AM Richard Guy Briggs wrote:
> > On 15/09/25, Paul Moore wrote:
> > > The audit_make_reply() function is the wrong thing to be using here, we
> > > should create our own buffer from scratch like most other records. Also,
> > > yes, we want to include the new pid, but I really don't think there is
> > > any value in including the seqno of the AUDIT_SET/AUDIT_STATUS_PID
> > > message.
> >
> > Most other records use audit_log_start(), which isn't what we want here,
> > since we want to bypass the queue to test if it is still alive. We
> > don't care if it is delivered. We just care if the socket is still
> > alive. We don't want a context either.
>
> Yes, that is why I mentioned creating the buffer from scratch.
>
> > So, I believ audit_make_reply() can be used just fine, setting portid,
> > seq, done and multi to zero.
>
> The 'multi' flag should definitely be set to zero, 'seq' is fine at zero, but
> I think we can do better with 'portid'; we know the 'portid' value so just use
> it in the call to audit_make_reply().
Most other audit_log_start() created messages set portid to zero except
user messages, and those are set using the initiating process' portid
and not the destination id. So here I think portid should be zero. The
target task should know its own portid and the netlink field for portid
isn't used for routing to that destination that I can discern from the
netlink code.
> I don't like that we are reusing audit_make_reply() for non-reply netlink
> messages, but I'll get over that. This will likely get a revamp when we get
> around to a proper fix of the queuing system.
This could even be renamed audit_make_message() and possibly be
generalized to be useful to audit_log_start(), or rather
audit_buffer_alloc(). Later...
> > > > > Also, this is more of a attempted hijack message and not a
> > > > > simple ping, right?
> > > >
> > > > Ok, so maybe AUDIT_PING is not the appropriate name for it. I don't
> > > > have a problem changing it, but I think the pid of the hijacker would be
> > > > useful information to the ping-ee unless the ping message was only ever
> > > > issues in a contextless kernel-initiated message.
> > >
> > > Let's change the message name, this isn't a ping message and we may want
> > > to have a ping message at some point in the future.
> >
> > Ok, how about AUDIT_HIJACK_TEST, with a payload of the u32
> > representation of the PID of the task attempting to replace it.
>
> Why add the TEST? It is a hijack attempt, or at least it is if the record is
> emitted successfully :) I would go simply with AUDIT_HIJACK or maybe
> AUDIT_REPLACE (or similar) if "hijack" is a bit too inflammatory (it probably
> is ...).
I had actually named it AUDIT_REPLACE_TEST, but your repeated use of the
term "hijack" swayed me... I'd still lean towards *_TEST since it is
testing to replace a stale socket and not a live one.
> paul moore
- 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
^ permalink raw reply
* Re: [PATCH 1/2] audit: stop an old auditd being starved out by a new auditd
From: Paul Moore @ 2015-09-28 18:55 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: v.rathor, linux-kernel, linux-audit
In-Reply-To: <20150928111731.GA22712@madcap2.tricolour.ca>
On Monday, September 28, 2015 07:17:31 AM Richard Guy Briggs wrote:
> On 15/09/25, Paul Moore wrote:
...
> > The audit_make_reply() function is the wrong thing to be using here, we
> > should create our own buffer from scratch like most other records. Also,
> > yes, we want to include the new pid, but I really don't think there is
> > any value in including the seqno of the AUDIT_SET/AUDIT_STATUS_PID
> > message.
>
> Most other records use audit_log_start(), which isn't what we want here,
> since we want to bypass the queue to test if it is still alive. We
> don't care if it is delivered. We just care if the socket is still
> alive. We don't want a context either.
Yes, that is why I mentioned creating the buffer from scratch.
> So, I believ audit_make_reply() can be used just fine, setting portid,
> seq, done and multi to zero.
The 'multi' flag should definitely be set to zero, 'seq' is fine at zero, but
I think we can do better with 'portid'; we know the 'portid' value so just use
it in the call to audit_make_reply().
I don't like that we are reusing audit_make_reply() for non-reply netlink
messages, but I'll get over that. This will likely get a revamp when we get
around to a proper fix of the queuing system.
> > > > Also, this is more of a attempted hijack message and not a simple
> > > > ping,
> > > > right?
> > >
> > > Ok, so maybe AUDIT_PING is not the appropriate name for it. I don't
> > > have a problem changing it, but I think the pid of the hijacker would be
> > > useful information to the ping-ee unless the ping message was only ever
> > > issues in a contextless kernel-initiated message.
> >
> > Let's change the message name, this isn't a ping message and we may want
> > to have a ping message at some point in the future.
>
> Ok, how about AUDIT_HIJACK_TEST, with a payload of the u32
> representation of the PID of the task attempting to replace it.
Why add the TEST? It is a hijack attempt, or at least it is if the record is
emitted successfully :) I would go simply with AUDIT_HIJACK or maybe
AUDIT_REPLACE (or similar) if "hijack" is a bit too inflammatory (it probably
is ...).
--
paul moore
security @ redhat
^ permalink raw reply
* Re: [PATCH 1/2] audit: stop an old auditd being starved out by a new auditd
From: Richard Guy Briggs @ 2015-09-28 11:17 UTC (permalink / raw)
To: Paul Moore; +Cc: v.rathor, linux-kernel, linux-audit
In-Reply-To: <37985395.CCHtq1r3PJ@sifl>
On 15/09/25, Paul Moore wrote:
> On Friday, September 25, 2015 07:10:19 AM Richard Guy Briggs wrote:
> > On 15/09/24, Paul Moore wrote:
> > > On Friday, September 18, 2015 03:59:58 AM Richard Guy Briggs wrote:
> > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > index 18cdfe2..3399ab2 100644
> > > > --- a/kernel/audit.c
> > > > +++ b/kernel/audit.c
> > > > @@ -810,6 +810,15 @@ static int audit_set_feature(struct sk_buff *skb)
> > > >
> > > > return 0;
> > > >
> > > > }
> > > >
> > > > +static int audit_ping(pid_t pid, u32 seq, u32 portid)
> > > > +{
> > > > + struct sk_buff *skb = audit_make_reply(portid,seq,AUDIT_PING,0,0,
> > > > + &pid, sizeof(pid));
> > >
> > > This is almost surely going to end up using the wrong netlink sequence
> > > number and portid since you are passing the new requestor's information
> > > below. I didn't chase down the netlink_unicast() guts to see if it
> > > replaces the portid, it might (it probably does), but that still leaves
> > > the sequence number.
> >
> > It is intended to use the new pid and new netlink sequence number to the
> > old audit_sock and old portid.
>
> You don't want to put the new portid/seqno in a netlink header that is being
> sent to the old auditd.
Ok, you are right, both should be set to zero, since nothing else makes sense.
> > There is no other sequence number available and it is this new sequence
> > number and pid that needs reporting to the old auditd.
>
> The audit_make_reply() function is the wrong thing to be using here, we should
> create our own buffer from scratch like most other records. Also, yes, we
> want to include the new pid, but I really don't think there is any value in
> including the seqno of the AUDIT_SET/AUDIT_STATUS_PID message.
Most other records use audit_log_start(), which isn't what we want here,
since we want to bypass the queue to test if it is still alive. We
don't care if it is delivered. We just care if the socket is still
alive. We don't want a context either.
So, I believ audit_make_reply() can be used just fine, setting portid,
seq, done and multi to zero.
> > > Also, this is more of a attempted hijack message and not a simple ping,
> > > right?
> >
> > Ok, so maybe AUDIT_PING is not the appropriate name for it. I don't
> > have a problem changing it, but I think the pid of the hijacker would be
> > useful information to the ping-ee unless the ping message was only ever
> > issues in a contextless kernel-initiated message.
>
> Let's change the message name, this isn't a ping message and we may want to
> have a ping message at some point in the future.
Ok, how about AUDIT_HIJACK_TEST, with a payload of the u32
representation of the PID of the task attempting to replace it.
> > > If we want to create a simple ping message, leave the pid out of it; if we
> > > want to indicate to an existing auditd that another process is attempting
> > > to hijack the audit connection then we should probably create a proper
> > > audit record with a type other than AUDIT_PING. I tend to think there is
> > > more value in the hijack message than the ping message, but I can be
> > > convinced either way.
> >
> > Is there any compelling reason to create a pure ping message that gets
> > sent out periodically to test if auditd is still alive (audit_pid,
> > audit_sock and audit_nlk_portid are valid)?
>
> Possibly, but let's leave that as a future experiment.
Ok.
> > Is there any reason to reserve that AUDIT_PING macro at this time should it
> > be determined that it is necessary in the future?
>
> I don't think we need to reserve it now, we can allocate the ping message type
> if/when we decide to implement it.
Ok.
> paul moore
- 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
^ permalink raw reply
* Re: [PATCH 1/2] audit: stop an old auditd being starved out by a new auditd
From: Paul Moore @ 2015-09-25 21:14 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: linux-audit, linux-kernel, sgrubb, eparis, v.rathor, ctcard
In-Reply-To: <20150925111019.GC23667@madcap2.tricolour.ca>
On Friday, September 25, 2015 07:10:19 AM Richard Guy Briggs wrote:
> On 15/09/24, Paul Moore wrote:
> > On Friday, September 18, 2015 03:59:58 AM Richard Guy Briggs wrote:
...
> > XXX
>
> ???
Sorry, ignore that. The "XXX" was a placeholder for me while I was reviewing
your patch; normally I got back and replace those all with text or drop them,
but one slipped through in this case.
> > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > index 18cdfe2..3399ab2 100644
> > > --- a/kernel/audit.c
> > > +++ b/kernel/audit.c
> > > @@ -810,6 +810,15 @@ static int audit_set_feature(struct sk_buff *skb)
> > >
> > > return 0;
> > >
> > > }
> > >
> > > +static int audit_ping(pid_t pid, u32 seq, u32 portid)
> > > +{
> > > + struct sk_buff *skb = audit_make_reply(portid,seq,AUDIT_PING,0,0,
> > > + &pid, sizeof(pid));
> >
> > This is almost surely going to end up using the wrong netlink sequence
> > number and portid since you are passing the new requestor's information
> > below. I didn't chase down the netlink_unicast() guts to see if it
> > replaces the portid, it might (it probably does), but that still leaves
> > the sequence number.
>
> It is intended to use the new pid and new netlink sequence number to the
> old audit_sock and old portid.
You don't want to put the new portid/seqno in a netlink header that is being
sent to the old auditd.
> There is no other sequence number available and it is this new sequence
> number and pid that needs reporting to the old auditd.
The audit_make_reply() function is the wrong thing to be using here, we should
create our own buffer from scratch like most other records. Also, yes, we
want to include the new pid, but I really don't think there is any value in
including the seqno of the AUDIT_SET/AUDIT_STATUS_PID message.
> > Also, this is more of a attempted hijack message and not a simple ping,
> > right?
>
> Ok, so maybe AUDIT_PING is not the appropriate name for it. I don't
> have a problem changing it, but I think the pid of the hijacker would be
> useful information to the ping-ee unless the ping message was only ever
> issues in a contextless kernel-initiated message.
Let's change the message name, this isn't a ping message and we may want to
have a ping message at some point in the future.
> > If we want to create a simple ping message, leave the pid out of it; if we
> > want to indicate to an existing auditd that another process is attempting
> > to hijack the audit connection then we should probably create a proper
> > audit record with a type other than AUDIT_PING. I tend to think there is
> > more value in the hijack message than the ping message, but I can be
> > convinced either way.
>
> Is there any compelling reason to create a pure ping message that gets
> sent out periodically to test if auditd is still alive (audit_pid,
> audit_sock and audit_nlk_portid are valid)?
Possibly, but let's leave that as a future experiment.
> Is there any reason to reserve that AUDIT_PING macro at this time should it
> be determined that it is necessary in the future?
I don't think we need to reserve it now, we can allocate the ping message type
if/when we decide to implement it.
--
paul moore
security @ redhat
^ permalink raw reply
* Re: [PATCH 1/2] audit: stop an old auditd being starved out by a new auditd
From: Richard Guy Briggs @ 2015-09-25 11:10 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit, linux-kernel, sgrubb, eparis, v.rathor, ctcard
In-Reply-To: <6749672.utgyqf2Omq@sifl>
On 15/09/24, Paul Moore wrote:
> On Friday, September 18, 2015 03:59:58 AM Richard Guy Briggs wrote:
> > Nothing prevents a new auditd starting up and replacing a valid
> > audit_pid when an old auditd is still running, effectively starving out
> > the old auditd since audit_pid no longer points to the old valid auditd.
> >
> > If no message to auditd has been attempted since auditd died unnaturally
> > or got killed, audit_pid will still indicate it is alive. There isn't
> > an easy way to detect if an old auditd is still running on the existing
> > audit_pid other than attempting to send a message to see if it fails.
> > An -ECONNREFUSED almost certainly means it disappeared and can be
> > replaced. Other errors are not so straightforward and may indicate
> > transient problems that will resolve themselves and the old auditd will
> > recover. Yet others will likely need manual intervention for which a
> > new auditd will not solve the problem.
> >
> > Send a new message type (AUDIT_PING) to the old auditd containing a u32
> > with the PID of the new auditd. If the audit ping succeeds (or doesn't
> > fail with certainty), fail to register the new auditd and return an
> > error (-EEXIST).
> >
> > This is expected to make the patch preventing an old auditd orphaning a
> > new auditd redundant.
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > include/uapi/linux/audit.h | 1 +
> > kernel/audit.c | 19 +++++++++++++++++--
> > 2 files changed, 18 insertions(+), 2 deletions(-)
>
> XXX
???
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 18cdfe2..3399ab2 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -810,6 +810,15 @@ static int audit_set_feature(struct sk_buff *skb)
> > return 0;
> > }
> >
> > +static int audit_ping(pid_t pid, u32 seq, u32 portid)
> > +{
> > + struct sk_buff *skb = audit_make_reply(portid, seq, AUDIT_PING, 0, 0,
> > + &pid, sizeof(pid));
>
> This is almost surely going to end up using the wrong netlink sequence number
> and portid since you are passing the new requestor's information below. I
> didn't chase down the netlink_unicast() guts to see if it replaces the portid,
> it might (it probably does), but that still leaves the sequence number.
It is intended to use the new pid and new netlink sequence number to the
old audit_sock and old portid. There is no other sequence number
available and it is this new sequence number and pid that needs
reporting to the old auditd.
> Also, this is more of a attempted hijack message and not a simple ping, right?
Ok, so maybe AUDIT_PING is not the appropriate name for it. I don't
have a problem changing it, but I think the pid of the hijacker would be
useful information to the ping-ee unless the ping message was only ever
issues in a contextless kernel-initiated message.
> If we want to create a simple ping message, leave the pid out of it; if we
> want to indicate to an existing auditd that another process is attempting to
> hijack the audit connection then we should probably create a proper audit
> record with a type other than AUDIT_PING. I tend to think there is more value
> in the hijack message than the ping message, but I can be convinced either
> way.
Is there any compelling reason to create a pure ping message that gets
sent out periodically to test if auditd is still alive (audit_pid,
audit_sock and audit_nlk_portid are valid)? Is there any reason to
reserve that AUDIT_PING macro at this time should it be determined that
it is necessary in the future?
> > + if (!skb)
> > + return -ENOMEM;
> > + return netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> > +}
>
> ...
>
> > @@ -871,13 +880,19 @@ static int audit_receive_msg(struct sk_buff *skb,
> > if (s.mask & AUDIT_STATUS_PID) {
> > int new_pid = s.pid;
> > + pid_t requesting_pid = task_tgid_vnr(current);
> > + u32 portid = NETLINK_CB(skb).portid;
> >
> > - if ((!new_pid) && (task_tgid_vnr(current) != audit_pid))
> > + if ((!new_pid) && (requesting_pid != audit_pid))
> > return -EACCES;
> > + if (audit_pid && new_pid &&
> > + audit_ping(requesting_pid, nlmsg_hdr(skb)->..., portid) !=
> > + -ECONNREFUSED)
> > + return -EEXIST;
>
> See my comments above about audit_ping().
>
> > if (audit_enabled != AUDIT_OFF)
> > audit_log_config_change("audit_pid", new_pid, audit_pid, 1);
> > audit_pid = new_pid;
> > - audit_nlk_portid = NETLINK_CB(skb).portid;
> > + audit_nlk_portid = portid;
> > audit_sock = skb->sk;
> > }
> > if (s.mask & AUDIT_STATUS_RATE_LIMIT) {
>
> paul moore
- 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
^ permalink raw reply
* Re: [PATCH 2/2] audit: log failed attempts to change audit_pid configuration
From: Paul Moore @ 2015-09-24 20:12 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: v.rathor, linux-kernel, linux-audit
In-Reply-To: <1897d9f5007c4212aac4095b7fa2491e44cc880f.1442494593.git.rgb@redhat.com>
On Friday, September 18, 2015 03:59:59 AM Richard Guy Briggs wrote:
> Failed attempts to change the audit_pid configuration are not presently
> logged. One case is an attempt to starve an old auditd by starting up a
> new auditd when the old one is still alive and active. The other case
> is an attempt to orphan a new auditd when an old auditd shuts down.
>
> Log both as AUDIT_CONFIG_CHANGE messages with failure result.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> kernel/audit.c | 8 ++++++--
> 1 files changed, 6 insertions(+), 2 deletions(-)
This patch tends to reinforce the idea of a hijack message instead of a ping
message. Unfortunately, we can't use audit_log_config_change() to generate
the hijack message as it queues the record, but you get the idea.
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 3399ab2..65dcd45 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -883,12 +883,16 @@ static int audit_receive_msg(struct sk_buff *skb,
> struct nlmsghdr *nlh) pid_t requesting_pid = task_tgid_vnr(current);
> u32 portid = NETLINK_CB(skb).portid;
>
> - if ((!new_pid) && (requesting_pid != audit_pid))
> + if ((!new_pid) && (requesting_pid != audit_pid)) {
> + audit_log_config_change("audit_pid", new_pid, audit_pid, 0);
> return -EACCES;
> + }
> if (audit_pid && new_pid &&
> audit_ping(requesting_pid, nlmsg_hdr(skb)->nlmsg_seq, portid)
!=
> - -ECONNREFUSED)
> + -ECONNREFUSED) {
> + audit_log_config_change("audit_pid", new_pid, audit_pid, 0);
> return -EEXIST;
> + }
> if (audit_enabled != AUDIT_OFF)
> audit_log_config_change("audit_pid", new_pid, audit_pid, 1);
> audit_pid = new_pid;
--
paul moore
security @ redhat
^ permalink raw reply
* Re: [PATCH 1/2] audit: stop an old auditd being starved out by a new auditd
From: Paul Moore @ 2015-09-24 20:07 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: linux-audit, linux-kernel, sgrubb, eparis, v.rathor, ctcard
In-Reply-To: <206b0f415832c9fde6befaa13a7b6efe916d1ba4.1442494593.git.rgb@redhat.com>
On Friday, September 18, 2015 03:59:58 AM Richard Guy Briggs wrote:
> Nothing prevents a new auditd starting up and replacing a valid
> audit_pid when an old auditd is still running, effectively starving out
> the old auditd since audit_pid no longer points to the old valid auditd.
>
> If no message to auditd has been attempted since auditd died unnaturally
> or got killed, audit_pid will still indicate it is alive. There isn't
> an easy way to detect if an old auditd is still running on the existing
> audit_pid other than attempting to send a message to see if it fails.
> An -ECONNREFUSED almost certainly means it disappeared and can be
> replaced. Other errors are not so straightforward and may indicate
> transient problems that will resolve themselves and the old auditd will
> recover. Yet others will likely need manual intervention for which a
> new auditd will not solve the problem.
>
> Send a new message type (AUDIT_PING) to the old auditd containing a u32
> with the PID of the new auditd. If the audit ping succeeds (or doesn't
> fail with certainty), fail to register the new auditd and return an
> error (-EEXIST).
>
> This is expected to make the patch preventing an old auditd orphaning a
> new auditd redundant.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> include/uapi/linux/audit.h | 1 +
> kernel/audit.c | 19 +++++++++++++++++--
> 2 files changed, 18 insertions(+), 2 deletions(-)
XXX
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 18cdfe2..3399ab2 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -810,6 +810,15 @@ static int audit_set_feature(struct sk_buff *skb)
> return 0;
> }
>
> +static int audit_ping(pid_t pid, u32 seq, u32 portid)
> +{
> + struct sk_buff *skb = audit_make_reply(portid, seq, AUDIT_PING, 0, 0,
> + &pid, sizeof(pid));
This is almost surely going to end up using the wrong netlink sequence number
and portid since you are passing the new requestor's information below. I
didn't chase down the netlink_unicast() guts to see if it replaces the portid,
it might (it probably does), but that still leaves the sequence number.
Also, this is more of a attempted hijack message and not a simple ping, right?
If we want to create a simple ping message, leave the pid out of it; if we
want to indicate to an existing auditd that another process is attempting to
hijack the audit connection then we should probably create a proper audit
record with a type other than AUDIT_PING. I tend to think there is more value
in the hijack message than the ping message, but I can be convinced either
way.
> + if (!skb)
> + return -ENOMEM;
> + return netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> +}
...
> @@ -871,13 +880,19 @@ static int audit_receive_msg(struct sk_buff *skb,
> if (s.mask & AUDIT_STATUS_PID) {
> int new_pid = s.pid;
> + pid_t requesting_pid = task_tgid_vnr(current);
> + u32 portid = NETLINK_CB(skb).portid;
>
> - if ((!new_pid) && (task_tgid_vnr(current) != audit_pid))
> + if ((!new_pid) && (requesting_pid != audit_pid))
> return -EACCES;
> + if (audit_pid && new_pid &&
> + audit_ping(requesting_pid, nlmsg_hdr(skb)->..., portid) !=
> + -ECONNREFUSED)
> + return -EEXIST;
See my comments above about audit_ping().
> if (audit_enabled != AUDIT_OFF)
> audit_log_config_change("audit_pid", new_pid, audit_pid, 1);
> audit_pid = new_pid;
> - audit_nlk_portid = NETLINK_CB(skb).portid;
> + audit_nlk_portid = portid;
> audit_sock = skb->sk;
> }
> if (s.mask & AUDIT_STATUS_RATE_LIMIT) {
--
paul moore
security @ redhat
^ permalink raw reply
* Re: [PATCH] fixup! audit: try harder to send to auditd upon netlink failure
From: Paul Moore @ 2015-09-22 22:32 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: linux-audit, linux-kernel, sgrubb, eparis, v.rathor, ctcard
In-Reply-To: <b3c5c9441c3d461465a9686388e8baba2ae2449f.1442699085.git.rgb@redhat.com>
On Saturday, September 19, 2015 05:52:50 PM Richard Guy Briggs wrote:
> A bug was introduced by "audit: try harder to send to auditd upon
> netlink failure", caused by incomplete code and a function that expects
> a string and does not accept a format plus arguments. Create a
> temporary string variable to assemble the output text. It could be
> merged as a fixup if it is not yet upstream.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> kernel/audit.c | 5 ++++-
> 1 files changed, 4 insertions(+), 1 deletions(-)
Applied and both patches squashed into one.
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 18cdfe2..9d32218 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -420,7 +420,10 @@ restart:
> if (audit_pid) {
> if (err == -ECONNREFUSED || err == -EPERM
>
> || ++attempts >= AUDITD_RETRIES) {
>
> - audit_log_lost("audit_pid=%d reset");
> + char s[32];
> +
> + snprintf(s, sizeof(s), "audit_pid=%d reset", audit_pid);
> + audit_log_lost(s);
> audit_pid = 0;
> audit_sock = NULL;
> } else {
--
paul moore
security @ redhat
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox