| Message ID | 20190618230551.7475-19-casey@schaufler-ca.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | LSM: Module stacking for AppArmor | expand |
On Tue, Jun 18, 2019 at 04:05:44PM -0700, Casey Schaufler wrote: > Chance the security_dentry_init_security() interface to > fill an lsmcontext structure instead of a void * data area > and a length. The lone caller of this interface is NFS4, > which may make copies of the data using its own mechanisms. > A rework of the nfs4 code to use the lsmcontext properly > is a significant project, so the coward's way out is taken, > and the lsmcontext data from security_dentry_init_security() > is copied, then released directly. > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > --- > fs/nfs/nfs4proc.c | 26 ++++++++++++++++---------- > include/linux/security.h | 7 +++---- > security/security.c | 20 ++++++++++++++++---- > 3 files changed, 35 insertions(+), 18 deletions(-) > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index af1c0db29c39..952f805965bb 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -113,6 +113,7 @@ static inline struct nfs4_label * > nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > struct iattr *sattr, struct nfs4_label *label) > { > + struct lsmcontext context; > int err; > > if (label == NULL) > @@ -122,21 +123,26 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > return NULL; > > err = security_dentry_init_security(dentry, sattr->ia_mode, > - &dentry->d_name, (void **)&label->label, &label->len); > - if (err == 0) > - return label; > + &dentry->d_name, &context); > + > + if (err) > + return NULL; > + > + label->label = kmemdup(context.context, context.len, GFP_KERNEL); I think this is wrong: for NUL-terminated strings, "context.len" isn't currently including the NUL byte (it's set to strlen()). So, if kmemdup() is used here, it means strlen() isn't correct in the context init helper, it should be using the "size" argument, etc. > + if (label->label == NULL) > + label = NULL; > + else > + label->len = context.len; > + > + security_release_secctx(&context); > + > + return label; > > - return NULL; > } > static inline void > nfs4_label_release_security(struct nfs4_label *label) > { > - struct lsmcontext scaff; /* scaffolding */ > - > - if (label) { > - lsmcontext_init(&scaff, label->label, label->len, 0); > - security_release_secctx(&scaff); > - } > + kfree(label->label); Should label be set to NULL here and len reduced to 0? > } > static inline u32 *nfs4_bitmask(struct nfs_server *server, struct nfs4_label *label) > { > diff --git a/include/linux/security.h b/include/linux/security.h > index 1fd87e80656f..92c4960dd57f 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -346,8 +346,8 @@ int security_sb_clone_mnt_opts(const struct super_block *oldsb, > int security_add_mnt_opt(const char *option, const char *val, > int len, void **mnt_opts); > int security_dentry_init_security(struct dentry *dentry, int mode, > - const struct qstr *name, void **ctx, > - u32 *ctxlen); > + const struct qstr *name, > + struct lsmcontext *ctx); > int security_dentry_create_files_as(struct dentry *dentry, int mode, > struct qstr *name, > const struct cred *old, > @@ -718,8 +718,7 @@ static inline void security_inode_free(struct inode *inode) > static inline int security_dentry_init_security(struct dentry *dentry, > int mode, > const struct qstr *name, > - void **ctx, > - u32 *ctxlen) > + struct lsmcontext *ctx) > { > return -EOPNOTSUPP; > } > diff --git a/security/security.c b/security/security.c > index 2ea810fc4a45..23d8049ec0c1 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -453,6 +453,8 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count, > * secid in the lsmblob structure. > */ > if (hooks[i].head == &security_hook_heads.audit_rule_match || > + hooks[i].head == > + &security_hook_heads.dentry_init_security || > hooks[i].head == &security_hook_heads.kernel_act_as || > hooks[i].head == > &security_hook_heads.socket_getpeersec_dgram || > @@ -1030,11 +1032,21 @@ void security_inode_free(struct inode *inode) > } > > int security_dentry_init_security(struct dentry *dentry, int mode, > - const struct qstr *name, void **ctx, > - u32 *ctxlen) > + const struct qstr *name, > + struct lsmcontext *cp) > { > - return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode, > - name, ctx, ctxlen); > + int *display = current->security; > + struct security_hook_list *hp; > + > + hlist_for_each_entry(hp, &security_hook_heads.dentry_init_security, > + list) > + if (*display == 0 || *display == hp->slot) { > + cp->slot = hp->slot; > + return hp->hook.dentry_init_security(dentry, mode, > + name, (void **)&cp->context, &cp->len); > + } > + > + return -EOPNOTSUPP; > } > EXPORT_SYMBOL(security_dentry_init_security); > > -- > 2.20.1 >
On 6/18/2019 10:41 PM, Kees Cook wrote: > On Tue, Jun 18, 2019 at 04:05:44PM -0700, Casey Schaufler wrote: >> Chance the security_dentry_init_security() interface to Note to self: s/Chance/Change/ >> fill an lsmcontext structure instead of a void * data area >> and a length. The lone caller of this interface is NFS4, >> which may make copies of the data using its own mechanisms. >> A rework of the nfs4 code to use the lsmcontext properly >> is a significant project, so the coward's way out is taken, >> and the lsmcontext data from security_dentry_init_security() >> is copied, then released directly. >> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >> --- >> fs/nfs/nfs4proc.c | 26 ++++++++++++++++---------- >> include/linux/security.h | 7 +++---- >> security/security.c | 20 ++++++++++++++++---- >> 3 files changed, 35 insertions(+), 18 deletions(-) >> >> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c >> index af1c0db29c39..952f805965bb 100644 >> --- a/fs/nfs/nfs4proc.c >> +++ b/fs/nfs/nfs4proc.c >> @@ -113,6 +113,7 @@ static inline struct nfs4_label * >> nfs4_label_init_security(struct inode *dir, struct dentry *dentry, >> struct iattr *sattr, struct nfs4_label *label) >> { >> + struct lsmcontext context; >> int err; >> >> if (label == NULL) >> @@ -122,21 +123,26 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, >> return NULL; >> >> err = security_dentry_init_security(dentry, sattr->ia_mode, >> - &dentry->d_name, (void **)&label->label, &label->len); >> - if (err == 0) >> - return label; >> + &dentry->d_name, &context); >> + >> + if (err) >> + return NULL; >> + >> + label->label = kmemdup(context.context, context.len, GFP_KERNEL); > I think this is wrong: for NUL-terminated strings, "context.len" isn't > currently including the NUL byte (it's set to strlen()). > > So, if kmemdup() is used here, it means strlen() isn't correct in the > context init helper, it should be using the "size" argument, etc. Would all be true if the context where being set by lsmcontext_init, but it's not. It's coming from the dentry_init_security hook, and the one instance of that, selinux_dentry_init_security() sets the size to strlen() + 1. The kmemdup() will include the terminating NUL. I too wish that the hooks and their use where more consistent. My sincere hope is that this revision of the infrastructure will help that to some extent. >> + if (label->label == NULL) >> + label = NULL; >> + else >> + label->len = context.len; >> + >> + security_release_secctx(&context); >> + >> + return label; >> >> - return NULL; >> } >> static inline void >> nfs4_label_release_security(struct nfs4_label *label) >> { >> - struct lsmcontext scaff; /* scaffolding */ >> - >> - if (label) { >> - lsmcontext_init(&scaff, label->label, label->len, 0); >> - security_release_secctx(&scaff); >> - } >> + kfree(label->label); > Should label be set to NULL here and len reduced to 0? It wasn't before, and I'd hate to make too many assumptions about what might be fragile in the NFS code. >> } >> static inline u32 *nfs4_bitmask(struct nfs_server *server, struct nfs4_label *label) >> { >> diff --git a/include/linux/security.h b/include/linux/security.h >> index 1fd87e80656f..92c4960dd57f 100644 >> --- a/include/linux/security.h >> +++ b/include/linux/security.h >> @@ -346,8 +346,8 @@ int security_sb_clone_mnt_opts(const struct super_block *oldsb, >> int security_add_mnt_opt(const char *option, const char *val, >> int len, void **mnt_opts); >> int security_dentry_init_security(struct dentry *dentry, int mode, >> - const struct qstr *name, void **ctx, >> - u32 *ctxlen); >> + const struct qstr *name, >> + struct lsmcontext *ctx); >> int security_dentry_create_files_as(struct dentry *dentry, int mode, >> struct qstr *name, >> const struct cred *old, >> @@ -718,8 +718,7 @@ static inline void security_inode_free(struct inode *inode) >> static inline int security_dentry_init_security(struct dentry *dentry, >> int mode, >> const struct qstr *name, >> - void **ctx, >> - u32 *ctxlen) >> + struct lsmcontext *ctx) >> { >> return -EOPNOTSUPP; >> } >> diff --git a/security/security.c b/security/security.c >> index 2ea810fc4a45..23d8049ec0c1 100644 >> --- a/security/security.c >> +++ b/security/security.c >> @@ -453,6 +453,8 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count, >> * secid in the lsmblob structure. >> */ >> if (hooks[i].head == &security_hook_heads.audit_rule_match || >> + hooks[i].head == >> + &security_hook_heads.dentry_init_security || >> hooks[i].head == &security_hook_heads.kernel_act_as || >> hooks[i].head == >> &security_hook_heads.socket_getpeersec_dgram || >> @@ -1030,11 +1032,21 @@ void security_inode_free(struct inode *inode) >> } >> >> int security_dentry_init_security(struct dentry *dentry, int mode, >> - const struct qstr *name, void **ctx, >> - u32 *ctxlen) >> + const struct qstr *name, >> + struct lsmcontext *cp) >> { >> - return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode, >> - name, ctx, ctxlen); >> + int *display = current->security; >> + struct security_hook_list *hp; >> + >> + hlist_for_each_entry(hp, &security_hook_heads.dentry_init_security, >> + list) >> + if (*display == 0 || *display == hp->slot) { >> + cp->slot = hp->slot; >> + return hp->hook.dentry_init_security(dentry, mode, >> + name, (void **)&cp->context, &cp->len); >> + } >> + >> + return -EOPNOTSUPP; >> } >> EXPORT_SYMBOL(security_dentry_init_security); >> >> -- >> 2.20.1 >>
On Wed, Jun 19, 2019 at 10:31:45AM -0700, Casey Schaufler wrote: > On 6/18/2019 10:41 PM, Kees Cook wrote: > > I think this is wrong: for NUL-terminated strings, "context.len" isn't > > currently including the NUL byte (it's set to strlen()). > > > > So, if kmemdup() is used here, it means strlen() isn't correct in the > > context init helper, it should be using the "size" argument, etc. > > Would all be true if the context where being set by lsmcontext_init, > but it's not. It's coming from the dentry_init_security hook, and > the one instance of that, selinux_dentry_init_security() sets the > size to strlen() + 1. The kmemdup() will include the terminating NUL. Ah-ha! Okay, thanks, yes. I see now. Carry on! :) > I too wish that the hooks and their use where more consistent. > My sincere hope is that this revision of the infrastructure will > help that to some extent. Once these changes land it should be much much easier to find ways to refactor for greater sanity. :) > > Should label be set to NULL here and len reduced to 0? > > It wasn't before, and I'd hate to make too many assumptions > about what might be fragile in the NFS code. Gotcha.
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index af1c0db29c39..952f805965bb 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -113,6 +113,7 @@ static inline struct nfs4_label * nfs4_label_init_security(struct inode *dir, struct dentry *dentry, struct iattr *sattr, struct nfs4_label *label) { + struct lsmcontext context; int err; if (label == NULL) @@ -122,21 +123,26 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, return NULL; err = security_dentry_init_security(dentry, sattr->ia_mode, - &dentry->d_name, (void **)&label->label, &label->len); - if (err == 0) - return label; + &dentry->d_name, &context); + + if (err) + return NULL; + + label->label = kmemdup(context.context, context.len, GFP_KERNEL); + if (label->label == NULL) + label = NULL; + else + label->len = context.len; + + security_release_secctx(&context); + + return label; - return NULL; } static inline void nfs4_label_release_security(struct nfs4_label *label) { - struct lsmcontext scaff; /* scaffolding */ - - if (label) { - lsmcontext_init(&scaff, label->label, label->len, 0); - security_release_secctx(&scaff); - } + kfree(label->label); } static inline u32 *nfs4_bitmask(struct nfs_server *server, struct nfs4_label *label) { diff --git a/include/linux/security.h b/include/linux/security.h index 1fd87e80656f..92c4960dd57f 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -346,8 +346,8 @@ int security_sb_clone_mnt_opts(const struct super_block *oldsb, int security_add_mnt_opt(const char *option, const char *val, int len, void **mnt_opts); int security_dentry_init_security(struct dentry *dentry, int mode, - const struct qstr *name, void **ctx, - u32 *ctxlen); + const struct qstr *name, + struct lsmcontext *ctx); int security_dentry_create_files_as(struct dentry *dentry, int mode, struct qstr *name, const struct cred *old, @@ -718,8 +718,7 @@ static inline void security_inode_free(struct inode *inode) static inline int security_dentry_init_security(struct dentry *dentry, int mode, const struct qstr *name, - void **ctx, - u32 *ctxlen) + struct lsmcontext *ctx) { return -EOPNOTSUPP; } diff --git a/security/security.c b/security/security.c index 2ea810fc4a45..23d8049ec0c1 100644 --- a/security/security.c +++ b/security/security.c @@ -453,6 +453,8 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count, * secid in the lsmblob structure. */ if (hooks[i].head == &security_hook_heads.audit_rule_match || + hooks[i].head == + &security_hook_heads.dentry_init_security || hooks[i].head == &security_hook_heads.kernel_act_as || hooks[i].head == &security_hook_heads.socket_getpeersec_dgram || @@ -1030,11 +1032,21 @@ void security_inode_free(struct inode *inode) } int security_dentry_init_security(struct dentry *dentry, int mode, - const struct qstr *name, void **ctx, - u32 *ctxlen) + const struct qstr *name, + struct lsmcontext *cp) { - return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode, - name, ctx, ctxlen); + int *display = current->security; + struct security_hook_list *hp; + + hlist_for_each_entry(hp, &security_hook_heads.dentry_init_security, + list) + if (*display == 0 || *display == hp->slot) { + cp->slot = hp->slot; + return hp->hook.dentry_init_security(dentry, mode, + name, (void **)&cp->context, &cp->len); + } + + return -EOPNOTSUPP; } EXPORT_SYMBOL(security_dentry_init_security);
Chance the security_dentry_init_security() interface to fill an lsmcontext structure instead of a void * data area and a length. The lone caller of this interface is NFS4, which may make copies of the data using its own mechanisms. A rework of the nfs4 code to use the lsmcontext properly is a significant project, so the coward's way out is taken, and the lsmcontext data from security_dentry_init_security() is copied, then released directly. Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> --- fs/nfs/nfs4proc.c | 26 ++++++++++++++++---------- include/linux/security.h | 7 +++---- security/security.c | 20 ++++++++++++++++---- 3 files changed, 35 insertions(+), 18 deletions(-)