Smack: Assign smack_known_web label for kernel thread's
diff mbox

Message ID 1478170479-22841-1-git-send-email-jobol@nonadev.net
State New
Headers show

Commit Message

José Bollo Nov. 3, 2016, 10:54 a.m. UTC
From: jooseong lee <jooseong.lee@samsung.com>

Assign smack_known_web label for kernel thread's socket in the sk_alloc_security hook

Creating struct sock by sk_alloc function in various kernel subsystems
like bluetooth dosen't call smack_socket_post_create(). In such case,
received sock label is the floor('_') label and makes access deny.

Signed-off-by: jooseong lee <jooseong.lee@samsung.com>
---
 security/smack/smack_lsm.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Paul Moore Nov. 3, 2016, 10:10 p.m. UTC | #1
On Thu, Nov 3, 2016 at 6:54 AM,  <jobol@nonadev.net> wrote:
> From: jooseong lee <jooseong.lee@samsung.com>
>
> Assign smack_known_web label for kernel thread's socket in the sk_alloc_security hook
>
> Creating struct sock by sk_alloc function in various kernel subsystems
> like bluetooth dosen't call smack_socket_post_create(). In such case,
> received sock label is the floor('_') label and makes access deny.
>
> Signed-off-by: jooseong lee <jooseong.lee@samsung.com>
> ---
>  security/smack/smack_lsm.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)

I may be wrong, but given that you are mentioning bluetooth I suspect
you running into problems with the child sockets created by the
bluetooth code (you are probably seeing similar issues with AF_ALG,
AF_IUCV, AF_SCTP, AF_TIPC) as they don't go through the normal socket
creation process that you see.  Assuming that is the problem, we solve
this in SELinux by leveraging the security_sk_clone() hook to clone
the parent's security attributes to the newly created child socket; I
realize this diverges from the Smack socket labeling method of
inheriting from the task, but it seems like a reasonable approach.

> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 1cb0602..e73460d 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -2337,8 +2337,16 @@ static int smack_sk_alloc_security(struct sock *sk, int family, gfp_t gfp_flags)
>         if (ssp == NULL)
>                 return -ENOMEM;
>
> -       ssp->smk_in = skp;
> -       ssp->smk_out = skp;
> +       /*
> +        * Sockets created by kernel threads receive web label.
> +        */
> +       if (unlikely(current->flags & PF_KTHREAD)) {
> +               ssp->smk_in = &smack_known_web;
> +               ssp->smk_out = &smack_known_web;
> +       } else {
> +               ssp->smk_in = skp;
> +               ssp->smk_out = skp;
> +       }
>         ssp->smk_packet = NULL;
>
>         sk->sk_security = ssp;
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
José Bollo Nov. 4, 2016, 9:24 a.m. UTC | #2
Le jeudi 03 novembre 2016 à 18:10 -0400, Paul Moore a écrit :
> On Thu, Nov 3, 2016 at 6:54 AM,  <jobol@nonadev.net> wrote:
> > 
> > From: jooseong lee <jooseong.lee@samsung.com>
> > 
> > Assign smack_known_web label for kernel thread's socket in the
> > sk_alloc_security hook
> > 
> > Creating struct sock by sk_alloc function in various kernel
> > subsystems
> > like bluetooth dosen't call smack_socket_post_create(). In such
> > case,
> > received sock label is the floor('_') label and makes access deny.
> > 
> > Signed-off-by: jooseong lee <jooseong.lee@samsung.com>
> > ---
> >  security/smack/smack_lsm.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> I may be wrong, but given that you are mentioning bluetooth I suspect
> you running into problems with the child sockets created by the
> bluetooth code (you are probably seeing similar issues with AF_ALG,
> AF_IUCV, AF_SCTP, AF_TIPC) as they don't go through the normal socket
> creation process that you see.

Hello Paul,

You are right.

>   Assuming that is the problem, we solve
> this in SELinux by leveraging the security_sk_clone() hook to clone
> the parent's security attributes to the newly created child socket; I
> realize this diverges from the Smack socket labeling method of
> inheriting from the task, but it seems like a reasonable approach.

During some investigation, I sew the solution used by SELinux.
Casey, what do you think about using security_sk_clone()?
Wouldn't it be interesting for long term evolution to solve the same
issue with the same solution?

Best regards
José


> > 
> > diff --git a/security/smack/smack_lsm.c
> > b/security/smack/smack_lsm.c
> > index 1cb0602..e73460d 100644
> > --- a/security/smack/smack_lsm.c
> > +++ b/security/smack/smack_lsm.c
> > @@ -2337,8 +2337,16 @@ static int smack_sk_alloc_security(struct
> > sock *sk, int family, gfp_t gfp_flags)
> >         if (ssp == NULL)
> >                 return -ENOMEM;
> > 
> > -       ssp->smk_in = skp;
> > -       ssp->smk_out = skp;
> > +       /*
> > +        * Sockets created by kernel threads receive web label.
> > +        */
> > +       if (unlikely(current->flags & PF_KTHREAD)) {
> > +               ssp->smk_in = &smack_known_web;
> > +               ssp->smk_out = &smack_known_web;
> > +       } else {
> > +               ssp->smk_in = skp;
> > +               ssp->smk_out = skp;
> > +       }
> >         ssp->smk_packet = NULL;
> > 
> >         sk->sk_security = ssp;
> > --
> > 2.7.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-
> > security-module" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Casey Schaufler Nov. 4, 2016, 3:13 p.m. UTC | #3
On 11/4/2016 2:24 AM, José Bollo wrote:
> Le jeudi 03 novembre 2016 à 18:10 -0400, Paul Moore a écrit :
>> On Thu, Nov 3, 2016 at 6:54 AM,  <jobol@nonadev.net> wrote:
>>> From: jooseong lee <jooseong.lee@samsung.com>
>>>
>>> Assign smack_known_web label for kernel thread's socket in the
>>> sk_alloc_security hook
>>>
>>> Creating struct sock by sk_alloc function in various kernel
>>> subsystems
>>> like bluetooth dosen't call smack_socket_post_create(). In such
>>> case,
>>> received sock label is the floor('_') label and makes access deny.
>>>
>>> Signed-off-by: jooseong lee <jooseong.lee@samsung.com>
>>> ---
>>>  security/smack/smack_lsm.c | 12 ++++++++++--
>>>  1 file changed, 10 insertions(+), 2 deletions(-)
>> I may be wrong, but given that you are mentioning bluetooth I suspect
>> you running into problems with the child sockets created by the
>> bluetooth code (you are probably seeing similar issues with AF_ALG,
>> AF_IUCV, AF_SCTP, AF_TIPC) as they don't go through the normal socket
>> creation process that you see.
> Hello Paul,
>
> You are right.
>
>>   Assuming that is the problem, we solve
>> this in SELinux by leveraging the security_sk_clone() hook to clone
>> the parent's security attributes to the newly created child socket; I
>> realize this diverges from the Smack socket labeling method of
>> inheriting from the task, but it seems like a reasonable approach.
> During some investigation, I sew the solution used by SELinux.
> Casey, what do you think about using security_sk_clone()?
> Wouldn't it be interesting for long term evolution to solve the same
> issue with the same solution?

I don't see an advantage to doing it that way. Jobol's
patch addresses the issue very simply and has been tested
in Tizen.

>
> Best regards
> José
>
>
>>> diff --git a/security/smack/smack_lsm.c
>>> b/security/smack/smack_lsm.c
>>> index 1cb0602..e73460d 100644
>>> --- a/security/smack/smack_lsm.c
>>> +++ b/security/smack/smack_lsm.c
>>> @@ -2337,8 +2337,16 @@ static int smack_sk_alloc_security(struct
>>> sock *sk, int family, gfp_t gfp_flags)
>>>         if (ssp == NULL)
>>>                 return -ENOMEM;
>>>
>>> -       ssp->smk_in = skp;
>>> -       ssp->smk_out = skp;
>>> +       /*
>>> +        * Sockets created by kernel threads receive web label.
>>> +        */
>>> +       if (unlikely(current->flags & PF_KTHREAD)) {
>>> +               ssp->smk_in = &smack_known_web;
>>> +               ssp->smk_out = &smack_known_web;
>>> +       } else {
>>> +               ssp->smk_in = skp;
>>> +               ssp->smk_out = skp;
>>> +       }
>>>         ssp->smk_packet = NULL;
>>>
>>>         sk->sk_security = ssp;
>>> --
>>> 2.7.4
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-
>>> security-module" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 1cb0602..e73460d 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -2337,8 +2337,16 @@  static int smack_sk_alloc_security(struct sock *sk, int family, gfp_t gfp_flags)
 	if (ssp == NULL)
 		return -ENOMEM;
 
-	ssp->smk_in = skp;
-	ssp->smk_out = skp;
+	/*
+	 * Sockets created by kernel threads receive web label.
+	 */
+	if (unlikely(current->flags & PF_KTHREAD)) {
+		ssp->smk_in = &smack_known_web;
+		ssp->smk_out = &smack_known_web;
+	} else {
+		ssp->smk_in = skp;
+		ssp->smk_out = skp;
+	}
 	ssp->smk_packet = NULL;
 
 	sk->sk_security = ssp;