selinux: handle getcon_raw producing a NULL pointer, despite returning 0

[CmdrMoozy]

Previously, we assumed that success meant we definitely got a valid
pointer. There is at least one edge case where this is not true (i.e.,
we can get both a 0 return value, and also a NULL pointer):
https://github.com/SELinuxProject/selinux/blob/4246bb550dee5246c8567804325b7da206cd76cf/libselinux/src/procattr.c#L175

When this case occurrs, if we don't check the pointer we SIGSEGV in
early initialization.

[keszybz]

Pfffffffffffffff. LGTM.

[keszybz]

semaphoreci:

+ sudo lxc-create -n buster-amd64 -t download -- -d debian -r buster -a amd64 --keyserver hkp://keyserver.ubuntu.com:80
Setting up the GPG keyring
Downloading the image index
ERROR: Invalid signature for /tmp/tmp.TpoXRXrmoY/index.asc
lxc-create: buster-amd64: lxccontainer.c: create_run_template: 1617 Failed to create container from template
lxc-create: buster-amd64: tools/lxc_create.c: main: 327 Failed to create container buster-amd64

Unrelated.

[cgzones]

FWIW, this should really never happen,
see https://lore.kernel.org/selinux/CAEjxPJ5nExmrkbgMfD2B9cs-iOdSAVnNfqcqbfns10vOa31=5Q@mail.gmail.com/T/#r33960dc91492b5013b1d376fc980014b568918a3

[CmdrMoozy]

Interesting. I have a kernel upon which it does happen (this was my motivation for the patch), but admittedly it's a weird kernel, in that it has a lot of Google patches so it diverges significantly from upstream. It's totally possible it's just misbehaving in some way. Thanks for the pointer, I'll investigate and see if I can make this kernel a bit less weird. :)

[poettering]

What is getcon_raw() returning 0 with NULL supposed to mean though? Can a process have no label on Selinux? this sounds fishy to me, like paving over some bug elsewhere, and possibly something better caught with an assert()

[keszybz]

I don't think we should assert on data coming from an external library that is subject to user configuration, especially one as fickle as selinux.

[poettering]

hmm, well, the way i saw it so far is that getcon_raw() is a thin wrapper around a syscall, and assert()ing on impossible errors of kernel syscalls i think is OK... but anyway, my main point is: this looks fishy, and nothing to just tape over here...

[poettering]

i simply don't see why we should assume selinux has been initialized if getcon_raw() returns NULL. Because that's what this patch effectively does...

[CmdrMoozy]

It's not a syscall exactly. getcon_raw is returning NULL because it open()-s /proc//attr/, and then when it tries to read() it we just get EOF immediately (source).

Ignoring my patch, we would have assumed the context hadn't been initialized in any number of cases ... if asprintf() to get the /proc path fails to allocate, or if the open() fails, or etc. Actually the asprintf() case seems really weird to me, but that's a separate issue.

I don't think the thread @cgzones linked to tells the whole story. It's true that these functions share code, but my reading of the code is that getprocattrcon_raw (specifically the one which is actually returning 0 + NULL) is only for this particular attribute, it isn't shared among other kinds of attributes.

It's not exactly clear to me that it's a "kernel bug" for the file to exist, but be empty - I'll investigate that a bit more today and see why exactly that's happening on this particular kernel.

The fact that the libselinux code, which exists specifically for getcon_raw as far as I can tell, already checks for early EOF, makes me think that perhaps my kernel is not so weird after all though. Clearly whoever wrote that thought EOF was a possibility that needed to be handled?

[CmdrMoozy]

Ah, I guess this is the re-use they were talking about on the selinux list. These macros sure make it hard to grep for where things are used. :/

I looked more closely at the selinux code in the kernel, and I can see a bunch of code paths where we might get a zero length. Maybe these "should never happen", but the code at least allows for them.

The procfs code accounts for a zero length here.

This can happen for example if this function returns an error here.

There are several -ENOMEM conditions in which this might happen. There are also several -EINVAL exit codes, if the state is not what we expected. See this function.

There are enough different code paths where this can happen that I can't tell why this is happening in my specific case purely by inspection. If you feel strongly, let me know and I can track down the specific code path I'm hitting.

FWIW, I also looked a bit more closely at what weird patches this Google kernel might have. At least with git log --author='@google\.com' --perl-regexp security/selinux/ I don't see any out-of-tree patches. There are many backports though; this kernel is based on 4.15.

[cgzones]

getcon() (and getcon_raw(), which skips context translation, see man:setrans.conf(8)) is a wrapper around reading /proc/self/attr/current.

This userspace library function might only return 0 (success) and set *context to NULL if the read syscall returns 0. (mentioned in the commit message)

There is at least one edge case where this is not true (i.e.,
we can get both a 0 return value, and also a NULL pointer):
https://github.com/SELinuxProject/selinux/blob/4246bb550dee5246c8567804325b7da206cd76cf/libselinux/src/procattr.c#L175

This is not considered a failure (e.g. setting errno to ENODATA and ret to -1), because the function getprocattrcon_raw() is a template for different functions, e.g. set-/get-fscreatecon() (wrapper for /proc/self/attr/fscreate, see man:setfscreatecon(8)).
fscreate determines the SELinux security context used for creating a new file system object, and is valid to be empty (an empty string), which is the default and uses the default policy behaviour (see the usages of setfscreatecon_raw(NULL) in systemd).

Stephen (@stephensmalley) mentioned on the SELinux mailing list the kernel guarantees that /proc/self/attr/current contains a non-zero length string (on success), so the read syscall will never return 0 in getprocattrcon_raw().

I looked more closely at the selinux code in the kernel, and I can see a bunch of code paths where we might get a zero length. Maybe these "should never happen", but the code at least allows for them.

The procfs code accounts for a zero length here.

This can happen for example if this function returns an error here.

There are several -ENOMEM conditions in which this might happen. There are also several -EINVAL exit codes, if the state is not what we expected. See this function.

In case of a kernel failure providing /proc/self/attr/current the read syscall should propagate the failure, and also not return 0.

i simply don't see why we should assume selinux has been initialized if getcon_raw() returns NULL. Because that's what this patch effectively does...

I do not think the commit does this: in case getcon_raw() sets the context to NULL, the initialization check

systemd/src/core/selinux-setup.c

Lines 51 to 57 in b1d666a

	/* Already initialized by somebody else? */
	r = getcon_raw(&con);
	/* getcon_raw can return 0, and still give us a NULL pointer. */
	if (r == 0 && con) {
	initialized = !streq(con, "kernel");
	freecon(con);
	}

is skipped.

p.s.: I am not opposed to this commit, as under normal conditions it's just an unnecessary always-true NULL-check

[CmdrMoozy]

Actually, does the LSM API require this guarantee in general? It looks like what's happening here is that on my kernel we're actually calling a getprocattr function not from SELinux, but from some other LSM (some out-of-tree thing which is quite similar to PaX's mprotect). I reproduced this with CONFIG_SECURITY_SELINUX unset.

It looks like this is basically because other security modules reuse /proc/self/attr/current, and without this commit: torvalds/linux@6d9c939 (which my kernel doesn't have, being that it's 4.15) we don't do anything to check which LSM is actually getting called.

[cgzones]

I see now.
So we (may) read /proc/self/attr/current before SELinux is enabled (so the SELinux guarantee is void).

[cgzones]

Maybe we could improve the comment to something like:

/* getcon_raw can return 0, and still give us a NULL pointer if
 * SELinux is not yet initialized and another LSM provides /proc/self/attr/current. 
 */

Also with a possible stacking of LSM's in the future and not to rely on SELinux policy writers using the string "kernel" in the kernel initial sid, we may want in the near future to change the already-initialized logic from /proc/self/attr/current-contains-string-kernel to checking if the SELinux filesystem (/sys/fs/selinux or old style /selinux) exists.