Closed Bug 1540897 Opened 4 years ago Closed 4 years ago

webrender_bindings::make_slice_mut should be marked unsafe

Categories

(Core :: Graphics: WebRender, defect)

55 Branch
All
Unspecified
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox-esr60 --- unaffected
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- fixed

People

(Reporter: barret, Assigned: Alex_Gaynor)

Details

(Keywords: sec-audit, Whiteboard: [post-critsmash-triage][adv-main68-])

Attachments

(1 file)

The function make_slice_mut in webrender_bindings has an unsafe API exposed safely, which could be a cause of security bugs. This function calls slice::from_raw_parts with caller-supplied inputs, which is not guaranteed safe.

In fact, make_slice_mut imposes two contracts on the caller:

  1. ptr must point to valid memory (it only checks for null pointers)
  2. len must be less than or equal to the size of memory allocated at ptr

Functions that require these contracts to be enforced cannot be safe:

fn break_contract_1() {
    let mut v = vec![0; 10];
    let p = v.as_mut_slice().as_mut_ptr();
    drop(v); // p now points to unallocated memory

    let slice = make_slice_mut(p, 10);
    // we now have a slice pointing to unallocated memory.
}

fn break_contract_2() {
    let mut v: Vec<i32> = vec![];
    let p = v.as_slice().as_mut();

    let slice = make_slice_mut(p, 1000000);
    // we now have a slice pointing past allocated memory.
}

I don't know of any usage of this function that currently can be exploited, but I'm filing this as a sec bug just to be safe.

This same applies to the non-mut variant.

MutByteSlice::new() does not look safe -- right now the lifetime of the slice in new() is only required to live for the length of new, but it should really be required to live for the lifetime of the returned object. MutByteSlice should take a lifetime parameter and bind the argument to that. Same with ByteSlice.

It looks like all the callers to these functions are right after the boundary of an extern "C" function, so hopefully they're fine.

Keywords: sec-audit
Type: defect
Keywords: checkin-needed

Hey :Alex_Gaynor, according to our security bug approval process and since this has been in m-c since 55, shouldn't this bug get audited for sec-{low,moderate,...} rating before asking for checkin? If not, why not? This is my first time dealing with a security bug and I'd like to learn.

Flags: needinfo?(agaynor)
Assignee: nobody → agaynor
Group: core-security → gfx-core-security

Yup, security bugs need a reading before they can get landed. I reviewed this and marked it as a sec-audit, meaning it was reviewing for a vulnerability (and doing some hardening as a result), but there wasn't actually a vulnerability of any severity here.

Flags: needinfo?(agaynor)
Group: gfx-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

Is this something we should consider backporting to 67 or can this ride the trains? Note that it'll need a rebased patch if the answer is yes to uplifting.

Flags: needinfo?(agaynor)

I think it's fine for this to just ride the trains -- there was no vulnerabilities here in the end, just some cleanups to hopefully prevent them going forward.

Flags: needinfo?(agaynor)
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main68-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.