[PATCH] sysctl selinux: Don't look at table->de

[PATCH] sysctl selinux: Don't look at table->de

[Eric W. Biederman]

With the sysctl cleanups sysctl is not really a part of proc
it just shows up there, and any path based approach will not
adequately describe the data as sysctl is essentially a
union mount underneath the covers.  As designed this mechanism
is viewer dependent so trying to be path based gets even worse.

However the permissions in sys_sysctl are currently immutable
and going through proc does not change the permission checks
when accessing sysctl.  So we might as well stick with the well
defined sysctl sid, as that is what selinux uses when proc is
not compiled in.

I.e.  I see no hope for salvaging the selinux_proc_get_sid call
in selinux_sysctl so I'm removing it.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 security/selinux/hooks.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 7b38372..3a36057 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1438,12 +1438,8 @@ static int selinux_sysctl(ctl_table *table, int op)
 
 	tsec = current->security;
 
-	rc = selinux_proc_get_sid(table->de, (op == 001) ?
-	                          SECCLASS_DIR : SECCLASS_FILE, &tsid);
-	if (rc) {
-		/* Default to the well-defined sysctl SID. */
-		tsid = SECINITSID_SYSCTL;
-	}
+	/* Use the well-defined sysctl SID. */
+	tsid = SECINITSID_SYSCTL;
 
 	/* The op values are "defined" in sysctl.c, thereby creating
 	 * a bad coupling between this module and sysctl.c */
-- 
1.4.4.1.g278f

Re: [PATCH] sysctl selinux: Don't look at table->de

[Stephen Smalley]

On Sun, 2007-01-28 at 12:21 -0700, Eric W. Biederman wrote:
> With the sysctl cleanups sysctl is not really a part of proc
> it just shows up there, and any path based approach will not
> adequately describe the data as sysctl is essentially a
> union mount underneath the covers.  As designed this mechanism
> is viewer dependent so trying to be path based gets even worse.
> 
> However the permissions in sys_sysctl are currently immutable
> and going through proc does not change the permission checks
> when accessing sysctl.  So we might as well stick with the well
> defined sysctl sid, as that is what selinux uses when proc is
> not compiled in.
> 
> I.e.  I see no hope for salvaging the selinux_proc_get_sid call
> in selinux_sysctl so I'm removing it.
> 
> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
> ---
>  security/selinux/hooks.c |    8 ++------
>  1 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 7b38372..3a36057 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1438,12 +1438,8 @@ static int selinux_sysctl(ctl_table *table, int op)
>  
>  	tsec = current->security;
>  
> -	rc = selinux_proc_get_sid(table->de, (op == 001) ?
> -	                          SECCLASS_DIR : SECCLASS_FILE, &tsid);
> -	if (rc) {
> -		/* Default to the well-defined sysctl SID. */
> -		tsid = SECINITSID_SYSCTL;
> -	}
> +	/* Use the well-defined sysctl SID. */
> +	tsid = SECINITSID_SYSCTL;
>  
>  	/* The op values are "defined" in sysctl.c, thereby creating
>  	 * a bad coupling between this module and sysctl.c */

NAK.  Mapping all sysctls to a single security label prevents any kind
of fine-grained security on sysctls, and current policies already make
use of the current distinctions to limit access to particular sets of
sysctls to particular processes.  As is, I'd expect breakage of current
systems running SELinux from this patch, because (confined) processes
that formerly only required access to specific sysctl labels will
suddenly run into denials on the generic fallback label.

If the ctl_table supplied more information about the functional purpose
and the security sensitivity of the sysctl, then we could leverage that
information instead, as long as we can at least derive the current
labelings from that information for compatibility.

-- 
Stephen Smalley
National Security Agency

Re: [PATCH] sysctl selinux: Don't look at table->de

[James Morris]

On Mon, 29 Jan 2007, Stephen Smalley wrote:

> NAK.  Mapping all sysctls to a single security label prevents any kind
> of fine-grained security on sysctls, and current policies already make
> use of the current distinctions to limit access to particular sets of
> sysctls to particular processes.  As is, I'd expect breakage of current
> systems running SELinux from this patch, because (confined) processes
> that formerly only required access to specific sysctl labels will
> suddenly run into denials on the generic fallback label.

Agreed, 100% NACK.

Please don't just simply remove long-researched & analyzed MAC security 
which has been in the kernel for years, which is being used in the field 
for high assurance systems, because you neglected to consider it during a 
code cleanup.


- James
-- 
James Morris
<jmorris@namei.org>

Re: [PATCH] sysctl selinux: Don't look at table->de

[Eric W. Biederman]

James Morris <jmorris@namei.org> writes:

> On Mon, 29 Jan 2007, Stephen Smalley wrote:
>
>> NAK.  Mapping all sysctls to a single security label prevents any kind
>> of fine-grained security on sysctls, and current policies already make
>> use of the current distinctions to limit access to particular sets of
>> sysctls to particular processes.  As is, I'd expect breakage of current
>> systems running SELinux from this patch, because (confined) processes
>> that formerly only required access to specific sysctl labels will
>> suddenly run into denials on the generic fallback label.
>
> Agreed, 100% NACK.
>
> Please don't just simply remove long-researched & analyzed MAC security 
> which has been in the kernel for years, which is being used in the field 
> for high assurance systems, because you neglected to consider it during a 
> code cleanup.

Please don't shoot the messenger when a weakness is found in your code.
Systems that increase security without worry that their implementation
is correct do not impress me, but I do understand that security has
little to do with correctness and everything to do with making it
_expensive_ for the other guy to do what he isn't supposed to.

This code path was always in the selinux code for when /proc was
compiled out.  I could see no way to preserve it so I removed
it.

Not knowing if it was a problem, or if we needed to do something more
I copied the people who did, at the first available opportunity.
Before this code makes it's way into peoples production systems.

Of course after all of the rants against path based security I was
amazed to find a code path that was using exactly that in selinux.
Equally I'm amazed that all of that long-researched and analysis of
the MAC security has not found these issues where you integrate with
the rest of the linux kernel.

I'm trying to make things correct, and simple and will be happy to
work with you in a way to achieve what you need in a way that does
not conflict with the rest of the kernel.

Eric

Re: [PATCH] sysctl selinux: Don't look at table->de

[Stephen Smalley]

On Mon, 2007-01-29 at 10:55 -0700, Eric W. Biederman wrote:
> James Morris <jmorris@namei.org> writes:
> 
> > On Mon, 29 Jan 2007, Stephen Smalley wrote:
> >
> >> NAK.  Mapping all sysctls to a single security label prevents any kind
> >> of fine-grained security on sysctls, and current policies already make
> >> use of the current distinctions to limit access to particular sets of
> >> sysctls to particular processes.  As is, I'd expect breakage of current
> >> systems running SELinux from this patch, because (confined) processes
> >> that formerly only required access to specific sysctl labels will
> >> suddenly run into denials on the generic fallback label.
> >
> > Agreed, 100% NACK.
> >
> > Please don't just simply remove long-researched & analyzed MAC security 
> > which has been in the kernel for years, which is being used in the field 
> > for high assurance systems, because you neglected to consider it during a 
> > code cleanup.
> 
> Please don't shoot the messenger when a weakness is found in your code.

I'm not sure how breaking our code with your set of patches qualifies as
finding a weakness.  I will agree that the current handling of sysctl in
selinux is fragile and can be improved, but it did work (prior to your
patches).

> Systems that increase security without worry that their implementation
> is correct do not impress me, but I do understand that security has
> little to do with correctness and everything to do with making it
> _expensive_ for the other guy to do what he isn't supposed to.

I think you misunderstand; we are concerned about the correctness of our
implementation.  I think that possibly you are misunderstanding one of
the SELinux FAQ answers outside of its historical context (the initial
release of a proof-of-concept implementation of flexible MAC in Dec
2000).

> This code path was always in the selinux code for when /proc was
> compiled out.  I could see no way to preserve it so I removed
> it.
> 
> Not knowing if it was a problem, or if we needed to do something more
> I copied the people who did, at the first available opportunity.
> Before this code makes it's way into peoples production systems.

Which we appreciate, although it would be nice if you tried building
with selinux (and ideally testing with it) in the first place.

> Of course after all of the rants against path based security I was
> amazed to find a code path that was using exactly that in selinux.

To clarify, in this case, the pathname (relative to the root of proc) is
derived from the proc_dir_entry hierarchy and is thus not ambiguous or
mutable by userspace, unlike the pathname-based approaches that we have
criticized.  There is a difference.  But we are open to improving the
approach via explicit marking of the ctl_table entries with sufficient
information.

> I'm trying to make things correct, and simple and will be happy to
> work with you in a way to achieve what you need in a way that does
> not conflict with the rest of the kernel.

Good.

-- 
Stephen Smalley
National Security Agency

Re: [PATCH] sysctl selinux: Don't look at table->de

[Eric W. Biederman]

Stephen Smalley <sds@tycho.nsa.gov> writes:

> On Sun, 2007-01-28 at 12:21 -0700, Eric W. Biederman wrote:
>> With the sysctl cleanups sysctl is not really a part of proc
>> it just shows up there, and any path based approach will not
>> adequately describe the data as sysctl is essentially a
>> union mount underneath the covers.  As designed this mechanism
>> is viewer dependent so trying to be path based gets even worse.
>> 
>> However the permissions in sys_sysctl are currently immutable
>> and going through proc does not change the permission checks
>> when accessing sysctl.  So we might as well stick with the well
>> defined sysctl sid, as that is what selinux uses when proc is
>> not compiled in.
>> 
>> I.e.  I see no hope for salvaging the selinux_proc_get_sid call
>> in selinux_sysctl so I'm removing it.
>> 
>> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
>> ---
>>  security/selinux/hooks.c |    8 ++------
>>  1 files changed, 2 insertions(+), 6 deletions(-)
>> 
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 7b38372..3a36057 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -1438,12 +1438,8 @@ static int selinux_sysctl(ctl_table *table, int op)
>>  
>>  	tsec = current->security;
>>  
>> -	rc = selinux_proc_get_sid(table->de, (op == 001) ?
>> -	                          SECCLASS_DIR : SECCLASS_FILE, &tsid);
>> -	if (rc) {
>> -		/* Default to the well-defined sysctl SID. */
>> -		tsid = SECINITSID_SYSCTL;
>> -	}
>> +	/* Use the well-defined sysctl SID. */
>> +	tsid = SECINITSID_SYSCTL;
>>  
>>  	/* The op values are "defined" in sysctl.c, thereby creating
>>  	 * a bad coupling between this module and sysctl.c */
>
> NAK.  Mapping all sysctls to a single security label prevents any kind
> of fine-grained security on sysctls, and current policies already make
> use of the current distinctions to limit access to particular sets of
> sysctls to particular processes.  As is, I'd expect breakage of current
> systems running SELinux from this patch, because (confined) processes
> that formerly only required access to specific sysctl labels will
> suddenly run into denials on the generic fallback label.

Reasonable.  There is the issue that your code already had this code
path for when /proc was compiled out.

> If the ctl_table supplied more information about the functional purpose
> and the security sensitivity of the sysctl, then we could leverage that
> information instead, as long as we can at least derive the current
> labelings from that information for compatibility.

What do information do you need to do need?  Do you need extra fields in sysctl?
I am more than willing to help but I am not familiar enough with selinux
to do a reasonable job on my own.

Eric

Re: [PATCH] sysctl selinux: Don't look at table->de

[Stephen Smalley]

On Mon, 2007-01-29 at 10:43 -0700, Eric W. Biederman wrote:
> Stephen Smalley <sds@tycho.nsa.gov> writes:
> 
> > On Sun, 2007-01-28 at 12:21 -0700, Eric W. Biederman wrote:
> >> With the sysctl cleanups sysctl is not really a part of proc
> >> it just shows up there, and any path based approach will not
> >> adequately describe the data as sysctl is essentially a
> >> union mount underneath the covers.  As designed this mechanism
> >> is viewer dependent so trying to be path based gets even worse.
> >> 
> >> However the permissions in sys_sysctl are currently immutable
> >> and going through proc does not change the permission checks
> >> when accessing sysctl.  So we might as well stick with the well
> >> defined sysctl sid, as that is what selinux uses when proc is
> >> not compiled in.
> >> 
> >> I.e.  I see no hope for salvaging the selinux_proc_get_sid call
> >> in selinux_sysctl so I'm removing it.
> >> 
> >> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
> >> ---
> >>  security/selinux/hooks.c |    8 ++------
> >>  1 files changed, 2 insertions(+), 6 deletions(-)
> >> 
> >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >> index 7b38372..3a36057 100644
> >> --- a/security/selinux/hooks.c
> >> +++ b/security/selinux/hooks.c
> >> @@ -1438,12 +1438,8 @@ static int selinux_sysctl(ctl_table *table, int op)
> >>  
> >>  	tsec = current->security;
> >>  
> >> -	rc = selinux_proc_get_sid(table->de, (op == 001) ?
> >> -	                          SECCLASS_DIR : SECCLASS_FILE, &tsid);
> >> -	if (rc) {
> >> -		/* Default to the well-defined sysctl SID. */
> >> -		tsid = SECINITSID_SYSCTL;
> >> -	}
> >> +	/* Use the well-defined sysctl SID. */
> >> +	tsid = SECINITSID_SYSCTL;
> >>  
> >>  	/* The op values are "defined" in sysctl.c, thereby creating
> >>  	 * a bad coupling between this module and sysctl.c */
> >
> > NAK.  Mapping all sysctls to a single security label prevents any kind
> > of fine-grained security on sysctls, and current policies already make
> > use of the current distinctions to limit access to particular sets of
> > sysctls to particular processes.  As is, I'd expect breakage of current
> > systems running SELinux from this patch, because (confined) processes
> > that formerly only required access to specific sysctl labels will
> > suddenly run into denials on the generic fallback label.
> 
> Reasonable.  There is the issue that your code already had this code
> path for when /proc was compiled out.

True, but a system that disables proc is likely a system with a custom
policy anyway, and dependency on proc is fairly basic to selinux these
days (due to reliance on /proc/self/attr for process attribute
manipulation in place of the old selinux syscalls).  Possibly we should
just make selinux depend on proc and drop the #ifdef there.

> > If the ctl_table supplied more information about the functional purpose
> > and the security sensitivity of the sysctl, then we could leverage that
> > information instead, as long as we can at least derive the current
> > labelings from that information for compatibility.
> 
> What do information do you need to do need?  Do you need extra fields in sysctl?
> I am more than willing to help but I am not familiar enough with selinux
> to do a reasonable job on my own.

At present, we map the sysctls into functional groups (e.g. net, vm,
fs, ...) that parallel the sysctl hierarchy so that we can limit access
to only those programs/processes that need access for their purpose, and
further partition where it makes sense to do so.  We also separate out
particularly security sensitive ones like modprobe and hotplug.  So if
the ctl_table carried some indication of functional grouping and
security relevance (for some relatively small number of equivalence
classes), then we could map those to labels instead of the current
scheme.  And if we could have the ctl_table inherit the information from
its logical "parent" in the hierarchy by default, then it shouldn't
require too invasive a patch.

-- 
Stephen Smalley
National Security Agency

Re: [PATCH] sysctl selinux: Don't look at table->de

[Casey Schaufler]

--- Stephen Smalley <sds@tycho.nsa.gov> wrote:

> True, but a system that disables proc is likely a
> system with a custom
> policy anyway, and dependency on proc is fairly
> basic to selinux these
> days (due to reliance on /proc/self/attr for process
> attribute
> manipulation in place of the old selinux syscalls). 
> Possibly we should
> just make selinux depend on proc and drop the #ifdef
> there.

Alternativly you could move the SELinux specific
bits out of /proc/self/attr into an equivalent
/selinux/self/attr and avoid that /proc dependency.



Casey Schaufler
casey@schaufler-ca.com

Re: [PATCH] sysctl selinux: Don't look at table->de

[Stephen Smalley]

On Mon, 2007-01-29 at 11:08 -0800, Casey Schaufler wrote:
> --- Stephen Smalley <sds@tycho.nsa.gov> wrote:
> 
> > True, but a system that disables proc is likely a
> > system with a custom
> > policy anyway, and dependency on proc is fairly
> > basic to selinux these
> > days (due to reliance on /proc/self/attr for process
> > attribute
> > manipulation in place of the old selinux syscalls). 
> > Possibly we should
> > just make selinux depend on proc and drop the #ifdef
> > there.
> 
> Alternativly you could move the SELinux specific
> bits out of /proc/self/attr into an equivalent
> /selinux/self/attr and avoid that /proc dependency.

We could, but I don't see any compelling reason to do so.  We were
specifically told to use proc for the selinux process attributes when we
refactored the selinux api for 2.6 inclusion, as they are per-process
state and fit naturally into proc.

-- 
Stephen Smalley
National Security Agency

Re: [PATCH] sysctl selinux: Don't look at table->de

[Christoph Hellwig]

On Mon, Jan 29, 2007 at 11:08:39AM -0800, Casey Schaufler wrote:
> Alternativly you could move the SELinux specific
> bits out of /proc/self/attr into an equivalent
> /selinux/self/attr and avoid that /proc dependency.

Why?  procfs is essential for any kind of fullblown linux system,
and the selinux actually fits into the original procfs intention,
unlike all the other junk we've added over the years.

Re: [PATCH] sysctl selinux: Don't look at table->de

[Casey Schaufler]

--- Christoph Hellwig <hch@infradead.org> wrote:

> On Mon, Jan 29, 2007 at 11:08:39AM -0800, Casey
> Schaufler wrote:
> > Alternativly you could move the SELinux specific
> > bits out of /proc/self/attr into an equivalent
> > /selinux/self/attr and avoid that /proc
> dependency.
> 
> Why?

To avoid the dependency and remove the issue.
Just a thought. Weigh the advantages and
disadvantages and do what seems right.

> procfs is essential for any kind of fullblown
> linux system,

Kids!

> and the selinux actually fits into the original
> procfs intention,

Yes, it does. It's a good use of /proc.

> unlike all the other junk we've added over the
> years.

You won't get any arguements from me there.


Casey Schaufler
casey@schaufler-ca.com

Re: [PATCH] sysctl selinux: Don't look at table->de

[Eric W. Biederman]

Stephen Smalley <sds@tycho.nsa.gov> writes:


>> > If the ctl_table supplied more information about the functional purpose
>> > and the security sensitivity of the sysctl, then we could leverage that
>> > information instead, as long as we can at least derive the current
>> > labelings from that information for compatibility.
>> 
>> What do information do you need to do need?  Do you need extra fields in
> sysctl?
>> I am more than willing to help but I am not familiar enough with selinux
>> to do a reasonable job on my own.
>
> At present, we map the sysctls into functional groups (e.g. net, vm,
> fs, ...) that parallel the sysctl hierarchy so that we can limit access
> to only those programs/processes that need access for their purpose, and
> further partition where it makes sense to do so.  We also separate out
> particularly security sensitive ones like modprobe and hotplug.  So if
> the ctl_table carried some indication of functional grouping and
> security relevance (for some relatively small number of equivalence
> classes), then we could map those to labels instead of the current
> scheme.  And if we could have the ctl_table inherit the information from
> its logical "parent" in the hierarchy by default, then it shouldn't
> require too invasive a patch.

Ok.  So basically what you need is a parent pointer or some other way
of getting the full sysctl_path.   All of the names that show up in /proc
are still present in the ctl_table.

Hmm.  In parse_table we actually call sysctl_perm at each path component,
I'm not doing that in proc_sysctl.c at the moment but that would be easy to
add.

I think I will look at adding the back pointers.  Adding the security
check during lookup is nice but it won't really give you the context
you could use.  There may be a point in adding a security check during
lookup as well, but I think the way the VFS works there are weird implications
there.

Eric

Re: [PATCH] sysctl selinux: Don't look at table->de

[Russell Coker]

On Tuesday 30 January 2007 05:43, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> True, but a system that disables proc is likely a system with a custom
> policy anyway,

In practice we have to extensively customise policy long before getting to the 
non-proc stage of optimising for small hardware.  The Familiar distribution 
(used on the iPaQ) has /proc but needs significant policy changes when 
compared to a typical Fedora workstation.  Not only is there the issue that 
embedded distributions have different daemons and path names to workstations, 
but the memory constraints mean that even a modular targeted policy is not as 
small as you desire.

> and dependency on proc is fairly basic to selinux these 
> days (due to reliance on /proc/self/attr for process attribute
> manipulation in place of the old selinux syscalls).  Possibly we should
> just make selinux depend on proc and drop the #ifdef there.

I think that is the correct thing to do.  Someone who is prepared to do all 
the work needed to get a recent SE Linux system operating without /proc will 
have no problem changing the kernel config scripts and everyone else would be 
better off not being confused by being offered sets of options that are not 
viable.

-- 
russell@coker.com.au
http://etbe.blogspot.com/          My Blog

http://www.coker.com.au/sponsorship.html Sponsoring Free Software development

[PATCH 1/2] sysctl: Add a parent entry to ctl_table and set the parent entry.

[Eric W. Biederman]

Add a parent entry into the ctl_table so you can walk the list of
parents and find the entire path to a ctl_table entry.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---

This is an incremental patch on top of my previous sysctl work.

 include/linux/sysctl.h |    1 +
 kernel/sysctl.c        |   10 ++++++++++
 2 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 286e723..24f36f1 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -1024,6 +1024,7 @@ struct ctl_table
 	int maxlen;
 	mode_t mode;
 	ctl_table *child;
+	ctl_table *parent;		/* Automatically set */
 	proc_handler *proc_handler;	/* Callback for text formatting */
 	ctl_handler *strategy;		/* Callback function for all r/w */
 	void *extra1;
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index ae6a424..0a5499f 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1232,6 +1232,15 @@ int do_sysctl_strategy (ctl_table *table,
 }
 #endif /* CONFIG_SYSCTL_SYSCALL */
 
+static void sysctl_set_parent(struct ctl_table *parent, struct ctl_table *table)
+{
+	for (; table->ctl_name || table->procname; table++) {
+		table->parent = parent;
+		if (table->child)
+			sysctl_set_parent(table, table->child);
+	}
+}
+
 /**
  * register_sysctl_table - register a sysctl hierarchy
  * @table: the top-level table structure
@@ -1311,6 +1320,7 @@ static struct ctl_table_header *__register_sysctl_table(
 	INIT_LIST_HEAD(&tmp->ctl_entry);
 	tmp->used = 0;
 	tmp->unregistering = NULL;
+	sysctl_set_parent(NULL, table);
 	spin_lock(&sysctl_lock);
 	list_add_tail(&tmp->ctl_entry, &root->ctl_entry);
 	spin_unlock(&sysctl_lock);
-- 
1.4.4.1.g278f

[PATCH 2/2] sysctl: Restore the selinux path based label lookup for sysctls.

[Eric W. Biederman]

This time instead of generating the generating the paths from proc_dir_entries
generate the labels from the names in the sysctl ctl_tables themselves.  This
removes an unnecessary layer of indirection, allows this to work even when
procfs support is not compiled into the kernel, and especially allows it
to work now that ctl_tables no longer have a proc_dir_entry field.

I continue passing "proc" into genfs sid although that is complete nonsense
to allow existing selinux policies to work without modification.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 security/selinux/hooks.c |   43 +++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 3a36057..c17a8dd 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1424,6 +1424,41 @@ static int selinux_capable(struct task_struct *tsk, int cap)
 	return task_has_capability(tsk,cap);
 }
 
+static int selinux_sysctl_get_sid(ctl_table *table, u16 tclass, u32 *sid)
+{
+	int buflen, rc;
+	char *buffer, *path, *end;
+
+	rc = -ENOMEM;
+	buffer = (char*)__get_free_page(GFP_KERNEL);
+	if (!buffer)
+		goto out;
+
+	buflen = PAGE_SIZE;
+	end = buffer+buflen;
+	*--end = '\0';
+	buflen--;
+	path = end-1;
+	*path = '/';
+	while (table) {
+		const char *name = table->procname;
+		size_t namelen = strlen(name);
+		buflen -= namelen + 1;
+		if (buflen < 0)
+			goto out_free;
+		end -= namelen;
+		memcpy(end, name, namelen);
+		*--end = '/';
+		path = end;
+		table = table->parent;
+	}
+	rc = security_genfs_sid("proc", path, tclass, sid);
+out_free:
+	free_page((unsigned long)buffer);
+out:
+	return rc;
+}
+
 static int selinux_sysctl(ctl_table *table, int op)
 {
 	int error = 0;
@@ -1438,8 +1473,12 @@ static int selinux_sysctl(ctl_table *table, int op)
 
 	tsec = current->security;
 
-	/* Use the well-defined sysctl SID. */
-	tsid = SECINITSID_SYSCTL;
+	rc = selinux_sysctl_get_sid(table, (op == 0001) ?
+				    SECCLASS_DIR : SECCLASS_FILE, &tsid);
+	if (rc) {
+		/* Default to the well-defined sysctl SID. */
+		tsid = SECINITSID_SYSCTL;
+	}
 
 	/* The op values are "defined" in sysctl.c, thereby creating
 	 * a bad coupling between this module and sysctl.c */
-- 
1.4.4.1.g278f

Re: [PATCH 2/2] sysctl: Restore the selinux path based label lookup for sysctls.

[Stephen Smalley]

On Tue, 2007-02-06 at 14:21 -0700, Eric W. Biederman wrote:
> This time instead of generating the generating the paths from proc_dir_entries
> generate the labels from the names in the sysctl ctl_tables themselves.  This
> removes an unnecessary layer of indirection, allows this to work even when
> procfs support is not compiled into the kernel, and especially allows it
> to work now that ctl_tables no longer have a proc_dir_entry field.

Thanks, looks sane.

> I continue passing "proc" into genfs sid although that is complete nonsense
> to allow existing selinux policies to work without modification.
> 
> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>

Acked-by:  Stephen Smalley <sds@tycho.nsa.gov>

> ---
>  security/selinux/hooks.c |   43 +++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 3a36057..c17a8dd 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1424,6 +1424,41 @@ static int selinux_capable(struct task_struct *tsk, int cap)
>  	return task_has_capability(tsk,cap);
>  }
>  
> +static int selinux_sysctl_get_sid(ctl_table *table, u16 tclass, u32 *sid)
> +{
> +	int buflen, rc;
> +	char *buffer, *path, *end;
> +
> +	rc = -ENOMEM;
> +	buffer = (char*)__get_free_page(GFP_KERNEL);
> +	if (!buffer)
> +		goto out;
> +
> +	buflen = PAGE_SIZE;
> +	end = buffer+buflen;
> +	*--end = '\0';
> +	buflen--;
> +	path = end-1;
> +	*path = '/';
> +	while (table) {
> +		const char *name = table->procname;
> +		size_t namelen = strlen(name);
> +		buflen -= namelen + 1;
> +		if (buflen < 0)
> +			goto out_free;
> +		end -= namelen;
> +		memcpy(end, name, namelen);
> +		*--end = '/';
> +		path = end;
> +		table = table->parent;
> +	}
> +	rc = security_genfs_sid("proc", path, tclass, sid);
> +out_free:
> +	free_page((unsigned long)buffer);
> +out:
> +	return rc;
> +}
> +
>  static int selinux_sysctl(ctl_table *table, int op)
>  {
>  	int error = 0;
> @@ -1438,8 +1473,12 @@ static int selinux_sysctl(ctl_table *table, int op)
>  
>  	tsec = current->security;
>  
> -	/* Use the well-defined sysctl SID. */
> -	tsid = SECINITSID_SYSCTL;
> +	rc = selinux_sysctl_get_sid(table, (op == 0001) ?
> +				    SECCLASS_DIR : SECCLASS_FILE, &tsid);
> +	if (rc) {
> +		/* Default to the well-defined sysctl SID. */
> +		tsid = SECINITSID_SYSCTL;
> +	}
>  
>  	/* The op values are "defined" in sysctl.c, thereby creating
>  	 * a bad coupling between this module and sysctl.c */
-- 
Stephen Smalley
National Security Agency

Re: [PATCH 2/2] sysctl: Restore the selinux path based label lookup for sysctls.

[Stephen Smalley]

On Wed, 2007-02-07 at 13:24 -0500, Stephen Smalley wrote:
> On Tue, 2007-02-06 at 14:21 -0700, Eric W. Biederman wrote:
> > This time instead of generating the generating the paths from proc_dir_entries
> > generate the labels from the names in the sysctl ctl_tables themselves.  This
> > removes an unnecessary layer of indirection, allows this to work even when
> > procfs support is not compiled into the kernel, and especially allows it
> > to work now that ctl_tables no longer have a proc_dir_entry field.
> 
> Thanks, looks sane.
> 
> > I continue passing "proc" into genfs sid although that is complete nonsense
> > to allow existing selinux policies to work without modification.
> > 
> > Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
> 
> Acked-by:  Stephen Smalley <sds@tycho.nsa.gov>

Hmmm...but in testing the patch, I don't seem to (consistently) reach
these checks when accessing via /proc/sys.  I see that you are caching
the mode information and using it in some cases rather than calling the
sysctl_perm function.

One related but separate issue is that the /proc/sys inode labeling is
also affected by the sysctl patch series.  Those inodes used to be
labeled by selinux_proc_get_sid (from selinux_d_instantiate), but that
no longer works, so they now fall back to the superblock SID (generic
proc label).  That changes the inode permission checks on an attempt to
access a /proc/sys node and will likely cause denials under current
policy for confined domains since one wouldn't generally be writing to
the generic proc label. If you always called sysctl_perm from the proc
sysctl code, we could possibly dispense with inode permission checking
on those inodes, e.g. marking them private.
  
> 
> > ---
> >  security/selinux/hooks.c |   43 +++++++++++++++++++++++++++++++++++++++++--
> >  1 files changed, 41 insertions(+), 2 deletions(-)
> > 
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 3a36057..c17a8dd 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -1424,6 +1424,41 @@ static int selinux_capable(struct task_struct *tsk, int cap)
> >  	return task_has_capability(tsk,cap);
> >  }
> >  
> > +static int selinux_sysctl_get_sid(ctl_table *table, u16 tclass, u32 *sid)
> > +{
> > +	int buflen, rc;
> > +	char *buffer, *path, *end;
> > +
> > +	rc = -ENOMEM;
> > +	buffer = (char*)__get_free_page(GFP_KERNEL);
> > +	if (!buffer)
> > +		goto out;
> > +
> > +	buflen = PAGE_SIZE;
> > +	end = buffer+buflen;
> > +	*--end = '\0';
> > +	buflen--;
> > +	path = end-1;
> > +	*path = '/';
> > +	while (table) {
> > +		const char *name = table->procname;
> > +		size_t namelen = strlen(name);
> > +		buflen -= namelen + 1;
> > +		if (buflen < 0)
> > +			goto out_free;
> > +		end -= namelen;
> > +		memcpy(end, name, namelen);
> > +		*--end = '/';
> > +		path = end;
> > +		table = table->parent;
> > +	}
> > +	rc = security_genfs_sid("proc", path, tclass, sid);
> > +out_free:
> > +	free_page((unsigned long)buffer);
> > +out:
> > +	return rc;
> > +}
> > +
> >  static int selinux_sysctl(ctl_table *table, int op)
> >  {
> >  	int error = 0;
> > @@ -1438,8 +1473,12 @@ static int selinux_sysctl(ctl_table *table, int op)
> >  
> >  	tsec = current->security;
> >  
> > -	/* Use the well-defined sysctl SID. */
> > -	tsid = SECINITSID_SYSCTL;
> > +	rc = selinux_sysctl_get_sid(table, (op == 0001) ?
> > +				    SECCLASS_DIR : SECCLASS_FILE, &tsid);
> > +	if (rc) {
> > +		/* Default to the well-defined sysctl SID. */
> > +		tsid = SECINITSID_SYSCTL;
> > +	}
> >  
> >  	/* The op values are "defined" in sysctl.c, thereby creating
> >  	 * a bad coupling between this module and sysctl.c */
-- 
Stephen Smalley
National Security Agency

Re: [PATCH 2/2] sysctl: Restore the selinux path based label lookup for sysctls.

[Stephen Smalley]

On Wed, 2007-02-07 at 16:12 -0500, Stephen Smalley wrote:
> On Wed, 2007-02-07 at 13:24 -0500, Stephen Smalley wrote:
> > On Tue, 2007-02-06 at 14:21 -0700, Eric W. Biederman wrote:
> > > This time instead of generating the generating the paths from proc_dir_entries
> > > generate the labels from the names in the sysctl ctl_tables themselves.  This
> > > removes an unnecessary layer of indirection, allows this to work even when
> > > procfs support is not compiled into the kernel, and especially allows it
> > > to work now that ctl_tables no longer have a proc_dir_entry field.
> > 
> > Thanks, looks sane.
> > 
> > > I continue passing "proc" into genfs sid although that is complete nonsense
> > > to allow existing selinux policies to work without modification.
> > > 
> > > Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
> > 
> > Acked-by:  Stephen Smalley <sds@tycho.nsa.gov>
> 
> Hmmm...but in testing the patch, I don't seem to (consistently) reach
> these checks when accessing via /proc/sys.  I see that you are caching
> the mode information and using it in some cases rather than calling the
> sysctl_perm function.

Actually, on further inspection, it looks like the real issue is the
"path" name generation; "cat /proc/sys/kernel/modprobe" yields a call to
security_genfs_sid() with just "/modprobe" rather than the expected
"/sys/kernel/modprobe".  Which likewise leaves us with the generic proc
label, just as with the inode permission check, so I end up seeing
checks against it only.

> One related but separate issue is that the /proc/sys inode labeling is
> also affected by the sysctl patch series.  Those inodes used to be
> labeled by selinux_proc_get_sid (from selinux_d_instantiate), but that
> no longer works, so they now fall back to the superblock SID (generic
> proc label).  That changes the inode permission checks on an attempt to
> access a /proc/sys node and will likely cause denials under current
> policy for confined domains since one wouldn't generally be writing to
> the generic proc label. If you always called sysctl_perm from the proc
> sysctl code, we could possibly dispense with inode permission checking
> on those inodes, e.g. marking them private.

-- 
Stephen Smalley
National Security Agency

Re: [PATCH 2/2] sysctl: Restore the selinux path based label lookup for sysctls.

[Eric W. Biederman]

Stephen Smalley <sds@tycho.nsa.gov> writes:

> Actually, on further inspection, it looks like the real issue is the
> "path" name generation; "cat /proc/sys/kernel/modprobe" yields a call to
> security_genfs_sid() with just "/modprobe" rather than the expected
> "/sys/kernel/modprobe".  Which likewise leaves us with the generic proc
> label, just as with the inode permission check, so I end up seeing
> checks against it only.

Ok.  It looks like two silly thing are going on here.
I failed to register the root sysctl table, so none of the parent
pointers got set.

I didn't prepend /sys in the compatibility code, so for something with
the parent pointers set you would have gotten "/kernel/modprobe" instead
of /sys/kernel/modprobe"

Sorry about that.

I think the patch below will fix it.

Eric

diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 24f36f1..f316854 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -929,8 +929,6 @@ extern struct ctl_table_header *sysctl_head_next(struct ctl_table_header *prev);
 extern void sysctl_head_finish(struct ctl_table_header *prev);
 extern int sysctl_perm(struct ctl_table *table, int op);
 
-extern void sysctl_init(void);
-
 typedef struct ctl_table ctl_table;
 
 typedef int ctl_handler (ctl_table *table, int __user *name, int nlen,
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 0a5499f..0bb2c5f 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1241,6 +1241,14 @@ static void sysctl_set_parent(struct ctl_table *parent, struct ctl_table *table)
 	}
 }
 
+static __init int sysctl_init(void)
+{
+	sysctl_set_parent(NULL, root_table);
+	return 0;
+}
+
+core_initcall(sysctl_init);
+
 /**
  * register_sysctl_table - register a sysctl hierarchy
  * @table: the top-level table structure
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index c17a8dd..aad2697 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1452,6 +1452,12 @@ static int selinux_sysctl_get_sid(ctl_table *table, u16 tclass, u32 *sid)
 		path = end;
 		table = table->parent;
 	}
+	buflen -= 4;
+	if (buflen < 0)
+		goto out_free;
+	end -= 4;
+	memcpy(end, "/sys", 4);
+	path = end;
 	rc = security_genfs_sid("proc", path, tclass, sid);
 out_free:
 	free_page((unsigned long)buffer);

Re: [PATCH 2/2] sysctl: Restore the selinux path based label lookup for sysctls.

[Stephen Smalley]

On Wed, 2007-02-07 at 15:21 -0700, Eric W. Biederman wrote:
> Stephen Smalley <sds@tycho.nsa.gov> writes:
> 
> > Actually, on further inspection, it looks like the real issue is the
> > "path" name generation; "cat /proc/sys/kernel/modprobe" yields a call to
> > security_genfs_sid() with just "/modprobe" rather than the expected
> > "/sys/kernel/modprobe".  Which likewise leaves us with the generic proc
> > label, just as with the inode permission check, so I end up seeing
> > checks against it only.
> 
> Ok.  It looks like two silly thing are going on here.
> I failed to register the root sysctl table, so none of the parent
> pointers got set.
> 
> I didn't prepend /sys in the compatibility code, so for something with
> the parent pointers set you would have gotten "/kernel/modprobe" instead
> of /sys/kernel/modprobe"
> 
> Sorry about that.
> 
> I think the patch below will fix it.

Yes, thanks - this appears to correct the name generation and thus the
resulting SID computation for the selinux sysctl checks.  

> Eric
> 
> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> index 24f36f1..f316854 100644
> --- a/include/linux/sysctl.h
> +++ b/include/linux/sysctl.h
> @@ -929,8 +929,6 @@ extern struct ctl_table_header *sysctl_head_next(struct ctl_table_header *prev);
>  extern void sysctl_head_finish(struct ctl_table_header *prev);
>  extern int sysctl_perm(struct ctl_table *table, int op);
>  
> -extern void sysctl_init(void);
> -
>  typedef struct ctl_table ctl_table;
>  
>  typedef int ctl_handler (ctl_table *table, int __user *name, int nlen,
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 0a5499f..0bb2c5f 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -1241,6 +1241,14 @@ static void sysctl_set_parent(struct ctl_table *parent, struct ctl_table *table)
>  	}
>  }
>  
> +static __init int sysctl_init(void)
> +{
> +	sysctl_set_parent(NULL, root_table);
> +	return 0;
> +}
> +
> +core_initcall(sysctl_init);
> +
>  /**
>   * register_sysctl_table - register a sysctl hierarchy
>   * @table: the top-level table structure
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index c17a8dd..aad2697 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1452,6 +1452,12 @@ static int selinux_sysctl_get_sid(ctl_table *table, u16 tclass, u32 *sid)
>  		path = end;
>  		table = table->parent;
>  	}
> +	buflen -= 4;
> +	if (buflen < 0)
> +		goto out_free;
> +	end -= 4;
> +	memcpy(end, "/sys", 4);
> +	path = end;
>  	rc = security_genfs_sid("proc", path, tclass, sid);
>  out_free:
>  	free_page((unsigned long)buffer);
> 


-- 
Stephen Smalley
National Security Agency

Re: [PATCH 2/2] sysctl: Restore the selinux path based label lookup for sysctls.

[Eric W. Biederman]

Stephen Smalley <sds@tycho.nsa.gov> writes:

>
> One related but separate issue is that the /proc/sys inode labeling is
> also affected by the sysctl patch series.  Those inodes used to be
> labeled by selinux_proc_get_sid (from selinux_d_instantiate), but that
> no longer works, so they now fall back to the superblock SID (generic
> proc label).  That changes the inode permission checks on an attempt to
> access a /proc/sys node and will likely cause denials under current
> policy for confined domains since one wouldn't generally be writing to
> the generic proc label. If you always called sysctl_perm from the proc
> sysctl code, we could possibly dispense with inode permission checking
> on those inodes, e.g. marking them private.

Like this?

It seems a little weird but I'm happy with it if you are.

Eric

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index b9d59c0..7d6f7c7 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -47,6 +47,7 @@ static struct inode *proc_sys_make_inode(struct inode *dir, struct ctl_table *ta
 	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
 	inode->i_op = &proc_sys_inode_operations;
 	inode->i_fop = &proc_sys_file_operations;
+	inode->i_flags |= S_PRIVATE; /* tell selinux to ignore this inode */
 	proc_sys_refresh_inode(inode, table);
 out:
 	return inode;

Re: [PATCH 2/2] sysctl: Restore the selinux path based label lookup for sysctls.

[Stephen Smalley]

On Wed, 2007-02-07 at 18:57 -0700, Eric W. Biederman wrote:
> Stephen Smalley <sds@tycho.nsa.gov> writes:
> 
> >
> > One related but separate issue is that the /proc/sys inode labeling is
> > also affected by the sysctl patch series.  Those inodes used to be
> > labeled by selinux_proc_get_sid (from selinux_d_instantiate), but that
> > no longer works, so they now fall back to the superblock SID (generic
> > proc label).  That changes the inode permission checks on an attempt to
> > access a /proc/sys node and will likely cause denials under current
> > policy for confined domains since one wouldn't generally be writing to
> > the generic proc label. If you always called sysctl_perm from the proc
> > sysctl code, we could possibly dispense with inode permission checking
> > on those inodes, e.g. marking them private.
> 
> Like this?
> 
> It seems a little weird but I'm happy with it if you are.
> 
> Eric
> 
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index b9d59c0..7d6f7c7 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -47,6 +47,7 @@ static struct inode *proc_sys_make_inode(struct inode *dir, struct ctl_table *ta
>  	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
>  	inode->i_op = &proc_sys_inode_operations;
>  	inode->i_fop = &proc_sys_file_operations;
> +	inode->i_flags |= S_PRIVATE; /* tell selinux to ignore this inode */
>  	proc_sys_refresh_inode(inode, table);
>  out:
>  	return inode;

Hmmm...turns out to not be quite enough, as the /proc/sys inodes aren't
truly private to the fs, so we can run into them in a variety of
security hooks beyond just the inode hooks, such as
security_file_permission (when reading and writing them via the vfs
helpers), security_sb_mount (when mounting other filesystems on
directories in proc like binfmt_misc), and deeper within the security
module itself (as in flush_unauthorized_files upon inheritance across
execve).  So I think we have to add an IS_PRIVATE() guard within
SELinux, as below.  Note however that the use of the private flag here
could be confusing, as these inodes are _not_ private to the fs, are
exposed to userspace, and security modules must implement the sysctl
hook to get any access control over them.

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 65fb5e8..21bf2f0 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1078,6 +1077,9 @@ static int inode_has_perm(struct task_st
 	struct inode_security_struct *isec;
 	struct avc_audit_data ad;
 
+	if (unlikely (IS_PRIVATE (inode)))
+		return 0;
+
 	tsec = tsk->security;
 	isec = inode->i_security;
 


-- 
Stephen Smalley
National Security Agency

Re: [PATCH 2/2] sysctl: Restore the selinux path based label lookup for sysctls.

[Eric W. Biederman]

Stephen Smalley <sds@tycho.nsa.gov> writes:

>
> Hmmm...turns out to not be quite enough, as the /proc/sys inodes aren't
> truly private to the fs, so we can run into them in a variety of
> security hooks beyond just the inode hooks, such as
> security_file_permission (when reading and writing them via the vfs
> helpers), security_sb_mount (when mounting other filesystems on
> directories in proc like binfmt_misc), and deeper within the security
> module itself (as in flush_unauthorized_files upon inheritance across
> execve).  So I think we have to add an IS_PRIVATE() guard within
> SELinux, as below.  Note however that the use of the private flag here
> could be confusing, as these inodes are _not_ private to the fs, are
> exposed to userspace, and security modules must implement the sysctl
> hook to get any access control over them.

Agreed, the naming is confusing, and using private here doesn't quite
feel right.

A practical question is: Will we ever encounter these inodes
in the inode_init() path from superblock_init?  If all of the accesses
that we care about go through inode_doinit_with_dentry we can just
walk the dcache to get the names, and that should work for the normal
proc case as well.

A somewhat related question: How do you handle security labels for
sysfs?  No fine grained security yet.

If it doesn't look easy to solve this another way I will certainly
go with marking the inodes private.

Eric

Re: [PATCH 2/2] sysctl: Restore the selinux path based label lookup for sysctls.

[Stephen Smalley]

On Thu, 2007-02-08 at 10:53 -0700, Eric W. Biederman wrote:
> Stephen Smalley <sds@tycho.nsa.gov> writes:
> 
> >
> > Hmmm...turns out to not be quite enough, as the /proc/sys inodes aren't
> > truly private to the fs, so we can run into them in a variety of
> > security hooks beyond just the inode hooks, such as
> > security_file_permission (when reading and writing them via the vfs
> > helpers), security_sb_mount (when mounting other filesystems on
> > directories in proc like binfmt_misc), and deeper within the security
> > module itself (as in flush_unauthorized_files upon inheritance across
> > execve).  So I think we have to add an IS_PRIVATE() guard within
> > SELinux, as below.  Note however that the use of the private flag here
> > could be confusing, as these inodes are _not_ private to the fs, are
> > exposed to userspace, and security modules must implement the sysctl
> > hook to get any access control over them.
> 
> Agreed, the naming is confusing, and using private here doesn't quite
> feel right.
> 
> A practical question is: Will we ever encounter these inodes
> in the inode_init() path from superblock_init?

Possibly, during setup upon initial policy load (initiated by /sbin/init
these days) from selinux_complete_init, as early userspace may have
already been accessing them.

>   If all of the accesses
> that we care about go through inode_doinit_with_dentry we can just
> walk the dcache to get the names, and that should work for the normal
> proc case as well.

Walking the proc_dir_entry tree (or the ctl_table tree) is preferable as
it is a stable, user-immutable representation.  Also avoids taking the
dcache lock.

> A somewhat related question: How do you handle security labels for
> sysfs?  No fine grained security yet.

Right, they are all mapped to a single label presently.  I was thinking
of handling that from userspace after introducing a setxattr handler for
sysfs and a way to preserve the SID on the entry (likely caching it in
the sysfs_dirent and propagating that to the inode when the inode is
populated from the sysfs_dirent).  Then early userspace could walk sysfs
and apply finer-grained labeling from a configuration.

> If it doesn't look easy to solve this another way I will certainly
> go with marking the inodes private.

-- 
Stephen Smalley
National Security Agency

Re: [PATCH 2/2] sysctl: Restore the selinux path based label lookup for sysctls.

[Eric W. Biederman]

Stephen Smalley <sds@tycho.nsa.gov> writes:

> Possibly, during setup upon initial policy load (initiated by /sbin/init
> these days) from selinux_complete_init, as early userspace may have
> already been accessing them.

I believe if we chose we could walk the dentry tree under the root inode
and find all of these.

>>   If all of the accesses
>> that we care about go through inode_doinit_with_dentry we can just
>> walk the dcache to get the names, and that should work for the normal
>> proc case as well.
>
> Walking the proc_dir_entry tree (or the ctl_table tree) is preferable as
> it is a stable, user-immutable representation.  Also avoids taking the
> dcache lock.

The dcache lock is valid.  Since the per filesystem dentry tree is just
a mirror of the filesystem data there is no advantage over using
the proc_dir_entry or ctl_table tree.  (If you start messing with mounts
that is another matter.

>> If it doesn't look easy to solve this another way I will certainly
>> go with marking the inodes private.

I hereby conclude this doesn't look easy enough to solve another way,
to get it solved in a timely manner.  Since the cost is only 2 lines
of code to use private inodes if we want to fix this later it should
not be difficult.

Eric

[PATCH 0/5] sysctl cleanup selinux fixes

[Eric W. Biederman]

Andrew these are a set up simple fixes against the rollup you
sent me last night that should leave selinux working and should
close out the last of the outstanding issue with my sysctl cleanup.

Knowing how you organize I expect you will want to file most of
these as fix patches.

Patch 1 sysctl: Remove declaration of nonexistent sysctl_init
This is a small fix against "sysctl: Reimplement the sysctl proc support"
where I remove sysctl_init() but forgot to remove it's prototype in
sysctl.h


Patch 2 sysctl: Set the parent field in the root sysctl table
This is a small fix against 
"sysctl: Add a parent entry to ctl_table and set the parent entry."
where I forgot to call sysctl_set_parent on the root sysctl table.


Patch 3: sysctl: Fix selinux_sysctl_getsid
This is a small fix against
"sysctl: Restore the old selinux sysctl handling."

Patch 4: selinux: Enhance selinux to always ignore private inodes
Patch 5: sysctl: Hide the sysctl proc inodes from selinux

Eric

[PATCH 1/5] sysctl: Remove declaration of nonexistent sysctl_init()

[Eric W. Biederman]

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 include/linux/sysctl.h |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 538f6d2..b5c2b3e 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -929,8 +929,6 @@ extern struct ctl_table_header *sysctl_head_next(struct ctl_table_header *prev);
 extern void sysctl_head_finish(struct ctl_table_header *prev);
 extern int sysctl_perm(struct ctl_table *table, int op);
 
-extern void sysctl_init(void);
-
 typedef struct ctl_table ctl_table;
 
 typedef int ctl_handler (ctl_table *table, int __user *name, int nlen,
-- 
1.4.4.1.g278f

[PATCH 2/5] sysctl: Set the parent field in the root sysctl table

[Eric W. Biederman]

Oops I goofed and forgot to set the parent in the root sysctl table,
the rest are covered by register_sysctl_table.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 kernel/sysctl.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index d3a84fc..aa7d69e 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1296,6 +1296,14 @@ static void sysctl_set_parent(struct ctl_table *parent, struct ctl_table *table)
 	}
 }
 
+static __init int sysctl_init(void)
+{
+	sysctl_set_parent(NULL, root_table);
+	return 0;
+}
+
+core_initcall(sysctl_init);
+
 /**
  * register_sysctl_table - register a sysctl hierarchy
  * @table: the top-level table structure
-- 
1.4.4.1.g278f

[PATCH 3/5] sysctl: Fix the selinux_sysctl_get_sid

[Eric W. Biederman]

I goofed and when reenabling the fine grained selinux labels for
sysctls and forgot to add the "/sys" prefix before consulting
the policy database.  When computing the same path using
proc_dir_entries we got the "/sys" for free as it was part
of the tree, but it isn't true for clt_table trees.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 security/selinux/hooks.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 47fb937..de16b9f 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1445,6 +1445,12 @@ static int selinux_sysctl_get_sid(ctl_table *table, u16 tclass, u32 *sid)
 		path = end;
 		table = table->parent;
 	}
+	buflen -= 4;
+	if (buflen < 0)
+		goto out_free;
+	end -= 4;
+	memcpy(end, "/sys", 4);
+	path = end;
 	rc = security_genfs_sid("proc", path, tclass, sid);
 out_free:
 	free_page((unsigned long)buffer);
-- 
1.4.4.1.g278f

[PATCH 4/5] selinux: Enhance selinux to always ignore private inodes.

[Eric W. Biederman]

From: Stephen Smalley <sds@tycho.nsa.gov>

Hmmm...turns out to not be quite enough, as the /proc/sys inodes aren't
truly private to the fs, so we can run into them in a variety of
security hooks beyond just the inode hooks, such as
security_file_permission (when reading and writing them via the vfs
helpers), security_sb_mount (when mounting other filesystems on
directories in proc like binfmt_misc), and deeper within the security
module itself (as in flush_unauthorized_files upon inheritance across
execve).  So I think we have to add an IS_PRIVATE() guard within
SELinux, as below.  Note however that the use of the private flag here
could be confusing, as these inodes are _not_ private to the fs, are
exposed to userspace, and security modules must implement the sysctl
hook to get any access control over them.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>

---
 security/selinux/hooks.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index de16b9f..ff9fccc 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1077,6 +1077,9 @@ static int inode_has_perm(struct task_struct *tsk,
 	struct inode_security_struct *isec;
 	struct avc_audit_data ad;
 
+	if (unlikely (IS_PRIVATE (inode)))
+		return 0;
+
 	tsec = tsk->security;
 	isec = inode->i_security;
 
-- 

[PATCH 5/5] sysctl: Hide the sysctl proc inodes from selinux.

[Eric W. Biederman]

Since the security checks are applied on each read and write of a
sysctl file, just like they are applied when calling sys_sysctl, they
are redundant on the standard VFS constructs.  Since it is difficult
to compute the security labels on the standard VFS constructs we just
mark the sysctl inodes in proc private so selinux won't even bother
with them.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 fs/proc/proc_sysctl.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index bb16a1e..20e8cbb 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -47,6 +47,7 @@ static struct inode *proc_sys_make_inode(struct inode *dir, struct ctl_table *ta
 	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
 	inode->i_op = &proc_sys_inode_operations;
 	inode->i_fop = &proc_sys_file_operations;
+	inode->i_flags |= S_PRIVATE; /* tell selinux to ignore this inode */
 	proc_sys_refresh_inode(inode, table);
 out:
 	return inode;
-- 
1.4.4.1.g278f

Re: [PATCH 4/5] selinux: Enhance selinux to always ignore private inodes.

[Stephen Smalley]

On Thu, 2007-02-08 at 16:02 -0700, Eric W. Biederman wrote:
> From: Stephen Smalley <sds@tycho.nsa.gov>
> 
> Hmmm...turns out to not be quite enough, as the /proc/sys inodes aren't
> truly private to the fs, so we can run into them in a variety of
> security hooks beyond just the inode hooks, such as
> security_file_permission (when reading and writing them via the vfs
> helpers), security_sb_mount (when mounting other filesystems on
> directories in proc like binfmt_misc), and deeper within the security
> module itself (as in flush_unauthorized_files upon inheritance across
> execve).  So I think we have to add an IS_PRIVATE() guard within
> SELinux, as below.  Note however that the use of the private flag here
> could be confusing, as these inodes are _not_ private to the fs, are
> exposed to userspace, and security modules must implement the sysctl
> hook to get any access control over them.

Signed-off-by:  Stephen Smalley <sds@tycho.nsa.gov>

> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
> 
> ---
>  security/selinux/hooks.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index de16b9f..ff9fccc 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1077,6 +1077,9 @@ static int inode_has_perm(struct task_struct *tsk,
>  	struct inode_security_struct *isec;
>  	struct avc_audit_data ad;
>  
> +	if (unlikely (IS_PRIVATE (inode)))
> +		return 0;
> +
>  	tsec = tsk->security;
>  	isec = inode->i_security;
>  
-- 
Stephen Smalley
National Security Agency

Re: [PATCH 3/5] sysctl: Fix the selinux_sysctl_get_sid

[Stephen Smalley]

On Thu, 2007-02-08 at 15:55 -0700, Eric W. Biederman wrote:
> I goofed and when reenabling the fine grained selinux labels for
> sysctls and forgot to add the "/sys" prefix before consulting
> the policy database.  When computing the same path using
> proc_dir_entries we got the "/sys" for free as it was part
> of the tree, but it isn't true for clt_table trees.
> 
> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>

Acked-by:  Stephen Smalley <sds@tycho.nsa.gov>

> ---
>  security/selinux/hooks.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 47fb937..de16b9f 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1445,6 +1445,12 @@ static int selinux_sysctl_get_sid(ctl_table *table, u16 tclass, u32 *sid)
>  		path = end;
>  		table = table->parent;
>  	}
> +	buflen -= 4;
> +	if (buflen < 0)
> +		goto out_free;
> +	end -= 4;
> +	memcpy(end, "/sys", 4);
> +	path = end;
>  	rc = security_genfs_sid("proc", path, tclass, sid);
>  out_free:
>  	free_page((unsigned long)buffer);
-- 
Stephen Smalley
National Security Agency

Re: [PATCH 0/5] sysctl cleanup selinux fixes

[Andrew Morton]

On Thu, 08 Feb 2007 15:51:15 -0700 ebiederm@xmission.com (Eric W. Biederman) wrote:

> Patch 3: sysctl: Fix selinux_sysctl_getsid
> This is a small fix against
> "sysctl: Restore the old selinux sysctl handling."
> 

I cannot locate such a patch.  I just gave up and plastered
the fixes at end-of-series.

Re: [PATCH 0/5] sysctl cleanup selinux fixes

[Eric W. Biederman]

Andrew Morton <akpm@linux-foundation.org> writes:

> On Thu, 08 Feb 2007 15:51:15 -0700 ebiederm@xmission.com (Eric W. Biederman)
> wrote:
>
>> Patch 3: sysctl: Fix selinux_sysctl_getsid
>> This is a small fix against
>> "sysctl: Restore the old selinux sysctl handling."
>> 
>
> I cannot locate such a patch.  I just gave up and plastered
> the fixes at end-of-series.

Ok.  Sorry.  I'm guessing one of us (probably me) renamed that one.  I have
this habit of rewriting my comments as they go out the door, so they are
more readable.

Eric