1236358 - Improper reading of string16 in Pickle::ReadString16

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Julian Hector [:tedd] [:jhector]]

The Pickle function ReadString16 [1], doesn't properly check if a string of specified length fits inside the supplied packet data.

First the length of the string is read with ReadLength, after IteratorHasRoomFor[2] is called to check if the given string length fits inside the remaining space of the packet. But the size of char16 is not taken into consideration.

When the string data is copied in the output buffer[4] then the string may exceed the packet boundaries and copy from past the packet leading to a potential info leak.

Later on the iterator is updated using UpdateIter[3] but this time the size of char16 is taken into account.

Even though the function is not currently used anywhere in the code, it is still present and may be used in a future patch. So we should either fix the function or remove it.

[1] https://dxr.mozilla.org/mozilla-central/rev/d7a0ad85d9fb77916f9d77d62697b852f3dc63e6/ipc/chromium/src/base/pickle.cc#450
[2] https://dxr.mozilla.org/mozilla-central/rev/d7a0ad85d9fb77916f9d77d62697b852f3dc63e6/ipc/chromium/src/base/pickle.cc#458
[3] https://dxr.mozilla.org/mozilla-central/rev/d7a0ad85d9fb77916f9d77d62697b852f3dc63e6/ipc/chromium/src/base/pickle.cc#464
[4] https://dxr.mozilla.org/mozilla-central/rev/d7a0ad85d9fb77916f9d77d62697b852f3dc63e6/ipc/chromium/src/base/pickle.cc#462

Comment 1

[Andrew McCreight [:mccr8]]

Marking this sec-other because it is not currently used.

Comment 2

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

Fixed upstream in:

commit 8766556dd35a7295e2aef849a3ba33bedaa1106a
Author: [e-mail] <[e-mail]@0039d316-1c4b-4281-b951-d872f2087c98>
Date:   Thu Jun 25 16:54:02 2009 +0000

    Fix a couple of integer issues in Pickle deserialization. Neither represent
    a significant risk because the code is not directly exposed to user input. In
    addition, neither error leads to memory corruption. At worse, there's a C++
    exception or abort().
    
    BUG=NONE
    TEST=PickleTest.EvilLengths
    
    Review URL: http://codereview.chromium.org/146121

Comment 3

[Haik Aftandilian [:haik]]

Attachment: Improper reading of string16 in Pickle::ReadString16

Removed the unused ReadString16 and WriteString16 methods. Added the upstream overflow check in ReadWString to avoid the case where the (len * sizeof(wchar_t)) argument in the IteratorHasRoomFor call overflows in such a way that IteratorHasRoomFor returns true, but the actual number of bytes needed for the result->assign call is too large.

Comment 4

[Haik Aftandilian [:haik]]

Attachment: Improper reading of string16 in Pickle::ReadString16; r=jld

Comment 5

[Haik Aftandilian [:haik]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f3c2f2b0b8b
(I realize I should have obfuscated the bug subject on the try push. Note: the main issue here was in unused code).

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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

Comment 7

[Carsten Book [:Tomcat]]

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