[PATCH] coredump: run the coredump helper using the same namespace as the dead process

[PATCH] coredump: run the coredump helper using the same namespace as the dead process

[Aristeu Rozanski]

/proc/sys/kernel/core_pattern can be used to specify a userspace helper
to handle core files and it currently runs in the root namespace.
This patch allows the helper to run in the same namespace in a step
towards letting containers setting their own helpers.

Cc: "Serge E. Hallyn" <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org>
Cc: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
Cc: Al Viro <viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn@public.gmane.org>
Signed-off-by: Aristeu Rozanski <aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

diff --git a/fs/coredump.c b/fs/coredump.c
index ce47379..fa14ea1 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -455,6 +455,19 @@ static int umh_pipe_setup(struct subprocess_info *info, struct cred *new)
 	/* and disallow core files too */
 	current->signal->rlim[RLIMIT_CORE] = (struct rlimit){1, 1};
 
+	/*
+	 * We want to run the helper within the same namespace. Since we
+	 * already forked, current here is using init_nsproxy and the usage
+	 * was already accounted. switch_task_namespace() will revert that
+	 * but we need to bump the dead process' nsproxy before calling the
+	 * the helper. Once it exits, the dead process' nsproxy usage will be
+	 * decremented as part of normal process exit.
+	 */
+	if (current->nsproxy != cp->nsproxy) {
+		get_nsproxy(cp->nsproxy);
+		switch_task_namespaces(current, cp->nsproxy);
+	}
+
 	return err;
 }
 
@@ -482,6 +495,8 @@ void do_coredump(siginfo_t *siginfo, struct pt_regs *regs)
 		 * by any locks.
 		 */
 		.mm_flags = mm->flags,
+		/* we run the helper in the same namespace */
+		.nsproxy = current->nsproxy,
 	};
 
 	audit_core_dumps(siginfo->si_signo);
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index cfcc6bf..45113e6 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -63,6 +63,7 @@ struct coredump_params {
 	struct file *file;
 	unsigned long limit;
 	unsigned long mm_flags;
+	struct nsproxy *nsproxy;
 };
 
 /*

[PATCH] coredump: run the coredump helper using the same namespace as the dead process

[Aristeu Rozanski]

/proc/sys/kernel/core_pattern can be used to specify a userspace helper
to handle core files and it currently runs in the root namespace.
This patch allows the helper to run in the same namespace in a step
towards letting containers setting their own helpers.

Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Aristeu Rozanski <aris@redhat.com>

diff --git a/fs/coredump.c b/fs/coredump.c
index ce47379..fa14ea1 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -455,6 +455,19 @@ static int umh_pipe_setup(struct subprocess_info *info, struct cred *new)
 	/* and disallow core files too */
 	current->signal->rlim[RLIMIT_CORE] = (struct rlimit){1, 1};
 
+	/*
+	 * We want to run the helper within the same namespace. Since we
+	 * already forked, current here is using init_nsproxy and the usage
+	 * was already accounted. switch_task_namespace() will revert that
+	 * but we need to bump the dead process' nsproxy before calling the
+	 * the helper. Once it exits, the dead process' nsproxy usage will be
+	 * decremented as part of normal process exit.
+	 */
+	if (current->nsproxy != cp->nsproxy) {
+		get_nsproxy(cp->nsproxy);
+		switch_task_namespaces(current, cp->nsproxy);
+	}
+
 	return err;
 }
 
@@ -482,6 +495,8 @@ void do_coredump(siginfo_t *siginfo, struct pt_regs *regs)
 		 * by any locks.
 		 */
 		.mm_flags = mm->flags,
+		/* we run the helper in the same namespace */
+		.nsproxy = current->nsproxy,
 	};
 
 	audit_core_dumps(siginfo->si_signo);
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index cfcc6bf..45113e6 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -63,6 +63,7 @@ struct coredump_params {
 	struct file *file;
 	unsigned long limit;
 	unsigned long mm_flags;
+	struct nsproxy *nsproxy;
 };
 
 /*

Re: [PATCH] coredump: run the coredump helper using the same namespace as the dead process

[Eric W. Biederman]

Aristeu Rozanski <aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:

> /proc/sys/kernel/core_pattern can be used to specify a userspace helper
> to handle core files and it currently runs in the root namespace.
> This patch allows the helper to run in the same namespace in a step
> towards letting containers setting their own helpers.

I would argue that you very much need to define what it means to have a
per container core dump at the same time as you argue this.

Nacked-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>

Running in a namespace different than whoever set the core dump
pattern/helper makes core dump helpers much more attackable.  With this
patch and a little creativity I expect I can get root to write to
whatever file I would like.  Since I also control the content of what is
going into that file.... This design seems emintely exploitable.

Furthermore not all namespaces are pointed at by nsproxy, so even
for it's original design this patch is buggy.


I do think supporting a per container coredump setting makes a lot of
sense but I do not think this patch is the way to do it.

Eric


> Cc: "Serge E. Hallyn" <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org>
> Cc: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
> Cc: Al Viro <viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn@public.gmane.org>
> Signed-off-by: Aristeu Rozanski <aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>
> diff --git a/fs/coredump.c b/fs/coredump.c
> index ce47379..fa14ea1 100644
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -455,6 +455,19 @@ static int umh_pipe_setup(struct subprocess_info *info, struct cred *new)
>  	/* and disallow core files too */
>  	current->signal->rlim[RLIMIT_CORE] = (struct rlimit){1, 1};
>  
> +	/*
> +	 * We want to run the helper within the same namespace. Since we
> +	 * already forked, current here is using init_nsproxy and the usage
> +	 * was already accounted. switch_task_namespace() will revert that
> +	 * but we need to bump the dead process' nsproxy before calling the
> +	 * the helper. Once it exits, the dead process' nsproxy usage will be
> +	 * decremented as part of normal process exit.
> +	 */
> +	if (current->nsproxy != cp->nsproxy) {
> +		get_nsproxy(cp->nsproxy);
> +		switch_task_namespaces(current, cp->nsproxy);
> +	}
> +
>  	return err;
>  }
>  
> @@ -482,6 +495,8 @@ void do_coredump(siginfo_t *siginfo, struct pt_regs *regs)
>  		 * by any locks.
>  		 */
>  		.mm_flags = mm->flags,
> +		/* we run the helper in the same namespace */
> +		.nsproxy = current->nsproxy,
>  	};
>  
>  	audit_core_dumps(siginfo->si_signo);
> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> index cfcc6bf..45113e6 100644
> --- a/include/linux/binfmts.h
> +++ b/include/linux/binfmts.h
> @@ -63,6 +63,7 @@ struct coredump_params {
>  	struct file *file;
>  	unsigned long limit;
>  	unsigned long mm_flags;
> +	struct nsproxy *nsproxy;
>  };
>  
>  /*

Re: [PATCH] coredump: run the coredump helper using the same namespace as the dead process

[Eric W. Biederman]

Aristeu Rozanski <aris@redhat.com> writes:

> /proc/sys/kernel/core_pattern can be used to specify a userspace helper
> to handle core files and it currently runs in the root namespace.
> This patch allows the helper to run in the same namespace in a step
> towards letting containers setting their own helpers.

I would argue that you very much need to define what it means to have a
per container core dump at the same time as you argue this.

Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>

Running in a namespace different than whoever set the core dump
pattern/helper makes core dump helpers much more attackable.  With this
patch and a little creativity I expect I can get root to write to
whatever file I would like.  Since I also control the content of what is
going into that file.... This design seems emintely exploitable.

Furthermore not all namespaces are pointed at by nsproxy, so even
for it's original design this patch is buggy.


I do think supporting a per container coredump setting makes a lot of
sense but I do not think this patch is the way to do it.

Eric


> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Aristeu Rozanski <aris@redhat.com>
>
> diff --git a/fs/coredump.c b/fs/coredump.c
> index ce47379..fa14ea1 100644
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -455,6 +455,19 @@ static int umh_pipe_setup(struct subprocess_info *info, struct cred *new)
>  	/* and disallow core files too */
>  	current->signal->rlim[RLIMIT_CORE] = (struct rlimit){1, 1};
>  
> +	/*
> +	 * We want to run the helper within the same namespace. Since we
> +	 * already forked, current here is using init_nsproxy and the usage
> +	 * was already accounted. switch_task_namespace() will revert that
> +	 * but we need to bump the dead process' nsproxy before calling the
> +	 * the helper. Once it exits, the dead process' nsproxy usage will be
> +	 * decremented as part of normal process exit.
> +	 */
> +	if (current->nsproxy != cp->nsproxy) {
> +		get_nsproxy(cp->nsproxy);
> +		switch_task_namespaces(current, cp->nsproxy);
> +	}
> +
>  	return err;
>  }
>  
> @@ -482,6 +495,8 @@ void do_coredump(siginfo_t *siginfo, struct pt_regs *regs)
>  		 * by any locks.
>  		 */
>  		.mm_flags = mm->flags,
> +		/* we run the helper in the same namespace */
> +		.nsproxy = current->nsproxy,
>  	};
>  
>  	audit_core_dumps(siginfo->si_signo);
> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> index cfcc6bf..45113e6 100644
> --- a/include/linux/binfmts.h
> +++ b/include/linux/binfmts.h
> @@ -63,6 +63,7 @@ struct coredump_params {
>  	struct file *file;
>  	unsigned long limit;
>  	unsigned long mm_flags;
> +	struct nsproxy *nsproxy;
>  };
>  
>  /*

Re: [PATCH] coredump: run the coredump helper using the same namespace as the dead process

[Aristeu Rozanski]

On Mon, Nov 05, 2012 at 11:34:26AM -0800, Eric W. Biederman wrote:
> I would argue that you very much need to define what it means to have a
> per container core dump at the same time as you argue this.
> 
> Nacked-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
> 
> Running in a namespace different than whoever set the core dump
> pattern/helper makes core dump helpers much more attackable.  With this
> patch and a little creativity I expect I can get root to write to
> whatever file I would like.  Since I also control the content of what is
> going into that file.... This design seems emintely exploitable.

Understood. Indeed this is bad design. Having it tied to the mount
namespace of the process setting the pattern/helper, therefore any
process crashing under the same mount namespace would use the same
pattern/helper? 

> Furthermore not all namespaces are pointed at by nsproxy, so even
> for it's original design this patch is buggy.

is it userns? I just assumed it wasn't there yet because it's being
worked on.

> I do think supporting a per container coredump setting makes a lot of
> sense but I do not think this patch is the way to do it.

I understand, thanks for the time reviewing it.


-- 
Aristeu

Re: [PATCH] coredump: run the coredump helper using the same namespace as the dead process

[Aristeu Rozanski]

On Mon, Nov 05, 2012 at 11:34:26AM -0800, Eric W. Biederman wrote:
> I would argue that you very much need to define what it means to have a
> per container core dump at the same time as you argue this.
> 
> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> 
> Running in a namespace different than whoever set the core dump
> pattern/helper makes core dump helpers much more attackable.  With this
> patch and a little creativity I expect I can get root to write to
> whatever file I would like.  Since I also control the content of what is
> going into that file.... This design seems emintely exploitable.

Understood. Indeed this is bad design. Having it tied to the mount
namespace of the process setting the pattern/helper, therefore any
process crashing under the same mount namespace would use the same
pattern/helper? 

> Furthermore not all namespaces are pointed at by nsproxy, so even
> for it's original design this patch is buggy.

is it userns? I just assumed it wasn't there yet because it's being
worked on.

> I do think supporting a per container coredump setting makes a lot of
> sense but I do not think this patch is the way to do it.

I understand, thanks for the time reviewing it.


-- 
Aristeu

Re: [PATCH] coredump: run the coredump helper using the same namespace as the dead process

[Eric W. Biederman]

Aristeu Rozanski <aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:

> On Mon, Nov 05, 2012 at 11:34:26AM -0800, Eric W. Biederman wrote:
>> I would argue that you very much need to define what it means to have a
>> per container core dump at the same time as you argue this.
>> 
>> Nacked-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
>> 
>> Running in a namespace different than whoever set the core dump
>> pattern/helper makes core dump helpers much more attackable.  With this
>> patch and a little creativity I expect I can get root to write to
>> whatever file I would like.  Since I also control the content of what is
>> going into that file.... This design seems emintely exploitable.
>
> Understood. Indeed this is bad design. Having it tied to the mount
> namespace of the process setting the pattern/helper, therefore any
> process crashing under the same mount namespace would use the same
> pattern/helper? 

Other than knowing we need an intuitive and predictable set of
namespaces that will be for the core dumping application I don't
know what the proper design is at this time.

>> Furthermore not all namespaces are pointed at by nsproxy, so even
>> for it's original design this patch is buggy.
>
> is it userns? I just assumed it wasn't there yet because it's being
> worked on.

userns is certainly one case.  The user namespace has no reason to
appear in nsproxy and plenty of good reasons connected with not
duplicating data for not appearing in nsproxy.

>> I do think supporting a per container coredump setting makes a lot of
>> sense but I do not think this patch is the way to do it.
>
> I understand, thanks for the time reviewing it.

Eric

Re: [PATCH] coredump: run the coredump helper using the same namespace as the dead process

[Eric W. Biederman]

Aristeu Rozanski <aris@redhat.com> writes:

> On Mon, Nov 05, 2012 at 11:34:26AM -0800, Eric W. Biederman wrote:
>> I would argue that you very much need to define what it means to have a
>> per container core dump at the same time as you argue this.
>> 
>> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> 
>> Running in a namespace different than whoever set the core dump
>> pattern/helper makes core dump helpers much more attackable.  With this
>> patch and a little creativity I expect I can get root to write to
>> whatever file I would like.  Since I also control the content of what is
>> going into that file.... This design seems emintely exploitable.
>
> Understood. Indeed this is bad design. Having it tied to the mount
> namespace of the process setting the pattern/helper, therefore any
> process crashing under the same mount namespace would use the same
> pattern/helper? 

Other than knowing we need an intuitive and predictable set of
namespaces that will be for the core dumping application I don't
know what the proper design is at this time.

>> Furthermore not all namespaces are pointed at by nsproxy, so even
>> for it's original design this patch is buggy.
>
> is it userns? I just assumed it wasn't there yet because it's being
> worked on.

userns is certainly one case.  The user namespace has no reason to
appear in nsproxy and plenty of good reasons connected with not
duplicating data for not appearing in nsproxy.

>> I do think supporting a per container coredump setting makes a lot of
>> sense but I do not think this patch is the way to do it.
>
> I understand, thanks for the time reviewing it.

Eric