Skip to content

/ servo

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

script: Ban `FnBox<()>` and lock down the dangerous generic `no_jsmanaged_fields!` implementations. #14443

Closed
wants to merge 1 commit into from
Closed

script: Ban `FnBox<()>` and lock down the dangerous generic `no_jsmanaged_fields!` implementations. #14443

wants to merge 1 commit into from

Conversation

@pcwalton
Copy link
Contributor

pcwalton commented Dec 3, 2016 •
edited by larsbergstrom

Closes #14416.

The basic cause for that UAF was the unsafe use of FnBox<()> in the rooting protocol for the JS engine integration. This patch fixes that bug and removes all generics and existentials from the no-op tracing, as they were all dangerous. There should be no more ways to get UAF from DOM heap rooting without writing custom trace hooks.

r? @nox

This change is Reviewable
@pcwalton
script: Ban `FnBox<()>` and lock down a bunch of dangerous
Loading status checks…
47070a6 
`no_jsmanaged_fields!` implementations.

Closes #14416.
@highfive
Copy link

highfive commented Dec 3, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/document.rs, components/script/dom/htmllinkelement.rs, components/script/dom/macros.rs, components/script/dom/bindings/weakref.rs, components/script/dom/keyboardevent.rs, components/script/dom/cssrulelist.rs, components/script/dom/crypto.rs, components/script/dom/webgltexture.rs, components/script/dom/node.rs, components/script/dom/bindings/trace.rs, components/script/lib.rs, components/script/dom/window.rs, components/script/devtools.rs
  • @fitzgen: components/script/dom/document.rs, components/script/dom/htmllinkelement.rs, components/script/dom/macros.rs, components/script/dom/bindings/weakref.rs, components/script/dom/keyboardevent.rs, components/script/dom/cssrulelist.rs, components/script/dom/crypto.rs, components/script/dom/webgltexture.rs, components/script/dom/node.rs, components/script/dom/bindings/trace.rs, components/script/lib.rs, components/script/dom/window.rs, components/script/devtools.rs
  • @emilio: components/script/dom/webgltexture.rs
@highfive
Copy link

highfive commented Dec 3, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@pcwalton pcwalton changed the title script: Ban `FnBox<()>` and lock down a bunch of dangerous `no_jsmanaged_fields!` implementations. script: Ban `FnBox<()>` and lock down the dangerous generic `no_jsmanaged_fields!` implementations. Dec 3, 2016
@samlh
samlh reviewed Dec 3, 2016
View changes
components/script/dom/bindings/trace.rs
@@ -395,6 +457,7 @@ impl<'a> JSTraceable for &'a str {
}
}

// Safe because

This comment has been minimized.

Sign in to view
Copy link
@samlh

samlh Dec 3, 2016

Contributor

Because?

This comment has been minimized.

Sign in to view
Copy link
@Manishearth

Manishearth Dec 3, 2016

Member

because.

This comment has been minimized.

Sign in to view
Copy link
@Manishearth

Manishearth Dec 3, 2016

Member

(safe because fn pointers don't contain data and thus can't contain roots)
@Manishearth
Manishearth reviewed Dec 3, 2016
View changes
components/script/dom/bindings/trace.rs
#[inline]
fn trace(&self, _: *mut JSTracer) {
// Do nothing
}
}

impl JSTraceable for () {
impl<T: Reflectable> JSTraceable for Trusted<T> {

This comment has been minimized.

Sign in to view
Copy link
@Manishearth

Manishearth Dec 3, 2016

Member

"safe because Trusted<T> is a permaroot"
@Manishearth
Manishearth reviewed Dec 3, 2016
View changes
components/script/dom/bindings/trace.rs
}
}

impl JSTraceable for Mutex<Option<SharedRt>> {

This comment has been minimized.

Sign in to view
Copy link
@Manishearth

Manishearth Dec 3, 2016

Member

feel like we should be using the macro for these so that we can later lint that the impl is on a type that exists completely outside of this crate

This comment has been minimized.

Sign in to view
Copy link
@pcwalton

pcwalton Dec 3, 2016

Author Contributor

Well, I removed the ability of the macro to parse <> entirely, just to be on the safe side. Would you prefer I add it back or is this OK?

This comment has been minimized.

Sign in to view
Copy link
@Manishearth

Manishearth Dec 3, 2016

Member

We should be able to implement it on concrete types, just not generic ones (impl<T> JSTraceable for Foo<T>). When we add the lint check it can recurse type parameters of the concrete types.
@Manishearth
Manishearth approved these changes Dec 3, 2016
View changes
Copy link
Member

Manishearth left a comment

Overall looks good
@nox
nox reviewed Dec 3, 2016
View changes
components/script/dom/bindings/trace.rs
}
}

impl JSTraceable for Matrix4D<f64> {

This comment has been minimized.

Sign in to view
Copy link
@nox

nox Dec 3, 2016

Member

Can't you just implement it for Matrix4D<T> where T: JSTraceable?

This comment has been minimized.

Sign in to view
Copy link
@pcwalton

pcwalton Dec 3, 2016 •
edited

Author Contributor

Well, then I'd have to trace all the elements of the matrix. This ends up being shorter. I can do that if you prefer though.

This comment has been minimized.

Sign in to view
Copy link
@nox

nox Dec 6, 2016

Member

Yeah no it's ok.
@nox
Copy link
Member

nox commented Dec 6, 2016

I kinda want to clean up that PR, because you did many unrelated changes in a single commit.
@pcwalton
Copy link
Contributor Author

pcwalton commented Dec 6, 2016

Please land this very soon if you do.

Remote code execution fixes must land as soon as possible. Do not bikeshed on them. Do not delay them. Just land them. It sets a very very bad precedent if we do not.
@bors-servo
Copy link
Contributor

bors-servo commented Dec 7, 2016

☔ The latest upstream changes (presumably #14473) made this pull request unmergeable. Please resolve the merge conflicts.
@nox
Copy link
Member

nox commented Dec 7, 2016

Superseded by #14473.
@nox nox closed this Dec 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samlh samlh

@nox nox

@Manishearth Manishearth

Assignees

@nox nox

Labels
A-content/bindings A-content/script A-security I-crash I-safety S-awaiting-review S-needs-rebase
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

[security] Use-after-free in run_the_animation_frame_callbacks
6 participants
@pcwalton @highfive @nox @bors-servo @samlh @Manishearth
You can’t perform that action at this time.