diff mbox series

[1/1] ptrace: Fix ->ptracer_cred handling for PTRACE_TRACEME

Message ID 1563488579-18170-2-git-send-email-tyhicks@canonical.com
State New
Headers show
Series CVE-2019-13272: ptrace privilege escalation | expand

Commit Message

Tyler Hicks July 18, 2019, 10:22 p.m. UTC 
From: Jann Horn <jannh@google.com>

Fix two issues:

When called for PTRACE_TRACEME, ptrace_link() would obtain an RCU
reference to the parent's objective credentials, then give that pointer
to get_cred().  However, the object lifetime rules for things like
struct cred do not permit unconditionally turning an RCU reference into
a stable reference.

PTRACE_TRACEME records the parent's credentials as if the parent was
acting as the subject, but that's not the case.  If a malicious
unprivileged child uses PTRACE_TRACEME and the parent is privileged, and
at a later point, the parent process becomes attacker-controlled
(because it drops privileges and calls execve()), the attacker ends up
with control over two processes with a privileged ptrace relationship,
which can be abused to ptrace a suid binary and obtain root privileges.

Fix both of these by always recording the credentials of the process
that is requesting the creation of the ptrace relationship:
current_cred() can't change under us, and current is the proper subject
for access control.

This change is theoretically userspace-visible, but I am not aware of
any code that it will actually break.

Fixes: 64b875f7ac8a ("ptrace: Capture the ptracer's creds not PT_PTRACE_CAP")
Signed-off-by: Jann Horn <jannh@google.com>
Acked-by: Oleg Nesterov <oleg@redhat.com>
Cc: stable@vger.kernel.org
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

CVE-2019-13272

(cherry picked from commit 6994eefb0053799d2e07cd140df6c2ea106c41ee)
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
---
 kernel/ptrace.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

Marcelo Henrique Cerri July 18, 2019, 10:28 p.m. UTC | #1 
Acked-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>

On Thu, Jul 18, 2019 at 10:22:59PM +0000, Tyler Hicks wrote:
> From: Jann Horn <jannh@google.com>
> 
> Fix two issues:
> 
> When called for PTRACE_TRACEME, ptrace_link() would obtain an RCU
> reference to the parent's objective credentials, then give that pointer
> to get_cred().  However, the object lifetime rules for things like
> struct cred do not permit unconditionally turning an RCU reference into
> a stable reference.
> 
> PTRACE_TRACEME records the parent's credentials as if the parent was
> acting as the subject, but that's not the case.  If a malicious
> unprivileged child uses PTRACE_TRACEME and the parent is privileged, and
> at a later point, the parent process becomes attacker-controlled
> (because it drops privileges and calls execve()), the attacker ends up
> with control over two processes with a privileged ptrace relationship,
> which can be abused to ptrace a suid binary and obtain root privileges.
> 
> Fix both of these by always recording the credentials of the process
> that is requesting the creation of the ptrace relationship:
> current_cred() can't change under us, and current is the proper subject
> for access control.
> 
> This change is theoretically userspace-visible, but I am not aware of
> any code that it will actually break.
> 
> Fixes: 64b875f7ac8a ("ptrace: Capture the ptracer's creds not PT_PTRACE_CAP")
> Signed-off-by: Jann Horn <jannh@google.com>
> Acked-by: Oleg Nesterov <oleg@redhat.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> 
> CVE-2019-13272
> 
> (cherry picked from commit 6994eefb0053799d2e07cd140df6c2ea106c41ee)
> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
> ---
>  kernel/ptrace.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 6f357f4fc859..eb012477910e 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -78,9 +78,7 @@ void __ptrace_link(struct task_struct *child, struct task_struct *new_parent,
>   */
>  static void ptrace_link(struct task_struct *child, struct task_struct *new_parent)
>  {
> -	rcu_read_lock();
> -	__ptrace_link(child, new_parent, __task_cred(new_parent));
> -	rcu_read_unlock();
> +	__ptrace_link(child, new_parent, current_cred());
>  }
>  
>  /**
> -- 
> 2.7.4
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Connor Kuehl July 18, 2019, 10:45 p.m. UTC | #2 
On 7/18/19 3:22 PM, Tyler Hicks wrote:
> From: Jann Horn <jannh@google.com>
> 
> Fix two issues:
> 
> When called for PTRACE_TRACEME, ptrace_link() would obtain an RCU
> reference to the parent's objective credentials, then give that pointer
> to get_cred().  However, the object lifetime rules for things like
> struct cred do not permit unconditionally turning an RCU reference into
> a stable reference.
> 
> PTRACE_TRACEME records the parent's credentials as if the parent was
> acting as the subject, but that's not the case.  If a malicious
> unprivileged child uses PTRACE_TRACEME and the parent is privileged, and
> at a later point, the parent process becomes attacker-controlled
> (because it drops privileges and calls execve()), the attacker ends up
> with control over two processes with a privileged ptrace relationship,
> which can be abused to ptrace a suid binary and obtain root privileges.
> 
> Fix both of these by always recording the credentials of the process
> that is requesting the creation of the ptrace relationship:
> current_cred() can't change under us, and current is the proper subject
> for access control.
> 
> This change is theoretically userspace-visible, but I am not aware of
> any code that it will actually break.
> 
> Fixes: 64b875f7ac8a ("ptrace: Capture the ptracer's creds not PT_PTRACE_CAP")
> Signed-off-by: Jann Horn <jannh@google.com>
> Acked-by: Oleg Nesterov <oleg@redhat.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> 
> CVE-2019-13272
> 
> (cherry picked from commit 6994eefb0053799d2e07cd140df6c2ea106c41ee)
> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>

Acked-by: Connor Kuehl <connor.kuehl@canonical.com>


> ---
>  kernel/ptrace.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 6f357f4fc859..eb012477910e 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -78,9 +78,7 @@ void __ptrace_link(struct task_struct *child, struct task_struct *new_parent,
>   */
>  static void ptrace_link(struct task_struct *child, struct task_struct *new_parent)
>  {
> -	rcu_read_lock();
> -	__ptrace_link(child, new_parent, __task_cred(new_parent));
> -	rcu_read_unlock();
> +	__ptrace_link(child, new_parent, current_cred());
>  }
>  
>  /**
>
diff mbox series

Patch

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 6f357f4fc859..eb012477910e 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -78,9 +78,7 @@  void __ptrace_link(struct task_struct *child, struct task_struct *new_parent,
  */
 static void ptrace_link(struct task_struct *child, struct task_struct *new_parent)
 {
-	rcu_read_lock();
-	__ptrace_link(child, new_parent, __task_cred(new_parent));
-	rcu_read_unlock();
+	__ptrace_link(child, new_parent, current_cred());
 }
 
 /**