[PATCH] proc: prevent a task from writing on its own /proc/*/mem

[PATCH] proc: prevent a task from writing on its own /proc/*/mem

[Salvatore Mesoraca]

Prevent a task from opening, in "write" mode, any /proc/*/mem
file that operates on the task's mm.
/proc/*/mem is mainly a debugging means and, as such, it shouldn't
be used by the inspected process itself.
Current implementation always allow a task to access its own
/proc/*/mem file.
A process can use it to overwrite read-only memory, making
pointless the use of security_file_mprotect() or other ways to
enforce RO memory.

Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com>
---
 fs/proc/base.c       | 25 ++++++++++++++++++-------
 fs/proc/internal.h   |  3 ++-
 fs/proc/task_mmu.c   |  4 ++--
 fs/proc/task_nommu.c |  2 +-
 4 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 1a76d75..01ecfec 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -762,8 +762,9 @@ static int proc_single_open(struct inode *inode, struct file *filp)
 	.release	= single_release,
 };
 
-
-struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
+struct mm_struct *proc_mem_open(struct inode *inode,
+				unsigned int mode,
+				fmode_t f_mode)
 {
 	struct task_struct *task = get_proc_task(inode);
 	struct mm_struct *mm = ERR_PTR(-ESRCH);
@@ -773,10 +774,20 @@ struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
 		put_task_struct(task);
 
 		if (!IS_ERR_OR_NULL(mm)) {
-			/* ensure this mm_struct can't be freed */
-			mmgrab(mm);
-			/* but do not pin its memory */
-			mmput(mm);
+			/*
+			 * Prevent this interface from being used as a mean
+			 * to bypass memory restrictions, including those
+			 * imposed by LSMs.
+			 */
+			if (mm == current->mm &&
+			    f_mode & FMODE_WRITE)
+				mm = ERR_PTR(-EACCES);
+			else {
+				/* ensure this mm_struct can't be freed */
+				mmgrab(mm);
+				/* but do not pin its memory */
+				mmput(mm);
+			}
 		}
 	}
 
@@ -785,7 +796,7 @@ struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
 
 static int __mem_open(struct inode *inode, struct file *file, unsigned int mode)
 {
-	struct mm_struct *mm = proc_mem_open(inode, mode);
+	struct mm_struct *mm = proc_mem_open(inode, mode, file->f_mode);
 
 	if (IS_ERR(mm))
 		return PTR_ERR(mm);
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index 0f1692e..8d38cc7 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -275,7 +275,8 @@ struct proc_maps_private {
 #endif
 } __randomize_layout;
 
-struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode);
+struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode,
+				fmode_t f_mode);
 
 extern const struct file_operations proc_pid_maps_operations;
 extern const struct file_operations proc_tid_maps_operations;
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index c486ad4..efb6535 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -227,7 +227,7 @@ static int proc_maps_open(struct inode *inode, struct file *file,
 		return -ENOMEM;
 
 	priv->inode = inode;
-	priv->mm = proc_mem_open(inode, PTRACE_MODE_READ);
+	priv->mm = proc_mem_open(inode, PTRACE_MODE_READ, file->f_mode);
 	if (IS_ERR(priv->mm)) {
 		int err = PTR_ERR(priv->mm);
 
@@ -1534,7 +1534,7 @@ static int pagemap_open(struct inode *inode, struct file *file)
 {
 	struct mm_struct *mm;
 
-	mm = proc_mem_open(inode, PTRACE_MODE_READ);
+	mm = proc_mem_open(inode, PTRACE_MODE_READ, file->f_mode);
 	if (IS_ERR(mm))
 		return PTR_ERR(mm);
 	file->private_data = mm;
diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
index 5b62f57..dc38516 100644
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -280,7 +280,7 @@ static int maps_open(struct inode *inode, struct file *file,
 		return -ENOMEM;
 
 	priv->inode = inode;
-	priv->mm = proc_mem_open(inode, PTRACE_MODE_READ);
+	priv->mm = proc_mem_open(inode, PTRACE_MODE_READ, file->f_mode);
 	if (IS_ERR(priv->mm)) {
 		int err = PTR_ERR(priv->mm);
 
-- 
1.9.1

[PATCH] proc: prevent a task from writing on its own /proc/*/mem

[Salvatore Mesoraca]

Prevent a task from opening, in "write" mode, any /proc/*/mem
file that operates on the task's mm.
/proc/*/mem is mainly a debugging means and, as such, it shouldn't
be used by the inspected process itself.
Current implementation always allow a task to access its own
/proc/*/mem file.
A process can use it to overwrite read-only memory, making
pointless the use of security_file_mprotect() or other ways to
enforce RO memory.

Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com>
---
 fs/proc/base.c       | 25 ++++++++++++++++++-------
 fs/proc/internal.h   |  3 ++-
 fs/proc/task_mmu.c   |  4 ++--
 fs/proc/task_nommu.c |  2 +-
 4 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 1a76d75..01ecfec 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -762,8 +762,9 @@ static int proc_single_open(struct inode *inode, struct file *filp)
 	.release	= single_release,
 };
 
-
-struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
+struct mm_struct *proc_mem_open(struct inode *inode,
+				unsigned int mode,
+				fmode_t f_mode)
 {
 	struct task_struct *task = get_proc_task(inode);
 	struct mm_struct *mm = ERR_PTR(-ESRCH);
@@ -773,10 +774,20 @@ struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
 		put_task_struct(task);
 
 		if (!IS_ERR_OR_NULL(mm)) {
-			/* ensure this mm_struct can't be freed */
-			mmgrab(mm);
-			/* but do not pin its memory */
-			mmput(mm);
+			/*
+			 * Prevent this interface from being used as a mean
+			 * to bypass memory restrictions, including those
+			 * imposed by LSMs.
+			 */
+			if (mm == current->mm &&
+			    f_mode & FMODE_WRITE)
+				mm = ERR_PTR(-EACCES);
+			else {
+				/* ensure this mm_struct can't be freed */
+				mmgrab(mm);
+				/* but do not pin its memory */
+				mmput(mm);
+			}
 		}
 	}
 
@@ -785,7 +796,7 @@ struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
 
 static int __mem_open(struct inode *inode, struct file *file, unsigned int mode)
 {
-	struct mm_struct *mm = proc_mem_open(inode, mode);
+	struct mm_struct *mm = proc_mem_open(inode, mode, file->f_mode);
 
 	if (IS_ERR(mm))
 		return PTR_ERR(mm);
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index 0f1692e..8d38cc7 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -275,7 +275,8 @@ struct proc_maps_private {
 #endif
 } __randomize_layout;
 
-struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode);
+struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode,
+				fmode_t f_mode);
 
 extern const struct file_operations proc_pid_maps_operations;
 extern const struct file_operations proc_tid_maps_operations;
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index c486ad4..efb6535 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -227,7 +227,7 @@ static int proc_maps_open(struct inode *inode, struct file *file,
 		return -ENOMEM;
 
 	priv->inode = inode;
-	priv->mm = proc_mem_open(inode, PTRACE_MODE_READ);
+	priv->mm = proc_mem_open(inode, PTRACE_MODE_READ, file->f_mode);
 	if (IS_ERR(priv->mm)) {
 		int err = PTR_ERR(priv->mm);
 
@@ -1534,7 +1534,7 @@ static int pagemap_open(struct inode *inode, struct file *file)
 {
 	struct mm_struct *mm;
 
-	mm = proc_mem_open(inode, PTRACE_MODE_READ);
+	mm = proc_mem_open(inode, PTRACE_MODE_READ, file->f_mode);
 	if (IS_ERR(mm))
 		return PTR_ERR(mm);
 	file->private_data = mm;
diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
index 5b62f57..dc38516 100644
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -280,7 +280,7 @@ static int maps_open(struct inode *inode, struct file *file,
 		return -ENOMEM;
 
 	priv->inode = inode;
-	priv->mm = proc_mem_open(inode, PTRACE_MODE_READ);
+	priv->mm = proc_mem_open(inode, PTRACE_MODE_READ, file->f_mode);
 	if (IS_ERR(priv->mm)) {
 		int err = PTR_ERR(priv->mm);
 
-- 
1.9.1

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

Re: [PATCH] proc: prevent a task from writing on its own /proc/*/mem

[Alexey Dobriyan]

On Sat, May 26, 2018 at 04:50:46PM +0200, Salvatore Mesoraca wrote:
> Prevent a task from opening, in "write" mode, any /proc/*/mem
> file that operates on the task's mm.
> /proc/*/mem is mainly a debugging means and, as such, it shouldn't
> be used by the inspected process itself.
> Current implementation always allow a task to access its own
> /proc/*/mem file.
> A process can use it to overwrite read-only memory, making
> pointless the use of security_file_mprotect() or other ways to
> enforce RO memory.

You can do it in security_ptrace_access_check() or security_file_open()

[PATCH] proc: prevent a task from writing on its own /proc/*/mem

[Alexey Dobriyan]

On Sat, May 26, 2018 at 04:50:46PM +0200, Salvatore Mesoraca wrote:
> Prevent a task from opening, in "write" mode, any /proc/*/mem
> file that operates on the task's mm.
> /proc/*/mem is mainly a debugging means and, as such, it shouldn't
> be used by the inspected process itself.
> Current implementation always allow a task to access its own
> /proc/*/mem file.
> A process can use it to overwrite read-only memory, making
> pointless the use of security_file_mprotect() or other ways to
> enforce RO memory.

You can do it in security_ptrace_access_check() or security_file_open()
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] proc: prevent a task from writing on its own /proc/*/mem

[Salvatore Mesoraca]

2018-05-26 17:48 GMT+02:00 Alexey Dobriyan <adobriyan@gmail.com>:
> On Sat, May 26, 2018 at 04:50:46PM +0200, Salvatore Mesoraca wrote:
>> Prevent a task from opening, in "write" mode, any /proc/*/mem
>> file that operates on the task's mm.
>> /proc/*/mem is mainly a debugging means and, as such, it shouldn't
>> be used by the inspected process itself.
>> Current implementation always allow a task to access its own
>> /proc/*/mem file.
>> A process can use it to overwrite read-only memory, making
>> pointless the use of security_file_mprotect() or other ways to
>> enforce RO memory.
>
> You can do it in security_ptrace_access_check()

No, because that hook is skipped when mm == current->mm:
https://elixir.bootlin.com/linux/v4.17-rc6/source/kernel/fork.c#L1111

> or security_file_open()

This is true, but it looks a bit overkill to me, especially since many of
the macros/functions used to handle proc's files won't be in scope
for an external LSM.
Is there any particular reason why you prefer it done via LSM?

Thank you,

Salvatore

[PATCH] proc: prevent a task from writing on its own /proc/*/mem

[Salvatore Mesoraca]

2018-05-26 17:48 GMT+02:00 Alexey Dobriyan <adobriyan@gmail.com>:
> On Sat, May 26, 2018 at 04:50:46PM +0200, Salvatore Mesoraca wrote:
>> Prevent a task from opening, in "write" mode, any /proc/*/mem
>> file that operates on the task's mm.
>> /proc/*/mem is mainly a debugging means and, as such, it shouldn't
>> be used by the inspected process itself.
>> Current implementation always allow a task to access its own
>> /proc/*/mem file.
>> A process can use it to overwrite read-only memory, making
>> pointless the use of security_file_mprotect() or other ways to
>> enforce RO memory.
>
> You can do it in security_ptrace_access_check()

No, because that hook is skipped when mm == current->mm:
https://elixir.bootlin.com/linux/v4.17-rc6/source/kernel/fork.c#L1111

> or security_file_open()

This is true, but it looks a bit overkill to me, especially since many of
the macros/functions used to handle proc's files won't be in scope
for an external LSM.
Is there any particular reason why you prefer it done via LSM?

Thank you,

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

Re: [PATCH] proc: prevent a task from writing on its own /proc/*/mem

[Casey Schaufler]

On 5/26/2018 10:30 AM, Salvatore Mesoraca wrote:
> 2018-05-26 17:48 GMT+02:00 Alexey Dobriyan <adobriyan@gmail.com>:
>> On Sat, May 26, 2018 at 04:50:46PM +0200, Salvatore Mesoraca wrote:
>>> Prevent a task from opening, in "write" mode, any /proc/*/mem
>>> file that operates on the task's mm.
>>> /proc/*/mem is mainly a debugging means and, as such, it shouldn't
>>> be used by the inspected process itself.
>>> Current implementation always allow a task to access its own
>>> /proc/*/mem file.
>>> A process can use it to overwrite read-only memory, making
>>> pointless the use of security_file_mprotect() or other ways to
>>> enforce RO memory.
>> You can do it in security_ptrace_access_check()
> No, because that hook is skipped when mm == current->mm:
> https://elixir.bootlin.com/linux/v4.17-rc6/source/kernel/fork.c#L1111
>
>> or security_file_open()
> This is true, but it looks a bit overkill to me, especially since many of
> the macros/functions used to handle proc's files won't be in scope
> for an external LSM.
> Is there any particular reason why you prefer it done via LSM?

If you did a Yama style LSM it would be easy to configure.
Even though it might make no sense to allow this behavior,
someone, somewhere is counting on it.

>
> Thank you,
>
> Salvatore
> --
> 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] proc: prevent a task from writing on its own /proc/*/mem

[Casey Schaufler]

On 5/26/2018 10:30 AM, Salvatore Mesoraca wrote:
> 2018-05-26 17:48 GMT+02:00 Alexey Dobriyan <adobriyan@gmail.com>:
>> On Sat, May 26, 2018 at 04:50:46PM +0200, Salvatore Mesoraca wrote:
>>> Prevent a task from opening, in "write" mode, any /proc/*/mem
>>> file that operates on the task's mm.
>>> /proc/*/mem is mainly a debugging means and, as such, it shouldn't
>>> be used by the inspected process itself.
>>> Current implementation always allow a task to access its own
>>> /proc/*/mem file.
>>> A process can use it to overwrite read-only memory, making
>>> pointless the use of security_file_mprotect() or other ways to
>>> enforce RO memory.
>> You can do it in security_ptrace_access_check()
> No, because that hook is skipped when mm == current->mm:
> https://elixir.bootlin.com/linux/v4.17-rc6/source/kernel/fork.c#L1111
>
>> or security_file_open()
> This is true, but it looks a bit overkill to me, especially since many of
> the macros/functions used to handle proc's files won't be in scope
> for an external LSM.
> Is there any particular reason why you prefer it done via LSM?

If you did a Yama style LSM it would be easy to configure.
Even though it might make no sense to allow this behavior,
someone, somewhere is counting on it.

>
> Thank you,
>
> Salvatore
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo at 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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] proc: prevent a task from writing on its own /proc/*/mem

[Alexey Dobriyan]

On Sat, May 26, 2018 at 07:30:47PM +0200, Salvatore Mesoraca wrote:
> 2018-05-26 17:48 GMT+02:00 Alexey Dobriyan <adobriyan@gmail.com>:
> > On Sat, May 26, 2018 at 04:50:46PM +0200, Salvatore Mesoraca wrote:
> >> Prevent a task from opening, in "write" mode, any /proc/*/mem
> >> file that operates on the task's mm.
> >> /proc/*/mem is mainly a debugging means and, as such, it shouldn't
> >> be used by the inspected process itself.
> >> Current implementation always allow a task to access its own
> >> /proc/*/mem file.
> >> A process can use it to overwrite read-only memory, making
> >> pointless the use of security_file_mprotect() or other ways to
> >> enforce RO memory.
> >
> > You can do it in security_ptrace_access_check()
> 
> No, because that hook is skipped when mm == current->mm:
> https://elixir.bootlin.com/linux/v4.17-rc6/source/kernel/fork.c#L1111

OK

> > or security_file_open()
> 
> This is true, but it looks a bit overkill to me, especially since many of
> the macros/functions used to handle proc's files won't be in scope
> for an external LSM.
> Is there any particular reason why you prefer it done via LSM?

Well, it exists to implement all kinds of non-standard restrictions.

You're probably blacklisting mprotect() and worry that compromised
program might use /proc/self/mem instead. But you need to blacklist
much more that mprotect(). I think forking a dummy "worker" process
to open your /proc/*/mem and pass a descriptor back should still work
with your patch.

[PATCH] proc: prevent a task from writing on its own /proc/*/mem

[Alexey Dobriyan]

On Sat, May 26, 2018 at 07:30:47PM +0200, Salvatore Mesoraca wrote:
> 2018-05-26 17:48 GMT+02:00 Alexey Dobriyan <adobriyan@gmail.com>:
> > On Sat, May 26, 2018 at 04:50:46PM +0200, Salvatore Mesoraca wrote:
> >> Prevent a task from opening, in "write" mode, any /proc/*/mem
> >> file that operates on the task's mm.
> >> /proc/*/mem is mainly a debugging means and, as such, it shouldn't
> >> be used by the inspected process itself.
> >> Current implementation always allow a task to access its own
> >> /proc/*/mem file.
> >> A process can use it to overwrite read-only memory, making
> >> pointless the use of security_file_mprotect() or other ways to
> >> enforce RO memory.
> >
> > You can do it in security_ptrace_access_check()
> 
> No, because that hook is skipped when mm == current->mm:
> https://elixir.bootlin.com/linux/v4.17-rc6/source/kernel/fork.c#L1111

OK

> > or security_file_open()
> 
> This is true, but it looks a bit overkill to me, especially since many of
> the macros/functions used to handle proc's files won't be in scope
> for an external LSM.
> Is there any particular reason why you prefer it done via LSM?

Well, it exists to implement all kinds of non-standard restrictions.

You're probably blacklisting mprotect() and worry that compromised
program might use /proc/self/mem instead. But you need to blacklist
much more that mprotect(). I think forking a dummy "worker" process
to open your /proc/*/mem and pass a descriptor back should still work
with your patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] proc: prevent a task from writing on its own /proc/*/mem

[Kees Cook]

On Sat, May 26, 2018 at 7:50 AM, Salvatore Mesoraca
<s.mesoraca16@gmail.com> wrote:
> Prevent a task from opening, in "write" mode, any /proc/*/mem
> file that operates on the task's mm.
> /proc/*/mem is mainly a debugging means and, as such, it shouldn't
> be used by the inspected process itself.
> Current implementation always allow a task to access its own
> /proc/*/mem file.
> A process can use it to overwrite read-only memory, making
> pointless the use of security_file_mprotect() or other ways to
> enforce RO memory.

I went through some old threads from 2012 when e268337dfe26 was
introduced, and later when things got looked at during DirtyCOW. There
was discussion about removing FOLL_FORCE (in order to block writes on
a read-only memory region). But that was much more general, touched
ptrace, etc. I think this patch would be okay, since it's specific to
the proc "self" mem interface, not remote processes (via ptrace). This
patch would also have blocked the /proc/self/mem path to DirtyCOW
(though not ptrace), so that would be nice if we have similar issues
in the future. So, as long as this doesn't break anything, I'm for it
in general. I've CCed Linus and Jann too, since they've stared at this
a lot too. :P

Note that you're re-checking the mm-check-for-self in mm_access().
That's used in /proc and for process_vm_write(). Ptrace (and
mm_access()) uses ptrace_may_access() for stuff (which has a similar
check to bypass LSMs), so I'd be curious what would happen if this
logic was plumbed into mm_access() instead of into proc_mem_open().
(Does anything open /proc/$pid files for writing? Does anything using
process_vm_write() on itself?)

-Kees

>
> Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com>
> ---
>  fs/proc/base.c       | 25 ++++++++++++++++++-------
>  fs/proc/internal.h   |  3 ++-
>  fs/proc/task_mmu.c   |  4 ++--
>  fs/proc/task_nommu.c |  2 +-
>  4 files changed, 23 insertions(+), 11 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 1a76d75..01ecfec 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -762,8 +762,9 @@ static int proc_single_open(struct inode *inode, struct file *filp)
>         .release        = single_release,
>  };
>
> -
> -struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
> +struct mm_struct *proc_mem_open(struct inode *inode,
> +                               unsigned int mode,
> +                               fmode_t f_mode)
>  {
>         struct task_struct *task = get_proc_task(inode);
>         struct mm_struct *mm = ERR_PTR(-ESRCH);
> @@ -773,10 +774,20 @@ struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
>                 put_task_struct(task);
>
>                 if (!IS_ERR_OR_NULL(mm)) {
> -                       /* ensure this mm_struct can't be freed */
> -                       mmgrab(mm);
> -                       /* but do not pin its memory */
> -                       mmput(mm);
> +                       /*
> +                        * Prevent this interface from being used as a mean
> +                        * to bypass memory restrictions, including those
> +                        * imposed by LSMs.
> +                        */
> +                       if (mm == current->mm &&
> +                           f_mode & FMODE_WRITE)
> +                               mm = ERR_PTR(-EACCES);
> +                       else {
> +                               /* ensure this mm_struct can't be freed */
> +                               mmgrab(mm);
> +                               /* but do not pin its memory */
> +                               mmput(mm);
> +                       }
>                 }
>         }
>
> @@ -785,7 +796,7 @@ struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
>
>  static int __mem_open(struct inode *inode, struct file *file, unsigned int mode)
>  {
> -       struct mm_struct *mm = proc_mem_open(inode, mode);
> +       struct mm_struct *mm = proc_mem_open(inode, mode, file->f_mode);
>
>         if (IS_ERR(mm))
>                 return PTR_ERR(mm);
> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> index 0f1692e..8d38cc7 100644
> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -275,7 +275,8 @@ struct proc_maps_private {
>  #endif
>  } __randomize_layout;
>
> -struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode);
> +struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode,
> +                               fmode_t f_mode);
>
>  extern const struct file_operations proc_pid_maps_operations;
>  extern const struct file_operations proc_tid_maps_operations;
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index c486ad4..efb6535 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -227,7 +227,7 @@ static int proc_maps_open(struct inode *inode, struct file *file,
>                 return -ENOMEM;
>
>         priv->inode = inode;
> -       priv->mm = proc_mem_open(inode, PTRACE_MODE_READ);
> +       priv->mm = proc_mem_open(inode, PTRACE_MODE_READ, file->f_mode);
>         if (IS_ERR(priv->mm)) {
>                 int err = PTR_ERR(priv->mm);
>
> @@ -1534,7 +1534,7 @@ static int pagemap_open(struct inode *inode, struct file *file)
>  {
>         struct mm_struct *mm;
>
> -       mm = proc_mem_open(inode, PTRACE_MODE_READ);
> +       mm = proc_mem_open(inode, PTRACE_MODE_READ, file->f_mode);
>         if (IS_ERR(mm))
>                 return PTR_ERR(mm);
>         file->private_data = mm;
> diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
> index 5b62f57..dc38516 100644
> --- a/fs/proc/task_nommu.c
> +++ b/fs/proc/task_nommu.c
> @@ -280,7 +280,7 @@ static int maps_open(struct inode *inode, struct file *file,
>                 return -ENOMEM;
>
>         priv->inode = inode;
> -       priv->mm = proc_mem_open(inode, PTRACE_MODE_READ);
> +       priv->mm = proc_mem_open(inode, PTRACE_MODE_READ, file->f_mode);
>         if (IS_ERR(priv->mm)) {
>                 int err = PTR_ERR(priv->mm);
>
> --
> 1.9.1
>



-- 
Kees Cook
Pixel Security

[PATCH] proc: prevent a task from writing on its own /proc/*/mem

[Kees Cook]

On Sat, May 26, 2018 at 7:50 AM, Salvatore Mesoraca
<s.mesoraca16@gmail.com> wrote:
> Prevent a task from opening, in "write" mode, any /proc/*/mem
> file that operates on the task's mm.
> /proc/*/mem is mainly a debugging means and, as such, it shouldn't
> be used by the inspected process itself.
> Current implementation always allow a task to access its own
> /proc/*/mem file.
> A process can use it to overwrite read-only memory, making
> pointless the use of security_file_mprotect() or other ways to
> enforce RO memory.

I went through some old threads from 2012 when e268337dfe26 was
introduced, and later when things got looked at during DirtyCOW. There
was discussion about removing FOLL_FORCE (in order to block writes on
a read-only memory region). But that was much more general, touched
ptrace, etc. I think this patch would be okay, since it's specific to
the proc "self" mem interface, not remote processes (via ptrace). This
patch would also have blocked the /proc/self/mem path to DirtyCOW
(though not ptrace), so that would be nice if we have similar issues
in the future. So, as long as this doesn't break anything, I'm for it
in general. I've CCed Linus and Jann too, since they've stared at this
a lot too. :P

Note that you're re-checking the mm-check-for-self in mm_access().
That's used in /proc and for process_vm_write(). Ptrace (and
mm_access()) uses ptrace_may_access() for stuff (which has a similar
check to bypass LSMs), so I'd be curious what would happen if this
logic was plumbed into mm_access() instead of into proc_mem_open().
(Does anything open /proc/$pid files for writing? Does anything using
process_vm_write() on itself?)

-Kees

>
> Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com>
> ---
>  fs/proc/base.c       | 25 ++++++++++++++++++-------
>  fs/proc/internal.h   |  3 ++-
>  fs/proc/task_mmu.c   |  4 ++--
>  fs/proc/task_nommu.c |  2 +-
>  4 files changed, 23 insertions(+), 11 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 1a76d75..01ecfec 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -762,8 +762,9 @@ static int proc_single_open(struct inode *inode, struct file *filp)
>         .release        = single_release,
>  };
>
> -
> -struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
> +struct mm_struct *proc_mem_open(struct inode *inode,
> +                               unsigned int mode,
> +                               fmode_t f_mode)
>  {
>         struct task_struct *task = get_proc_task(inode);
>         struct mm_struct *mm = ERR_PTR(-ESRCH);
> @@ -773,10 +774,20 @@ struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
>                 put_task_struct(task);
>
>                 if (!IS_ERR_OR_NULL(mm)) {
> -                       /* ensure this mm_struct can't be freed */
> -                       mmgrab(mm);
> -                       /* but do not pin its memory */
> -                       mmput(mm);
> +                       /*
> +                        * Prevent this interface from being used as a mean
> +                        * to bypass memory restrictions, including those
> +                        * imposed by LSMs.
> +                        */
> +                       if (mm == current->mm &&
> +                           f_mode & FMODE_WRITE)
> +                               mm = ERR_PTR(-EACCES);
> +                       else {
> +                               /* ensure this mm_struct can't be freed */
> +                               mmgrab(mm);
> +                               /* but do not pin its memory */
> +                               mmput(mm);
> +                       }
>                 }
>         }
>
> @@ -785,7 +796,7 @@ struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
>
>  static int __mem_open(struct inode *inode, struct file *file, unsigned int mode)
>  {
> -       struct mm_struct *mm = proc_mem_open(inode, mode);
> +       struct mm_struct *mm = proc_mem_open(inode, mode, file->f_mode);
>
>         if (IS_ERR(mm))
>                 return PTR_ERR(mm);
> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> index 0f1692e..8d38cc7 100644
> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -275,7 +275,8 @@ struct proc_maps_private {
>  #endif
>  } __randomize_layout;
>
> -struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode);
> +struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode,
> +                               fmode_t f_mode);
>
>  extern const struct file_operations proc_pid_maps_operations;
>  extern const struct file_operations proc_tid_maps_operations;
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index c486ad4..efb6535 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -227,7 +227,7 @@ static int proc_maps_open(struct inode *inode, struct file *file,
>                 return -ENOMEM;
>
>         priv->inode = inode;
> -       priv->mm = proc_mem_open(inode, PTRACE_MODE_READ);
> +       priv->mm = proc_mem_open(inode, PTRACE_MODE_READ, file->f_mode);
>         if (IS_ERR(priv->mm)) {
>                 int err = PTR_ERR(priv->mm);
>
> @@ -1534,7 +1534,7 @@ static int pagemap_open(struct inode *inode, struct file *file)
>  {
>         struct mm_struct *mm;
>
> -       mm = proc_mem_open(inode, PTRACE_MODE_READ);
> +       mm = proc_mem_open(inode, PTRACE_MODE_READ, file->f_mode);
>         if (IS_ERR(mm))
>                 return PTR_ERR(mm);
>         file->private_data = mm;
> diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
> index 5b62f57..dc38516 100644
> --- a/fs/proc/task_nommu.c
> +++ b/fs/proc/task_nommu.c
> @@ -280,7 +280,7 @@ static int maps_open(struct inode *inode, struct file *file,
>                 return -ENOMEM;
>
>         priv->inode = inode;
> -       priv->mm = proc_mem_open(inode, PTRACE_MODE_READ);
> +       priv->mm = proc_mem_open(inode, PTRACE_MODE_READ, file->f_mode);
>         if (IS_ERR(priv->mm)) {
>                 int err = PTR_ERR(priv->mm);
>
> --
> 1.9.1
>



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

Re: [PATCH] proc: prevent a task from writing on its own /proc/*/mem

[Linus Torvalds]

On Sat, May 26, 2018 at 5:32 PM Kees Cook <keescook@chromium.org> wrote:

> I went through some old threads from 2012 when e268337dfe26 was
> introduced, and later when things got looked at during DirtyCOW. There
> was discussion about removing FOLL_FORCE (in order to block writes on
> a read-only memory region).

Side note, we did that for /dev/mem, and things broke.

Thus commit f511c0b17b08 "Yes, people use FOLL_FORCE ;)"

Side note, that very sam ecommit f511c0b17b08 is also the explanation for
why the patch under discussion now seems broken.

People really do use "write to /proc/self/mem" as a way to keep the
mappings read-only, but have a way to change them when required.

              Linus

[PATCH] proc: prevent a task from writing on its own /proc/*/mem

[Linus Torvalds]

On Sat, May 26, 2018 at 5:32 PM Kees Cook <keescook@chromium.org> wrote:

> I went through some old threads from 2012 when e268337dfe26 was
> introduced, and later when things got looked at during DirtyCOW. There
> was discussion about removing FOLL_FORCE (in order to block writes on
> a read-only memory region).

Side note, we did that for /dev/mem, and things broke.

Thus commit f511c0b17b08 "Yes, people use FOLL_FORCE ;)"

Side note, that very sam ecommit f511c0b17b08 is also the explanation for
why the patch under discussion now seems broken.

People really do use "write to /proc/self/mem" as a way to keep the
mappings read-only, but have a way to change them when required.

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

Re: [PATCH] proc: prevent a task from writing on its own /proc/*/mem

[Kees Cook]

On Sat, May 26, 2018 at 6:33 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> Thus commit f511c0b17b08 "Yes, people use FOLL_FORCE ;)"
>
> Side note, that very sam ecommit f511c0b17b08 is also the explanation for
> why the patch under discussion now seems broken.
>
> People really do use "write to /proc/self/mem" as a way to keep the
> mappings read-only, but have a way to change them when required.

Ah! Yes, that is the commit I was trying to find. Thanks!

-Kees

-- 
Kees Cook
Pixel Security

[PATCH] proc: prevent a task from writing on its own /proc/*/mem

[Kees Cook]

On Sat, May 26, 2018 at 6:33 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> Thus commit f511c0b17b08 "Yes, people use FOLL_FORCE ;)"
>
> Side note, that very sam ecommit f511c0b17b08 is also the explanation for
> why the patch under discussion now seems broken.
>
> People really do use "write to /proc/self/mem" as a way to keep the
> mappings read-only, but have a way to change them when required.

Ah! Yes, that is the commit I was trying to find. Thanks!

-Kees

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

Re: [PATCH] proc: prevent a task from writing on its own /proc/*/mem

[Jann Horn]

On Sat, May 26, 2018 at 4:50 PM, Salvatore Mesoraca
<s.mesoraca16@gmail.com> wrote:
> Prevent a task from opening, in "write" mode, any /proc/*/mem
> file that operates on the task's mm.
> /proc/*/mem is mainly a debugging means and, as such, it shouldn't
> be used by the inspected process itself.
> Current implementation always allow a task to access its own
> /proc/*/mem file.
> A process can use it to overwrite read-only memory, making
> pointless the use of security_file_mprotect() or other ways to
> enforce RO memory.
>
> Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com>
> ---
>  fs/proc/base.c       | 25 ++++++++++++++++++-------
>  fs/proc/internal.h   |  3 ++-
>  fs/proc/task_mmu.c   |  4 ++--
>  fs/proc/task_nommu.c |  2 +-
>  4 files changed, 23 insertions(+), 11 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 1a76d75..01ecfec 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -762,8 +762,9 @@ static int proc_single_open(struct inode *inode, struct file *filp)
>         .release        = single_release,
>  };
>
> -
> -struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
> +struct mm_struct *proc_mem_open(struct inode *inode,
> +                               unsigned int mode,
> +                               fmode_t f_mode)
>  {
>         struct task_struct *task = get_proc_task(inode);
>         struct mm_struct *mm = ERR_PTR(-ESRCH);
> @@ -773,10 +774,20 @@ struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
>                 put_task_struct(task);
>
>                 if (!IS_ERR_OR_NULL(mm)) {
> -                       /* ensure this mm_struct can't be freed */
> -                       mmgrab(mm);
> -                       /* but do not pin its memory */
> -                       mmput(mm);
> +                       /*
> +                        * Prevent this interface from being used as a mean
> +                        * to bypass memory restrictions, including those
> +                        * imposed by LSMs.
> +                        */
> +                       if (mm == current->mm &&
> +                           f_mode & FMODE_WRITE)
> +                               mm = ERR_PTR(-EACCES);
> +                       else {
> +                               /* ensure this mm_struct can't be freed */
> +                               mmgrab(mm);
> +                               /* but do not pin its memory */
> +                               mmput(mm);
> +                       }
>                 }
>         }

I don't have an opinion on the overall patch, but this part looks
buggy: In the error path, you set `mm` to an error pointer, but you
still own the reference that mm_access() took on the old `mm`. The
error path needs to call `mmput(mm)`.

[PATCH] proc: prevent a task from writing on its own /proc/*/mem

[Jann Horn]

On Sat, May 26, 2018 at 4:50 PM, Salvatore Mesoraca
<s.mesoraca16@gmail.com> wrote:
> Prevent a task from opening, in "write" mode, any /proc/*/mem
> file that operates on the task's mm.
> /proc/*/mem is mainly a debugging means and, as such, it shouldn't
> be used by the inspected process itself.
> Current implementation always allow a task to access its own
> /proc/*/mem file.
> A process can use it to overwrite read-only memory, making
> pointless the use of security_file_mprotect() or other ways to
> enforce RO memory.
>
> Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com>
> ---
>  fs/proc/base.c       | 25 ++++++++++++++++++-------
>  fs/proc/internal.h   |  3 ++-
>  fs/proc/task_mmu.c   |  4 ++--
>  fs/proc/task_nommu.c |  2 +-
>  4 files changed, 23 insertions(+), 11 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 1a76d75..01ecfec 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -762,8 +762,9 @@ static int proc_single_open(struct inode *inode, struct file *filp)
>         .release        = single_release,
>  };
>
> -
> -struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
> +struct mm_struct *proc_mem_open(struct inode *inode,
> +                               unsigned int mode,
> +                               fmode_t f_mode)
>  {
>         struct task_struct *task = get_proc_task(inode);
>         struct mm_struct *mm = ERR_PTR(-ESRCH);
> @@ -773,10 +774,20 @@ struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
>                 put_task_struct(task);
>
>                 if (!IS_ERR_OR_NULL(mm)) {
> -                       /* ensure this mm_struct can't be freed */
> -                       mmgrab(mm);
> -                       /* but do not pin its memory */
> -                       mmput(mm);
> +                       /*
> +                        * Prevent this interface from being used as a mean
> +                        * to bypass memory restrictions, including those
> +                        * imposed by LSMs.
> +                        */
> +                       if (mm == current->mm &&
> +                           f_mode & FMODE_WRITE)
> +                               mm = ERR_PTR(-EACCES);
> +                       else {
> +                               /* ensure this mm_struct can't be freed */
> +                               mmgrab(mm);
> +                               /* but do not pin its memory */
> +                               mmput(mm);
> +                       }
>                 }
>         }

I don't have an opinion on the overall patch, but this part looks
buggy: In the error path, you set `mm` to an error pointer, but you
still own the reference that mm_access() took on the old `mm`. The
error path needs to call `mmput(mm)`.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] proc: prevent a task from writing on its own /proc/*/mem

[Salvatore Mesoraca]

2018-05-27 3:33 GMT+02:00 Linus Torvalds <torvalds@linux-foundation.org>:
> On Sat, May 26, 2018 at 5:32 PM Kees Cook <keescook@chromium.org> wrote:
>
>> I went through some old threads from 2012 when e268337dfe26 was
>> introduced, and later when things got looked at during DirtyCOW. There
>> was discussion about removing FOLL_FORCE (in order to block writes on
>> a read-only memory region).
>
> Side note, we did that for /dev/mem, and things broke.
>
> Thus commit f511c0b17b08 "Yes, people use FOLL_FORCE ;)"
>
> Side note, that very sam ecommit f511c0b17b08 is also the explanation for
> why the patch under discussion now seems broken.
>
> People really do use "write to /proc/self/mem" as a way to keep the
> mappings read-only, but have a way to change them when required.

Oh, I didn't expect this, interesting...
A configurable LSM is probably the right way to do this.

Thank you for your time,

Salvatore

[PATCH] proc: prevent a task from writing on its own /proc/*/mem

[Salvatore Mesoraca]

2018-05-27 3:33 GMT+02:00 Linus Torvalds <torvalds@linux-foundation.org>:
> On Sat, May 26, 2018 at 5:32 PM Kees Cook <keescook@chromium.org> wrote:
>
>> I went through some old threads from 2012 when e268337dfe26 was
>> introduced, and later when things got looked at during DirtyCOW. There
>> was discussion about removing FOLL_FORCE (in order to block writes on
>> a read-only memory region).
>
> Side note, we did that for /dev/mem, and things broke.
>
> Thus commit f511c0b17b08 "Yes, people use FOLL_FORCE ;)"
>
> Side note, that very sam ecommit f511c0b17b08 is also the explanation for
> why the patch under discussion now seems broken.
>
> People really do use "write to /proc/self/mem" as a way to keep the
> mappings read-only, but have a way to change them when required.

Oh, I didn't expect this, interesting...
A configurable LSM is probably the right way to do this.

Thank you for your time,

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

Re: [PATCH] proc: prevent a task from writing on its own /proc/*/mem

[Salvatore Mesoraca]

2018-05-28 11:06 GMT+02:00 Jann Horn <jannh@google.com>:
> On Sat, May 26, 2018 at 4:50 PM, Salvatore Mesoraca
> <s.mesoraca16@gmail.com> wrote:
>> Prevent a task from opening, in "write" mode, any /proc/*/mem
>> file that operates on the task's mm.
>> /proc/*/mem is mainly a debugging means and, as such, it shouldn't
>> be used by the inspected process itself.
>> Current implementation always allow a task to access its own
>> /proc/*/mem file.
>> A process can use it to overwrite read-only memory, making
>> pointless the use of security_file_mprotect() or other ways to
>> enforce RO memory.
>>
>> Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com>
>> ---
>>  fs/proc/base.c       | 25 ++++++++++++++++++-------
>>  fs/proc/internal.h   |  3 ++-
>>  fs/proc/task_mmu.c   |  4 ++--
>>  fs/proc/task_nommu.c |  2 +-
>>  4 files changed, 23 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>> index 1a76d75..01ecfec 100644
>> --- a/fs/proc/base.c
>> +++ b/fs/proc/base.c
>> @@ -762,8 +762,9 @@ static int proc_single_open(struct inode *inode, struct file *filp)
>>         .release        = single_release,
>>  };
>>
>> -
>> -struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
>> +struct mm_struct *proc_mem_open(struct inode *inode,
>> +                               unsigned int mode,
>> +                               fmode_t f_mode)
>>  {
>>         struct task_struct *task = get_proc_task(inode);
>>         struct mm_struct *mm = ERR_PTR(-ESRCH);
>> @@ -773,10 +774,20 @@ struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
>>                 put_task_struct(task);
>>
>>                 if (!IS_ERR_OR_NULL(mm)) {
>> -                       /* ensure this mm_struct can't be freed */
>> -                       mmgrab(mm);
>> -                       /* but do not pin its memory */
>> -                       mmput(mm);
>> +                       /*
>> +                        * Prevent this interface from being used as a mean
>> +                        * to bypass memory restrictions, including those
>> +                        * imposed by LSMs.
>> +                        */
>> +                       if (mm == current->mm &&
>> +                           f_mode & FMODE_WRITE)
>> +                               mm = ERR_PTR(-EACCES);
>> +                       else {
>> +                               /* ensure this mm_struct can't be freed */
>> +                               mmgrab(mm);
>> +                               /* but do not pin its memory */
>> +                               mmput(mm);
>> +                       }
>>                 }
>>         }
>
> I don't have an opinion on the overall patch, but this part looks
> buggy: In the error path, you set `mm` to an error pointer, but you
> still own the reference that mm_access() took on the old `mm`. The
> error path needs to call `mmput(mm)`.

You are absolutely right,

Thank you,

Salvatore

[PATCH] proc: prevent a task from writing on its own /proc/*/mem

[Salvatore Mesoraca]

2018-05-28 11:06 GMT+02:00 Jann Horn <jannh@google.com>:
> On Sat, May 26, 2018 at 4:50 PM, Salvatore Mesoraca
> <s.mesoraca16@gmail.com> wrote:
>> Prevent a task from opening, in "write" mode, any /proc/*/mem
>> file that operates on the task's mm.
>> /proc/*/mem is mainly a debugging means and, as such, it shouldn't
>> be used by the inspected process itself.
>> Current implementation always allow a task to access its own
>> /proc/*/mem file.
>> A process can use it to overwrite read-only memory, making
>> pointless the use of security_file_mprotect() or other ways to
>> enforce RO memory.
>>
>> Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com>
>> ---
>>  fs/proc/base.c       | 25 ++++++++++++++++++-------
>>  fs/proc/internal.h   |  3 ++-
>>  fs/proc/task_mmu.c   |  4 ++--
>>  fs/proc/task_nommu.c |  2 +-
>>  4 files changed, 23 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>> index 1a76d75..01ecfec 100644
>> --- a/fs/proc/base.c
>> +++ b/fs/proc/base.c
>> @@ -762,8 +762,9 @@ static int proc_single_open(struct inode *inode, struct file *filp)
>>         .release        = single_release,
>>  };
>>
>> -
>> -struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
>> +struct mm_struct *proc_mem_open(struct inode *inode,
>> +                               unsigned int mode,
>> +                               fmode_t f_mode)
>>  {
>>         struct task_struct *task = get_proc_task(inode);
>>         struct mm_struct *mm = ERR_PTR(-ESRCH);
>> @@ -773,10 +774,20 @@ struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
>>                 put_task_struct(task);
>>
>>                 if (!IS_ERR_OR_NULL(mm)) {
>> -                       /* ensure this mm_struct can't be freed */
>> -                       mmgrab(mm);
>> -                       /* but do not pin its memory */
>> -                       mmput(mm);
>> +                       /*
>> +                        * Prevent this interface from being used as a mean
>> +                        * to bypass memory restrictions, including those
>> +                        * imposed by LSMs.
>> +                        */
>> +                       if (mm == current->mm &&
>> +                           f_mode & FMODE_WRITE)
>> +                               mm = ERR_PTR(-EACCES);
>> +                       else {
>> +                               /* ensure this mm_struct can't be freed */
>> +                               mmgrab(mm);
>> +                               /* but do not pin its memory */
>> +                               mmput(mm);
>> +                       }
>>                 }
>>         }
>
> I don't have an opinion on the overall patch, but this part looks
> buggy: In the error path, you set `mm` to an error pointer, but you
> still own the reference that mm_access() took on the old `mm`. The
> error path needs to call `mmput(mm)`.

You are absolutely right,

Thank you,

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

Re: [PATCH] proc: prevent a task from writing on its own /proc/*/mem

[Steve Kemp]

> A configurable LSM is probably the right way to do this.

I wonder how many out of tree LSM there are?  Looking at the mainline
kernel the only "small" LSM bundled is YAMA, and it seems that most of
the patches proposing new ones eventually die out.

I appreciate that there are probably a lot of "toy" or "local" modules
out there for specific fields, companies, or products, but it does
seem odd that there are so few discussed publicly.

(The last two I remember were S.A.R.A and something relating to
xattr-attributes being used to whitelist execution.)

Steve

[PATCH] proc: prevent a task from writing on its own /proc/*/mem

[Steve Kemp]

> A configurable LSM is probably the right way to do this.

I wonder how many out of tree LSM there are?  Looking at the mainline
kernel the only "small" LSM bundled is YAMA, and it seems that most of
the patches proposing new ones eventually die out.

I appreciate that there are probably a lot of "toy" or "local" modules
out there for specific fields, companies, or products, but it does
seem odd that there are so few discussed publicly.

(The last two I remember were S.A.R.A and something relating to
xattr-attributes being used to whitelist execution.)

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

Re: [PATCH] proc: prevent a task from writing on its own /proc/*/mem

[Steve Kemp]

> A configurable LSM is probably the right way to do this.

I wonder how many out of tree LSM there are?  Looking at the mainline
kernel the only "small" LSM bundled is YAMA, and it seems that most of
the patches proposing new ones eventually die out.

I appreciate that there are probably a lot of "toy" or "local" modules
out there for specific fields, companies, or products, but it does
seem odd that there are so few discussed publicly.

(The last two I remember were S.A.R.A and something relating to
xattr-attributes being used to whitelist execution.)

Steve

Re: [PATCH] proc: prevent a task from writing on its own /proc/*/mem

[Casey Schaufler]

On 6/4/2018 9:57 AM, Steve Kemp wrote:
>> A configurable LSM is probably the right way to do this.
> I wonder how many out of tree LSM there are?  Looking at the mainline
> kernel the only "small" LSM bundled is YAMA, and it seems that most of
> the patches proposing new ones eventually die out.

LoadPin is upstream.

> I appreciate that there are probably a lot of "toy" or "local" modules
> out there for specific fields, companies, or products, but it does
> seem odd that there are so few discussed publicly.

Minor modules like Yama and LoadPin are constrained by not being
able to use security blobs. That seriously limits the sort of thing
you can do with them. It often makes more sense to get the behavior
in mainline under CONFIG_SOMETHING than to provide a minor LSM in
that case.

> (The last two I remember were S.A.R.A and something relating to
> xattr-attributes being used to whitelist execution.)

Anything that would have to be a major (blob using) module has
a very tough time because you have to displace an existing major
module (SELinux, AppArmor, Smack, TOMOYO) in order to use it.
When we get infrastructure managed security blobs upstream most
of the proposed modules could be used in conjunction with the
existing installed modules. Some would have to wait for the
complete stacking solution, but that's limited to use of networking
facilities.

There's also renewed interest in minor modules being dynamically
loadable, so they can be added on a running system as new and
interesting threats get newer and more interesting mitigations.

We don't make it easy for new modules. Some of that is an
artifact of the infrastructure, and some is based on caution.

> Steve
> --
> 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] proc: prevent a task from writing on its own /proc/*/mem

[Casey Schaufler]

On 6/4/2018 9:57 AM, Steve Kemp wrote:
>> A configurable LSM is probably the right way to do this.
> I wonder how many out of tree LSM there are?  Looking at the mainline
> kernel the only "small" LSM bundled is YAMA, and it seems that most of
> the patches proposing new ones eventually die out.

LoadPin is upstream.

> I appreciate that there are probably a lot of "toy" or "local" modules
> out there for specific fields, companies, or products, but it does
> seem odd that there are so few discussed publicly.

Minor modules like Yama and LoadPin are constrained by not being
able to use security blobs. That seriously limits the sort of thing
you can do with them. It often makes more sense to get the behavior
in mainline under CONFIG_SOMETHING than to provide a minor LSM in
that case.

> (The last two I remember were S.A.R.A and something relating to
> xattr-attributes being used to whitelist execution.)

Anything that would have to be a major (blob using) module has
a very tough time because you have to displace an existing major
module (SELinux, AppArmor, Smack, TOMOYO) in order to use it.
When we get infrastructure managed security blobs upstream most
of the proposed modules could be used in conjunction with the
existing installed modules. Some would have to wait for the
complete stacking solution, but that's limited to use of networking
facilities.

There's also renewed interest in minor modules being dynamically
loadable, so they can be added on a running system as new and
interesting threats get newer and more interesting mitigations.

We don't make it easy for new modules. Some of that is an
artifact of the infrastructure, and some is based on caution.

> Steve
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo at 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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] proc: prevent a task from writing on its own /proc/*/mem

[Salvatore Mesoraca]

2018-06-04 18:57 GMT+02:00 Steve Kemp <steve.backup.kemp@gmail.com>:
>> A configurable LSM is probably the right way to do this.
>
> I wonder how many out of tree LSM there are?  Looking at the mainline
> kernel the only "small" LSM bundled is YAMA, and it seems that most of
> the patches proposing new ones eventually die out.
>
> I appreciate that there are probably a lot of "toy" or "local" modules
> out there for specific fields, companies, or products, but it does
> seem odd that there are so few discussed publicly.
>
> (The last two I remember were S.A.R.A and something relating to
> xattr-attributes being used to whitelist execution.)

FWIW S.A.R.A. is not dead [1].
Unfortunately it needs infrastructure managed security blobs, so I didn't
tried to get it upstream, yet.
Of course, I can't give you any guarantees about when or if it will be
upstreamed,
but it's definitely still alive.

[1] https://github.com/smeso/sara/releases/latest

[PATCH] proc: prevent a task from writing on its own /proc/*/mem

[Salvatore Mesoraca]

2018-06-04 18:57 GMT+02:00 Steve Kemp <steve.backup.kemp@gmail.com>:
>> A configurable LSM is probably the right way to do this.
>
> I wonder how many out of tree LSM there are?  Looking at the mainline
> kernel the only "small" LSM bundled is YAMA, and it seems that most of
> the patches proposing new ones eventually die out.
>
> I appreciate that there are probably a lot of "toy" or "local" modules
> out there for specific fields, companies, or products, but it does
> seem odd that there are so few discussed publicly.
>
> (The last two I remember were S.A.R.A and something relating to
> xattr-attributes being used to whitelist execution.)

FWIW S.A.R.A. is not dead [1].
Unfortunately it needs infrastructure managed security blobs, so I didn't
tried to get it upstream, yet.
Of course, I can't give you any guarantees about when or if it will be
upstreamed,
but it's definitely still alive.

[1] https://github.com/smeso/sara/releases/latest
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html