[PATCH] Smack: replace capable() with ns_capable()

[PATCH] Smack: replace capable() with ns_capable()

[Sungbae Yoo]

If current task has capabilities, Smack operations (eg. Changing own smack
label) should be available even inside of namespace.

Signed-off-by: Sungbae Yoo <sungbae.yoo@samsung.com>

diff --git a/security/smack/smack_access.c b/security/smack/smack_access.c
index 00f6b38..f6b2c35 100644
--- a/security/smack/smack_access.c
+++ b/security/smack/smack_access.c
@@ -639,7 +639,7 @@ int smack_privileged(int cap)
 	struct smack_known *skp = smk_of_current();
 	struct smack_onlycap *sop;
 
-	if (!capable(cap))
+	if (!ns_capable(current_user_ns(), cap))
 		return 0;
 
 	rcu_read_lock();
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index a143328..7fdc3dd 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -403,7 +403,8 @@ static int smk_ptrace_rule_check(struct task_struct *tracer,
 			rc = 0;
 		else if (smack_ptrace_rule == SMACK_PTRACE_DRACONIAN)
 			rc = -EACCES;
-		else if (capable(CAP_SYS_PTRACE))
+		else if (ns_capable(__task_cred(tracer)->user_ns,
+				    CAP_SYS_PTRACE))
 			rc = 0;
 		else
 			rc = -EACCES;
-- 
1.9.1

Re: [PATCH] Smack: replace capable() with ns_capable()

[Lukasz Pawelczyk]

On pią, 2015-07-24 at 20:26 +0900, Sungbae Yoo wrote:
> If current task has capabilities, Smack operations (eg. Changing own 
> smack
> label) should be available even inside of namespace.
> 
> Signed-off-by: Sungbae Yoo <sungbae.yoo@samsung.com>
> 
> diff --git a/security/smack/smack_access.c 
> b/security/smack/smack_access.c
> index 00f6b38..f6b2c35 100644
> --- a/security/smack/smack_access.c
> +++ b/security/smack/smack_access.c
> @@ -639,7 +639,7 @@ int smack_privileged(int cap)
>  	struct smack_known *skp = smk_of_current();
>  	struct smack_onlycap *sop;
>  
> -	if (!capable(cap))
> +	if (!ns_capable(current_user_ns(), cap))
>  		return 0;

It's not that easy.

With this change Smack becomes completely insecure. You can change
rules as an unprivileged user without any problems now.
What you want is Smack namespace that was made to remedy exactly this
issue (e.g. changing own labels inside a namespace).

>  
>  	rcu_read_lock();
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index a143328..7fdc3dd 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -403,7 +403,8 @@ static int smk_ptrace_rule_check(struct 
> task_struct *tracer,
>  			rc = 0;
>  		else if (smack_ptrace_rule == 
> SMACK_PTRACE_DRACONIAN)
>  			rc = -EACCES;
> -		else if (capable(CAP_SYS_PTRACE))
> +		else if (ns_capable(__task_cred(tracer)->user_ns,
> +				    CAP_SYS_PTRACE))
>  			rc = 0;
>  		else
>  			rc = -EACCES;
-- 
Lukasz Pawelczyk
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH] Smack: replace capable() with ns_capable()

[Casey Schaufler]

On 7/24/2015 4:40 AM, Lukasz Pawelczyk wrote:
> On pią, 2015-07-24 at 20:26 +0900, Sungbae Yoo wrote:
>> If current task has capabilities, Smack operations (eg. Changing own 
>> smack
>> label) should be available even inside of namespace.
>>
>> Signed-off-by: Sungbae Yoo <sungbae.yoo@samsung.com>

For the reasons Lukasz outlines below.

Nacked-by: Casey Schaufler <casey@schaufler-ca.com>

>>
>> diff --git a/security/smack/smack_access.c 
>> b/security/smack/smack_access.c
>> index 00f6b38..f6b2c35 100644
>> --- a/security/smack/smack_access.c
>> +++ b/security/smack/smack_access.c
>> @@ -639,7 +639,7 @@ int smack_privileged(int cap)
>>  	struct smack_known *skp = smk_of_current();
>>  	struct smack_onlycap *sop;
>>  
>> -	if (!capable(cap))
>> +	if (!ns_capable(current_user_ns(), cap))
>>  		return 0;
> It's not that easy.
>
> With this change Smack becomes completely insecure. You can change
> rules as an unprivileged user without any problems now.
> What you want is Smack namespace that was made to remedy exactly this
> issue (e.g. changing own labels inside a namespace).
>
>>  
>>  	rcu_read_lock();
>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>> index a143328..7fdc3dd 100644
>> --- a/security/smack/smack_lsm.c
>> +++ b/security/smack/smack_lsm.c
>> @@ -403,7 +403,8 @@ static int smk_ptrace_rule_check(struct 
>> task_struct *tracer,
>>  			rc = 0;
>>  		else if (smack_ptrace_rule == 
>> SMACK_PTRACE_DRACONIAN)
>>  			rc = -EACCES;
>> -		else if (capable(CAP_SYS_PTRACE))
>> +		else if (ns_capable(__task_cred(tracer)->user_ns,
>> +				    CAP_SYS_PTRACE))
>>  			rc = 0;
>>  		else
>>  			rc = -EACCES;

RE: [PATCH] Smack: replace capable() with ns_capable()

[Sungbae Yoo]

So, Do you agree to allow the process to change its own labels?

Now, init process(eg. systemd) can't be running in user namespace properly
because it can't be assign smack label to service.

If you agree, I'll upload another patch limited to this.


-----Original Message-----
From: Lukasz Pawelczyk [mailto:l.pawelczyk@samsung.com] 
Sent: Friday, July 24, 2015 8:41 PM
To: Sungbae Yoo; Casey Schaufler
Cc: James Morris; Serge E. Hallyn; linux-security-module@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Smack: replace capable() with ns_capable()

On pią, 2015-07-24 at 20:26 +0900, Sungbae Yoo wrote:
> If current task has capabilities, Smack operations (eg. Changing own 
> smack
> label) should be available even inside of namespace.
> 
> Signed-off-by: Sungbae Yoo <sungbae.yoo@samsung.com>
> 
> diff --git a/security/smack/smack_access.c 
> b/security/smack/smack_access.c index 00f6b38..f6b2c35 100644
> --- a/security/smack/smack_access.c
> +++ b/security/smack/smack_access.c
> @@ -639,7 +639,7 @@ int smack_privileged(int cap)
>  	struct smack_known *skp = smk_of_current();
>  	struct smack_onlycap *sop;
>  
> -	if (!capable(cap))
> +	if (!ns_capable(current_user_ns(), cap))
>  		return 0;

It's not that easy.

With this change Smack becomes completely insecure. You can change rules as an unprivileged user without any problems now.
What you want is Smack namespace that was made to remedy exactly this issue (e.g. changing own labels inside a namespace).

>  
>  	rcu_read_lock();
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c 
> index a143328..7fdc3dd 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -403,7 +403,8 @@ static int smk_ptrace_rule_check(struct 
> task_struct *tracer,
>  			rc = 0;
>  		else if (smack_ptrace_rule ==
> SMACK_PTRACE_DRACONIAN)
>  			rc = -EACCES;
> -		else if (capable(CAP_SYS_PTRACE))
> +		else if (ns_capable(__task_cred(tracer)->user_ns,
> +				    CAP_SYS_PTRACE))
>  			rc = 0;
>  		else
>  			rc = -EACCES;
--
Lukasz Pawelczyk
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH] Smack: replace capable() with ns_capable()

[Lukasz Pawelczyk]

On pon, 2015-07-27 at 10:27 +0900, Sungbae Yoo wrote:
> So, Do you agree to allow the process to change its own labels?

Yes, by using a proper method as I mentioned below (e.g. Smack
namespace posted to this list).

> Now, init process(eg. systemd) can't be running in user namespace 
> properly
> because it can't be assign smack label to service.
> 
> If you agree, I'll upload another patch limited to this.

This won't help. Limiting this to init process will still allow every
process outside of a namespace to change its own label, still insecure.


> -----Original Message-----
> From: Lukasz Pawelczyk [mailto:l.pawelczyk@samsung.com] 
> Sent: Friday, July 24, 2015 8:41 PM
> To: Sungbae Yoo; Casey Schaufler
> Cc: James Morris; Serge E. Hallyn; 
> linux-security-module@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] Smack: replace capable() with ns_capable()
> 
> On pią, 2015-07-24 at 20:26 +0900, Sungbae Yoo wrote:
> > If current task has capabilities, Smack operations (eg. Changing 
> > own 
> > smack
> > label) should be available even inside of namespace.
> > 
> > Signed-off-by: Sungbae Yoo <sungbae.yoo@samsung.com>
> > 
> > diff --git a/security/smack/smack_access.c 
> > b/security/smack/smack_access.c index 00f6b38..f6b2c35 100644
> > --- a/security/smack/smack_access.c
> > +++ b/security/smack/smack_access.c
> > @@ -639,7 +639,7 @@ int smack_privileged(int cap)
> >  	struct smack_known *skp = smk_of_current();
> >  	struct smack_onlycap *sop;
> >  
> > -	if (!capable(cap))
> > +	if (!ns_capable(current_user_ns(), cap))
> >  		return 0;
> 
> It's not that easy.
> 
> With this change Smack becomes completely insecure. You can change 
> rules as an unprivileged user without any problems now.
> What you want is Smack namespace that was made to remedy exactly this 
> issue (e.g. changing own labels inside a namespace).
> 
> >  
> >  	rcu_read_lock();
> > diff --git a/security/smack/smack_lsm.c 
> > b/security/smack/smack_lsm.c 
> > index a143328..7fdc3dd 100644
> > --- a/security/smack/smack_lsm.c
> > +++ b/security/smack/smack_lsm.c
> > @@ -403,7 +403,8 @@ static int smk_ptrace_rule_check(struct 
> > task_struct *tracer,
> >  			rc = 0;
> >  		else if (smack_ptrace_rule ==
> > SMACK_PTRACE_DRACONIAN)
> >  			rc = -EACCES;
> > -		else if (capable(CAP_SYS_PTRACE))
> > +		else if (ns_capable(__task_cred(tracer)->user_ns,
> > +				    CAP_SYS_PTRACE))
> >  			rc = 0;
> >  		else
> >  			rc = -EACCES;
> --
> Lukasz Pawelczyk
> Samsung R&D Institute Poland
> Samsung Electronics
> 
> 
> 
> --
> 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
-- 
Lukasz Pawelczyk
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH] Smack: replace capable() with ns_capable()

[Casey Schaufler]

On 7/26/2015 6:27 PM, Sungbae Yoo wrote:
> So, Do you agree to allow the process to change its own labels?

No. This requires CAP_MAC_ADMIN. Smack is mandatory access control.
Being in a namespace (as they are implemented today) is not sufficient.

>
> Now, init process(eg. systemd) can't be running in user namespace properly
> because it can't be assign smack label to service.
>
> If you agree, I'll upload another patch limited to this.
>
>
> -----Original Message-----
> From: Lukasz Pawelczyk [mailto:l.pawelczyk@samsung.com] 
> Sent: Friday, July 24, 2015 8:41 PM
> To: Sungbae Yoo; Casey Schaufler
> Cc: James Morris; Serge E. Hallyn; linux-security-module@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] Smack: replace capable() with ns_capable()
>
> On pią, 2015-07-24 at 20:26 +0900, Sungbae Yoo wrote:
>> If current task has capabilities, Smack operations (eg. Changing own 
>> smack
>> label) should be available even inside of namespace.
>>
>> Signed-off-by: Sungbae Yoo <sungbae.yoo@samsung.com>
>>
>> diff --git a/security/smack/smack_access.c 
>> b/security/smack/smack_access.c index 00f6b38..f6b2c35 100644
>> --- a/security/smack/smack_access.c
>> +++ b/security/smack/smack_access.c
>> @@ -639,7 +639,7 @@ int smack_privileged(int cap)
>>  	struct smack_known *skp = smk_of_current();
>>  	struct smack_onlycap *sop;
>>  
>> -	if (!capable(cap))
>> +	if (!ns_capable(current_user_ns(), cap))
>>  		return 0;
> It's not that easy.
>
> With this change Smack becomes completely insecure. You can change rules as an unprivileged user without any problems now.
> What you want is Smack namespace that was made to remedy exactly this issue (e.g. changing own labels inside a namespace).
>
>>  
>>  	rcu_read_lock();
>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c 
>> index a143328..7fdc3dd 100644
>> --- a/security/smack/smack_lsm.c
>> +++ b/security/smack/smack_lsm.c
>> @@ -403,7 +403,8 @@ static int smk_ptrace_rule_check(struct 
>> task_struct *tracer,
>>  			rc = 0;
>>  		else if (smack_ptrace_rule ==
>> SMACK_PTRACE_DRACONIAN)
>>  			rc = -EACCES;
>> -		else if (capable(CAP_SYS_PTRACE))
>> +		else if (ns_capable(__task_cred(tracer)->user_ns,
>> +				    CAP_SYS_PTRACE))
>>  			rc = 0;
>>  		else
>>  			rc = -EACCES;
> --
> Lukasz Pawelczyk
> Samsung R&D Institute Poland
> Samsung Electronics
>
>
>
>

Re: [PATCH] Smack: replace capable() with ns_capable()

[Serge E. Hallyn]

On Tue, Jul 28, 2015 at 07:36:30AM -0700, Casey Schaufler wrote:
> On 7/26/2015 6:27 PM, Sungbae Yoo wrote:
> > So, Do you agree to allow the process to change its own labels?
> 
> No. This requires CAP_MAC_ADMIN. Smack is mandatory access control.
> Being in a namespace (as they are implemented today) is not sufficient.

"requires CAP_MAC_ADMIN" should probably read "requires
CAP_MAC_ADMIN against initial user namespace."  Any unprivileged
user can unshare a user_ns and get CAP_MAC_ADMIN.

I'm terribly sorry I'm not yet caught up on the smack-lsm thread.
But intuitively I'd think that you'd want a way for smack policy
to say "this label is allowed to create a user-ns which will be
allowed to CAP_MAC_ADMIN", so then smack_capable() can use that
information to cleanly deny CAP_MAC_ADMIN in namespaces.

-serge

Re: [PATCH] Smack: replace capable() with ns_capable()

[Casey Schaufler]

On 7/28/2015 8:06 AM, Serge E. Hallyn wrote:
> On Tue, Jul 28, 2015 at 07:36:30AM -0700, Casey Schaufler wrote:
>> On 7/26/2015 6:27 PM, Sungbae Yoo wrote:
>>> So, Do you agree to allow the process to change its own labels?
>> No. This requires CAP_MAC_ADMIN. Smack is mandatory access control.
>> Being in a namespace (as they are implemented today) is not sufficient.
> "requires CAP_MAC_ADMIN" should probably read "requires
> CAP_MAC_ADMIN against initial user namespace."  Any unprivileged
> user can unshare a user_ns and get CAP_MAC_ADMIN.

As you say. Since the inode's xattrs are common you need
privilege relative to the initial namespace.

>
> I'm terribly sorry I'm not yet caught up on the smack-lsm thread.
> But intuitively I'd think that you'd want a way for smack policy
> to say "this label is allowed to create a user-ns which will be
> allowed to CAP_MAC_ADMIN", so then smack_capable() can use that
> information to cleanly deny CAP_MAC_ADMIN in namespaces.
>
> -serge
>