| Message ID | 20190621185233.6766-11-casey@schaufler-ca.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | LSM: Module stacking for AppArmor | expand |
On Fri, Jun 21, 2019 at 11:52:19AM -0700, Casey Schaufler wrote: > There may be more than one LSM that provides IPC data > for auditing. Change security_ipc_getsecid() to fill in > a lsmblob structure instead of the u32 secid. The > audit data structure containing the secid will be updated > later, so there is a bit of scaffolding here. > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> Reviewed-by: Kees Cook <keescook@chromium.org> One thought below... > --- > include/linux/security.h | 7 ++++--- > kernel/auditsc.c | 5 ++++- > security/security.c | 9 ++++++--- > 3 files changed, 14 insertions(+), 7 deletions(-) > > diff --git a/include/linux/security.h b/include/linux/security.h > index c6cddeff8a17..0d5e172341fc 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -413,7 +413,7 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3, > unsigned long arg4, unsigned long arg5); > void security_task_to_inode(struct task_struct *p, struct inode *inode); > int security_ipc_permission(struct kern_ipc_perm *ipcp, short flag); > -void security_ipc_getsecid(struct kern_ipc_perm *ipcp, u32 *secid); > +void security_ipc_getsecid(struct kern_ipc_perm *ipcp, struct lsmblob *blob); > int security_msg_msg_alloc(struct msg_msg *msg); > void security_msg_msg_free(struct msg_msg *msg); > int security_msg_queue_alloc(struct kern_ipc_perm *msq); > @@ -1098,9 +1098,10 @@ static inline int security_ipc_permission(struct kern_ipc_perm *ipcp, > return 0; > } > > -static inline void security_ipc_getsecid(struct kern_ipc_perm *ipcp, u32 *secid) > +static inline void security_ipc_getsecid(struct kern_ipc_perm *ipcp, > + struct lsmblob *blob) > { > - *secid = 0; > + lsmblob_init(blob, 0); > } > > static inline int security_msg_msg_alloc(struct msg_msg *msg) > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index d31914088a82..148733ec3c72 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -2268,11 +2268,14 @@ 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, &context->ipc.osid); > + security_ipc_getsecid(ipcp, &blob); > + /* scaffolding on the [0] - change "osid" to a lsmblob */ > + context->ipc.osid = blob.secid[0]; > context->type = AUDIT_IPC; > } > > diff --git a/security/security.c b/security/security.c > index 5ab07631df75..d55f01041f05 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -1812,10 +1812,13 @@ int security_ipc_permission(struct kern_ipc_perm *ipcp, short flag) > return call_int_hook(ipc_permission, 0, ipcp, flag); > } > > -void security_ipc_getsecid(struct kern_ipc_perm *ipcp, u32 *secid) > +void security_ipc_getsecid(struct kern_ipc_perm *ipcp, struct lsmblob *blob) > { > - *secid = 0; > - call_void_hook(ipc_getsecid, ipcp, secid); > + struct security_hook_list *hp; > + > + lsmblob_init(blob, 0); > + hlist_for_each_entry(hp, &security_hook_heads.ipc_getsecid, list) > + hp->hook.ipc_getsecid(ipcp, &blob->secid[hp->slot]); Just for sanity when using hp->slot, it might be good to do something like this in the places it gets used. Like for here: if (!WARN_ON(hp->slot < 0 || hp->slot >= LSMBLOB_COUNT)) hp->hook.ipc_getsecid(ipcp, &blob->secid[hp->slot]); This _should_ be overkill, but since lists of hooks that trigger slot assignment is hardcoded, it seems nice to cover any future problems or mismatches.
On 6/22/2019 3:48 PM, Kees Cook wrote: > On Fri, Jun 21, 2019 at 11:52:19AM -0700, Casey Schaufler wrote: >> There may be more than one LSM that provides IPC data >> for auditing. Change security_ipc_getsecid() to fill in >> a lsmblob structure instead of the u32 secid. The >> audit data structure containing the secid will be updated >> later, so there is a bit of scaffolding here. >> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > Reviewed-by: Kees Cook <keescook@chromium.org> > > One thought below... > >> --- >> include/linux/security.h | 7 ++++--- >> kernel/auditsc.c | 5 ++++- >> security/security.c | 9 ++++++--- >> 3 files changed, 14 insertions(+), 7 deletions(-) >> >> diff --git a/include/linux/security.h b/include/linux/security.h >> index c6cddeff8a17..0d5e172341fc 100644 >> --- a/include/linux/security.h >> +++ b/include/linux/security.h >> @@ -413,7 +413,7 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3, >> unsigned long arg4, unsigned long arg5); >> void security_task_to_inode(struct task_struct *p, struct inode *inode); >> int security_ipc_permission(struct kern_ipc_perm *ipcp, short flag); >> -void security_ipc_getsecid(struct kern_ipc_perm *ipcp, u32 *secid); >> +void security_ipc_getsecid(struct kern_ipc_perm *ipcp, struct lsmblob *blob); >> int security_msg_msg_alloc(struct msg_msg *msg); >> void security_msg_msg_free(struct msg_msg *msg); >> int security_msg_queue_alloc(struct kern_ipc_perm *msq); >> @@ -1098,9 +1098,10 @@ static inline int security_ipc_permission(struct kern_ipc_perm *ipcp, >> return 0; >> } >> >> -static inline void security_ipc_getsecid(struct kern_ipc_perm *ipcp, u32 *secid) >> +static inline void security_ipc_getsecid(struct kern_ipc_perm *ipcp, >> + struct lsmblob *blob) >> { >> - *secid = 0; >> + lsmblob_init(blob, 0); >> } >> >> static inline int security_msg_msg_alloc(struct msg_msg *msg) >> diff --git a/kernel/auditsc.c b/kernel/auditsc.c >> index d31914088a82..148733ec3c72 100644 >> --- a/kernel/auditsc.c >> +++ b/kernel/auditsc.c >> @@ -2268,11 +2268,14 @@ 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, &context->ipc.osid); >> + security_ipc_getsecid(ipcp, &blob); >> + /* scaffolding on the [0] - change "osid" to a lsmblob */ >> + context->ipc.osid = blob.secid[0]; >> context->type = AUDIT_IPC; >> } >> >> diff --git a/security/security.c b/security/security.c >> index 5ab07631df75..d55f01041f05 100644 >> --- a/security/security.c >> +++ b/security/security.c >> @@ -1812,10 +1812,13 @@ int security_ipc_permission(struct kern_ipc_perm *ipcp, short flag) >> return call_int_hook(ipc_permission, 0, ipcp, flag); >> } >> >> -void security_ipc_getsecid(struct kern_ipc_perm *ipcp, u32 *secid) >> +void security_ipc_getsecid(struct kern_ipc_perm *ipcp, struct lsmblob *blob) >> { >> - *secid = 0; >> - call_void_hook(ipc_getsecid, ipcp, secid); >> + struct security_hook_list *hp; >> + >> + lsmblob_init(blob, 0); >> + hlist_for_each_entry(hp, &security_hook_heads.ipc_getsecid, list) >> + hp->hook.ipc_getsecid(ipcp, &blob->secid[hp->slot]); > Just for sanity when using hp->slot, it might be good to do something > like this in the places it gets used. Like for here: > > if (!WARN_ON(hp->slot < 0 || hp->slot >= LSMBLOB_COUNT)) > hp->hook.ipc_getsecid(ipcp, &blob->secid[hp->slot]); > > This _should_ be overkill, but since lists of hooks that trigger slot > assignment is hardcoded, it seems nice to cover any future problems or > mismatches. How about a CONFIG_LSM_SLOT_CHECK around a function lsm_slot_check()? If configured, it does the WARN_ON, and if not it's a static inline true return. As you say, it's probably overkill, but it would be available for the paranoid/debug/bringup situation.
On 6/24/2019 9:39 AM, Casey Schaufler wrote: > On 6/22/2019 3:48 PM, Kees Cook wrote: >> On Fri, Jun 21, 2019 at 11:52:19AM -0700, Casey Schaufler wrote: >>> There may be more than one LSM that provides IPC data >>> for auditing. Change security_ipc_getsecid() to fill in >>> a lsmblob structure instead of the u32 secid. The >>> audit data structure containing the secid will be updated >>> later, so there is a bit of scaffolding here. >>> >>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >> Reviewed-by: Kees Cook <keescook@chromium.org> >> >> One thought below... >> >>> --- >>> include/linux/security.h | 7 ++++--- >>> kernel/auditsc.c | 5 ++++- >>> security/security.c | 9 ++++++--- >>> 3 files changed, 14 insertions(+), 7 deletions(-) >>> >>> diff --git a/include/linux/security.h b/include/linux/security.h >>> index c6cddeff8a17..0d5e172341fc 100644 >>> --- a/include/linux/security.h >>> +++ b/include/linux/security.h >>> @@ -413,7 +413,7 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3, >>> unsigned long arg4, unsigned long arg5); >>> void security_task_to_inode(struct task_struct *p, struct inode *inode); >>> int security_ipc_permission(struct kern_ipc_perm *ipcp, short flag); >>> -void security_ipc_getsecid(struct kern_ipc_perm *ipcp, u32 *secid); >>> +void security_ipc_getsecid(struct kern_ipc_perm *ipcp, struct lsmblob *blob); >>> int security_msg_msg_alloc(struct msg_msg *msg); >>> void security_msg_msg_free(struct msg_msg *msg); >>> int security_msg_queue_alloc(struct kern_ipc_perm *msq); >>> @@ -1098,9 +1098,10 @@ static inline int security_ipc_permission(struct kern_ipc_perm *ipcp, >>> return 0; >>> } >>> >>> -static inline void security_ipc_getsecid(struct kern_ipc_perm *ipcp, u32 *secid) >>> +static inline void security_ipc_getsecid(struct kern_ipc_perm *ipcp, >>> + struct lsmblob *blob) >>> { >>> - *secid = 0; >>> + lsmblob_init(blob, 0); >>> } >>> >>> static inline int security_msg_msg_alloc(struct msg_msg *msg) >>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c >>> index d31914088a82..148733ec3c72 100644 >>> --- a/kernel/auditsc.c >>> +++ b/kernel/auditsc.c >>> @@ -2268,11 +2268,14 @@ 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, &context->ipc.osid); >>> + security_ipc_getsecid(ipcp, &blob); >>> + /* scaffolding on the [0] - change "osid" to a lsmblob */ >>> + context->ipc.osid = blob.secid[0]; >>> context->type = AUDIT_IPC; >>> } >>> >>> diff --git a/security/security.c b/security/security.c >>> index 5ab07631df75..d55f01041f05 100644 >>> --- a/security/security.c >>> +++ b/security/security.c >>> @@ -1812,10 +1812,13 @@ int security_ipc_permission(struct kern_ipc_perm *ipcp, short flag) >>> return call_int_hook(ipc_permission, 0, ipcp, flag); >>> } >>> >>> -void security_ipc_getsecid(struct kern_ipc_perm *ipcp, u32 *secid) >>> +void security_ipc_getsecid(struct kern_ipc_perm *ipcp, struct lsmblob *blob) >>> { >>> - *secid = 0; >>> - call_void_hook(ipc_getsecid, ipcp, secid); >>> + struct security_hook_list *hp; >>> + >>> + lsmblob_init(blob, 0); >>> + hlist_for_each_entry(hp, &security_hook_heads.ipc_getsecid, list) >>> + hp->hook.ipc_getsecid(ipcp, &blob->secid[hp->slot]); >> Just for sanity when using hp->slot, it might be good to do something >> like this in the places it gets used. Like for here: >> >> if (!WARN_ON(hp->slot < 0 || hp->slot >= LSMBLOB_COUNT)) >> hp->hook.ipc_getsecid(ipcp, &blob->secid[hp->slot]); >> >> This _should_ be overkill, but since lists of hooks that trigger slot >> assignment is hardcoded, it seems nice to cover any future problems or >> mismatches. > How about a CONFIG_LSM_SLOT_CHECK around a function lsm_slot_check()? > If configured, it does the WARN_ON, and if not it's a static inline > true return. As you say, it's probably overkill, but it would be available > for the paranoid/debug/bringup situation. ... or maybe it would be better if security_add_hooks() had an explicit parameter whereby an LSM can tell the infrastructure that it wants a slot. The only future problems would come if the LSM writer got the parameter wrong or if secid dependence got added to an existing interface and a LSM that didn't used to need a slot suddenly did. I don't see either of those as compelling.
On 6/21/19 11:52 AM, Casey Schaufler wrote: > There may be more than one LSM that provides IPC data > for auditing. Change security_ipc_getsecid() to fill in > a lsmblob structure instead of the u32 secid. The > audit data structure containing the secid will be updated > later, so there is a bit of scaffolding here. > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> Reviewed-by: John Johansen <john.johansen@canonical.com> > --- > include/linux/security.h | 7 ++++--- > kernel/auditsc.c | 5 ++++- > security/security.c | 9 ++++++--- > 3 files changed, 14 insertions(+), 7 deletions(-) > > diff --git a/include/linux/security.h b/include/linux/security.h > index c6cddeff8a17..0d5e172341fc 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -413,7 +413,7 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3, > unsigned long arg4, unsigned long arg5); > void security_task_to_inode(struct task_struct *p, struct inode *inode); > int security_ipc_permission(struct kern_ipc_perm *ipcp, short flag); > -void security_ipc_getsecid(struct kern_ipc_perm *ipcp, u32 *secid); > +void security_ipc_getsecid(struct kern_ipc_perm *ipcp, struct lsmblob *blob); > int security_msg_msg_alloc(struct msg_msg *msg); > void security_msg_msg_free(struct msg_msg *msg); > int security_msg_queue_alloc(struct kern_ipc_perm *msq); > @@ -1098,9 +1098,10 @@ static inline int security_ipc_permission(struct kern_ipc_perm *ipcp, > return 0; > } > > -static inline void security_ipc_getsecid(struct kern_ipc_perm *ipcp, u32 *secid) > +static inline void security_ipc_getsecid(struct kern_ipc_perm *ipcp, > + struct lsmblob *blob) > { > - *secid = 0; > + lsmblob_init(blob, 0); > } > > static inline int security_msg_msg_alloc(struct msg_msg *msg) > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index d31914088a82..148733ec3c72 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -2268,11 +2268,14 @@ 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, &context->ipc.osid); > + security_ipc_getsecid(ipcp, &blob); > + /* scaffolding on the [0] - change "osid" to a lsmblob */ > + context->ipc.osid = blob.secid[0]; > context->type = AUDIT_IPC; > } > > diff --git a/security/security.c b/security/security.c > index 5ab07631df75..d55f01041f05 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -1812,10 +1812,13 @@ int security_ipc_permission(struct kern_ipc_perm *ipcp, short flag) > return call_int_hook(ipc_permission, 0, ipcp, flag); > } > > -void security_ipc_getsecid(struct kern_ipc_perm *ipcp, u32 *secid) > +void security_ipc_getsecid(struct kern_ipc_perm *ipcp, struct lsmblob *blob) > { > - *secid = 0; > - call_void_hook(ipc_getsecid, ipcp, secid); > + struct security_hook_list *hp; > + > + lsmblob_init(blob, 0); > + hlist_for_each_entry(hp, &security_hook_heads.ipc_getsecid, list) > + hp->hook.ipc_getsecid(ipcp, &blob->secid[hp->slot]); > } > > int security_msg_msg_alloc(struct msg_msg *msg) >
On Mon, Jun 24, 2019 at 09:39:05AM -0700, Casey Schaufler wrote: > On 6/22/2019 3:48 PM, Kees Cook wrote: > > On Fri, Jun 21, 2019 at 11:52:19AM -0700, Casey Schaufler wrote: > >> + struct security_hook_list *hp; > >> + > >> + lsmblob_init(blob, 0); > >> + hlist_for_each_entry(hp, &security_hook_heads.ipc_getsecid, list) > >> + hp->hook.ipc_getsecid(ipcp, &blob->secid[hp->slot]); > > Just for sanity when using hp->slot, it might be good to do something > > like this in the places it gets used. Like for here: > > > > if (!WARN_ON(hp->slot < 0 || hp->slot >= LSMBLOB_COUNT)) > > hp->hook.ipc_getsecid(ipcp, &blob->secid[hp->slot]); > > > > This _should_ be overkill, but since lists of hooks that trigger slot > > assignment is hardcoded, it seems nice to cover any future problems or > > mismatches. > > How about a CONFIG_LSM_SLOT_CHECK around a function lsm_slot_check()? > If configured, it does the WARN_ON, and if not it's a static inline > true return. As you say, it's probably overkill, but it would be available > for the paranoid/debug/bringup situation. No, this doesn't need another CONFIG. The test is nearly free and WARN means it's wrapped in an unlikely already. I think just adding it outright would be fine.
diff --git a/include/linux/security.h b/include/linux/security.h index c6cddeff8a17..0d5e172341fc 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -413,7 +413,7 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3, unsigned long arg4, unsigned long arg5); void security_task_to_inode(struct task_struct *p, struct inode *inode); int security_ipc_permission(struct kern_ipc_perm *ipcp, short flag); -void security_ipc_getsecid(struct kern_ipc_perm *ipcp, u32 *secid); +void security_ipc_getsecid(struct kern_ipc_perm *ipcp, struct lsmblob *blob); int security_msg_msg_alloc(struct msg_msg *msg); void security_msg_msg_free(struct msg_msg *msg); int security_msg_queue_alloc(struct kern_ipc_perm *msq); @@ -1098,9 +1098,10 @@ static inline int security_ipc_permission(struct kern_ipc_perm *ipcp, return 0; } -static inline void security_ipc_getsecid(struct kern_ipc_perm *ipcp, u32 *secid) +static inline void security_ipc_getsecid(struct kern_ipc_perm *ipcp, + struct lsmblob *blob) { - *secid = 0; + lsmblob_init(blob, 0); } static inline int security_msg_msg_alloc(struct msg_msg *msg) diff --git a/kernel/auditsc.c b/kernel/auditsc.c index d31914088a82..148733ec3c72 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -2268,11 +2268,14 @@ 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, &context->ipc.osid); + security_ipc_getsecid(ipcp, &blob); + /* scaffolding on the [0] - change "osid" to a lsmblob */ + context->ipc.osid = blob.secid[0]; context->type = AUDIT_IPC; } diff --git a/security/security.c b/security/security.c index 5ab07631df75..d55f01041f05 100644 --- a/security/security.c +++ b/security/security.c @@ -1812,10 +1812,13 @@ int security_ipc_permission(struct kern_ipc_perm *ipcp, short flag) return call_int_hook(ipc_permission, 0, ipcp, flag); } -void security_ipc_getsecid(struct kern_ipc_perm *ipcp, u32 *secid) +void security_ipc_getsecid(struct kern_ipc_perm *ipcp, struct lsmblob *blob) { - *secid = 0; - call_void_hook(ipc_getsecid, ipcp, secid); + struct security_hook_list *hp; + + lsmblob_init(blob, 0); + hlist_for_each_entry(hp, &security_hook_heads.ipc_getsecid, list) + hp->hook.ipc_getsecid(ipcp, &blob->secid[hp->slot]); } int security_msg_msg_alloc(struct msg_msg *msg)
There may be more than one LSM that provides IPC data for auditing. Change security_ipc_getsecid() to fill in a lsmblob structure instead of the u32 secid. The audit data structure containing the secid will be updated later, so there is a bit of scaffolding here. Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> --- include/linux/security.h | 7 ++++--- kernel/auditsc.c | 5 ++++- security/security.c | 9 ++++++--- 3 files changed, 14 insertions(+), 7 deletions(-)