[PATCH] SELinux: audit failed attempts to set invalid labels

[PATCH] SELinux: audit failed attempts to set invalid labels

[Eric Paris]

We know that some yum operation is causing CAP_MAC_ADMIN failures.  This
implies that an RPM is laying down (or attempting to lay down) a file with
an invalid label.  The problem is that we don't have any information to
track down the cause.  This patch with cause such a failure to report the
failed label in an SELINUX_ERR audit message.  This is similar to the
SELINUX_ERR reports on invalid transitions and things like that.  It should
help run down problems on what is trying to set invalid labels in the
future.

Resulting records look something like:
type=AVC msg=audit(1319659241.138:71): avc:  denied  { mac_admin } for pid=2594 comm="chcon" capability=33 scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tcontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tclass=capability2
type=SELINUX_ERR msg=audit(1319659241.138:71): op=setxattr invalid_context=unconfined_u:object_r:hello:s0
type=SYSCALL msg=audit(1319659241.138:71): arch=c000003e syscall=188 success=no exit=-22 a0=a2c0e0 a1=390341b79b a2=a2d620 a3=1f items=1 ppid=2519 pid=2594 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=1 comm="chcon" exe="/usr/bin/chcon" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)
type=CWD msg=audit(1319659241.138:71):  cwd="/root" type=PATH msg=audit(1319659241.138:71): item=0 name="test" inode=785879 dev=fc:03 mode=0100644 ouid=0 ogid=0 rdev=00:00 obj=unconfined_u:object_r:admin_home_t:s0

Signed-off-by: Eric Paris <eparis@redhat.com>
---

 security/selinux/hooks.c |   20 ++++++++++++++++++--
 1 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 2887517..a925bf3 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2767,8 +2767,16 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name,
 
 	rc = security_context_to_sid(value, size, &newsid);
 	if (rc == -EINVAL) {
-		if (!capable(CAP_MAC_ADMIN))
+		if (!capable(CAP_MAC_ADMIN)) {
+			struct audit_buffer *ab;
+
+			ab = audit_log_start(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR);
+			audit_log_format(ab, "op=setxattr invalid_context=");
+			audit_log_n_untrustedstring(ab, value, strnlen(value, size));
+			audit_log_end(ab);
+
 			return rc;
+		}
 		rc = security_context_to_sid_force(value, size, &newsid);
 	}
 	if (rc)
@@ -5277,8 +5285,16 @@ static int selinux_setprocattr(struct task_struct *p,
 		}
 		error = security_context_to_sid(value, size, &sid);
 		if (error == -EINVAL && !strcmp(name, "fscreate")) {
-			if (!capable(CAP_MAC_ADMIN))
+			if (!capable(CAP_MAC_ADMIN)) {
+				struct audit_buffer *ab;
+
+				ab = audit_log_start(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR);
+				audit_log_format(ab, "op=fscreate invalid_context=");
+				audit_log_n_untrustedstring(ab, value, strnlen(value, size));
+				audit_log_end(ab);
+
 				return error;
+			}
 			error = security_context_to_sid_force(value, size,
 							      &sid);
 		}


--
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] SELinux: audit failed attempts to set invalid labels

[Kyle Moffett]

Hi Eric!

On Thu, Oct 27, 2011 at 17:05, Eric Paris <eparis@redhat.com> wrote:
> We know that some yum operation is causing CAP_MAC_ADMIN failures.  This
> implies that an RPM is laying down (or attempting to lay down) a file with
> an invalid label.  The problem is that we don't have any information to
> track down the cause.  This patch with cause such a failure to report the
> failed label in an SELINUX_ERR audit message.  This is similar to the
> SELINUX_ERR reports on invalid transitions and things like that.  It should
> help run down problems on what is trying to set invalid labels in the
> future.

So, the existing code seems to do the right thing:
>        rc = security_context_to_sid(value, size, &newsid);

But this new code looks very wrong:
> +                       audit_log_n_untrustedstring(ab, value, strnlen(value, size));

If somebody stuck a "\0" character in there that would not do what you want.

I would expect this is the correct way:
  audit_log_n_untrustedstring(ab, value, size)

I looked at audit_log_n_untrustedstring and
security_context_to_sid_core, and both seem to assume that "size" does
not include a trailing NUL.

Reading through "security_context_to_sid_core()", it appears to have a
bug when !ss_initialized:
  if (!strcmp(initial_sid_to_string[i], scontext))

Where's the references to scontext_len???

It looks like it will allow (during early boot) a process to set an
invalid xattr on a file yet cache a valid SID for the same file, EG:
"system_u:object_r:kernel_t:s0\0HelloWorld", hm?

Furthermore, the rest of that code is very non-obviously correct
regarding checking against the length instead of a NUL character.

Cheers,
Kyle Moffett

-- 
Curious about my work on the Debian powerpcspe port?
I'm keeping a blog here: http://pureperl.blogspot.com/


--
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] SELinux: audit failed attempts to set invalid labels

[Eric Paris]

On Thu, 2011-10-27 at 21:10 -0400, Kyle Moffett wrote:
> Hi Eric!
> 
> On Thu, Oct 27, 2011 at 17:05, Eric Paris <eparis@redhat.com> wrote:
> > We know that some yum operation is causing CAP_MAC_ADMIN failures.  This
> > implies that an RPM is laying down (or attempting to lay down) a file with
> > an invalid label.  The problem is that we don't have any information to
> > track down the cause.  This patch with cause such a failure to report the
> > failed label in an SELINUX_ERR audit message.  This is similar to the
> > SELINUX_ERR reports on invalid transitions and things like that.  It should
> > help run down problems on what is trying to set invalid labels in the
> > future.
> 
> So, the existing code seems to do the right thing:
> >        rc = security_context_to_sid(value, size, &newsid);
> 
> But this new code looks very wrong:
> > +                       audit_log_n_untrustedstring(ab, value, strnlen(value, size));
> 
> If somebody stuck a "\0" character in there that would not do what you want.

It does what I want, but maybe you are right that it is still wrong.  On
a normal setxattr call 'size' is the size of the string including the
nul.  I know this from testing.  My first attempt at using the audit
function included the nul in the ouput since I used exactly the line you
have below (as you noticed the audit function does not expect to include
the nul).  Which meant that the audit output was encoded rather than a
readable string since the nul was not a valid audit character.

> I would expect this is the correct way:
>   audit_log_n_untrustedstring(ab, value, size)
> 
> I looked at audit_log_n_untrustedstring and
> security_context_to_sid_core, and both seem to assume that "size" does
> not include a trailing NUL.

Actually it presumes that it MIGHT not include the nul.  If the nul is
there it will however politely add a second nul....

> Reading through "security_context_to_sid_core()", it appears to have a
> bug when !ss_initialized:
>   if (!strcmp(initial_sid_to_string[i], scontext))
> 
> Where's the references to scontext_len???
>
> It looks like it will allow (during early boot) a process to set an
> invalid xattr on a file yet cache a valid SID for the same file, EG:
> "system_u:object_r:kernel_t:s0\0HelloWorld", hm?

That is a bit interesting, and is points out the problem that my
function might have.  Maybe I should instead use

size = (value[size-1] == '\0' ? size-1 : size);
audit_log_n_untrustedstring(ab, value, size);

So I only avoid the nul if it is at the end of the buffer....

I guess really the question comes down to what that label means.  Is
"system_u:object_r:kernel_t:s0\0HelloWorld" supposed to be equal to
"system_u:object_r:kernel_t:s0"?  If the labels are strings those are
equal....

> Furthermore, the rest of that code is very non-obviously correct
> regarding checking against the length instead of a NUL character.

I'm still questioning (I only looked 2 seconds) how and if
string_to_context_struct() works properly if there wasn't already a nul
in the size....   But I agree the rest of it looks non-obviously
safe  :)

-Eric


--
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] SELinux: audit failed attempts to set invalid labels

[Stephen Smalley]

On Thu, 2011-10-27 at 21:10 -0400, Kyle Moffett wrote:
> I looked at audit_log_n_untrustedstring and
> security_context_to_sid_core, and both seem to assume that "size" does
> not include a trailing NUL.

security_context_to_sid_core() is supposed to handle it either way, with
or without a trailing NUL.  Originally it only took NUL-terminated
strings (with the original SELinux interfaces), but we had to make it
more forgiving of its input when we switched to using the xattr APIs.

> Reading through "security_context_to_sid_core()", it appears to have a
> bug when !ss_initialized:
>   if (!strcmp(initial_sid_to_string[i], scontext))
> 
> Where's the references to scontext_len???

Fair enough, we don't seem to have updated that case.  Of course, that
only happens before policy is loaded and thus anything is allowed
anyway.

> It looks like it will allow (during early boot) a process to set an
> invalid xattr on a file yet cache a valid SID for the same file, EG:
> "system_u:object_r:kernel_t:s0\0HelloWorld", hm?

Well, technically it will only accept strings with initial SID names
(e.g. "kernel" or "kernel\0HelloWorld", which aren't even valid contexts
once the policy has loaded).  And that's merely to ensure that
security_sid_to_context() followed by security_context_to_sid() on an
initial SID during bootup yields the right SID.  Those contexts should
never be stored at all, as selinux_inode_setxattr() won't let you set an
SELinux label on a filesystem until after policy has loaded.

> Furthermore, the rest of that code is very non-obviously correct
> regarding checking against the length instead of a NUL character.

That's because the original code only dealt with NUL-terminated strings,
and we merely changed security_context_to_sid_core() to create a
NUL-terminated copy for use by the rest of the code (it was already
copying it to allow modifying it during parsing, so we just extended it
by a byte and cleared the last byte).  And then
string_to_context_struct() applies a check against scontext_len just
before the policydb_context_isvalid() call to ensure that we have
consumed the entire context value during parsing.

So trying to set a context value with content after a NUL will just
return -1 with errno EINVAL; feel free to try it (I did).

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

[PATCH] SELinux: audit failed attempts to set invalid labels

[Eric Paris]

We know that some yum operation is causing CAP_MAC_ADMIN failures.  This
implies that an RPM is laying down (or attempting to lay down) a file with
an invalid label.  The problem is that we don't have any information to
track down the cause.  This patch with cause such a failure to report the
failed label in an SELINUX_ERR audit message.  This is similar to the
SELINUX_ERR reports on invalid transitions and things like that.  It should
help run down problems on what is trying to set invalid labels in the
future.

Resulting records look something like:
type=AVC msg=audit(1319659241.138:71): avc:  denied  { mac_admin } for pid=2594 comm="chcon" capability=33 scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tcontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tclass=capability2
type=SELINUX_ERR msg=audit(1319659241.138:71): op=setxattr invalid_context=unconfined_u:object_r:hello:s0
type=SYSCALL msg=audit(1319659241.138:71): arch=c000003e syscall=188 success=no exit=-22 a0=a2c0e0 a1=390341b79b a2=a2d620 a3=1f items=1 ppid=2519 pid=2594 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=1 comm="chcon" exe="/usr/bin/chcon" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)
type=CWD msg=audit(1319659241.138:71):  cwd="/root" type=PATH msg=audit(1319659241.138:71): item=0 name="test" inode=785879 dev=fc:03 mode=0100644 ouid=0 ogid=0 rdev=00:00 obj=unconfined_u:object_r:admin_home_t:s0

Signed-off-by: Eric Paris <eparis@redhat.com>
---

 security/selinux/hooks.c |   36 ++++++++++++++++++++++++++++++++++--
 1 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 2887517..d4fcd1e 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2767,8 +2767,25 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name,
 
 	rc = security_context_to_sid(value, size, &newsid);
 	if (rc == -EINVAL) {
-		if (!capable(CAP_MAC_ADMIN))
+		if (!capable(CAP_MAC_ADMIN)) {
+			struct audit_buffer *ab;
+			size_t audit_size;
+			const char *str;
+
+			/* We strip a nul only if it is at the end, otherwise the
+			 * context contains a nul and we should audit that */
+			str = value;
+			if (str[size - 1] == '\0')
+				audit_size = size - 1;
+			else
+				audit_size = size;
+			ab = audit_log_start(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR);
+			audit_log_format(ab, "op=setxattr invalid_context=");
+			audit_log_n_untrustedstring(ab, value, audit_size);
+			audit_log_end(ab);
+
 			return rc;
+		}
 		rc = security_context_to_sid_force(value, size, &newsid);
 	}
 	if (rc)
@@ -5277,8 +5294,23 @@ static int selinux_setprocattr(struct task_struct *p,
 		}
 		error = security_context_to_sid(value, size, &sid);
 		if (error == -EINVAL && !strcmp(name, "fscreate")) {
-			if (!capable(CAP_MAC_ADMIN))
+			if (!capable(CAP_MAC_ADMIN)) {
+				struct audit_buffer *ab;
+				size_t audit_size;
+
+				/* We strip a nul only if it is at the end, otherwise the
+				 * context contains a nul and we should audit that */
+				if (str[size - 1] == '\0')
+					audit_size = size - 1;
+				else
+					audit_size = size;
+				ab = audit_log_start(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR);
+				audit_log_format(ab, "op=fscreate invalid_context=");
+				audit_log_n_untrustedstring(ab, value, audit_size);
+				audit_log_end(ab);
+
 				return error;
+			}
 			error = security_context_to_sid_force(value, size,
 							      &sid);
 		}


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

[PATCH] SELinux: audit failed attempts to set invalid labels

[Eric Paris]

We know that some yum operation is causing CAP_MAC_ADMIN failures.  This
implies that an RPM is laying down (or attempting to lay down) a file with
an invalid label.  The problem is that we don't have any information to
track down the cause.  This patch with cause such a failure to report the
failed label in an SELINUX_ERR audit message.  This is similar to the
SELINUX_ERR reports on invalid transitions and things like that.  It should
help run down problems on what is trying to set invalid labels in the
future.

Resulting records look something like:
type=AVC msg=audit(1319659241.138:71): avc:  denied  { mac_admin } for pid=2594 comm="chcon" capability=33 scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tcontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tclass=capability2
type=SELINUX_ERR msg=audit(1319659241.138:71): op=setxattr invalid_context=unconfined_u:object_r:hello:s0
type=SYSCALL msg=audit(1319659241.138:71): arch=c000003e syscall=188 success=no exit=-22 a0=a2c0e0 a1=390341b79b a2=a2d620 a3=1f items=1 ppid=2519 pid=2594 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=1 comm="chcon" exe="/usr/bin/chcon" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)
type=CWD msg=audit(1319659241.138:71):  cwd="/root" type=PATH msg=audit(1319659241.138:71): item=0 name="test" inode=785879 dev=fc:03 mode=0100644 ouid=0 ogid=0 rdev=00:00 obj=unconfined_u:object_r:admin_home_t:s0

Signed-off-by: Eric Paris <eparis@redhat.com>
---

 security/selinux/hooks.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 2887517..0c277bc 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2767,8 +2767,11 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name,
 
 	rc = security_context_to_sid(value, size, &newsid);
 	if (rc == -EINVAL) {
-		if (!capable(CAP_MAC_ADMIN))
+		if (!capable(CAP_MAC_ADMIN)) {
+			audit_log(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR,
+				  "op=setxattr invalid_context=%s", (char *)value);
 			return rc;
+		}
 		rc = security_context_to_sid_force(value, size, &newsid);
 	}
 	if (rc)
@@ -5277,8 +5280,11 @@ static int selinux_setprocattr(struct task_struct *p,
 		}
 		error = security_context_to_sid(value, size, &sid);
 		if (error == -EINVAL && !strcmp(name, "fscreate")) {
-			if (!capable(CAP_MAC_ADMIN))
+			if (!capable(CAP_MAC_ADMIN)) {
+				audit_log(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR,
+					  "op=fscreate invalid_context=%s", str);
 				return error;
+			}
 			error = security_context_to_sid_force(value, size,
 							      &sid);
 		}


--
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] SELinux: audit failed attempts to set invalid labels

[Stephen Smalley]

On Wed, 2011-10-26 at 16:56 -0400, Eric Paris wrote:
> We know that some yum operation is causing CAP_MAC_ADMIN failures.  This
> implies that an RPM is laying down (or attempting to lay down) a file with
> an invalid label.  The problem is that we don't have any information to
> track down the cause.  This patch with cause such a failure to report the
> failed label in an SELINUX_ERR audit message.  This is similar to the
> SELINUX_ERR reports on invalid transitions and things like that.  It should
> help run down problems on what is trying to set invalid labels in the
> future.
> 
> Resulting records look something like:
> type=AVC msg=audit(1319659241.138:71): avc:  denied  { mac_admin } for pid=2594 comm="chcon" capability=33 scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tcontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tclass=capability2
> type=SELINUX_ERR msg=audit(1319659241.138:71): op=setxattr invalid_context=unconfined_u:object_r:hello:s0
> type=SYSCALL msg=audit(1319659241.138:71): arch=c000003e syscall=188 success=no exit=-22 a0=a2c0e0 a1=390341b79b a2=a2d620 a3=1f items=1 ppid=2519 pid=2594 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=1 comm="chcon" exe="/usr/bin/chcon" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)
> type=CWD msg=audit(1319659241.138:71):  cwd="/root" type=PATH msg=audit(1319659241.138:71): item=0 name="test" inode=785879 dev=fc:03 mode=0100644 ouid=0 ogid=0 rdev=00:00 obj=unconfined_u:object_r:admin_home_t:s0
> 
> Signed-off-by: Eric Paris <eparis@redhat.com>
> ---
> 
>  security/selinux/hooks.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 2887517..0c277bc 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2767,8 +2767,11 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name,
>  
>  	rc = security_context_to_sid(value, size, &newsid);
>  	if (rc == -EINVAL) {
> -		if (!capable(CAP_MAC_ADMIN))
> +		if (!capable(CAP_MAC_ADMIN)) {
> +			audit_log(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR,
> +				  "op=setxattr invalid_context=%s", (char *)value);

You aren't guaranteed that value is NUL-terminated.  Or even a printable
string.  Could be any arbitrary binary blob passed to setxattr(2).

>  			return rc;
> +		}
>  		rc = security_context_to_sid_force(value, size, &newsid);
>  	}
>  	if (rc)
> @@ -5277,8 +5280,11 @@ static int selinux_setprocattr(struct task_struct *p,
>  		}
>  		error = security_context_to_sid(value, size, &sid);
>  		if (error == -EINVAL && !strcmp(name, "fscreate")) {
> -			if (!capable(CAP_MAC_ADMIN))
> +			if (!capable(CAP_MAC_ADMIN)) {
> +				audit_log(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR,
> +					  "op=fscreate invalid_context=%s", str);

Likewise here.

>  				return error;
> +			}
>  			error = security_context_to_sid_force(value, size,
>  							      &sid);
>  		}
> 
> 
> --
> 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.