[PATCH] security: unconditionally call Yama

[PATCH] security: unconditionally call Yama

[Kees Cook]

Unconditionally call Yama, no matter what LSM module is selected.

Ubuntu and Chrome OS already carry patches to do this, and Fedora has
voiced interest in doing this as well. Instead of having everyone carry
these patches, just switch Yama to being unconditional when compiled
into the kernel.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/security.h |   31 +++++++++++++++++++++++++++++++
 security/Kconfig         |    5 -----
 security/security.c      |   13 +++++++++++++
 security/yama/yama_lsm.c |   14 ++++----------
 4 files changed, 48 insertions(+), 15 deletions(-)

diff --git a/include/linux/security.h b/include/linux/security.h
index 3dea6a9..01ef030 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -3021,5 +3021,36 @@ static inline void free_secdata(void *secdata)
 { }
 #endif /* CONFIG_SECURITY */
 
+#ifdef CONFIG_SECURITY_YAMA
+extern int yama_ptrace_access_check(struct task_struct *child,
+				    unsigned int mode);
+extern int yama_ptrace_traceme(struct task_struct *parent);
+extern void yama_task_free(struct task_struct *task);
+extern int yama_task_prctl(int option, unsigned long arg2, unsigned long arg3,
+			   unsigned long arg4, unsigned long arg5);
+#else
+static inline int yama_ptrace_access_check(struct task_struct *child,
+					   unsigned int mode)
+{
+	return 0;
+}
+
+static inline int yama_ptrace_traceme(struct task_struct *parent)
+{
+	return 0;
+}
+
+static inline void yama_task_free(struct task_struct *task)
+{
+}
+
+static inline int yama_task_prctl(int option, unsigned long arg2,
+				  unsigned long arg3, unsigned long arg4,
+				  unsigned long arg5)
+{
+	return -ENOSYS;
+}
+#endif /* CONFIG_SECURITY_YAMA */
+
 #endif /* ! __LINUX_SECURITY_H */
 
diff --git a/security/Kconfig b/security/Kconfig
index e9c6ac7..bde1b31 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -131,7 +131,6 @@ choice
 	default DEFAULT_SECURITY_SMACK if SECURITY_SMACK
 	default DEFAULT_SECURITY_TOMOYO if SECURITY_TOMOYO
 	default DEFAULT_SECURITY_APPARMOR if SECURITY_APPARMOR
-	default DEFAULT_SECURITY_YAMA if SECURITY_YAMA
 	default DEFAULT_SECURITY_DAC
 
 	help
@@ -150,9 +149,6 @@ choice
 	config DEFAULT_SECURITY_APPARMOR
 		bool "AppArmor" if SECURITY_APPARMOR=y
 
-	config DEFAULT_SECURITY_YAMA
-		bool "Yama" if SECURITY_YAMA=y
-
 	config DEFAULT_SECURITY_DAC
 		bool "Unix Discretionary Access Controls"
 
@@ -164,7 +160,6 @@ config DEFAULT_SECURITY
 	default "smack" if DEFAULT_SECURITY_SMACK
 	default "tomoyo" if DEFAULT_SECURITY_TOMOYO
 	default "apparmor" if DEFAULT_SECURITY_APPARMOR
-	default "yama" if DEFAULT_SECURITY_YAMA
 	default "" if DEFAULT_SECURITY_DAC
 
 endmenu
diff --git a/security/security.c b/security/security.c
index 860aeb3..92b723a 100644
--- a/security/security.c
+++ b/security/security.c
@@ -136,11 +136,19 @@ int __init register_security(struct security_operations *ops)
 
 int security_ptrace_access_check(struct task_struct *child, unsigned int mode)
 {
+	int rc;
+	rc = yama_ptrace_access_check(child, mode);
+	if (rc)
+		return rc;
 	return security_ops->ptrace_access_check(child, mode);
 }
 
 int security_ptrace_traceme(struct task_struct *parent)
 {
+	int rc;
+	rc = yama_ptrace_traceme(parent);
+	if (rc)
+		return rc;
 	return security_ops->ptrace_traceme(parent);
 }
 
@@ -761,6 +769,7 @@ int security_task_create(unsigned long clone_flags)
 
 void security_task_free(struct task_struct *task)
 {
+	yama_task_free(task);
 	security_ops->task_free(task);
 }
 
@@ -876,6 +885,10 @@ int security_task_wait(struct task_struct *p)
 int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
 			 unsigned long arg4, unsigned long arg5)
 {
+	int rc;
+	rc = yama_task_prctl(option, arg2, arg3, arg4, arg5);
+	if (rc != -ENOSYS)
+		return rc;
 	return security_ops->task_prctl(option, arg2, arg3, arg4, arg5);
 }
 
diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
index dcd6178..aef5c6a 100644
--- a/security/yama/yama_lsm.c
+++ b/security/yama/yama_lsm.c
@@ -100,7 +100,7 @@ static void yama_ptracer_del(struct task_struct *tracer,
  * yama_task_free - check for task_pid to remove from exception list
  * @task: task being removed
  */
-static void yama_task_free(struct task_struct *task)
+void yama_task_free(struct task_struct *task)
 {
 	yama_ptracer_del(task, task);
 }
@@ -116,7 +116,7 @@ static void yama_task_free(struct task_struct *task)
  * Return 0 on success, -ve on error.  -ENOSYS is returned when Yama
  * does not handle the given option.
  */
-static int yama_task_prctl(int option, unsigned long arg2, unsigned long arg3,
+int yama_task_prctl(int option, unsigned long arg2, unsigned long arg3,
 			   unsigned long arg4, unsigned long arg5)
 {
 	int rc;
@@ -243,7 +243,7 @@ static int ptracer_exception_found(struct task_struct *tracer,
  *
  * Returns 0 if following the ptrace is allowed, -ve on error.
  */
-static int yama_ptrace_access_check(struct task_struct *child,
+int yama_ptrace_access_check(struct task_struct *child,
 				    unsigned int mode)
 {
 	int rc;
@@ -293,7 +293,7 @@ static int yama_ptrace_access_check(struct task_struct *child,
  *
  * Returns 0 if following the ptrace is allowed, -ve on error.
  */
-static int yama_ptrace_traceme(struct task_struct *parent)
+int yama_ptrace_traceme(struct task_struct *parent)
 {
 	int rc;
 
@@ -378,14 +378,8 @@ static struct ctl_table yama_sysctl_table[] = {
 
 static __init int yama_init(void)
 {
-	if (!security_module_enable(&yama_ops))
-		return 0;
-
 	printk(KERN_INFO "Yama: becoming mindful.\n");
 
-	if (register_security(&yama_ops))
-		panic("Yama: kernel registration failed.\n");
-
 #ifdef CONFIG_SYSCTL
 	if (!register_sysctl_paths(yama_sysctl_path, yama_sysctl_table))
 		panic("Yama: sysctl registration failed.\n");
-- 
1.7.0.4


-- 
Kees Cook
Chrome OS Security

Re: [PATCH] security: unconditionally call Yama

[Alan Cox]

On Fri, 31 Aug 2012 14:31:26 -0700
Kees Cook <keescook@chromium.org> wrote:

> Unconditionally call Yama, no matter what LSM module is selected.
> 
> Ubuntu and Chrome OS already carry patches to do this, and Fedora has
> voiced interest in doing this as well. Instead of having everyone carry
> these patches, just switch Yama to being unconditional when compiled
> into the kernel.

Not a good plan. What happens when everyone decides to stack every module
by ifdeffing in the kernel - mayhem. I'm all for it being possible but
done right - espeicall as I believe a proper proposal now for stacking
modules, and it should be done as part of that.

Alan

Re: [PATCH] security: unconditionally call Yama

[Eric Paris]

On Fri, Aug 31, 2012 at 2:39 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Fri, 31 Aug 2012 14:31:26 -0700
> Kees Cook <keescook@chromium.org> wrote:
>
>> Unconditionally call Yama, no matter what LSM module is selected.

> Not a good plan. What happens when everyone decides to stack every module
> by ifdeffing in the kernel - mayhem. I'm all for it being possible but
> done right - espeicall as I believe a proper proposal now for stacking
> modules, and it should be done as part of that.

I think a lot of us agree it's a 'difficult' plan going forward.  Kees
wrote this patch after we (James, SELinux, AppArmor people) talked at
the Security Summit earlier today.  From my PoV we have a couple of
'classes' of LSMs.

Big with Labels: SELinux and SMACK
Big without Labels: AppArmor and TOMOYO
Small and stateless: Capabilities and Yama (and maybe integrity)

Those small and stateless LSMs can easily stack.  We don't have object
lifetime issues.  We don't have difficult technical details.  We all
here seem to want to stack 'small and stateless' with 'big boy'.
Outside one little capabilities and selinux setxattr issue I can't
think of anything even quirky.  So the fast path forward, in my mind,
is to start here with Yama.  Our choice *today* is adding these static
hooks inside the 'big boy' LSMs (which I think all of us are willing
to do) vs adding it one time in the LSM and not having to worry about
it all over the place.  Getting the big guys and the little guys to
play together is not going to lead to a mess of crazy.

Stacking the big boys is a future goal most of us are happy with
seeing done, if it turns out to be reasonable.  We know it's close to
possible.  Dave Howell's gave us an implementation about 2 years ago.
Casey Schaufler demo'd another stacking interface today.  No code from
the latter, but the only limitation he really mentioned today was that
two big guys with labels can't stack together.  I don't know how/if
Dave's implementation wanted to handle that case.

I really think this patch is good.

Acked-by: Eric Paris <eparis@redhat.com>

I think I even want to do the same thing with capabilities so I don't
have to make sure I'm getting those right in SELinux.  And everyone
else probably doesn't want to keep those hooks themselves either.  I
should send that patch to Linus.  I bet I can give him a large
negative diff.  He did just say two days ago that he'll take any -1000
line diff after -rc6   :)

I think Casey and Dave need to both get their larger stacking
solutions posted and we should go with the best one.  It'll let us
pull out the static code, but for now, I like the static coding
between the big boys and the little guys.  Lets fix today what's easy
and fix tomorrow what we can't fix today.

-Eric

Re: [PATCH] security: unconditionally call Yama

[Eric W. Biederman]

Eric Paris <eparis@parisplace.org> writes:

> On Fri, Aug 31, 2012 at 2:39 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> On Fri, 31 Aug 2012 14:31:26 -0700
>> Kees Cook <keescook@chromium.org> wrote:
>>
>>> Unconditionally call Yama, no matter what LSM module is selected.
>
>> Not a good plan. What happens when everyone decides to stack every module
>> by ifdeffing in the kernel - mayhem. I'm all for it being possible but
>> done right - espeicall as I believe a proper proposal now for stacking
>> modules, and it should be done as part of that.
>
> I think a lot of us agree it's a 'difficult' plan going forward.  Kees
> wrote this patch after we (James, SELinux, AppArmor people) talked at
> the Security Summit earlier today.  From my PoV we have a couple of
> 'classes' of LSMs.
>
> Big with Labels: SELinux and SMACK
> Big without Labels: AppArmor and TOMOYO
> Small and stateless: Capabilities and Yama (and maybe integrity)
>
> Those small and stateless LSMs can easily stack.  We don't have object
> lifetime issues.  We don't have difficult technical details.  We all
> here seem to want to stack 'small and stateless' with 'big boy'.
> Outside one little capabilities and selinux setxattr issue I can't
> think of anything even quirky.  So the fast path forward, in my mind,
> is to start here with Yama.  Our choice *today* is adding these static
> hooks inside the 'big boy' LSMs (which I think all of us are willing
> to do) vs adding it one time in the LSM and not having to worry about
> it all over the place.  Getting the big guys and the little guys to
> play together is not going to lead to a mess of crazy.
>
> Stacking the big boys is a future goal most of us are happy with
> seeing done, if it turns out to be reasonable.  We know it's close to
> possible.  Dave Howell's gave us an implementation about 2 years ago.
> Casey Schaufler demo'd another stacking interface today.  No code from
> the latter, but the only limitation he really mentioned today was that
> two big guys with labels can't stack together.  I don't know how/if
> Dave's implementation wanted to handle that case.
>
> I really think this patch is good.
>
> Acked-by: Eric Paris <eparis@redhat.com>
>
> I think I even want to do the same thing with capabilities so I don't
> have to make sure I'm getting those right in SELinux.  And everyone
> else probably doesn't want to keep those hooks themselves either.  I
> should send that patch to Linus.  I bet I can give him a large
> negative diff.  He did just say two days ago that he'll take any -1000
> line diff after -rc6   :)
>
> I think Casey and Dave need to both get their larger stacking
> solutions posted and we should go with the best one.  It'll let us
> pull out the static code, but for now, I like the static coding
> between the big boys and the little guys.  Lets fix today what's easy
> and fix tomorrow what we can't fix today.

>From a overal kernel maintenance and use perspective the unconditional
enablement is a pain.

We long ago established the principle that compiling additional code
into the kernel should not change the semenatics of the kernel.

So this code needs to come with a command line or sysctl on/off switch
not an unconditional enable.

Eric

Re: [PATCH] security: unconditionally call Yama

[Eric Paris]

On Fri, Aug 31, 2012 at 4:59 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:

> From a overal kernel maintenance and use perspective the unconditional
> enablement is a pain.
>
> We long ago established the principle that compiling additional code
> into the kernel should not change the semenatics of the kernel.
>
> So this code needs to come with a command line or sysctl on/off switch
> not an unconditional enable.

Your argument makes zero sense.  If I decide to build new code, that
new code can do something.  It happens all the time.  If you don't
like Yama, don't build Yama.  If you don't like the only thing that
Yama does (it only implements one protection), disable that protection
from sysctl.  I don't get it.

-Eric

Re: [PATCH] security: unconditionally call Yama

[Eric W. Biederman]

Eric Paris <eparis@parisplace.org> writes:

> On Fri, Aug 31, 2012 at 4:59 PM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>
>> From a overal kernel maintenance and use perspective the unconditional
>> enablement is a pain.
>>
>> We long ago established the principle that compiling additional code
>> into the kernel should not change the semenatics of the kernel.
>>
>> So this code needs to come with a command line or sysctl on/off switch
>> not an unconditional enable.
>
> Your argument makes zero sense.  If I decide to build new code, that
> new code can do something.

Sure but it should not change the existing behavior without being
configured to.

This comes out of the practice that kernels that need to support a
wide variety of use cases enable everything by default.

Having to vet kernel options for will this make my kernel do strange
things if this option is enabled, massively increase the burden on
people building and supporting kernels.

> It happens all the time.  If you don't like Yama, don't build Yama.
> If you don't like the only thing that Yama does (it only implements
> one protection), disable that protection from sysctl.  I don't get it.

Having taken the time now to vet Yama ugh.  Having Yama enabled if
simply compiled in breaks using gdb to attach to a process runing
in another window.

Talk about something you don't want to surprise someone with.

It is very much not ok to have that be enabled by default just
because it happens to be compiled in.

Eric

Re: [PATCH] security: unconditionally call Yama

[Kees Cook]

On Fri, Aug 31, 2012 at 8:31 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Eric Paris <eparis@parisplace.org> writes:
>
>> On Fri, Aug 31, 2012 at 4:59 PM, Eric W. Biederman
>> <ebiederm@xmission.com> wrote:
>>
>>> From a overal kernel maintenance and use perspective the unconditional
>>> enablement is a pain.
>>>
>>> We long ago established the principle that compiling additional code
>>> into the kernel should not change the semenatics of the kernel.
>>>
>>> So this code needs to come with a command line or sysctl on/off switch
>>> not an unconditional enable.
>>
>> Your argument makes zero sense.  If I decide to build new code, that
>> new code can do something.
>
> Sure but it should not change the existing behavior without being
> configured to.
>
> This comes out of the practice that kernels that need to support a
> wide variety of use cases enable everything by default.
>
> Having to vet kernel options for will this make my kernel do strange
> things if this option is enabled, massively increase the burden on
> people building and supporting kernels.
>
>> It happens all the time.  If you don't like Yama, don't build Yama.
>> If you don't like the only thing that Yama does (it only implements
>> one protection), disable that protection from sysctl.  I don't get it.
>
> Having taken the time now to vet Yama ugh.  Having Yama enabled if
> simply compiled in breaks using gdb to attach to a process runing
> in another window.
>
> Talk about something you don't want to surprise someone with.
>
> It is very much not ok to have that be enabled by default just
> because it happens to be compiled in.

I think it is better to look at the kernel's defaults from the
perspective of the user, not the developer. If we only looked to the
developer, we'd turn on all the debugging by default. No end user
wants that. It's much easier for a developer to twiddle configs and
sysctls.

Given that several distros use (or want to use) Yama, I think that's
reason enough for this. I think it's important for us to take a
practical approach here, and having the big LSMs each hook Yama
instead of doing this in a single global place will make it needlessly
duplicated code.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH] security: unconditionally call Yama

[Eric W. Biederman]

Kees Cook <keescook@chromium.org> writes:

> On Fri, Aug 31, 2012 at 8:31 PM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>> Eric Paris <eparis@parisplace.org> writes:
>>
>>> On Fri, Aug 31, 2012 at 4:59 PM, Eric W. Biederman
>>> <ebiederm@xmission.com> wrote:
>>>
>> Having taken the time now to vet Yama ugh.  Having Yama enabled if
>> simply compiled in breaks using gdb to attach to a process runing
>> in another window.
>>
>> Talk about something you don't want to surprise someone with.
>>
>> It is very much not ok to have that be enabled by default just
>> because it happens to be compiled in.
>
> I think it is better to look at the kernel's defaults from the
> perspective of the user, not the developer.

It is best to look at the kernel defaults with respect to regressions
from previous behavior.  And YAMA uncondionally enabled and restricting
ptrace when compiled simply in, is most definitely a regression from
previous behavior.  And it is most definitely is not a behavior no
one cares about.

> If we only looked to the
> developer, we'd turn on all the debugging by default. No end user
> wants that. It's much easier for a developer to twiddle configs and
> sysctls.

If you want to change the default behaior of the kernel provide a
config option that is specifically about changing the behavior.

But changing the behavior by default and compiling in the code
are two very very different things, and they should continue to be.

> Given that several distros use (or want to use) Yama, I think that's
> reason enough for this. I think it's important for us to take a
> practical approach here, and having the big LSMs each hook Yama
> instead of doing this in a single global place will make it needlessly
> duplicated code.

People wanting to use a feature is certainly an argument for figuring
out how to do it well.  It most definitely is not a reason for randomly
breaking long established kernel features when they happen to run
make oldconfig and telling other people to just deal with it.

Eric

Re: [PATCH] security: unconditionally call Yama

[Jiri Kosina]

On Fri, 31 Aug 2012, Kees Cook wrote:

> Given that several distros use (or want to use) Yama, I think that's 
> reason enough for this. I think it's important for us to take a 
> practical approach here, and having the big LSMs each hook Yama instead 
> of doing this in a single global place will make it needlessly 
> duplicated code.

With my SUSE hat on, I would be really reluctant to enable config option 
in the distribution kernel that will make attaching gdb to running 
processess impossible without any further tuning (like sysctl/procfs knob 
to explicitly enable this feature).

Thanks,

-- 
Jiri Kosina
SUSE Labs