Closed
Bug 969756
Opened 11 years ago
Closed 11 years ago
Heap-buffer-overflow in AppendValueToString
Categories
(Core :: CSS Parsing and Computation, defect)
Tracking
()
VERIFIED
FIXED
mozilla30
Tracking | Status | |
---|---|---|
firefox28 | --- | unaffected |
firefox29 | + | verified |
firefox30 | --- | verified |
firefox-esr24 | --- | unaffected |
People
(Reporter: inferno, Assigned: MatsPalmgren_bugz)
Details
(4 keywords, Whiteboard: [asan])
Attachments
(3 files, 1 obsolete file)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
heycam
:
review+
abillings
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
>==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
>
>
Assignee | ||
Comment 1•11 years ago
|
||
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.
Assignee | ||
Comment 2•11 years ago
|
||
This seems to fix it. I get "var(var6) hangul mongolian" as the result which is what is specified in the testcase.
Assignee | ||
Updated•11 years ago
|
Attachment #8372828 -
Flags: review?(cam)
Assignee | ||
Updated•11 years ago
|
Comment 3•11 years ago
|
||
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•11 years ago
|
||
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.
Attachment #8372828 -
Flags: review?(cam) → review+
Assignee | ||
Comment 5•11 years ago
|
||
I suggest we wait with this part to avoid revealing additional details about the bug.
Assignee: nobody → matspal
Attachment #8373025 -
Flags: review?(cam)
Assignee | ||
Comment 6•11 years ago
|
||
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?
Assignee | ||
Comment 7•11 years ago
|
||
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.
Keywords: csectype-bounds,
sec-other
Comment 8•11 years ago
|
||
(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•11 years ago
|
||
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.
Assignee | ||
Comment 10•11 years ago
|
||
Updated the test and code comment as requested.
Attachment #8373025 -
Attachment is obsolete: true
Attachment #8373025 -
Flags: review?(cam)
Attachment #8376731 -
Flags: review+
Assignee | ||
Comment 11•11 years ago
|
||
Landed the fix only: https://hg.mozilla.org/integration/mozilla-inbound/rev/b3a9c9540bfc
Flags: in-testsuite?
Comment 12•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b3a9c9540bfc
Status: NEW → RESOLVED
Closed: 11 years ago
status-firefox30:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Comment 14•11 years ago
|
||
CSS Variables landed in Firefox 29, though it is not yet enabled for Release/Beta.
Assignee | ||
Comment 16•11 years ago
|
||
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...
Assignee | ||
Comment 17•11 years ago
|
||
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
Attachment #8372828 -
Flags: approval-mozilla-aurora?
Updated•11 years ago
|
Attachment #8372828 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•11 years ago
|
status-firefox28:
--- → unaffected
status-firefox29:
--- → affected
status-firefox-esr24:
--- → unaffected
tracking-firefox29:
--- → +
Comment 19•11 years ago
|
||
Confirmed crash in ASan Fx29, 2013-12-19. Verified fixed in ASan Fx29, Fx30, 2014-04-17.
Assignee | ||
Comment 20•10 years ago
|
||
Landed the test and code comment: https://hg.mozilla.org/integration/mozilla-inbound/rev/3690e3e1ec34
Group: core-security
Flags: in-testsuite? → in-testsuite+
Comment 21•10 years ago
|
||
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
Flags: needinfo?(mats)
Assignee | ||
Comment 22•10 years ago
|
||
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.
Flags: needinfo?(mats)
Assignee | ||
Comment 23•10 years ago
|
||
Landed an updated test that use the -- prefix for variables: https://hg.mozilla.org/integration/mozilla-inbound/rev/f9e98ae28067
You need to log in
before you can comment on or make changes to this bug.
Description
•