1081242 - ASAN error logging conflicts with sandboxing

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

ASAN's error logging starts by doing tcgetattr on stderr (i.e., `ioctl(2, TCGETS)`) and goes downhill from there, trying to walk around procfs to translate addresses into symbols.  What this means is that we'll get nothing out of ASAN in a GMP child, and due to the current laxity of content process policies we'll get the error message but no stack.

This can be worked around by making sandboxing failures non-fatal on ASAN builds (returning with -ENOSYS in the result register instead of killing the process), but then non-ASAN-induced sandbox violations won't make the test run orange (which would likely “fix” bug 1080165, but this is not ideal).

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

More precisely: libsanitizer's symbolizing code tries to do a path search for the llvm-symbolizer executable with stat(), and then does readlink("/proc/self/exe").

If there were some way we could hook in and use NS_DescribeCodeAddress/NS_FormatCodeAddressDetails instead, that would help fix this more cleanly — those don't use any syscalls that aren't already allowed, even in the media plugin policy.  (See also bug 1080077, for which I have a working patch that uses those + NS_StackWalk in the sandbox signal handler.)

If not… non-fatal sandbox failures + convincing mozharness to recognize those log messages as errors on desktop would also work.

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

There's a better answer: __sanitizer_sandbox_on_notify().  libsanitizer already has support for avoiding filesystem access in order to cooperate with sandboxing — unsurprisingly, given that it's used by Chromium, and their CI does ASAN+sandbox runs[1].  It will still check if stderr is a tty, but it's simple enough to have the policy softly fail that.

[1]: http://www.chromium.org/developers/testing/addresssanitizer

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug1081242-asan-vs-sandbox-hg2.diff

It turns out there are a few bugs/misfeatures in libsanitizer.  Before r209773, it will try to readlink("/proc/self/exe") even if it has the value cached, and use the cache only after that fails.  And for some reason the attempts to find/run an external symbolizer (the stat() calls) aren't stopped by it on releng; I gave up trying to source-dive the compiler-rt version history to figure out why, because after a certain point it no longer seems an ideal use of time.

So, this patch notifies libsanitizer of the sandboxing, *and* causes readlink and stat (and ioctl calls on stderr) to quietly fail instead of invoking the sandbox reporter — for ASAN builds only.

Comment 4

[Christian Holler (:decoder)]

Comment on attachment 8505975 [details] [diff] [review]
bug1081242-asan-vs-sandbox-hg2.diff

If there are bugs in libsanitizer that cause you problems, then I can also file bugs for ASan itself to make it easier for you. Of course that does not solve our immediate problem.

Regarding the patch, I can't say much about the sandboxing stuff, but I know that until now, we avoided including any ASan/compiler-rt headers (sanitizer/common_interface_defs.h), I'm not even sure if they're available on the builders.

Instead, we copied the necessary header definitions to MFBT headers, e.g. mfbt/MemoryChecking.h. Maybe you can add your stuff there as well.

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Christian Holler (:decoder) from comment #4)
> If there are bugs in libsanitizer that cause you problems, then I can also
> file bugs for ASan itself to make it easier for you. Of course that does not
> solve our immediate problem.

For the readlink call, I think there's already a fix that landed more recently than the version releng is using.  For the symbolizer path search, that doesn't happen when I run it, but I'm not sure why — and finding out means either doing trial-and-error with Try runs or bisecting LLVM (and I recall that old enough versions don't build with the GCC I'm using).

The impact is just that readlink() and stat() calls will simply fail instead of crashing, in media plugins on ASAN builds only.

> Regarding the patch, I can't say much about the sandboxing stuff, but I know
> that until now, we avoided including any ASan/compiler-rt headers
> (sanitizer/common_interface_defs.h), I'm not even sure if they're available
> on the builders.

It works on Try, so the headers are available, for what it's worth.

> Instead, we copied the necessary header definitions to MFBT headers, e.g.
> mfbt/MemoryChecking.h. Maybe you can add your stuff there as well.

I guess that explains why searching for includes of the libsanitizer header wasn't finding anything.  I actually like that approach better, because I can locally define the arguments struct and it will Just Work with older libsanitizers (where __sanitizer_sandbox_on_notify takes a void* and ignores it).  But I don't know if it makes sense to put this in MFBT, given that the only consumer currently is the Linux sandbox.

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8505975 [details] [diff] [review]
bug1081242-asan-vs-sandbox-hg2.diff

Actually, I think I might know what's going on with the symbolizer path.

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I know what's going on here, and it's annoying.  What happens at __sanitizer_sandbox_on_notify time is the $PATH searching and the decision of what symbolizer to use.  On my local build it found nothing (no llvm-symbolizer in $PATH, and allow_addr2line was false), but on automation we set $ASAN_SYMBOLIZER_PATH.  And when the crash happens, ASAN uses whatever symbolizer object it constructed.

(For extra fun, it doesn't even use getenv to obtain $PATH; it has a replacement called GetEnv that reads the file /proc/self/environ the first time it's called and caches the result, so I can't even unsetenv("PATH") before calling __sanitizer_sandbox_on_notify or anything like that.)

If this were inside libsanitizer, I could do `common_flags()->external_symbolizer_path = "";`, but it's not, so I can't.  That might be worth filing a bug against ASAN.

But until then, and until that gets onto automation, we'll need workarounds.

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug1081242-asan-vs-sandbox-hg3.diff

The same workaround, but better comments, and copying the __sanitizer declarations instead of including them.

Comment 9

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug1081242-asan-vs-sandbox-hg4.diff

Rebase; carrying over r+.

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=14c9dff182fa

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2f036dd38b4

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c2f036dd38b4