Closed Bug 1613827 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla75
Tracking Status
firefox75 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

Attachments

(1 file)

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 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...

Attachment #9124953 - Flags: sec-approval?

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!

Flags: needinfo?(sledru)

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.

(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.

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

Flags: needinfo?(sledru)

Forwarded to the relevant folks

(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.

Could we get a security rating or approval here?

Flags: needinfo?(dveditz)

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.

Group: dom-core-security
Attachment #9124953 - Flags: sec-approval?
Flags: needinfo?(dveditz)
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c4ae1e6ed447
Be more careful with SharedBuffer::Create callsites. r=padenot
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla75
You need to log in before you can comment on or make changes to this bug.