| Message ID | 20190726233923.2570-27-casey@schaufler-ca.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | LSM: Module stacking for AppArmor | expand |
On Fri, Jul 26, 2019 at 04:39:22PM -0700, Casey Schaufler wrote: > Add an entry /proc/.../attr/context which displays the full > process security "context" in compound format:' > lsm1\0value\0lsm2\0value\0... > This entry is not writable. As this is a new API, would it make sense to make this a bit more human readable (i.e. newlines not %NUL)? (And if not, please justify the reasoning in the commit log). -Kees > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > --- > fs/proc/base.c | 1 + > security/security.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 46 insertions(+) > > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 7bf70e041315..79600df5f7a2 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -2619,6 +2619,7 @@ static const struct pid_entry attr_dir_stuff[] = { > ATTR(NULL, "keycreate", 0666), > ATTR(NULL, "sockcreate", 0666), > ATTR(NULL, "display", 0666), > + ATTR(NULL, "context", 0666), > #ifdef CONFIG_SECURITY_SMACK > DIR("smack", 0555, > proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops), > diff --git a/security/security.c b/security/security.c > index 5551c146c035..6a89a6b90cce 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -2057,6 +2057,14 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name, > char **value) > { > struct security_hook_list *hp; > + char *final = NULL; > + char *cp; > + char *tp; > + int rc = 0; > + int finallen = 0; > + int llen; > + int clen; > + int tlen; > int display = lsm_task_display(current); > int slot = 0; > > @@ -2074,6 +2082,43 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name, > return -ENOMEM; > } > > + if (!strcmp(name, "context")) { > + hlist_for_each_entry(hp, &security_hook_heads.getprocattr, > + list) { > + rc = hp->hook.getprocattr(p, "current", &cp); > + if (rc == -EINVAL || rc == -ENOPROTOOPT) > + continue; > + if (rc < 0) { > + kfree(final); > + return rc; > + } > + llen = strlen(hp->lsmid->lsm) + 1; > + clen = strlen(cp) + 1; > + tlen = llen + clen; > + if (final) > + tlen += finallen; > + tp = kzalloc(tlen, GFP_KERNEL); > + if (tp == NULL) { > + kfree(cp); > + kfree(final); > + return -ENOMEM; > + } > + if (final) > + memcpy(tp, final, finallen); > + memcpy(tp + finallen, hp->lsmid->lsm, llen); > + memcpy(tp + finallen + llen, cp, clen); > + kfree(cp); > + if (final) > + kfree(final); > + final = tp; > + finallen = tlen; > + } > + if (final == NULL) > + return -EINVAL; > + *value = final; > + return finallen; > + } > + > hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) { > if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm)) > continue; > -- > 2.20.1 >
On 7/29/2019 10:19 AM, Kees Cook wrote: > On Fri, Jul 26, 2019 at 04:39:22PM -0700, Casey Schaufler wrote: >> Add an entry /proc/.../attr/context which displays the full >> process security "context" in compound format:' >> lsm1\0value\0lsm2\0value\0... >> This entry is not writable. > As this is a new API, would it make sense to make this a bit more > human readable (i.e. newlines not %NUL)? With the far reaching discussion about what format would be acceptable in mind I went with Simon McVittie's suggestion. Also note that AppArmor includes newline in attr/current, and this way we can preserve the existing value. It's compatible with /proc/.../cmdline and easily keesized: cat /proc/self/attr/context | tr '\0' '\n' > (And if not, please justify the > reasoning in the commit log). Good idea. > > -Kees > >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >> --- >> fs/proc/base.c | 1 + >> security/security.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 46 insertions(+) >> >> diff --git a/fs/proc/base.c b/fs/proc/base.c >> index 7bf70e041315..79600df5f7a2 100644 >> --- a/fs/proc/base.c >> +++ b/fs/proc/base.c >> @@ -2619,6 +2619,7 @@ static const struct pid_entry attr_dir_stuff[] = { >> ATTR(NULL, "keycreate", 0666), >> ATTR(NULL, "sockcreate", 0666), >> ATTR(NULL, "display", 0666), >> + ATTR(NULL, "context", 0666), >> #ifdef CONFIG_SECURITY_SMACK >> DIR("smack", 0555, >> proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops), >> diff --git a/security/security.c b/security/security.c >> index 5551c146c035..6a89a6b90cce 100644 >> --- a/security/security.c >> +++ b/security/security.c >> @@ -2057,6 +2057,14 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name, >> char **value) >> { >> struct security_hook_list *hp; >> + char *final = NULL; >> + char *cp; >> + char *tp; >> + int rc = 0; >> + int finallen = 0; >> + int llen; >> + int clen; >> + int tlen; >> int display = lsm_task_display(current); >> int slot = 0; >> >> @@ -2074,6 +2082,43 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name, >> return -ENOMEM; >> } >> >> + if (!strcmp(name, "context")) { >> + hlist_for_each_entry(hp, &security_hook_heads.getprocattr, >> + list) { >> + rc = hp->hook.getprocattr(p, "current", &cp); >> + if (rc == -EINVAL || rc == -ENOPROTOOPT) >> + continue; >> + if (rc < 0) { >> + kfree(final); >> + return rc; >> + } >> + llen = strlen(hp->lsmid->lsm) + 1; >> + clen = strlen(cp) + 1; >> + tlen = llen + clen; >> + if (final) >> + tlen += finallen; >> + tp = kzalloc(tlen, GFP_KERNEL); >> + if (tp == NULL) { >> + kfree(cp); >> + kfree(final); >> + return -ENOMEM; >> + } >> + if (final) >> + memcpy(tp, final, finallen); >> + memcpy(tp + finallen, hp->lsmid->lsm, llen); >> + memcpy(tp + finallen + llen, cp, clen); >> + kfree(cp); >> + if (final) >> + kfree(final); >> + final = tp; >> + finallen = tlen; >> + } >> + if (final == NULL) >> + return -EINVAL; >> + *value = final; >> + return finallen; >> + } >> + >> hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) { >> if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm)) >> continue; >> -- >> 2.20.1 >>
On Mon, Jul 29, 2019 at 12:22:37PM -0700, Casey Schaufler wrote: > On 7/29/2019 10:19 AM, Kees Cook wrote: > > On Fri, Jul 26, 2019 at 04:39:22PM -0700, Casey Schaufler wrote: > >> Add an entry /proc/.../attr/context which displays the full > >> process security "context" in compound format:' > >> lsm1\0value\0lsm2\0value\0... > >> This entry is not writable. > > As this is a new API, would it make sense to make this a bit more > > human readable (i.e. newlines not %NUL)? > > With the far reaching discussion about what format would be > acceptable in mind I went with Simon McVittie's suggestion. > Also note that AppArmor includes newline in attr/current, > and this way we can preserve the existing value. > It's compatible with /proc/.../cmdline and easily keesized: > > cat /proc/self/attr/context | tr '\0' '\n' Okay, cool. I suspected it must be the result of so many bike sheds but I couldn't quite find those memories. > > (And if not, please justify the > > reasoning in the commit log). > > Good idea. Thanks! It'll help my poor brain. :)
diff --git a/fs/proc/base.c b/fs/proc/base.c index 7bf70e041315..79600df5f7a2 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -2619,6 +2619,7 @@ static const struct pid_entry attr_dir_stuff[] = { ATTR(NULL, "keycreate", 0666), ATTR(NULL, "sockcreate", 0666), ATTR(NULL, "display", 0666), + ATTR(NULL, "context", 0666), #ifdef CONFIG_SECURITY_SMACK DIR("smack", 0555, proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops), diff --git a/security/security.c b/security/security.c index 5551c146c035..6a89a6b90cce 100644 --- a/security/security.c +++ b/security/security.c @@ -2057,6 +2057,14 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name, char **value) { struct security_hook_list *hp; + char *final = NULL; + char *cp; + char *tp; + int rc = 0; + int finallen = 0; + int llen; + int clen; + int tlen; int display = lsm_task_display(current); int slot = 0; @@ -2074,6 +2082,43 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name, return -ENOMEM; } + if (!strcmp(name, "context")) { + hlist_for_each_entry(hp, &security_hook_heads.getprocattr, + list) { + rc = hp->hook.getprocattr(p, "current", &cp); + if (rc == -EINVAL || rc == -ENOPROTOOPT) + continue; + if (rc < 0) { + kfree(final); + return rc; + } + llen = strlen(hp->lsmid->lsm) + 1; + clen = strlen(cp) + 1; + tlen = llen + clen; + if (final) + tlen += finallen; + tp = kzalloc(tlen, GFP_KERNEL); + if (tp == NULL) { + kfree(cp); + kfree(final); + return -ENOMEM; + } + if (final) + memcpy(tp, final, finallen); + memcpy(tp + finallen, hp->lsmid->lsm, llen); + memcpy(tp + finallen + llen, cp, clen); + kfree(cp); + if (final) + kfree(final); + final = tp; + finallen = tlen; + } + if (final == NULL) + return -EINVAL; + *value = final; + return finallen; + } + hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) { if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm)) continue;
Add an entry /proc/.../attr/context which displays the full process security "context" in compound format:' lsm1\0value\0lsm2\0value\0... This entry is not writable. Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> --- fs/proc/base.c | 1 + security/security.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+)