XDG overrides can be used to inject a library to suid process

[abbradar]

From reading of source code, env override is disabled for suid binaries on this line but this doesn't affect using XDG paths in any way -- it only depends on override == NULL later. One can override XDG_CONFIG_DIRS with malicious configuration and run a setuid process, which then will inject malicious library into itself.

I haven't actually checked that it would work and was just reading the source code, so I'm sorry if I'm mistaken!

[Ralith]

Thanks for the code review! On reviewing it myself, I confirmed XDG_CONFIG_DIRS and XDG_DATA_DIRS are accessed with secure_getenv, which I understand to return NULL when the process is SUID or SGID. Therefore, I do not believe this report is valid as written.

However, this report did prompt me to review the loader's use of environment variables in general, and loader_add_layer_env looks pretty suspicious; it does not use secure_getenv and looks like it's invoked unconditionally during vkCreateInstance.

[abbradar]

@Ralith Oh, I have not noticed secure_getenv -- I feel embarrassed and should have been more attentive, sorry for the noise.

About loader_add_layer_env -- from a very cursory glance it only adds layers from already filtered sources (i.e. from "approved" layer locations) so it should be fine. I'm not sure however -- that is literally after two minutes of reading.

[MarkY-LunarG]

Forgive my ignorance, but is there a reason we should not always use secure_getenv? I could change the one, or I could change the loader_getenv to always use secure_getenv on Linux and just replace the manual usages of secure_getenv back to loader_getenv.

[MarkY-LunarG]

I didn't hear back, so I've switched all loader getenv calls with secure_getenv if that's available. This change is in commit f7a2742355bb67cd3f2d7a7e3dce36f7249f8bd5. Please let me know if I missed anything. Otherwise, I'll close this issue in a few days.

[Ralith]

The downside with secure_getenv is, of course, that it doesn't work when the process is SUID, so if there are any uses of environment variables which do not represent a security vulnerability, said uses become unreasonable. I'm not 100% sure what the semantics of all environment variables read by the loader are, but e.g. @abbradar suggested that VK_INSTANCE_LAYERS may be one such.

f7a2742355bb67cd3f2d7a7e3dce36f7249f8bd5 falls back to exploitable behavior if secure_getenv is unavailable. Are you sure that's appropriate? To quote the manpage, secure_getenv returns NULL when:

• the process's effective user ID did not match its real user ID or the process's effective group ID did not match its real group ID (typically this is the result of executing a set-user-ID or set-group-ID program);
• the effective capability bit was set on the executable file; or
• the process has a nonempty permitted capability set.

It seems desirable to manually perform a similar set of tests when accessing sensitive environment variables such as layer search paths if secure_getenv cannot be a hard dep.

[Ralith]

getauxval(AT_SECURE) from sys/auxv.h appears to be a good, forwards-compatible solution for testing whether the environment can be trusted on Linux, but is not available on BSDs. issetugid might be appropriate on BSDs, but I haven't checked carefully.

[MarkY-LunarG]

Some libc's don't support secure_getenv at all, like musl. For our official loader builds, we use a C library that does include this. Only people building their own loader will run into issues with falling back to a non-secure getenv. I'll add a pragma which triggers a warning in these cases as well, that should fail on our internal builds if it ever happens.

As far as environment variables go, we have several that could cause different drivers/layers to load that could also be a security concern. I'll investigate and use the secure get on only those.

Thanks again, I'll update with the hopefully final commit.

[Ralith]

To be clear, I believe the behavior of secure_getenv can be reliably emulated on other libcs by guarding getenv calls with something like if(getauxval(AT_SECURE)) return NULL;, removing any need for unsafe behavior on non-glibc. This is very linux-specific but I don't think it's glibc-specific--musl certainly defines it, at least, and I suspect any working libc needs the facilities to set up prior to main properly.

[MarkY-LunarG]

I've committed all my changes with a984b99946b3c8a7d047861698972da26a032c61 and 60dc418bdc8ca3000f254c2a8940cf73d9b42bce. It doesn't fix the case which doesn't find secure_getenv or __secure_getenv. I'm hesitant to make the getauxval change without being able to test it on more platforms.

I would file a separate issue from this since the original issue should be fixed by the above commits, and the remaining issue is limited to user developer loader's on systems with no direct support of the secure versions.