1613827 - SharedBuffer::Create callers don't check for multiplication overflow

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Boris Zbarsky [:bzbarsky]]

A typical caller looks like this:

      RefPtr<SharedBuffer> floatBuffer = SharedBuffer::Create(
          sizeof(float) * data.mDuration * data.ChannelCount());
      auto floatData = static_cast<float*>(floatBuffer->Data());

followed by writing to floatData. If the multiplication overflowed, this can lead to out-of-bounds writes.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1613827. Be more careful with SharedBuffer::Create callsites.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9124953 [details]
Bug 1613827. Be more careful with SharedBuffer::Create callsites.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I'm not sure, because I don't know how much control web pages have over the things being multiplied.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• Which older supported branches are affected by this flaw?: Probably all of them
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: I suspect this should be pretty simple to backport.
• How likely is this patch to cause regressions; how much testing does it need?: I don't think it's likely to cause regressions, assuming I didn't get any isValid() checks backwards. If it does, they should be very obvious on treeherder, if the relevant codepaths are tested at all.

I could try to phrase the commit message to focus on the "fallible overload of SharedBuffer::Create should handle overflow in adding the buffer size better" or something like that, but I think anyone looking at this patch will immediately realize that the key is the incoming buffer sizes and whether they could have overflowed...

Comment 3

[Christian Holler (:decoder)]

This sounds like something that could be detectable with static analysis. Ccing the relevant people. Sylvestre, can you make sure this finds the right owner? Thanks!

Comment 4

[Paul Adenot (:padenot)]

I agree this is a good idea. I don't think it's a problem in practice however, I just read all the call sites and traced the code that determines the length, and we're good. It's absolutely not obvious (especially without domain knowledge about the MediaTrackGraph, which is what is moving all those buffers around) that this is the case however.

I'll write a comment under each one to explain why it's OK in the current state of the code, and then double check my reasoning and probably just r+, but I wanted to make sure there wasn't something super scary.

Comment 5

[Christian Holler (:decoder)]

(In reply to Paul Adenot (:padenot) from comment #4)

I just read all the call sites and traced the code that determines the length, and we're good. It's absolutely not obvious

In general, this type of pattern (the checks being somewhere at entrance or the middle of the call chain and the potentially dangerous operation being further away from that) has led to security vulnerabilities before, including one Pwn2Own incident.

Of course, one cannot always have the checks right in place where the operations occur (for performance reasons), but it should be the default and leaving it out for performance should be the exception (and well documented). It is too easy for people to add more callers/APIs and forget about the necessary checks.

Alternatively, we could think about enforcing more of these checks with the type system, so becomes more easily visible that something is checked and one can't accidentally forget it.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Paul, thank you for auditing those! The domain knowledge was definitely lacking over here. ;)

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Forwarded to the relevant folks

Comment 8

[Paul Adenot (:padenot)]

(In reply to Christian Holler (:decoder) from comment #5)

Of course, one cannot always have the checks right in place where the operations occur (for performance reasons), but it should be the default and leaving it out for performance should be the exception (and well documented). It is too easy for people to add more callers/APIs and forget about the necessary checks.

In the instances that Boris fixes here, there is no performance concerns: none of those locations are particularly hot. In general in media code, we don't allocate in hot paths, because it's too expensive to do so.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Could we get a security rating or approval here?

Comment 10

[Andrew McCreight [:mccr8]]

Per comment 4, it sounds like this isn't a security issue, so it could be unhidden or, if you want to keep it hidden, marked something like sec-audit or sec-other.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c4ae1e6ed447
Be more careful with SharedBuffer::Create callsites. r=padenot

Comment 12

[Bogdan Tara[:bogdan_tara | bogdant]]

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