1297740 - ASAN UAF in PerformAsyncLaunch (if MOZ_LOG/etc are set)

Status: RESOLVED FIXED

(Core :: IPC, defect, P2)

Description

[Randell Jesup [:jesup] (needinfo me)]

I modified testing/mochitest/runtests.py to add a MOZ_LOG setting for Try, and got multiple ASAN UAFs.

It appears that the ownership model for the GeckoChildProcessHost is borked - when PerformAsyncLaunch runs (in a nonowningrunnablemethod!) the 'this' object has been deleted already.  PerformAsyncLaunch seems to only touch 'this' if MOZ_LOG or NSPR_LOG_MODULES is set.

https://treeherder.mozilla.org/logviewer.html#?job_id=26272983&repo=try#L1943
https://treeherder.mozilla.org/logviewer.html#?job_id=26272995&repo=try#L2069

Marking as sec-high instead of critical since it requires MOZ_LOG/NSPR_LOG_MODULES, which is unusual for non-mozilla-devs.  Note that this may also mean that the value doesn't get into the child process; I'm not sure.

Comment 1

[Benjamin Smedberg]

Jesup what kind of process was being launched?

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #1)
> Jesup what kind of process was being launched?

Content Process I'm 99% sure; but I suspect it happens to any of them

Comment 3

[Benjamin Smedberg]

This can't happen with NPAPI plugin processes because of the synclaunch behavior. I guess it can happen with the async-launched forms if the client decides to kill off the process before we actually get around to launching it. I'd be interested *why* that's happening, but we should fix this regardless.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Attachment: asan_crash

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

This happened again restarting a profile after a crash - session restore came up for a second, then ASAN crash.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

It repros under ASAN+rr :-)

It appears the problem is that if the Host that called PR_SetEnv goes away, the string use for PR_SetEnv() is destroyed, and subsequent PR_GetEnv's return the destroyed string (see prenv.h for why this is).  So this comment is incorrect.  I suggest for this case we simply leak the (short) string to avoid violating NSPR requirements.  I'm sure there are more complex solutions, but any string passed to PR_SetEnv() must have a lifetime past the last time it's fetched by PR_GetEnv (and past where the value fetched is used).  I'm not certain that doing it in ClearOnShutdown would even be sufficient (and that's a pain since it requires MainThread).

  // Passing temporary to PR_SetEnv is ok here if we keep the temporary
  // for the time we launch the sub-process.  It's copied to the new
  // environment.
  PR_SetEnv(buffer.BeginReading());

(Note: some process is shutting down in the SessionRestore case):
console.error: PushService: 
  stateChangeProcessEnqueue: Error transitioning state
  UnknownError

###!!! [Parent][RunMessage] Error: Channel closing: too late to send/recv, messages will be lost

[New Thread 8992.9364]
...
<spawns a new Content process and crashes with ASAN error>

Comment 7

[Jim Mathies [:jimm]]

Gabor, could you please take a look at this issue, see if there's a fix.

Comment 8

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

For what it's worth, the changes I'm proposing in bug 1401786 will make it easy to do this kind of thing without side-effecting the parent process's environment.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Cool - though until that lands, I'd love to see a fix in 58 to avoid weird stuff happening - and to avoid ASAN bitching at me after a crash (and maybe other times)

Comment 10

[Gabor Krizsanits (INACTIVE)]

Seems like :gcp is trying to land something that removes the problematic bits mentioned in Comment 6.

Bug 1386404.

Comment 11

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

For reference: the unsafe use of PR_SetEnv seems to have originated in bug 534764.  Bug 1248565 made it worse (always a use-after-free rather than conditional/racy) but then bug 1270752 undid that part.  There's also another instance of this anti-pattern in the profiler (AutoSetProfilerEnvVarsForChildProcess); that one might be safe but we should clean it up at some point.


But… is this really the same problem as in comment #0?  The logs have expired by now, but if that's about the entire GeckoChildProcessHost object being deleted then it's a different bug from the env var problems.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

I reported the original; I think this is likely the same issue.  It was easier to investigate with rr...

Comment 13

[Gian-Carlo Pascutto [:gcp]]

Bug 1386404 is somewhat blocked on audioipc using locations on the filesystem, so I will extract the relevant parts and land them here first.

Comment 14

[Gian-Carlo Pascutto [:gcp]]

Attachment: fix-asan-env

MozReview-Commit-ID: KTdvzYIFYzv

Comment 15

[Gian-Carlo Pascutto [:gcp]]

Welp, base::environment_map is only supported on POSIX, but the Windows code also wants to get those log variables set.

Comment 16

[Gian-Carlo Pascutto [:gcp]]

Attachment: env_patch.diff

WIP on environment handling change for Unix and Windows

Comment 17

[Gian-Carlo Pascutto [:gcp]]

Attachment: env-set-directly

MozReview-Commit-ID: IL1MMoy8iH8

Comment 18

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

Comment on attachment 8934609 [details] [diff] [review]
env-set-directly

Review of attachment 8934609 [details] [diff] [review]:
-----------------------------------------------------------------

Mostly looks good, but make sure this doesn't completely break --disable-sandbox, and warn about env_map not working yet in the non-sandboxed Windows case.

::: ipc/chromium/src/base/process_util.h
@@ +112,3 @@
>    // Environment variables to be applied in addition to the current
>    // process's environment, replacing them where necessary.
> +  EnvironmentMap env_map;

This isn't used by process_util_win.cc, only by the sandbox path in GeckoChildProcessHost, so these environment changes won't apply to non-sandboxed processes.

That may not need to be fixed in this bug, but there should at least be a FIXME comment pointing to bug 1257276.

::: ipc/glue/GeckoChildProcessHost.cpp
@@ +22,5 @@
>  #include "mozilla/Sprintf.h"
>  #include "prenv.h"
>  #include "nsXPCOMPrivate.h"
>  
> +#include "mozilla/EnvironmentMap.h"

Won't this break on --disable-sandbox builds, because the header is in security/sandbox?

::: security/sandbox/chromium/sandbox/win/src/target_process.cc
@@ +115,5 @@
> +  // Ignore return value? What can we do?
> +  FreeEnvironmentStrings(original_environment);
> +  // Cast away const and type. It's an input param only
> +  // so casting away const should be safe.
> +  LPVOID new_env_ptr = (void*)new_environment.data();

Comment nit: I agree that it's safe to cast away const here, but I'm not seeing why the pointer target would be const in the first place — new_environment isn't const and there's a non-const basic_string::data.

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.h
@@ +8,5 @@
>  #define __SECURITY_SANDBOX_SANDBOXBROKER_H__
>  
>  #include <stdint.h>
> +#include <map>
> +#include <string>

Nit: these includes might have been needed in an earlier version of this patch but it doesn't look like that's the case now.

Comment 19

[Gian-Carlo Pascutto [:gcp]]

Attachment: env.diff

Addresses review comments. I ended up including just the code needed in the old non-sandboxed callpath. I think it's the least bad solution.

Comment 20

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

Comment on attachment 8935806 [details] [diff] [review]
env.diff

Review of attachment 8935806 [details] [diff] [review]:
-----------------------------------------------------------------

r=me if the missing files are unchanged from the previous diff.

::: ipc/chromium/src/base/process_util_win.cc
@@ +287,5 @@
> +    cur++;
> +  return cur + 1;
> +}
> +
> +std::wstring AlterEnvironment(const wchar_t* env,

A comment that mentions where these two functions are copied from would be nice.  Also, can they be static?

::: ipc/glue/moz.build
@@ +20,5 @@
>      'CrashReporterHost.h',
>      'CrashReporterMetadataShmem.h',
>      'CrossProcessMutex.h',
>      'CrossProcessSemaphore.h',
> +    'EnvironmentMap.h',

This file seems to have been omitted from the diff.

::: security/sandbox/moz.build
@@ +43,5 @@
>          'chromium/base/callback_internal.cc',
>          'chromium/base/cpu.cc',
>          'chromium/base/debug/alias.cc',
>          'chromium/base/debug/profiler.cc',
> +        'chromium/base/environment.cc',

…and this file.

Comment 21

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bdd7743f057a0b8242a615cddb1fd35639aa223
Bug 1297740. r=jld

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for build bustage:

https://hg.mozilla.org/integration/mozilla-inbound/rev/0249dbbc148cf6774839af83e4d30200f5d491d0

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=3bdd7743f057a0b8242a615cddb1fd35639aa223&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=152916894&repo=mozilla-inbound
/builds/worker/workspace/build/src/ipc/chromium/src/base/process_util_mac.mm:39:57: error: no member named 'environ' in 'base::LaunchOptions'

Comment 23

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2501f2e295ebfc91ed5db1b0c16f70f443669f3
Bug 1297740. r=jld

Comment 24

[Gian-Carlo Pascutto [:gcp]]

This put the mingw builds on fire. The fix is probably easy but I need to verify it doesn't break MSVC or clang again (sigh).

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backout for the broken ming32 builds: https://hg.mozilla.org/integration/mozilla-inbound/rev/a8321a24d981e0618001fc89e1d0c87433727f8d

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e2501f2e295ebfc91ed5db1b0c16f70f443669f3&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=154436404&repo=mozilla-inbound

/builds/worker/workspace/build/src/ipc/chromium/src/base/process_util_win.cc:293:21: error: 'std::__cxx11::wstring base::AlterEnvironment(const wchar_t*, const EnvironmentMap&)' was declared 'extern' and later 'static' [-fpermissive]

Comment 26

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8fdd1dda7efce8f93d72627f0fa27aed6d5d5223
Bug 1297740. r=jld

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8fdd1dda7efc