Closed Bug 955913 Opened 6 years ago Closed 6 years ago

Heap-buffer-overflow in mozilla::css::Declaration::GetCustomPropertyNameAt

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox27 --- unaffected
firefox28 --- unaffected
firefox29 --- fixed
firefox-esr24 --- unaffected
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- unaffected
b2g-v1.3 --- unaffected

People

(Reporter: inferno, Assigned: heycam)

References

Details

(Keywords: csectype-bounds, sec-high)

Attachments

(2 files)

Attached file test.html
==1132==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x615000a3d6b8 at pc 0x7fa26818b3ea bp 0x7fff5ce2e850 sp 0x7fff5ce2e848
READ of size 8 at 0x615000a3d6b8 thread T0
    #0 0x7fa26818b3e9 in nsAString_internal::Data() const extensions/gio/../../dist/include/nsTSubstring.h:209
    #1 0x7fa268148bd7 in nsAString_internal::Replace(unsigned int, unsigned int, nsAString_internal const&) extensions/gio/../../dist/include/nsTSubstring.h:438
    #2 0x7fa2682ada50 in nsAString_internal::Append(nsAString_internal const&) extensions/gio/../../dist/include/nsTSubstring.h:450
    #3 0x7fa2789b8b99 in mozilla::css::Declaration::GetCustomPropertyNameAt(unsigned int, nsAString_internal&) const layout/style/../../dist/include/mozilla/css/Declaration.h:315
    #4 0x7fa2789b864d in mozilla::css::Declaration::GetNthProperty(unsigned int, nsAString_internal&) const /usr/local/google/home/aarya/firefox/src/layout/style/Declaration.cpp:1152
    #5 0x7fa278cf2772 in nsDOMCSSDeclaration::IndexedGetter(unsigned int, bool&, nsAString_internal&) /usr/local/google/home/aarya/firefox/src/layout/style/nsDOMCSSDeclaration.cpp:165
    #6 0x7fa26e8b0945 in mozilla::dom::CSS2PropertiesBinding::DOMProxyHandler::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) dom/bindings/./CSS2PropertiesBinding.cpp:19927
    #7 0x7fa28654ac61 in js::Proxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) /usr/local/google/home/aarya/firefox/src/js/src/jsproxy.cpp:2508
    #8 0x7fa286566853 in proxy_GetGeneric(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) /usr/local/google/home/aarya/firefox/src/js/src/jsproxy.cpp:2876
    #9 0x7fa2865683c7 in proxy_GetElement(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, unsigned int, JS::MutableHandle<JS::Value>) /usr/local/google/home/aarya/firefox/src/js/src/jsproxy.cpp:2894
    #10 0x7fa284375714 in JSObject::getElement(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, unsigned int, JS::MutableHandle<JS::Value>) /usr/local/google/home/aarya/firefox/src/js/src/jsobjinlines.h:621
    #11 0x7fa284ed358f in js::GetObjectElementOperation(JSContext*, JSOp, JSObject*, bool, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) /usr/local/google/home/aarya/firefox/src/js/src/vm/Interpreter-inl.h:348:18
    #12 0x7fa284ed358f in js::GetElementOperation(JSContext*, JSOp, JS::MutableHandle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) /usr/local/google/home/aarya/firefox/src/js/src/vm/Interpreter-inl.h:455
    #13 0x7fa284ed358f in js::jit::DoGetElemFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICGetElem_Fallback*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) /usr/local/google/home/aarya/firefox/src/js/src/jit/BaselineIC.cpp:3978
    #14 0x7fa251aeb9a7 in
0x615000a3d6b8 is located 160 bytes to the right of 24-byte region [0x615000a3d600,0x615000a3d618)
allocated by thread T0 here:
    #0 0x434065 in
Shadow bytes around the buggy address:
  0x0c2a8013fa80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2a8013fa90: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2a8013faa0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2a8013fab0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2a8013fac0: 00 00 00 fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c2a8013fad0: fa fa fa fa fa fa fa[fa]fa fa fa fa fa fa fa fa
  0x0c2a8013fae0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2a8013faf0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2a8013fb00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2a8013fb10: 00 00 00 00 06 fa fa fa fa fa fa fa fa fa fa fa
  0x0c2a8013fb20: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
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
==1132==ABORTING
GetCustomPropertyNameAt is wrong; it should be indexing mVariableOrder with mOrder[aIndex], not aIndex.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Hardware: x86_64 → All
Attached patch patchSplinter Review
Attachment #8355087 - Flags: review?(dbaron)
The bug means we can interpret memory past the end of the array we're indexing as an nsString object, which we then try to append to a result string (that can then be passed back to script).  Depending on the values of the "string"'s mData pointer and mLength, we'll either crash when trying to access the bad pointer or we'll copy a bunch of memory to the result string.
Comment on attachment 8355087 [details] [diff] [review]
patch

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

Not easily just based on the patch.  You'd need to analyse what's likely to be in the heap following the array of nsStrings and see how easily they're controllable.


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

Don't think so?


Which older supported branches are affected by this flaw?

None.


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

Bug 773296.


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

N/A


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

Not likely.  I think the added test is sufficient.
Attachment #8355087 - Flags: sec-approval?
FWIW, trunk-only sec bugs don't need approval to land.
As Ryan says, you don't need sec-approval for trunk only bugs (see https://wiki.mozilla.org/Security/Bug_Approval_Process). That said, this bug does need a security rating in general.
Attachment #8355087 - Flags: sec-approval?
Thanks.  I'm inclined to say sec-high, since it can hand out chunks of memory to JS.
Comment on attachment 8355087 [details] [diff] [review]
patch

r=dbaron

(And please backport if this is on any branches, though my memory says it's not.)
Attachment #8355087 - Flags: review?(dbaron) → review+
Right, it hasn't merged to Aurora yet.
https://hg.mozilla.org/mozilla-central/rev/4153db3d9d37
Blocks: 773296
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
sec-high per comment 7
Keywords: sec-high
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Group: core-security
You need to log in before you can comment on or make changes to this bug.