Closed Bug 1512450 Opened 3 years ago Closed 3 years ago

Potential heap buffer overflow in audio-ipc

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox-esr60 65+ fixed
firefox64 --- wontfix
firefox65 + fixed
firefox66 + fixed

People

(Reporter: Alex_Gaynor, Assigned: kinetik)

Details

(Keywords: csectype-bounds, sec-high, Whiteboard: [post-critsmash-triage][adv-main65+][adv-esr60.5+])

Attachments

(2 files)

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?
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.
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
Sorry, when I say "remote peer" what I really meant was "this can be used as a sandbox escape"
Keywords: sec-high
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.
Attachment #9031291 - Flags: review?(cchang)
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.
Attachment #9031291 - Flags: sec-approval?
Attachment #9031291 - Flags: approval-mozilla-beta?
Looks like the code in question affects ESR60 also?
Flags: needinfo?(kinetik)
It does.  I can prepare a patch for that too, it's a trivial rebase.
Flags: needinfo?(kinetik)
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()) ?
Attachment #9031291 - Flags: review?(cchang) → review+
(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.
Rebased for ESR60.
Attachment #9031318 - Flags: review+
Attachment #9031318 - Flags: approval-mozilla-esr60?
(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 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.
(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.
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 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.
Attachment #9031291 - Flags: sec-approval?
Attachment #9031291 - Flags: sec-approval+
Attachment #9031291 - Flags: approval-mozilla-beta?
Attachment #9031291 - Flags: approval-mozilla-beta+
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.
https://hg.mozilla.org/mozilla-central/rev/3cf80bcaa78c
Group: media-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Comment on attachment 9031318 [details] [diff] [review]
bug1512450_v0_esr60.patch

sec-high fix in audioipc, approved for 60.5.0esr
Attachment #9031318 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
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.
(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.
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main65+][adv-esr60.5+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.