Closed Bug 1198979 Opened 6 years ago Closed 6 years ago

Memory-safety bug in mozilla::layers::Edit (via nsTArray)


(Core :: Graphics: Layers, defect)

40 Branch
Not set



Tracking Status
firefox43 --- fixed


(Reporter: q1, Assigned: jld)



(Keywords: sec-want, Whiteboard: [adv-main43-])


(1 file)

mozilla::layers::Edit (obj*\ipc\ipdl\_ipdlheaders\mozilla\layers\LayersMessages.h) is stored in TArray objects (as in PLayerTransactionChild::Read), but it indirectly contains self-pointers in some cases, making this storage unsafe. Because *TArray objects by default memcpy their elements during reallocation (as when the array is expanded), it is generally unsafe to store within them objects that contain self-pointers, because memcpy leaves these pointers pointing at a given object's previous storage, instead of updating them to point to the object's new storage. This restriction can be overcome by specializing nsTArray, as has been done for, e.g., js::Heap, but that has not been done for layers::Edit.

It is unclear whether this bug is active or latent; it would be latent if the questionable TArrays' capacities are never expanded after the first element is added.


mozilla::layers::Edit contains an OpSetLayerAttributes object (as a char array, which it manipulates as an OpSetLayerAttributes)
OpSetLayerAttributes contains a LayerAttributes object 
LayerAttributes contains a CommonLayerAttributes object
CommonLayerAttributes contains the std::string object contentDescription_

On at least STLPort (which is used in Gonk) std::string's |_M_start_of_storage._M_data| pointer can point to "short string storage" within the object. This is so when it is built with _STLP_USE_SHORT_STRING_OPTIM (which is the default) and when the string is sufficiently short:


53: #if defined (_STLP_USE_SHORT_STRING_OPTIM)
54:   union _Buffers {
55:     _Tp*  _M_end_of_storage;
56:     _Tp   _M_static_buf[_DEFAULT_SIZE];
57:   } _M_buffers;
58: #else
59:   _Tp*    _M_end_of_storage;
61: protected:
62: #if defined (_STLP_USE_SHORT_STRING_OPTIM)
63:   bool _M_using_static_buf() const
64:   { return (_M_start_of_storage._M_data == _M_buffers._M_static_buf); }
65:   _Tp const* _M_Start() const { return _M_start_of_storage._M_data; }
66:   _Tp* _M_Start() { return _M_start_of_storage._M_data; }
69:   _Tp* _M_End()
70:   { return _M_using_static_buf() ? _M_buffers._M_static_buf + _DEFAULT_SIZE : _M_buffers._M_end_of_storage; }
71:   size_type _M_capacity() const
72:   { return _M_using_static_buf() ? _DEFAULT_SIZE : _M_buffers._M_end_of_storage - _M_start_of_storage._M_data; }
75: #else
86:   _Tp*    _M_finish;
87:   _AllocProxy _M_start_of_storage;

The code uses _M_Start() (line 65) to access its buffer, which simply returns |_M_start_of_storage._M_data|. Thus, if an std::string object that is using short string storage is moved with memcpy, its _M_data member still points to the _M_static_buf in its former location, which rightfully belongs to some other object. Also, the object incorrectly will conclude that it's using heap storage, because _M_start_of_storage._M_data will != _M_buffers._M_static_buf (see _M_using_static_buf() at lines 63-4) . This will cause other malfunctions; for example, _M_capacity() (lines 71-2) will return an incorrect -- and possibly gigantic -- value.

It seems that the std::string that comes with VS 2013 is unaffected, though I have not puzzled out all of its implementation.

[1] On a related topic, CommonLayerAttributes also contains the member

   nsIntRegion visibleRegion_;

for which nsTArray *is* specialized to avoid memcpy. There does not appear to be a reason for this, other than avoiding the undefined behavior inherent in memcpy-ing anything that isn't a trivially-copyable object. Does anyone know why this was done?
"for which nsTArray *is* specialized" should say "for which *type* nsTArray *is* specialized".
Flags: sec-bounty?
Group: firefox-core-security → core-security
Component: Untriaged → Graphics: Layers
Product: Firefox → Core
I suppose this may be more of an issue with IPDL codegen than Layers IPC.
Summary: Memory-safety bug in mozilla::layers::Edit → Memory-safety bug in mozilla::layers::Edit (via nsTArray)
(In reply to Andrew McCreight [:mccr8] from comment #2)
> I suppose this may be more of an issue with IPDL codegen than Layers IPC.

It looks to me like it's either a bug in the IPDL compiler (not emitting specializations to prevent the memcpy-ing) or in nsTArray (memcpy-ing non-PoD types).
> // nsTArray assumes that your "T" can be memmoved()'ed safely.

Ignore second part of previous comment.
Component: Graphics: Layers → IPC
I notice that we've recently added a static analysis for this (bug 1159433).  I've made a few enhancements in a local branch, and:

> ../../dist/include/nsTArray.h:662:34: error: Cannot instantiate 'nsTArray_CopyChooser<mozilla::layers::Edit>' with non-memmovable template argument 'mozilla::layers::Edit'
> struct MOZ_NEEDS_MEMMOVABLE_TYPE nsTArray_CopyChooser
>                                  ^
> ../../dist/include/nsTArray.h:793:42: note: instantiation of 'nsTArray_CopyChooser<mozilla::layers::Edit>' requested here
>   : public nsTArray_base<Alloc, typename nsTArray_CopyChooser<E>::Type>
>                                          ^
> /usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/basic_string.tcc:1132:25: note: 'mozilla::layers::Edit' is non-memmovable because of the MOZ_NON_MEMMOVABLE annotation on 'basic_string<char, std::char_traits<char>, std::allocator<char> >'
>   extern template class basic_string<char>;
>                         ^
> 1 error generated.

The good news is that is the only instance of this bug it finds (although, as the error message suggests, I special-cased std::basic_string and the analysis otherwise depends on opt-in Gecko-specific annotations, so there may be other similar bugs it doesn't find yet).
Assignee: nobody → jld
Keywords: sec-moderate
It turns out that CommonLayerAttributes::contentDescription isn't actually used in (non-IPDL-generated) code, which gives us an easy fix.  This also means I don't have to teach the IPDL compiler to generate nsTArray_CopyChooser specializations; instead, I can just land the patches that let the static analysis build detect this bug.

As for the impact: ParamTraits<FallibleTArray<T>>::Read does a SetCapacity first (and only appends elements; no general insertions/deletions that might be implemented with memmove()), so there shouldn't be reallocation from that, and the LayerTransactionParent::RecvUpdate* methods appear to only iterate over the array, not modify it.  Which, if I understand correctly, would mean this isn't exploitable.  In any case I'm glad it was reported, so we can try not to let any more potential bugs like this land.
Group: core-security → dom-core-security
Component: IPC → Graphics: Layers
It looks like bug 967844 is where the only use of this struct member was removed.

Attachment #8656034 - Flags: review?(jmuizelaar)
Do I need sec-approval, and does this bug still need to be private?  I'm reasonably sure it's not exploitable (comment #6), and there seem to be no similar bugs hiding in the code, at least with respect to std::basic_string and the nsAuto* containers (comment #5).
Flags: needinfo?(dveditz)
As a sec-moderate, it doesn't need sec-approval no matter what.

I'll let Dan decide if we should open this up.
Group: dom-core-security
Flags: needinfo?(dveditz)
Keywords: sec-moderatesec-want
Attachment #8656034 - Flags: review?(jmuizelaar) → review+
See Also: → 1201309
See Also: → 1201314
See Also: → 1201329
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Flags: sec-bounty? → sec-bounty+
Whiteboard: [adv-main43-]
You need to log in before you can comment on or make changes to this bug.