Closed Bug 1244595 Opened 4 years ago Closed 4 years ago

Global-buffer-overflow in ConvertKeyframeSequence

Categories

(Core :: DOM: Animation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox44 --- disabled
firefox45 --- disabled
firefox46 + fixed
firefox47 + fixed
firefox-esr38 --- unaffected
firefox-esr45 --- disabled

People

(Reporter: inferno, Assigned: heycam)

References

Details

(Keywords: csectype-bounds, regression, sec-high)

Attachments

(1 file, 1 obsolete file)

Repro:
<div id=target><script>
  var player = target.animate([{background: 'green'}, {background: 'green'}]);
  </script>

Stacktrace:
=================================================================
==7359==ERROR: AddressSanitizer: global-buffer-overflow on address 0x7f2c1baf56dc at pc 0x7f2c127e074b bp 0x7ffed2a5cd30 sp 0x7ffed2a5cd28
READ of size 4 at 0x7f2c1baf56dc thread T0 (Web Content)
    #0 0x7f2c127e074a in mozilla::dom::GetPropertyValuesPairs(JSContext*, JS::Handle<JSObject*>, mozilla::dom::ListAllowance, nsTArray<mozilla::dom::PropertyValuesPair>&) /build/firefox/src/dom/animation/KeyframeEffect.cpp:1050:9
    #1 0x7f2c127d2164 in ConvertKeyframeSequence /build/firefox/src/dom/animation/KeyframeEffect.cpp:1115:12
    #2 0x7f2c127d2164 in BuildAnimationPropertyListFromKeyframeSequence /build/firefox/src/dom/animation/KeyframeEffect.cpp:1409
    #3 0x7f2c127d2164 in mozilla::dom::KeyframeEffectReadOnly::BuildAnimationPropertyList(JSContext*, mozilla::dom::Element*, JS::Handle<JSObject*>, nsTArray<mozilla::AnimationProperty>&, mozilla::ErrorResult&) /build/firefox/src/dom/animation/KeyframeEffect.cpp:1662
    #4 0x7f2c127d6ffe in mozilla::dom::KeyframeEffectReadOnly::Constructor(mozilla::dom::GlobalObject const&, mozilla::dom::Element*, JS::Handle<JSObject*>, mozilla::TimingParams const&, mozilla::ErrorResult&) /build/firefox/src/dom/animation/KeyframeEffect.cpp:1686:3
    #5 0x7f2c129f2dde in mozilla::dom::Element::Animate(JSContext*, JS::Handle<JSObject*>, mozilla::dom::UnrestrictedDoubleOrKeyframeAnimationOptions const&, mozilla::ErrorResult&) /build/firefox/src/dom/base/Element.cpp:3341:5
    #6 0x7f2c14392f87 in mozilla::dom::ElementBinding::animate(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JSJitMethodCallArgs const&) /build/firefox/src/objdir-ff-asan/dom/bindings/ElementBinding.cpp:3074:55
    #7 0x7f2c1473c22c in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /build/firefox/src/dom/bindings/BindingUtils.cpp:2715:13
    #8 0x7f2c1a8cb885 in CallJSNative /build/firefox/src/js/src/jscntxtinlines.h:235:15
    #9 0x7f2c1a8cb885 in js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /build/firefox/src/js/src/vm/Interpreter.cpp:463
    #10 0x7f2c1a8b2162 in Interpret(JSContext*, js::RunState&) /build/firefox/src/js/src/vm/Interpreter.cpp:2799:18
    #11 0x7f2c1a898816 in js::RunScript(JSContext*, js::RunState&) /build/firefox/src/js/src/vm/Interpreter.cpp:425:12
    #12 0x7f2c1a8cddcc in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) /build/firefox/src/js/src/vm/Interpreter.cpp:681:15
    #13 0x7f2c1a8ce38c in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /build/firefox/src/js/src/vm/Interpreter.cpp:713:12
    #14 0x7f2c1a393da3 in Evaluate(JSContext*, JS::Handle<JSObject*>, JS::Handle<js::StaticScope*>, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::MutableHandle<JS::Value>) /build/firefox/src/js/src/jsapi.cpp:4451:19
    #15 0x7f2c1a394717 in Evaluate /build/firefox/src/js/src/jsapi.cpp:4478:12
    #16 0x7f2c1a394717 in JS::Evaluate(JSContext*, JS::AutoVectorRooter<JSObject*>&, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::MutableHandle<JS::Value>) /build/firefox/src/js/src/jsapi.cpp:4539
    #17 0x7f2c12d1536b in nsJSUtils::EvaluateString(JSContext*, JS::SourceBufferHolder&, JS::Handle<JSObject*>, JS::CompileOptions&, nsJSUtils::EvaluateOptions const&, JS::MutableHandle<JS::Value>, void**) /build/firefox/src/dom/base/nsJSUtils.cpp:224:12
    #18 0x7f2c12d161ca in nsJSUtils::EvaluateString(JSContext*, JS::SourceBufferHolder&, JS::Handle<JSObject*>, JS::CompileOptions&, void**) /build/firefox/src/dom/base/nsJSUtils.cpp:286:10
    #19 0x7f2c12da398f in nsScriptLoader::EvaluateScript(nsScriptLoadRequest*, JS::SourceBufferHolder&) /build/firefox/src/dom/base/nsScriptLoader.cpp:1151:10
    #20 0x7f2c12da03ec in nsScriptLoader::ProcessRequest(nsScriptLoadRequest*) /build/firefox/src/dom/base/nsScriptLoader.cpp:970:10
    #21 0x7f2c12d978c8 in nsScriptLoader::ProcessScriptElement(nsIScriptElement*) /build/firefox/src/dom/base/nsScriptLoader.cpp:726:10
    #22 0x7f2c12d954bf in nsScriptElement::MaybeProcessScript() /build/firefox/src/dom/base/nsScriptElement.cpp:142:10
    #23 0x7f2c11ff50ba in AttemptToExecute /build/firefox/src/dom/base/nsIScriptElement.h:221:18
    #24 0x7f2c11ff50ba in nsHtml5TreeOpExecutor::RunScript(nsIContent*) /build/firefox/src/parser/html/nsHtml5TreeOpExecutor.cpp:666
    #25 0x7f2c11ff3971 in nsHtml5TreeOpExecutor::RunFlushLoop() /build/firefox/src/parser/html/nsHtml5TreeOpExecutor.cpp:491:7
    #26 0x7f2c11ff809b in nsHtml5ExecutorFlusher::Run() /build/firefox/src/parser/html/nsHtml5StreamParser.cpp:128:9
    #27 0x7f2c103e1742 in nsThread::ProcessNextEvent(bool, bool*) /build/firefox/src/xpcom/threads/nsThread.cpp:995:7
    #28 0x7f2c1046392c in NS_ProcessNextEvent(nsIThread*, bool) /build/firefox/src/xpcom/glue/nsThreadUtils.cpp:297:10
    #29 0x7f2c10e5595f in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /build/firefox/src/ipc/glue/MessagePump.cpp:127:5
    #30 0x7f2c10dbc8f1 in RunInternal /build/firefox/src/ipc/chromium/src/base/message_loop.cc:234:3
    #31 0x7f2c10dbc8f1 in RunHandler /build/firefox/src/ipc/chromium/src/base/message_loop.cc:227
    #32 0x7f2c10dbc8f1 in MessageLoop::Run() /build/firefox/src/ipc/chromium/src/base/message_loop.cc:201
    #33 0x7f2c1621339f in nsBaseAppShell::Run() /build/firefox/src/widget/nsBaseAppShell.cpp:156:3
    #34 0x7f2c18293c13 in XRE_RunAppShell /build/firefox/src/toolkit/xre/nsEmbedFunctions.cpp:789:12
    #35 0x7f2c10dbc8f1 in RunInternal /build/firefox/src/ipc/chromium/src/base/message_loop.cc:234:3
    #36 0x7f2c10dbc8f1 in RunHandler /build/firefox/src/ipc/chromium/src/base/message_loop.cc:227
    #37 0x7f2c10dbc8f1 in MessageLoop::Run() /build/firefox/src/ipc/chromium/src/base/message_loop.cc:201
    #38 0x7f2c182931b9 in XRE_InitChildProcess /build/firefox/src/toolkit/xre/nsEmbedFunctions.cpp:625:7
    #39 0x4ea61e in content_process_main(int, char**) /build/firefox/src/ipc/app/../contentproc/plugin-container.cpp:237:19
    #40 0x7f2c0d528ec4 in __libc_start_main
    #41 0x41dc56 in _start

0x7f2c1baf56dc is located 12 bytes to the right of global variable 'nsCSSProps::kAnimTypeTable' defined in '/build/firefox/src/layout/style/nsCSSProps.cpp:2462:13' (0x7f2c1baf5200) of size 1232
SUMMARY: AddressSanitizer: global-buffer-overflow (/build/firefox/src/objdir-ff-asan/dist/bin/libxul.so+0x42e874a)
Shadow bytes around the buggy address:
  0x0fe603756a80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe603756a90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe603756aa0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe603756ab0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe603756ac0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0fe603756ad0: 00 00 00 00 00 00 00 00 00 00 f9[f9]f9 f9 f9 f9
  0x0fe603756ae0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x0fe603756af0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x0fe603756b00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe603756b10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe603756b20: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==7359==ABORTING
Based on this:
0x7f2c1baf56dc is located 12 bytes to the right of global variable 'nsCSSProps::kAnimTypeTable'

I'm guessing the buffer overflow is in the access to kAnimTypeTable here:

999     if (property != eCSSProperty_UNKNOWN &&
1000         nsCSSProps::kAnimTypeTable[property] != eStyleAnimType_None) {

That code initially landed in bug 1208951, but it looks like there have been some recent changes to KeyFrameEffect.cpp in bug 1216842.
kAnimTypeTable has eCSSProperty_COUNT_no_shorthands entries.

I see nothing in the code in GetPropertyValuesPairs that would restrict to non-shorthands.  Should there be something, or some special handling of shorthands?

Of course in the testcase the relevant property is "background", which is in fact a shorthand.

I expect this got introduced by the fix for bug 1208951, yes.

Now the good news is that this is all behind the "dom.animations-api.core.enabled" pref, which I think is only enabled on nightly and aurora, right?

I guess Brian and Cameron are both out for a few days, but needinfoing them.
Blocks: 1208951
Flags: needinfo?(cam)
Flags: needinfo?(bbirtles)
I'll look at this later today.
Assignee: nobody → cam
Flags: needinfo?(cam)
Group: core-security → dom-core-security
Flags: needinfo?(bbirtles)
I think we can just skip that kAnimTypeTable check.  Later on, when we call StyleAnimationValue::ComputeValues, it handles shorthands, and any non-animatable longhand components will be skipped over.

Brian tells me that we should really be exposing all properties on the KeyframeEffect.getFrames() return value, and so be storing these non-animatable properties in the AnimationProperties object, but let's do that in a separate bug.
(In reply to Boris Zbarsky [:bz] from comment #2)
> Now the good news is that this is all behind the
> "dom.animations-api.core.enabled" pref, which I think is only enabled on
> nightly and aurora, right?

That's right.
Attached patch patch (obsolete) — Splinter Review
I don't have an ASAN build handy locally but presumably should avoid the issue.

And I think it's the right fix per comment 4.
Attachment #8715151 - Flags: review?(bbirtles)
As discussed in person, if we start storing non-animatable properties (as per attachment 8715151 [details] [diff] [review]) we might hit an assertion in ExtractComputedValue[1] when we pass a non-animatable longhand.

It seems odd that StyleAnimationValue::ComputeValue only checks for non-animatable values for shorthands[2] so we should probably adjust it to check for longhands too.

[1] https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/layout/style/StyleAnimationValue.cpp#3614
[2] https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/layout/style/StyleAnimationValue.cpp#2559
(In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #8)
> As discussed in person, if we start storing non-animatable properties (as per attachment 8715151 [details] [diff] [review]) we
> might hit an assertion in ExtractComputedValue[1] when we pass a non-animatable longhand.

OK, good point.

> It seems odd that StyleAnimationValue::ComputeValue only checks for
> non-animatable values for shorthands[2] so we should probably adjust it to
> check for longhands too.

Since ComputeValue already returns a boolean to indicate whether the string value for the property parsed or not, and I didn't want to bother adding an additional outparam (or an enum) to indicate the reason for failure (bad string value or non-animatable property), I'm just going to check the longhand animatableness in KeyframeEffect.
Attached patch patch v2Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ac13420bfca
Attachment #8715151 - Attachment is obsolete: true
Attachment #8715151 - Flags: review?(bbirtles)
Attachment #8715154 - Flags: review?(bbirtles)
Comment on attachment 8715154 [details] [diff] [review]
patch v2

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

::: dom/animation/KeyframeEffect.cpp
@@ +1050,5 @@
> +        (nsCSSProps::IsShorthand(property) ||
> +         nsCSSProps::kAnimTypeTable[property] != eStyleAnimType_None)) {
> +      // Only need to check for longhands being animatable, as the
> +      // StyleAnimationValue::ComputeValues calls later on will check for
> +      // a shorthand's components being animatable.

As discussed, this won't work if |property| is an alias. However, LookupPropertyByIDLName asserts that property is < eCSSProperty_COUNT so it won't return aliases (which come after eCSSProperty_COUNT). We might want to support aliases in the future so can we add an assertion here that property < eCSSProperty_COUNT in case we update LookupPropertyByIDLName?
Attachment #8715154 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #11)
> We might want to
> support aliases in the future so can we add an assertion here that property
> < eCSSProperty_COUNT in case we update LookupPropertyByIDLName?

This is already asserted by nsCSSProps::IsShorthand so I don't think we need to add another assertion here.
Comment on attachment 8715154 [details] [diff] [review]
patch v2

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

Difficult to impossible.  AFAICT, all we can do in response to reading the memory past the end of the array is decide to add or not some properties to the animation.  Since we're looking at constant data, we're probably just going to end up looking into some of the adjacent nsCSSProps constant data arrays, which shouldn't be sensitive, though I didn't check.  (We could tell whether this out-of-bounds word is equal to 16 (=eStyleAnimType_None), by checking whether the properties got added to the animation.)

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

You would need to realise that IsShorthand doesn't range check its argument, so it's not obvious, though you could work it out.

Which older supported branches are affected by this flaw?

aurora

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

bug 1208951

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

I don't but it should be straightforward.

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

Unlikely.  The added crashtest plus the existing KeyframeEffectReadOnly tests that involve shorthands should be sufficient.
Attachment #8715154 - Flags: sec-approval?
Duplicate of this bug: 1245427
Comment on attachment 8715154 [details] [diff] [review]
patch v2

sec-approval+ for trunk.

Can we get an Aurora patch nomination as well so we can take it there after trunk?
Flags: needinfo?(cam)
Attachment #8715154 - Flags: sec-approval? → sec-approval+
Comment on attachment 8715154 [details] [diff] [review]
patch v2

Approval Request Comment
[Feature/regressing bug #]: bug 1208951
[User impact if declined]: possible security problem due to out of range reads, but unlikely to be harmful
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ac13420bfca
[Risks and why]: little, we are just skipping an array lookup to ensure we don't access it out of range; tested that the new control flow for such cases makes sense
[String/UUID change made/needed]: none
Flags: needinfo?(cam)
Attachment #8715154 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/7b6557aa9bdb
Status: NEW → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Attachment #8715154 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Group: dom-core-security → core-security-release
Group: core-security-release
Keywords: regression
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
You need to log in before you can comment on or make changes to this bug.