2008293 - Integer overflow in ContentParent::RecvGetIconForExtension

Status: NEW

(GeckoView :: General, defect)

Description

[Steven Julian]

Steps to reproduce:

1. Reviewed IPC handlers in ContentParent that accept size parameters from untrusted child processes.

2. Analyzed ContentParent::RecvGetIconForExtension in dom/ipc/ContentParent.cpp.

3. Identified unchecked arithmetic using the IPC parameter aIconSize when calculating allocation size:

   bits->AppendElements(aIconSize * aIconSize * 4);

4. Reasoned about integer overflow behavior for large aIconSize values (e.g. >= 32768).

5. Traced the downstream usage of the allocated buffer, where pixel data is written based on the semantic icon size rather than the allocated length.

Actual results:

The untrusted IPC parameter aIconSize (uint32_t) is used directly in a chained multiplication to compute an allocation size without overflow checking.

For sufficiently large values of aIconSize, the expression aIconSize * aIconSize * 4 overflows in 32-bit arithmetic, resulting in an undersized allocation via nsTArray::AppendElements.

Downstream code uses the semantic icon size (aIconSize × aIconSize × 4 bytes) when populating the buffer, which can result in writes past the end of the allocated buffer, leading to heap buffer overflow in the parent process.

Expected results:

Allocation size calculations derived from IPC parameters should be validated against integer overflow before allocation.

The code should use overflow-checked arithmetic (e.g. mozilla::CheckedInt or SafeInt), and reject IPC messages where the computed allocation size is invalid or unreasonably large, preventing undersized allocations and potential memory corruption.

Comment 1

[Andrew McCreight [:mccr8]]

Android-only code. I'll add the sandbox-escape keyword here, but I don't know the actual sandboxing situation on Android so maybe it isn't.

Comment 2

[Andrew McCreight [:mccr8]]

I guess this is really kind of a Android widget issue, so I'll move it to Geckoview.

Comment 3

[Andrew McCreight [:mccr8]]

Here is the recv code:

  bits->AppendElements(aIconSize * aIconSize * 4);
  AndroidBridge::Bridge()->GetIconForExtension(aFileExt, aIconSize, bits->Elements());

Here is the relevant code in the bridge method:

  uint32_t len = static_cast<uint32_t>(env->GetArrayLength(arr.Get()));
  jbyte* elements = env->GetByteArrayElements(arr.Get(), 0);
  uint32_t bufSize = aIconSize * aIconSize * 4;
  if (len == bufSize) memcpy(aBuf, elements, bufSize);

I'll have to double check this, but I think this code won't actually result in a buffer overflow. bits can end up truncated relative to aIconSize, but AndroidBridge::GetIconForExtension actually repeats the potentially overflowing calculation to get bufSize, so I think it will still be the correct length for aBuf. The code also explicitly checks that the source and destination buffers have the same length so there can be an issue there, either.

The overflowing operations are unsigned, so I think it isn't undefined behavior, either.

So, I think this isn't actually a security problem, although I do think we want to avoid this kind of "two wrongs make a right" situation in the Recv method so I think it is good for a static analysis to complain about it.

Do you agree with my analysis, or am I overlooking something? Thanks.

Comment 4

[Andrew McCreight [:mccr8]]

It would probably be a good idea to mozilla::Span-ify AndroidBridge::GetIconForExtension but I'm not sure how much that will fan out.

Comment 9

[Steven Julian]

Hey Andrew, i just thinking here if there's still remain Potential DOS due to Allocation Panic.

An attacker can craft an aIconSize such that the calculation aIconSize * aIconSize * 4 does not wrap around to a small number, but instead results in a massive valid allocation size (e.g., close to 2GB or the process memory limit).

For example, if aIconSize is approx 23,000, the result is ~2.1GB.
Since nsTArray typically operates with infallible semantics in IPC (or simply because contiguous allocations of this magnitude are likely to fail on mobile), the allocation failure will trigger an immediate MOZ_CRASH / abort().

How about that?

Comment 10

[Andrew McCreight [:mccr8]]

Well, that doesn't seem great, but if somebody manages to take over the child process and all they do is crash the browser, that's not really a big deal. We don't really treat those kinds of resource exhaustion issues as security bugs.

Comment 11

[Steven Julian]

Understood. Thanks for clarifying

Comment 12

[BugBot [:suhaib / :marco]]

The severity field is not set for this bug.
:owlish, could you have a look please?

For more information, please visit BugBot documentation.

Comment 13

[BugBot [:suhaib / :marco]]

The severity field is not set for this bug.
:owlish, could you have a look please?

For more information, please visit BugBot documentation.