Closed Bug 1412313 Opened 2 years ago Closed 2 years ago

ParamTraits<nsAString> Deserialization - Integer Overflow

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 - wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 + fixed

People

(Reporter: stephen, Assigned: Alex_Gaynor)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-intoverflow, sec-moderate, Whiteboard: [adv-main58+][post-critsmash-triage])

Attachments

(1 file, 1 obsolete file)

As part of a code review I am conducting for Paul Theriault against the sandbox, the following vulnerability has been discovered.

The read method for ParamTraits<nsAString> is susceptible to an integer overflow when using the uint32_t length value for calculating the amount to read in the ReadBytesInto parameter. A length value > 0x80000000 multiplied by sizeof(char16_t) could overflow and cause ReadBytesInto to read less data than intended, leaving the string with uninitialized data in place which could potentially be accessible by the attacker (and cause an info leak).

> template <>
> struct ParamTraits<nsAString>
> {
>   typedef nsAString paramType;
> 
>   static bool Read(const Message* aMsg, PickleIterator* aIter, paramType* aResult)
>   {
>     bool isVoid;
>     if (!aMsg->ReadBool(aIter, &isVoid))
>       return false;
> 
>     if (isVoid) {
>       aResult->SetIsVoid(true);
>       return true;
>     }
> 
>     uint32_t length;
>     if (!ReadParam(aMsg, aIter, &length)) {
>       return false;
>     }
>     aResult->SetLength(length);
> 
>     return aMsg->ReadBytesInto(aIter, aResult->BeginWriting(), length * sizeof(char16_t));
>   }
  
For completeness and to match the other string de serialization routines (See Pickle::ReadWString in .\ipc\chromium\src\base\pickle.cc) a multiplication overflow check should be added after ReadParam is called and before SetLength is called. The check would look like this:

>    // Avoid integer multiplication overflow.
>    if (length > UINT_MAX / static_cast<uint32_t>(sizeof(char16_t))) {
>      return false;
>    }
Group: core-security → dom-core-security
Attached patch 1412313.patch (obsolete) — Splinter Review
Straightforward patch.

Do we have an existing internal library for doing safe arithmetic? If not, I'd like to file a public bug to add one, with the intent of using it in places like ParamTraits and other parts of IPC. Thoughts?
I think we should use CheckedInt for this (mozilla/CheckedInt.h).
Perfect, that's exactly what I was looking for.

Note to self: put CheckedInt in the secure-ipc-coding-guide that Julian started.
Attached patch 1412313.patchSplinter Review
Assignee: nobody → agaynor
Attachment #8922950 - Attachment is obsolete: true
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8922955 - Flags: review?(wmccloskey)
Attachment #8922955 - Flags: review?(wmccloskey) → review+
Comment on attachment 8922955 [details] [diff] [review]
1412313.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

-----> Very easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

-----> The checkin message combined with the patch contents makes it very obvious.

Which older supported branches are affected by this flaw?

-----> All branches, but this is a sandbox escape, and the sandbox wasn't particularly useful in ESR52 (I'm not even sure how it was enabled on each platform).

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

-----> Haven't created them, looks like they should be trivial.

How likely is this patch to cause regressions; how much testing does it need?

-----> Unlikely, this code is extremely frequently used.
Attachment #8922955 - Flags: sec-approval?
If I'm understanding this correctly there's no overwrite as a result of the overflow. Rather the message is truncated and padded with junk from memory in the process on the other side of the sandbox.
Whiteboard: sandbox escape
Now that I look at this more closely, I'm not sure it's more than a DOS. It looks like strings have a kMaxCapacity that is too small to cause the overflow:
http://searchfox.org/mozilla-central/rev/831f2ed98fd6fe85d1e775d67e5da2d419608551/xpcom/string/nsTSubstring.cpp#17

When we try to call SetLength on the string with an overly large length, we'll intentionally crash. I haven't tried this, though, and the logic is a little complex.
Comment on attachment 8922955 [details] [diff] [review]
1412313.patch

This doesn't need sec-approval now and can just go into mozilla-central.
Attachment #8922955 - Flags: sec-approval?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7cc3c1afc8ce

I'm going to assume this can just ride the trains, but feel free to set the statuses back to affected and nominate for approval if you feel strongly otherwise.
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Group: dom-core-security → core-security-release
Whiteboard: [adv-main58+]
Flags: qe-verify-
Whiteboard: [adv-main58+] → [adv-main58+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.