On 8/8/2019 3:21 PM, Kees Cook wrote:
> On Wed, Aug 07, 2019 at 12:44:08PM -0700, 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
>> uasable.
>>
>> Signed-off-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
>> ---
>> [...]
>> diff --git a/security/security.c b/security/security.c
>> index 2f4a430a1126..0ea7ee27e331 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -2117,8 +2117,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);
>> }
>> @@ -2342,17 +2342,91 @@ 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;
>> + char *tp;
>> + int rc = 0;
>> + unsigned finallen = 0;
>> + unsigned llen;
>> + unsigned clen = 0;
>> + unsigned tlen;
> Please move the case-specific variables into the case scope, like (and
> expand type names):
>
> case LSMBLOB_COMPOUND: {
> unsigned int clen ...;
I've never been a fan of that style. I'll probably make the
special cases into functions.
>
>> +
>> + 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);
> Is passing "len" here useful at all? It's kind of a lie, but nothing
> else wouldn't also be a lie. :)
I could be reducing the value of len for each component gathered.
It requires yet another temporary length variable, and exceeding the
len passed has to get checked in here anyway. I may be able to make
this more sensible.
>
>> + if (rc == -EINVAL || rc == -ENOPROTOOPT) {
>> + rc = 0;
>> + continue;
>> + }
>> + if (rc) {
>> + kfree(final);
>> + return rc;
>> + }
>> + /*
>> + * Don't propogate trailing nul bytes.
> typo: propagate
Not a typo, a spelling error. I'm real good at spelling errors.
>
>> + */
>> + clen = strnlen(cp, clen) + 1;
>> + llen = strlen(hp->lsmid->lsm) + 1;
>> + tlen = llen + clen;
>> + if (final)
>> + tlen += finallen;
> You can drop the "if (final)" since finallen is initialized to 0.
Right you are.
>
>> + tp = kzalloc(tlen, GFP_KERNEL);
> I'm not a huge fan of "c", "l", and "t" prefixes -- can you just make
> these a little more self-documenting? cp and clen could be value and
> value_len. llen could be lsm_name_len. tp and tlen could be tuple and
> tuple_len. (And maybe final and finallen could be compound and
> compound_len?)
You didn't spend much time using TTY33s, did you? Or 80 column punch cards
where the last 8 are reserved for collation numbers, I'd bet. Kids. OK,
I'll see what I can do since I will be fiddling about anyway.
>
>> + if (tp == NULL) {
>> + kfree(cp);
>> + kfree(final);
>> + return -ENOMEM;
>> + }
>> + if (final)
>> + memcpy(tp, final, finallen);
>> + memcpy(tp + finallen, hp->lsmid->lsm, llen);
>> + memcpy(tp + finallen + llen, cp, clen);
>> + kfree(cp);
>> + if (final)
>> + kfree(final);
> Just kfree(final) is safe here -- kfree(NULL) is valid.
Just so. At some point there was more in the if condition.
>
>> + final = tp;
>> + finallen = tlen;
>> + }
>> + if (final == NULL)
>> + return -EINVAL;
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>>
>> - 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;
>> + 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;
>> }
> Otherwise, looks good.
>
> Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
>
> -Kees
>
>>
>> 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 fcad2e3432d2..5e7d61754798 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -4923,10 +4923,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;
>> @@ -4946,18 +4944,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 7a30b8692b1e..40c75205a914 100644
>> --- a/security/smack/smack_lsm.c
>> +++ b/security/smack/smack_lsm.c
>> @@ -3919,28 +3919,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;
>> }
>> --
>> 2.20.1
>>
