1540897 - webrender_bindings::make_slice_mut should be marked unsafe

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Barret Rennie [:barret] (they/them)]

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.

Comment 1

[Alex Gaynor [:Alex_Gaynor]]

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.

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1540897 - make the ByteSlice APIs in webrender's bindings fully sound; r?brennie

Comment 3

[Barret Rennie [:barret] (they/them)]

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.

Comment 4

[Alex Gaynor [:Alex_Gaynor]]

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.

Comment 5

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

https://hg.mozilla.org/integration/autoland/rev/75ec33249c8ff9f6ae1aca8df29ef38538bf1928

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/75ec33249c8f

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 8

[Alex Gaynor [:Alex_Gaynor]]

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.