restorecon and symbolic links

restorecon and symbolic links

[Martin Orr]

With policycoreutils 2.0.71, "restorecon /dev/stdin" fails if stdin is a pipe:
martin@caligula:~$ echo hi | sudo restorecon /dev/stdin
realpath(/dev/stdin) failed No such file or directory

Why would you want to do this?
The Debian udev init script does
ln -s /proc/self/fd/0 /dev/stdin
restorecon /dev/stdin
I am not sure why stdin is a pipe here but it is some consequence of the
boot process.

The intention here (and what happened with policycoreutils 2.0.69) is to
relabel the symbolic link.  But the recent realpath patch changed this, and
I don't think there is a way now to ask restorecon to relabel an individual
symlink.

-- 
Martin Orr

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: restorecon and symbolic links

[Manoj Srivastava]

On Sat, Aug 29 2009, Martin Orr wrote:

> With policycoreutils 2.0.71, "restorecon /dev/stdin" fails if stdin is a pipe:
> martin@caligula:~$ echo hi | sudo restorecon /dev/stdin
> realpath(/dev/stdin) failed No such file or directory
>
> Why would you want to do this?
> The Debian udev init script does
> ln -s /proc/self/fd/0 /dev/stdin
> restorecon /dev/stdin
> I am not sure why stdin is a pipe here but it is some consequence of the
> boot process.
>
> The intention here (and what happened with policycoreutils 2.0.69) is to
> relabel the symbolic link.  But the recent realpath patch changed this, and
> I don't think there is a way now to ask restorecon to relabel an individual
> symlink.

        There are consequences to this change not mentioned above: when
 booting with policycoreutils 2.0.71 /dev/pts (and several other device
 nodes) are not created which causes all sorts of trouble.

 This is a consequence of the realpath changes in restorecon, because
 when /lib/udev/create_static_nodes does
         ln -s /proc/self/fd/0 /dev/stdin
         restorecon /dev/stdin
 it now fails with the error
         realpath(/dev/stdin) failed No such file or directory
 This causes create_static_nodes to exit (due to set -e) before creating
 /dev/pts.

        I am planning on reverting the removal of special treatment of
 symlinks from the debian  unstable version until this is resolved.

        manoj
-- 
Manoj Srivastava <srivasta@acm.org> <http://www.golden-gryphon.com/>  
1024D/BF24424C print 4966 F272 D093 B493 410B  924B 21BA DABB BF24 424C

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: restorecon and symbolic links

[Stephen Smalley]

On Sat, 2009-08-29 at 18:19 -0500, Manoj Srivastava wrote:
> On Sat, Aug 29 2009, Martin Orr wrote:
> 
> > With policycoreutils 2.0.71, "restorecon /dev/stdin" fails if stdin is a pipe:
> > martin@caligula:~$ echo hi | sudo restorecon /dev/stdin
> > realpath(/dev/stdin) failed No such file or directory
> >
> > Why would you want to do this?
> > The Debian udev init script does
> > ln -s /proc/self/fd/0 /dev/stdin
> > restorecon /dev/stdin
> > I am not sure why stdin is a pipe here but it is some consequence of the
> > boot process.
> >
> > The intention here (and what happened with policycoreutils 2.0.69) is to
> > relabel the symbolic link.  But the recent realpath patch changed this, and
> > I don't think there is a way now to ask restorecon to relabel an individual
> > symlink.
> 
>         There are consequences to this change not mentioned above: when
>  booting with policycoreutils 2.0.71 /dev/pts (and several other device
>  nodes) are not created which causes all sorts of trouble.
> 
>  This is a consequence of the realpath changes in restorecon, because
>  when /lib/udev/create_static_nodes does
>          ln -s /proc/self/fd/0 /dev/stdin
>          restorecon /dev/stdin
>  it now fails with the error
>          realpath(/dev/stdin) failed No such file or directory
>  This causes create_static_nodes to exit (due to set -e) before creating
>  /dev/pts.
> 
>         I am planning on reverting the removal of special treatment of
>  symlinks from the debian  unstable version until this is resolved.

The particular commit was:

commit 37c5c30998b73d9c6efe53fd0534256463991c5e
Author: Stephen Smalley <sds@tycho.nsa.gov>
Date:   Mon Aug 3 09:34:52 2009 -0400

    setfiles: only call realpath() on user-supplied pathnames
    
    Change setfiles/restorecon to only call realpath() on the user-supplied
    pathnames prior to invoking fts_open().  This ensures that commands such
    as restorecon -R /etc/init.d and (cd /etc && restorecon shadow gshadow)
    will work as expected while avoiding the overhead of calling realpath()
    on each file during a file tree walk.
    
    Since we are now only acting on user-supplied pathnames, drop the
    special case handling of symlinks (when a user invokes restorecon
    -R /etc/init.d he truly wants it to descend /etc/rc.d/init.d).  We can
    also defer allocation of the pathname buffer to libc by passing NULL
    (freeing on the out path) and we can drop the redundant exclude() check
    as it will now get handled on the normal path.
    
    Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>

What behavior would you like instead?  I'd still like to have restorecon
-R /etc/init.d to do what the user expects, which isn't just relabel the
symlink.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: restorecon and symbolic links

[Stephen Smalley]

On Mon, 2009-08-31 at 08:17 -0400, Stephen Smalley wrote:
> On Sat, 2009-08-29 at 18:19 -0500, Manoj Srivastava wrote:
> > On Sat, Aug 29 2009, Martin Orr wrote:
> > 
> > > With policycoreutils 2.0.71, "restorecon /dev/stdin" fails if stdin is a pipe:
> > > martin@caligula:~$ echo hi | sudo restorecon /dev/stdin
> > > realpath(/dev/stdin) failed No such file or directory
> > >
> > > Why would you want to do this?
> > > The Debian udev init script does
> > > ln -s /proc/self/fd/0 /dev/stdin
> > > restorecon /dev/stdin
> > > I am not sure why stdin is a pipe here but it is some consequence of the
> > > boot process.
> > >
> > > The intention here (and what happened with policycoreutils 2.0.69) is to
> > > relabel the symbolic link.  But the recent realpath patch changed this, and
> > > I don't think there is a way now to ask restorecon to relabel an individual
> > > symlink.
> > 
> >         There are consequences to this change not mentioned above: when
> >  booting with policycoreutils 2.0.71 /dev/pts (and several other device
> >  nodes) are not created which causes all sorts of trouble.
> > 
> >  This is a consequence of the realpath changes in restorecon, because
> >  when /lib/udev/create_static_nodes does
> >          ln -s /proc/self/fd/0 /dev/stdin
> >          restorecon /dev/stdin
> >  it now fails with the error
> >          realpath(/dev/stdin) failed No such file or directory
> >  This causes create_static_nodes to exit (due to set -e) before creating
> >  /dev/pts.
> > 
> >         I am planning on reverting the removal of special treatment of
> >  symlinks from the debian  unstable version until this is resolved.
> 
> The particular commit was:
> 
> commit 37c5c30998b73d9c6efe53fd0534256463991c5e
> Author: Stephen Smalley <sds@tycho.nsa.gov>
> Date:   Mon Aug 3 09:34:52 2009 -0400
> 
>     setfiles: only call realpath() on user-supplied pathnames
>     
>     Change setfiles/restorecon to only call realpath() on the user-supplied
>     pathnames prior to invoking fts_open().  This ensures that commands such
>     as restorecon -R /etc/init.d and (cd /etc && restorecon shadow gshadow)
>     will work as expected while avoiding the overhead of calling realpath()
>     on each file during a file tree walk.
>     
>     Since we are now only acting on user-supplied pathnames, drop the
>     special case handling of symlinks (when a user invokes restorecon
>     -R /etc/init.d he truly wants it to descend /etc/rc.d/init.d).  We can
>     also defer allocation of the pathname buffer to libc by passing NULL
>     (freeing on the out path) and we can drop the redundant exclude() check
>     as it will now get handled on the normal path.
>     
>     Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
> 
> What behavior would you like instead?  I'd still like to have restorecon
> -R /etc/init.d to do what the user expects, which isn't just relabel the
> symlink.

Also, I'm a little unclear as to why we need the restorecon /dev/stdin
above.  Why isn't /dev/stdin created in the right type to begin with?

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: restorecon and symbolic links

[Martin Orr]

On 31/08/09 13:17, Stephen Smalley wrote:
> On Sat, 2009-08-29 at 18:19 -0500, Manoj Srivastava wrote:
>> On Sat, Aug 29 2009, Martin Orr wrote:
>>
>>> With policycoreutils 2.0.71, "restorecon /dev/stdin" fails if stdin is a pipe:
>>> martin@caligula:~$ echo hi | sudo restorecon /dev/stdin
>>> realpath(/dev/stdin) failed No such file or directory
>>>
>>> Why would you want to do this?
>>> The Debian udev init script does
>>> ln -s /proc/self/fd/0 /dev/stdin
>>> restorecon /dev/stdin
>>> I am not sure why stdin is a pipe here but it is some consequence of the
>>> boot process.
>>>
>>> The intention here (and what happened with policycoreutils 2.0.69) is to
>>> relabel the symbolic link.  But the recent realpath patch changed this, and
>>> I don't think there is a way now to ask restorecon to relabel an individual
>>> symlink.
>>         There are consequences to this change not mentioned above: when
>>  booting with policycoreutils 2.0.71 /dev/pts (and several other device
>>  nodes) are not created which causes all sorts of trouble.
>>
>>  This is a consequence of the realpath changes in restorecon, because
>>  when /lib/udev/create_static_nodes does
>>          ln -s /proc/self/fd/0 /dev/stdin
>>          restorecon /dev/stdin
>>  it now fails with the error
>>          realpath(/dev/stdin) failed No such file or directory
>>  This causes create_static_nodes to exit (due to set -e) before creating
>>  /dev/pts.
>>
>>         I am planning on reverting the removal of special treatment of
>>  symlinks from the debian  unstable version until this is resolved.
> 
> The particular commit was:
> 
> commit 37c5c30998b73d9c6efe53fd0534256463991c5e
> Author: Stephen Smalley <sds@tycho.nsa.gov>
> Date:   Mon Aug 3 09:34:52 2009 -0400
> 
>     setfiles: only call realpath() on user-supplied pathnames
>     
>     Change setfiles/restorecon to only call realpath() on the user-supplied
>     pathnames prior to invoking fts_open().  This ensures that commands such
>     as restorecon -R /etc/init.d and (cd /etc && restorecon shadow gshadow)
>     will work as expected while avoiding the overhead of calling realpath()
>     on each file during a file tree walk.
>     
>     Since we are now only acting on user-supplied pathnames, drop the
>     special case handling of symlinks (when a user invokes restorecon
>     -R /etc/init.d he truly wants it to descend /etc/rc.d/init.d).  We can
>     also defer allocation of the pathname buffer to libc by passing NULL
>     (freeing on the out path) and we can drop the redundant exclude() check
>     as it will now get handled on the normal path.
>     
>     Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
> 
> What behavior would you like instead?  I'd still like to have restorecon
> -R /etc/init.d to do what the user expects, which isn't just relabel the
> symlink.

True, but I would expect restorecon -R /etc/init.d to relabel the symlink as
well as descend into the directory.  On the whole I would expect it not to
relabel the target of the symlink itself but I don't have a strong opinion
on that, as long as it doesn't fail if the symlink is broken.

-- 
Martin Orr

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: restorecon and symbolic links

[Martin Orr]

On 31/08/09 13:20, Stephen Smalley wrote:
> On Mon, 2009-08-31 at 08:17 -0400, Stephen Smalley wrote:
>> On Sat, 2009-08-29 at 18:19 -0500, Manoj Srivastava wrote:
>>> On Sat, Aug 29 2009, Martin Orr wrote:
>>>
>>>> With policycoreutils 2.0.71, "restorecon /dev/stdin" fails if stdin is a pipe:
>>>> martin@caligula:~$ echo hi | sudo restorecon /dev/stdin
>>>> realpath(/dev/stdin) failed No such file or directory
>>>>
>>>> Why would you want to do this?
>>>> The Debian udev init script does
>>>> ln -s /proc/self/fd/0 /dev/stdin
>>>> restorecon /dev/stdin
>>>> I am not sure why stdin is a pipe here but it is some consequence of the
>>>> boot process.
>>>>
>>>> The intention here (and what happened with policycoreutils 2.0.69) is to
>>>> relabel the symbolic link.  But the recent realpath patch changed this, and
>>>> I don't think there is a way now to ask restorecon to relabel an individual
>>>> symlink.
>>>         There are consequences to this change not mentioned above: when
>>>  booting with policycoreutils 2.0.71 /dev/pts (and several other device
>>>  nodes) are not created which causes all sorts of trouble.
>>>
>>>  This is a consequence of the realpath changes in restorecon, because
>>>  when /lib/udev/create_static_nodes does
>>>          ln -s /proc/self/fd/0 /dev/stdin
>>>          restorecon /dev/stdin
>>>  it now fails with the error
>>>          realpath(/dev/stdin) failed No such file or directory
>>>  This causes create_static_nodes to exit (due to set -e) before creating
>>>  /dev/pts.
>>>
>>>         I am planning on reverting the removal of special treatment of
>>>  symlinks from the debian  unstable version until this is resolved.
>> The particular commit was:
>>
>> commit 37c5c30998b73d9c6efe53fd0534256463991c5e
>> Author: Stephen Smalley <sds@tycho.nsa.gov>
>> Date:   Mon Aug 3 09:34:52 2009 -0400
>>
>>     setfiles: only call realpath() on user-supplied pathnames
>>     
>>     Change setfiles/restorecon to only call realpath() on the user-supplied
>>     pathnames prior to invoking fts_open().  This ensures that commands such
>>     as restorecon -R /etc/init.d and (cd /etc && restorecon shadow gshadow)
>>     will work as expected while avoiding the overhead of calling realpath()
>>     on each file during a file tree walk.
>>     
>>     Since we are now only acting on user-supplied pathnames, drop the
>>     special case handling of symlinks (when a user invokes restorecon
>>     -R /etc/init.d he truly wants it to descend /etc/rc.d/init.d).  We can
>>     also defer allocation of the pathname buffer to libc by passing NULL
>>     (freeing on the out path) and we can drop the redundant exclude() check
>>     as it will now get handled on the normal path.
>>     
>>     Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
>>
>> What behavior would you like instead?  I'd still like to have restorecon
>> -R /etc/init.d to do what the user expects, which isn't just relabel the
>> symlink.
> 
> Also, I'm a little unclear as to why we need the restorecon /dev/stdin
> above.  Why isn't /dev/stdin created in the right type to begin with?

The same script creates various links, directories and device nodes which
need to be created before udev starts.  Some of these do need to be
relabelled, and it would increase complexity to relabel some but not others.
 Just doing restorecon -R /dev at the end of the script might be an
alternative however.

-- 
Martin Orr

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: restorecon and symbolic links

[Stephen Smalley]

On Mon, 2009-08-31 at 14:21 +0100, Martin Orr wrote:
> On 31/08/09 13:17, Stephen Smalley wrote:
> > On Sat, 2009-08-29 at 18:19 -0500, Manoj Srivastava wrote:
> >> On Sat, Aug 29 2009, Martin Orr wrote:
> >>
> >>> With policycoreutils 2.0.71, "restorecon /dev/stdin" fails if stdin is a pipe:
> >>> martin@caligula:~$ echo hi | sudo restorecon /dev/stdin
> >>> realpath(/dev/stdin) failed No such file or directory
> >>>
> >>> Why would you want to do this?
> >>> The Debian udev init script does
> >>> ln -s /proc/self/fd/0 /dev/stdin
> >>> restorecon /dev/stdin
> >>> I am not sure why stdin is a pipe here but it is some consequence of the
> >>> boot process.
> >>>
> >>> The intention here (and what happened with policycoreutils 2.0.69) is to
> >>> relabel the symbolic link.  But the recent realpath patch changed this, and
> >>> I don't think there is a way now to ask restorecon to relabel an individual
> >>> symlink.
> >>         There are consequences to this change not mentioned above: when
> >>  booting with policycoreutils 2.0.71 /dev/pts (and several other device
> >>  nodes) are not created which causes all sorts of trouble.
> >>
> >>  This is a consequence of the realpath changes in restorecon, because
> >>  when /lib/udev/create_static_nodes does
> >>          ln -s /proc/self/fd/0 /dev/stdin
> >>          restorecon /dev/stdin
> >>  it now fails with the error
> >>          realpath(/dev/stdin) failed No such file or directory
> >>  This causes create_static_nodes to exit (due to set -e) before creating
> >>  /dev/pts.
> >>
> >>         I am planning on reverting the removal of special treatment of
> >>  symlinks from the debian  unstable version until this is resolved.
> > 
> > The particular commit was:
> > 
> > commit 37c5c30998b73d9c6efe53fd0534256463991c5e
> > Author: Stephen Smalley <sds@tycho.nsa.gov>
> > Date:   Mon Aug 3 09:34:52 2009 -0400
> > 
> >     setfiles: only call realpath() on user-supplied pathnames
> >     
> >     Change setfiles/restorecon to only call realpath() on the user-supplied
> >     pathnames prior to invoking fts_open().  This ensures that commands such
> >     as restorecon -R /etc/init.d and (cd /etc && restorecon shadow gshadow)
> >     will work as expected while avoiding the overhead of calling realpath()
> >     on each file during a file tree walk.
> >     
> >     Since we are now only acting on user-supplied pathnames, drop the
> >     special case handling of symlinks (when a user invokes restorecon
> >     -R /etc/init.d he truly wants it to descend /etc/rc.d/init.d).  We can
> >     also defer allocation of the pathname buffer to libc by passing NULL
> >     (freeing on the out path) and we can drop the redundant exclude() check
> >     as it will now get handled on the normal path.
> >     
> >     Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
> > 
> > What behavior would you like instead?  I'd still like to have restorecon
> > -R /etc/init.d to do what the user expects, which isn't just relabel the
> > symlink.
> 
> True, but I would expect restorecon -R /etc/init.d to relabel the symlink as
> well as descend into the directory.  On the whole I would expect it not to
> relabel the target of the symlink itself but I don't have a strong opinion
> on that, as long as it doesn't fail if the symlink is broken.

How about this for a compromise?

Add a -h option to restorecon so that the user can explicitly ask to
relabel a symlink (as with chcon), but also fall back to proceeding with
the original path if realpath() fails, in which case we will relabel the
symlink if the path was a broken symlink.

diff --git a/policycoreutils/setfiles/setfiles.c b/policycoreutils/setfiles/setfiles.c
index 4c47f21..7ea7262 100644
--- a/policycoreutils/setfiles/setfiles.c
+++ b/policycoreutils/setfiles/setfiles.c
@@ -73,6 +73,7 @@ static int iamrestorecon;
 
 /* Behavior flags determined based on setfiles vs. restorecon */
 static int expand_realpath;  /* Expand paths via realpath. */
+static int follow_symlink; /* follow symlinks */
 static int abort_on_error; /* Abort the file tree walk upon an error. */
 static int add_assoc; /* Track inode associations for conflict detection. */
 static int fts_flags; /* Flags to fts, e.g. follow links, follow mounts */
@@ -547,7 +548,7 @@ int canoncon(char **contextp)
 
 static int process_one(char *name)
 {
-	int rc = 0;
+	int rc = 0, free_name = 0;
 	const char *namelist[2];
 	dev_t dev_num = 0;
 	FTS *fts_handle;
@@ -555,16 +556,23 @@ static int process_one(char *name)
 
 	if (expand_realpath) {
 		char *p;
-		p = realpath(name, NULL);
-		if (!p) {
-			fprintf(stderr, "realpath(%s) failed %s\n", name,
-				strerror(errno));
+		struct stat sb;
+		
+		rc = lstat(name, &sb);
+		if (rc < 0) {
+			fprintf(stderr, "%s:  lstat(%s) failed:  %s\n",
+				progname, name,	strerror(errno));
 			return -1;
 		}
-		name = p;
+		if (!S_ISLNK(sb.st_mode) || follow_symlink) {
+			p = realpath(name, NULL);
+			if (p) {
+				name = p;
+				free_name = 1;
+			}
+		}
 	}
 
-
 	if (!strcmp(name, "/"))
 		mass_relabel = 1;
 
@@ -619,7 +627,7 @@ out:
 	}
 	if (fts_handle)
 		fts_close(fts_handle);
-	if (expand_realpath)
+	if (free_name)
 		free(name);
 	return rc;
 
@@ -777,6 +785,7 @@ int main(int argc, char **argv)
 		iamrestorecon = 1;
 		recurse = 0;
 		expand_realpath = 1;
+		follow_symlink = 1;
 		abort_on_error = 0;
 		add_assoc = 0;
 		fts_flags = FTS_PHYSICAL;
@@ -792,7 +801,7 @@ int main(int argc, char **argv)
 	exclude_non_seclabel_mounts();
 
 	/* Process any options. */
-	while ((opt = getopt(argc, argv, "c:de:f:ilnpqrsvo:FRW0")) > 0) {
+	while ((opt = getopt(argc, argv, "c:de:f:hilnpqrsvo:FRW0")) > 0) {
 		switch (opt) {
 		case 'c':
 			{
@@ -843,6 +852,9 @@ int main(int argc, char **argv)
 		case 'd':
 			debug = 1;
 			break;
+		case 'h':
+			follow_symlink = 0;
+			break;
 		case 'i':
 			ignore_enoent = 1;
 			break;


-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: restorecon and symbolic links

[Martin Orr]

On 31/08/09 21:27, Stephen Smalley wrote:
> On Mon, 2009-08-31 at 14:21 +0100, Martin Orr wrote:
>> On 31/08/09 13:17, Stephen Smalley wrote:
>>> On Sat, 2009-08-29 at 18:19 -0500, Manoj Srivastava wrote:
>>>> On Sat, Aug 29 2009, Martin Orr wrote:
>>>>
>>>>> With policycoreutils 2.0.71, "restorecon /dev/stdin" fails if stdin is a pipe:
>>>>> martin@caligula:~$ echo hi | sudo restorecon /dev/stdin
>>>>> realpath(/dev/stdin) failed No such file or directory
>>>>>
>>>>> The intention here (and what happened with policycoreutils 2.0.69) is to
>>>>> relabel the symbolic link.  But the recent realpath patch changed this, and
>>>>> I don't think there is a way now to ask restorecon to relabel an individual
>>>>> symlink.
>>>
>>> The particular commit was:
>>>
>>> commit 37c5c30998b73d9c6efe53fd0534256463991c5e
>>> Author: Stephen Smalley <sds@tycho.nsa.gov>
>>> Date:   Mon Aug 3 09:34:52 2009 -0400
>>>
>>>     setfiles: only call realpath() on user-supplied pathnames
>>>     
>>>     Change setfiles/restorecon to only call realpath() on the user-supplied
>>>     pathnames prior to invoking fts_open().  This ensures that commands such
>>>     as restorecon -R /etc/init.d and (cd /etc && restorecon shadow gshadow)
>>>     will work as expected while avoiding the overhead of calling realpath()
>>>     on each file during a file tree walk.
>>>     
>>>     Since we are now only acting on user-supplied pathnames, drop the
>>>     special case handling of symlinks (when a user invokes restorecon
>>>     -R /etc/init.d he truly wants it to descend /etc/rc.d/init.d).  We can
>>>     also defer allocation of the pathname buffer to libc by passing NULL
>>>     (freeing on the out path) and we can drop the redundant exclude() check
>>>     as it will now get handled on the normal path.
>>>     
>>>     Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
>>>
>>> What behavior would you like instead?  I'd still like to have restorecon
>>> -R /etc/init.d to do what the user expects, which isn't just relabel the
>>> symlink.
>> True, but I would expect restorecon -R /etc/init.d to relabel the symlink as
>> well as descend into the directory.  On the whole I would expect it not to
>> relabel the target of the symlink itself but I don't have a strong opinion
>> on that, as long as it doesn't fail if the symlink is broken.
> 
> How about this for a compromise?
> 
> Add a -h option to restorecon so that the user can explicitly ask to
> relabel a symlink (as with chcon), but also fall back to proceeding with
> the original path if realpath() fails, in which case we will relabel the
> symlink if the path was a broken symlink.

I think that this is pretty complicated and the fallback behaviour is
potentially confusing.  I still think that restorecon -R /etc/init.d should
by default change the context on the symlink as well as descending into the
directory.

Also, I think it is necessary to restore the code to compute the real path
of all but the last component of a symlink when relabelling the link itself.

The following patch shows approximately the behaviour I would like, except
that it doesn't work, because apply_spec needs an FTSENT:

diff --git a/policycoreutils/setfiles/setfiles.c b/policycoreutils/setfiles/setfiles.c
index 4c47f21..4fd5583 100644
--- a/policycoreutils/setfiles/setfiles.c
+++ b/policycoreutils/setfiles/setfiles.c
@@ -545,6 +545,60 @@ int canoncon(char **contextp)
 	return rc;
 }
 
+static int process_symlink(char *name)
+{
+	int rc = 0;
+	char path[PATH_MAX + 1];
+
+	if (verbose > 1)
+		fprintf(stderr,
+			"Warning! %s refers to a symbolic link, not following last component.\n",
+			name);
+	char *p = NULL, *file_sep;
+	char *tmp_path = strdupa(name);
+	size_t len = 0;
+	if (!tmp_path) {
+		fprintf(stderr, "strdupa on %s failed:  %s\n", name,
+			strerror(errno));
+		return -1;
+	}
+	file_sep = strrchr(tmp_path, '/');
+	if (file_sep == tmp_path) {
+		file_sep++;
+		p = strcpy(path, "");
+	} else if (file_sep) {
+		*file_sep = 0;
+		file_sep++;
+		p = realpath(tmp_path, path);
+	} else {
+		file_sep = tmp_path;
+		p = realpath("./", path);
+	}
+	if (p)
+		len = strlen(p);
+	if (!p || len + strlen(file_sep) + 2 > PATH_MAX) {
+		fprintf(stderr, "symlink realpath(%s) failed %s\n", name,
+			strerror(errno));
+		return -1;
+	}
+	p += len;
+	/* ensure trailing slash of directory name */
+	if (len == 0 || *(p - 1) != '/') {
+		*p = '/';
+		p++;
+	}
+	strcpy(p, file_sep);
+	name = path;
+	if (excludeCtr > 0 && exclude(name))
+		return -1;
+
+	/*rc = apply_spec(name);*/ /* FIXME: Need a FTSENT for apply_spec */
+	fprintf(stderr, "Would restore symlink context: %s\n", name);
+	if (rc == ERR)
+		return -1;
+	return 0;
+}
+
 static int process_one(char *name)
 {
 	int rc = 0;
@@ -555,13 +609,35 @@ static int process_one(char *name)
 
 	if (expand_realpath) {
 		char *p;
-		p = realpath(name, NULL);
-		if (!p) {
-			fprintf(stderr, "realpath(%s) failed %s\n", name,
-				strerror(errno));
+		struct stat sb;
+
+		rc = lstat(name, &sb);
+		if (rc < 0) {
+			fprintf(stderr, "%s:  lstat(%s) failed:  %s\n",
+				progname, name,	strerror(errno));
 			return -1;
 		}
-		name = p;
+		if (S_ISLNK(sb.st_mode)) {
+			rc = process_symlink(name);
+			if (rc < 0)
+				return rc;
+
+			p = realpath(name, NULL);
+			if (!p) {
+				fprintf(stderr, "Warning: %s: Broken symlink: %s\n",
+					name, strerror(errno));
+				return 0;
+			}
+			name = p;
+		} else {
+			p = realpath(name, NULL);
+			if (!p) {
+				fprintf(stderr, "realpath(%s) failed %s\n", name,
+					strerror(errno));
+				return -1;
+			}
+			name = p;
+		}
 	}
 
 

-- 
Martin Orr

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: restorecon and symbolic links

[Martin Orr]

On 01/09/09 14:43, Martin Orr wrote:
> On 31/08/09 21:27, Stephen Smalley wrote:
>> On Mon, 2009-08-31 at 14:21 +0100, Martin Orr wrote:
>>> On 31/08/09 13:17, Stephen Smalley wrote:
>>>> On Sat, 2009-08-29 at 18:19 -0500, Manoj Srivastava wrote:
>>>>> On Sat, Aug 29 2009, Martin Orr wrote:
>>>>>
>>>>>> With policycoreutils 2.0.71, "restorecon /dev/stdin" fails if stdin is a pipe:
>>>>>> martin@caligula:~$ echo hi | sudo restorecon /dev/stdin
>>>>>> realpath(/dev/stdin) failed No such file or directory
>>>>>>
>>>>>> The intention here (and what happened with policycoreutils 2.0.69) is to
>>>>>> relabel the symbolic link.  But the recent realpath patch changed this, and
>>>>>> I don't think there is a way now to ask restorecon to relabel an individual
>>>>>> symlink.
>>>> The particular commit was:
>>>>
>>>> commit 37c5c30998b73d9c6efe53fd0534256463991c5e
>>>> Author: Stephen Smalley <sds@tycho.nsa.gov>
>>>> Date:   Mon Aug 3 09:34:52 2009 -0400
>>>>
>>>>     setfiles: only call realpath() on user-supplied pathnames
>>>>     
>>>>     Change setfiles/restorecon to only call realpath() on the user-supplied
>>>>     pathnames prior to invoking fts_open().  This ensures that commands such
>>>>     as restorecon -R /etc/init.d and (cd /etc && restorecon shadow gshadow)
>>>>     will work as expected while avoiding the overhead of calling realpath()
>>>>     on each file during a file tree walk.
>>>>     
>>>>     Since we are now only acting on user-supplied pathnames, drop the
>>>>     special case handling of symlinks (when a user invokes restorecon
>>>>     -R /etc/init.d he truly wants it to descend /etc/rc.d/init.d).  We can
>>>>     also defer allocation of the pathname buffer to libc by passing NULL
>>>>     (freeing on the out path) and we can drop the redundant exclude() check
>>>>     as it will now get handled on the normal path.
>>>>     
>>>>     Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
>>>>
>>>> What behavior would you like instead?  I'd still like to have restorecon
>>>> -R /etc/init.d to do what the user expects, which isn't just relabel the
>>>> symlink.
>>> True, but I would expect restorecon -R /etc/init.d to relabel the symlink as
>>> well as descend into the directory.  On the whole I would expect it not to
>>> relabel the target of the symlink itself but I don't have a strong opinion
>>> on that, as long as it doesn't fail if the symlink is broken.
>> How about this for a compromise?
>>
>> Add a -h option to restorecon so that the user can explicitly ask to
>> relabel a symlink (as with chcon), but also fall back to proceeding with
>> the original path if realpath() fails, in which case we will relabel the
>> symlink if the path was a broken symlink.
> 
> I think that this is pretty complicated and the fallback behaviour is
> potentially confusing.  I still think that restorecon -R /etc/init.d should
> by default change the context on the symlink as well as descending into the
> directory.
> 
> Also, I think it is necessary to restore the code to compute the real path
> of all but the last component of a symlink when relabelling the link itself.
> 
> The following patch shows approximately the behaviour I would like, except
> that it doesn't work, because apply_spec needs an FTSENT:

Here's a patch that works, and does what I want.

It moves most of process_one into process_one_realpath, which assumes that
the path it is given requires no further expansion.
Now if given a symlink, call symlink_realpath to get the real path ignoring
the last component, and give that to process_one_realpath; tell
process_one_realpath not to recurse so this acts on the symlink only.
Then call process_one_realpath again with the real path of the target, to
recurse into it.

diff --git a/policycoreutils/setfiles/setfiles.c b/policycoreutils/setfiles/setfiles.c
index 4c47f21..c84cd6f 100644
--- a/policycoreutils/setfiles/setfiles.c
+++ b/policycoreutils/setfiles/setfiles.c
@@ -545,7 +545,47 @@ int canoncon(char **contextp)
 	return rc;
 }
 
-static int process_one(char *name)
+static int symlink_realpath(char *name, char *path)
+{
+	char *p = NULL, *file_sep;
+	char *tmp_path = strdupa(name);
+	size_t len = 0;
+
+	if (!tmp_path) {
+		fprintf(stderr, "strdupa on %s failed:  %s\n", name,
+			strerror(errno));
+		return -1;
+	}
+	file_sep = strrchr(tmp_path, '/');
+	if (file_sep == tmp_path) {
+		file_sep++;
+		p = strcpy(path, "");
+	} else if (file_sep) {
+		*file_sep = 0;
+		file_sep++;
+		p = realpath(tmp_path, path);
+	} else {
+		file_sep = tmp_path;
+		p = realpath("./", path);
+	}
+	if (p)
+		len = strlen(p);
+	if (!p || len + strlen(file_sep) + 2 > PATH_MAX) {
+		fprintf(stderr, "symlink_realpath(%s) failed %s\n", name,
+			strerror(errno));
+		return -1;
+	}
+	p += len;
+	/* ensure trailing slash of directory name */
+	if (len == 0 || *(p - 1) != '/') {
+		*p = '/';
+		p++;
+	}
+	strcpy(p, file_sep);
+	return 0;
+}
+
+static int process_one_realpath(char *name, int recurse_this_path)
 {
 	int rc = 0;
 	const char *namelist[2];
@@ -553,18 +593,6 @@ static int process_one(char *name)
 	FTS *fts_handle;
 	FTSENT *ftsent;
 
-	if (expand_realpath) {
-		char *p;
-		p = realpath(name, NULL);
-		if (!p) {
-			fprintf(stderr, "realpath(%s) failed %s\n", name,
-				strerror(errno));
-			return -1;
-		}
-		name = p;
-	}
-
-
 	if (!strcmp(name, "/"))
 		mass_relabel = 1;
 
@@ -604,7 +632,7 @@ static int process_one(char *name)
 			fts_set(fts_handle, ftsent, FTS_SKIP);
 		if (rc == ERR)
 			goto err;
-		if (!recurse)
+		if (!recurse_this_path)
 			break;
 	} while ((ftsent = fts_read(fts_handle)) != NULL);
 
@@ -619,8 +647,6 @@ out:
 	}
 	if (fts_handle)
 		fts_close(fts_handle);
-	if (expand_realpath)
-		free(name);
 	return rc;
 
 err:
@@ -630,6 +656,55 @@ err:
 	goto out;
 }
 
+static int process_one(char *name)
+{
+	int rc = 0;
+	char *p;
+	struct stat sb;
+
+	if (!expand_realpath) {
+		return process_one_realpath(name, recurse);
+	} else {
+		rc = lstat(name, &sb);
+		if (rc < 0) {
+			fprintf(stderr, "%s:  lstat(%s) failed:  %s\n",
+				progname, name,	strerror(errno));
+			return -1;
+		}
+
+		if (S_ISLNK(sb.st_mode)) {
+			char path[PATH_MAX + 1];
+
+			rc = symlink_realpath(name, path);
+			if (rc < 0)
+				return rc;
+			rc = process_one_realpath(path, 0);
+			if (rc < 0)
+				return rc;
+
+			p = realpath(name, NULL);
+			if (!p) {
+				fprintf(stderr, "Warning: %s: Broken symlink: %s\n",
+					name, strerror(errno));
+				return 0;
+			}
+			rc = process_one_realpath(p, recurse);
+			free(p);
+			return rc;
+		} else {
+			p = realpath(name, NULL);
+			if (!p) {
+				fprintf(stderr, "realpath(%s) failed %s\n", name,
+					strerror(errno));
+				return -1;
+			}
+			rc = process_one_realpath(p, recurse);
+			free(p);
+			return rc;
+		}
+	}
+}
+
 #ifndef USE_AUDIT
 static void maybe_audit_mass_relabel(void)
 {

-- 
Martin Orr

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: restorecon and symbolic links

[Stephen Smalley]

On Tue, 2009-09-01 at 15:34 +0100, Martin Orr wrote:
> Here's a patch that works, and does what I want.
> 
> It moves most of process_one into process_one_realpath, which assumes that
> the path it is given requires no further expansion.
> Now if given a symlink, call symlink_realpath to get the real path ignoring
> the last component, and give that to process_one_realpath; tell
> process_one_realpath not to recurse so this acts on the symlink only.
> Then call process_one_realpath again with the real path of the target, to
> recurse into it.

The function naming seems the reverse of what one might expect (the
function with _realpath in the name doesn't call realpath).   Also, you
can pass NULL for the resolved path argument to realpath() rather than
giving it a fixed-size buffer from the stack and let it allocate the
buffer for you, as we already do elsewhere.

Not sure it is worth warning about the broken symlink case, and that
will trigger warnings for your existing users in the
restorecon /dev/stdin case, right?

> diff --git a/policycoreutils/setfiles/setfiles.c b/policycoreutils/setfiles/setfiles.c
> index 4c47f21..c84cd6f 100644
> --- a/policycoreutils/setfiles/setfiles.c
> +++ b/policycoreutils/setfiles/setfiles.c
> @@ -545,7 +545,47 @@ int canoncon(char **contextp)
>  	return rc;
>  }
>  
> -static int process_one(char *name)
> +static int symlink_realpath(char *name, char *path)
> +{
> +	char *p = NULL, *file_sep;
> +	char *tmp_path = strdupa(name);
> +	size_t len = 0;
> +
> +	if (!tmp_path) {
> +		fprintf(stderr, "strdupa on %s failed:  %s\n", name,
> +			strerror(errno));
> +		return -1;
> +	}
> +	file_sep = strrchr(tmp_path, '/');
> +	if (file_sep == tmp_path) {
> +		file_sep++;
> +		p = strcpy(path, "");
> +	} else if (file_sep) {
> +		*file_sep = 0;
> +		file_sep++;
> +		p = realpath(tmp_path, path);
> +	} else {
> +		file_sep = tmp_path;
> +		p = realpath("./", path);
> +	}
> +	if (p)
> +		len = strlen(p);
> +	if (!p || len + strlen(file_sep) + 2 > PATH_MAX) {
> +		fprintf(stderr, "symlink_realpath(%s) failed %s\n", name,
> +			strerror(errno));
> +		return -1;
> +	}
> +	p += len;
> +	/* ensure trailing slash of directory name */
> +	if (len == 0 || *(p - 1) != '/') {
> +		*p = '/';
> +		p++;
> +	}
> +	strcpy(p, file_sep);
> +	return 0;
> +}
> +
> +static int process_one_realpath(char *name, int recurse_this_path)
>  {
>  	int rc = 0;
>  	const char *namelist[2];
> @@ -553,18 +593,6 @@ static int process_one(char *name)
>  	FTS *fts_handle;
>  	FTSENT *ftsent;
>  
> -	if (expand_realpath) {
> -		char *p;
> -		p = realpath(name, NULL);
> -		if (!p) {
> -			fprintf(stderr, "realpath(%s) failed %s\n", name,
> -				strerror(errno));
> -			return -1;
> -		}
> -		name = p;
> -	}
> -
> -
>  	if (!strcmp(name, "/"))
>  		mass_relabel = 1;
>  
> @@ -604,7 +632,7 @@ static int process_one(char *name)
>  			fts_set(fts_handle, ftsent, FTS_SKIP);
>  		if (rc == ERR)
>  			goto err;
> -		if (!recurse)
> +		if (!recurse_this_path)
>  			break;
>  	} while ((ftsent = fts_read(fts_handle)) != NULL);
>  
> @@ -619,8 +647,6 @@ out:
>  	}
>  	if (fts_handle)
>  		fts_close(fts_handle);
> -	if (expand_realpath)
> -		free(name);
>  	return rc;
>  
>  err:
> @@ -630,6 +656,55 @@ err:
>  	goto out;
>  }
>  
> +static int process_one(char *name)
> +{
> +	int rc = 0;
> +	char *p;
> +	struct stat sb;
> +
> +	if (!expand_realpath) {
> +		return process_one_realpath(name, recurse);
> +	} else {
> +		rc = lstat(name, &sb);
> +		if (rc < 0) {
> +			fprintf(stderr, "%s:  lstat(%s) failed:  %s\n",
> +				progname, name,	strerror(errno));
> +			return -1;
> +		}
> +
> +		if (S_ISLNK(sb.st_mode)) {
> +			char path[PATH_MAX + 1];
> +
> +			rc = symlink_realpath(name, path);
> +			if (rc < 0)
> +				return rc;
> +			rc = process_one_realpath(path, 0);
> +			if (rc < 0)
> +				return rc;
> +
> +			p = realpath(name, NULL);
> +			if (!p) {
> +				fprintf(stderr, "Warning: %s: Broken symlink: %s\n",
> +					name, strerror(errno));
> +				return 0;
> +			}
> +			rc = process_one_realpath(p, recurse);
> +			free(p);
> +			return rc;
> +		} else {
> +			p = realpath(name, NULL);
> +			if (!p) {
> +				fprintf(stderr, "realpath(%s) failed %s\n", name,
> +					strerror(errno));
> +				return -1;
> +			}
> +			rc = process_one_realpath(p, recurse);
> +			free(p);
> +			return rc;
> +		}
> +	}
> +}
> +
>  #ifndef USE_AUDIT
>  static void maybe_audit_mass_relabel(void)
>  {
> 
-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: restorecon and symbolic links

[Martin Orr]

On 01/09/09 15:46, Stephen Smalley wrote:
> On Tue, 2009-09-01 at 15:34 +0100, Martin Orr wrote:
>> Here's a patch that works, and does what I want.
>>
>> It moves most of process_one into process_one_realpath, which assumes that
>> the path it is given requires no further expansion.
>> Now if given a symlink, call symlink_realpath to get the real path ignoring
>> the last component, and give that to process_one_realpath; tell
>> process_one_realpath not to recurse so this acts on the symlink only.
>> Then call process_one_realpath again with the real path of the target, to
>> recurse into it.
> 
> The function naming seems the reverse of what one might expect (the
> function with _realpath in the name doesn't call realpath).   Also, you
> can pass NULL for the resolved path argument to realpath() rather than
> giving it a fixed-size buffer from the stack and let it allocate the
> buffer for you, as we already do elsewhere.

The reason I named the function that way was because the function with
_realpath expects a real path as an argument, but if this is confusing or
goes against a convention used elsewhere then it could be changed.

I copied the symlink_realpath code from what used to be in match.  I assumed
that it needed a buffer on the stack because it then concatenates the last
component of the path on to that buffer, and realpath might not allocate a
long enough buffer.

> > Not sure it is worth warning about the broken symlink case, and that
> > will trigger warnings for your existing users in the
> > restorecon /dev/stdin case, right?

I agree, it is not worth warning about broken symlinks, except maybe if -v
is specified.

>> diff --git a/policycoreutils/setfiles/setfiles.c b/policycoreutils/setfiles/setfiles.c
>> index 4c47f21..c84cd6f 100644
>> --- a/policycoreutils/setfiles/setfiles.c
>> +++ b/policycoreutils/setfiles/setfiles.c
>> @@ -545,7 +545,47 @@ int canoncon(char **contextp)
>>  	return rc;
>>  }
>>  
>> -static int process_one(char *name)
>> +static int symlink_realpath(char *name, char *path)
>> +{
>> +	char *p = NULL, *file_sep;
>> +	char *tmp_path = strdupa(name);
>> +	size_t len = 0;
>> +
>> +	if (!tmp_path) {
>> +		fprintf(stderr, "strdupa on %s failed:  %s\n", name,
>> +			strerror(errno));
>> +		return -1;
>> +	}
>> +	file_sep = strrchr(tmp_path, '/');
>> +	if (file_sep == tmp_path) {
>> +		file_sep++;
>> +		p = strcpy(path, "");
>> +	} else if (file_sep) {
>> +		*file_sep = 0;
>> +		file_sep++;
>> +		p = realpath(tmp_path, path);
>> +	} else {
>> +		file_sep = tmp_path;
>> +		p = realpath("./", path);
>> +	}
>> +	if (p)
>> +		len = strlen(p);
>> +	if (!p || len + strlen(file_sep) + 2 > PATH_MAX) {
>> +		fprintf(stderr, "symlink_realpath(%s) failed %s\n", name,
>> +			strerror(errno));
>> +		return -1;
>> +	}
>> +	p += len;
>> +	/* ensure trailing slash of directory name */
>> +	if (len == 0 || *(p - 1) != '/') {
>> +		*p = '/';
>> +		p++;
>> +	}
>> +	strcpy(p, file_sep);
>> +	return 0;
>> +}
>> +
>> +static int process_one_realpath(char *name, int recurse_this_path)
>>  {
>>  	int rc = 0;
>>  	const char *namelist[2];
>> @@ -553,18 +593,6 @@ static int process_one(char *name)
>>  	FTS *fts_handle;
>>  	FTSENT *ftsent;
>>  
>> -	if (expand_realpath) {
>> -		char *p;
>> -		p = realpath(name, NULL);
>> -		if (!p) {
>> -			fprintf(stderr, "realpath(%s) failed %s\n", name,
>> -				strerror(errno));
>> -			return -1;
>> -		}
>> -		name = p;
>> -	}
>> -
>> -
>>  	if (!strcmp(name, "/"))
>>  		mass_relabel = 1;
>>  
>> @@ -604,7 +632,7 @@ static int process_one(char *name)
>>  			fts_set(fts_handle, ftsent, FTS_SKIP);
>>  		if (rc == ERR)
>>  			goto err;
>> -		if (!recurse)
>> +		if (!recurse_this_path)
>>  			break;
>>  	} while ((ftsent = fts_read(fts_handle)) != NULL);
>>  
>> @@ -619,8 +647,6 @@ out:
>>  	}
>>  	if (fts_handle)
>>  		fts_close(fts_handle);
>> -	if (expand_realpath)
>> -		free(name);
>>  	return rc;
>>  
>>  err:
>> @@ -630,6 +656,55 @@ err:
>>  	goto out;
>>  }
>>  
>> +static int process_one(char *name)
>> +{
>> +	int rc = 0;
>> +	char *p;
>> +	struct stat sb;
>> +
>> +	if (!expand_realpath) {
>> +		return process_one_realpath(name, recurse);
>> +	} else {
>> +		rc = lstat(name, &sb);
>> +		if (rc < 0) {
>> +			fprintf(stderr, "%s:  lstat(%s) failed:  %s\n",
>> +				progname, name,	strerror(errno));
>> +			return -1;
>> +		}
>> +
>> +		if (S_ISLNK(sb.st_mode)) {
>> +			char path[PATH_MAX + 1];
>> +
>> +			rc = symlink_realpath(name, path);
>> +			if (rc < 0)
>> +				return rc;
>> +			rc = process_one_realpath(path, 0);
>> +			if (rc < 0)
>> +				return rc;
>> +
>> +			p = realpath(name, NULL);
>> +			if (!p) {
>> +				fprintf(stderr, "Warning: %s: Broken symlink: %s\n",
>> +					name, strerror(errno));
>> +				return 0;
>> +			}
>> +			rc = process_one_realpath(p, recurse);
>> +			free(p);
>> +			return rc;
>> +		} else {
>> +			p = realpath(name, NULL);
>> +			if (!p) {
>> +				fprintf(stderr, "realpath(%s) failed %s\n", name,
>> +					strerror(errno));
>> +				return -1;
>> +			}
>> +			rc = process_one_realpath(p, recurse);
>> +			free(p);
>> +			return rc;
>> +		}
>> +	}
>> +}
>> +
>>  #ifndef USE_AUDIT
>>  static void maybe_audit_mass_relabel(void)
>>  {
>>


-- 
Martin Orr

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: restorecon and symbolic links

[Stephen Smalley]

On Wed, 2009-09-02 at 13:24 +0100, Martin Orr wrote:
> The reason I named the function that way was because the function with
> _realpath expects a real path as an argument, but if this is confusing or
> goes against a convention used elsewhere then it could be changed.

Except that symlink_realpath() internally calls realpath().  I think it
makes more sense to use process_one_realpath() for the function that
invokes realpath(), and process_one() for the function that merely acts
on its argument.  

> I copied the symlink_realpath code from what used to be in match.  I assumed
> that it needed a buffer on the stack because it then concatenates the last
> component of the path on to that buffer, and realpath might not allocate a
> long enough buffer.

That's fair.

> > > Not sure it is worth warning about the broken symlink case, and that
> > > will trigger warnings for your existing users in the
> > > restorecon /dev/stdin case, right?
> 
> I agree, it is not worth warning about broken symlinks, except maybe if -v
> is specified.

If the behavior of restorecon on symlinks is to always relabel the
symlink and then if the target exists, relabel it as well, then it isn't
truly an error if the target doesn't exist.  It would be a bit clearer
if we used an explicit flag like -h, as existing utilities like chown
and chcon do, when we want to act on symlinks rather than their targets,
but that would break existing usage it seems.

Modified patch below.  Seem reasonable?

diff --git a/policycoreutils/setfiles/setfiles.c b/policycoreutils/setfiles/setfiles.c
index 4c47f21..313767a 100644
--- a/policycoreutils/setfiles/setfiles.c
+++ b/policycoreutils/setfiles/setfiles.c
@@ -545,7 +545,47 @@ int canoncon(char **contextp)
 	return rc;
 }
 
-static int process_one(char *name)
+static int symlink_realpath(char *name, char *path)
+{
+	char *p = NULL, *file_sep;
+	char *tmp_path = strdupa(name);
+	size_t len = 0;
+
+	if (!tmp_path) {
+		fprintf(stderr, "strdupa on %s failed:  %s\n", name,
+			strerror(errno));
+		return -1;
+	}
+	file_sep = strrchr(tmp_path, '/');
+	if (file_sep == tmp_path) {
+		file_sep++;
+		p = strcpy(path, "");
+	} else if (file_sep) {
+		*file_sep = 0;
+		file_sep++;
+		p = realpath(tmp_path, path);
+	} else {
+		file_sep = tmp_path;
+		p = realpath("./", path);
+	}
+	if (p)
+		len = strlen(p);
+	if (!p || len + strlen(file_sep) + 2 > PATH_MAX) {
+		fprintf(stderr, "symlink_realpath(%s) failed %s\n", name,
+			strerror(errno));
+		return -1;
+	}
+	p += len;
+	/* ensure trailing slash of directory name */
+	if (len == 0 || *(p - 1) != '/') {
+		*p = '/';
+		p++;
+	}
+	strcpy(p, file_sep);
+	return 0;
+}
+
+static int process_one(char *name, int recurse_this_path)
 {
 	int rc = 0;
 	const char *namelist[2];
@@ -553,18 +593,6 @@ static int process_one(char *name)
 	FTS *fts_handle;
 	FTSENT *ftsent;
 
-	if (expand_realpath) {
-		char *p;
-		p = realpath(name, NULL);
-		if (!p) {
-			fprintf(stderr, "realpath(%s) failed %s\n", name,
-				strerror(errno));
-			return -1;
-		}
-		name = p;
-	}
-
-
 	if (!strcmp(name, "/"))
 		mass_relabel = 1;
 
@@ -604,7 +632,7 @@ static int process_one(char *name)
 			fts_set(fts_handle, ftsent, FTS_SKIP);
 		if (rc == ERR)
 			goto err;
-		if (!recurse)
+		if (!recurse_this_path)
 			break;
 	} while ((ftsent = fts_read(fts_handle)) != NULL);
 
@@ -619,8 +647,6 @@ out:
 	}
 	if (fts_handle)
 		fts_close(fts_handle);
-	if (expand_realpath)
-		free(name);
 	return rc;
 
 err:
@@ -630,6 +656,52 @@ err:
 	goto out;
 }
 
+static int process_one_realpath(char *name)
+{
+	int rc = 0;
+	char *p;
+	struct stat sb;
+
+	if (!expand_realpath) {
+		return process_one(name, recurse);
+	} else {
+		rc = lstat(name, &sb);
+		if (rc < 0) {
+			fprintf(stderr, "%s:  lstat(%s) failed:  %s\n",
+				progname, name,	strerror(errno));
+			return -1;
+		}
+
+		if (S_ISLNK(sb.st_mode)) {
+			char path[PATH_MAX + 1];
+
+			rc = symlink_realpath(name, path);
+			if (rc < 0)
+				return rc;
+			rc = process_one(path, 0);
+			if (rc < 0)
+				return rc;
+
+			p = realpath(name, NULL);
+			if (p) {
+				rc = process_one(p, recurse);
+				free(p);
+			}
+			return rc;
+		} else {
+			p = realpath(name, NULL);
+			if (!p) {
+				fprintf(stderr, "realpath(%s) failed %s\n", name,
+					strerror(errno));
+				return -1;
+			}
+			rc = process_one(p, recurse);
+			free(p);
+			return rc;
+		}
+	}
+}
+
 #ifndef USE_AUDIT
 static void maybe_audit_mass_relabel(void)
 {
@@ -987,13 +1059,13 @@ int main(int argc, char **argv)
 		delim = (null_terminated != 0) ? '\0' : '\n';
 		while ((len = getdelim(&buf, &buf_len, delim, f)) > 0) {
 			buf[len - 1] = 0;
-			errors |= process_one(buf);
+			errors |= process_one_realpath(buf);
 		}
 		if (strcmp(input_filename, "-") != 0)
 			fclose(f);
 	} else {
 		for (i = optind; i < argc; i++) {
-			errors |= process_one(argv[i]);
+			errors |= process_one_realpath(argv[i]);
 		}
 	}
 

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: restorecon and symbolic links

[Martin Orr]

On 02/09/09 13:52, Stephen Smalley wrote:
> On Wed, 2009-09-02 at 13:24 +0100, Martin Orr wrote:
>> The reason I named the function that way was because the function with
>> _realpath expects a real path as an argument, but if this is confusing or
>> goes against a convention used elsewhere then it could be changed.
> 
> Except that symlink_realpath() internally calls realpath().  I think it
> makes more sense to use process_one_realpath() for the function that
> invokes realpath(), and process_one() for the function that merely acts
> on its argument.  
> 
>> I copied the symlink_realpath code from what used to be in match.  I assumed
>> that it needed a buffer on the stack because it then concatenates the last
>> component of the path on to that buffer, and realpath might not allocate a
>> long enough buffer.
> 
> That's fair.
> 
>>>> Not sure it is worth warning about the broken symlink case, and that
>>>> will trigger warnings for your existing users in the
>>>> restorecon /dev/stdin case, right?
>> I agree, it is not worth warning about broken symlinks, except maybe if -v
>> is specified.
> 
> If the behavior of restorecon on symlinks is to always relabel the
> symlink and then if the target exists, relabel it as well, then it isn't
> truly an error if the target doesn't exist.  It would be a bit clearer
> if we used an explicit flag like -h, as existing utilities like chown
> and chcon do, when we want to act on symlinks rather than their targets,
> but that would break existing usage it seems.
> 
> Modified patch below.  Seem reasonable?

Looks fine to me.

> diff --git a/policycoreutils/setfiles/setfiles.c b/policycoreutils/setfiles/setfiles.c
> index 4c47f21..313767a 100644
> --- a/policycoreutils/setfiles/setfiles.c
> +++ b/policycoreutils/setfiles/setfiles.c
> @@ -545,7 +545,47 @@ int canoncon(char **contextp)
>  	return rc;
>  }
>  
> -static int process_one(char *name)
> +static int symlink_realpath(char *name, char *path)
> +{
> +	char *p = NULL, *file_sep;
> +	char *tmp_path = strdupa(name);
> +	size_t len = 0;
> +
> +	if (!tmp_path) {
> +		fprintf(stderr, "strdupa on %s failed:  %s\n", name,
> +			strerror(errno));
> +		return -1;
> +	}
> +	file_sep = strrchr(tmp_path, '/');
> +	if (file_sep == tmp_path) {
> +		file_sep++;
> +		p = strcpy(path, "");
> +	} else if (file_sep) {
> +		*file_sep = 0;
> +		file_sep++;
> +		p = realpath(tmp_path, path);
> +	} else {
> +		file_sep = tmp_path;
> +		p = realpath("./", path);
> +	}
> +	if (p)
> +		len = strlen(p);
> +	if (!p || len + strlen(file_sep) + 2 > PATH_MAX) {
> +		fprintf(stderr, "symlink_realpath(%s) failed %s\n", name,
> +			strerror(errno));
> +		return -1;
> +	}
> +	p += len;
> +	/* ensure trailing slash of directory name */
> +	if (len == 0 || *(p - 1) != '/') {
> +		*p = '/';
> +		p++;
> +	}
> +	strcpy(p, file_sep);
> +	return 0;
> +}
> +
> +static int process_one(char *name, int recurse_this_path)
>  {
>  	int rc = 0;
>  	const char *namelist[2];
> @@ -553,18 +593,6 @@ static int process_one(char *name)
>  	FTS *fts_handle;
>  	FTSENT *ftsent;
>  
> -	if (expand_realpath) {
> -		char *p;
> -		p = realpath(name, NULL);
> -		if (!p) {
> -			fprintf(stderr, "realpath(%s) failed %s\n", name,
> -				strerror(errno));
> -			return -1;
> -		}
> -		name = p;
> -	}
> -
> -
>  	if (!strcmp(name, "/"))
>  		mass_relabel = 1;
>  
> @@ -604,7 +632,7 @@ static int process_one(char *name)
>  			fts_set(fts_handle, ftsent, FTS_SKIP);
>  		if (rc == ERR)
>  			goto err;
> -		if (!recurse)
> +		if (!recurse_this_path)
>  			break;
>  	} while ((ftsent = fts_read(fts_handle)) != NULL);
>  
> @@ -619,8 +647,6 @@ out:
>  	}
>  	if (fts_handle)
>  		fts_close(fts_handle);
> -	if (expand_realpath)
> -		free(name);
>  	return rc;
>  
>  err:
> @@ -630,6 +656,52 @@ err:
>  	goto out;
>  }
>  
> +static int process_one_realpath(char *name)
> +{
> +	int rc = 0;
> +	char *p;
> +	struct stat sb;
> +
> +	if (!expand_realpath) {
> +		return process_one(name, recurse);
> +	} else {
> +		rc = lstat(name, &sb);
> +		if (rc < 0) {
> +			fprintf(stderr, "%s:  lstat(%s) failed:  %s\n",
> +				progname, name,	strerror(errno));
> +			return -1;
> +		}
> +
> +		if (S_ISLNK(sb.st_mode)) {
> +			char path[PATH_MAX + 1];
> +
> +			rc = symlink_realpath(name, path);
> +			if (rc < 0)
> +				return rc;
> +			rc = process_one(path, 0);
> +			if (rc < 0)
> +				return rc;
> +
> +			p = realpath(name, NULL);
> +			if (p) {
> +				rc = process_one(p, recurse);
> +				free(p);
> +			}
> +			return rc;
> +		} else {
> +			p = realpath(name, NULL);
> +			if (!p) {
> +				fprintf(stderr, "realpath(%s) failed %s\n", name,
> +					strerror(errno));
> +				return -1;
> +			}
> +			rc = process_one(p, recurse);
> +			free(p);
> +			return rc;
> +		}
> +	}
> +}
> +
>  #ifndef USE_AUDIT
>  static void maybe_audit_mass_relabel(void)
>  {
> @@ -987,13 +1059,13 @@ int main(int argc, char **argv)
>  		delim = (null_terminated != 0) ? '\0' : '\n';
>  		while ((len = getdelim(&buf, &buf_len, delim, f)) > 0) {
>  			buf[len - 1] = 0;
> -			errors |= process_one(buf);
> +			errors |= process_one_realpath(buf);
>  		}
>  		if (strcmp(input_filename, "-") != 0)
>  			fclose(f);
>  	} else {
>  		for (i = optind; i < argc; i++) {
> -			errors |= process_one(argv[i]);
> +			errors |= process_one_realpath(argv[i]);
>  		}
>  	}
>  
> 


-- 
Martin Orr

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: restorecon and symbolic links

[Stephen Smalley]

On Thu, 2009-09-03 at 10:47 +0100, Martin Orr wrote:
> On 02/09/09 13:52, Stephen Smalley wrote:
> > On Wed, 2009-09-02 at 13:24 +0100, Martin Orr wrote:
> >> The reason I named the function that way was because the function with
> >> _realpath expects a real path as an argument, but if this is confusing or
> >> goes against a convention used elsewhere then it could be changed.
> > 
> > Except that symlink_realpath() internally calls realpath().  I think it
> > makes more sense to use process_one_realpath() for the function that
> > invokes realpath(), and process_one() for the function that merely acts
> > on its argument.  
> > 
> >> I copied the symlink_realpath code from what used to be in match.  I assumed
> >> that it needed a buffer on the stack because it then concatenates the last
> >> component of the path on to that buffer, and realpath might not allocate a
> >> long enough buffer.
> > 
> > That's fair.
> > 
> >>>> Not sure it is worth warning about the broken symlink case, and that
> >>>> will trigger warnings for your existing users in the
> >>>> restorecon /dev/stdin case, right?
> >> I agree, it is not worth warning about broken symlinks, except maybe if -v
> >> is specified.
> > 
> > If the behavior of restorecon on symlinks is to always relabel the
> > symlink and then if the target exists, relabel it as well, then it isn't
> > truly an error if the target doesn't exist.  It would be a bit clearer
> > if we used an explicit flag like -h, as existing utilities like chown
> > and chcon do, when we want to act on symlinks rather than their targets,
> > but that would break existing usage it seems.
> > 
> > Modified patch below.  Seem reasonable?
> 
> Looks fine to me.

Ok, applied as policycoreutils 2.0.72.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

SELinux and SSH Timers ?...

[Hasan Rezaul-CHR010]

Hi All,

I have defined policy such that user_t cannot 'write' to certain
files/directories, e.g.  /etc/passwd.

I have Selinux Strict policy running in "Permissive" Mode, and lets just
say I am not at liberty to run it in "Enforcing" mode.

I also have  /selinux/avc/cache_threshold set to the default  "512".

And to test that my policy works, I SSH-login as a user "test" who has
default context  user_u:user_r:user_t
And I attempt to "vi /etc/passwd".

Given the above conditions: 
- I understand that SELinux will (and does) report a deny into audit.log
the first time I attempt the "vi /etc/passwd".

- The next time I do "vi /etc/passwd" within the same SSH-session, it
does NOT log the deny! I understand this is because of the Permissive
Mode + the cache_threshold value.

- If I exit my SSH session, and then create a new SSH-session and login
again with user "test", and then "vi /etc/passwd" again, I thought I
should see a deny in the audit.log ? Since this is a new SSH-Session...
But I DON'T see it ???

- If I however SSH-login as "test" after a 'long' time, may be a few
hours, and then "vi /etc/passwd", then I do see the deny again...

So my question is: Is there a default timer somewhere (may be for SSH),
during which duplicate audit denies don't get logged ? And perhaps after
the timer expires, previous denies are eligible for reporting again ?

What would be the impact of changing the "cache_threshold" to "5" for
example as opposed to the default '512' ?

Hopefully that wasn't too confusing.  Thanks in advance for your help
:-)


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: SELinux and SSH Timers ?...

[Stephen Smalley]

On Thu, 2009-09-03 at 16:17 -0400, Hasan Rezaul-CHR010 wrote:
> Hi All,
> 
> I have defined policy such that user_t cannot 'write' to certain
> files/directories, e.g.  /etc/passwd.
> 
> I have Selinux Strict policy running in "Permissive" Mode, and lets just
> say I am not at liberty to run it in "Enforcing" mode.
> 
> I also have  /selinux/avc/cache_threshold set to the default  "512".
> 
> And to test that my policy works, I SSH-login as a user "test" who has
> default context  user_u:user_r:user_t
> And I attempt to "vi /etc/passwd".
> 
> Given the above conditions: 
> - I understand that SELinux will (and does) report a deny into audit.log
> the first time I attempt the "vi /etc/passwd".
> 
> - The next time I do "vi /etc/passwd" within the same SSH-session, it
> does NOT log the deny! I understand this is because of the Permissive
> Mode + the cache_threshold value.
> 
> - If I exit my SSH session, and then create a new SSH-session and login
> again with user "test", and then "vi /etc/passwd" again, I thought I
> should see a deny in the audit.log ? Since this is a new SSH-Session...
> But I DON'T see it ???

No.  In permissive mode, when the denial is encountered, the AVC adds
that denied permission into the allowed vector in the corresponding AVC
cache entry so that subsequent denials do not get logged.  It remains
there until something causes the entry to be evicted, whether that is a
due to a policy reload, changing a boolean, toggling enforcing mode, or
just needing to reclaim an entry over time.  It doesn't have anything to
do with ssh sessions.

You can force an AVC reset at any time by reloading policy, changing a
boolean, or toggling enforcing mode.  Or you can set the threshold low
to cause nodes to get reclaimed quickly for reuse (that sets the upper
limit on the number of nodes to be used by the AVC).  Or you could just
patch your kernel to _not_ add the denied permission to the AVC in the
first place.

Last time we talked about this, Eric Paris also pointed out how to get
audit information via auditallow rules or using auditctl.

> 
> - If I however SSH-login as "test" after a 'long' time, may be a few
> hours, and then "vi /etc/passwd", then I do see the deny again...
> 
> So my question is: Is there a default timer somewhere (may be for SSH),
> during which duplicate audit denies don't get logged ? And perhaps after
> the timer expires, previous denies are eligible for reporting again ?
> 
> What would be the impact of changing the "cache_threshold" to "5" for
> example as opposed to the default '512' ?
> 
> Hopefully that wasn't too confusing.  Thanks in advance for your help
> :-)
-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: SELinux and SSH Timers ?...

[Stephen Smalley]

On Thu, 2009-09-03 at 16:32 -0400, Stephen Smalley wrote:
> On Thu, 2009-09-03 at 16:17 -0400, Hasan Rezaul-CHR010 wrote:
> > Hi All,
> > 
> > I have defined policy such that user_t cannot 'write' to certain
> > files/directories, e.g.  /etc/passwd.
> > 
> > I have Selinux Strict policy running in "Permissive" Mode, and lets just
> > say I am not at liberty to run it in "Enforcing" mode.
> > 
> > I also have  /selinux/avc/cache_threshold set to the default  "512".
> > 
> > And to test that my policy works, I SSH-login as a user "test" who has
> > default context  user_u:user_r:user_t
> > And I attempt to "vi /etc/passwd".
> > 
> > Given the above conditions: 
> > - I understand that SELinux will (and does) report a deny into audit.log
> > the first time I attempt the "vi /etc/passwd".
> > 
> > - The next time I do "vi /etc/passwd" within the same SSH-session, it
> > does NOT log the deny! I understand this is because of the Permissive
> > Mode + the cache_threshold value.
> > 
> > - If I exit my SSH session, and then create a new SSH-session and login
> > again with user "test", and then "vi /etc/passwd" again, I thought I
> > should see a deny in the audit.log ? Since this is a new SSH-Session...
> > But I DON'T see it ???
> 
> No.  In permissive mode, when the denial is encountered, the AVC adds
> that denied permission into the allowed vector in the corresponding AVC
> cache entry so that subsequent denials do not get logged.  It remains
> there until something causes the entry to be evicted, whether that is a
> due to a policy reload, changing a boolean, toggling enforcing mode, or
> just needing to reclaim an entry over time.  It doesn't have anything to
> do with ssh sessions.
> 
> You can force an AVC reset at any time by reloading policy, changing a
> boolean, or toggling enforcing mode.  Or you can set the threshold low
> to cause nodes to get reclaimed quickly for reuse (that sets the upper
> limit on the number of nodes to be used by the AVC).  Or you could just
> patch your kernel to _not_ add the denied permission to the AVC in the
> first place.

What kernel version are you using?

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

RE: SELinux and SSH Timers ?...

[Hasan Rezaul-CHR010]

My Linux kernel version is 2.6.21.
 

-----Original Message-----
From: Stephen Smalley [mailto:sds@tycho.nsa.gov] 
Sent: Friday, September 04, 2009 6:49 AM
To: Hasan Rezaul-CHR010
Cc: selinux@tycho.nsa.gov
Subject: Re: SELinux and SSH Timers ?...

On Thu, 2009-09-03 at 16:32 -0400, Stephen Smalley wrote:
> On Thu, 2009-09-03 at 16:17 -0400, Hasan Rezaul-CHR010 wrote:
> > Hi All,
> > 
> > I have defined policy such that user_t cannot 'write' to certain 
> > files/directories, e.g.  /etc/passwd.
> > 
> > I have Selinux Strict policy running in "Permissive" Mode, and lets 
> > just say I am not at liberty to run it in "Enforcing" mode.
> > 
> > I also have  /selinux/avc/cache_threshold set to the default  "512".
> > 
> > And to test that my policy works, I SSH-login as a user "test" who 
> > has default context  user_u:user_r:user_t And I attempt to "vi 
> > /etc/passwd".
> > 
> > Given the above conditions: 
> > - I understand that SELinux will (and does) report a deny into 
> > audit.log the first time I attempt the "vi /etc/passwd".
> > 
> > - The next time I do "vi /etc/passwd" within the same SSH-session, 
> > it does NOT log the deny! I understand this is because of the 
> > Permissive Mode + the cache_threshold value.
> > 
> > - If I exit my SSH session, and then create a new SSH-session and 
> > login again with user "test", and then "vi /etc/passwd" again, I 
> > thought I should see a deny in the audit.log ? Since this is a new
SSH-Session...
> > But I DON'T see it ???
> 
> No.  In permissive mode, when the denial is encountered, the AVC adds 
> that denied permission into the allowed vector in the corresponding 
> AVC cache entry so that subsequent denials do not get logged.  It 
> remains there until something causes the entry to be evicted, whether 
> that is a due to a policy reload, changing a boolean, toggling 
> enforcing mode, or just needing to reclaim an entry over time.  It 
> doesn't have anything to do with ssh sessions.
> 
> You can force an AVC reset at any time by reloading policy, changing a

> boolean, or toggling enforcing mode.  Or you can set the threshold low

> to cause nodes to get reclaimed quickly for reuse (that sets the upper

> limit on the number of nodes to be used by the AVC).  Or you could 
> just patch your kernel to _not_ add the denied permission to the AVC 
> in the first place.

What kernel version are you using?

--
Stephen Smalley
National Security Agency



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

RE: SELinux and SSH Timers ?...

[Hasan Rezaul-CHR010]

Great, Thank You Sir  :-)
 

-----Original Message-----
From: Stephen Smalley [mailto:sds@tycho.nsa.gov] 
Sent: Friday, September 04, 2009 9:56 AM
To: Hasan Rezaul-CHR010
Cc: selinux@tycho.nsa.gov
Subject: RE: SELinux and SSH Timers ?...

On Fri, 2009-09-04 at 10:45 -0400, Hasan Rezaul-CHR010 wrote:
> My Linux kernel version is 2.6.21.

So if you wanted to have SELinux audit every denial in permissive mode,
you'd just apply this patch and rebuild your kernel.  

diff --git a/security/selinux/avc.c b/security/selinux/avc.c index
da8caf1..b190eb7 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -874,10 +874,6 @@ int avc_has_perm_noaudit(u32 ssid, u32 tsid,
 	if (!requested || denied) {
 		if (selinux_enforcing)
 			rc = -EACCES;
-		else
-			if (node)
-
avc_update_node(AVC_CALLBACK_GRANT,requested,
-						ssid,tsid,tclass);
 	}
 
 	rcu_read_unlock();

--
Stephen Smalley
National Security Agency



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

RE: SELinux and SSH Timers ?...

[Stephen Smalley]

On Fri, 2009-09-04 at 10:45 -0400, Hasan Rezaul-CHR010 wrote:
> My Linux kernel version is 2.6.21.

So if you wanted to have SELinux audit every denial in permissive mode,
you'd just apply this patch and rebuild your kernel.  

diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index da8caf1..b190eb7 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -874,10 +874,6 @@ int avc_has_perm_noaudit(u32 ssid, u32 tsid,
 	if (!requested || denied) {
 		if (selinux_enforcing)
 			rc = -EACCES;
-		else
-			if (node)
-				avc_update_node(AVC_CALLBACK_GRANT,requested,
-						ssid,tsid,tclass);
 	}
 
 	rcu_read_unlock();

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

RE: SELinux and SSH Timers ?...

[Hasan Rezaul-CHR010]

By the way, would patching the kernel this way have the same effect as
changing the  /selinux/avc/cache_threshold value to "0" for example ? 

If that is the case, which would be the more recommended approach to
follow (kernel patch  vs.  Changing cache_threshold)? And would you
kindly explain why the preferred approach is better over the other ?
Thanks again for all your help !


-----Original Message-----
From: Hasan Rezaul-CHR010 
Sent: Friday, September 04, 2009 9:56 AM
To: 'Stephen Smalley'
Cc: selinux@tycho.nsa.gov
Subject: RE: SELinux and SSH Timers ?...

Great, Thank You Sir  :-)
 

-----Original Message-----
From: Stephen Smalley [mailto:sds@tycho.nsa.gov]
Sent: Friday, September 04, 2009 9:56 AM
To: Hasan Rezaul-CHR010
Cc: selinux@tycho.nsa.gov
Subject: RE: SELinux and SSH Timers ?...

On Fri, 2009-09-04 at 10:45 -0400, Hasan Rezaul-CHR010 wrote:
> My Linux kernel version is 2.6.21.

So if you wanted to have SELinux audit every denial in permissive mode,
you'd just apply this patch and rebuild your kernel.  

diff --git a/security/selinux/avc.c b/security/selinux/avc.c index
da8caf1..b190eb7 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -874,10 +874,6 @@ int avc_has_perm_noaudit(u32 ssid, u32 tsid,
 	if (!requested || denied) {
 		if (selinux_enforcing)
 			rc = -EACCES;
-		else
-			if (node)
-
avc_update_node(AVC_CALLBACK_GRANT,requested,
-						ssid,tsid,tclass);
 	}
 
 	rcu_read_unlock();

--
Stephen Smalley
National Security Agency



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

RE: SELinux and SSH Timers ?...

[Stephen Smalley]

On Fri, 2009-09-04 at 11:17 -0400, Hasan Rezaul-CHR010 wrote:
> By the way, would patching the kernel this way have the same effect as
> changing the  /selinux/avc/cache_threshold value to "0" for example ? 
> 
> If that is the case, which would be the more recommended approach to
> follow (kernel patch  vs.  Changing cache_threshold)? And would you
> kindly explain why the preferred approach is better over the other ?
> Thanks again for all your help !

The kernel patch completely removes the adding of denied permissions to
the AVC.  So if they never get added, then they always get logged and
you don't need to evict the cache entry.  More reliable and more
performant.

Setting the cache threshold is just a way to speed up the eviction of
cache entries (by reducing the max number of entries).  It carries with
it a performance cost.

As I've said before, you also have the option of manually flushing the
cache by running load_policy at any time.  So if you only wanted to see
each instance of a denial within a ssh session, you could do that just
by running load_policy upon each login, or you could run load_policy at
selected points during a session (e.g. just before re-running an
application) to force a reset.

But if you truly want to see every instance of every denial in
permissive mode, just like enforcing mode, then the kernel patch is the
best choice.  Just be warned that it may render your system unusable if
your policy is in poor shape or your processes are running in the wrong
domains, as it can easily flood the system with AVC denials.

FWIW, modern SELinux also has permissive domains (the ability to make
individual domains permissive while leaving the rest of the system
enforcing).  But you'd need a newer kernel (>= 2.6.26) and the right
version of the SELinux userland for that.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

RE: SELinux and SSH Timers ?...

[Hasan Rezaul-CHR010]

Awesome. Thanks for the detailed info   :-)

 

-----Original Message-----
From: Stephen Smalley [mailto:sds@tycho.nsa.gov] 
Sent: Friday, September 04, 2009 11:06 AM
To: Hasan Rezaul-CHR010
Cc: selinux@tycho.nsa.gov
Subject: RE: SELinux and SSH Timers ?...

On Fri, 2009-09-04 at 11:17 -0400, Hasan Rezaul-CHR010 wrote:
> By the way, would patching the kernel this way have the same effect as

> changing the  /selinux/avc/cache_threshold value to "0" for example ?
> 
> If that is the case, which would be the more recommended approach to 
> follow (kernel patch  vs.  Changing cache_threshold)? And would you 
> kindly explain why the preferred approach is better over the other ?
> Thanks again for all your help !

The kernel patch completely removes the adding of denied permissions to
the AVC.  So if they never get added, then they always get logged and
you don't need to evict the cache entry.  More reliable and more
performant.

Setting the cache threshold is just a way to speed up the eviction of
cache entries (by reducing the max number of entries).  It carries with
it a performance cost.

As I've said before, you also have the option of manually flushing the
cache by running load_policy at any time.  So if you only wanted to see
each instance of a denial within a ssh session, you could do that just
by running load_policy upon each login, or you could run load_policy at
selected points during a session (e.g. just before re-running an
application) to force a reset.

But if you truly want to see every instance of every denial in
permissive mode, just like enforcing mode, then the kernel patch is the
best choice.  Just be warned that it may render your system unusable if
your policy is in poor shape or your processes are running in the wrong
domains, as it can easily flood the system with AVC denials.

FWIW, modern SELinux also has permissive domains (the ability to make
individual domains permissive while leaving the rest of the system
enforcing).  But you'd need a newer kernel (>= 2.6.26) and the right
version of the SELinux userland for that.

--
Stephen Smalley
National Security Agency



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.