selinux October 2011 archive
Main Archive Page > Month Archives  > selinux archives
selinux: Re: [PATCH] SELinux: audit failed attempts to set inval

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

From: Eric Paris <eparis_at_nospam>
Date: Fri Oct 28 2011 - 01:36:16 GMT
To: Kyle Moffett <>

On Thu, 2011-10-27 at 21:10 -0400, Kyle Moffett wrote:
> Hi Eric!
> On Thu, Oct 27, 2011 at 17:05, 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.
> 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

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


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