Closed Bug 1037830 Opened 6 years ago Closed 6 years ago

GMP encoder trusts plugin's length report

Categories

(Core :: WebRTC: Audio/Video, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox32 --- unaffected
firefox33 --- wontfix
firefox34 --- fixed
firefox-esr24 --- unaffected
firefox-esr31 --- unaffected
b2g-v1.3 --- unaffected
b2g-v1.3T --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.1 --- unaffected

People

(Reporter: ekr, Assigned: jesup)

Details

(Keywords: sec-high, Whiteboard: [adv-main34+])

Attachments

(2 files, 2 obsolete files)

The following code processes encoded video data coming from the GMP plugin.

http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp#348

It reads the buffer length out of the packet at, for instance:
http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp#406

However, it does not check the buffer length and therefore a bogus size will
cause a read past the end of the buffer. We should be protecting against a misbehaving plugin here.
Assignee: nobody → rjesup
Attachment #8454908 - Flags: review?(ekr)
Comment on attachment 8454908 [details] [diff] [review]
Enforce size limit on returned encoded data from GMP plugins for webrtc

Review of attachment 8454908 [details] [diff] [review]:
-----------------------------------------------------------------

Jesup,

If this is really what you want, I will r+ it, but I would suggest cleaning up
the below. I believe it does fix the proximal problem.

::: media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp
@@ +352,5 @@
> +        size_bytes = 4;
> +        break;
> +      default:
> +        // really that it's not in the enum; gives more readable error
> +        MOZ_ASSERT(aEncodedFrame->BufferType() != GMP_BufferSingle);

Do we actually want an assert here? This would mean that plugin misbehavior crashes FFOX in debug, right?

@@ +361,3 @@
>      uint32_t size;
> +    // make sure we don't read past the end of the buffer getting the size
> +    while (buffer+size_bytes < end) {

This seems like it fails to report plugin errors where the remainder is < size_bytes. Wouldn't that be more helpful?

@@ +391,5 @@
> +      }
> +      if (buffer+size > end) {
> +        LOG(PR_LOG_ERROR,
> +            ("GMP plugin returned badly formatted encoded data: end is %d bytes past buffer end",
> +             buffer+size - end));

/Is %d really what we want for ptrdiff_t
Attachment #8454908 - Flags: review?(ekr)
Attachment #8454908 - Attachment is obsolete: true
Comment on attachment 8454921 [details] [diff] [review]
Enforce size limit on returned encoded data from GMP plugins for webrtc

Updated.  Left the MOZ_ASSERT on bad type in DEBUG builds in on purpose.  

f? to cpearce on that issue.
Attachment #8454921 - Flags: review?(ekr)
Attachment #8454921 - Flags: feedback?(cpearce)
(In reply to Randell Jesup [:jesup] from comment #4)
> Comment on attachment 8454921 [details] [diff] [review]
> Enforce size limit on returned encoded data from GMP plugins for webrtc
> 
> Updated.  Left the MOZ_ASSERT on bad type in DEBUG builds in on purpose.  
> 
> f? to cpearce on that issue.

Making mistakes more visible in debug builds sounds like a good thing to me.
Attachment #8454921 - Flags: feedback?(cpearce) → feedback+
(In reply to Chris Pearce (:cpearce) from comment #5)
> (In reply to Randell Jesup [:jesup] from comment #4)
> > Comment on attachment 8454921 [details] [diff] [review]
> > Enforce size limit on returned encoded data from GMP plugins for webrtc
> > 
> > Updated.  Left the MOZ_ASSERT on bad type in DEBUG builds in on purpose.  
> > 
> > f? to cpearce on that issue.
> 
> Making mistakes more visible in debug builds sounds like a good thing to me.

My concern here is that defects in the plugin should not crash the browser.

The browser should kill the plugin here, perhaps...
This sounds like sec-moderate at worst to me, because it requires a misbehaving plugin.  I assume we're not trying to protect against an arbitrary misbehaving plugin?  Requiring a user to install a misbehaving plugin would mitigate it a bit to me.
(In reply to Andrew McCreight (Away July 17-24) [:mccr8] from comment #7)
> This sounds like sec-moderate at worst to me, because it requires a
> misbehaving plugin.  I assume we're not trying to protect against an
> arbitrary misbehaving plugin?  Requiring a user to install a misbehaving
> plugin would mitigate it a bit to me.

We are trying to protect against plugin compromise which could lead to a
misbehaving plugin. Hence sandboxing the plugin.
Ok, thanks.  I guess I'll mark this sec-high.
Keywords: sec-high
don't crash; look to improve this in an update of the GMP codec <-> users API RSN
Attachment #8454921 - Attachment is obsolete: true
Attachment #8454921 - Flags: review?(ekr)
Comment on attachment 8459703 [details] [diff] [review]
Enforce size limit on returned encoded data from GMP plugins for webrtc

We'll need uplift once this is on central
Attachment #8459703 - Flags: review?(ekr)
ping
Flags: needinfo?(ekr)
I'm at IETF. I should get to this this weekend. If you need it now, needinfo?josh
Flags: needinfo?(ekr)
Comment on attachment 8459703 [details] [diff] [review]
Enforce size limit on returned encoded data from GMP plugins for webrtc

Review of attachment 8459703 [details] [diff] [review]:
-----------------------------------------------------------------

r+ but IMO you should rewrite the length reading code to be a loop.
It would be less code and you wouldn't need to worry about unaligned
reads.

::: media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp
@@ +393,5 @@
>            buffer += 4;
>            break;
>          default:
> +          break; // already handled above
> +      }

Wouldn't this code be cleaner with a loop incrementing
through size_bytes?

size_t size = 0;

for (size_t i=0; i < size_bytes; ++i) {
  size_t b = *buffer;
  size += b << (i * 8);
  ++buffer;
}

@@ +395,5 @@
>          default:
> +          break; // already handled above
> +      }
> +      if (buffer+size > end) {
> +        // XXX see above - should we kill the plugin for returning extra bytes?  Probably

This comment is confusing. Unless I misunderstand you don't do that.
Attachment #8459703 - Flags: review?(ekr) → review+
Attachment #8466620 - Attachment description: Enforce size limit on returned encoded data from GMP plugins for webrtc → Enforce size limit on returned encoded data from GMP plugins for webrtc (unbitrotted)
This should have gone through sec-approval since it affects 33 and 34.
Whiteboard: [adv-main34+]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.