webrender_bindings::make_slice_mut should be marked unsafe
Categories
(Core :: Graphics: WebRender, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox66 | --- | wontfix |
firefox67 | --- | wontfix |
firefox68 | --- | fixed |
People
(Reporter: beth, 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:
ptr
must point to valid memory (it only checks for null pointers)len
must be less than or equal to the size of memory allocated atptr
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.
Assignee | ||
Comment 1•6 years ago
|
||
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.
Assignee | ||
Comment 2•6 years ago
|
||
Reporter | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Reporter | ||
Comment 3•6 years ago
|
||
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.
Updated•6 years ago
|
Assignee | ||
Comment 4•6 years ago
|
||
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•6 years ago
|
||
![]() |
||
Comment 6•6 years ago
|
||
Comment 7•6 years ago
|
||
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.
Assignee | ||
Comment 8•6 years ago
|
||
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.
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•5 years ago
|
Description
•