When unserializing GonkNativeHandle [1], the return value of the native_handle_create(...) call is not checked for a NULL return, which is a possible return value [2]. This would then lead to a NULL pointer dereference here [3] Furthermore I think that the function ReadBytes [4] is not properly used. From my understanding, ReadBytes doesn't copy the data into the given pointer pointer, instead it returns a pointer[5] into the serialized message after it checked whether or not a byte sequence of the given length fits in there. Therefore, this call [6] doesn't actually read into the native handle (unless I am missing something) GonkNativeHandle is used as part of the PImageBridge protocol, and based on a discussion in #gfx, this protocol is spoken between parent process and content process. So I filed this as a security bug just in case (I also wasn't sure about the right component to file this under, feel free to change that). The affected code was introduced in Bug 1234472. [1] https://dxr.mozilla.org/mozilla-central/rev/c33f30666b37dbceffb9fbe5089a668db8893a85/gfx/layers/ipc/GonkNativeHandleUtils.cpp#33 [2] http://androidxref.com/6.0.0_r1/xref/system/core/libcutils/native_handle.c#31 [3] https://dxr.mozilla.org/mozilla-central/rev/c33f30666b37dbceffb9fbe5089a668db8893a85/gfx/layers/ipc/GonkNativeHandleUtils.cpp#54 [4] https://dxr.mozilla.org/mozilla-central/rev/6020a4cb41a77a09484c24a5875bb221714c0e6a/ipc/chromium/src/base/pickle.cc#468 [5] https://dxr.mozilla.org/mozilla-central/rev/6020a4cb41a77a09484c24a5875bb221714c0e6a/ipc/chromium/src/base/pickle.cc#494 [6] https://dxr.mozilla.org/mozilla-central/rev/c33f30666b37dbceffb9fbe5089a668db8893a85/gfx/layers/ipc/GonkNativeHandleUtils.cpp#45

Thanks for creating the bug. I am going to address it.

Sotaro, how much of a security issue do you think this is? The null deref isn't much of an issue, but it sounds like maybe things could go wrong with the other part of it? Thanks.

(In reply to Andrew McCreight [:mccr8] from comment #2) > Sotaro, how much of a security issue do you think this is? The null deref > isn't much of an issue, but it sounds like maybe things could go wrong with > the other part of it? Thanks. The null deref could cause chrome process abort.

ParamTraits<GonkNativeHandle>::Read() also has problem of numInts and numBytes handling.

Thanks :sotaro for the patch, I also had a look at it and it would introduce a vulnerability, since an attacker can cause the allocation of lesser memory than needed. For example: When an unaligned |nbytes| is supplied (for example 7), then > size_t numInts = nbytes / sizeof(int); will result in |numInts| being 7/4 which equals to 1, because of size_t. Next the allocation in native_create_handle() [1] will only allocate |numInts| * sizeof(int) (for the integer data) which equals to 4. Next the memcpy (of the patch): > memcpy(nativeHandle->data + nativeHandle->numFds, data, nbytes); copies |nbytes| which is still 7. Depending on the heap implementation, chunks will be aligned so there might be enough room available in the chunk. But it is highly likely that this could result in a heap overflow (3 bytes in the above mentioned case) [1] http://androidxref.com/6.0.0_r1/xref/system/core/libcutils/native_handle.c#37

Comment on attachment 8709261 [details] [diff] [review] patch - Update ParamTraits<GonkNativeHandle>::Read() Do not land! Julien Hector has found a serious security issue. See Comment 7 NI to :nical so he is aware.

(In reply to Julian Hector [:tedd] [:jhector] from comment #7) > Thanks :sotaro for the patch, I also had a look at it and it would introduce > a vulnerability, since an attacker can cause the allocation of lesser memory > than needed. Indeed, and we even already have this issue in GonkNativeHandleUtils.cpp from a patch which I also reviewed, so it's the second time I let this pass, sorry. I'll be more careful about this class of errors in the future.

Thanks for the review. I am going to update the patch.

Comment on attachment 8709731 [details] [diff] [review] patch - Update ParamTraits<GonkNativeHandle>::Read() [Security approval request comment] How easily could an exploit be constructed based on the patch? Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? > No. The patch does not have a comment of saying that the patch fixes a security problem. Which older supported branches are affected by this flaw? > It affect only to master(b2g v2.6). If not all supported branches, which bug introduced the flaw? > Bug 1234472 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? > The bug does not affect to branches. Backports are not necessary. How likely is this patch to cause regressions; how much testing does it need? > It is not likely to cause regressions. There is no use case of using the code now.

(In reply to Sotaro Ikeda [:sotaro] from comment #12) > [Security approval request comment] > How easily could an exploit be constructed based on the patch? Can you answer this question?

Thoughts on when to check this in, Paul?

(In reply to Al Billings [:abillings] from comment #13) > (In reply to Sotaro Ikeda [:sotaro] from comment #12) > > > [Security approval request comment] > > How easily could an exploit be constructed based on the patch? > > Can you answer this question? It is difficult.

(In reply to Al Billings [:abillings] from comment #14) > Thoughts on when to check this in, Paul? There are no products on 2.6 yet and Sotaro says above that this code isn't actually used. Land away I think.

Comment on attachment 8709731 [details] [diff] [review] patch - Update ParamTraits<GonkNativeHandle>::Read() sec-approval+

Not writing a Firefox 47 advisory as this is b2g only.