Home » Mailing list archives » linux-kernel » 2007 » November » 2

Re: [PATCH] file capabilities: allow sigcont within session (v2)

Previous thread: [2.6 patch] the scheduled I2C RTC driver removal by Adrian Bunk on Wednesday, October 31, 2007 - 7:03 pm. (3 messages)

Next thread: [patch 3/7] Allocpercpu: Do __percpu_disguise() only if CONFIG_DEBUG_VM is set by Christoph Lameter on Wednesday, October 31, 2007 - 8:02 pm. (3 messages)
[view original message]
From: Serge E. Hallyn <serue@...>
To: lkml <linux-kernel@...>
Cc: <linux-security-module@...>, Andrew Morton <akpm@...>, Andrew Morgan <morgan@...>, Chris Wright <chrisw@...>, Theodore Ts'o <tytso@...>, Stephen Smalley <sds@...>, Rafael J. Wysocki <rjw@...>, Natalie Protasevich <protasnb@...>
Subject: [PATCH] file capabilities: allow sigcont within session (v2)
Date: Wednesday, October 31, 2007 - 7:49 pm

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

From: Serge E. Hallyn <serue@us.ibm.com>

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 <serue@us.ibm.com>

---

 security/commoncap.c |    9 +++++++++

 1 files changed, 9 insertions(+), 0 deletions(-)


diff --git a/security/commoncap.c b/security/commoncap.c

index bf67871..4de6857 100644

--- a/security/commoncap.c

+++ b/security/commoncap.c

@@ -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.

--

1.5.1.1.GIT


-
[view original message]
From: Stephen Smalley <sds@...>
To: Serge E. Hallyn <serue@...>
Cc: lkml <linux-kernel@...>, <linux-security-module@...>, Andrew Morton <akpm@...>, Andrew Morgan <morgan@...>, Chris Wright <chrisw@...>, Theodore Ts'o <tytso@...>, Rafael J. Wysocki <rjw@...>, Natalie Protasevich <protasnb@...>
Subject: Re: [PATCH] file capabilities: allow sigcont within session (v2)
Date: Thursday, November 1, 2007 - 8:07 am

Description doesn't match the code.  And in the non-matching uid case,

check_kill_permission typically returns an error before it reaches


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

--

Stephen Smalley

National Security Agency


-
[view original message]
From: Serge E. Hallyn <serue@...>
To: Stephen Smalley <sds@...>
Cc: Serge E. Hallyn <serue@...>, lkml <linux-kernel@...>, <linux-security-module@...>, Andrew Morton <akpm@...>, Andrew Morgan <morgan@...>, Chris Wright <chrisw@...>, Theodore Ts'o <tytso@...>, Rafael J. Wysocki <rjw@...>, Natalie Protasevich <protasnb@...>
Subject: Re: [PATCH] file capabilities: allow sigcont within session (v2)
Date: Thursday, November 1, 2007 - 9:47 am

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.


Typically, but when it doesn't, then the file capabilities shouldn't get

in the way of check_kill_permission() granting permission.  The file


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


Thanks, Stephen.


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

From: Serge E. Hallyn <serue@us.ibm.com>

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 <serue@us.ibm.com>

---

 security/commoncap.c |    5 -----

 1 files changed, 0 insertions(+), 5 deletions(-)


diff --git a/security/commoncap.c b/security/commoncap.c

index f04784a..302e8d0 100644

--- a/security/commoncap.c

+++ b/security/commoncap.c

@@ -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 == SIGCON...
[ message continues ]
[view original message]
From: Andrew Morgan <morgan@...>
To: Serge E. Hallyn <serue@...>
Cc: Stephen Smalley <sds@...>, lkml <linux-kernel@...>, <linux-security-module@...>, Andrew Morton <akpm@...>, Chris Wright <chrisw@...>, Theodore Ts'o <tytso@...>, Rafael J. Wysocki <rjw@...>, Natalie Protasevich <protasnb@...>
Subject: Re: [PATCH] file capabilities: allow sigcont within session (v2)
Date: Saturday, November 3, 2007 - 5:31 pm

-----BEGIN PGP SIGNED MESSAGE-----

Hash: SHA1


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-----

-
[view original message]
From: Theodore Tso <tytso@...>
To: Serge E. Hallyn <serue@...>
Cc: Stephen Smalley <sds@...>, lkml <linux-kernel@...>, <linux-security-module@...>, Andrew Morton <akpm@...>, Andrew Morgan <morgan@...>, Chris Wright <chrisw@...>, Rafael J. Wysocki <rjw@...>, Natalie Protasevich <protasnb@...>
Subject: Re: [PATCH] file capabilities: allow sigcont within session (v2)
Date: Thursday, November 1, 2007 - 9:54 pm

Tested-by: "Theodore Ts'o" <tytso@mit.edu>


Thanks, this fixes the issue I reported!


				- Ted

-
[view original message]
From: Theodore Tso <tytso@...>
To: Serge E. Hallyn <serue@...>
Cc: Stephen Smalley <sds@...>, lkml <linux-kernel@...>, <linux-security-module@...>, Andrew Morton <akpm@...>, Andrew Morgan <morgan@...>, Chris Wright <chrisw@...>, Rafael J. Wysocki <rjw@...>, Natalie Protasevich <protasnb@...>
Subject: Re: [PATCH] file capabilities: allow sigcont within session (v2)
Date: Thursday, November 1, 2007 - 4:12 pm

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

-
[view original message]
From: Andrew Morgan <morgan@...>
To: Serge E. Hallyn <serue@...>
Cc: lkml <linux-kernel@...>, <linux-security-module@...>, Andrew Morton <akpm@...>, Chris Wright <chrisw@...>, Theodore Ts'o <tytso@...>, Stephen Smalley <sds@...>, Rafael J. Wysocki <rjw@...>, Natalie Protasevich <protasnb@...>
Subject: Re: [PATCH] file capabilities: allow sigcont within session (v2)
Date: Thursday, November 1, 2007 - 12:47 am

-----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 <morgan@kernel.org>


Cheers


Andrew


-----BEGIN PGP SIGNATURE-----

Version: GnuPG v1.2.6 (GNU/Linux)


iD8DBQFHKVpRQheEq9QabfIRAnp9AKCZHb526eioQWKycH7V7LfcHP7VvQCdG0AJ

QTVOLvQ2hip+j2qZ1mb2Y6w=

=45et

-----END PGP SIGNATURE-----

-
[view original message]
From: Andrew Morgan <morgan@...>
To: Serge E. Hallyn <serue@...>
Cc: lkml <linux-kernel@...>, <linux-security-module@...>, Andrew Morton <akpm@...>, Chris Wright <chrisw@...>, Theodore Ts'o <tytso@...>, Stephen Smalley <sds@...>, Rafael J. Wysocki <rjw@...>, Natalie Protasevich <protasnb@...>
Subject: Re: [PATCH] file capabilities: allow sigcont within session (v2)
Date: Wednesday, October 31, 2007 - 9:27 pm

-----BEGIN PGP SIGNED MESSAGE-----

Hash: SHA1


Ackd.


Cheers


Andrew


-----BEGIN PGP SIGNATURE-----

Version: GnuPG v1.4.7 (Darwin)

Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org


iD8DBQFHKSuSmwytjiwfWMwRAkHXAJ4lr07yW936ychUGNxqQSOanZTecwCePYVc

d8uP0I3AEDjTpG8s7Nojo7Y=

=8777

-----END PGP SIGNATURE-----

-
Previous thread: [2.6 patch] the scheduled I2C RTC driver removal by Adrian Bunk on Wednesday, October 31, 2007 - 7:03 pm. (3 messages)

Next thread: [patch 3/7] Allocpercpu: Do __percpu_disguise() only if CONFIG_DEBUG_VM is set by Christoph Lameter on Wednesday, October 31, 2007 - 8:02 pm. (3 messages)