[v3,21/24] Audit: Store LSM audit information in an lsmblob
diff mbox series

Message ID 20190621185233.6766-22-casey@schaufler-ca.com
State Superseded
Headers show
Series
  • LSM: Module stacking for AppArmor
Related show

Commit Message

Casey Schaufler June 21, 2019, 6:52 p.m. UTC
Change the audit code to store full lsmblob data instead of
a single u32 secid. This allows for multiple security modules
to use the audit system at the same time. It also allows the
removal of scaffolding code that was included during the
revision of LSM interfaces.

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
 kernel/audit.h   |  6 +++---
 kernel/auditsc.c | 38 +++++++++++---------------------------
 2 files changed, 14 insertions(+), 30 deletions(-)

Comments

Kees Cook June 22, 2019, 11:12 p.m. UTC | #1
On Fri, Jun 21, 2019 at 11:52:30AM -0700, Casey Schaufler wrote:
> Change the audit code to store full lsmblob data instead of
> a single u32 secid. This allows for multiple security modules
> to use the audit system at the same time. It also allows the
> removal of scaffolding code that was included during the
> revision of LSM interfaces.
> 
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
>  kernel/audit.h   |  6 +++---
>  kernel/auditsc.c | 38 +++++++++++---------------------------
>  2 files changed, 14 insertions(+), 30 deletions(-)
> 
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 29e29c6f4afb..a8dd479e9556 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -91,7 +91,7 @@ struct audit_names {
>  	kuid_t			uid;
>  	kgid_t			gid;
>  	dev_t			rdev;
> -	u32			osid;
> +	struct lsmblob		olsm;
>  	struct audit_cap_data	fcap;
>  	unsigned int		fcap_ver;
>  	unsigned char		type;		/* record type */
> @@ -148,7 +148,7 @@ struct audit_context {
>  	kuid_t		    target_auid;
>  	kuid_t		    target_uid;
>  	unsigned int	    target_sessionid;
> -	struct lsmblob   target_lsm;
> +	struct lsmblob	    target_lsm;
>  	char		    target_comm[TASK_COMM_LEN];
>  
>  	struct audit_tree_refs *trees, *first_trees;
> @@ -165,7 +165,7 @@ struct audit_context {
>  			kuid_t			uid;
>  			kgid_t			gid;
>  			umode_t			mode;
> -			u32			osid;
> +			struct lsmblob		olsm;
>  			int			has_perm;
>  			uid_t			perm_uid;
>  			gid_t			perm_gid;
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 0478680cd0a8..d3ad13f11788 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -646,17 +646,15 @@ static int audit_filter_rules(struct task_struct *tsk,
>  			if (f->lsm_rule) {
>  				/* Find files that match */
>  				if (name) {
> -					lsmblob_init(&blob, name->osid);
>  					result = security_audit_rule_match(
> -								&blob,
> +								&name->olsm,
>  								f->type,
>  								f->op,
>  								f->lsm_rule);
>  				} else if (ctx) {
>  					list_for_each_entry(n, &ctx->names_list, list) {
> -						lsmblob_init(&blob, n->osid);
>  						if (security_audit_rule_match(
> -								&blob,
> +								&n->olsm,
>  								f->type,
>  								f->op,
>  								f->lsm_rule)) {
> @@ -668,8 +666,7 @@ static int audit_filter_rules(struct task_struct *tsk,
>  				/* Find ipc objects that match */
>  				if (!ctx || ctx->type != AUDIT_IPC)
>  					break;
> -				lsmblob_init(&blob, ctx->ipc.osid);
> -				if (security_audit_rule_match(&blob,
> +				if (security_audit_rule_match(&ctx->ipc.olsm,
>  							      f->type, f->op,
>  							      f->lsm_rule))
>  					++result;
> @@ -1187,21 +1184,18 @@ static void show_special(struct audit_context *context, int *call_panic)
>  				context->socketcall.args[i]);
>  		break; }
>  	case AUDIT_IPC: {
> -		u32 osid = context->ipc.osid;
> +		struct lsmblob *olsm = &context->ipc.olsm;
>  
>  		audit_log_format(ab, "ouid=%u ogid=%u mode=%#ho",
>  				 from_kuid(&init_user_ns, context->ipc.uid),
>  				 from_kgid(&init_user_ns, context->ipc.gid),
>  				 context->ipc.mode);
> -		if (osid) {
> +		if (lsmblob_is_set(olsm)) {
>  			struct lsmcontext lsmcxt;
> -			struct lsmblob blob;
>  
> -			lsmblob_init(&blob, osid);
> -			if (security_secid_to_secctx(&blob, &lsmcxt)) {
> -				audit_log_format(ab, " osid=%u", osid);

Should this (and the other) audit_log_format() calls actually be
removed?

-Kees

> +			if (security_secid_to_secctx(olsm, &lsmcxt))
>  				*call_panic = 1;
> -			} else {
> +			else {
>  				audit_log_format(ab, " obj=%s", lsmcxt.context);
>  				security_release_secctx(&lsmcxt);
>  			}
> @@ -1346,13 +1340,10 @@ static void audit_log_name(struct audit_context *context, struct audit_names *n,
>  				 from_kgid(&init_user_ns, n->gid),
>  				 MAJOR(n->rdev),
>  				 MINOR(n->rdev));
> -	if (n->osid != 0) {
> -		struct lsmblob blob;
> +	if (lsmblob_is_set(&n->olsm)) {
>  		struct lsmcontext lsmctx;
>  
> -		lsmblob_init(&blob, n->osid);
> -		if (security_secid_to_secctx(&blob, &lsmctx)) {
> -			audit_log_format(ab, " osid=%u", n->osid);
> +		if (security_secid_to_secctx(&n->olsm, &lsmctx)) {
>  			if (call_panic)
>  				*call_panic = 2;
>  		} else {
> @@ -1906,17 +1897,13 @@ static inline int audit_copy_fcaps(struct audit_names *name,
>  void audit_copy_inode(struct audit_names *name, const struct dentry *dentry,
>  		      struct inode *inode, unsigned int flags)
>  {
> -	struct lsmblob blob;
> -
>  	name->ino   = inode->i_ino;
>  	name->dev   = inode->i_sb->s_dev;
>  	name->mode  = inode->i_mode;
>  	name->uid   = inode->i_uid;
>  	name->gid   = inode->i_gid;
>  	name->rdev  = inode->i_rdev;
> -	security_inode_getsecid(inode, &blob);
> -	/* scaffolding until osid is updated */
> -	name->osid = blob.secid[0];
> +	security_inode_getsecid(inode, &name->olsm);
>  	if (flags & AUDIT_INODE_NOEVAL) {
>  		name->fcap_ver = -1;
>  		return;
> @@ -2266,14 +2253,11 @@ void __audit_mq_getsetattr(mqd_t mqdes, struct mq_attr *mqstat)
>  void __audit_ipc_obj(struct kern_ipc_perm *ipcp)
>  {
>  	struct audit_context *context = audit_context();
> -	struct lsmblob blob;
>  	context->ipc.uid = ipcp->uid;
>  	context->ipc.gid = ipcp->gid;
>  	context->ipc.mode = ipcp->mode;
>  	context->ipc.has_perm = 0;
> -	security_ipc_getsecid(ipcp, &blob);
> -	/* scaffolding on the [0] - change "osid" to a lsmblob */
> -	context->ipc.osid = blob.secid[0];
> +	security_ipc_getsecid(ipcp, &context->ipc.olsm);
>  	context->type = AUDIT_IPC;
>  }
>  
> -- 
> 2.20.1
>
John Johansen June 24, 2019, 9:33 p.m. UTC | #2
On 6/21/19 11:52 AM, Casey Schaufler wrote:
> Change the audit code to store full lsmblob data instead of
> a single u32 secid. This allows for multiple security modules
> to use the audit system at the same time. It also allows the
> removal of scaffolding code that was included during the
> revision of LSM interfaces.
> 
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>

I know Kees raised this too, but I haven't seen a reply

Eric (Paul is already CCed): I have directly added you because of
the question below.

In summary there isn't necessarily a single secid any more, and
we need to know whether dropping the logging of the secid or
logging all secids is the correct action.

> ---
>  kernel/audit.h   |  6 +++---
>  kernel/auditsc.c | 38 +++++++++++---------------------------
>  2 files changed, 14 insertions(+), 30 deletions(-)
> 
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 29e29c6f4afb..a8dd479e9556 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -91,7 +91,7 @@ struct audit_names {
>  	kuid_t			uid;
>  	kgid_t			gid;
>  	dev_t			rdev;
> -	u32			osid;
> +	struct lsmblob		olsm;
>  	struct audit_cap_data	fcap;
>  	unsigned int		fcap_ver;
>  	unsigned char		type;		/* record type */
> @@ -148,7 +148,7 @@ struct audit_context {
>  	kuid_t		    target_auid;
>  	kuid_t		    target_uid;
>  	unsigned int	    target_sessionid;
> -	struct lsmblob   target_lsm;
> +	struct lsmblob	    target_lsm;
>  	char		    target_comm[TASK_COMM_LEN];
>  
>  	struct audit_tree_refs *trees, *first_trees;
> @@ -165,7 +165,7 @@ struct audit_context {
>  			kuid_t			uid;
>  			kgid_t			gid;
>  			umode_t			mode;
> -			u32			osid;
> +			struct lsmblob		olsm;
>  			int			has_perm;
>  			uid_t			perm_uid;
>  			gid_t			perm_gid;
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 0478680cd0a8..d3ad13f11788 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -646,17 +646,15 @@ static int audit_filter_rules(struct task_struct *tsk,
>  			if (f->lsm_rule) {
>  				/* Find files that match */
>  				if (name) {
> -					lsmblob_init(&blob, name->osid);
>  					result = security_audit_rule_match(
> -								&blob,
> +								&name->olsm,
>  								f->type,
>  								f->op,
>  								f->lsm_rule);
>  				} else if (ctx) {
>  					list_for_each_entry(n, &ctx->names_list, list) {
> -						lsmblob_init(&blob, n->osid);
>  						if (security_audit_rule_match(
> -								&blob,
> +								&n->olsm,
>  								f->type,
>  								f->op,
>  								f->lsm_rule)) {
> @@ -668,8 +666,7 @@ static int audit_filter_rules(struct task_struct *tsk,
>  				/* Find ipc objects that match */
>  				if (!ctx || ctx->type != AUDIT_IPC)
>  					break;
> -				lsmblob_init(&blob, ctx->ipc.osid);
> -				if (security_audit_rule_match(&blob,
> +				if (security_audit_rule_match(&ctx->ipc.olsm,
>  							      f->type, f->op,
>  							      f->lsm_rule))
>  					++result;
> @@ -1187,21 +1184,18 @@ static void show_special(struct audit_context *context, int *call_panic)
>  				context->socketcall.args[i]);
>  		break; }
>  	case AUDIT_IPC: {
> -		u32 osid = context->ipc.osid;
> +		struct lsmblob *olsm = &context->ipc.olsm;
>  
>  		audit_log_format(ab, "ouid=%u ogid=%u mode=%#ho",
>  				 from_kuid(&init_user_ns, context->ipc.uid),
>  				 from_kgid(&init_user_ns, context->ipc.gid),
>  				 context->ipc.mode);
> -		if (osid) {
> +		if (lsmblob_is_set(olsm)) {
>  			struct lsmcontext lsmcxt;
> -			struct lsmblob blob;
>  
> -			lsmblob_init(&blob, osid);
> -			if (security_secid_to_secctx(&blob, &lsmcxt)) {
> -				audit_log_format(ab, " osid=%u", osid);

I am not comfortable just dropping this I would think logging all secids is the
correct action here.


> +			if (security_secid_to_secctx(olsm, &lsmcxt))
>  				*call_panic = 1;
> -			} else {
> +			else {
>  				audit_log_format(ab, " obj=%s", lsmcxt.context);
>  				security_release_secctx(&lsmcxt);
>  			}
> @@ -1346,13 +1340,10 @@ static void audit_log_name(struct audit_context *context, struct audit_names *n,
>  				 from_kgid(&init_user_ns, n->gid),
>  				 MAJOR(n->rdev),
>  				 MINOR(n->rdev));
> -	if (n->osid != 0) {
> -		struct lsmblob blob;
> +	if (lsmblob_is_set(&n->olsm)) {
>  		struct lsmcontext lsmctx;
>  
> -		lsmblob_init(&blob, n->osid);
> -		if (security_secid_to_secctx(&blob, &lsmctx)) {
> -			audit_log_format(ab, " osid=%u", n->osid);

and here


> +		if (security_secid_to_secctx(&n->olsm, &lsmctx)) {
>  			if (call_panic)
>  				*call_panic = 2;
>  		} else {
> @@ -1906,17 +1897,13 @@ static inline int audit_copy_fcaps(struct audit_names *name,
>  void audit_copy_inode(struct audit_names *name, const struct dentry *dentry,
>  		      struct inode *inode, unsigned int flags)
>  {
> -	struct lsmblob blob;
> -
>  	name->ino   = inode->i_ino;
>  	name->dev   = inode->i_sb->s_dev;
>  	name->mode  = inode->i_mode;
>  	name->uid   = inode->i_uid;
>  	name->gid   = inode->i_gid;
>  	name->rdev  = inode->i_rdev;
> -	security_inode_getsecid(inode, &blob);
> -	/* scaffolding until osid is updated */
> -	name->osid = blob.secid[0];
> +	security_inode_getsecid(inode, &name->olsm);
>  	if (flags & AUDIT_INODE_NOEVAL) {
>  		name->fcap_ver = -1;
>  		return;
> @@ -2266,14 +2253,11 @@ void __audit_mq_getsetattr(mqd_t mqdes, struct mq_attr *mqstat)
>  void __audit_ipc_obj(struct kern_ipc_perm *ipcp)
>  {
>  	struct audit_context *context = audit_context();
> -	struct lsmblob blob;
>  	context->ipc.uid = ipcp->uid;
>  	context->ipc.gid = ipcp->gid;
>  	context->ipc.mode = ipcp->mode;
>  	context->ipc.has_perm = 0;
> -	security_ipc_getsecid(ipcp, &blob);
> -	/* scaffolding on the [0] - change "osid" to a lsmblob */
> -	context->ipc.osid = blob.secid[0];
> +	security_ipc_getsecid(ipcp, &context->ipc.olsm);
>  	context->type = AUDIT_IPC;
>  }
>  
>
Casey Schaufler June 25, 2019, 1:01 a.m. UTC | #3
On 6/24/2019 2:33 PM, John Johansen wrote:
> On 6/21/19 11:52 AM, Casey Schaufler wrote:
>> Change the audit code to store full lsmblob data instead of
>> a single u32 secid. This allows for multiple security modules
>> to use the audit system at the same time. It also allows the
>> removal of scaffolding code that was included during the
>> revision of LSM interfaces.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> I know Kees raised this too, but I haven't seen a reply
>
> Eric (Paul is already CCed): I have directly added you because of
> the question below.
>
> In summary there isn't necessarily a single secid any more, and
> we need to know whether dropping the logging of the secid or
> logging all secids is the correct action.

It is to be considered that this is an error case. If
everything is working normally you should have produced
a secctx previously, which you'll have included in the
audit record. Including the secid in the record ought to
be pointless, as the secid is strictly an internal token
with no meaning outside the running kernel. You are providing
no security relevant information by providing the secid.
I will grant the possibility that the secid might be useful
in debugging, but for that a pr_warn is more appropriate
than a field in the audit record.

>
>> ---
>>  kernel/audit.h   |  6 +++---
>>  kernel/auditsc.c | 38 +++++++++++---------------------------
>>  2 files changed, 14 insertions(+), 30 deletions(-)
>>
>> diff --git a/kernel/audit.h b/kernel/audit.h
>> index 29e29c6f4afb..a8dd479e9556 100644
>> --- a/kernel/audit.h
>> +++ b/kernel/audit.h
>> @@ -91,7 +91,7 @@ struct audit_names {
>>  	kuid_t			uid;
>>  	kgid_t			gid;
>>  	dev_t			rdev;
>> -	u32			osid;
>> +	struct lsmblob		olsm;
>>  	struct audit_cap_data	fcap;
>>  	unsigned int		fcap_ver;
>>  	unsigned char		type;		/* record type */
>> @@ -148,7 +148,7 @@ struct audit_context {
>>  	kuid_t		    target_auid;
>>  	kuid_t		    target_uid;
>>  	unsigned int	    target_sessionid;
>> -	struct lsmblob   target_lsm;
>> +	struct lsmblob	    target_lsm;
>>  	char		    target_comm[TASK_COMM_LEN];
>>  
>>  	struct audit_tree_refs *trees, *first_trees;
>> @@ -165,7 +165,7 @@ struct audit_context {
>>  			kuid_t			uid;
>>  			kgid_t			gid;
>>  			umode_t			mode;
>> -			u32			osid;
>> +			struct lsmblob		olsm;
>>  			int			has_perm;
>>  			uid_t			perm_uid;
>>  			gid_t			perm_gid;
>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> index 0478680cd0a8..d3ad13f11788 100644
>> --- a/kernel/auditsc.c
>> +++ b/kernel/auditsc.c
>> @@ -646,17 +646,15 @@ static int audit_filter_rules(struct task_struct *tsk,
>>  			if (f->lsm_rule) {
>>  				/* Find files that match */
>>  				if (name) {
>> -					lsmblob_init(&blob, name->osid);
>>  					result = security_audit_rule_match(
>> -								&blob,
>> +								&name->olsm,
>>  								f->type,
>>  								f->op,
>>  								f->lsm_rule);
>>  				} else if (ctx) {
>>  					list_for_each_entry(n, &ctx->names_list, list) {
>> -						lsmblob_init(&blob, n->osid);
>>  						if (security_audit_rule_match(
>> -								&blob,
>> +								&n->olsm,
>>  								f->type,
>>  								f->op,
>>  								f->lsm_rule)) {
>> @@ -668,8 +666,7 @@ static int audit_filter_rules(struct task_struct *tsk,
>>  				/* Find ipc objects that match */
>>  				if (!ctx || ctx->type != AUDIT_IPC)
>>  					break;
>> -				lsmblob_init(&blob, ctx->ipc.osid);
>> -				if (security_audit_rule_match(&blob,
>> +				if (security_audit_rule_match(&ctx->ipc.olsm,
>>  							      f->type, f->op,
>>  							      f->lsm_rule))
>>  					++result;
>> @@ -1187,21 +1184,18 @@ static void show_special(struct audit_context *context, int *call_panic)
>>  				context->socketcall.args[i]);
>>  		break; }
>>  	case AUDIT_IPC: {
>> -		u32 osid = context->ipc.osid;
>> +		struct lsmblob *olsm = &context->ipc.olsm;
>>  
>>  		audit_log_format(ab, "ouid=%u ogid=%u mode=%#ho",
>>  				 from_kuid(&init_user_ns, context->ipc.uid),
>>  				 from_kgid(&init_user_ns, context->ipc.gid),
>>  				 context->ipc.mode);
>> -		if (osid) {
>> +		if (lsmblob_is_set(olsm)) {
>>  			struct lsmcontext lsmcxt;
>> -			struct lsmblob blob;
>>  
>> -			lsmblob_init(&blob, osid);
>> -			if (security_secid_to_secctx(&blob, &lsmcxt)) {
>> -				audit_log_format(ab, " osid=%u", osid);
> I am not comfortable just dropping this I would think logging all secids is the
> correct action here.
>
>
>> +			if (security_secid_to_secctx(olsm, &lsmcxt))
>>  				*call_panic = 1;
>> -			} else {
>> +			else {
>>  				audit_log_format(ab, " obj=%s", lsmcxt.context);
>>  				security_release_secctx(&lsmcxt);
>>  			}
>> @@ -1346,13 +1340,10 @@ static void audit_log_name(struct audit_context *context, struct audit_names *n,
>>  				 from_kgid(&init_user_ns, n->gid),
>>  				 MAJOR(n->rdev),
>>  				 MINOR(n->rdev));
>> -	if (n->osid != 0) {
>> -		struct lsmblob blob;
>> +	if (lsmblob_is_set(&n->olsm)) {
>>  		struct lsmcontext lsmctx;
>>  
>> -		lsmblob_init(&blob, n->osid);
>> -		if (security_secid_to_secctx(&blob, &lsmctx)) {
>> -			audit_log_format(ab, " osid=%u", n->osid);
> and here
>
>
>> +		if (security_secid_to_secctx(&n->olsm, &lsmctx)) {
>>  			if (call_panic)
>>  				*call_panic = 2;
>>  		} else {
>> @@ -1906,17 +1897,13 @@ static inline int audit_copy_fcaps(struct audit_names *name,
>>  void audit_copy_inode(struct audit_names *name, const struct dentry *dentry,
>>  		      struct inode *inode, unsigned int flags)
>>  {
>> -	struct lsmblob blob;
>> -
>>  	name->ino   = inode->i_ino;
>>  	name->dev   = inode->i_sb->s_dev;
>>  	name->mode  = inode->i_mode;
>>  	name->uid   = inode->i_uid;
>>  	name->gid   = inode->i_gid;
>>  	name->rdev  = inode->i_rdev;
>> -	security_inode_getsecid(inode, &blob);
>> -	/* scaffolding until osid is updated */
>> -	name->osid = blob.secid[0];
>> +	security_inode_getsecid(inode, &name->olsm);
>>  	if (flags & AUDIT_INODE_NOEVAL) {
>>  		name->fcap_ver = -1;
>>  		return;
>> @@ -2266,14 +2253,11 @@ void __audit_mq_getsetattr(mqd_t mqdes, struct mq_attr *mqstat)
>>  void __audit_ipc_obj(struct kern_ipc_perm *ipcp)
>>  {
>>  	struct audit_context *context = audit_context();
>> -	struct lsmblob blob;
>>  	context->ipc.uid = ipcp->uid;
>>  	context->ipc.gid = ipcp->gid;
>>  	context->ipc.mode = ipcp->mode;
>>  	context->ipc.has_perm = 0;
>> -	security_ipc_getsecid(ipcp, &blob);
>> -	/* scaffolding on the [0] - change "osid" to a lsmblob */
>> -	context->ipc.osid = blob.secid[0];
>> +	security_ipc_getsecid(ipcp, &context->ipc.olsm);
>>  	context->type = AUDIT_IPC;
>>  }
>>  
>>
Paul Moore June 25, 2019, 1:46 a.m. UTC | #4
On Mon, Jun 24, 2019 at 9:01 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 6/24/2019 2:33 PM, John Johansen wrote:
> > On 6/21/19 11:52 AM, Casey Schaufler wrote:
> >> Change the audit code to store full lsmblob data instead of
> >> a single u32 secid. This allows for multiple security modules
> >> to use the audit system at the same time. It also allows the
> >> removal of scaffolding code that was included during the
> >> revision of LSM interfaces.
> >>
> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> > I know Kees raised this too, but I haven't seen a reply
> >
> > Eric (Paul is already CCed): I have directly added you because of
> > the question below.
> >
> > In summary there isn't necessarily a single secid any more, and
> > we need to know whether dropping the logging of the secid or
> > logging all secids is the correct action.
>
> It is to be considered that this is an error case. If
> everything is working normally you should have produced
> a secctx previously, which you'll have included in the
> audit record. Including the secid in the record ought to
> be pointless, as the secid is strictly an internal token
> with no meaning outside the running kernel. You are providing
> no security relevant information by providing the secid.
> I will grant the possibility that the secid might be useful
> in debugging, but for that a pr_warn is more appropriate
> than a field in the audit record.

FWIW, this probably should have been CC'd to the audit list.

I agree that this is an error case (security_secid_to_secctx() failed
to resolve the secid) and further that logging the secid, or a
collection of secids, has little value the way things currently work.
Since secids are a private kernel implementation detail, we don't
really display them outside the context of the kernel, including in
the audit logs.  Recording a secid in this case doesn't provide
anything meaningful since secids aren't recorded in the audit record
stream, only the secctxs, and there is no "magic decoder ring" to go
between the two in the audit logs, or anywhere else in userspace for
that matter.

> >> ---
> >>  kernel/audit.h   |  6 +++---
> >>  kernel/auditsc.c | 38 +++++++++++---------------------------
> >>  2 files changed, 14 insertions(+), 30 deletions(-)

...

> >> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> >> index 0478680cd0a8..d3ad13f11788 100644
> >> --- a/kernel/auditsc.c
> >> +++ b/kernel/auditsc.c
> >> @@ -1187,21 +1184,18 @@ static void show_special(struct audit_context *context, int *call_panic)
> >>                              context->socketcall.args[i]);
> >>              break; }
> >>      case AUDIT_IPC: {
> >> -            u32 osid = context->ipc.osid;
> >> +            struct lsmblob *olsm = &context->ipc.olsm;
> >>
> >>              audit_log_format(ab, "ouid=%u ogid=%u mode=%#ho",
> >>                               from_kuid(&init_user_ns, context->ipc.uid),
> >>                               from_kgid(&init_user_ns, context->ipc.gid),
> >>                               context->ipc.mode);
> >> -            if (osid) {
> >> +            if (lsmblob_is_set(olsm)) {
> >>                      struct lsmcontext lsmcxt;
> >> -                    struct lsmblob blob;
> >>
> >> -                    lsmblob_init(&blob, osid);
> >> -                    if (security_secid_to_secctx(&blob, &lsmcxt)) {
> >> -                            audit_log_format(ab, " osid=%u", osid);
> > I am not comfortable just dropping this I would think logging all secids is the
> > correct action here.
> >
> >
> >> +                    if (security_secid_to_secctx(olsm, &lsmcxt))
> >>                              *call_panic = 1;
> >> -                    } else {
> >> +                    else {
> >>                              audit_log_format(ab, " obj=%s", lsmcxt.context);
> >>                              security_release_secctx(&lsmcxt);
> >>                      }
John Johansen June 25, 2019, 2:14 a.m. UTC | #5
On 6/24/19 6:46 PM, Paul Moore wrote:
> On Mon, Jun 24, 2019 at 9:01 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 6/24/2019 2:33 PM, John Johansen wrote:
>>> On 6/21/19 11:52 AM, Casey Schaufler wrote:
>>>> Change the audit code to store full lsmblob data instead of
>>>> a single u32 secid. This allows for multiple security modules
>>>> to use the audit system at the same time. It also allows the
>>>> removal of scaffolding code that was included during the
>>>> revision of LSM interfaces.
>>>>
>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>> I know Kees raised this too, but I haven't seen a reply
>>>
>>> Eric (Paul is already CCed): I have directly added you because of
>>> the question below.
>>>
>>> In summary there isn't necessarily a single secid any more, and
>>> we need to know whether dropping the logging of the secid or
>>> logging all secids is the correct action.
>>
>> It is to be considered that this is an error case. If
>> everything is working normally you should have produced
>> a secctx previously, which you'll have included in the
>> audit record. Including the secid in the record ought to
>> be pointless, as the secid is strictly an internal token
>> with no meaning outside the running kernel. You are providing
>> no security relevant information by providing the secid.
>> I will grant the possibility that the secid might be useful
>> in debugging, but for that a pr_warn is more appropriate
>> than a field in the audit record.
> 
> FWIW, this probably should have been CC'd to the audit list.
> 
hrmm indeed, sorry

> I agree that this is an error case (security_secid_to_secctx() failed
> to resolve the secid) and further that logging the secid, or a
> collection of secids, has little value the way things currently work.
> Since secids are a private kernel implementation detail, we don't
> really display them outside the context of the kernel, including in
> the audit logs.  Recording a secid in this case doesn't provide
> anything meaningful since secids aren't recorded in the audit record
> stream, only the secctxs, and there is no "magic decoder ring" to go
> between the two in the audit logs, or anywhere else in userspace for
> that matter.
> 
Okay, thanks. Casey I am good with just a pr_warn here. I just didn't
have context of why it was going to the audit_log and didn't want
to change that without some more input.


>>>> ---
>>>>  kernel/audit.h   |  6 +++---
>>>>  kernel/auditsc.c | 38 +++++++++++---------------------------
>>>>  2 files changed, 14 insertions(+), 30 deletions(-)
> 
> ...
> 
>>>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>>>> index 0478680cd0a8..d3ad13f11788 100644
>>>> --- a/kernel/auditsc.c
>>>> +++ b/kernel/auditsc.c
>>>> @@ -1187,21 +1184,18 @@ static void show_special(struct audit_context *context, int *call_panic)
>>>>                              context->socketcall.args[i]);
>>>>              break; }
>>>>      case AUDIT_IPC: {
>>>> -            u32 osid = context->ipc.osid;
>>>> +            struct lsmblob *olsm = &context->ipc.olsm;
>>>>
>>>>              audit_log_format(ab, "ouid=%u ogid=%u mode=%#ho",
>>>>                               from_kuid(&init_user_ns, context->ipc.uid),
>>>>                               from_kgid(&init_user_ns, context->ipc.gid),
>>>>                               context->ipc.mode);
>>>> -            if (osid) {
>>>> +            if (lsmblob_is_set(olsm)) {
>>>>                      struct lsmcontext lsmcxt;
>>>> -                    struct lsmblob blob;
>>>>
>>>> -                    lsmblob_init(&blob, osid);
>>>> -                    if (security_secid_to_secctx(&blob, &lsmcxt)) {
>>>> -                            audit_log_format(ab, " osid=%u", osid);
>>> I am not comfortable just dropping this I would think logging all secids is the
>>> correct action here.
>>>
>>>
>>>> +                    if (security_secid_to_secctx(olsm, &lsmcxt))
>>>>                              *call_panic = 1;
>>>> -                    } else {
>>>> +                    else {
>>>>                              audit_log_format(ab, " obj=%s", lsmcxt.context);
>>>>                              security_release_secctx(&lsmcxt);
>>>>                      }
>
Paul Moore June 25, 2019, 2:42 a.m. UTC | #6
On Mon, Jun 24, 2019 at 10:15 PM John Johansen
<john.johansen@canonical.com> wrote:
> On 6/24/19 6:46 PM, Paul Moore wrote:
> > On Mon, Jun 24, 2019 at 9:01 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 6/24/2019 2:33 PM, John Johansen wrote:
> >>> On 6/21/19 11:52 AM, Casey Schaufler wrote:
> >>>> Change the audit code to store full lsmblob data instead of
> >>>> a single u32 secid. This allows for multiple security modules
> >>>> to use the audit system at the same time. It also allows the
> >>>> removal of scaffolding code that was included during the
> >>>> revision of LSM interfaces.
> >>>>
> >>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> >>> I know Kees raised this too, but I haven't seen a reply
> >>>
> >>> Eric (Paul is already CCed): I have directly added you because of
> >>> the question below.
> >>>
> >>> In summary there isn't necessarily a single secid any more, and
> >>> we need to know whether dropping the logging of the secid or
> >>> logging all secids is the correct action.
> >>
> >> It is to be considered that this is an error case. If
> >> everything is working normally you should have produced
> >> a secctx previously, which you'll have included in the
> >> audit record. Including the secid in the record ought to
> >> be pointless, as the secid is strictly an internal token
> >> with no meaning outside the running kernel. You are providing
> >> no security relevant information by providing the secid.
> >> I will grant the possibility that the secid might be useful
> >> in debugging, but for that a pr_warn is more appropriate
> >> than a field in the audit record.
> >
> > FWIW, this probably should have been CC'd to the audit list.
> >
> hrmm indeed, sorry
>
> > I agree that this is an error case (security_secid_to_secctx() failed
> > to resolve the secid) and further that logging the secid, or a
> > collection of secids, has little value the way things currently work.
> > Since secids are a private kernel implementation detail, we don't
> > really display them outside the context of the kernel, including in
> > the audit logs.  Recording a secid in this case doesn't provide
> > anything meaningful since secids aren't recorded in the audit record
> > stream, only the secctxs, and there is no "magic decoder ring" to go
> > between the two in the audit logs, or anywhere else in userspace for
> > that matter.
>
> Okay, thanks. Casey I am good with just a pr_warn here. I just didn't
> have context of why it was going to the audit_log and didn't want
> to change that without some more input.

Hmm.  Actually, let me change my comments slightly ... perhaps what we
should do here is keep the audit_log_format(), but change it from
audit_log_format("osid=%u",...) to audit_log_format("obj=?").  The "?"
is used in audit when we can't determine a piece of information, but
we normally log it.  It wasn't used very widely originally, which is
probably why it isn't in this piece of code.

> >>>> ---
> >>>>  kernel/audit.h   |  6 +++---
> >>>>  kernel/auditsc.c | 38 +++++++++++---------------------------
> >>>>  2 files changed, 14 insertions(+), 30 deletions(-)
> >
> > ...
> >
> >>>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> >>>> index 0478680cd0a8..d3ad13f11788 100644
> >>>> --- a/kernel/auditsc.c
> >>>> +++ b/kernel/auditsc.c
> >>>> @@ -1187,21 +1184,18 @@ static void show_special(struct audit_context *context, int *call_panic)
> >>>>                              context->socketcall.args[i]);
> >>>>              break; }
> >>>>      case AUDIT_IPC: {
> >>>> -            u32 osid = context->ipc.osid;
> >>>> +            struct lsmblob *olsm = &context->ipc.olsm;
> >>>>
> >>>>              audit_log_format(ab, "ouid=%u ogid=%u mode=%#ho",
> >>>>                               from_kuid(&init_user_ns, context->ipc.uid),
> >>>>                               from_kgid(&init_user_ns, context->ipc.gid),
> >>>>                               context->ipc.mode);
> >>>> -            if (osid) {
> >>>> +            if (lsmblob_is_set(olsm)) {
> >>>>                      struct lsmcontext lsmcxt;
> >>>> -                    struct lsmblob blob;
> >>>>
> >>>> -                    lsmblob_init(&blob, osid);
> >>>> -                    if (security_secid_to_secctx(&blob, &lsmcxt)) {
> >>>> -                            audit_log_format(ab, " osid=%u", osid);
> >>> I am not comfortable just dropping this I would think logging all secids is the
> >>> correct action here.
> >>>
> >>>
> >>>> +                    if (security_secid_to_secctx(olsm, &lsmcxt))
> >>>>                              *call_panic = 1;
> >>>> -                    } else {
> >>>> +                    else {
> >>>>                              audit_log_format(ab, " obj=%s", lsmcxt.context);
> >>>>                              security_release_secctx(&lsmcxt);
> >>>>                      }
Casey Schaufler June 25, 2019, 3:30 p.m. UTC | #7
On 6/24/2019 7:42 PM, Paul Moore wrote:
> On Mon, Jun 24, 2019 at 10:15 PM John Johansen
> <john.johansen@canonical.com> wrote:
>> On 6/24/19 6:46 PM, Paul Moore wrote:
>>> On Mon, Jun 24, 2019 at 9:01 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> On 6/24/2019 2:33 PM, John Johansen wrote:
>>>>> On 6/21/19 11:52 AM, Casey Schaufler wrote:
>>>>>> Change the audit code to store full lsmblob data instead of
>>>>>> a single u32 secid. This allows for multiple security modules
>>>>>> to use the audit system at the same time. It also allows the
>>>>>> removal of scaffolding code that was included during the
>>>>>> revision of LSM interfaces.
>>>>>>
>>>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>>>> I know Kees raised this too, but I haven't seen a reply
>>>>>
>>>>> Eric (Paul is already CCed): I have directly added you because of
>>>>> the question below.
>>>>>
>>>>> In summary there isn't necessarily a single secid any more, and
>>>>> we need to know whether dropping the logging of the secid or
>>>>> logging all secids is the correct action.
>>>> It is to be considered that this is an error case. If
>>>> everything is working normally you should have produced
>>>> a secctx previously, which you'll have included in the
>>>> audit record. Including the secid in the record ought to
>>>> be pointless, as the secid is strictly an internal token
>>>> with no meaning outside the running kernel. You are providing
>>>> no security relevant information by providing the secid.
>>>> I will grant the possibility that the secid might be useful
>>>> in debugging, but for that a pr_warn is more appropriate
>>>> than a field in the audit record.
>>> FWIW, this probably should have been CC'd to the audit list.
>>>
>> hrmm indeed, sorry
>>
>>> I agree that this is an error case (security_secid_to_secctx() failed
>>> to resolve the secid) and further that logging the secid, or a
>>> collection of secids, has little value the way things currently work.
>>> Since secids are a private kernel implementation detail, we don't
>>> really display them outside the context of the kernel, including in
>>> the audit logs.  Recording a secid in this case doesn't provide
>>> anything meaningful since secids aren't recorded in the audit record
>>> stream, only the secctxs, and there is no "magic decoder ring" to go
>>> between the two in the audit logs, or anywhere else in userspace for
>>> that matter.
>> Okay, thanks. Casey I am good with just a pr_warn here. I just didn't
>> have context of why it was going to the audit_log and didn't want
>> to change that without some more input.
> Hmm.  Actually, let me change my comments slightly ... perhaps what we
> should do here is keep the audit_log_format(), but change it from
> audit_log_format("osid=%u",...) to audit_log_format("obj=?").  The "?"
> is used in audit when we can't determine a piece of information, but
> we normally log it.  It wasn't used very widely originally, which is
> probably why it isn't in this piece of code.

Works for me. I'll make the change. Thank you.
??

Patch
diff mbox series

diff --git a/kernel/audit.h b/kernel/audit.h
index 29e29c6f4afb..a8dd479e9556 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -91,7 +91,7 @@  struct audit_names {
 	kuid_t			uid;
 	kgid_t			gid;
 	dev_t			rdev;
-	u32			osid;
+	struct lsmblob		olsm;
 	struct audit_cap_data	fcap;
 	unsigned int		fcap_ver;
 	unsigned char		type;		/* record type */
@@ -148,7 +148,7 @@  struct audit_context {
 	kuid_t		    target_auid;
 	kuid_t		    target_uid;
 	unsigned int	    target_sessionid;
-	struct lsmblob   target_lsm;
+	struct lsmblob	    target_lsm;
 	char		    target_comm[TASK_COMM_LEN];
 
 	struct audit_tree_refs *trees, *first_trees;
@@ -165,7 +165,7 @@  struct audit_context {
 			kuid_t			uid;
 			kgid_t			gid;
 			umode_t			mode;
-			u32			osid;
+			struct lsmblob		olsm;
 			int			has_perm;
 			uid_t			perm_uid;
 			gid_t			perm_gid;
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 0478680cd0a8..d3ad13f11788 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -646,17 +646,15 @@  static int audit_filter_rules(struct task_struct *tsk,
 			if (f->lsm_rule) {
 				/* Find files that match */
 				if (name) {
-					lsmblob_init(&blob, name->osid);
 					result = security_audit_rule_match(
-								&blob,
+								&name->olsm,
 								f->type,
 								f->op,
 								f->lsm_rule);
 				} else if (ctx) {
 					list_for_each_entry(n, &ctx->names_list, list) {
-						lsmblob_init(&blob, n->osid);
 						if (security_audit_rule_match(
-								&blob,
+								&n->olsm,
 								f->type,
 								f->op,
 								f->lsm_rule)) {
@@ -668,8 +666,7 @@  static int audit_filter_rules(struct task_struct *tsk,
 				/* Find ipc objects that match */
 				if (!ctx || ctx->type != AUDIT_IPC)
 					break;
-				lsmblob_init(&blob, ctx->ipc.osid);
-				if (security_audit_rule_match(&blob,
+				if (security_audit_rule_match(&ctx->ipc.olsm,
 							      f->type, f->op,
 							      f->lsm_rule))
 					++result;
@@ -1187,21 +1184,18 @@  static void show_special(struct audit_context *context, int *call_panic)
 				context->socketcall.args[i]);
 		break; }
 	case AUDIT_IPC: {
-		u32 osid = context->ipc.osid;
+		struct lsmblob *olsm = &context->ipc.olsm;
 
 		audit_log_format(ab, "ouid=%u ogid=%u mode=%#ho",
 				 from_kuid(&init_user_ns, context->ipc.uid),
 				 from_kgid(&init_user_ns, context->ipc.gid),
 				 context->ipc.mode);
-		if (osid) {
+		if (lsmblob_is_set(olsm)) {
 			struct lsmcontext lsmcxt;
-			struct lsmblob blob;
 
-			lsmblob_init(&blob, osid);
-			if (security_secid_to_secctx(&blob, &lsmcxt)) {
-				audit_log_format(ab, " osid=%u", osid);
+			if (security_secid_to_secctx(olsm, &lsmcxt))
 				*call_panic = 1;
-			} else {
+			else {
 				audit_log_format(ab, " obj=%s", lsmcxt.context);
 				security_release_secctx(&lsmcxt);
 			}
@@ -1346,13 +1340,10 @@  static void audit_log_name(struct audit_context *context, struct audit_names *n,
 				 from_kgid(&init_user_ns, n->gid),
 				 MAJOR(n->rdev),
 				 MINOR(n->rdev));
-	if (n->osid != 0) {
-		struct lsmblob blob;
+	if (lsmblob_is_set(&n->olsm)) {
 		struct lsmcontext lsmctx;
 
-		lsmblob_init(&blob, n->osid);
-		if (security_secid_to_secctx(&blob, &lsmctx)) {
-			audit_log_format(ab, " osid=%u", n->osid);
+		if (security_secid_to_secctx(&n->olsm, &lsmctx)) {
 			if (call_panic)
 				*call_panic = 2;
 		} else {
@@ -1906,17 +1897,13 @@  static inline int audit_copy_fcaps(struct audit_names *name,
 void audit_copy_inode(struct audit_names *name, const struct dentry *dentry,
 		      struct inode *inode, unsigned int flags)
 {
-	struct lsmblob blob;
-
 	name->ino   = inode->i_ino;
 	name->dev   = inode->i_sb->s_dev;
 	name->mode  = inode->i_mode;
 	name->uid   = inode->i_uid;
 	name->gid   = inode->i_gid;
 	name->rdev  = inode->i_rdev;
-	security_inode_getsecid(inode, &blob);
-	/* scaffolding until osid is updated */
-	name->osid = blob.secid[0];
+	security_inode_getsecid(inode, &name->olsm);
 	if (flags & AUDIT_INODE_NOEVAL) {
 		name->fcap_ver = -1;
 		return;
@@ -2266,14 +2253,11 @@  void __audit_mq_getsetattr(mqd_t mqdes, struct mq_attr *mqstat)
 void __audit_ipc_obj(struct kern_ipc_perm *ipcp)
 {
 	struct audit_context *context = audit_context();
-	struct lsmblob blob;
 	context->ipc.uid = ipcp->uid;
 	context->ipc.gid = ipcp->gid;
 	context->ipc.mode = ipcp->mode;
 	context->ipc.has_perm = 0;
-	security_ipc_getsecid(ipcp, &blob);
-	/* scaffolding on the [0] - change "osid" to a lsmblob */
-	context->ipc.osid = blob.secid[0];
+	security_ipc_getsecid(ipcp, &context->ipc.olsm);
 	context->type = AUDIT_IPC;
 }