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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Abhishek Arya]

Attachment: 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

Comment 1

[Cameron McCormack (:heycam)]

GetCustomPropertyNameAt is wrong; it should be indexing mVariableOrder with mOrder[aIndex], not aIndex.

Comment 2

[Cameron McCormack (:heycam)]

Attachment: patch

Comment 3

[Cameron McCormack (:heycam)]

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 4

[Cameron McCormack (:heycam)]

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.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

FWIW, trunk-only sec bugs don't need approval to land.

Comment 6

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

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.

Comment 7

[Cameron McCormack (:heycam)]

Thanks.  I'm inclined to say sec-high, since it can hand out chunks of memory to JS.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 9

[Cameron McCormack (:heycam)]

Right, it hasn't merged to Aurora yet.

Comment 10

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4153db3d9d37

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4153db3d9d37

Comment 12

[Andrew McCreight [:mccr8]]

sec-high per comment 7