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

VERIFIED FIXED in Firefox 59

Status

()

defect
P1
normal
VERIFIED FIXED
Last year
8 months ago

People

(Reporter: RyanVM, Assigned: chutten)

Tracking

({sec-moderate})

unspecified
mozilla60
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox58 wontfix, firefox59+ fixed, firefox60 verified)

Details

(Whiteboard: [adv-main59+])

Attachments

(1 attachment)

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
Reporter

Updated

Last year
Group: firefox-core-security → toolkit-core-security
Assignee

Comment 1

Last year
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?
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 |
Assignee

Comment 3

Last year
delete is a use? Interesting.

Well, we could early-return in internal_ClearHistogramById. Just a quick if (!gHistogramStorage[index]) { return; }
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?
Assignee

Comment 5

Last year
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?
Assignee

Comment 7

Last year
Assignee: nobody → chutten
Status: NEW → ASSIGNED
Attachment #8945200 - Flags: review?(gfritzsche)
Assignee

Updated

Last year
Priority: -- → P1
Attachment #8945200 - Flags: review?(gfritzsche) → review+
Assignee

Comment 8

Last year
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.
Attachment #8945200 - Flags: sec-approval?
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.
Attachment #8945200 - Flags: sec-approval?
Assignee

Comment 11

Last year
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.
Attachment #8945200 - Flags: approval-mozilla-beta?
Let's verify this on Nightly and then uplift for beta 6, middle of next week. 
Andrei can your team handle the verifying? Thanks.
Flags: qe-verify+
Flags: needinfo?(andrei.vaida)
Flags: needinfo?(andrei.vaida)
Confirmed that I now get a plain test failure instead of a UAF on a build that includes this fix.
Status: RESOLVED → VERIFIED
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.
Attachment #8945200 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: toolkit-core-security → core-security-release
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.
Flags: qe-verify+ → qe-verify-
Whiteboard: [adv-main59+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.