[sanitizer] Disable writes to log files for binaries in a secure context.

[bigb4ng]

Fix for google/sanitizers#1130.

An original issue described by Szabolcs Nagy at https://seclists.org/oss-sec/2016/q1/363.

Implemented by disabling setting log_path in ASAN_OPTIONS to values other then "stderr" and "stdout".

The fix provided for Linux and Android API 18+ based on AT_SECURE auxv variable (man 3 getauxval is your friend)..

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: None (bigb4ng)

Changes

Fix for google/sanitizers#1130.

An original issue described by Szabolcs Nagy at https://seclists.org/oss-sec/2016/q1/363.

Implemented by disabling setting log_path in ASAN_OPTIONS to values other then "stderr" and "stdout".

The fix provided for Linux and Android API 18+ based on AT_SECURE auxv variable (man 3 getauxval is your friend)..

---

Full diff: https://github.com/llvm/llvm-project/pull/92593.diff

1 Files Affected:

• (modified) compiler-rt/lib/sanitizer_common/sanitizer_file.cpp (+17)

diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_file.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_file.cpp
index 7ef499ce07b13..d9682145a62e4 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_file.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_file.cpp
@@ -19,6 +19,13 @@
 
 #include "sanitizer_common.h"
 #include "sanitizer_file.h"
+
+#if SANITIZER_LINUX || (SANITIZER_ANDROID && __ANDROID_API__ >= 18)
+// Android API as per https://developer.android.com/ndk/guides/cpu-features#features_using_libcs_getauxval3
+#  include <sys/auxv.h>
+#  define HAS_GETAUXVAL
+#endif
+
 #  include "sanitizer_interface_internal.h"
 
 namespace __sanitizer {
@@ -104,6 +111,16 @@ void ReportFile::SetReportPath(const char *path) {
     }
   }
 
+#ifdef HAS_GETAUXVAL
+  if (getauxval(AT_SECURE) != 0 && path &&
+      internal_strcmp(path, "stderr") != 0 &&
+      internal_strcmp(path, "stdout") != 0) {
+    Report(
+        "ERROR: Permission denied setting log_path for a binary in a secure context. You must run on a same priviledge level\n");
+    Die();
+  }
+#endif
+
   SpinMutexLock l(mu);
   if (fd != kStdoutFd && fd != kStderrFd && fd != kInvalidFd)
     CloseFile(fd);

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[fmayer]

Thank you for your contribution.

LGTM, looks like a clear improvement. More generally, I would kind of prefer if each option was either allowed or disallowed with AT_SECURE, but that means we would need a separate one for 'stdout' and 'stderr' only. Let's merge this for now, and think of a more general solution later.

[fmayer]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
View the diff from clang-format here.

Please run git clang-format HEAD~

[fmayer]

Sorry for the bother, but please do not format parts of the CL that you didn't change. Did you run git clang-format HEAD~? That should only format your diff.

[bigb4ng]

@fmayer Yeah, my bad. No bother at all was already on it :)

[fmayer]

Thanks a lot for your work, looks good to me. Sending to @vitalybuka just to get a second set of eyes on it.

[vitalybuka]

why not just return false?

[fmayer]

This is for older versions of glibc that don't have getauxval, or other posix OS that don't have it.

[vitalybuka]

would be possible to test this?

[vitalybuka]

Seems like behavior change which can brake existing users.
Also I don't think it's reasonable to link sanitizer runtimes in security sensitive processes. Sanitizers are bug detection tools, but runtime code was not build as mitigation. Only -fsanitizer-trap= can run without runtimes, but it's supported only UBSAN.

So I am not sure that we should break behavior to improve "not a goal" use cases.

@eugenis @dvyukov

[fmayer]

@eugenis suggested this approach in google/sanitizers#1130 (comment)

[vitalybuka]

@eugenis suggested this approach in google/sanitizers#1130 (comment)

Maybe @eugenis wants to reconsider?

At least I will rerun our internal tests to see if it breaks stuff.

[bigb4ng]

@vitalybuka I see your point. As long as there will be a clear warning in the docs about the dangers of using a sanitizer in a production environment, this can become a won't-fix.

As an alternative, we can consider jailing the path in a smarter way, but there are downsides to this because writing files as a more privileged software albeit in jailed path can become a weaknesses that can potentially be exploited.

As for testing, it should be possible with some linux magic:

unshare -Ur
chmod +s ./suid
unshare -U
ASAN_OPTIONS='log_path=a' ./suid

[fmayer]

@eugenis PTAL

[eugenis]

I do not have a strong opinion here. It is certainly useful to build setiud binaries with sanitizers, for testing. It is very convenient to simply build everything with asan/hwasan, without cherry-picking, like we do in Android for memory testing, but with understanding that there are security holes.

We've got ubsan_minimal runtime library that is half way between trap mode and a full sanitizer runtime, developed with security in mind.

If there is no viable plan to make sanitizer runtime production-level secure, then maybe we should err on the side of usability.

[fmayer]

I do not have a strong opinion here. It is certainly useful to build setiud binaries with sanitizers, for testing. It is very convenient to simply build everything with asan/hwasan, without cherry-picking, like we do in Android for memory testing, but with understanding that there are security holes.

I think the question is whether we are likely to need a custom log path for these cases, or not.

[bigb4ng]

Upon revisiting this PR, I have devised an alternative solution. We can enhance security by implementing a privilege-dropping mechanism when writing log files. This would involve temporarily dropping effective user and group IDs, as well as effective capabilities, when obtaining a log file descriptor.

This approach should effectively mitigate the attack surface exposed through command-line arguments. However, I have yet to formulate an equivalent solution for addressing potential SELinux use cases.

I welcome all insights and suggestions.

[vitalybuka]

@hctim google/sanitizers#1130
I'd recommend to close with works as intended, and just don't do it. I am not convinced it has practical value, but I have limited experience with SELinux.

[bigb4ng]

I would strongly recommend against closing an open security issue with publicly available exploits without documenting that such behavior (writing arbitrary files in a context of the running binary) is intended. Otherwise, this might be the best solution after all.

[hctim]

I think there's two concerns:

1. We don't want to break existing users, and
2. Putting something like this in the code may give the impression that we are really trying very hard to solve this problem, and that it's okay to build setuid binaries with sanitizers and expect it to be a security mitigation.

But it's cheap and solves a known problem.

I'd be in favour of landing a slightly different version of this patch. When we see setuid or some of the other things in AT_SECURE, let's just not do a some cheap stuff (but not die). So:

1. Disable the symbolizer (not currently in this patch but should happen),
2. Simply don't allow a log path to be set, don't die, but just return early.
3. Add a log around both areas saying "you're running a setuid binary under sanitizers, this is not a security mitigation, and you may be actually weakening your security by doing this because the sanitizers were not written with these kind of constraints in mind".

This approach should effectively mitigate the attack surface exposed through command-line arguments. However, I have yet to formulate an equivalent solution for addressing potential SELinux use cases.

Can you elaborate on what you're concerned about here? A user with selinux permissions to run setuid binaries, and they use sanitizers as a trojan horse to execute syscalls they don't have access to? IMHO this is a "write bad selinux, deal with consequences" problem. I highly doubt any setuid software is designed to not be used as a syscall-trojan-horse.

[bigb4ng]

@hctim

Can you elaborate on what you're concerned about here? A user with selinux permissions to run setuid binaries, and they use sanitizers as a trojan horse to execute syscalls they don't have access to? IMHO this is a "write bad selinux, deal with consequences" problem. I highly doubt any setuid software is designed to not be used as a syscall-trojan-horse.

Sure! Mostly it's the same concern described in the original writeup. A user with SELinux permissions to run setuid binaries (which is not a "bad selinux" by itself) and a setuid binary compiled with ASAN for "extra security". As there is no warning not to compile setuid with ASAN it may seem harmless and such setuids may end up in certain production environments.

The issue with SELinux in regard to dropping privilege is that runtime domain is provided based on e.g. file attributes and by design cannot assume privileges of the process owner.

[fmayer]

The way AT_SECURE interacts with SELinux is as follows: if a binary call results in a domain transition, it is considered AT_SECURE because it runs with different permissions from the caller.

[bigb4ng]

The way AT_SECURE interacts with SELinux is as follows: if a binary call results in a domain transition, it is considered AT_SECURE because it runs with different permissions from the caller.

Just to clarify, the issue I described is for alternative drop-privilege solution I proposed earlier. SELinux use-cases will indeed be patched by the current version of the PR.