1294769 - gmp-clearkey AnnexB::ConvertFrameInPlace potential out of bounds write

Status: RESOLVED FIXED

(Core :: Audio/Video: GMP, defect, P3)

Description

[kernxploit]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160623154057

Steps to reproduce:

AnnexB::ConvertFrameInPlace does not verify "nalLen" loaded from the input buffer data. This might cause an integer overflow in
i += nalLen + 4;
so it could pass the bounds check
i < aBuffer.size() - 4 - sizeof(kAnnexBDelimiter) + 1
This results in a memcpy
memcpy(&aBuffer[i], kAnnexBDelimiter, sizeof(kAnnexBDelimiter));
which might be outside the buffer.

https://github.com/cpearce/gmp-clearkey/blob/master/src/AnnexB.cpp#L27

Maybe I am just overlooking something and this data is already checked anywhere?

Comment 1

[Daniel Veditz [:dveditz]]

Could you take a look, Gerald?

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

The analysis in comment 0 seems correct to me: nalLen is not sanity-checked, and then could produce an overflow and subsequent OOB reads&writes.
So it would probably be a good idea to fix that potential issue anyway.

Now, as to the actual risk in Firefox: Prior to that decoding,
- There is a conversion *from* Annex B in H264Converter, which means in this case *we* write NAL lengths so they should be correct&safe.
- In non-Annex B cases, NAL lengths might be (implicitly or explicitly) checked during demuxing, but I'm not sure.

Chris, would you happen to know the answer? NI:me back if you want me to investigate further.
(Or Jean-Yves might also know, as I think he's worked on NALs a lot.)

Comment 3

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

I'm pretty sure if the NAL has an invalid length, we'll fail to read it in H264::DecodeNALUnit() in the parent process, and so we'd never end up sending a bad NAL to the GMP process.

It would be good practice to check the nal length here for certain regardless.

Comment 4

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

Confirmed in that we at least want to add belt-and-suspenders, even if this can't happen at the moment per comment 3

Tentatively marking as moderate given comment 3

Comment 5

[Jean-Yves Avenard [:jya]]

We had the same issue in the media/libstagefright/binding code. The AnnexB was rewritten with ByteReader to prevent all out of bound access. Maybe we can just resync the code there (I believe they have common ancestry)

Comment 6

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

jya, cpearce, gerald - can we do one of these things and get this off our plate?  Doesn't sound like some belt-and-suspenders checks here would be tough.

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Effectively fixed by bug 1318965, which removed media/gmp-clearkey/0.1/AnnexB.cpp.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Is there something we can safely backport to ESR52 for this or should we wontfix?

Comment 9

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Sorry for the delay in responding to the NI.

I think it wouldn't be possible to backport a fix from bug 1318965, as it's super big (and the actual solution there just removed the whole file!)
And I don't want to just wontfix it, in case it's an actually-exploitable issue.

A small patch to catch the overflow in ESR52 should be easy, I'll work on it so it's ready for the next dot release...

Comment 10

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Reopening bug, so it's less likely to be forgotten (by me).

Comment 11

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

Mass change P3->P4 to align with new Mozilla triage process.

Comment 12

[Nils Ohlmeier [:drno]]

Since the last 52 ESR ship has sailed I think we can close this.

Comment 13

[kernxploit]

Have you decided if that was an "actually-exploitable issue"?

Comment 14

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

I believe this was never exploitable, due to what clearkey is, and also where the issue was; we were just considering some added safety checks.  Chris? Gerald?

Comment 15

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Right. I too think it's not exploitable, but I didn't dive deep enough to prove it for sure.