1512450 - Potential heap buffer overflow in audio-ipc

Status: RESOLVED FIXED

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

Description

[Alex Gaynor [:Alex_Gaynor]]

I'm not an expert in the audioipc code, so let me know if I misunderstood any of this.

Relevant code is: https://github.com/djg/audioipc-2/blob/master/server/src/server.rs#L107-L110

As far as I can tell, we get |frames| from a remote peer, and there's no bounds checking to ensure |output| is at least |nbytes| long. As a result, the peer can cause you can make |real_output| longer than the underlying storage, causing |self.output_shm.read(&mut real_output[..nbytes])| to read past the end of the data.

I don't see a reason for |unsafe| to be used at all here, it's simply taking a slice of the slice. Have I misunderstood?

Comment 1

[Matthew Gregan [:kinetik]]

I looked at this last week but got distracted and lost my comment:

Just to clarify: the "remote peer" can only be a local process bootstrapped by the Gecko parent since audioipc is restricted to anonymous local socket pairs (Unix domain sockets, specifically).  Still, definitely needs fixing.

The unsafe is unfortunately used to reinterpret the shared memory between u8 and the actual sample type of i16 or f32, e.g. a [u8; 100] becomes [f32; 25].  I think I have an idea to improve the safety of this.

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

Sorry, when I say "remote peer" what I really meant was "this can be used as a sandbox escape"

Comment 3

[Matthew Gregan [:kinetik]]

Attachment: bug1512450_v0.patch

Fix potential out-of-bounds heap access by adjusting output slice length based on trusted maximum size first, then reslicing safely relying on Rust's bounds checking.

This will apply as-is to central and beta.  The code moved from lib.rs to server.rs between beta and release, so the patch will need a small rebase to apply to release if we want the fix there.

I'll upstream this into the Git repo after it's approved and landed into Gecko.

Comment 4

[Matthew Gregan [:kinetik]]

Comment on attachment 9031291 [details] [diff] [review]
bug1512450_v0.patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1434156

User impact if declined: Potential sandbox escape.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Very simple/safe change, adds an additional size check when reslicing a buffer.

String changes made/needed: 

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: Pretty obvious from the patch, but would need arbitrary code execution in the sandboxed child already before this was exploitable.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No

Which older supported branches are affected by this flaw?: release, beta

If not all supported branches, which bug introduced the flaw?: Bug 1434156

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: Applies as-is to beta, would need a trivial rebase for release if the patch is wanted there.

How likely is this patch to cause regressions; how much testing does it need?: Very simple/safe change, adds an additional size check when reslicing a buffer.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Looks like the code in question affects ESR60 also?

Comment 6

[Matthew Gregan [:kinetik]]

It does.  I can prepare a patch for that too, it's a trivial rebase.

Comment 7

[C.M.Chang[:chunmin]]

Comment on attachment 9031291 [details] [diff] [review]
bug1512450_v0.patch

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

LGTM.

I think it would be better if we add a check to make sure the data size read from CallbackResp::Data is smaller than or equal to the size of real_output we have.

::: media/audioipc/server/src/server.rs
@@ +107,5 @@
>  
>          match r {
>              Ok(CallbackResp::Data(frames)) => {
>                  if frames >= 0 {
>                      let nbytes = frames as usize * self.output_frame_size as usize;

Should we add an assertion to check the nbytes is less than or equal to the bytes of real_output (or frames is less than output.len()) ?

Comment 8

[Matthew Gregan [:kinetik]]

(In reply to Chun-Min Chang[:chunmin] from comment #7)
> I think it would be better if we add a check to make sure the data size read
> from CallbackResp::Data is smaller than or equal to the size of real_output
> we have.
> 
> Should we add an assertion to check the nbytes is less than or equal to the
> bytes of real_output (or frames is less than output.len()) ?

Unless I've misunderstood your comment this is already covered by the (safe) reslice: |&mut real_output[..nbytes]| will panic if nbytes is out of bounds.

Comment 9

[Matthew Gregan [:kinetik]]

Attachment: bug1512450_v0_esr60.patch

Rebased for ESR60.

Comment 10

[C.M.Chang[:chunmin]]

(In reply to Matthew Gregan [:kinetik] from comment #8)
> Unless I've misunderstood your comment this is already covered by the (safe)
> reslice: |&mut real_output[..nbytes]| will panic if nbytes is out of bounds.
Yes you're right. I was thinking in C. Rust will check that!

Comment 11

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 9031291 [details] [diff] [review]
bug1512450_v0.patch

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

::: media/audioipc/server/src/server.rs
@@ +92,5 @@
>              slice::from_raw_parts(input.as_ptr(), nbytes)
>          };
> +        let real_output = unsafe {
> +            let nbytes = output.len() * self.output_frame_size as usize;
> +            slice::from_raw_parts_mut(output.as_mut_ptr(), nbytes)

This doesn't seem right, the length computed here is _larger_ than the length of |output|. The varname |nbytes| is throwing me off because |slice::from_raw_parts_mut| does take a number of bytes, it takes a number of elements.

Comment 12

[Matthew Gregan [:kinetik]]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #11)
> This doesn't seem right, the length computed here is _larger_ than the
> length of |output|. The varname |nbytes| is throwing me off because
> |slice::from_raw_parts_mut| does take a number of bytes, it takes a number
> of elements.

I explained it incorrectly earlier - we're not converting the slice to a different type here, it's always u8.  The allocation passed in via the data_callback is sized in audio frames - the FFI shim converts it to a slice of u8 but treats the frame count as a byte count because it doesn't know the audio sample format.  Once the slice is passed to the Rust callback and we have the frame size available (output_frame_size), we rewrap the allocation as a slice with the correct size.

I can see how this is confusing!  I'm not sure why it was done this way - I think it'd be clearer to pass the raw ptr and frame count directly to the Rust callback, where it can perform a single conversion to a slice of nframes * output_frame_size.

Comment 13

[Matthew Gregan [:kinetik]]

Actually, the FFI shim *does* have this available now (it used to be stateless), so a much cleaner fix is to create the slices (via from_raw_parts) with the correct size from the outset.  I'll do this as a follow up bug after this security bug is out of the way, since it's a bit more intrusive in terms of lines of code changed.

Comment 14

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

Comment on attachment 9031291 [details] [diff] [review]
bug1512450_v0.patch

Giving sec-approval+ for trunk and beta approval. I'll let release management determine ESR60 approval but we should probably take it there.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3cf80bcaa78cf6186312d27811fa876d5b2b1b8f

Comment 16

[Alex Gaynor [:Alex_Gaynor]]

Ooooook, now I understand what's going on with the lengths.

Definitely agree that just creating the slices correctly from the start will make this code way more obvious.

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3cf80bcaa78c

Comment 18

[Julien Cristau [:jcristau]]

Comment on attachment 9031318 [details] [diff] [review]
bug1512450_v0_esr60.patch

sec-high fix in audioipc, approved for 60.5.0esr

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/46828493249a

This fix will be in 65.0b6.

Comment 20

[Alex Gaynor [:Alex_Gaynor]]

Does this also get ported over to the github repo for audioipc, or is m-c now the canonical location? I want to ensure this doesn't accidentally get overwritten.

Comment 21

[Matthew Gregan [:kinetik]]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #20)
> Does this also get ported over to the github repo for audioipc, or is m-c
> now the canonical location? I want to ensure this doesn't accidentally get
> overwritten.

https://github.com/djg/audioipc-2/commit/24befc0193601cd8972ec90aa246cc82456a1f45

I was waiting for approval before making it publicly visible in the upstream repo.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/b4e59b552ae8