1037830 - GMP encoder trusts plugin's length report

Status: RESOLVED FIXED

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

Description

[Eric Rescorla (:ekr)]

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.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Enforce size limit on returned encoded data from GMP plugins for webrtc

Comment 2

[Eric Rescorla (: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

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Enforce size limit on returned encoded data from GMP plugins for webrtc

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 6

[Eric Rescorla (:ekr)]

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

Comment 7

[Andrew McCreight [:mccr8]]

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.

Comment 8

[Eric Rescorla (:ekr)]

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

Comment 9

[Andrew McCreight [:mccr8]]

Ok, thanks.  I guess I'll mark this sec-high.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Enforce size limit on returned encoded data from GMP plugins for webrtc

don't crash; look to improve this in an update of the GMP codec <-> users API RSN

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

ping

Comment 13

[Eric Rescorla (:ekr)]

I'm at IETF. I should get to this this weekend. If you need it now, needinfo?josh

Comment 14

[Eric Rescorla (: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.

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Enforce size limit on returned encoded data from GMP plugins for webrtc (unbitrotted)

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e7b30eb1a7c

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0e7b30eb1a7c

Comment 18

[Al Billings [:abillings - ex-MoCo]]

This should have gone through sec-approval since it affects 33 and 34.