[patch] libselinux: fix getfilecon handling of zero-length context

[patch] libselinux: fix getfilecon handling of zero-length context

[Stephen Smalley]

As observed by John Ramsdell, *getfilecon() on a /proc/sys inode on
returns garbage and can lead to memory corruption upon later freecon.
This happens on 2.6.21 and later due to the proc sysctl rewrite in the
kernel.  The *getfilecon_raw() functions correctly return zero in this
case, but the non-raw functions are not handling that result properly.
Initialize *context on entry to *getfilecon() so that it has a
well-defined value even if *getfilecon_raw() returns zero.

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

---

 libselinux/src/fgetfilecon.c |    2 ++
 libselinux/src/getfilecon.c  |    2 ++
 libselinux/src/lgetfilecon.c |    2 ++
 3 files changed, 6 insertions(+)

Index: trunk/libselinux/src/fgetfilecon.c
===================================================================
--- trunk/libselinux/src/fgetfilecon.c	(revision 2490)
+++ trunk/libselinux/src/fgetfilecon.c	(working copy)
@@ -51,6 +51,8 @@
 	security_context_t rcontext;
 	int ret;
 
+	*context = NULL;
+
 	ret = fgetfilecon_raw(fd, &rcontext);
 
 	if (ret > 0) {
Index: trunk/libselinux/src/lgetfilecon.c
===================================================================
--- trunk/libselinux/src/lgetfilecon.c	(revision 2490)
+++ trunk/libselinux/src/lgetfilecon.c	(working copy)
@@ -51,6 +51,8 @@
 	int ret;
 	security_context_t rcontext;
 
+	*context = NULL;
+
 	ret = lgetfilecon_raw(path, &rcontext);
 
 	if (ret > 0) {
Index: trunk/libselinux/src/getfilecon.c
===================================================================
--- trunk/libselinux/src/getfilecon.c	(revision 2490)
+++ trunk/libselinux/src/getfilecon.c	(working copy)
@@ -51,6 +51,8 @@
 	int ret;
 	security_context_t rcontext;
 
+	*context = NULL;
+
 	ret = getfilecon_raw(path, &rcontext);
 
 	if (ret > 0) {


-- 
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: [patch] libselinux: fix getfilecon handling of zero-length context

[Joshua Brindle]

Stephen Smalley wrote:
> As observed by John Ramsdell, *getfilecon() on a /proc/sys inode on
> returns garbage and can lead to memory corruption upon later freecon.
> This happens on 2.6.21 and later due to the proc sysctl rewrite in the
> kernel.  The *getfilecon_raw() functions correctly return zero in this
> case, but the non-raw functions are not handling that result properly.
> Initialize *context on entry to *getfilecon() so that it has a
> well-defined value even if *getfilecon_raw() returns zero.
>
> Signed-off-by:  Stephen Smalley <sds@tycho.nsa.gov>
>
>   

Acked-By: Joshua Brindle <jbrindle@manicmethod.com>

This should go into stable/1_0 and trunk, right?

> ---
>
>  libselinux/src/fgetfilecon.c |    2 ++
>  libselinux/src/getfilecon.c  |    2 ++
>  libselinux/src/lgetfilecon.c |    2 ++
>  3 files changed, 6 insertions(+)
>
> Index: trunk/libselinux/src/fgetfilecon.c
> ===================================================================
> --- trunk/libselinux/src/fgetfilecon.c	(revision 2490)
> +++ trunk/libselinux/src/fgetfilecon.c	(working copy)
> @@ -51,6 +51,8 @@
>  	security_context_t rcontext;
>  	int ret;
>  
> +	*context = NULL;
> +
>  	ret = fgetfilecon_raw(fd, &rcontext);
>  
>  	if (ret > 0) {
> Index: trunk/libselinux/src/lgetfilecon.c
> ===================================================================
> --- trunk/libselinux/src/lgetfilecon.c	(revision 2490)
> +++ trunk/libselinux/src/lgetfilecon.c	(working copy)
> @@ -51,6 +51,8 @@
>  	int ret;
>  	security_context_t rcontext;
>  
> +	*context = NULL;
> +
>  	ret = lgetfilecon_raw(path, &rcontext);
>  
>  	if (ret > 0) {
> Index: trunk/libselinux/src/getfilecon.c
> ===================================================================
> --- trunk/libselinux/src/getfilecon.c	(revision 2490)
> +++ trunk/libselinux/src/getfilecon.c	(working copy)
> @@ -51,6 +51,8 @@
>  	int ret;
>  	security_context_t rcontext;
>  
> +	*context = NULL;
> +
>  	ret = getfilecon_raw(path, &rcontext);
>  
>  	if (ret > 0) {
>
>
>   



--
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: [patch] libselinux: fix getfilecon handling of zero-length context

[Stephen Smalley]

On Mon, 2007-07-09 at 12:48 -0400, Joshua Brindle wrote:
> Stephen Smalley wrote:
> > As observed by John Ramsdell, *getfilecon() on a /proc/sys inode on
> > returns garbage and can lead to memory corruption upon later freecon.
> > This happens on 2.6.21 and later due to the proc sysctl rewrite in the
> > kernel.  The *getfilecon_raw() functions correctly return zero in this
> > case, but the non-raw functions are not handling that result properly.
> > Initialize *context on entry to *getfilecon() so that it has a
> > well-defined value even if *getfilecon_raw() returns zero.
> >
> > Signed-off-by:  Stephen Smalley <sds@tycho.nsa.gov>
> >
> >   
> 
> Acked-By: Joshua Brindle <jbrindle@manicmethod.com>
> 
> This should go into stable/1_0 and trunk, right?

Yes.

> 
> > ---
> >
> >  libselinux/src/fgetfilecon.c |    2 ++
> >  libselinux/src/getfilecon.c  |    2 ++
> >  libselinux/src/lgetfilecon.c |    2 ++
> >  3 files changed, 6 insertions(+)
> >
> > Index: trunk/libselinux/src/fgetfilecon.c
> > ===================================================================
> > --- trunk/libselinux/src/fgetfilecon.c	(revision 2490)
> > +++ trunk/libselinux/src/fgetfilecon.c	(working copy)
> > @@ -51,6 +51,8 @@
> >  	security_context_t rcontext;
> >  	int ret;
> >  
> > +	*context = NULL;
> > +
> >  	ret = fgetfilecon_raw(fd, &rcontext);
> >  
> >  	if (ret > 0) {
> > Index: trunk/libselinux/src/lgetfilecon.c
> > ===================================================================
> > --- trunk/libselinux/src/lgetfilecon.c	(revision 2490)
> > +++ trunk/libselinux/src/lgetfilecon.c	(working copy)
> > @@ -51,6 +51,8 @@
> >  	int ret;
> >  	security_context_t rcontext;
> >  
> > +	*context = NULL;
> > +
> >  	ret = lgetfilecon_raw(path, &rcontext);
> >  
> >  	if (ret > 0) {
> > Index: trunk/libselinux/src/getfilecon.c
> > ===================================================================
> > --- trunk/libselinux/src/getfilecon.c	(revision 2490)
> > +++ trunk/libselinux/src/getfilecon.c	(working copy)
> > @@ -51,6 +51,8 @@
> >  	int ret;
> >  	security_context_t rcontext;
> >  
> > +	*context = NULL;
> > +
> >  	ret = getfilecon_raw(path, &rcontext);
> >  
> >  	if (ret > 0) {
> >
> >
> >   
> 
-- 
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: [patch] libselinux: fix getfilecon handling of zero-length context

[Joshua Brindle]

Stephen Smalley wrote:
> On Mon, 2007-07-09 at 12:48 -0400, Joshua Brindle wrote:
>   
>> Stephen Smalley wrote:
>>     
>>> As observed by John Ramsdell, *getfilecon() on a /proc/sys inode on
>>> returns garbage and can lead to memory corruption upon later freecon.
>>> This happens on 2.6.21 and later due to the proc sysctl rewrite in the
>>> kernel.  The *getfilecon_raw() functions correctly return zero in this
>>> case, but the non-raw functions are not handling that result properly.
>>> Initialize *context on entry to *getfilecon() so that it has a
>>> well-defined value even if *getfilecon_raw() returns zero.
>>>
>>> Signed-off-by:  Stephen Smalley <sds@tycho.nsa.gov>
>>>
>>>   
>>>       
>> Acked-By: Joshua Brindle <jbrindle@manicmethod.com>
>>
>> This should go into stable/1_0 and trunk, right?
>>     
>
> Yes.
>
>   

Thanks, merged into 1.34.11 and 2.0.24

>>> ---
>>>
>>>  libselinux/src/fgetfilecon.c |    2 ++
>>>  libselinux/src/getfilecon.c  |    2 ++
>>>  libselinux/src/lgetfilecon.c |    2 ++
>>>  3 files changed, 6 insertions(+)
>>>
>>> Index: trunk/libselinux/src/fgetfilecon.c
>>> ===================================================================
>>> --- trunk/libselinux/src/fgetfilecon.c	(revision 2490)
>>> +++ trunk/libselinux/src/fgetfilecon.c	(working copy)
>>> @@ -51,6 +51,8 @@
>>>  	security_context_t rcontext;
>>>  	int ret;
>>>  
>>> +	*context = NULL;
>>> +
>>>  	ret = fgetfilecon_raw(fd, &rcontext);
>>>  
>>>  	if (ret > 0) {
>>> Index: trunk/libselinux/src/lgetfilecon.c
>>> ===================================================================
>>> --- trunk/libselinux/src/lgetfilecon.c	(revision 2490)
>>> +++ trunk/libselinux/src/lgetfilecon.c	(working copy)
>>> @@ -51,6 +51,8 @@
>>>  	int ret;
>>>  	security_context_t rcontext;
>>>  
>>> +	*context = NULL;
>>> +
>>>  	ret = lgetfilecon_raw(path, &rcontext);
>>>  
>>>  	if (ret > 0) {
>>> Index: trunk/libselinux/src/getfilecon.c
>>> ===================================================================
>>> --- trunk/libselinux/src/getfilecon.c	(revision 2490)
>>> +++ trunk/libselinux/src/getfilecon.c	(working copy)
>>> @@ -51,6 +51,8 @@
>>>  	int ret;
>>>  	security_context_t rcontext;
>>>  
>>> +	*context = NULL;
>>> +
>>>  	ret = getfilecon_raw(path, &rcontext);
>>>  
>>>  	if (ret > 0) {
>>>
>>>
>>>   
>>>       



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

getfilecon return code

[John D. Ramsdell]

I hadn't carefully read the manual page for getfilecon until now, but
I notice it states that a positive number is returned indicating the
number of bytes malloc'd for the context, and -1 is returned
indicating failure and that errno is set.  I would have guessed from
the description that zero is never an allowed return value.  In fact,
I wrote code that freecon'd a context whenever the return value was
not -1.

In the example below, when checking out a file in /proc/sys, zero is
returned and the result is a NULL context, something that need not be
free'd.  To me, failure to allocate a context feels like a failure
condition.

Perhaps the best strategy is to assume success when the context produced
in non-null, and not worry about the return code.

  security_context_t con = NULL;
  int rc = getfilecon(MP, &con);
  if (!con) /* Handle failure here */        
  else /* do something and then freecon(con) */

Is the following expression always true?

  (con != NULL) == (rc > 0)

John

[ramsdell@goo selinux]$ make
cc     mygetfilecon.c  /home/ramsdell/src/libselinux-2.0.8/src/libselinux.a -o mygetfilecon
[ramsdell@goo selinux]$ ./mygetfilecon
getfilecon("/proc/sys/kernel/pid_max", &con) = 0
con = NULL
[ramsdell@goo selinux]$ cat mygetfilecon.c
#include <stdio.h>
#include <selinux/selinux.h>

#define MP "/proc/sys/kernel/pid_max"

int
main(int argc, char **argv)
{
  security_context_t con;
  int rc = getfilecon(MP, &con);
  printf("getfilecon(\"%s\", &con) = %d\n", MP, rc);
  if (rc < 0)
    perror("getfilecon");
  else if (con)
    printf("con = \"%s\"\n", con);
  else
    printf("con = NULL\n", con);
  return 0;
}
[ramsdell@goo selinux]$ 

--
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: getfilecon return code

[Stephen Smalley]

On Mon, 2007-07-09 at 14:07 -0400, John D. Ramsdell wrote:
> I hadn't carefully read the manual page for getfilecon until now, but
> I notice it states that a positive number is returned indicating the
> number of bytes malloc'd for the context, and -1 is returned
> indicating failure and that errno is set.  I would have guessed from
> the description that zero is never an allowed return value.  In fact,
> I wrote code that freecon'd a context whenever the return value was
> not -1.

freecon(NULL) is perfectly legal and harmless, like free(NULL), so that
part is ok.

It is possible to set extended attributes with no values, e.g. 
	$ setfattr -n user.foo /path/to/foo
	$ getfattr -n user.foo /path/to/foo
and directly calling getxattr() on that file will return 0.

So technically this is a possible case, even if it is unusual and was
introduced in this case by the proc sysctl rewrite in the kernel leaving
us with "private" /proc/sys inodes.

I'd be inclined to change security_inode_getsecurity() in the kernel to
return -EOPNOTSUPP in the IS_PRIVATE(inode) case.  But that won't help
with current kernels, of course.

libselinux could remap a zero return from getxattr to a -1 return with
errno EOPNOTSUPP in the meantime if we want to present this behavior to
applications now.

> In the example below, when checking out a file in /proc/sys, zero is
> returned and the result is a NULL context, something that need not be
> free'd.  To me, failure to allocate a context feels like a failure
> condition.
> 
> Perhaps the best strategy is to assume success when the context produced
> in non-null, and not worry about the return code.
> 
>   security_context_t con = NULL;
>   int rc = getfilecon(MP, &con);
>   if (!con) /* Handle failure here */        
>   else /* do something and then freecon(con) */
> 
> Is the following expression always true?
> 
>   (con != NULL) == (rc > 0)

I don't think ignoring the return value is the right approach.  With the
libselinux patch I posted, there is no bug here; freecon(NULL) is
harmless.  But I can see an argument for making libselinux return -1
with errno EOPNOTSUPP in this case, and ultimately making the kernel do
that too.

> John
> 
> [ramsdell@goo selinux]$ make
> cc     mygetfilecon.c  /home/ramsdell/src/libselinux-2.0.8/src/libselinux.a -o mygetfilecon
> [ramsdell@goo selinux]$ ./mygetfilecon
> getfilecon("/proc/sys/kernel/pid_max", &con) = 0
> con = NULL
> [ramsdell@goo selinux]$ cat mygetfilecon.c
> #include <stdio.h>
> #include <selinux/selinux.h>
> 
> #define MP "/proc/sys/kernel/pid_max"
> 
> int
> main(int argc, char **argv)
> {
>   security_context_t con;
>   int rc = getfilecon(MP, &con);
>   printf("getfilecon(\"%s\", &con) = %d\n", MP, rc);
>   if (rc < 0)
>     perror("getfilecon");
>   else if (con)
>     printf("con = \"%s\"\n", con);
>   else
>     printf("con = NULL\n", con);
>   return 0;
> }
> [ramsdell@goo selinux]$ 
> 
> --
> 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.
-- 
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: getfilecon return code

[Stephen Smalley]

On Mon, 2007-07-09 at 14:30 -0400, Stephen Smalley wrote:
> On Mon, 2007-07-09 at 14:07 -0400, John D. Ramsdell wrote:
> > I hadn't carefully read the manual page for getfilecon until now, but
> > I notice it states that a positive number is returned indicating the
> > number of bytes malloc'd for the context, and -1 is returned
> > indicating failure and that errno is set.  I would have guessed from
> > the description that zero is never an allowed return value.  In fact,
> > I wrote code that freecon'd a context whenever the return value was
> > not -1.
> 
> freecon(NULL) is perfectly legal and harmless, like free(NULL), so that
> part is ok.
> 
> It is possible to set extended attributes with no values, e.g. 
> 	$ setfattr -n user.foo /path/to/foo
> 	$ getfattr -n user.foo /path/to/foo
> and directly calling getxattr() on that file will return 0.
> 
> So technically this is a possible case, even if it is unusual and was
> introduced in this case by the proc sysctl rewrite in the kernel leaving
> us with "private" /proc/sys inodes.
> 
> I'd be inclined to change security_inode_getsecurity() in the kernel to
> return -EOPNOTSUPP in the IS_PRIVATE(inode) case.  But that won't help
> with current kernels, of course.
> 
> libselinux could remap a zero return from getxattr to a -1 return with
> errno EOPNOTSUPP in the meantime if we want to present this behavior to
> applications now.

Like so:

Index: trunk/libselinux/src/fgetfilecon.c
===================================================================
--- trunk/libselinux/src/fgetfilecon.c	(revision 2492)
+++ trunk/libselinux/src/fgetfilecon.c	(working copy)
@@ -37,6 +37,11 @@
 		ret = fgetxattr(fd, XATTR_NAME_SELINUX, buf, size - 1);
 	}
       out:
+	if (ret == 0) {
+		/* Re-map empty attribute values to errors. */
+		errno = EOPNOTSUPP;
+		ret = -1;
+	}
 	if (ret < 0)
 		free(buf);
 	else
Index: trunk/libselinux/src/lgetfilecon.c
===================================================================
--- trunk/libselinux/src/lgetfilecon.c	(revision 2492)
+++ trunk/libselinux/src/lgetfilecon.c	(working copy)
@@ -37,6 +37,11 @@
 		ret = lgetxattr(path, XATTR_NAME_SELINUX, buf, size - 1);
 	}
       out:
+	if (ret == 0) {
+		/* Re-map empty attribute values to errors. */
+		errno = EOPNOTSUPP;
+		ret = -1;
+	}
 	if (ret < 0)
 		free(buf);
 	else
Index: trunk/libselinux/src/getfilecon.c
===================================================================
--- trunk/libselinux/src/getfilecon.c	(revision 2492)
+++ trunk/libselinux/src/getfilecon.c	(working copy)
@@ -37,6 +37,11 @@
 		ret = getxattr(path, XATTR_NAME_SELINUX, buf, size - 1);
 	}
       out:
+	if (ret == 0) {
+		/* Re-map empty attribute values to errors. */
+		errno = EOPNOTSUPP;
+		ret = -1;
+	}
 	if (ret < 0)
 		free(buf);
 	else

-- 
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: getfilecon return code

[Joshua Brindle]

Stephen Smalley wrote:
> On Mon, 2007-07-09 at 14:30 -0400, Stephen Smalley wrote:
>   
>> On Mon, 2007-07-09 at 14:07 -0400, John D. Ramsdell wrote:
>>     
>>> I hadn't carefully read the manual page for getfilecon until now, but
>>> I notice it states that a positive number is returned indicating the
>>> number of bytes malloc'd for the context, and -1 is returned
>>> indicating failure and that errno is set.  I would have guessed from
>>> the description that zero is never an allowed return value.  In fact,
>>> I wrote code that freecon'd a context whenever the return value was
>>> not -1.
>>>       
>> freecon(NULL) is perfectly legal and harmless, like free(NULL), so that
>> part is ok.
>>
>> It is possible to set extended attributes with no values, e.g. 
>> 	$ setfattr -n user.foo /path/to/foo
>> 	$ getfattr -n user.foo /path/to/foo
>> and directly calling getxattr() on that file will return 0.
>>
>> So technically this is a possible case, even if it is unusual and was
>> introduced in this case by the proc sysctl rewrite in the kernel leaving
>> us with "private" /proc/sys inodes.
>>
>> I'd be inclined to change security_inode_getsecurity() in the kernel to
>> return -EOPNOTSUPP in the IS_PRIVATE(inode) case.  But that won't help
>> with current kernels, of course.
>>
>> libselinux could remap a zero return from getxattr to a -1 return with
>> errno EOPNOTSUPP in the meantime if we want to present this behavior to
>> applications now.
>>     
>
> Like so:
>   

I'd almost rather we use the same return semantics as fgetxattr(), 
unfortunately that would impact many existing applications (though how 
many of them explicitly check for ret == 0?). I suppose this is an OK 
assumption though since even on filesystems where xattrs aren't 
supported getfilecon() will return the canonicalized security server 
version which should always return *something*.

Are you suggesting this get merged or just RFC'ing?

> Index: trunk/libselinux/src/fgetfilecon.c
> ===================================================================
> --- trunk/libselinux/src/fgetfilecon.c	(revision 2492)
> +++ trunk/libselinux/src/fgetfilecon.c	(working copy)
> @@ -37,6 +37,11 @@
>  		ret = fgetxattr(fd, XATTR_NAME_SELINUX, buf, size - 1);
>  	}
>        out:
> +	if (ret == 0) {
> +		/* Re-map empty attribute values to errors. */
> +		errno = EOPNOTSUPP;
> +		ret = -1;
> +	}
>  	if (ret < 0)
>  		free(buf);
>  	else
> Index: trunk/libselinux/src/lgetfilecon.c
> ===================================================================
> --- trunk/libselinux/src/lgetfilecon.c	(revision 2492)
> +++ trunk/libselinux/src/lgetfilecon.c	(working copy)
> @@ -37,6 +37,11 @@
>  		ret = lgetxattr(path, XATTR_NAME_SELINUX, buf, size - 1);
>  	}
>        out:
> +	if (ret == 0) {
> +		/* Re-map empty attribute values to errors. */
> +		errno = EOPNOTSUPP;
> +		ret = -1;
> +	}
>  	if (ret < 0)
>  		free(buf);
>  	else
> Index: trunk/libselinux/src/getfilecon.c
> ===================================================================
> --- trunk/libselinux/src/getfilecon.c	(revision 2492)
> +++ trunk/libselinux/src/getfilecon.c	(working copy)
> @@ -37,6 +37,11 @@
>  		ret = getxattr(path, XATTR_NAME_SELINUX, buf, size - 1);
>  	}
>        out:
> +	if (ret == 0) {
> +		/* Re-map empty attribute values to errors. */
> +		errno = EOPNOTSUPP;
> +		ret = -1;
> +	}
>  	if (ret < 0)
>  		free(buf);
>  	else
>
>   



--
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: getfilecon return code

[John D. Ramsdell]

Steve, 

Thanks for your quick reply.

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

> On Mon, 2007-07-09 at 14:07 -0400, John D. Ramsdell wrote:
> > ... In fact, I wrote code that freecon'd a context whenever the
> > return value was not -1.
> 
> freecon(NULL) is perfectly legal and harmless, like free(NULL), so
> that part is ok.

There is also the case of printing a security context.  The getfilecon
program that is part of the libselinux package thinks one can print a
security context if the return code is non-negative, and thus prints:

/proc/sys/kernel/pid_max	(null)

If some other program tried to use the second field of this output as
a security context, it could spell trouble.

John


--
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: getfilecon return code

[Stephen Smalley]

On Mon, 2007-07-09 at 15:13 -0400, Joshua Brindle wrote:
> Stephen Smalley wrote:
> > On Mon, 2007-07-09 at 14:30 -0400, Stephen Smalley wrote:
> >   
> >> On Mon, 2007-07-09 at 14:07 -0400, John D. Ramsdell wrote:
> >>     
> >>> I hadn't carefully read the manual page for getfilecon until now, but
> >>> I notice it states that a positive number is returned indicating the
> >>> number of bytes malloc'd for the context, and -1 is returned
> >>> indicating failure and that errno is set.  I would have guessed from
> >>> the description that zero is never an allowed return value.  In fact,
> >>> I wrote code that freecon'd a context whenever the return value was
> >>> not -1.
> >>>       
> >> freecon(NULL) is perfectly legal and harmless, like free(NULL), so that
> >> part is ok.
> >>
> >> It is possible to set extended attributes with no values, e.g. 
> >> 	$ setfattr -n user.foo /path/to/foo
> >> 	$ getfattr -n user.foo /path/to/foo
> >> and directly calling getxattr() on that file will return 0.
> >>
> >> So technically this is a possible case, even if it is unusual and was
> >> introduced in this case by the proc sysctl rewrite in the kernel leaving
> >> us with "private" /proc/sys inodes.
> >>
> >> I'd be inclined to change security_inode_getsecurity() in the kernel to
> >> return -EOPNOTSUPP in the IS_PRIVATE(inode) case.  But that won't help
> >> with current kernels, of course.
> >>
> >> libselinux could remap a zero return from getxattr to a -1 return with
> >> errno EOPNOTSUPP in the meantime if we want to present this behavior to
> >> applications now.
> >>     
> >
> > Like so:
> >   
> 
> I'd almost rather we use the same return semantics as fgetxattr(), 
> unfortunately that would impact many existing applications (though how 
> many of them explicitly check for ret == 0?). I suppose this is an OK 
> assumption though since even on filesystems where xattrs aren't 
> supported getfilecon() will return the canonicalized security server 
> version which should always return *something*.

*getxattr() can return with errno EOPNOTSUPP (file doesn't support the
getxattr operation) or ENODATA (file supports the operation but has no
attribute value set).

Also, callers of *getxattr() typically use the length returned by it
since they cannot assume that the attribute value is a string (or
terminated) and since they are also responsible for allocation of the
value buffer.  In contrast, callers of *getfilecon() are generally only
checking for success/failure since *getfilecon() internally allocates
the buffer and ensures that the string is terminated.

> Are you suggesting this get merged or just RFC'ing?

I was suggesting it for merge, although comments are certainly welcome.

> > Index: trunk/libselinux/src/fgetfilecon.c
> > ===================================================================
> > --- trunk/libselinux/src/fgetfilecon.c	(revision 2492)
> > +++ trunk/libselinux/src/fgetfilecon.c	(working copy)
> > @@ -37,6 +37,11 @@
> >  		ret = fgetxattr(fd, XATTR_NAME_SELINUX, buf, size - 1);
> >  	}
> >        out:
> > +	if (ret == 0) {
> > +		/* Re-map empty attribute values to errors. */
> > +		errno = EOPNOTSUPP;
> > +		ret = -1;
> > +	}
> >  	if (ret < 0)
> >  		free(buf);
> >  	else
> > Index: trunk/libselinux/src/lgetfilecon.c
> > ===================================================================
> > --- trunk/libselinux/src/lgetfilecon.c	(revision 2492)
> > +++ trunk/libselinux/src/lgetfilecon.c	(working copy)
> > @@ -37,6 +37,11 @@
> >  		ret = lgetxattr(path, XATTR_NAME_SELINUX, buf, size - 1);
> >  	}
> >        out:
> > +	if (ret == 0) {
> > +		/* Re-map empty attribute values to errors. */
> > +		errno = EOPNOTSUPP;
> > +		ret = -1;
> > +	}
> >  	if (ret < 0)
> >  		free(buf);
> >  	else
> > Index: trunk/libselinux/src/getfilecon.c
> > ===================================================================
> > --- trunk/libselinux/src/getfilecon.c	(revision 2492)
> > +++ trunk/libselinux/src/getfilecon.c	(working copy)
> > @@ -37,6 +37,11 @@
> >  		ret = getxattr(path, XATTR_NAME_SELINUX, buf, size - 1);
> >  	}
> >        out:
> > +	if (ret == 0) {
> > +		/* Re-map empty attribute values to errors. */
> > +		errno = EOPNOTSUPP;
> > +		ret = -1;
> > +	}
> >  	if (ret < 0)
> >  		free(buf);
> >  	else
> >
> >   
> 
-- 
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: getfilecon return code

[Joshua Brindle]

Stephen Smalley wrote:
> On Mon, 2007-07-09 at 15:13 -0400, Joshua Brindle wrote:
>   
>> Stephen Smalley wrote:
>>     
>>> On Mon, 2007-07-09 at 14:30 -0400, Stephen Smalley wrote:
>>>   
>>>       
>>>> On Mon, 2007-07-09 at 14:07 -0400, John D. Ramsdell wrote:
>>>>     
>>>>         
>>>>> I hadn't carefully read the manual page for getfilecon until now, but
>>>>> I notice it states that a positive number is returned indicating the
>>>>> number of bytes malloc'd for the context, and -1 is returned
>>>>> indicating failure and that errno is set.  I would have guessed from
>>>>> the description that zero is never an allowed return value.  In fact,
>>>>> I wrote code that freecon'd a context whenever the return value was
>>>>> not -1.
>>>>>       
>>>>>           
>>>> freecon(NULL) is perfectly legal and harmless, like free(NULL), so that
>>>> part is ok.
>>>>
>>>> It is possible to set extended attributes with no values, e.g. 
>>>> 	$ setfattr -n user.foo /path/to/foo
>>>> 	$ getfattr -n user.foo /path/to/foo
>>>> and directly calling getxattr() on that file will return 0.
>>>>
>>>> So technically this is a possible case, even if it is unusual and was
>>>> introduced in this case by the proc sysctl rewrite in the kernel leaving
>>>> us with "private" /proc/sys inodes.
>>>>
>>>> I'd be inclined to change security_inode_getsecurity() in the kernel to
>>>> return -EOPNOTSUPP in the IS_PRIVATE(inode) case.  But that won't help
>>>> with current kernels, of course.
>>>>
>>>> libselinux could remap a zero return from getxattr to a -1 return with
>>>> errno EOPNOTSUPP in the meantime if we want to present this behavior to
>>>> applications now.
>>>>     
>>>>         
>>> Like so:
>>>   
>>>       
>> I'd almost rather we use the same return semantics as fgetxattr(), 
>> unfortunately that would impact many existing applications (though how 
>> many of them explicitly check for ret == 0?). I suppose this is an OK 
>> assumption though since even on filesystems where xattrs aren't 
>> supported getfilecon() will return the canonicalized security server 
>> version which should always return *something*.
>>     
>
> *getxattr() can return with errno EOPNOTSUPP (file doesn't support the
> getxattr operation) or ENODATA (file supports the operation but has no
> attribute value set).
>
> Also, callers of *getxattr() typically use the length returned by it
> since they cannot assume that the attribute value is a string (or
> terminated) and since they are also responsible for allocation of the
> value buffer.  In contrast, callers of *getfilecon() are generally only
> checking for success/failure since *getfilecon() internally allocates
> the buffer and ensures that the string is terminated.
>
>   
>> Are you suggesting this get merged or just RFC'ing?
>>     
>
> I was suggesting it for merge, although comments are certainly welcome.
>   

I'm satisfied with these answers.

Acked-By: Joshua Brindle <method@manicmethod.com>

for trunk, I'm not sure if this is suitable for stable/1_0 since it 
changes existing behavior, do you disagree?

>   
>>> Index: trunk/libselinux/src/fgetfilecon.c
>>> ===================================================================
>>> --- trunk/libselinux/src/fgetfilecon.c	(revision 2492)
>>> +++ trunk/libselinux/src/fgetfilecon.c	(working copy)
>>> @@ -37,6 +37,11 @@
>>>  		ret = fgetxattr(fd, XATTR_NAME_SELINUX, buf, size - 1);
>>>  	}
>>>        out:
>>> +	if (ret == 0) {
>>> +		/* Re-map empty attribute values to errors. */
>>> +		errno = EOPNOTSUPP;
>>> +		ret = -1;
>>> +	}
>>>  	if (ret < 0)
>>>  		free(buf);
>>>  	else
>>> Index: trunk/libselinux/src/lgetfilecon.c
>>> ===================================================================
>>> --- trunk/libselinux/src/lgetfilecon.c	(revision 2492)
>>> +++ trunk/libselinux/src/lgetfilecon.c	(working copy)
>>> @@ -37,6 +37,11 @@
>>>  		ret = lgetxattr(path, XATTR_NAME_SELINUX, buf, size - 1);
>>>  	}
>>>        out:
>>> +	if (ret == 0) {
>>> +		/* Re-map empty attribute values to errors. */
>>> +		errno = EOPNOTSUPP;
>>> +		ret = -1;
>>> +	}
>>>  	if (ret < 0)
>>>  		free(buf);
>>>  	else
>>> Index: trunk/libselinux/src/getfilecon.c
>>> ===================================================================
>>> --- trunk/libselinux/src/getfilecon.c	(revision 2492)
>>> +++ trunk/libselinux/src/getfilecon.c	(working copy)
>>> @@ -37,6 +37,11 @@
>>>  		ret = getxattr(path, XATTR_NAME_SELINUX, buf, size - 1);
>>>  	}
>>>        out:
>>> +	if (ret == 0) {
>>> +		/* Re-map empty attribute values to errors. */
>>> +		errno = EOPNOTSUPP;
>>> +		ret = -1;
>>> +	}
>>>  	if (ret < 0)
>>>  		free(buf);
>>>  	else
>>>
>>>   
>>>       



--
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: getfilecon return code

[Stephen Smalley]

On Mon, 2007-07-09 at 14:42 -0400, Stephen Smalley wrote:
> On Mon, 2007-07-09 at 14:30 -0400, Stephen Smalley wrote:
> > On Mon, 2007-07-09 at 14:07 -0400, John D. Ramsdell wrote:
> > > I hadn't carefully read the manual page for getfilecon until now, but
> > > I notice it states that a positive number is returned indicating the
> > > number of bytes malloc'd for the context, and -1 is returned
> > > indicating failure and that errno is set.  I would have guessed from
> > > the description that zero is never an allowed return value.  In fact,
> > > I wrote code that freecon'd a context whenever the return value was
> > > not -1.
> > 
> > freecon(NULL) is perfectly legal and harmless, like free(NULL), so that
> > part is ok.
> > 
> > It is possible to set extended attributes with no values, e.g. 
> > 	$ setfattr -n user.foo /path/to/foo
> > 	$ getfattr -n user.foo /path/to/foo
> > and directly calling getxattr() on that file will return 0.
> > 
> > So technically this is a possible case, even if it is unusual and was
> > introduced in this case by the proc sysctl rewrite in the kernel leaving
> > us with "private" /proc/sys inodes.
> > 
> > I'd be inclined to change security_inode_getsecurity() in the kernel to
> > return -EOPNOTSUPP in the IS_PRIVATE(inode) case.  But that won't help
> > with current kernels, of course.
> > 
> > libselinux could remap a zero return from getxattr to a -1 return with
> > errno EOPNOTSUPP in the meantime if we want to present this behavior to
> > applications now.
> 
> Like so:
> 
> Index: trunk/libselinux/src/fgetfilecon.c
> ===================================================================
> --- trunk/libselinux/src/fgetfilecon.c	(revision 2492)
> +++ trunk/libselinux/src/fgetfilecon.c	(working copy)
> @@ -37,6 +37,11 @@
>  		ret = fgetxattr(fd, XATTR_NAME_SELINUX, buf, size - 1);
>  	}
>        out:
> +	if (ret == 0) {
> +		/* Re-map empty attribute values to errors. */
> +		errno = EOPNOTSUPP;
> +		ret = -1;
> +	}
>  	if (ret < 0)
>  		free(buf);
>  	else
> Index: trunk/libselinux/src/lgetfilecon.c
> ===================================================================
> --- trunk/libselinux/src/lgetfilecon.c	(revision 2492)
> +++ trunk/libselinux/src/lgetfilecon.c	(working copy)
> @@ -37,6 +37,11 @@
>  		ret = lgetxattr(path, XATTR_NAME_SELINUX, buf, size - 1);
>  	}
>        out:
> +	if (ret == 0) {
> +		/* Re-map empty attribute values to errors. */
> +		errno = EOPNOTSUPP;
> +		ret = -1;
> +	}
>  	if (ret < 0)
>  		free(buf);
>  	else
> Index: trunk/libselinux/src/getfilecon.c
> ===================================================================
> --- trunk/libselinux/src/getfilecon.c	(revision 2492)
> +++ trunk/libselinux/src/getfilecon.c	(working copy)
> @@ -37,6 +37,11 @@
>  		ret = getxattr(path, XATTR_NAME_SELINUX, buf, size - 1);
>  	}
>        out:
> +	if (ret == 0) {
> +		/* Re-map empty attribute values to errors. */
> +		errno = EOPNOTSUPP;
> +		ret = -1;
> +	}
>  	if (ret < 0)
>  		free(buf);
>  	else

Applied this patch on trunk and stable, as merely returning 0 with a
NULL context was crashing find /proc/sys -context
system_u:object_r:default_t, as reported by Steve Grubb.

-- 


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