Skip to contentSkip to conversation

file capabilities: allow sigcont within session (v2)

PATCH In review Serge E. Hallyn 5 participants 8 messages Oct 31, 2007 – Nov 3, 2007

Subsystems (2)
Security (LSM) Capabilities
lore
Share & tools
lore

Serge E. Hallyn

author
#0 lore raw

PATCH 1/1 file capabilities: allow sigcont within session (v2)

Quoted 1 line

From 5bff8967f45a35f858b96ca673d9bf98eac53d49 Mon Sep 17 00:00:00 2001

From: Serge E. Hallyn <[email protected]>
Date: Wed, 31 Oct 2007 11:22:04 -0500
Subject: [PATCH 1/1] file capabilities: allow sigcont within session (v2)

(This is a proposed fix to http://bugzilla.kernel.org/show_bug.cgi?id=9247)

Allow sigcont to be sent to a process with greater capabilities if it is in the same session. Otherwise, a shell from which I've started a root shell and done 'suspend' can't be restarted by the parent shell.

Also don't do file-capabilities signaling checks when uids for the processes don't match, since the standard check_kill_permission will have done those checks.

Signed-off-by Serge E. Hallyn <[email protected]>
---
 security/commoncap.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)
security/commoncap.c +9 -0
index bf67871..4de6857 100644
@@ -526,6 +526,15 @@ int cap_task_kill(struct task_struct *p, struct siginfo *info,
 	if (info != SEND_SIG_NOINFO && (is_si_special(info) || SI_FROMKERNEL(info)))
 		return 0;
 
+	/* if tasks have same uid, then check_kill_permission did check */
+	if (current->uid == p->uid || current->euid == p->uid ||
+		current->uid == p->suid || current->euid == p->suid)
+		return 0;
+
+	/* sigcont is permitted within same session */
+	if (sig == SIGCONT && (task_session_nr(current)==task_session_nr(p)))
+		return 0;
+
 	if (secid)
 		/*
 		 * Signal sent as a particular user.
Signature · 1 line
1.5.1.1.GIT
#1 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Ackd.

Cheers

Andrew

Quoted 31 lines · Serge E. Hallyn
Quoted 1 line

From 5bff8967f45a35f858b96ca673d9bf98eac53d49 Mon Sep 17 00:00:00 2001

From: Serge E. Hallyn <[email protected]>
Date: Wed, 31 Oct 2007 11:22:04 -0500
Subject: [PATCH 1/1] file capabilities: allow sigcont within session (v2)

(This is a proposed fix to http://bugzilla.kernel.org/show_bug.cgi?id=9247)

Allow sigcont to be sent to a process with greater capabilities if it is in the same session. Otherwise, a shell from which I've started a root shell and done 'suspend' can't be restarted by the parent shell.

Also don't do file-capabilities signaling checks when uids for the processes don't match, since the standard check_kill_permission will have done those checks.

Signed-off-by Serge E. Hallyn <[email protected]>
---
 security/commoncap.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)
security/commoncap.c +2 -0
index bf67871..4de6857 100644
@@ -526,6 +526,15 @@ int cap_task_kill(struct task_struct *p, struct siginfo *info,
 	if (info != SEND_SIG_NOINFO && (is_si_special(info) || SI_FROMKERNEL(info)))
 		return 0;
 
+	/* if tasks have same uid, then check_kill_permission did check */
+	if (current->uid == p->uid || current->euid == p->uid ||

security/commoncap.c int cap_task_kill(struct task_struct *p, struct siginfo *info, · line 540

Quoted 10 lines
security/commoncap.c +7 -0
+		current->uid == p->suid || current->euid == p->suid)
+		return 0;
+
+	/* sigcont is permitted within same session */
+	if (sig == SIGCONT && (task_session_nr(current)==task_session_nr(p)))
+		return 0;
+
 	if (secid)
 		/*
 		 * Signal sent as a particular user.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFHKSuSmwytjiwfWMwRAkHXAJ4lr07yW936ychUGNxqQSOanZTecwCePYVc d8uP0I3AEDjTpG8s7Nojo7Y=
=8777
-----END PGP SIGNATURE-----

#2 lore raw
In reply to Serge E. Hallyn

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

[kernel/signal.c:check_kill_permission() could probably benefit from getting more consistently indented!]

I'm not sure I can grok your comment. Did you mean:

/* as per, check_kill_permission(), permit if tasks have same uid */

As to content:

Signed-off-by Andrew G. Morgan <[email protected]>

Cheers

Andrew

Quoted 31 lines · Serge E. Hallyn
Quoted 1 line

From 5bff8967f45a35f858b96ca673d9bf98eac53d49 Mon Sep 17 00:00:00 2001

From: Serge E. Hallyn <[email protected]>
Date: Wed, 31 Oct 2007 11:22:04 -0500
Subject: [PATCH 1/1] file capabilities: allow sigcont within session (v2)

(This is a proposed fix to http://bugzilla.kernel.org/show_bug.cgi?id=9247)

Allow sigcont to be sent to a process with greater capabilities if it is in the same session. Otherwise, a shell from which I've started a root shell and done 'suspend' can't be restarted by the parent shell.

Also don't do file-capabilities signaling checks when uids for the processes don't match, since the standard check_kill_permission will have done those checks.

Signed-off-by Serge E. Hallyn <[email protected]>
---
 security/commoncap.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)
security/commoncap.c +2 -0
index bf67871..4de6857 100644
@@ -526,6 +526,15 @@ int cap_task_kill(struct task_struct *p, struct siginfo *info,
 	if (info != SEND_SIG_NOINFO && (is_si_special(info) || SI_FROMKERNEL(info)))
 		return 0;
 
+	/* if tasks have same uid, then check_kill_permission did check */
+	if (current->uid == p->uid || current->euid == p->uid ||

security/commoncap.c int cap_task_kill(struct task_struct *p, struct siginfo *info, · line 540

Quoted 10 lines
security/commoncap.c +7 -0
+		current->uid == p->suid || current->euid == p->suid)
+		return 0;
+
+	/* sigcont is permitted within same session */
+	if (sig == SIGCONT && (task_session_nr(current)==task_session_nr(p)))
+		return 0;
+
 	if (secid)
 		/*
 		 * Signal sent as a particular user.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)

iD8DBQFHKVpRQheEq9QabfIRAnp9AKCZHb526eioQWKycH7V7LfcHP7VvQCdG0AJ QTVOLvQ2hip+j2qZ1mb2Y6w=
=45et
-----END PGP SIGNATURE-----

#3 lore raw
In reply to Serge E. Hallyn
Quoted 7 lines · Serge E. Hallyn
Quoted 1 line

From 5bff8967f45a35f858b96ca673d9bf98eac53d49 Mon Sep 17 00:00:00 2001

From: Serge E. Hallyn <[email protected]>
Date: Wed, 31 Oct 2007 11:22:04 -0500
Subject: [PATCH 1/1] file capabilities: allow sigcont within session (v2)

(This is a proposed fix to http://bugzilla.kernel.org/show_bug.cgi?id=9247)

Quoted 8 lines

Allow sigcont to be sent to a process with greater capabilities if it is in the same session. Otherwise, a shell from which I've started a root shell and done 'suspend' can't be restarted by the parent shell.

Also don't do file-capabilities signaling checks when uids for the processes don't match, since the standard check_kill_permission will have done those checks.

Description doesn't match the code. And in the non-matching uid case, check_kill_permission typically returns an error before it reaches cap_task_kill (modulo the special cases).

Quoted 11 lines
Signed-off-by Serge E. Hallyn <[email protected]>
---
 security/commoncap.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)
security/commoncap.c +0 -0
index bf67871..4de6857 100644

security/commoncap.c int cap_task_kill(struct task_struct *p, struct siginfo *info, · line 532

Quoted 7 lines
security/commoncap.c +4 -0
@@ -526,6 +526,15 @@ int cap_task_kill(struct task_struct *p, struct siginfo *info,
 	if (info != SEND_SIG_NOINFO && (is_si_special(info) || SI_FROMKERNEL(info)))
 		return 0;
 
+	/* if tasks have same uid, then check_kill_permission did check */
+	if (current->uid == p->uid || current->euid == p->uid ||
+		current->uid == p->suid || current->euid == p->suid)
+		return 0;

I'm confused - if you are allowing all signals within the same uid, then what was the point of having a cap_task_kill at all? cap_task_kill was supposed to prevent a process with lesser capabilities from killing a process with more capabilities, even if they have the same uid, so that when you have a program marked with file capabilities instead of a setuid-0 program, that program can't be sent arbitrary signals by the caller.

Quoted 8 lines
security/commoncap.c +5 -0
+
+	/* sigcont is permitted within same session */
+	if (sig == SIGCONT && (task_session_nr(current)==task_session_nr(p)))
+		return 0;
+
 	if (secid)
 		/*
 		 * Signal sent as a particular user.
Signature · 2 lines
Stephen Smalley
National Security Agency
author
#4 lore raw
Quoted 13 lines · Stephen Smalley
Quoted 12 lines · Serge E. Hallyn
Quoted 1 line

From 5bff8967f45a35f858b96ca673d9bf98eac53d49 Mon Sep 17 00:00:00 2001

From: Serge E. Hallyn <[email protected]>
Date: Wed, 31 Oct 2007 11:22:04 -0500
Subject: [PATCH 1/1] file capabilities: allow sigcont within session (v2)

(This is a proposed fix to http://bugzilla.kernel.org/show_bug.cgi?id=9247)

Allow sigcont to be sent to a process with greater capabilities if it is in the same session. Otherwise, a shell from which I've started a root shell and done 'suspend' can't be restarted by the parent shell.

Quoted 5 lines
Quoted 3 lines

Also don't do file-capabilities signaling checks when uids for the processes don't match, since the standard check_kill_permission will have done those checks.

Description doesn't match the code.

Egads. I knew I should've just kept that part out of it for the first patch...

New patch on top of previous one is appended.

Thanks.

Quoted 3 lines

And in the non-matching uid case,
check_kill_permission typically returns an error before it reaches cap_task_kill (modulo the special cases).

Typically, but when it doesn't, then the file capabilities shouldn't get in the way of check_kill_permission() granting permission. The file capabilities

Quoted 15 lines
Quoted 15 lines
Signed-off-by Serge E. Hallyn <[email protected]>
---
 security/commoncap.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)
security/commoncap.c +0 -0
index bf67871..4de6857 100644
@@ -526,6 +526,15 @@ int cap_task_kill(struct task_struct *p, struct siginfo *info,
 	if (info != SEND_SIG_NOINFO && (is_si_special(info) || SI_FROMKERNEL(info)))
 		return 0;
Quoted 6 lines

security/commoncap.c int cap_task_kill(struct task_struct *p, struct siginfo *info, · line 532

Quoted 4 lines
security/commoncap.c +4 -0
+	/* if tasks have same uid, then check_kill_permission did check */
+	if (current->uid == p->uid || current->euid == p->uid ||
+		current->uid == p->suid || current->euid == p->suid)
+		return 0;

I'm confused - if you are allowing all signals within the same uid, then

No I was confused. I wanted to allow for tasks with different uids.

But in fact that's not safe anyway. A binary can be setuid and owned by a non-root user user1, have file capabilities, and be executed by user2.

(Anyway given how grossly my code missed my erroneous intentions, I need to add some signal tests to my file capabilities tests - and get those tests into LTP)

Quoted 7 lines

what was the point of having a cap_task_kill at all? cap_task_kill was supposed to prevent a process with lesser capabilities from killing a process with more capabilities, even if they have the same uid, so that when you have a program marked with file capabilities instead of a setuid-0 program, that program can't be sent arbitrary signals by the caller.

Quoted 11 lines
Quoted 8 lines
security/commoncap.c +5 -0
+
+	/* sigcont is permitted within same session */
+	if (sig == SIGCONT && (task_session_nr(current)==task_session_nr(p)))
+		return 0;
+
 	if (secid)
 		/*
 		 * Signal sent as a particular user.
Signature · 2 lines
Stephen Smalley
National Security Agency

Thanks, Stephen.

Quoted 1 line

From 98741f07ab1bc4a1fc2de7fedfb9023ea30bf988 Mon Sep 17 00:00:00 2001

From: Serge E. Hallyn <[email protected]>
Date: Thu, 1 Nov 2007 08:20:12 -0500
Subject: [PATCH 1/1] file capabilities: remove the non-matching uid special case for kill

There I went again having one patch do two (related) things.

Remove the special check I had added to cap_task_kill() for non-matching uids. In fact it turns out the check wouldn't be safe even if I'd coded it correctly. A binary can be setuid and owned by a non-root user user1, have file capabilities, and be executed by user2.

Signed-off-by Serge E. Hallyn <[email protected]>
---
 security/commoncap.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)
security/commoncap.c +0 -5
index f04784a..302e8d0 100644
@@ -526,11 +526,6 @@ int cap_task_kill(struct task_struct *p, struct siginfo *info,
 	if (info != SEND_SIG_NOINFO && (is_si_special(info) || SI_FROMKERNEL(info)))
 		return 0;
 
-	/* if tasks have same uid, then check_kill_permission did check */
-	if (current->uid == p->uid || current->euid == p->uid ||
-		current->uid == p->suid || current->euid == p->suid)
-		return 0;
-
 	/* sigcont is permitted within same session */
 	if (sig == SIGCONT && (task_session_nr(current) == task_session_nr(p)))
 		return 0;
Signature · 1 line
1.5.1.1.GIT
#5 lore raw
Quoted 4 lines · Serge E. Hallyn

Egads. I knew I should've just kept that part out of it for the first patch...

New patch on top of previous one is appended.

I assume you'll just collapse the two patches together before you submit them? I've been distracted dealing with the ALPM suspend2ram regression, which Jeff Garzik beat me to in bisecting, but I'll try out your newer patch now...

- Ted

Tested-by Theodore Ts'o
#6 lore raw
In reply to Serge E. Hallyn
Quoted 5 lines · Serge E. Hallyn
Quoted 4 lines
Quoted 4 lines
Quoted 1 line

From 5bff8967f45a35f858b96ca673d9bf98eac53d49 Mon Sep 17 00:00:00 2001

From: Serge E. Hallyn <[email protected]>
Date: Wed, 31 Oct 2007 11:22:04 -0500
Subject: [PATCH 1/1] file capabilities: allow sigcont within session (v2)

Quoted 7 lines

New patch on top of previous one is appended.

From 98741f07ab1bc4a1fc2de7fedfb9023ea30bf988 Mon Sep 17 00:00:00 2001
From: Serge E. Hallyn <[email protected]>
Date: Thu, 1 Nov 2007 08:20:12 -0500
Subject: [PATCH 1/1] file capabilities: remove the non-matching uid special case for kill

Tested-by "Theodore Ts'o" <[email protected]>

Thanks, this fixes the issue I reported!

- Ted

#7 lore raw
In reply to Serge E. Hallyn

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 2 lines · Serge E. Hallyn
Quoted 1 line · Stephen Smalley

On Wed, 2007-10-31 at 18:49 -0500, Serge E. Hallyn wrote:

[..]

Quoted 9 lines
Quoted 4 lines
Quoted 3 lines

Also don't do file-capabilities signaling checks when uids for the processes don't match, since the standard check_kill_permission will have done those checks.

Description doesn't match the code.

Egads. I knew I should've just kept that part out of it for the first patch...

New patch on top of previous one is appended.

Dang! I stared at the code a long time to see what you were doing...

And concluded that you had coded what you intended; allow processes that share UIDs to kill one another - independent of capabilities. The fact that this is the reverse of the words you used to introduce your patch, I didn't notice.

I totally missed the fact that this was (unwanted) new functionality!! Mea culpa for the bad review.

I certainly Sign off the revised patch.

Cheers

Andrew
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFHLOicmwytjiwfWMwRAq5HAJ49eajMT4myf1oKfrab2oCw/o9HnwCgkYt2 RyIsmHVWmClsrxCz5s1HRJY=
=hGLO
-----END PGP SIGNATURE-----