1432855 - AddressSanitizer: heap-use-after-free in TelemetryHistogram.cpp when a test depends on an expired probe

Status: VERIFIED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Ryan VanderMeulen [:RyanVM]]

Not sure if this is security-sensitive or not, but starting under that assumption until proven otherwise. When running the version bump simulations, I noticed that ASAN builds hit a use-after-free in their test runs:

https://treeherder.mozilla.org/logviewer.html#?job_id=158237012&repo=try

Comment 1

[Chris H-C :chutten]

The code in question is:

// Clear a histogram from storage.
void
internal_ClearHistogramById(HistogramID histogramId, ProcessID processId, SessionType sessionType)
{
  size_t index = internal_HistogramStorageIndex(histogramId, processId, sessionType);
  delete gHistogramStorage[index];
  gHistogramStorage[index] = nullptr;
}

...so... we're not allowed to null out a pointer to something we just deleted? Or maybe, since the offsets are different, are we clearing something more than once?

Comment 2

[Georg Fritzsche [:gfritzsche]]

The way i read this, we try to delete this histogram twice.

This histogram is read/deleted here:

> READ of size 8 at 0x60d000010100 thread T0
> #0 0x7f4f9d665f57 in internal_ClearHistogramById /builds/worker/workspace/build/src/toolkit/components/telemetry/TelemetryHistogram.cpp:386:3
> #1 0x7f4f9d665f57 in internal_ClearHistogram /builds/worker/workspace/build/src/toolkit/components/telemetry/TelemetryHistogram.cpp:1070
> #2 0x7f4f9d665f57 in (anonymous namespace)::internal_JSHistogram_Clear(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/toolkit/components/telemetry/TelemetryHistogram.cpp:1279
> #3 0x7f4f9db115e4 in CallJSNative /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:291:15
> #4 0x7f4f9db115e4 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:473
> #5 0x7f4f9dafc5d6 in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:528:12
> #6 0x7f4f9dafc5d6 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3096
> #7 0x7f4f9dae2e40 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:423:12
> #8 0x7f4f9db11b1c in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:495:15
> #9 0x7f4f9db12642 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:541:10
> #10 0x7f4f9e39e366 in js::jit::InterpretResume(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<js::PropertyName*>, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/jit/VMFunctions.cpp:954:12
> #11 0x7f4f3d150226  (<unknown module>)

... but was previously freed from this stack:

> [task 2018-01-24T17:06:23.988Z] 17:06:23     INFO -  PID 12893 | freed by thread T0 here:
> #0 0x4c18d2 in free /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:68:3
> #1 0x7f4f9d665bba in internal_ClearHistogramById /builds/worker/workspace/build/src/toolkit/components/telemetry/TelemetryHistogram.cpp:386:3
> #2 0x7f4f9d665bba in internal_ClearHistogram /builds/worker/workspace/build/src/toolkit/components/telemetry/TelemetryHistogram.cpp:1070
> #3 0x7f4f9d665bba in (anonymous namespace)::internal_JSHistogram_Clear(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/toolkit/components/telemetry/TelemetryHistogram.cpp:1279
> #4 0x7f4f9db115e4 in CallJSNative /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:291:15
> #5 0x7f4f9db115e4 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:473
> #6 0x7f4f9dafc5d6 in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:528:12
> #7 0x7f4f9dafc5d6 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3096
> #8 0x7f4f9dae2e40 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:423:12
> #9 0x7f4f9db11b1c in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:495:15
> #10 0x7f4f9db12642 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:541:10
> #11 0x7f4f9e39e366 in js::jit::InterpretResume(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<js::PropertyName*>, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/jit/VMFunctions.cpp:954:12
> #12 0x7f4f3d150226  (<unknown module>)
> #13 0x7f4f3d14f4e7  (<unknown module>)
> #14 0x7f4f9e0d302e in EnterJit /builds/worker/workspace/build/src/js/src/jit/Jit.cpp:99:9
> #15 0x7f4f9e0d302e in js::jit::MaybeEnterJit(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/jit/Jit.cpp:163
> #16 0x7f4f9dae2cc7 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:408:34
> #17 0x7f4f9db11b1c in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:495:15
> #18 0x7f4f9db12642 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:541:10
> #19 0x7f4f9ec5716f in js::CallSelfHostedFunction(JSContext*, JS::Handle<js::PropertyName*>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/SelfHosting.cpp:1728:12
> #20 0x7f4f9e90d44f in AsyncFunctionResume(JSContext*, JS::Handle<js::PromiseObject*>, JS::Handle<JS::Value>, ResumeKind, JS::Handle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/AsyncFunction.cpp:191:10
> #21 0x7f4f9e90c225 in AsyncFunctionStart /builds/worker/workspace/build/src/js/src/vm/AsyncFunction.cpp:204:12
> #22 0x7f4f9e90c225 in WrappedAsyncFunction(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/AsyncFunction.cpp:90
> #23 0x7f4f9db115e4 in CallJSNative /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:291:15
> #24 0x7f4f9db115e4 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:473
> #25 0x7f4f9db12642 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:541:10
> #26 0x7f4f9e6af5e9 in js::fun_apply(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/js/src/jsfun.cpp:1255:12
> #27 0x7f4f9db115e4 in CallJSNative /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:291:15
> #28 0x7f4f9db115e4 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:473
> #29 0x7f4f9db12642 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:541:10
> #30 0x7f4f9e8e76f2 in js::ForwardingProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const /builds/worker/workspace/build/src/js/src/proxy/Wrapper.cpp:176:12
> #31 0x7f4f9e8951bd in js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const /builds/worker/workspace/build/src/js/src/proxy/CrossCompartmentWrapper.cpp:359:23
> #32 0x7f4f9e8c58f1 in js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) /builds/worker/workspace/build/src/js/src/proxy/Proxy.cpp:511:21
> #33 0x7f4f9e8c8184 in js::proxy_Call(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/js/src/proxy/Proxy.cpp:770:12
> #34 0x7f4f9db11da2 in CallJSNative /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:291:15
> #35 0x7f4f9db11da2 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:455
> #36 0x7f4f9dafc5d6 in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:528:12
> #37 0x7f4f9dafc5d6 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3096
> #38 0x7f4f9dae2e40 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:423:12
> #39 0x7f4f9db11b1c in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:495:15
[task 2018-01-24T17:06:24.020Z] 17:06:24     INFO -  PID 12893 |

Comment 3

[Chris H-C :chutten]

delete is a use? Interesting.

Well, we could early-return in internal_ClearHistogramById. Just a quick if (!gHistogramStorage[index]) { return; }

Comment 4

[Georg Fritzsche [:gfritzsche]]

I don't think this solves it. `delete nullptr` is fine in C++.
This must be a non-null pointer histogram value, that was previously freed there.

It's both on thread T0 though, so it can't be a race?

Comment 5

[Chris H-C :chutten]

The key here might be that it's an expired histogram. All expired histograms are the same histogram, in storage. Would that make a difference?

Comment 6

[Georg Fritzsche [:gfritzsche]]

Good catch, we are not protecting against this in this code path:
https://dxr.mozilla.org/mozilla-central/rev/b0baaec09caf3e1b30ec6b238f5c46ef9b3188be/toolkit/components/telemetry/TelemetryHistogram.cpp#1070
https://dxr.mozilla.org/mozilla-central/rev/b0baaec09caf3e1b30ec6b238f5c46ef9b3188be/toolkit/components/telemetry/TelemetryHistogram.cpp#386

Comment 7

[Chris H-C :chutten]

Attachment: 0001-bug-1432855-Early-return-if-we-re-asked-to-clear-the.patch

Comment 8

[Chris H-C :chutten]

Comment on attachment 8945200 [details] [diff] [review]
0001-bug-1432855-Early-return-if-we-re-asked-to-clear-the.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

  If an attacker could trigger us to clear an expired histogram in privileged chrome JS, we could try to use or delete a pointer that had already been deleted. Not sure how easy it is for attackers to convince us to clear histograms using chrome JS, since we intend for JSHistogram's `clear()` to only be used in tests.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

  Probably not? I tried to be a little cagey about it.

Which older supported branches are affected by this flaw?

  This code was changed last summer in bug 1373240, so Firefox 57 and up.

If not all supported branches, which bug introduced the flaw?

  Bug 1373240

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

  Nope, but they should be exactly this patch. This part of the file has proved remarkably static these past five months.

How likely is this patch to cause regressions; how much testing does it need?

  Unlikely. A clean try run (paying close attention to ASan) ought to do the trick.

Comment 9

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8945200 [details] [diff] [review]
0001-bug-1432855-Early-return-if-we-re-asked-to-clear-the.patch

As a (now) sec-moderate, this doesn't need sec-approval+ to check in so feel free to check it into trunk.

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c936b9eb5e7b570b83ed412010f71da33ffb618
https://hg.mozilla.org/mozilla-central/rev/2c936b9eb5e7

Comment 11

[Chris H-C :chutten]

Comment on attachment 8945200 [details] [diff] [review]
0001-bug-1432855-Early-return-if-we-re-asked-to-clear-the.patch

Approval Request Comment
[Feature/Bug causing the regression]:
 bug 1373240
[User impact if declined]:
 Use-after-free possibly exploitable if combined with other attacks.
[Is this code covered by automated tests?]:
 Yes
[Has the fix been verified in Nightly?]:
 Not yet
[Needs manual test from QE? If yes, steps to reproduce]: 
 Follow RyanVM's "advance the version number" script and run ASan on try 
[List of other uplifts needed for the feature/fix]:
 None.
[Is the change risky?]:
 No.
[Why is the change risky/not risky?]:
 Clearing histograms is something that's only supposed to be done in tests. We don't ship code that should run this except on de-init (and in that case it uses a different code path that avoids the exploit)
[String changes made/needed]:
 None.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Let's verify this on Nightly and then uplift for beta 6, middle of next week. 
Andrei can your team handle the verifying? Thanks.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Confirmed that I now get a plain test failure instead of a UAF on a build that includes this fix.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8945200 [details] [diff] [review]
0001-bug-1432855-Early-return-if-we-re-asked-to-clear-the.patch

Uplift to avoid a crash. This should land for the 59 beta 6 build.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/db9269c469a7

Comment 16

[Ryan VanderMeulen [:RyanVM]]

I was pinged about trying to verify this bug on Beta. I don't think it's worth it. It requires some carefully-crafted Try pushes to do so. I was able to verify this on trunk, which I think is satisfactory enough to confirm that the fix worked.