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

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[q1]

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.


Details:
--------

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:

(build\stlport\stlport\stl\_string_base.h):

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;
60: #endif /* _STLP_USE_SHORT_STRING_OPTIM */
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?

Comment 1

[q1]

"for which nsTArray *is* specialized" should say "for which *type* nsTArray *is* specialized".

Comment 2

[Andrew McCreight [:mccr8]]

I suppose this may be more of an issue with IPDL codegen than Layers IPC.

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

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

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

> // nsTArray assumes that your "T" can be memmoved()'ed safely.

Ignore second part of previous comment.

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

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

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

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.

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: bug1198979-gc-ipdl-hg0.diff

It looks like bug 967844 is where the only use of this struct member was removed.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1bd9da3a1ade

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

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

Comment 9

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

As a sec-moderate, it doesn't need sec-approval no matter what. https://wiki.mozilla.org/Security/Bug_Approval_Process

I'll let Dan decide if we should open this up.

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Thanks!

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0328c0765da

Comment 12

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

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