| Message ID | 20191216223621.5127-24-casey@schaufler-ca.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v12,01/25] LSM: Infrastructure management of the sock security | expand |
On 12/16/19 5:36 PM, Casey Schaufler wrote: > The getsockopt SO_PEERSEC provides the LSM based security > information for a single module, but for reasons of backward > compatibility cannot include the information for multiple > modules. A new option SO_PEERCONTEXT is added to report the > security "context" of multiple modules using a "compound" format > > lsm1\0value\0lsm2\0value\0 > > This is expected to be used by system services, including dbus-daemon. > The exact format of a compound context has been the subject of > considerable debate. This format was suggested by Simon McVittie, > a dbus maintainer with a significant stake in the format being > usable. > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > cc: netdev@vger.kernel.org Requires ack by netdev and linux-api. A couple of comments below. > --- > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 2bf82e1cf347..2ae10e7f81a7 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -880,8 +880,8 @@ > * SO_GETPEERSEC. For tcp sockets this can be meaningful if the > * socket is associated with an ipsec SA. > * @sock is the local socket. > - * @optval userspace memory where the security state is to be copied. > - * @optlen userspace int where the module should copy the actual length > + * @optval memory where the security state is to be copied. This is misleading; it suggests that the caller is providing an allocated buffer into which the security module copies its data. Instead it is just a pointer to a pointer that is then set by the security module to a buffer the module allocates. > diff --git a/include/linux/security.h b/include/linux/security.h > index 536db4dbfcbb..b72bb90b1903 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -181,7 +181,7 @@ struct lsmblob { > #define LSMBLOB_NEEDED -2 /* Slot requested on initialization */ > #define LSMBLOB_NOT_NEEDED -3 /* Slot not requested */ > #define LSMBLOB_DISPLAY -4 /* Use the "display" slot */ > -#define LSMBLOB_FIRST -5 /* Use the default "display" slot */ > +#define LSMBLOB_COMPOUND -5 /* A compound "display" */ I'm puzzled by the removal of LSMBLOB_FIRST by this patch; it suggests it was never needed in the first place by the patch that introduced it. But more below. > diff --git a/security/security.c b/security/security.c > index d0b57a7c3b31..1afe245f3246 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -723,6 +723,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; > +} You should likely take some precautions against integer overflows in the above code? > + > /* > * Hook list operation macros. > * > @@ -2164,8 +2200,8 @@ int security_setprocattr(const char *lsm, const char *name, void *value, > hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) { > if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm)) > continue; > - if (lsm == NULL && *display != LSMBLOB_INVALID && > - *display != hp->lsmid->slot) > + if (lsm == NULL && display != NULL && > + *display != LSMBLOB_INVALID && *display != hp->lsmid->slot) > continue; > return hp->hook.setprocattr(name, value, size); > } Is this a bug fix that should be folded into the earlier patch that introduced it? > @@ -2196,7 +2232,7 @@ int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp, > */ > if (display == LSMBLOB_DISPLAY) > display = lsm_task_display(current); > - else if (display == LSMBLOB_FIRST) > + else if (display == 0) > display = LSMBLOB_INVALID; > else if (display < 0) { > WARN_ONCE(true, Why is it necessary to re-map display 0 in this manner? Previously if display 0 was specified, it would require it to match the lsmid->slot value. Won't it match anyway? > @@ -2246,6 +2282,15 @@ void security_release_secctx(struct lsmcontext *cp) > struct security_hook_list *hp; > bool found = false; > > + if (cp->slot == LSMBLOB_INVALID) > + return; > + > + if (cp->slot == LSMBLOB_COMPOUND) { > + kfree(cp->context); > + found = true; > + goto clear_out; > + } > + If you re-order your pr_warn() below with your memset() to address the earlier comment, you'll end up trying to print the freed memory. Not a problem if you just drop the pr_warn() altogether.
On 12/18/19 1:28 PM, Stephen Smalley wrote: > On 12/16/19 5:36 PM, Casey Schaufler wrote: >> The getsockopt SO_PEERSEC provides the LSM based security >> information for a single module, but for reasons of backward >> compatibility cannot include the information for multiple >> modules. A new option SO_PEERCONTEXT is added to report the >> security "context" of multiple modules using a "compound" format >> >> lsm1\0value\0lsm2\0value\0 >> >> This is expected to be used by system services, including dbus-daemon. >> The exact format of a compound context has been the subject of >> considerable debate. This format was suggested by Simon McVittie, >> a dbus maintainer with a significant stake in the format being >> usable. >> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >> cc: netdev@vger.kernel.org > > Requires ack by netdev and linux-api. A couple of comments below. Also, have you tested this new interface? I may be doing something wrong, but a trivial attempt to use SO_PEERCONTEXT with both SELinux and AppArmor enabled only appeared to return the SELinux portion of the label (selinux\0unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023\0), whereas /proc/self/attr/context returned a compound context (the same but with apparmor\0unconfined\n\0 appended). > >> --- > >> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >> index 2bf82e1cf347..2ae10e7f81a7 100644 >> --- a/include/linux/lsm_hooks.h >> +++ b/include/linux/lsm_hooks.h >> @@ -880,8 +880,8 @@ >> * SO_GETPEERSEC. For tcp sockets this can be meaningful if the >> * socket is associated with an ipsec SA. >> * @sock is the local socket. >> - * @optval userspace memory where the security state is to be copied. >> - * @optlen userspace int where the module should copy the actual >> length >> + * @optval memory where the security state is to be copied. > > This is misleading; it suggests that the caller is providing an > allocated buffer into which the security module copies its data. Instead > it is just a pointer to a pointer that is then set by the security > module to a buffer the module allocates. > >> diff --git a/include/linux/security.h b/include/linux/security.h >> index 536db4dbfcbb..b72bb90b1903 100644 >> --- a/include/linux/security.h >> +++ b/include/linux/security.h >> @@ -181,7 +181,7 @@ struct lsmblob { >> #define LSMBLOB_NEEDED -2 /* Slot requested on >> initialization */ >> #define LSMBLOB_NOT_NEEDED -3 /* Slot not requested */ >> #define LSMBLOB_DISPLAY -4 /* Use the "display" slot */ >> -#define LSMBLOB_FIRST -5 /* Use the default "display" slot */ >> +#define LSMBLOB_COMPOUND -5 /* A compound "display" */ > > I'm puzzled by the removal of LSMBLOB_FIRST by this patch; it suggests > it was never needed in the first place by the patch that introduced it. > But more below. > >> diff --git a/security/security.c b/security/security.c >> index d0b57a7c3b31..1afe245f3246 100644 >> --- a/security/security.c >> +++ b/security/security.c >> @@ -723,6 +723,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; >> +} > > You should likely take some precautions against integer overflows in the > above code? > >> + >> /* >> * Hook list operation macros. >> * >> @@ -2164,8 +2200,8 @@ int security_setprocattr(const char *lsm, const >> char *name, void *value, >> hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) { >> if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm)) >> continue; >> - if (lsm == NULL && *display != LSMBLOB_INVALID && >> - *display != hp->lsmid->slot) >> + if (lsm == NULL && display != NULL && >> + *display != LSMBLOB_INVALID && *display != hp->lsmid->slot) >> continue; >> return hp->hook.setprocattr(name, value, size); >> } > > Is this a bug fix that should be folded into the earlier patch that > introduced it? > >> @@ -2196,7 +2232,7 @@ int security_secid_to_secctx(struct lsmblob >> *blob, struct lsmcontext *cp, >> */ >> if (display == LSMBLOB_DISPLAY) >> display = lsm_task_display(current); >> - else if (display == LSMBLOB_FIRST) >> + else if (display == 0) >> display = LSMBLOB_INVALID; >> else if (display < 0) { >> WARN_ONCE(true, > > Why is it necessary to re-map display 0 in this manner? Previously if > display 0 was specified, it would require it to match the lsmid->slot > value. Won't it match anyway? > >> @@ -2246,6 +2282,15 @@ void security_release_secctx(struct lsmcontext >> *cp) >> struct security_hook_list *hp; >> bool found = false; >> + if (cp->slot == LSMBLOB_INVALID) >> + return; >> + >> + if (cp->slot == LSMBLOB_COMPOUND) { >> + kfree(cp->context); >> + found = true; >> + goto clear_out; >> + } >> + > > If you re-order your pr_warn() below with your memset() to address the > earlier comment, you'll end up trying to print the freed memory. Not a > problem if you just drop the pr_warn() altogether.
On 12/18/19 2:12 PM, Stephen Smalley wrote: > On 12/18/19 1:28 PM, Stephen Smalley wrote: >> On 12/16/19 5:36 PM, Casey Schaufler wrote: >>> The getsockopt SO_PEERSEC provides the LSM based security >>> information for a single module, but for reasons of backward >>> compatibility cannot include the information for multiple >>> modules. A new option SO_PEERCONTEXT is added to report the >>> security "context" of multiple modules using a "compound" format >>> >>> lsm1\0value\0lsm2\0value\0 >>> >>> This is expected to be used by system services, including dbus-daemon. >>> The exact format of a compound context has been the subject of >>> considerable debate. This format was suggested by Simon McVittie, >>> a dbus maintainer with a significant stake in the format being >>> usable. >>> >>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >>> cc: netdev@vger.kernel.org >> >> Requires ack by netdev and linux-api. A couple of comments below. > > Also, have you tested this new interface? I may be doing something > wrong, but a trivial attempt to use SO_PEERCONTEXT with both SELinux and > AppArmor enabled only appeared to return the SELinux portion of the > label > (selinux\0unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023\0), > whereas /proc/self/attr/context returned a compound context (the same > but with apparmor\0unconfined\n\0 appended). Ok, this seems to be a lack of support in AppArmor for saving the peer info for unix/local domain sockets, so not your bug. Doesn't implement the necessary hooks. > >> >>> --- >> >>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >>> index 2bf82e1cf347..2ae10e7f81a7 100644 >>> --- a/include/linux/lsm_hooks.h >>> +++ b/include/linux/lsm_hooks.h >>> @@ -880,8 +880,8 @@ >>> * SO_GETPEERSEC. For tcp sockets this can be meaningful if the >>> * socket is associated with an ipsec SA. >>> * @sock is the local socket. >>> - * @optval userspace memory where the security state is to be >>> copied. >>> - * @optlen userspace int where the module should copy the actual >>> length >>> + * @optval memory where the security state is to be copied. >> >> This is misleading; it suggests that the caller is providing an >> allocated buffer into which the security module copies its data. >> Instead it is just a pointer to a pointer that is then set by the >> security module to a buffer the module allocates. >> >>> diff --git a/include/linux/security.h b/include/linux/security.h >>> index 536db4dbfcbb..b72bb90b1903 100644 >>> --- a/include/linux/security.h >>> +++ b/include/linux/security.h >>> @@ -181,7 +181,7 @@ struct lsmblob { >>> #define LSMBLOB_NEEDED -2 /* Slot requested on >>> initialization */ >>> #define LSMBLOB_NOT_NEEDED -3 /* Slot not requested */ >>> #define LSMBLOB_DISPLAY -4 /* Use the "display" slot */ >>> -#define LSMBLOB_FIRST -5 /* Use the default "display" slot */ >>> +#define LSMBLOB_COMPOUND -5 /* A compound "display" */ >> >> I'm puzzled by the removal of LSMBLOB_FIRST by this patch; it suggests >> it was never needed in the first place by the patch that introduced >> it. But more below. >> >>> diff --git a/security/security.c b/security/security.c >>> index d0b57a7c3b31..1afe245f3246 100644 >>> --- a/security/security.c >>> +++ b/security/security.c >>> @@ -723,6 +723,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; >>> +} >> >> You should likely take some precautions against integer overflows in >> the above code? >> >>> + >>> /* >>> * Hook list operation macros. >>> * >>> @@ -2164,8 +2200,8 @@ int security_setprocattr(const char *lsm, const >>> char *name, void *value, >>> hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) { >>> if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm)) >>> continue; >>> - if (lsm == NULL && *display != LSMBLOB_INVALID && >>> - *display != hp->lsmid->slot) >>> + if (lsm == NULL && display != NULL && >>> + *display != LSMBLOB_INVALID && *display != hp->lsmid->slot) >>> continue; >>> return hp->hook.setprocattr(name, value, size); >>> } >> >> Is this a bug fix that should be folded into the earlier patch that >> introduced it? >> >>> @@ -2196,7 +2232,7 @@ int security_secid_to_secctx(struct lsmblob >>> *blob, struct lsmcontext *cp, >>> */ >>> if (display == LSMBLOB_DISPLAY) >>> display = lsm_task_display(current); >>> - else if (display == LSMBLOB_FIRST) >>> + else if (display == 0) >>> display = LSMBLOB_INVALID; >>> else if (display < 0) { >>> WARN_ONCE(true, >> >> Why is it necessary to re-map display 0 in this manner? Previously if >> display 0 was specified, it would require it to match the lsmid->slot >> value. Won't it match anyway? >> >>> @@ -2246,6 +2282,15 @@ void security_release_secctx(struct lsmcontext >>> *cp) >>> struct security_hook_list *hp; >>> bool found = false; >>> + if (cp->slot == LSMBLOB_INVALID) >>> + return; >>> + >>> + if (cp->slot == LSMBLOB_COMPOUND) { >>> + kfree(cp->context); >>> + found = true; >>> + goto clear_out; >>> + } >>> + >> >> If you re-order your pr_warn() below with your memset() to address the >> earlier comment, you'll end up trying to print the freed memory. Not >> a problem if you just drop the pr_warn() altogether. >
On Wed, 18 Dec 2019 at 15:50:45 -0500, Stephen Smalley wrote: > Ok, this seems to be a lack of support in AppArmor for saving the peer info > for unix/local domain sockets, so not your bug. Doesn't implement the > necessary hooks. Ubuntu kernels have working getsockopt SO_PEERSEC for AppArmor (which definitely works for D-Bus, or did when I tried it in the past), but it seems to be a downstream patch. Is there a combination of LSMs where this works correctly and shows multiple LSMs' labels, either upstream or with downstream patches? D-Bus ought to be an early consumer of this information, but that's going to be difficult if there's nowhere I can test it. If there was a pair of in-tree or out-of-tree test LSMs that automatically labelled process 12345 and all sockets that it created with something like (pseudocode) { "labeltest1": "labeltest1process12345", "labeltest2": "labeltest2process12345" } then that would make testing straightforward. From my user-space developer perspective, I'd really like the kernel to have at least brief documentation of the assumptions that can be made about the compound format, to avoid people like me having to ask the linux-security-module mailing list and use the answers as our documentation: - Can I assume that the LSM names (conceptually, keys in the associative array) are printable ASCII? If not, can I at least assume that they are printable UTF-8, or that LSMs whose names are not printable UTF-8 are too weird to be worth supporting in user-space APIs? If this decision hasn't been made yet, I would like to request ASCII, or UTF-8 if a wider character repertoire is desired - that would make user-space APIs a lot easier to write. - What can I assume about the format of the labels (values in the associative array)? From previous conversations on linux-security-module it seems the answer is: they are intended to be printable strings; their encoding is unspecified (could be ASCII or UTF-8, but equally could be Latin-1 or anything else); there are no reserved or disallowed characters except '\0'; individual LSMs like SELinux and AppArmor might impose tighter restrictions, but LSM-agnostic code can't assume those restrictions are present. Even if the format is conceptually an unspecified encoding with every nonzero byte allowed, if LSM and security policy authors establish a convention that the labels are ASCII or UTF-8 without control characters such as newlines, that would make user-space a lot easier to deal with. - Can I assume that the names and labels in /proc/$pid/attr/context correspond exactly (same label <=> same bytes) to the ones in SO_PEERCONTEXT? In particular, a design issue in the previous interfaces with /proc/$pid/attr/current and SO_PEERSEC was that AppArmor puts "unconfined\n" in /proc/$pid/attr/current, but "unconfined\0" in SO_PEERSEC, and implementations of protocols like D-Bus can't know that these are meant to be the same without LSM-specific knowledge (namely "AppArmor profile names can't contain newlines"). If this new API is an opportunity to declare that LSMs are expected to put the same canonical form of a label in /proc/$pid/attr/context and SO_PEERCONTEXT, possibly with a non-canonical version (adding '\n' or '\0' or similar) exposed in the older /proc/$pid/attr/current and SO_PEERSEC interfaces for backwards compatibility, then that would make life a lot easier for user-space developers like me. - Does the order of the names and values matter? Can I assume anything about them? It would make the D-Bus API more straightforward if I can assume that the order doesn't matter, and represent it as an unordered map (associative array), like a Perl hash or Python dict. I'm asking all this from the D-Bus perspective, but anything that wants to store or forward LSM contexts in an LSM-agnostic way will have essentially the same requirements - I could imagine X11, Wayland, varlink etc. hitting the same problems D-Bus did. Thanks, smcv
On 12/19/19 7:19 AM, Simon McVittie wrote: > On Wed, 18 Dec 2019 at 15:50:45 -0500, Stephen Smalley wrote: >> Ok, this seems to be a lack of support in AppArmor for saving the peer info >> for unix/local domain sockets, so not your bug. Doesn't implement the >> necessary hooks. > > Ubuntu kernels have working getsockopt SO_PEERSEC for AppArmor (which > definitely works for D-Bus, or did when I tried it in the past), but it > seems to be a downstream patch. > > Is there a combination of LSMs where this works correctly and shows > multiple LSMs' labels, either upstream or with downstream patches? D-Bus > ought to be an early consumer of this information, but that's going to > be difficult if there's nowhere I can test it. If there was a pair of > in-tree or out-of-tree test LSMs that automatically labelled process > 12345 and all sockets that it created with something like (pseudocode) > > { > "labeltest1": "labeltest1process12345", > "labeltest2": "labeltest2process12345" > } > > then that would make testing straightforward. AFAICT, you can't yet stack Smack+SELinux, and AppArmor requires out-of-tree patches to support SO_PEERSEC so there is no in-tree combination that will demonstrate the new SO_PEERCONTEXT for multiple lsms. > From my user-space developer perspective, I'd really like the kernel > to have at least brief documentation of the assumptions that can be > made about the compound format, to avoid people like me having to > ask the linux-security-module mailing list and use the answers as our > documentation: > > - Can I assume that the LSM names (conceptually, keys in the associative > array) are printable ASCII? If not, can I at least assume that they are > printable UTF-8, or that LSMs whose names are not printable UTF-8 are > too weird to be worth supporting in user-space APIs? > > If this decision hasn't been made yet, I would like to request ASCII, > or UTF-8 if a wider character repertoire is desired - that would make > user-space APIs a lot easier to write. ASCII works for me. > > - What can I assume about the format of the labels (values in the > associative array)? > > From previous conversations on linux-security-module it seems the > answer is: they are intended to be printable strings; their encoding > is unspecified (could be ASCII or UTF-8, but equally could be Latin-1 > or anything else); there are no reserved or disallowed characters > except '\0'; individual LSMs like SELinux and AppArmor might impose > tighter restrictions, but LSM-agnostic code can't assume those > restrictions are present. > > Even if the format is conceptually an unspecified encoding with every > nonzero byte allowed, if LSM and security policy authors establish a > convention that the labels are ASCII or UTF-8 without control characters > such as newlines, that would make user-space a lot easier to deal with. I believe there is existing userspace code that seems to presume that they are NUL-terminated strings printable using %s format specifiers to printf-like functions in output and log messages _without_ any escaping. Not just the LSM-specific libraries. systemd (for SO_PEERSEC), procps-ng (for /proc/pid/attr/current), perhaps others. I think we can rely on that remaining true since the world would break. > - Can I assume that the names and labels in /proc/$pid/attr/context > correspond exactly (same label <=> same bytes) to the ones in > SO_PEERCONTEXT? > > In particular, a design issue in the previous interfaces with > /proc/$pid/attr/current and SO_PEERSEC was that AppArmor puts > "unconfined\n" in /proc/$pid/attr/current, but "unconfined\0" in > SO_PEERSEC, and implementations of protocols like D-Bus can't know > that these are meant to be the same without LSM-specific knowledge > (namely "AppArmor profile names can't contain newlines"). > > If this new API is an opportunity to declare that LSMs are expected > to put the same canonical form of a label in /proc/$pid/attr/context and > SO_PEERCONTEXT, possibly with a non-canonical version (adding '\n' or > '\0' or similar) exposed in the older /proc/$pid/attr/current and > SO_PEERSEC interfaces for backwards compatibility, then that would make > life a lot easier for user-space developers like me. I'm all for this but the current implementation reuses the same underlying hooks as SO_PEERSEC, so it gets the same result for the per-lsm values. We'd need a separate hook if we cannot alter the current AppArmor SO_PEERSEC format. Technically AppArmor seemingly doesn't truly provide SO_PEERSEC in mainline today even though it implements the hook for returning the result because it never actually sets the peer on Unix domain or INET domain connections AFAICT, so they could change the format upstream without a compatibility break but it may break Ubuntu. So it might require using a new hook for SO_PEERCONTEXT. Which is fine with me. > - Does the order of the names and values matter? Can I assume anything > about them? > > It would make the D-Bus API more straightforward if I can assume that > the order doesn't matter, and represent it as an unordered map > (associative array), like a Perl hash or Python dict. IMHO order shouldn't matter. > I'm asking all this from the D-Bus perspective, but anything that > wants to store or forward LSM contexts in an LSM-agnostic way will > have essentially the same requirements - I could imagine X11, Wayland, > varlink etc. hitting the same problems D-Bus did. > > Thanks, > smcv >
On 12/19/19 8:47 AM, Stephen Smalley wrote: > On 12/19/19 7:19 AM, Simon McVittie wrote: >> On Wed, 18 Dec 2019 at 15:50:45 -0500, Stephen Smalley wrote: >>> Ok, this seems to be a lack of support in AppArmor for saving the >>> peer info >>> for unix/local domain sockets, so not your bug. Doesn't implement the >>> necessary hooks. >> >> Ubuntu kernels have working getsockopt SO_PEERSEC for AppArmor (which >> definitely works for D-Bus, or did when I tried it in the past), but it >> seems to be a downstream patch. >> >> Is there a combination of LSMs where this works correctly and shows >> multiple LSMs' labels, either upstream or with downstream patches? D-Bus >> ought to be an early consumer of this information, but that's going to >> be difficult if there's nowhere I can test it. If there was a pair of >> in-tree or out-of-tree test LSMs that automatically labelled process >> 12345 and all sockets that it created with something like (pseudocode) >> >> { >> "labeltest1": "labeltest1process12345", >> "labeltest2": "labeltest2process12345" >> } >> >> then that would make testing straightforward. > > AFAICT, you can't yet stack Smack+SELinux, and AppArmor requires > out-of-tree patches to support SO_PEERSEC so there is no in-tree > combination that will demonstrate the new SO_PEERCONTEXT for multiple lsms. > >> From my user-space developer perspective, I'd really like the kernel >> to have at least brief documentation of the assumptions that can be >> made about the compound format, to avoid people like me having to >> ask the linux-security-module mailing list and use the answers as our >> documentation: >> >> - Can I assume that the LSM names (conceptually, keys in the associative >> array) are printable ASCII? If not, can I at least assume that they >> are >> printable UTF-8, or that LSMs whose names are not printable UTF-8 are >> too weird to be worth supporting in user-space APIs? >> >> If this decision hasn't been made yet, I would like to request ASCII, >> or UTF-8 if a wider character repertoire is desired - that would make >> user-space APIs a lot easier to write. > > ASCII works for me. > >> >> - What can I assume about the format of the labels (values in the >> associative array)? >> >> From previous conversations on linux-security-module it seems the >> answer is: they are intended to be printable strings; their encoding >> is unspecified (could be ASCII or UTF-8, but equally could be Latin-1 >> or anything else); there are no reserved or disallowed characters >> except '\0'; individual LSMs like SELinux and AppArmor might impose >> tighter restrictions, but LSM-agnostic code can't assume those >> restrictions are present. >> >> Even if the format is conceptually an unspecified encoding with every >> nonzero byte allowed, if LSM and security policy authors establish a >> convention that the labels are ASCII or UTF-8 without control >> characters >> such as newlines, that would make user-space a lot easier to deal >> with. > > I believe there is existing userspace code that seems to presume that > they are NUL-terminated strings printable using %s format specifiers to > printf-like functions in output and log messages _without_ any escaping. > Not just the LSM-specific libraries. systemd (for SO_PEERSEC), > procps-ng (for /proc/pid/attr/current), perhaps others. I think we can > rely on that remaining true since the world would break. Looks like userspace is generally forgiving of whether the terminating NUL byte is included or omitted by the kernel (with different behaviors for SELinux - always included, Smack - omitted by /proc/pid/attr/current but included in SO_PEERSEC, and AppArmor - omitted for /proc/pid/attr/current but includes a terminating \n, omitted for SO_PEERSEC but no terminating \n), and procps-ng explicitly tests for printable characters (but truncates on the first unprintable character). If we want consistency for /proc/pid/attr/context and SO_PEERCONTEXT, we'd need different hooks for one or both of them; the current stacking patches just internally use the existing hooks with their current behaviors for current and SO_PEERSEC and then compose them. > >> - Can I assume that the names and labels in /proc/$pid/attr/context >> correspond exactly (same label <=> same bytes) to the ones in >> SO_PEERCONTEXT? >> >> In particular, a design issue in the previous interfaces with >> /proc/$pid/attr/current and SO_PEERSEC was that AppArmor puts >> "unconfined\n" in /proc/$pid/attr/current, but "unconfined\0" in >> SO_PEERSEC, and implementations of protocols like D-Bus can't know >> that these are meant to be the same without LSM-specific knowledge >> (namely "AppArmor profile names can't contain newlines"). >> >> If this new API is an opportunity to declare that LSMs are expected >> to put the same canonical form of a label in >> /proc/$pid/attr/context and >> SO_PEERCONTEXT, possibly with a non-canonical version (adding '\n' or >> '\0' or similar) exposed in the older /proc/$pid/attr/current and >> SO_PEERSEC interfaces for backwards compatibility, then that would >> make >> life a lot easier for user-space developers like me. > > I'm all for this but the current implementation reuses the same > underlying hooks as SO_PEERSEC, so it gets the same result for the > per-lsm values. We'd need a separate hook if we cannot alter the > current AppArmor SO_PEERSEC format. Technically AppArmor seemingly > doesn't truly provide SO_PEERSEC in mainline today even though it > implements the hook for returning the result because it never actually > sets the peer on Unix domain or INET domain connections AFAICT, so they > could change the format upstream without a compatibility break but it > may break Ubuntu. So it might require using a new hook for > SO_PEERCONTEXT. Which is fine with me. > >> - Does the order of the names and values matter? Can I assume anything >> about them? >> >> It would make the D-Bus API more straightforward if I can assume that >> the order doesn't matter, and represent it as an unordered map >> (associative array), like a Perl hash or Python dict. > > IMHO order shouldn't matter. > >> I'm asking all this from the D-Bus perspective, but anything that >> wants to store or forward LSM contexts in an LSM-agnostic way will >> have essentially the same requirements - I could imagine X11, Wayland, >> varlink etc. hitting the same problems D-Bus did. >> >> Thanks, >> smcv >> >
On Thu, 19 Dec 2019 at 10:00:31 -0500, Stephen Smalley wrote: > Looks like userspace is generally forgiving of whether the terminating NUL > byte is included or omitted by the kernel (with different behaviors for > SELinux - always included, Smack - omitted by /proc/pid/attr/current but > included in SO_PEERSEC, and AppArmor - omitted for /proc/pid/attr/current > but includes a terminating \n, omitted for SO_PEERSEC but no terminating > \n), and procps-ng explicitly tests for printable characters (but truncates > on the first unprintable character). Because LSM people have told me in the past that the '\0' is not conceptually part of the label, the D-Bus specification and reference implementation already assume that its presence or absence is irrelevant, and normalize to a canonical form (which happens to be that it appends a '\0' if missing, to be nice to C-like languages, but I could equally have chosen to strip the '\0' and rely on an out-of-band length count). By design, SO_PEERCONTEXT and /proc/pid/attr/context don't (can't!) preserve whether the label originally ended with '\0' or not (because they are designed to use '\0' as a terminator for each label), so these new kernel interfaces are already a bit closer than the old kernel interfaces to how D-Bus represents this information. The problematic case is AppArmor's terminating '\n' on /proc/pid/attr/current, because when I asked in the past, I was told that it would be (unwise but) valid to have a LSM where "foo" and "foo\n" are distinct labels. If that hypothetical LSM would make procps-ng lose information (because procps-ng truncates at the first unprintable character), does that change the situation any? Would that make it acceptable for other LSM-agnostic user-space components, like the reference implementation of D-Bus, to assume that stripping a trailing newline from /proc/pid/attr/context or from one of the component strings of /proc/pid/attr/current is a non-lossy operation? > > > If this new API is an opportunity to declare that LSMs are expected > > > to put the same canonical form of a label in > > > /proc/$pid/attr/context and > > > SO_PEERCONTEXT, possibly with a non-canonical version (adding '\n' or > > > '\0' or similar) exposed in the older /proc/$pid/attr/current and > > > SO_PEERSEC interfaces for backwards compatibility, then that > > > would make > > > life a lot easier for user-space developers like me. > > > > I'm all for this but the current implementation reuses the same > > underlying hooks as SO_PEERSEC, so it gets the same result for the > > per-lsm values. We'd need a separate hook if we cannot alter the > > current AppArmor SO_PEERSEC format. If AppArmor was going to change the format of one of its interfaces (or deviate from it when implementing new interfaces), I'd actually prefer it to be /proc/pid/attr/current that changed or was superseded, because /proc/pid/attr/current is the one that contains a newline that consumers are meant to ignore. For what it's worth, libapparmor explicitly removes the newline, so this only matters to LSM-agnostic readers like D-Bus implementations, and to lower-level AppArmor-aware readers that use the kernel interfaces directly in preference to using libapparmor. smcv
On 12/19/19 11:48 AM, Simon McVittie wrote: > On Thu, 19 Dec 2019 at 10:00:31 -0500, Stephen Smalley wrote: >> Looks like userspace is generally forgiving of whether the terminating NUL >> byte is included or omitted by the kernel (with different behaviors for >> SELinux - always included, Smack - omitted by /proc/pid/attr/current but >> included in SO_PEERSEC, and AppArmor - omitted for /proc/pid/attr/current >> but includes a terminating \n, omitted for SO_PEERSEC but no terminating >> \n), and procps-ng explicitly tests for printable characters (but truncates >> on the first unprintable character). > > Because LSM people have told me in the past that the '\0' is not > conceptually part of the label, the D-Bus specification and reference > implementation already assume that its presence or absence is irrelevant, > and normalize to a canonical form (which happens to be that it appends a > '\0' if missing, to be nice to C-like languages, but I could equally > have chosen to strip the '\0' and rely on an out-of-band length count). > > By design, SO_PEERCONTEXT and /proc/pid/attr/context don't (can't!) > preserve whether the label originally ended with '\0' or not (because > they are designed to use '\0' as a terminator for each label), so these > new kernel interfaces are already a bit closer than the old kernel > interfaces to how D-Bus represents this information. > > The problematic case is AppArmor's terminating '\n' on > /proc/pid/attr/current, because when I asked in the past, I was told > that it would be (unwise but) valid to have a LSM where "foo" and "foo\n" > are distinct labels. I don't agree with that stance, but obviously others may differ. > If that hypothetical LSM would make procps-ng lose information (because > procps-ng truncates at the first unprintable character), does that change > the situation any? Would that make it acceptable for other LSM-agnostic > user-space components, like the reference implementation of D-Bus, to > assume that stripping a trailing newline from /proc/pid/attr/context > or from one of the component strings of /proc/pid/attr/current is a > non-lossy operation? IMHO, yes. In fact, looking further, I see that systemd's src/libsystemd/sd-bus/bus-creds.c:bus_creds_add_more() reads /proc/pid/attr/current with its read_one_line_file() helper which ultimately uses read_line_full() and treats EOF, \n, \r, or \0 as terminators and truncates on first such occurrence. > >>>> If this new API is an opportunity to declare that LSMs are expected >>>> to put the same canonical form of a label in >>>> /proc/$pid/attr/context and >>>> SO_PEERCONTEXT, possibly with a non-canonical version (adding '\n' or >>>> '\0' or similar) exposed in the older /proc/$pid/attr/current and >>>> SO_PEERSEC interfaces for backwards compatibility, then that >>>> would make >>>> life a lot easier for user-space developers like me. >>> >>> I'm all for this but the current implementation reuses the same >>> underlying hooks as SO_PEERSEC, so it gets the same result for the >>> per-lsm values. We'd need a separate hook if we cannot alter the >>> current AppArmor SO_PEERSEC format. > > If AppArmor was going to change the format of one of its interfaces > (or deviate from it when implementing new interfaces), I'd actually > prefer it to be /proc/pid/attr/current that changed or was superseded, > because /proc/pid/attr/current is the one that contains a newline that > consumers are meant to ignore. > > For what it's worth, libapparmor explicitly removes the newline, so this > only matters to LSM-agnostic readers like D-Bus implementations, and to > lower-level AppArmor-aware readers that use the kernel interfaces directly > in preference to using libapparmor. Deferring to the AA maintainer(s) to speak to this.
On 12/19/19 8:48 AM, Simon McVittie wrote: > On Thu, 19 Dec 2019 at 10:00:31 -0500, Stephen Smalley wrote: >> Looks like userspace is generally forgiving of whether the terminating NUL >> byte is included or omitted by the kernel (with different behaviors for >> SELinux - always included, Smack - omitted by /proc/pid/attr/current but >> included in SO_PEERSEC, and AppArmor - omitted for /proc/pid/attr/current >> but includes a terminating \n, omitted for SO_PEERSEC but no terminating >> \n), and procps-ng explicitly tests for printable characters (but truncates >> on the first unprintable character). > > Because LSM people have told me in the past that the '\0' is not > conceptually part of the label, the D-Bus specification and reference > implementation already assume that its presence or absence is irrelevant, > and normalize to a canonical form (which happens to be that it appends a > '\0' if missing, to be nice to C-like languages, but I could equally > have chosen to strip the '\0' and rely on an out-of-band length count). > > By design, SO_PEERCONTEXT and /proc/pid/attr/context don't (can't!) > preserve whether the label originally ended with '\0' or not (because > they are designed to use '\0' as a terminator for each label), so these > new kernel interfaces are already a bit closer than the old kernel > interfaces to how D-Bus represents this information. > > The problematic case is AppArmor's terminating '\n' on > /proc/pid/attr/current, because when I asked in the past, I was told > that it would be (unwise but) valid to have a LSM where "foo" and "foo\n" > are distinct labels. > that is true if any value except \0 is allowed, which is/was the case. I am not opposed to the LSM defining it otherwise. I would also love to ditch the trailing \n from /proc/pid/attr/current, we tried that before and ran into problems with something in userspace. I can look into it again. > If that hypothetical LSM would make procps-ng lose information (because > procps-ng truncates at the first unprintable character), does that change > the situation any? Would that make it acceptable for other LSM-agnostic > user-space components, like the reference implementation of D-Bus, to > assume that stripping a trailing newline from /proc/pid/attr/context > or from one of the component strings of /proc/pid/attr/current is a > non-lossy operation? > >>>> If this new API is an opportunity to declare that LSMs are expected >>>> to put the same canonical form of a label in >>>> /proc/$pid/attr/context and >>>> SO_PEERCONTEXT, possibly with a non-canonical version (adding '\n' or >>>> '\0' or similar) exposed in the older /proc/$pid/attr/current and >>>> SO_PEERSEC interfaces for backwards compatibility, then that >>>> would make >>>> life a lot easier for user-space developers like me. >>> >>> I'm all for this but the current implementation reuses the same >>> underlying hooks as SO_PEERSEC, so it gets the same result for the >>> per-lsm values. We'd need a separate hook if we cannot alter the >>> current AppArmor SO_PEERSEC format. > > If AppArmor was going to change the format of one of its interfaces > (or deviate from it when implementing new interfaces), I'd actually > prefer it to be /proc/pid/attr/current that changed or was superseded, > because /proc/pid/attr/current is the one that contains a newline that > consumers are meant to ignore. > Right, I am not opposed to a new interface. Ditching the trailing \n would be even better if we can get rid of it > For what it's worth, libapparmor explicitly removes the newline, so this > only matters to LSM-agnostic readers like D-Bus implementations, and to > lower-level AppArmor-aware readers that use the kernel interfaces directly > in preference to using libapparmor. > yeah > smcv >
On 12/19/19 9:02 AM, Stephen Smalley wrote: > On 12/19/19 11:48 AM, Simon McVittie wrote: >> On Thu, 19 Dec 2019 at 10:00:31 -0500, Stephen Smalley wrote: >>> Looks like userspace is generally forgiving of whether the terminating NUL >>> byte is included or omitted by the kernel (with different behaviors for >>> SELinux - always included, Smack - omitted by /proc/pid/attr/current but >>> included in SO_PEERSEC, and AppArmor - omitted for /proc/pid/attr/current >>> but includes a terminating \n, omitted for SO_PEERSEC but no terminating >>> \n), and procps-ng explicitly tests for printable characters (but truncates >>> on the first unprintable character). >> >> Because LSM people have told me in the past that the '\0' is not >> conceptually part of the label, the D-Bus specification and reference >> implementation already assume that its presence or absence is irrelevant, >> and normalize to a canonical form (which happens to be that it appends a >> '\0' if missing, to be nice to C-like languages, but I could equally >> have chosen to strip the '\0' and rely on an out-of-band length count). >> >> By design, SO_PEERCONTEXT and /proc/pid/attr/context don't (can't!) >> preserve whether the label originally ended with '\0' or not (because >> they are designed to use '\0' as a terminator for each label), so these >> new kernel interfaces are already a bit closer than the old kernel >> interfaces to how D-Bus represents this information. >> >> The problematic case is AppArmor's terminating '\n' on >> /proc/pid/attr/current, because when I asked in the past, I was told >> that it would be (unwise but) valid to have a LSM where "foo" and "foo\n" >> are distinct labels. > > I don't agree with that stance, but obviously others may differ. > Its not so much a stance as a reality. The LSM allowed anything except \0 values as part of the interface and there was no documentation to set expectations beyond what the code allowed. This could be tightened. >> If that hypothetical LSM would make procps-ng lose information (because >> procps-ng truncates at the first unprintable character), does that change >> the situation any? Would that make it acceptable for other LSM-agnostic >> user-space components, like the reference implementation of D-Bus, to >> assume that stripping a trailing newline from /proc/pid/attr/context >> or from one of the component strings of /proc/pid/attr/current is a >> non-lossy operation? > > IMHO, yes. In fact, looking further, I see that systemd's src/libsystemd/sd-bus/bus-creds.c:bus_creds_add_more() reads /proc/pid/attr/current with its read_one_line_file() helper which ultimately uses read_line_full() and treats EOF, \n, \r, or \0 as terminators and truncates on first such occurrence. > fun >> >>>>> If this new API is an opportunity to declare that LSMs are expected >>>>> to put the same canonical form of a label in >>>>> /proc/$pid/attr/context and >>>>> SO_PEERCONTEXT, possibly with a non-canonical version (adding '\n' or >>>>> '\0' or similar) exposed in the older /proc/$pid/attr/current and >>>>> SO_PEERSEC interfaces for backwards compatibility, then that >>>>> would make >>>>> life a lot easier for user-space developers like me. >>>> >>>> I'm all for this but the current implementation reuses the same >>>> underlying hooks as SO_PEERSEC, so it gets the same result for the >>>> per-lsm values. We'd need a separate hook if we cannot alter the >>>> current AppArmor SO_PEERSEC format. >> >> If AppArmor was going to change the format of one of its interfaces >> (or deviate from it when implementing new interfaces), I'd actually >> prefer it to be /proc/pid/attr/current that changed or was superseded, >> because /proc/pid/attr/current is the one that contains a newline that >> consumers are meant to ignore. >> >> For what it's worth, libapparmor explicitly removes the newline, so this >> only matters to LSM-agnostic readers like D-Bus implementations, and to >> lower-level AppArmor-aware readers that use the kernel interfaces directly >> in preference to using libapparmor. > > Deferring to the AA maintainer(s) to speak to this. I will look into what I can do. If we can ditch the trailing \n, that would be best. I tried to do that once before and we ran into some problems and I had to revert the change. But that was a long time ago and we can probably get away with doing it now.
On 12/19/2019 11:27 AM, John Johansen wrote: > On 12/19/19 9:02 AM, Stephen Smalley wrote: >> On 12/19/19 11:48 AM, Simon McVittie wrote: >>> On Thu, 19 Dec 2019 at 10:00:31 -0500, Stephen Smalley wrote: >>>> Looks like userspace is generally forgiving of whether the terminating NUL >>>> byte is included or omitted by the kernel (with different behaviors for >>>> SELinux - always included, Smack - omitted by /proc/pid/attr/current but >>>> included in SO_PEERSEC, and AppArmor - omitted for /proc/pid/attr/current >>>> but includes a terminating \n, omitted for SO_PEERSEC but no terminating >>>> \n), and procps-ng explicitly tests for printable characters (but truncates >>>> on the first unprintable character). >>> Because LSM people have told me in the past that the '\0' is not >>> conceptually part of the label, the D-Bus specification and reference >>> implementation already assume that its presence or absence is irrelevant, >>> and normalize to a canonical form (which happens to be that it appends a >>> '\0' if missing, to be nice to C-like languages, but I could equally >>> have chosen to strip the '\0' and rely on an out-of-band length count). >>> >>> By design, SO_PEERCONTEXT and /proc/pid/attr/context don't (can't!) >>> preserve whether the label originally ended with '\0' or not (because >>> they are designed to use '\0' as a terminator for each label), so these >>> new kernel interfaces are already a bit closer than the old kernel >>> interfaces to how D-Bus represents this information. >>> >>> The problematic case is AppArmor's terminating '\n' on >>> /proc/pid/attr/current, because when I asked in the past, I was told >>> that it would be (unwise but) valid to have a LSM where "foo" and "foo\n" >>> are distinct labels. >> I don't agree with that stance, but obviously others may differ. >> > Its not so much a stance as a reality. The LSM allowed anything except > \0 values as part of the interface and there was no documentation > to set expectations beyond what the code allowed. > > This could be tightened. > >>> If that hypothetical LSM would make procps-ng lose information (because >>> procps-ng truncates at the first unprintable character), does that change >>> the situation any? Would that make it acceptable for other LSM-agnostic >>> user-space components, like the reference implementation of D-Bus, to >>> assume that stripping a trailing newline from /proc/pid/attr/context >>> or from one of the component strings of /proc/pid/attr/current is a >>> non-lossy operation? >> IMHO, yes. In fact, looking further, I see that systemd's src/libsystemd/sd-bus/bus-creds.c:bus_creds_add_more() reads /proc/pid/attr/current with its read_one_line_file() helper which ultimately uses read_line_full() and treats EOF, \n, \r, or \0 as terminators and truncates on first such occurrence. >> > fun > >>>>>> If this new API is an opportunity to declare that LSMs are expected >>>>>> to put the same canonical form of a label in >>>>>> /proc/$pid/attr/context and >>>>>> SO_PEERCONTEXT, possibly with a non-canonical version (adding '\n' or >>>>>> '\0' or similar) exposed in the older /proc/$pid/attr/current and >>>>>> SO_PEERSEC interfaces for backwards compatibility, then that >>>>>> would make >>>>>> life a lot easier for user-space developers like me. >>>>> I'm all for this but the current implementation reuses the same >>>>> underlying hooks as SO_PEERSEC, so it gets the same result for the >>>>> per-lsm values. We'd need a separate hook if we cannot alter the >>>>> current AppArmor SO_PEERSEC format. >>> If AppArmor was going to change the format of one of its interfaces >>> (or deviate from it when implementing new interfaces), I'd actually >>> prefer it to be /proc/pid/attr/current that changed or was superseded, >>> because /proc/pid/attr/current is the one that contains a newline that >>> consumers are meant to ignore. >>> >>> For what it's worth, libapparmor explicitly removes the newline, so this >>> only matters to LSM-agnostic readers like D-Bus implementations, and to >>> lower-level AppArmor-aware readers that use the kernel interfaces directly >>> in preference to using libapparmor. >> Deferring to the AA maintainer(s) to speak to this. > I will look into what I can do. If we can ditch the trailing \n, that would > be best. I tried to do that once before and we ran into some problems and > I had to revert the change. But that was a long time ago and we can probably > get away with doing it now. I would be happy to define and document that the compound context does not include trailing \n. There are no existing security modules or user space that would be impacted. No one uses /proc/self/attr/context or SO_PEERCONTEXT yet. If AppArmor is happy with stripping the \n I think we're good to go.
On 12/19/19 12:51 PM, Casey Schaufler wrote: > On 12/19/2019 11:27 AM, John Johansen wrote: >> On 12/19/19 9:02 AM, Stephen Smalley wrote: >>> On 12/19/19 11:48 AM, Simon McVittie wrote: >>>> On Thu, 19 Dec 2019 at 10:00:31 -0500, Stephen Smalley wrote: >>>>> Looks like userspace is generally forgiving of whether the terminating NUL >>>>> byte is included or omitted by the kernel (with different behaviors for >>>>> SELinux - always included, Smack - omitted by /proc/pid/attr/current but >>>>> included in SO_PEERSEC, and AppArmor - omitted for /proc/pid/attr/current >>>>> but includes a terminating \n, omitted for SO_PEERSEC but no terminating >>>>> \n), and procps-ng explicitly tests for printable characters (but truncates >>>>> on the first unprintable character). >>>> Because LSM people have told me in the past that the '\0' is not >>>> conceptually part of the label, the D-Bus specification and reference >>>> implementation already assume that its presence or absence is irrelevant, >>>> and normalize to a canonical form (which happens to be that it appends a >>>> '\0' if missing, to be nice to C-like languages, but I could equally >>>> have chosen to strip the '\0' and rely on an out-of-band length count). >>>> >>>> By design, SO_PEERCONTEXT and /proc/pid/attr/context don't (can't!) >>>> preserve whether the label originally ended with '\0' or not (because >>>> they are designed to use '\0' as a terminator for each label), so these >>>> new kernel interfaces are already a bit closer than the old kernel >>>> interfaces to how D-Bus represents this information. >>>> >>>> The problematic case is AppArmor's terminating '\n' on >>>> /proc/pid/attr/current, because when I asked in the past, I was told >>>> that it would be (unwise but) valid to have a LSM where "foo" and "foo\n" >>>> are distinct labels. >>> I don't agree with that stance, but obviously others may differ. >>> >> Its not so much a stance as a reality. The LSM allowed anything except >> \0 values as part of the interface and there was no documentation >> to set expectations beyond what the code allowed. >> >> This could be tightened. >> >>>> If that hypothetical LSM would make procps-ng lose information (because >>>> procps-ng truncates at the first unprintable character), does that change >>>> the situation any? Would that make it acceptable for other LSM-agnostic >>>> user-space components, like the reference implementation of D-Bus, to >>>> assume that stripping a trailing newline from /proc/pid/attr/context >>>> or from one of the component strings of /proc/pid/attr/current is a >>>> non-lossy operation? >>> IMHO, yes. In fact, looking further, I see that systemd's src/libsystemd/sd-bus/bus-creds.c:bus_creds_add_more() reads /proc/pid/attr/current with its read_one_line_file() helper which ultimately uses read_line_full() and treats EOF, \n, \r, or \0 as terminators and truncates on first such occurrence. >>> >> fun >> >>>>>>> If this new API is an opportunity to declare that LSMs are expected >>>>>>> to put the same canonical form of a label in >>>>>>> /proc/$pid/attr/context and >>>>>>> SO_PEERCONTEXT, possibly with a non-canonical version (adding '\n' or >>>>>>> '\0' or similar) exposed in the older /proc/$pid/attr/current and >>>>>>> SO_PEERSEC interfaces for backwards compatibility, then that >>>>>>> would make >>>>>>> life a lot easier for user-space developers like me. >>>>>> I'm all for this but the current implementation reuses the same >>>>>> underlying hooks as SO_PEERSEC, so it gets the same result for the >>>>>> per-lsm values. We'd need a separate hook if we cannot alter the >>>>>> current AppArmor SO_PEERSEC format. >>>> If AppArmor was going to change the format of one of its interfaces >>>> (or deviate from it when implementing new interfaces), I'd actually >>>> prefer it to be /proc/pid/attr/current that changed or was superseded, >>>> because /proc/pid/attr/current is the one that contains a newline that >>>> consumers are meant to ignore. >>>> >>>> For what it's worth, libapparmor explicitly removes the newline, so this >>>> only matters to LSM-agnostic readers like D-Bus implementations, and to >>>> lower-level AppArmor-aware readers that use the kernel interfaces directly >>>> in preference to using libapparmor. >>> Deferring to the AA maintainer(s) to speak to this. >> I will look into what I can do. If we can ditch the trailing \n, that would >> be best. I tried to do that once before and we ran into some problems and >> I had to revert the change. But that was a long time ago and we can probably >> get away with doing it now. > > I would be happy to define and document that the compound context does > not include trailing \n. There are no existing security modules or > user space that would be impacted. No one uses /proc/self/attr/context > or SO_PEERCONTEXT yet. If AppArmor is happy with stripping the \n I > think we're good to go. > yes, we can certainly remove it from peer context, and I expect we can remove it from current as well
diff --git a/arch/alpha/include/uapi/asm/socket.h b/arch/alpha/include/uapi/asm/socket.h index de6c4df61082..b26fb34e4226 100644 --- a/arch/alpha/include/uapi/asm/socket.h +++ b/arch/alpha/include/uapi/asm/socket.h @@ -123,6 +123,7 @@ #define SO_SNDTIMEO_NEW 67 #define SO_DETACH_REUSEPORT_BPF 68 +#define SO_PEERCONTEXT 69 #if !defined(__KERNEL__) diff --git a/arch/mips/include/uapi/asm/socket.h b/arch/mips/include/uapi/asm/socket.h index d0a9ed2ca2d6..10e03507b1ed 100644 --- a/arch/mips/include/uapi/asm/socket.h +++ b/arch/mips/include/uapi/asm/socket.h @@ -134,6 +134,7 @@ #define SO_SNDTIMEO_NEW 67 #define SO_DETACH_REUSEPORT_BPF 68 +#define SO_PEERCONTEXT 69 #if !defined(__KERNEL__) diff --git a/arch/parisc/include/uapi/asm/socket.h b/arch/parisc/include/uapi/asm/socket.h index 10173c32195e..e11df59a84d1 100644 --- a/arch/parisc/include/uapi/asm/socket.h +++ b/arch/parisc/include/uapi/asm/socket.h @@ -115,6 +115,7 @@ #define SO_SNDTIMEO_NEW 0x4041 #define SO_DETACH_REUSEPORT_BPF 0x4042 +#define SO_PEERCONTEXT 0x4043 #if !defined(__KERNEL__) diff --git a/arch/sparc/include/uapi/asm/socket.h b/arch/sparc/include/uapi/asm/socket.h index 8029b681fc7c..5b41ef778040 100644 --- a/arch/sparc/include/uapi/asm/socket.h +++ b/arch/sparc/include/uapi/asm/socket.h @@ -116,6 +116,7 @@ #define SO_SNDTIMEO_NEW 0x0045 #define SO_DETACH_REUSEPORT_BPF 0x0047 +#define SO_PEERCONTEXT 0x0048 #if !defined(__KERNEL__) diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 2bf82e1cf347..2ae10e7f81a7 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -880,8 +880,8 @@ * SO_GETPEERSEC. For tcp sockets this can be meaningful if the * socket is associated with an ipsec SA. * @sock is the local socket. - * @optval userspace memory where the security state is to be copied. - * @optlen userspace int where the module should copy the actual length + * @optval memory where the security state is to be copied. + * @optlen int where the module should copy the actual length * of the security state. * @len as input is the maximum length to copy to userspace provided * by the caller. @@ -1724,9 +1724,8 @@ union security_list_options { int (*socket_setsockopt)(struct socket *sock, int level, int optname); int (*socket_shutdown)(struct socket *sock, int how); int (*socket_sock_rcv_skb)(struct sock *sk, struct sk_buff *skb); - int (*socket_getpeersec_stream)(struct socket *sock, - char __user *optval, - int __user *optlen, unsigned len); + int (*socket_getpeersec_stream)(struct socket *sock, char **optval, + int *optlen, unsigned len); int (*socket_getpeersec_dgram)(struct socket *sock, struct sk_buff *skb, u32 *secid); int (*sk_alloc_security)(struct sock *sk, int family, gfp_t priority); diff --git a/include/linux/security.h b/include/linux/security.h index 536db4dbfcbb..b72bb90b1903 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -181,7 +181,7 @@ struct lsmblob { #define LSMBLOB_NEEDED -2 /* Slot requested on initialization */ #define LSMBLOB_NOT_NEEDED -3 /* Slot not requested */ #define LSMBLOB_DISPLAY -4 /* Use the "display" slot */ -#define LSMBLOB_FIRST -5 /* Use the default "display" slot */ +#define LSMBLOB_COMPOUND -5 /* A compound "display" */ /** * lsmblob_init - initialize an lsmblob structure. @@ -1400,7 +1400,8 @@ int security_socket_setsockopt(struct socket *sock, int level, int optname); int security_socket_shutdown(struct socket *sock, int how); int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb); int security_socket_getpeersec_stream(struct socket *sock, char __user *optval, - int __user *optlen, unsigned len); + int __user *optlen, unsigned len, + int display); int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, struct lsmblob *blob); int security_sk_alloc(struct sock *sk, int family, gfp_t priority); @@ -1534,8 +1535,10 @@ static inline int security_sock_rcv_skb(struct sock *sk, return 0; } -static inline int security_socket_getpeersec_stream(struct socket *sock, char __user *optval, - int __user *optlen, unsigned len) +static inline int security_socket_getpeersec_stream(struct socket *sock, + char __user *optval, + int __user *optlen, + unsigned len, int display) { return -ENOPROTOOPT; } diff --git a/include/uapi/asm-generic/socket.h b/include/uapi/asm-generic/socket.h index 77f7c1638eb1..e3a853d53705 100644 --- a/include/uapi/asm-generic/socket.h +++ b/include/uapi/asm-generic/socket.h @@ -118,6 +118,7 @@ #define SO_SNDTIMEO_NEW 67 #define SO_DETACH_REUSEPORT_BPF 68 +#define SO_PEERCONTEXT 69 #if !defined(__KERNEL__) diff --git a/kernel/audit.c b/kernel/audit.c index d40f64a47c4b..71151ba2a6c2 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -1424,7 +1424,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) len = 0; if (lsmblob_is_set(&audit_sig_lsm)) { err = security_secid_to_secctx(&audit_sig_lsm, - &context, LSMBLOB_FIRST); + &context, 0); if (err) return err; } @@ -2099,7 +2099,7 @@ int audit_log_task_context(struct audit_buffer *ab) if (!lsmblob_is_set(&blob)) return 0; - error = security_secid_to_secctx(&blob, &context, LSMBLOB_FIRST); + error = security_secid_to_secctx(&blob, &context, 0); if (error) { if (error != -EINVAL) goto error_path; diff --git a/net/core/sock.c b/net/core/sock.c index 043db3ce023e..63b7eda81a90 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -1411,7 +1411,12 @@ int sock_getsockopt(struct socket *sock, int level, int optname, break; case SO_PEERSEC: - return security_socket_getpeersec_stream(sock, optval, optlen, len); + return security_socket_getpeersec_stream(sock, optval, optlen, + len, LSMBLOB_DISPLAY); + + case SO_PEERCONTEXT: + return security_socket_getpeersec_stream(sock, optval, optlen, + len, LSMBLOB_COMPOUND); case SO_MARK: v.val = sk->sk_mark; diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c index 60a7665de0e3..fefd1f2d26f8 100644 --- a/net/netlabel/netlabel_unlabeled.c +++ b/net/netlabel/netlabel_unlabeled.c @@ -436,8 +436,7 @@ int netlbl_unlhsh_add(struct net *net, unlhsh_add_return: rcu_read_unlock(); if (audit_buf != NULL) { - if (security_secid_to_secctx(lsmblob, &context, - LSMBLOB_FIRST) == 0) { + if (security_secid_to_secctx(lsmblob, &context, 0) == 0) { audit_log_format(audit_buf, " sec_obj=%s", context.context); security_release_secctx(&context); @@ -493,7 +492,7 @@ static int netlbl_unlhsh_remove_addr4(struct net *net, dev_put(dev); if (entry != NULL && security_secid_to_secctx(&entry->lsmblob, &context, - LSMBLOB_FIRST) == 0) { + 0) == 0) { audit_log_format(audit_buf, " sec_obj=%s", context.context); security_release_secctx(&context); @@ -554,7 +553,7 @@ static int netlbl_unlhsh_remove_addr6(struct net *net, dev_put(dev); if (entry != NULL && security_secid_to_secctx(&entry->lsmblob, &context, - LSMBLOB_FIRST) == 0) { + 0) == 0) { audit_log_format(audit_buf, " sec_obj=%s", context.context); security_release_secctx(&context); @@ -1125,7 +1124,7 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd, lsmb = (struct lsmblob *)&addr6->lsmblob; } - ret_val = security_secid_to_secctx(lsmb, &context, LSMBLOB_FIRST); + ret_val = security_secid_to_secctx(lsmb, &context, 0); if (ret_val != 0) goto list_cb_failure; ret_val = nla_put(cb_arg->skb, diff --git a/net/netlabel/netlabel_user.c b/net/netlabel/netlabel_user.c index 1941877fd16f..537c0bf25e3c 100644 --- a/net/netlabel/netlabel_user.c +++ b/net/netlabel/netlabel_user.c @@ -100,7 +100,7 @@ struct audit_buffer *netlbl_audit_start_common(int type, lsmblob_init(&blob, audit_info->secid); if (audit_info->secid != 0 && - security_secid_to_secctx(&blob, &context, LSMBLOB_FIRST) == 0) { + security_secid_to_secctx(&blob, &context, 0) == 0) { audit_log_format(audit_buf, " subj=%s", context.context); security_release_secctx(&context); } diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 16b992235c11..34edfd29c32f 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -1078,10 +1078,8 @@ static struct aa_label *sk_peer_label(struct sock *sk) * * Note: for tcp only valid if using ipsec or cipso on lan */ -static int apparmor_socket_getpeersec_stream(struct socket *sock, - char __user *optval, - int __user *optlen, - unsigned int len) +static int apparmor_socket_getpeersec_stream(struct socket *sock, char **optval, + int *optlen, unsigned int len) { char *name; int slen, error = 0; @@ -1101,17 +1099,11 @@ static int apparmor_socket_getpeersec_stream(struct socket *sock, if (slen < 0) { error = -ENOMEM; } else { - if (slen > len) { + if (slen > len) error = -ERANGE; - } else if (copy_to_user(optval, name, slen)) { - error = -EFAULT; - goto out; - } - if (put_user(slen, optlen)) - error = -EFAULT; -out: - kfree(name); - + else + *optval = name; + *optlen = slen; } done: diff --git a/security/security.c b/security/security.c index d0b57a7c3b31..1afe245f3246 100644 --- a/security/security.c +++ b/security/security.c @@ -723,6 +723,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. * @@ -2164,8 +2200,8 @@ int security_setprocattr(const char *lsm, const char *name, void *value, hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) { if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm)) continue; - if (lsm == NULL && *display != LSMBLOB_INVALID && - *display != hp->lsmid->slot) + if (lsm == NULL && display != NULL && + *display != LSMBLOB_INVALID && *display != hp->lsmid->slot) continue; return hp->hook.setprocattr(name, value, size); } @@ -2196,7 +2232,7 @@ int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp, */ if (display == LSMBLOB_DISPLAY) display = lsm_task_display(current); - else if (display == LSMBLOB_FIRST) + else if (display == 0) display = LSMBLOB_INVALID; else if (display < 0) { WARN_ONCE(true, @@ -2246,6 +2282,15 @@ void security_release_secctx(struct lsmcontext *cp) struct security_hook_list *hp; bool found = false; + if (cp->slot == LSMBLOB_INVALID) + return; + + if (cp->slot == LSMBLOB_COMPOUND) { + kfree(cp->context); + found = true; + goto clear_out; + } + hlist_for_each_entry(hp, &security_hook_heads.release_secctx, list) if (cp->slot == hp->lsmid->slot) { hp->hook.release_secctx(cp->context, cp->len); @@ -2253,6 +2298,7 @@ void security_release_secctx(struct lsmcontext *cp) break; } +clear_out: memset(cp, 0, sizeof(*cp)); if (!found) @@ -2389,17 +2435,67 @@ int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb) EXPORT_SYMBOL(security_sock_rcv_skb); int security_socket_getpeersec_stream(struct socket *sock, char __user *optval, - int __user *optlen, unsigned len) + int __user *optlen, unsigned len, + int display) { - int display = lsm_task_display(current); struct security_hook_list *hp; + char *final = NULL; + char *cp; + int rc = 0; + unsigned finallen = 0; + unsigned clen = 0; - hlist_for_each_entry(hp, &security_hook_heads.socket_getpeersec_stream, - list) - if (display == LSMBLOB_INVALID || display == hp->lsmid->slot) - return hp->hook.socket_getpeersec_stream(sock, optval, - optlen, len); - return -ENOPROTOOPT; + switch (display) { + case LSMBLOB_DISPLAY: + rc = -ENOPROTOOPT; + display = lsm_task_display(current); + hlist_for_each_entry(hp, + &security_hook_heads.socket_getpeersec_stream, + list) + if (display == LSMBLOB_INVALID || + display == hp->lsmid->slot) { + rc = hp->hook.socket_getpeersec_stream(sock, + &final, &finallen, len); + break; + } + break; + case LSMBLOB_COMPOUND: + /* + * A compound context, in the form [lsm\0value\0]... + */ + hlist_for_each_entry(hp, + &security_hook_heads.socket_getpeersec_stream, + list) { + rc = hp->hook.socket_getpeersec_stream(sock, &cp, &clen, + len); + if (rc == -EINVAL || rc == -ENOPROTOOPT) { + rc = 0; + continue; + } + if (rc) { + kfree(final); + return rc; + } + rc = append_ctx(&final, &finallen, hp->lsmid->lsm, + cp, clen); + } + if (final == NULL) + return -EINVAL; + break; + default: + return -EINVAL; + } + + if (finallen > len) + rc = -ERANGE; + else if (copy_to_user(optval, final, finallen)) + rc = -EFAULT; + + if (put_user(finallen, optlen)) + rc = -EFAULT; + + kfree(final); + return rc; } int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index cd4743331800..c3e6fd3f8c56 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -5056,10 +5056,8 @@ static int selinux_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb) return err; } -static int selinux_socket_getpeersec_stream(struct socket *sock, - char __user *optval, - int __user *optlen, - unsigned int len) +static int selinux_socket_getpeersec_stream(struct socket *sock, char **optval, + int *optlen, unsigned int len) { int err = 0; char *scontext; @@ -5079,18 +5077,12 @@ static int selinux_socket_getpeersec_stream(struct socket *sock, if (err) return err; - if (scontext_len > len) { + if (scontext_len > len) err = -ERANGE; - goto out_len; - } - - if (copy_to_user(optval, scontext, scontext_len)) - err = -EFAULT; + else + *optval = scontext; -out_len: - if (put_user(scontext_len, optlen)) - err = -EFAULT; - kfree(scontext); + *optlen = scontext_len; return err; } diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 7349ce263759..7d449188ca16 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -3957,28 +3957,29 @@ static int smack_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb) * * returns zero on success, an error code otherwise */ -static int smack_socket_getpeersec_stream(struct socket *sock, - char __user *optval, - int __user *optlen, unsigned len) +static int smack_socket_getpeersec_stream(struct socket *sock, char **optval, + int *optlen, unsigned len) { - struct socket_smack *ssp; - char *rcp = ""; - int slen = 1; + struct socket_smack *ssp = smack_sock(sock->sk); + char *rcp; + int slen; int rc = 0; - ssp = smack_sock(sock->sk); - if (ssp->smk_packet != NULL) { - rcp = ssp->smk_packet->smk_known; - slen = strlen(rcp) + 1; + if (ssp->smk_packet == NULL) { + *optlen = 0; + return -EINVAL; } + rcp = ssp->smk_packet->smk_known; + slen = strlen(rcp) + 1; if (slen > len) rc = -ERANGE; - else if (copy_to_user(optval, rcp, slen) != 0) - rc = -EFAULT; - - if (put_user(slen, optlen) != 0) - rc = -EFAULT; + else { + *optval = kstrdup(rcp, GFP_KERNEL); + if (*optval == NULL) + rc = -ENOMEM; + } + *optlen = slen; return rc; }
The getsockopt SO_PEERSEC provides the LSM based security information for a single module, but for reasons of backward compatibility cannot include the information for multiple modules. A new option SO_PEERCONTEXT is added to report the security "context" of multiple modules using a "compound" format lsm1\0value\0lsm2\0value\0 This is expected to be used by system services, including dbus-daemon. The exact format of a compound context has been the subject of considerable debate. This format was suggested by Simon McVittie, a dbus maintainer with a significant stake in the format being usable. Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> cc: netdev@vger.kernel.org --- arch/alpha/include/uapi/asm/socket.h | 1 + arch/mips/include/uapi/asm/socket.h | 1 + arch/parisc/include/uapi/asm/socket.h | 1 + arch/sparc/include/uapi/asm/socket.h | 1 + include/linux/lsm_hooks.h | 9 +- include/linux/security.h | 11 ++- include/uapi/asm-generic/socket.h | 1 + kernel/audit.c | 4 +- net/core/sock.c | 7 +- net/netlabel/netlabel_unlabeled.c | 9 +- net/netlabel/netlabel_user.c | 2 +- security/apparmor/lsm.c | 20 ++--- security/security.c | 118 +++++++++++++++++++++++--- security/selinux/hooks.c | 20 ++--- security/smack/smack_lsm.c | 31 +++---- 15 files changed, 164 insertions(+), 72 deletions(-)