969756 - Heap-buffer-overflow in AppendValueToString

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Abhishek Arya]

Attachment: Tescase

>==24328==ERROR: AddressSanitizer: global-buffer-overflow on address 0x7f86874d4118 at pc 0x7f86836094c9 bp 0x7fffe9ff9610 sp 0x7fffe9ff9608
>READ of size 4 at 0x7f86874d4118 thread T0
>    #0 0x7f86836094c8 in ValueFor layout/style/nsCSSDataBlock.cpp:170
>    #1 0x7f86836094c8 in AppendValueToString layout/style/Declaration.cpp:107
>    #2 0x7f86836094c8 in mozilla::css::Declaration::AppendPropertyAndValueToString(nsCSSProperty, nsAutoString&, nsAString_internal&) const layout/style/Declaration.cpp:1009
>    #3 0x7f868360a2f8 in mozilla::css::Declaration::ToString(nsAString_internal&) const layout/style/Declaration.cpp:1146
>    #4 0x7f8682ac9e7f in nsAttrValue::ToString(nsAString_internal&) const content/base/src/nsAttrValue.cpp:643
>    #5 0x7f8682a5a4a4 in ToString content/base/src/nsAttrValue.h:500
>    #6 0x7f8682a5a4a4 in GetAttr objdir-ff-asan/content/base/src/../../../dist/include/mozilla/dom/Element.h:533
>    #7 0x7f8682a5a4a4 in mozilla::dom::Element::GetAttr(int, nsIAtom*, nsAString_internal&) const content/base/src/Element.cpp:1945
>    #8 0x7f8682a611f4 in mozilla::dom::Element::SetAttrAndNotify(int, nsIAtom*, nsIAtom*, nsAttrValue const&, nsAttrValue&, unsigned char, bool, bool, bool) content/base/src/Element.cpp:1880
>    #9 0x7f8682c3682e in nsStyledElementNotElementCSSInlineStyle::SetInlineStyleRule(mozilla::css::StyleRule*, nsAString_internal const*, bool) content/base/src/nsStyledElement.cpp:171
>    #10 0x7f86837289ba in nsDOMCSSAttributeDeclaration::SetCSSDeclaration(mozilla::css::Declaration*) layout/style/nsDOMCSSAttrDeclaration.cpp:88
>    #11 0x7f8683729a27 in nsDOMCSSDeclaration::ParsePropertyValue(nsCSSProperty, nsAString_internal const&, bool) layout/style/nsDOMCSSDeclaration.cpp:340
>    #12 0x7f8680cfe9c2 in SetColor layout/style/nsCSSPropList.h:1423
>    #13 0x7f8680cfe9c2 in mozilla::dom::CSS2PropertiesBinding::set_color(JSContext*, JS::Handle<JSObject*>, nsDOMCSSDeclaration*, JSJitSetterCallArgs) objdir-ff-asan/dom/bindings/./CSS2PropertiesBinding.cpp:5002
>    #14 0x7f8680c59985 in mozilla::dom::CSS2PropertiesBinding::genericSetter(JSContext*, unsigned int, JS::Value*) objdir-ff-asan/dom/bindings/./CSS2PropertiesBinding.cpp:18290
>    #15 0x7f868670fcdb in CallJSNative js/src/../../js/src/jscntxtinlines.h:220
>    #16 0x7f868670fcdb in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/../../js/src/vm/Interpreter.cpp:469
>    #17 0x7f86866ab244 in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) js/src/../../js/src/vm/Interpreter.cpp:532
>    #18 0x7f8686711a1f in js::InvokeGetterOrSetter(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) js/src/../../js/src/vm/Interpreter.cpp:604
>    #19 0x7f8686527634 in js::CallSetter(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>), unsigned int, unsigned int, bool, JS::MutableHandle<JS::Value>) js/src/../../js/src/jscntxtinlines.h:323
>    #20 0x7f868656442c in js::BaseProxyHandler::set(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>) js/src/../../js/src/jsproxy.cpp:201:18
>    #21 0x7f8686587636 in js::Proxy::set(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>) js/src/../../js/src/jsproxy.cpp:2563
>    #22 0x7f86864f96f8 in JSObject::nonNativeSetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>, bool) js/src/../../js/src/jsobj.cpp:1663
>    #23 0x7f86866ffd33 in setGeneric js/src/../../js/src/jsobj.h:1061
>    #24 0x7f86866ffd33 in SetPropertyOperation js/src/../../js/src/vm/Interpreter.cpp:341
>    #25 0x7f86866ffd33 in Interpret(JSContext*, js::RunState&) js/src/../../js/src/vm/Interpreter.cpp:2444
>    #26 0x7f86866e85c6 in js::RunScript(JSContext*, js::RunState&) js/src/../../js/src/vm/Interpreter.cpp:423
>    #27 0x7f86866d0f64 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*) js/src/../../js/src/vm/Interpreter.cpp:631
>    #28 0x7f8686711d57 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) js/src/../../js/src/vm/Interpreter.cpp:667
>    #29 0x7f86863f22f6 in JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::ReadOnlyCompileOptions const&, char16_t const*, unsigned long, JS::Value*) js/src/../../js/src/jsapi.cpp:4812
>    #30 0x7f86822fa7d4 in nsJSUtils::EvaluateString(JSContext*, nsAString_internal const&, JS::Handle<JSObject*>, JS::CompileOptions&, nsJSUtils::EvaluateOptions&, JS::Value*, void**) dom/base/nsJSUtils.cpp:220
>    #31 0x7f8682248d33 in nsJSContext::EvaluateString(nsAString_internal const&, JS::Handle<JSObject*>, JS::CompileOptions&, bool, JS::Value*, void**) dom/base/nsJSEnvironment.cpp:977
>    #32 0x7f8682c2cdbe in nsScriptLoader::EvaluateScript(nsScriptLoadRequest*, nsString const&, void**) content/base/src/nsScriptLoader.cpp:1084
>    #33 0x7f8682c2a801 in nsScriptLoader::ProcessRequest(nsScriptLoadRequest*, void**) content/base/src/nsScriptLoader.cpp:932
>    #34 0x7f8682c23eaf in nsScriptLoader::ProcessScriptElement(nsIScriptElement*) content/base/src/nsScriptLoader.cpp:754
>    #35 0x7f8682c20e19 in nsScriptElement::MaybeProcessScript() content/base/src/nsScriptElement.cpp:140
>    #36 0x7f868215511d in operator-> objdir-ff-asan/parser/html/../../dist/include/nsIScriptElement.h:220
>    #37 0x7f868215511d in nsHtml5TreeOpExecutor::RunScript(nsIContent*) parser/html/nsHtml5TreeOpExecutor.cpp:756
>    #38 0x7f8682148c25 in nsHtml5TreeOpExecutor::RunFlushLoop() parser/html/nsHtml5TreeOpExecutor.cpp:563
>    #39 0x7f868215eaa8 in nsHtml5ExecutorFlusher::Run() parser/html/nsHtml5StreamParser.cpp:128
>    #40 0x7f867f2d8132 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:643
>    #41 0x7f867f1ae4f1 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp:263
>    #42 0x7f867fb0ebe1 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:95
>    #43 0x7f867fa83273 in RunInternal ipc/chromium/src/base/message_loop.cc:226
>    #44 0x7f867fa83273 in RunHandler ipc/chromium/src/base/message_loop.cc:219
>    #45 0x7f867fa83273 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:193
>    #46 0x7f8681dfcd3c in nsBaseAppShell::Run() widget/xpwidgets/nsBaseAppShell.cpp:161
>    #47 0x7f8684b5f5e6 in nsAppStartup::Run() toolkit/components/startup/nsAppStartup.cpp:276
>    #48 0x7f8684980de8 in XREMain::XRE_mainRun() toolkit/xre/nsAppRunner.cpp:4090
>    #49 0x7f8684981d7d in XREMain::XRE_main(int, char**, nsXREAppData const*) toolkit/xre/nsAppRunner.cpp:4158
>    #50 0x7f8684982cbb in XRE_main toolkit/xre/nsAppRunner.cpp:4368
>    #51 0x44aa70 in do_main browser/app/nsBrowserApp.cpp:282
>    #52 0x44aa70 in main browser/app/nsBrowserApp.cpp:643
>    #53 0x7f868fa3276c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226
>    #54 0x44a05c in _start
>0x7f86874d4118 is located 128 bytes to the right of global variable 'nsCSSProps::kSIDTable' from 'objdir-ff-asan/layout/style/Unified_cpp_layout_style1.cpp' (0x7f86874d3c60) of size 1080
>Shadow bytes around the buggy address:
>  0x0ff150e927d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>  0x0ff150e927e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>  0x0ff150e927f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>  0x0ff150e92800: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>  0x0ff150e92810: 00 00 00 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
>=>0x0ff150e92820: f9 f9 f9[f9]f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
>  0x0ff150e92830: f9 f9 f9 f9 05 f9 f9 f9 f9 f9 f9 f9 00 02 f9 f9
>  0x0ff150e92840: f9 f9 f9 f9 00 00 00 00 00 00 00 00 00 00 00 04
>  0x0ff150e92850: f9 f9 f9 f9 06 f9 f9 f9 f9 f9 f9 f9 01 f9 f9 f9
>  0x0ff150e92860: f9 f9 f9 f9 00 00 04 f9 f9 f9 f9 f9 00 00 04 f9
>  0x0ff150e92870: f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9 00 04 f9 f9
>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
>  ASan internal:         fe
>==24328==ABORTING
>
>

Comment 1

[Mats Palmgren (inactive)]

In a debug build I get:
###!!! ABORT: aValue should be given for shorthands but not longhands: '(aProperty < eCSSProperty_COUNT_no_shorthands) == aValue.IsEmpty()', file layout/style/Declaration.cpp, line 1005

The problem seems to be that serializing a eCSSUnit_TokenStream value
for eCSSProperty__x_system_font results in an empty string.
http://hg.mozilla.org/mozilla-central/annotate/e931763f41b5/layout/style/Declaration.cpp#l1144
Which the AppendPropertyAndValueToString that follows can't handle.

Comment 2

[Mats Palmgren (inactive)]

Attachment: fix?

This seems to fix it.  I get "var(var6) hangul mongolian" as the result
which is what is specified in the testcase.

Comment 3

[Cameron McCormack (:heycam)]

OK, so since we don't know whether the token stream would expand to a system font keyword, we should assume that it might, and thus serialize the declaration in the "font shorthand followed by overriding font longhand components" form.

Comment 4

[Cameron McCormack (:heycam)]

Comment on attachment 8372828 [details] [diff] [review]
fix?

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

::: layout/style/nsCSSValue.cpp
@@ +1266,1 @@
>        // We treat serialization of aliases like '-moz-transform' as a special

Can you please add to the comment to say that we need to serialise -x-system-font's token stream value, even though the value is set through the font shrothand, because of the way we have to serialize the declaration?

Also please add a test (maybe in layout/style/test/test_variables.html) that tests the serialization of the declaration.

Comment 5

[Mats Palmgren (inactive)]

Attachment: Add code comment and test (DONT LAND UNTIL BUG IS PUBLIC)

I suggest we wait with this part to avoid revealing additional details about
the bug.

Comment 6

[Mats Palmgren (inactive)]

BTW, I was a bit surprised about the initial space in the property value. I guess
it has something to do with how variables are serialized?

Comment 7

[Mats Palmgren (inactive)]

The out-of-bounds read occurs here:
http://hg.mozilla.org/mozilla-central/annotate/7133bb431eba/layout/style/nsCSSDataBlock.cpp#l170
It looks like we use the read value as an optimization to avoid the loop
that follows.  That loop looks safe though since "PropertyAtIndex(i) == aProperty"
will never be true so we'll return null.  So in both cases we return null so it
doesn't leak any information about the read value, except maybe as a difference in
timing.

In a Linux64 build, asan says:
0x7ffff1c9f798 is located 128 bytes to the right of global variable 'nsCSSProps::kSIDTable' from 'layout/style/Unified_cpp_layout_style1.cpp' (0x7ffff1c9f2e0) of size 1080

nsCSSProps::kSIDTable lives in a .rodata section:
00000000061682e0 l O .rodata 0000000000000540 .hidden _ZN10nsCSSProps9kSIDTableE
and there appears to be plenty of other data after it in that section,
so I suspect that the read value is some constant value.

Comment 8

[Cameron McCormack (:heycam)]

(In reply to Mats Palmgren (:mats) from comment #6)
> BTW, I was a bit surprised about the initial space in the property value. I guess
> it has something to do with how variables are serialized?

Yes, white space tokens are significant in variables.

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8373025 [details] [diff] [review]
Add code comment and test (DONT LAND UNTIL BUG IS PUBLIC)

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

r=me with the comment tweaked.  Note I recently changed test_variables.html so that the style sheet text is inserted in each of the test functions, so that it is parsed with the pref turned on.

::: layout/style/nsCSSValue.cpp
@@ +1267,5 @@
>        // case, since it really wants to be serialized as if it were a longhand
> +      // even though it is implemented as a shorthand. We also need to
> +      // serialize -x-system-font's token stream value, even though the value
> +      // is set through the font shorthand, to a "font shorthand followed by
> +      // overriding font longhand components" form.

Well, it's not that -x-system-font must be serialized to a "font shorthand followed by overriding font longhand components" form.  It's more the entire set of font properties must be serialized in that way when -x-system-font has been set.  So I think better would be:

  // ... We also need to
  // serialize -x-system-font's token stream value, even though the
  // value is set through the font shorthand.  This serialization
  // of -x-system-font is needed when we need to output the
  // 'font' shorthand followed by a number of overriding font
  // longhand components.

Comment 10

[Mats Palmgren (inactive)]

Attachment: Add code comment and test (DONT LAND UNTIL BUG IS PUBLIC)

Updated the test and code comment as requested.

Comment 11

[Mats Palmgren (inactive)]

Landed the fix only:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3a9c9540bfc

Comment 12

[Phil Ringnalda (:philor)]

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

Comment 13

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

How far back does this go?

Comment 14

[Cameron McCormack (:heycam)]

CSS Variables landed in Firefox 29, though it is not yet enabled for Release/Beta.

Comment 15

[Cameron McCormack (:heycam)]

Mats can you check whether it affects current Aurora?

Comment 16

[Mats Palmgren (inactive)]

I haven't checked, but I'd expect all branches that have CSS Variables enabled to be affected.
The patch applies cleanly on Aurora and is low risk so I suggest we take it there...

Comment 17

[Mats Palmgren (inactive)]

Comment on attachment 8372828 [details] [diff] [review]
fix?

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 773296 (I think)
User impact if declined: crash when using CSS variables in the 'font' property in certain ways
Testing completed (on m-c, etc.): on m-c since 2014-02-15
Risk to taking this patch (and alternatives if risky): low risk
String or IDL/UUID changes made by this patch: none

Comment 18

[Mats Palmgren (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/499d9ea5eaa3

Comment 19

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed crash in ASan Fx29, 2013-12-19.
Verified fixed in ASan Fx29, Fx30, 2014-04-17.

Comment 20

[Mats Palmgren (inactive)]

Landed the test and code comment:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3690e3e1ec34

Comment 21

[Xidorn Quan [:xidorn] UTC+11]

You probably should backout the test you just landed. It causes orange like https://treeherder.mozilla.org/logviewer.html#?job_id=6856126&repo=mozilla-inbound

Comment 22

[Mats Palmgren (inactive)]

Oops, thanks for the heads up Xidorn!

Backed out the test in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7b44ecfcaec

I think the problem is that the test was written before the CSS Variables
changed syntax to use the -- prefix.  I'll try to fix it tomorrow.

Comment 23

[Mats Palmgren (inactive)]

Landed an updated test that use the -- prefix for variables:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9e98ae28067

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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