| Message ID | 20200124002306.3552-23-casey@schaufler-ca.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | LSM: Module stacking for AppArmor | expand |
On 1/23/20 7:23 PM, 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. > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > Cc: linux-api@vger.kernel.org As previously discussed, there are issues with AppArmor's implementation of getprocattr() particularly around the trailing newline that dbus-daemon and perhaps others would like to see go away in any new interface. Hence, I don't think we should implement this new API using the existing getprocattr() hook lest it also be locked into the current behavior forever. > --- > Documentation/security/lsm.rst | 14 ++++++++ > fs/proc/base.c | 1 + > security/security.c | 63 ++++++++++++++++++++++++++++++++++ > 3 files changed, 78 insertions(+) > > diff --git a/Documentation/security/lsm.rst b/Documentation/security/lsm.rst > index aadf47c808c0..a4979060f5d3 100644 > --- a/Documentation/security/lsm.rst > +++ b/Documentation/security/lsm.rst > @@ -199,3 +199,17 @@ capability-related fields: > - ``fs/nfsd/auth.c``::c:func:`nfsd_setuser()` > > - ``fs/proc/array.c``::c:func:`task_cap()` > + > +LSM External Interfaces > +======================= > + > +The LSM infrastructure does not generally provide external interfaces. > +The individual security modules provide what external interfaces they > +require. The infrastructure does provide an interface for the special > +case where multiple security modules provide a process context. This > +is provided in compound context format. > + > +- `lsm1\0value\0lsm2\0value\0` > + > +The special file ``/proc/pid/attr/context`` provides the security > +context of the identified process. > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 950c200cb9ad..d13c2cf50e4b 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -2653,6 +2653,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 6a77c8b2ffbc..fdd0c85df89e 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -722,6 +722,42 @@ static void __init lsm_early_task(struct task_struct *task) > panic("%s: Early task alloc failed.\n", __func__); > } > > +/** > + * append_ctx - append a lsm/context pair to a compound context > + * @ctx: the existing compound context > + * @ctxlen: size of the old context, including terminating nul byte > + * @lsm: new lsm name, nul terminated > + * @new: new context, possibly nul terminated > + * @newlen: maximum size of @new > + * > + * replace @ctx with a new compound context, appending @newlsm and @new > + * to @ctx. On exit the new data replaces the old, which is freed. > + * @ctxlen is set to the new size, which includes a trailing nul byte. > + * > + * Returns 0 on success, -ENOMEM if no memory is available. > + */ > +static int append_ctx(char **ctx, int *ctxlen, const char *lsm, char *new, > + int newlen) > +{ > + char *final; > + int llen; > + > + llen = strlen(lsm) + 1; > + newlen = strnlen(new, newlen) + 1; > + > + final = kzalloc(*ctxlen + llen + newlen, GFP_KERNEL); > + if (final == NULL) > + return -ENOMEM; > + if (*ctxlen) > + memcpy(final, *ctx, *ctxlen); > + memcpy(final + *ctxlen, lsm, llen); > + memcpy(final + *ctxlen + llen, new, newlen); > + kfree(*ctx); > + *ctx = final; > + *ctxlen = *ctxlen + llen + newlen; > + return 0; > +} > + > /* > * Hook list operation macros. > * > @@ -2041,6 +2077,10 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name, > char **value) > { > struct security_hook_list *hp; > + char *final = NULL; > + char *cp; > + int rc = 0; > + int finallen = 0; > int display = lsm_task_display(current); > int slot = 0; > > @@ -2068,6 +2108,29 @@ 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; > + } > + rc = append_ctx(&final, &finallen, hp->lsmid->lsm, > + cp, rc); > + if (rc < 0) { > + kfree(final); > + return rc; > + } > + } > + 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; >
On 1/24/20 9:42 AM, Stephen Smalley wrote: > On 1/23/20 7:23 PM, 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. >> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >> Cc: linux-api@vger.kernel.org > > As previously discussed, there are issues with AppArmor's implementation > of getprocattr() particularly around the trailing newline that > dbus-daemon and perhaps others would like to see go away in any new > interface. Hence, I don't think we should implement this new API using > the existing getprocattr() hook lest it also be locked into the current > behavior forever. Also, it would be good if whatever hook is introduced to support /proc/pid/attr/context could also be leveraged by the SO_PEERCONTEXT implementation in the future so that we are guaranteed a consistent result between the two interfaces, unlike the current situation for /proc/self/attr/current versus SO_PEERSEC. > >> --- >> Documentation/security/lsm.rst | 14 ++++++++ >> fs/proc/base.c | 1 + >> security/security.c | 63 ++++++++++++++++++++++++++++++++++ >> 3 files changed, 78 insertions(+) >> >> diff --git a/Documentation/security/lsm.rst >> b/Documentation/security/lsm.rst >> index aadf47c808c0..a4979060f5d3 100644 >> --- a/Documentation/security/lsm.rst >> +++ b/Documentation/security/lsm.rst >> @@ -199,3 +199,17 @@ capability-related fields: >> - ``fs/nfsd/auth.c``::c:func:`nfsd_setuser()` >> - ``fs/proc/array.c``::c:func:`task_cap()` >> + >> +LSM External Interfaces >> +======================= >> + >> +The LSM infrastructure does not generally provide external interfaces. >> +The individual security modules provide what external interfaces they >> +require. The infrastructure does provide an interface for the special >> +case where multiple security modules provide a process context. This >> +is provided in compound context format. >> + >> +- `lsm1\0value\0lsm2\0value\0` >> + >> +The special file ``/proc/pid/attr/context`` provides the security >> +context of the identified process. >> diff --git a/fs/proc/base.c b/fs/proc/base.c >> index 950c200cb9ad..d13c2cf50e4b 100644 >> --- a/fs/proc/base.c >> +++ b/fs/proc/base.c >> @@ -2653,6 +2653,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 6a77c8b2ffbc..fdd0c85df89e 100644 >> --- a/security/security.c >> +++ b/security/security.c >> @@ -722,6 +722,42 @@ static void __init lsm_early_task(struct >> task_struct *task) >> panic("%s: Early task alloc failed.\n", __func__); >> } >> +/** >> + * append_ctx - append a lsm/context pair to a compound context >> + * @ctx: the existing compound context >> + * @ctxlen: size of the old context, including terminating nul byte >> + * @lsm: new lsm name, nul terminated >> + * @new: new context, possibly nul terminated >> + * @newlen: maximum size of @new >> + * >> + * replace @ctx with a new compound context, appending @newlsm and @new >> + * to @ctx. On exit the new data replaces the old, which is freed. >> + * @ctxlen is set to the new size, which includes a trailing nul byte. >> + * >> + * Returns 0 on success, -ENOMEM if no memory is available. >> + */ >> +static int append_ctx(char **ctx, int *ctxlen, const char *lsm, char >> *new, >> + int newlen) >> +{ >> + char *final; >> + int llen; >> + >> + llen = strlen(lsm) + 1; >> + newlen = strnlen(new, newlen) + 1; >> + >> + final = kzalloc(*ctxlen + llen + newlen, GFP_KERNEL); >> + if (final == NULL) >> + return -ENOMEM; >> + if (*ctxlen) >> + memcpy(final, *ctx, *ctxlen); >> + memcpy(final + *ctxlen, lsm, llen); >> + memcpy(final + *ctxlen + llen, new, newlen); >> + kfree(*ctx); >> + *ctx = final; >> + *ctxlen = *ctxlen + llen + newlen; >> + return 0; >> +} >> + >> /* >> * Hook list operation macros. >> * >> @@ -2041,6 +2077,10 @@ int security_getprocattr(struct task_struct *p, >> const char *lsm, char *name, >> char **value) >> { >> struct security_hook_list *hp; >> + char *final = NULL; >> + char *cp; >> + int rc = 0; >> + int finallen = 0; >> int display = lsm_task_display(current); >> int slot = 0; >> @@ -2068,6 +2108,29 @@ 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; >> + } >> + rc = append_ctx(&final, &finallen, hp->lsmid->lsm, >> + cp, rc); >> + if (rc < 0) { >> + kfree(final); >> + return rc; >> + } >> + } >> + 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; >> >
On 1/24/2020 8:20 AM, Stephen Smalley wrote: > On 1/24/20 9:42 AM, Stephen Smalley wrote: >> On 1/23/20 7:23 PM, 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. >>> >>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >>> Cc: linux-api@vger.kernel.org >> >> As previously discussed, there are issues with AppArmor's implementation of getprocattr() particularly around the trailing newline that dbus-daemon and perhaps others would like to see go away in any new interface. Hence, I don't think we should implement this new API using the existing getprocattr() hook lest it also be locked into the current behavior forever. > > Also, it would be good if whatever hook is introduced to support /proc/pid/attr/context could also be leveraged by the SO_PEERCONTEXT implementation in the future so that we are guaranteed a consistent result between the two interfaces, unlike the current situation for /proc/self/attr/current versus SO_PEERSEC. I don't believe that a new hook is necessary, and that introducing one just to deal with a '\n' would be pedantic. We really have two rational options. AppArmor could drop the '\n' from their "context". Or, we can simply document that the /proc/pid/attr/context interface will trim any trailing whitespace. I understand that this would be a break from the notion that the LSM infrastructure does not constrain what a module uses for its own data. On the other hand, we have been saying that "context"s are strings, and ignoring trailing whitespace is usual behavior for strings. > >> >>> --- >>> Documentation/security/lsm.rst | 14 ++++++++ >>> fs/proc/base.c | 1 + >>> security/security.c | 63 ++++++++++++++++++++++++++++++++++ >>> 3 files changed, 78 insertions(+) >>> >>> diff --git a/Documentation/security/lsm.rst b/Documentation/security/lsm.rst >>> index aadf47c808c0..a4979060f5d3 100644 >>> --- a/Documentation/security/lsm.rst >>> +++ b/Documentation/security/lsm.rst >>> @@ -199,3 +199,17 @@ capability-related fields: >>> - ``fs/nfsd/auth.c``::c:func:`nfsd_setuser()` >>> - ``fs/proc/array.c``::c:func:`task_cap()` >>> + >>> +LSM External Interfaces >>> +======================= >>> + >>> +The LSM infrastructure does not generally provide external interfaces. >>> +The individual security modules provide what external interfaces they >>> +require. The infrastructure does provide an interface for the special >>> +case where multiple security modules provide a process context. This >>> +is provided in compound context format. >>> + >>> +- `lsm1\0value\0lsm2\0value\0` >>> + >>> +The special file ``/proc/pid/attr/context`` provides the security >>> +context of the identified process. >>> diff --git a/fs/proc/base.c b/fs/proc/base.c >>> index 950c200cb9ad..d13c2cf50e4b 100644 >>> --- a/fs/proc/base.c >>> +++ b/fs/proc/base.c >>> @@ -2653,6 +2653,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 6a77c8b2ffbc..fdd0c85df89e 100644 >>> --- a/security/security.c >>> +++ b/security/security.c >>> @@ -722,6 +722,42 @@ static void __init lsm_early_task(struct task_struct *task) >>> panic("%s: Early task alloc failed.\n", __func__); >>> } >>> +/** >>> + * append_ctx - append a lsm/context pair to a compound context >>> + * @ctx: the existing compound context >>> + * @ctxlen: size of the old context, including terminating nul byte >>> + * @lsm: new lsm name, nul terminated >>> + * @new: new context, possibly nul terminated >>> + * @newlen: maximum size of @new >>> + * >>> + * replace @ctx with a new compound context, appending @newlsm and @new >>> + * to @ctx. On exit the new data replaces the old, which is freed. >>> + * @ctxlen is set to the new size, which includes a trailing nul byte. >>> + * >>> + * Returns 0 on success, -ENOMEM if no memory is available. >>> + */ >>> +static int append_ctx(char **ctx, int *ctxlen, const char *lsm, char *new, >>> + int newlen) >>> +{ >>> + char *final; >>> + int llen; >>> + >>> + llen = strlen(lsm) + 1; >>> + newlen = strnlen(new, newlen) + 1; >>> + >>> + final = kzalloc(*ctxlen + llen + newlen, GFP_KERNEL); >>> + if (final == NULL) >>> + return -ENOMEM; >>> + if (*ctxlen) >>> + memcpy(final, *ctx, *ctxlen); >>> + memcpy(final + *ctxlen, lsm, llen); >>> + memcpy(final + *ctxlen + llen, new, newlen); >>> + kfree(*ctx); >>> + *ctx = final; >>> + *ctxlen = *ctxlen + llen + newlen; >>> + return 0; >>> +} >>> + >>> /* >>> * Hook list operation macros. >>> * >>> @@ -2041,6 +2077,10 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name, >>> char **value) >>> { >>> struct security_hook_list *hp; >>> + char *final = NULL; >>> + char *cp; >>> + int rc = 0; >>> + int finallen = 0; >>> int display = lsm_task_display(current); >>> int slot = 0; >>> @@ -2068,6 +2108,29 @@ 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; >>> + } >>> + rc = append_ctx(&final, &finallen, hp->lsmid->lsm, >>> + cp, rc); >>> + if (rc < 0) { >>> + kfree(final); >>> + return rc; >>> + } >>> + } >>> + 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; >>> >> >
On 1/24/20 2:28 PM, Casey Schaufler wrote: > On 1/24/2020 8:20 AM, Stephen Smalley wrote: >> On 1/24/20 9:42 AM, Stephen Smalley wrote: >>> On 1/23/20 7:23 PM, 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. >>>> >>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >>>> Cc: linux-api@vger.kernel.org >>> >>> As previously discussed, there are issues with AppArmor's implementation of getprocattr() particularly around the trailing newline that dbus-daemon and perhaps others would like to see go away in any new interface. Hence, I don't think we should implement this new API using the existing getprocattr() hook lest it also be locked into the current behavior forever. >> >> Also, it would be good if whatever hook is introduced to support /proc/pid/attr/context could also be leveraged by the SO_PEERCONTEXT implementation in the future so that we are guaranteed a consistent result between the two interfaces, unlike the current situation for /proc/self/attr/current versus SO_PEERSEC. > > I don't believe that a new hook is necessary, and that introducing one > just to deal with a '\n' would be pedantic. We really have two rational > options. AppArmor could drop the '\n' from their "context". Or, we can > simply document that the /proc/pid/attr/context interface will trim any > trailing whitespace. I understand that this would be a break from the > notion that the LSM infrastructure does not constrain what a module uses > for its own data. On the other hand, we have been saying that "context"s > are strings, and ignoring trailing whitespace is usual behavior for > strings. Well, you can either introduce a new common underlying hook for use by /proc/pid/attr/context and SO_PEERCONTEXT to produce the string that is to be returned to userspace (in order to guarantee consistency in format and allowing them to be directly compared, which I think is what the dbus maintainers wanted), or you can modify every security module to provide that guarantee in its existing getprocattr and getpeersec* hook functions (SELinux already provides this guarantee; Smack and AppArmor produce slightly different results with respect to \0 and/or \n), or you can have the framework trim the values it gets from the security modules before composing them. But you need to do one of those things before this interface gets merged upstream. Aside from the trailing newline and \0 issues, AppArmor also has a whitespace-separated (mode) field that may or may not be present in the contexts it presently returns, ala "/usr/sbin/cupsd (enforce)". Not sure what they want for the new interfaces.
On Fri, 24 Jan 2020 at 15:16:36 -0500, Stephen Smalley wrote: > Aside from the trailing newline and \0 issues, AppArmor also has a > whitespace-separated (mode) field that may or may not be present in the > contexts it presently returns, ala "/usr/sbin/cupsd (enforce)". My understanding from last time I worked with AppArmor is that this is genuinely part of the context, and whether it is present or absent does not vary according to the kernel API used to access contexts. AppArmor-specific higher-level APIs parse it into a label and an optional mode, but LSM-agnostic user-space APIs (like the one in dbus) pass the whole string through as-is. (In practice it seems to be present if and only if the context is something other than "unconfined", although I don't know offhand whether that's an API guarantee.) smcv
On 1/24/2020 12:16 PM, Stephen Smalley wrote: > On 1/24/20 2:28 PM, Casey Schaufler wrote: >> On 1/24/2020 8:20 AM, Stephen Smalley wrote: >>> On 1/24/20 9:42 AM, Stephen Smalley wrote: >>>> On 1/23/20 7:23 PM, 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. >>>>> >>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >>>>> Cc: linux-api@vger.kernel.org >>>> >>>> As previously discussed, there are issues with AppArmor's implementation of getprocattr() particularly around the trailing newline that dbus-daemon and perhaps others would like to see go away in any new interface. Hence, I don't think we should implement this new API using the existing getprocattr() hook lest it also be locked into the current behavior forever. >>> >>> Also, it would be good if whatever hook is introduced to support /proc/pid/attr/context could also be leveraged by the SO_PEERCONTEXT implementation in the future so that we are guaranteed a consistent result between the two interfaces, unlike the current situation for /proc/self/attr/current versus SO_PEERSEC. >> >> I don't believe that a new hook is necessary, and that introducing one >> just to deal with a '\n' would be pedantic. We really have two rational >> options. AppArmor could drop the '\n' from their "context". Or, we can >> simply document that the /proc/pid/attr/context interface will trim any >> trailing whitespace. I understand that this would be a break from the >> notion that the LSM infrastructure does not constrain what a module uses >> for its own data. On the other hand, we have been saying that "context"s >> are strings, and ignoring trailing whitespace is usual behavior for >> strings. > > Well, you can either introduce a new common underlying hook for use by /proc/pid/attr/context and SO_PEERCONTEXT to produce the string that is to be returned to userspace (in order to guarantee consistency in format and allowing them to be directly compared, which I think is what the dbus maintainers wanted), The getprocattr() hooks are already required to provide a nul terminated string. The behavior of /proc/self/attr/current with regard to trailing whitespace or the terminating nul is left to the security module. The behavior of /proc/self/attr/context is new, and as such it can be defined to be reasonable and consistent. > or you can modify every security module to provide that guarantee in its existing getprocattr and getpeersec* hook functions (SELinux already provides this guarantee; Smack and AppArmor produce slightly different results with respect to \0 and/or \n), or you can have the framework trim the values it gets from the security modules before composing them. Changing "every security module" turns out to be trivial, really. The security_getprocattr() interface doesn't change, the getprocattr hooks get a bool argument indicating if newlines are allowed, the AppArmor code changes to check the bool and security_getprocattr() sets to bool specially for the "context" case. The getpeersec() interface use for SO_PEERCONTEXT isn't any more difficult. The security modules are free to include a terminating nul or not in the existing use case, but a bool can tell them what the behavior must be when we get to that interface. It's not like there are all that many security modules to bring in line at this point. Establishing sane rules of behavior is overdue. With the current set of new modules sniffing at the door it really is time to tighten these up. > But you need to do one of those things before this interface gets merged upstream. > > Aside from the trailing newline and \0 issues, AppArmor also has a whitespace-separated (mode) field that may or may not be present in the contexts it presently returns, ala "/usr/sbin/cupsd (enforce)". Not sure what they want for the new interfaces. Using '\0' as a field terminator addresses that issue. That's a primary motivator.
On 1/24/2020 12:16 PM, Stephen Smalley wrote: > On 1/24/20 2:28 PM, Casey Schaufler wrote: >> On 1/24/2020 8:20 AM, Stephen Smalley wrote: >>> On 1/24/20 9:42 AM, Stephen Smalley wrote: >>>> On 1/23/20 7:23 PM, 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. >>>>> >>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >>>>> Cc: linux-api@vger.kernel.org >>>> >>>> As previously discussed, there are issues with AppArmor's implementation of getprocattr() particularly around the trailing newline that dbus-daemon and perhaps others would like to see go away in any new interface. Hence, I don't think we should implement this new API using the existing getprocattr() hook lest it also be locked into the current behavior forever. >>> >>> Also, it would be good if whatever hook is introduced to support /proc/pid/attr/context could also be leveraged by the SO_PEERCONTEXT implementation in the future so that we are guaranteed a consistent result between the two interfaces, unlike the current situation for /proc/self/attr/current versus SO_PEERSEC. >> >> I don't believe that a new hook is necessary, and that introducing one >> just to deal with a '\n' would be pedantic. We really have two rational >> options. AppArmor could drop the '\n' from their "context". Or, we can >> simply document that the /proc/pid/attr/context interface will trim any >> trailing whitespace. I understand that this would be a break from the >> notion that the LSM infrastructure does not constrain what a module uses >> for its own data. On the other hand, we have been saying that "context"s >> are strings, and ignoring trailing whitespace is usual behavior for >> strings. > > Well, you can either introduce a new common underlying hook for use by /proc/pid/attr/context and SO_PEERCONTEXT to produce the string that is to be returned to userspace (in order to guarantee consistency in format and allowing them to be directly compared, which I think is what the dbus maintainers wanted), or you can modify every security module to provide that guarantee in its existing getprocattr and getpeersec* hook functions (SELinux already provides this guarantee; Smack and AppArmor produce slightly different results with respect to \0 and/or \n), or you can have the framework trim the values it gets from the security modules before composing them. But you need to do one of those things before this interface gets merged upstream. > > Aside from the trailing newline and \0 issues, AppArmor also has a whitespace-separated (mode) field that may or may not be present in the contexts it presently returns, ala "/usr/sbin/cupsd (enforce)". Not sure what they want for the new interfaces. > From c4085435215653b7c4d07a35a9df308120441d79 Mon Sep 17 00:00:00 2001 From: Casey Schaufler <casey@schaufler-ca.com> Date: Fri, 31 Jan 2020 13:57:23 -0800 Subject: [PATCH v14] LSM: Move "context" format enforcement into security modules Document in lsm_hooks.h what is expected of a security module that supplies the "context" attribute. Add handling of the "context" attribute to SELinux, Smack and AppArmor security modules. The AppArmor implementation provides a different string for "context" than it does for other attributes to conform with the "context" format. Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> --- include/linux/lsm_hooks.h | 6 ++++++ security/apparmor/include/procattr.h | 2 +- security/apparmor/lsm.c | 8 ++++++-- security/apparmor/procattr.c | 11 +++++++---- security/security.c | 2 +- security/selinux/hooks.c | 2 +- security/smack/smack_lsm.c | 2 +- 7 files changed, 23 insertions(+), 10 deletions(-) diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 2bf82e1cf347..61977a33f2c3 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -1321,6 +1321,12 @@ * @pages contains the number of pages. * Return 0 if permission is granted. * + * @getprocattr: + * Provide the named process attribute for display in special files in + * the /proc/.../attr directory. Attribute naming and the data displayed + * is at the discretion of the security modules. The exception is the + * "context" attribute, which will contain the security context of the + * task as a nul terminated text string without trailing whitespace. * @ismaclabel: * Check if the extended attribute specified by @name * represents a MAC label. Returns 1 if name is a MAC diff --git a/security/apparmor/include/procattr.h b/security/apparmor/include/procattr.h index 31689437e0e1..03dbfdb2f2c0 100644 --- a/security/apparmor/include/procattr.h +++ b/security/apparmor/include/procattr.h @@ -11,7 +11,7 @@ #ifndef __AA_PROCATTR_H #define __AA_PROCATTR_H -int aa_getprocattr(struct aa_label *label, char **string); +int aa_getprocattr(struct aa_label *label, char **string, bool newline); int aa_setprocattr_changehat(char *args, size_t size, int flags); #endif /* __AA_PROCATTR_H */ diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 37d003568e82..07729c28275e 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -593,6 +593,7 @@ static int apparmor_getprocattr(struct task_struct *task, char *name, const struct cred *cred = get_task_cred(task); struct aa_task_ctx *ctx = task_ctx(current); struct aa_label *label = NULL; + bool newline = true; if (strcmp(name, "current") == 0) label = aa_get_newest_label(cred_label(cred)); @@ -600,11 +601,14 @@ static int apparmor_getprocattr(struct task_struct *task, char *name, label = aa_get_newest_label(ctx->previous); else if (strcmp(name, "exec") == 0 && ctx->onexec) label = aa_get_newest_label(ctx->onexec); - else + else if (strcmp(name, "context") == 0) { + label = aa_get_newest_label(cred_label(cred)); + newline = false; + } else error = -EINVAL; if (label) - error = aa_getprocattr(label, value); + error = aa_getprocattr(label, value, newline); aa_put_label(label); put_cred(cred); diff --git a/security/apparmor/procattr.c b/security/apparmor/procattr.c index c929bf4a3df1..1098bca3d0e4 100644 --- a/security/apparmor/procattr.c +++ b/security/apparmor/procattr.c @@ -20,6 +20,7 @@ * aa_getprocattr - Return the profile information for @profile * @profile: the profile to print profile info about (NOT NULL) * @string: Returns - string containing the profile info (NOT NULL) + * @newline: Should a newline be added to @string. * * Returns: length of @string on success else error on failure * @@ -30,7 +31,7 @@ * * Returns: size of string placed in @string else error code on failure */ -int aa_getprocattr(struct aa_label *label, char **string) +int aa_getprocattr(struct aa_label *label, char **string, bool newline) { struct aa_ns *ns = labels_ns(label); struct aa_ns *current_ns = aa_get_current_ns(); @@ -60,11 +61,13 @@ int aa_getprocattr(struct aa_label *label, char **string) return len; } - (*string)[len] = '\n'; - (*string)[len + 1] = 0; + if (newline) { + (*string)[len] = '\n'; + (*string)[++len] = 0; + } aa_put_ns(current_ns); - return len + 1; + return len; } /** diff --git a/security/security.c b/security/security.c index fdd0c85df89e..5a4d90256fd7 100644 --- a/security/security.c +++ b/security/security.c @@ -2111,7 +2111,7 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name, if (!strcmp(name, "context")) { hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) { - rc = hp->hook.getprocattr(p, "current", &cp); + rc = hp->hook.getprocattr(p, "context", &cp); if (rc == -EINVAL || rc == -ENOPROTOOPT) continue; if (rc < 0) { diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index cd4743331800..1f53a0c66a46 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -6281,7 +6281,7 @@ static int selinux_getprocattr(struct task_struct *p, goto bad; } - if (!strcmp(name, "current")) + if (!strcmp(name, "current") || !strcmp(name, "context")) sid = __tsec->sid; else if (!strcmp(name, "prev")) sid = __tsec->osid; diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 9ce67e03ac49..834b6e886e7b 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -3487,7 +3487,7 @@ static int smack_getprocattr(struct task_struct *p, char *name, char **value) char *cp; int slen; - if (strcmp(name, "current") != 0) + if (strcmp(name, "current") != 0 && strcmp(name, "context") != 0) return -EINVAL; cp = kstrdup(skp->smk_known, GFP_KERNEL);
On 1/31/20 5:10 PM, Casey Schaufler wrote: > From c4085435215653b7c4d07a35a9df308120441d79 Mon Sep 17 00:00:00 2001 > From: Casey Schaufler <casey@schaufler-ca.com> > Date: Fri, 31 Jan 2020 13:57:23 -0800 > Subject: [PATCH v14] LSM: Move "context" format enforcement into security > modules > > Document in lsm_hooks.h what is expected of a security module that > supplies the "context" attribute. Add handling of the "context" > attribute to SELinux, Smack and AppArmor security modules. The > AppArmor implementation provides a different string for "context" > than it does for other attributes to conform with the "context" > format. > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > --- > include/linux/lsm_hooks.h | 6 ++++++ > security/apparmor/include/procattr.h | 2 +- > security/apparmor/lsm.c | 8 ++++++-- > security/apparmor/procattr.c | 11 +++++++---- > security/security.c | 2 +- > security/selinux/hooks.c | 2 +- > security/smack/smack_lsm.c | 2 +- > 7 files changed, 23 insertions(+), 10 deletions(-) > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 2bf82e1cf347..61977a33f2c3 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -1321,6 +1321,12 @@ > * @pages contains the number of pages. > * Return 0 if permission is granted. > * > + * @getprocattr: > + * Provide the named process attribute for display in special files in > + * the /proc/.../attr directory. Attribute naming and the data displayed > + * is at the discretion of the security modules. The exception is the > + * "context" attribute, which will contain the security context of the > + * task as a nul terminated text string without trailing whitespace. I'd suggest something like the following instead: * @getprocattr * Get the value of process attribute @name for task @p into a buffer * allocated by the security module and returned via @value. The * caller will free the returned buffer via kfree. The set of * attribute names is fixed by proc but the format of @value is up * to the security module authors except for the "context" attribute, * whose value is required to be a NUL-terminated printable ASCII * string without trailing whitespace. * @p the task whose attribute is being fetched * @name the name of the process attribute being fetched * @value set to point to the buffer containing the attribute value * Return the length of @value including the NUL on success, or -errno on error. The printable ASCII bit is based on what the dbus maintainer requested in previous discussions. The question of whether the terminating NUL should be included in the returned length was otherwise left ambiguous and inconsistent in your patch among the different security modules; if you prefer not including it in the length returned by the security modules, you'll need to adjust SELinux at least to not do so for "context".
On 2/3/20 1:54 PM, Stephen Smalley wrote: > I'd suggest something like the following instead: > * @getprocattr > * Get the value of process attribute @name for task @p into a buffer > * allocated by the security module and returned via @value. The > * caller will free the returned buffer via kfree. The set of > * attribute names is fixed by proc but the format of @value is up > * to the security module authors except for the "context" attribute, > * whose value is required to be a NUL-terminated printable ASCII > * string without trailing whitespace. > * @p the task whose attribute is being fetched > * @name the name of the process attribute being fetched > * @value set to point to the buffer containing the attribute value > * Return the length of @value including the NUL on success, or -errno > on error. > > The printable ASCII bit is based on what the dbus maintainer requested > in previous discussions. The question of whether the terminating NUL > should be included in the returned length was otherwise left ambiguous > and inconsistent in your patch among the different security modules; if > you prefer not including it in the length returned by the security > modules, you'll need to adjust SELinux at least to not do so for "context". BTW, I think the above guarantees with the exception of no trailing whitespace and whether the NUL byte is included or excluded from length are in reality also required for "current" (and SO_PEERSEC) or existing userspace will break.
On 1/27/20 12:05 PM, Simon McVittie wrote: > On Fri, 24 Jan 2020 at 15:16:36 -0500, Stephen Smalley wrote: >> Aside from the trailing newline and \0 issues, AppArmor also has a >> whitespace-separated (mode) field that may or may not be present in the >> contexts it presently returns, ala "/usr/sbin/cupsd (enforce)". > > My understanding from last time I worked with AppArmor is that this > is genuinely part of the context, and whether it is present or absent > does not vary according to the kernel API used to access contexts. > AppArmor-specific higher-level APIs parse it into a label and an optional > mode, but LSM-agnostic user-space APIs (like the one in dbus) pass the > whole string through as-is. > > (In practice it seems to be present if and only if the context is > something other than "unconfined", although I don't know offhand whether > that's an API guarantee.) > Correct, currently it is always included unless the context is unconfined. There is no guarantee that I am aware of beyond that is what the code did in the past and so as to not break things we continue to do exactly that. The mode certainly does not need to be included in a newer interface.
On 1/24/20 11:28 AM, Casey Schaufler wrote: > On 1/24/2020 8:20 AM, Stephen Smalley wrote: >> On 1/24/20 9:42 AM, Stephen Smalley wrote: >>> On 1/23/20 7:23 PM, 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. >>>> >>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >>>> Cc: linux-api@vger.kernel.org >>> >>> As previously discussed, there are issues with AppArmor's implementation of getprocattr() particularly around the trailing newline that dbus-daemon and perhaps others would like to see go away in any new interface. Hence, I don't think we should implement this new API using the existing getprocattr() hook lest it also be locked into the current behavior forever. >> >> Also, it would be good if whatever hook is introduced to support /proc/pid/attr/context could also be leveraged by the SO_PEERCONTEXT implementation in the future so that we are guaranteed a consistent result between the two interfaces, unlike the current situation for /proc/self/attr/current versus SO_PEERSEC. > > I don't believe that a new hook is necessary, and that introducing one > just to deal with a '\n' would be pedantic. We really have two rational > options. AppArmor could drop the '\n' from their "context". Or, we can > simply document that the /proc/pid/attr/context interface will trim any > trailing whitespace. I understand that this would be a break from the > notion that the LSM infrastructure does not constrain what a module uses > for its own data. On the other hand, we have been saying that "context"s > are strings, and ignoring trailing whitespace is usual behavior for > strings. > > AppArmor can drop the trailing '\n' it is not required. I would say it could be dropped from /proc/pid/attr/current except there may be some third party code that requires it.
On 1/24/20 12:16 PM, Stephen Smalley wrote: > On 1/24/20 2:28 PM, Casey Schaufler wrote: >> On 1/24/2020 8:20 AM, Stephen Smalley wrote: >>> On 1/24/20 9:42 AM, Stephen Smalley wrote: >>>> On 1/23/20 7:23 PM, 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. >>>>> >>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >>>>> Cc: linux-api@vger.kernel.org >>>> >>>> As previously discussed, there are issues with AppArmor's implementation of getprocattr() particularly around the trailing newline that dbus-daemon and perhaps others would like to see go away in any new interface. Hence, I don't think we should implement this new API using the existing getprocattr() hook lest it also be locked into the current behavior forever. >>> >>> Also, it would be good if whatever hook is introduced to support /proc/pid/attr/context could also be leveraged by the SO_PEERCONTEXT implementation in the future so that we are guaranteed a consistent result between the two interfaces, unlike the current situation for /proc/self/attr/current versus SO_PEERSEC. >> >> I don't believe that a new hook is necessary, and that introducing one >> just to deal with a '\n' would be pedantic. We really have two rational >> options. AppArmor could drop the '\n' from their "context". Or, we can >> simply document that the /proc/pid/attr/context interface will trim any >> trailing whitespace. I understand that this would be a break from the >> notion that the LSM infrastructure does not constrain what a module uses >> for its own data. On the other hand, we have been saying that "context"s >> are strings, and ignoring trailing whitespace is usual behavior for >> strings. > > Well, you can either introduce a new common underlying hook for use by /proc/pid/attr/context and SO_PEERCONTEXT to produce the string that is to be returned to userspace (in order to guarantee consistency in format and allowing them to be directly compared, which I think is what the dbus maintainers wanted), or you can modify every security module to provide that guarantee in its existing getprocattr and getpeersec* hook functions (SELinux already provides this guarantee; Smack and AppArmor produce slightly different results with respect to \0 and/or \n), or you can have the framework trim the values it gets from the security modules before composing them. But you need to do one of those things before this interface gets merged upstream. > > Aside from the trailing newline and \0 issues, AppArmor also has a whitespace-separated (mode) field that may or may not be present in the contexts it presently returns, ala "/usr/sbin/cupsd (enforce)". Not sure what they want for the new interfaces. > It is not needed for the new interface. And if I could go back and remove it from the old interface I would.
On 2/3/2020 11:37 AM, Stephen Smalley wrote: > On 2/3/20 1:54 PM, Stephen Smalley wrote: >> I'd suggest something like the following instead: >> * @getprocattr >> * Get the value of process attribute @name for task @p into a buffer >> * allocated by the security module and returned via @value. The >> * caller will free the returned buffer via kfree. The set of >> * attribute names is fixed by proc but the format of @value is up >> * to the security module authors except for the "context" attribute, >> * whose value is required to be a NUL-terminated printable ASCII >> * string without trailing whitespace. >> * @p the task whose attribute is being fetched >> * @name the name of the process attribute being fetched >> * @value set to point to the buffer containing the attribute value >> * Return the length of @value including the NUL on success, or -errno on error. I'm fine with either description, so I'll adopt yours. >> >> The printable ASCII bit is based on what the dbus maintainer requested in previous discussions. The question of whether the terminating NUL should be included in the returned length was otherwise left ambiguous and inconsistent in your patch among the different security modules; if you prefer not including it in the length returned by the security modules, you'll need to adjust SELinux at least to not do so for "context". append_ctx() is supposed to take the possibility that the module specific context string may or may not include a trailing '\0' into account. Alas, I see a problem in the current version for the "no trailing '\0'" case. On the other hand, with your proposed description the trailing '\0' is required, so it's a matter of verifying that all modules providing a "context" provide one that includes a trailing '\0' and return strlen()+1. > > BTW, I think the above guarantees with the exception of no trailing whitespace and whether the NUL byte is included or excluded from length are in reality also required for "current" (and SO_PEERSEC) or existing userspace will break. The behavior of interfaces (e.g. "current", "exec") that are module defined is only of concern with respect to to "display" behavior. If a security module wants to provide a variable size binary blob in "current" I would object on principle, but policy as I understand it has long been that if the authors want to do that, it's their call. The "context" has a defined format, and it would be up to the authors to come up with a printable ASCII representation of the binary blob. If they care. They're not required to provide a "context".
On 2/3/2020 1:02 PM, John Johansen wrote: > On 1/24/20 12:16 PM, Stephen Smalley wrote: >> ... >> >> Aside from the trailing newline and \0 issues, AppArmor also has a whitespace-separated (mode) field that may or may not be present in the contexts it presently returns, ala "/usr/sbin/cupsd (enforce)". Not sure what they want for the new interfaces. >> > > It is not needed for the new interface. And if I could go back and remove it from the old interface I would. So, what would the "context" for this case be? "/usr/sbin/cupsd" or "enforce"?
On 2/3/20 1:43 PM, Casey Schaufler wrote: > On 2/3/2020 1:02 PM, John Johansen wrote: >> On 1/24/20 12:16 PM, Stephen Smalley wrote: >>> ... >>> >>> Aside from the trailing newline and \0 issues, AppArmor also has a whitespace-separated (mode) field that may or may not be present in the contexts it presently returns, ala "/usr/sbin/cupsd (enforce)". Not sure what they want for the new interfaces. >>> >> >> It is not needed for the new interface. And if I could go back and remove it from the old interface I would. > > So, what would the "context" for this case be? "/usr/sbin/cupsd" or "enforce"? > "/usr/sbin/cupsd" "enforce" is the profile mode which can be looked up separately using "/usr/sbin/cupsd" if it is really needed.
On 2/3/20 4:39 PM, Casey Schaufler wrote: > On 2/3/2020 11:37 AM, Stephen Smalley wrote: >> BTW, I think the above guarantees with the exception of no trailing whitespace and whether the NUL byte is included or excluded from length are in reality also required for "current" (and SO_PEERSEC) or existing userspace will break. > > The behavior of interfaces (e.g. "current", "exec") that are module defined > is only of concern with respect to to "display" behavior. If a security module > wants to provide a variable size binary blob in "current" I would object on > principle, but policy as I understand it has long been that if the authors want > to do that, it's their call. Doing so would break existing userspace (not just LSM-specific userspace), so I think it would be a deal breaker even for new security modules to move away from those guarantees for "current" at least. procps-ng (and I think procps before it) directly reads /proc/pid/attr/current and truncates at the first unprintable character. systemd's sd-bus reads /proc/pid/attr/current directly and treats \n, \r, or \0 byte as terminators and truncated on first occurrence. A variety of userspace code uses the value obtained from /proc/pid/attr/current and/or SO_PEERSEC as something that it can pass to printf-like functions using a %s specifier for inclusion in logs and audit messages. > The "context" has a defined format, and it would > be up to the authors to come up with a printable ASCII representation of the > binary blob. If they care. They're not required to provide a "context".
On 2/4/2020 5:37 AM, Stephen Smalley wrote: > On 2/3/20 4:39 PM, Casey Schaufler wrote: >> On 2/3/2020 11:37 AM, Stephen Smalley wrote: >>> BTW, I think the above guarantees with the exception of no trailing whitespace and whether the NUL byte is included or excluded from length are in reality also required for "current" (and SO_PEERSEC) or existing userspace will break. >> >> The behavior of interfaces (e.g. "current", "exec") that are module defined >> is only of concern with respect to to "display" behavior. If a security module >> wants to provide a variable size binary blob in "current" I would object on >> principle, but policy as I understand it has long been that if the authors want >> to do that, it's their call. > > Doing so would break existing userspace (not just LSM-specific userspace), so I think it would be a deal breaker even for new security modules to move away from those guarantees for "current" at least. procps-ng (and I think procps before it) directly reads /proc/pid/attr/current and truncates at the first unprintable character. An user-space that makes invalid assumptions about an interface can't implicitly define the behavior of that interface. You can't declare that "current" is defined to be a string just because a developer looked at how one security module uses it and coded the application accordingly. You can't declare that "current" will always be a SELinux context. That horse left the barn in 2007, and there are still people writing code assuming that is what they're getting. If the sub-system maintainer, James Morris, is willing to state that "current" must have a particular format that would be different. > systemd's sd-bus reads /proc/pid/attr/current directly and treats \n, \r, or \0 byte as terminators and truncated on first occurrence. A variety of userspace code uses the value obtained from /proc/pid/attr/current and/or SO_PEERSEC as something that it can pass to printf-like functions using a %s specifier for inclusion in logs and audit messages. Yup. And so far no security module has been foolish enough to export a binary blob in a /proc/.../attr interface. That doesn't mean that the interface is defined as a string. It's certainly the convention, but nowhere is it documented as a requirement. That's why I'm putting in the effort to define the format for "context" and SO_PEERCONTEXT. Interfaces at the LSM level need to be defined so as to allow the underlying security modules to provide the information they want in a way that is unambiguous and application non-hostile. The help I've gotten from you and the rest of the reviewers over the life of this effort has been extremely helpful, if not always easy to swallow. > >> The "context" has a defined format, and it would >> be up to the authors to come up with a printable ASCII representation of the >> binary blob. If they care. They're not required to provide a "context". > >
On Mon, 03 Feb 2020 at 13:54:45 -0500, Stephen Smalley wrote: > The printable ASCII bit is based on what the dbus maintainer requested in > previous discussions. I thought in previous discussions, we had come to the conclusion that I can't assume it's 7-bit ASCII. (If I *can* assume that for this new API, that's even better.) To be clear, when I say ASCII I mean a sequence of bytes != '\0' with their high bit unset (x & 0x7f == x) and the obvious mapping to/from Unicode (bytes '\1' to '\x7f' represent codepoints U+0001 to U+007F). Is that the same thing you mean? I thought the conclusion we had come to in previous conversations was that the LSM context is what GLib calls a "bytestring", the same as filenames and environment variables - an opaque sequence of bytes != '\0', with no further guarantees, and no specified encoding or mapping to/from Unicode (most likely some superset of ASCII like UTF-8 or Latin-1, but nobody knows which one, and they could equally well be some binary encoding with no Unicode meaning, as long as it avoids '\0'). If I can safely assume that a new kernel <-> user-space API is constrained to UTF-8 or a UTF-8 subset like ASCII, then I can provide more friendly APIs for user-space features built over it. If that isn't possible, the next best thing is a "bytestring" like filenames, environment variables, and most kernel <-> user-space strings in general. smcv
On 2/10/20 6:56 AM, Simon McVittie wrote: > On Mon, 03 Feb 2020 at 13:54:45 -0500, Stephen Smalley wrote: >> The printable ASCII bit is based on what the dbus maintainer requested in >> previous discussions. > > I thought in previous discussions, we had come to the conclusion that > I can't assume it's 7-bit ASCII. (If I *can* assume that for this new > API, that's even better.) > > To be clear, when I say ASCII I mean a sequence of bytes != '\0' with > their high bit unset (x & 0x7f == x) and the obvious mapping to/from > Unicode (bytes '\1' to '\x7f' represent codepoints U+0001 to U+007F). Is > that the same thing you mean? I mean the subset of 7-bit ASCII that satisfies isprint() using the "C" locale. That is already true for SELinux with the existing interfaces. I can't necessarily speak for the others. > I thought the conclusion we had come to in previous conversations was > that the LSM context is what GLib calls a "bytestring", the same as > filenames and environment variables - an opaque sequence of bytes != '\0', > with no further guarantees, and no specified encoding or mapping to/from > Unicode (most likely some superset of ASCII like UTF-8 or Latin-1, > but nobody knows which one, and they could equally well be some binary > encoding with no Unicode meaning, as long as it avoids '\0'). > > If I can safely assume that a new kernel <-> user-space API is constrained > to UTF-8 or a UTF-8 subset like ASCII, then I can provide more friendly > APIs for user-space features built over it. If that isn't possible, the > next best thing is a "bytestring" like filenames, environment variables, > and most kernel <-> user-space strings in general. > > smcv >
On 2/10/20 8:25 AM, Stephen Smalley wrote: > On 2/10/20 6:56 AM, Simon McVittie wrote: >> On Mon, 03 Feb 2020 at 13:54:45 -0500, Stephen Smalley wrote: >>> The printable ASCII bit is based on what the dbus maintainer >>> requested in >>> previous discussions. >> >> I thought in previous discussions, we had come to the conclusion that >> I can't assume it's 7-bit ASCII. (If I *can* assume that for this new >> API, that's even better.) >> >> To be clear, when I say ASCII I mean a sequence of bytes != '\0' with >> their high bit unset (x & 0x7f == x) and the obvious mapping to/from >> Unicode (bytes '\1' to '\x7f' represent codepoints U+0001 to U+007F). Is >> that the same thing you mean? > > I mean the subset of 7-bit ASCII that satisfies isprint() using the "C" > locale. That is already true for SELinux with the existing interfaces. > I can't necessarily speak for the others. Looks like Smack labels are similarly restricted, per Documentation/admin-guide/LSM/Smack.rst. So I guess the only one that is perhaps unclear is AppArmor, since its labels are typically derived from pathnames? Can an AppArmor label returned via its getprocattr() hook be any legal pathname? >> I thought the conclusion we had come to in previous conversations was >> that the LSM context is what GLib calls a "bytestring", the same as >> filenames and environment variables - an opaque sequence of bytes != >> '\0', >> with no further guarantees, and no specified encoding or mapping to/from >> Unicode (most likely some superset of ASCII like UTF-8 or Latin-1, >> but nobody knows which one, and they could equally well be some binary >> encoding with no Unicode meaning, as long as it avoids '\0'). >> >> If I can safely assume that a new kernel <-> user-space API is >> constrained >> to UTF-8 or a UTF-8 subset like ASCII, then I can provide more friendly >> APIs for user-space features built over it. If that isn't possible, the >> next best thing is a "bytestring" like filenames, environment variables, >> and most kernel <-> user-space strings in general. >> >> smcv >> >
On 2/10/2020 6:55 AM, Stephen Smalley wrote: > On 2/10/20 8:25 AM, Stephen Smalley wrote: >> On 2/10/20 6:56 AM, Simon McVittie wrote: >>> On Mon, 03 Feb 2020 at 13:54:45 -0500, Stephen Smalley wrote: >>>> The printable ASCII bit is based on what the dbus maintainer requested in >>>> previous discussions. >>> >>> I thought in previous discussions, we had come to the conclusion that >>> I can't assume it's 7-bit ASCII. (If I *can* assume that for this new >>> API, that's even better.) >>> >>> To be clear, when I say ASCII I mean a sequence of bytes != '\0' with >>> their high bit unset (x & 0x7f == x) and the obvious mapping to/from >>> Unicode (bytes '\1' to '\x7f' represent codepoints U+0001 to U+007F). Is >>> that the same thing you mean? >> >> I mean the subset of 7-bit ASCII that satisfies isprint() using the "C" locale. That is already true for SELinux with the existing interfaces. I can't necessarily speak for the others. > > Looks like Smack labels are similarly restricted, per Documentation/admin-guide/LSM/Smack.rst. So I guess the only one that is perhaps unclear is AppArmor, since its labels are typically derived from pathnames? Can an AppArmor label returned via its getprocattr() hook be any legal pathname? Because attr/context (and later, SO_PEERCONTEXT) are new interfaces there is no need to exactly duplicate what is in attr/current (later SO_PEERSEC). I already plan to omit the "mode" component of the AppArmor data in the AppArmor hook, as was discussed earlier. I would prefer ASCII, but if AppArmor needs bytestrings, that's what we'll have to do. > >>> I thought the conclusion we had come to in previous conversations was >>> that the LSM context is what GLib calls a "bytestring", the same as >>> filenames and environment variables - an opaque sequence of bytes != '\0', >>> with no further guarantees, and no specified encoding or mapping to/from >>> Unicode (most likely some superset of ASCII like UTF-8 or Latin-1, >>> but nobody knows which one, and they coould equally well be some binary >>> encoding with no Unicode meaning, as long as it avoids '\0'). >>> >>> If I can safely assume that a new kernel <-> user-space API is constrained >>> to UTF-8 or a UTF-8 subset like ASCII, then I can provide more friendly >>> APIs for user-space features built over it. If that isn't possible, the >>> next best thing is a "bytestring" like filenames, environment variables, >>> and most kernel <-> user-space strings in general. >>> >>> smcv >>> >> >
On 2/10/20 6:55 AM, Stephen Smalley wrote: > On 2/10/20 8:25 AM, Stephen Smalley wrote: >> On 2/10/20 6:56 AM, Simon McVittie wrote: >>> On Mon, 03 Feb 2020 at 13:54:45 -0500, Stephen Smalley wrote: >>>> The printable ASCII bit is based on what the dbus maintainer requested in >>>> previous discussions. >>> >>> I thought in previous discussions, we had come to the conclusion that >>> I can't assume it's 7-bit ASCII. (If I *can* assume that for this new >>> API, that's even better.) >>> >>> To be clear, when I say ASCII I mean a sequence of bytes != '\0' with >>> their high bit unset (x & 0x7f == x) and the obvious mapping to/from >>> Unicode (bytes '\1' to '\x7f' represent codepoints U+0001 to U+007F). Is >>> that the same thing you mean? >> >> I mean the subset of 7-bit ASCII that satisfies isprint() using the "C" locale. That is already true for SELinux with the existing interfaces. I can't necessarily speak for the others. > > Looks like Smack labels are similarly restricted, per Documentation/admin-guide/LSM/Smack.rst. So I guess the only one that is perhaps unclear is AppArmor, since its labels are typically derived from pathnames? Can an AppArmor label returned via its getprocattr() hook be any legal pathname? > yes sadly, as much as I would like to depracate/remove it the pathname based profile names are a byte string. AppArmor supports a more restricted profile name and I have been trying to get people to move to it for 12 years with only limited success. >>> I thought the conclusion we had come to in previous conversations was >>> that the LSM context is what GLib calls a "bytestring", the same as >>> filenames and environment variables - an opaque sequence of bytes != '\0', >>> with no further guarantees, and no specified encoding or mapping to/from >>> Unicode (most likely some superset of ASCII like UTF-8 or Latin-1, >>> but nobody knows which one, and they could equally well be some binary >>> encoding with no Unicode meaning, as long as it avoids '\0'). >>> >>> If I can safely assume that a new kernel <-> user-space API is constrained >>> to UTF-8 or a UTF-8 subset like ASCII, then I can provide more friendly >>> APIs for user-space features built over it. If that isn't possible, the >>> next best thing is a "bytestring" like filenames, environment variables, >>> and most kernel <-> user-space strings in general. >>> >>> smcv >>> >> >
On 2/10/20 10:32 AM, Casey Schaufler wrote: > On 2/10/2020 6:55 AM, Stephen Smalley wrote: >> On 2/10/20 8:25 AM, Stephen Smalley wrote: >>> On 2/10/20 6:56 AM, Simon McVittie wrote: >>>> On Mon, 03 Feb 2020 at 13:54:45 -0500, Stephen Smalley wrote: >>>>> The printable ASCII bit is based on what the dbus maintainer requested in >>>>> previous discussions. >>>> >>>> I thought in previous discussions, we had come to the conclusion that >>>> I can't assume it's 7-bit ASCII. (If I *can* assume that for this new >>>> API, that's even better.) >>>> >>>> To be clear, when I say ASCII I mean a sequence of bytes != '\0' with >>>> their high bit unset (x & 0x7f == x) and the obvious mapping to/from >>>> Unicode (bytes '\1' to '\x7f' represent codepoints U+0001 to U+007F). Is >>>> that the same thing you mean? >>> >>> I mean the subset of 7-bit ASCII that satisfies isprint() using the "C" locale. That is already true for SELinux with the existing interfaces. I can't necessarily speak for the others. >> >> Looks like Smack labels are similarly restricted, per Documentation/admin-guide/LSM/Smack.rst. So I guess the only one that is perhaps unclear is AppArmor, since its labels are typically derived from pathnames? Can an AppArmor label returned via its getprocattr() hook be any legal pathname? > > Because attr/context (and later, SO_PEERCONTEXT) are new interfaces > there is no need to exactly duplicate what is in attr/current (later > SO_PEERSEC). I already plan to omit the "mode" component of the > AppArmor data in the AppArmor hook, as was discussed earlier. I would > prefer ASCII, but if AppArmor needs bytestrings, that's what we'll > have to do. > sadly, to not break userspace its a byte string because that is what the path based profile names are. AppArmor does support a more limited non path based profile name but I can't guarantee that is what userspace is using in policy. >> >>>> I thought the conclusion we had come to in previous conversations was >>>> that the LSM context is what GLib calls a "bytestring", the same as >>>> filenames and environment variables - an opaque sequence of bytes != '\0', >>>> with no further guarantees, and no specified encoding or mapping to/from >>>> Unicode (most likely some superset of ASCII like UTF-8 or Latin-1, >>>> but nobody knows which one, and they coould equally well be some binary >>>> encoding with no Unicode meaning, as long as it avoids '\0'). >>>> >>>> If I can safely assume that a new kernel <-> user-space API is constrained >>>> to UTF-8 or a UTF-8 subset like ASCII, then I can provide more friendly >>>> APIs for user-space features built over it. If that isn't possible, the >>>> next best thing is a "bytestring" like filenames, environment variables, >>>> and most kernel <-> user-space strings in general. >>>> >>>> smcv >>>> >>> >> >
On 2/10/20 2:00 PM, John Johansen wrote: > On 2/10/20 10:32 AM, Casey Schaufler wrote: >> Because attr/context (and later, SO_PEERCONTEXT) are new interfaces >> there is no need to exactly duplicate what is in attr/current (later >> SO_PEERSEC). I already plan to omit the "mode" component of the >> AppArmor data in the AppArmor hook, as was discussed earlier. I would >> prefer ASCII, but if AppArmor needs bytestrings, that's what we'll >> have to do. >> > > sadly, to not break userspace its a byte string because that is what the > path based profile names are. AppArmor does support a more limited non > path based profile name but I can't guarantee that is what userspace is > using in policy. Ok, so /proc/pid/attr/context and SO_PEERCONTEXT have to be defined as returning bytestrings. So far we've talked about having AppArmor drop the trailing newline, be consistent with regard to whether the terminating NUL is included or excluded, and drop the mode field from what it returns for /proc/pid/attr/context and SO_PEERCONTEXT versus the current /proc/pid/attr/current and SO_PEERSEC. Is that correct? How do we envision a liblsm processing the value returned from /proc/pid/attr/context or SO_PEEERCONTEXT? It can certainly split it up per LSM. But can it then take the apparmor portion of the context and pass that to existing libapparmor interfaces without breakage? Or will the changes to the format described above create issues there?
On 2/11/20 7:59 AM, Stephen Smalley wrote: > On 2/10/20 2:00 PM, John Johansen wrote: >> On 2/10/20 10:32 AM, Casey Schaufler wrote: >>> Because attr/context (and later, SO_PEERCONTEXT) are new interfaces >>> there is no need to exactly duplicate what is in attr/current (later >>> SO_PEERSEC). I already plan to omit the "mode" component of the >>> AppArmor data in the AppArmor hook, as was discussed earlier. I would >>> prefer ASCII, but if AppArmor needs bytestrings, that's what we'll >>> have to do. >>> >> >> sadly, to not break userspace its a byte string because that is what the path based profile names are. AppArmor does support a more limited non path based profile name but I can't guarantee that is what userspace is using in policy. > > Ok, so /proc/pid/attr/context and SO_PEERCONTEXT have to be defined as returning bytestrings. > > So far we've talked about having AppArmor drop the trailing newline, be consistent with regard to whether the terminating NUL is included or excluded, and drop the mode field from what it returns for /proc/pid/attr/context and SO_PEERCONTEXT versus the current /proc/pid/attr/current and SO_PEERSEC. Is that correct? > > How do we envision a liblsm processing the value returned from /proc/pid/attr/context or SO_PEEERCONTEXT? It can certainly split it up per LSM. But can it then take the apparmor portion of the context and pass that to existing libapparmor interfaces without breakage? Or will the changes to the format described above create issues there? > > libapparmor can handle the changes. It does not require the mode string, currently anything unconfined does not include it, and we have already done experiments with dropping it in other cases. The trailing '\n' is handled conditionally so only if its there; this is well tested as the currently out of upstream af_unix socket mediation that is used by dbus does not include a trailing '\n' on the SO_PEERSEC.
On 2/11/2020 9:58 AM, John Johansen wrote: > On 2/11/20 7:59 AM, Stephen Smalley wrote: >> On 2/10/20 2:00 PM, John Johansen wrote: >>> On 2/10/20 10:32 AM, Casey Schaufler wrote: >>>> Because attr/context (and later, SO_PEERCONTEXT) are new interfaces >>>> there is no need to exactly duplicate what is in attr/current (later >>>> SO_PEERSEC). I already plan to omit the "mode" component of the >>>> AppArmor data in the AppArmor hook, as was discussed earlier. I would >>>> prefer ASCII, but if AppArmor needs bytestrings, that's what we'll >>>> have to do. >>>> >>> >>> sadly, to not break userspace its a byte string because that is what the path based profile names are. AppArmor does support a more limited non path based profile name but I can't guarantee that is what userspace is using in policy. >> >> Ok, so /proc/pid/attr/context and SO_PEERCONTEXT have to be defined as returning bytestrings. >> >> So far we've talked about having AppArmor drop the trailing newline, be consistent with regard to whether the terminating NUL is included or excluded, and drop the mode field from what it returns for /proc/pid/attr/context and SO_PEERCONTEXT versus the current /proc/pid/attr/current and SO_PEERSEC. Is that correct? >> >> How do we envision a liblsm processing the value returned from /proc/pid/attr/context or SO_PEEERCONTEXT? There hasn't been any serious thought put into liblsm. To date there hasn't been an LSM level interface to worry about except for /sys/kernel/security/lsm. My notions of what a liblsm ought provide would seem archaic to most of the people here. I can make proposals if there's a notion that liblsm makes sense. >> It can certainly split it up per LSM. But can it then take the apparmor portion of the context and pass that to existing libapparmor interfaces without breakage? Or will the changes to the format described above create issues there? >> >> > libapparmor can handle the changes. It does not require the mode string, currently anything unconfined does not include it, and we have already done experiments with dropping it in other cases. The trailing '\n' is handled conditionally so only if its there; this is well tested as the currently out of upstream af_unix socket mediation that is used by dbus does not include a trailing '\n' on the SO_PEERSEC. So it doesn't seem that there would be significant difficulties switching from "current" to "context". It won't be transparent, but we're providing "display" to address that.
On 2/11/20 10:35 AM, Casey Schaufler wrote: > On 2/11/2020 9:58 AM, John Johansen wrote: >> On 2/11/20 7:59 AM, Stephen Smalley wrote: >>> On 2/10/20 2:00 PM, John Johansen wrote: >>>> On 2/10/20 10:32 AM, Casey Schaufler wrote: >>>>> Because attr/context (and later, SO_PEERCONTEXT) are new interfaces >>>>> there is no need to exactly duplicate what is in attr/current (later >>>>> SO_PEERSEC). I already plan to omit the "mode" component of the >>>>> AppArmor data in the AppArmor hook, as was discussed earlier. I would >>>>> prefer ASCII, but if AppArmor needs bytestrings, that's what we'll >>>>> have to do. >>>>> >>>> >>>> sadly, to not break userspace its a byte string because that is what the path based profile names are. AppArmor does support a more limited non path based profile name but I can't guarantee that is what userspace is using in policy. >>> >>> Ok, so /proc/pid/attr/context and SO_PEERCONTEXT have to be defined as returning bytestrings. >>> >>> So far we've talked about having AppArmor drop the trailing newline, be consistent with regard to whether the terminating NUL is included or excluded, and drop the mode field from what it returns for /proc/pid/attr/context and SO_PEERCONTEXT versus the current /proc/pid/attr/current and SO_PEERSEC. Is that correct? >>> >>> How do we envision a liblsm processing the value returned from /proc/pid/attr/context or SO_PEEERCONTEXT? > > There hasn't been any serious thought put into liblsm. To date > there hasn't been an LSM level interface to worry about except > for /sys/kernel/security/lsm. My notions of what a liblsm ought > provide would seem archaic to most of the people here. I can make > proposals if there's a notion that liblsm makes sense. > > >>> It can certainly split it up per LSM. But can it then take the apparmor portion of the context and pass that to existing libapparmor interfaces without breakage? Or will the changes to the format described above create issues there? >>> >>> >> libapparmor can handle the changes. It does not require the mode string, currently anything unconfined does not include it, and we have already done experiments with dropping it in other cases. The trailing '\n' is handled conditionally so only if its there; this is well tested as the currently out of upstream af_unix socket mediation that is used by dbus does not include a trailing '\n' on the SO_PEERSEC. > > So it doesn't seem that there would be significant difficulties > switching from "current" to "context". It won't be transparent, > but we're providing "display" to address that. > > right
diff --git a/Documentation/security/lsm.rst b/Documentation/security/lsm.rst index aadf47c808c0..a4979060f5d3 100644 --- a/Documentation/security/lsm.rst +++ b/Documentation/security/lsm.rst @@ -199,3 +199,17 @@ capability-related fields: - ``fs/nfsd/auth.c``::c:func:`nfsd_setuser()` - ``fs/proc/array.c``::c:func:`task_cap()` + +LSM External Interfaces +======================= + +The LSM infrastructure does not generally provide external interfaces. +The individual security modules provide what external interfaces they +require. The infrastructure does provide an interface for the special +case where multiple security modules provide a process context. This +is provided in compound context format. + +- `lsm1\0value\0lsm2\0value\0` + +The special file ``/proc/pid/attr/context`` provides the security +context of the identified process. diff --git a/fs/proc/base.c b/fs/proc/base.c index 950c200cb9ad..d13c2cf50e4b 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -2653,6 +2653,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 6a77c8b2ffbc..fdd0c85df89e 100644 --- a/security/security.c +++ b/security/security.c @@ -722,6 +722,42 @@ static void __init lsm_early_task(struct task_struct *task) panic("%s: Early task alloc failed.\n", __func__); } +/** + * append_ctx - append a lsm/context pair to a compound context + * @ctx: the existing compound context + * @ctxlen: size of the old context, including terminating nul byte + * @lsm: new lsm name, nul terminated + * @new: new context, possibly nul terminated + * @newlen: maximum size of @new + * + * replace @ctx with a new compound context, appending @newlsm and @new + * to @ctx. On exit the new data replaces the old, which is freed. + * @ctxlen is set to the new size, which includes a trailing nul byte. + * + * Returns 0 on success, -ENOMEM if no memory is available. + */ +static int append_ctx(char **ctx, int *ctxlen, const char *lsm, char *new, + int newlen) +{ + char *final; + int llen; + + llen = strlen(lsm) + 1; + newlen = strnlen(new, newlen) + 1; + + final = kzalloc(*ctxlen + llen + newlen, GFP_KERNEL); + if (final == NULL) + return -ENOMEM; + if (*ctxlen) + memcpy(final, *ctx, *ctxlen); + memcpy(final + *ctxlen, lsm, llen); + memcpy(final + *ctxlen + llen, new, newlen); + kfree(*ctx); + *ctx = final; + *ctxlen = *ctxlen + llen + newlen; + return 0; +} + /* * Hook list operation macros. * @@ -2041,6 +2077,10 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name, char **value) { struct security_hook_list *hp; + char *final = NULL; + char *cp; + int rc = 0; + int finallen = 0; int display = lsm_task_display(current); int slot = 0; @@ -2068,6 +2108,29 @@ 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; + } + rc = append_ctx(&final, &finallen, hp->lsmid->lsm, + cp, rc); + if (rc < 0) { + kfree(final); + return rc; + } + } + 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> Cc: linux-api@vger.kernel.org --- Documentation/security/lsm.rst | 14 ++++++++ fs/proc/base.c | 1 + security/security.c | 63 ++++++++++++++++++++++++++++++++++ 3 files changed, 78 insertions(+)