From 73562af36879623bb31ce63a80f744818926f105 Mon Sep 17 00:00:00 2001 From: David Herman Date: Sat, 18 Sep 2021 13:40:50 -0700 Subject: [PATCH 1/5] Make it harder to forge `Throw` tokens - added a private field to the type - changed the test case that forges a `Throw` token to be much more devious (since it's so much harder to forge now) --- src/context/mod.rs | 2 +- src/object/class/internal.rs | 2 +- src/object/class/mod.rs | 6 ++-- src/object/mod.rs | 4 +-- src/result/mod.rs | 2 +- src/types/error.rs | 2 +- src/types/mod.rs | 2 +- test/dynamic/native/src/js/functions.rs | 41 ++++++++++++++++++++++++- 8 files changed, 50 insertions(+), 11 deletions(-) diff --git a/src/context/mod.rs b/src/context/mod.rs index 39c99da1b..34f578d03 100644 --- a/src/context/mod.rs +++ b/src/context/mod.rs @@ -494,7 +494,7 @@ pub trait Context<'a>: ContextInternal<'a> { unsafe { neon_runtime::error::throw(self.env().to_raw(), v.to_raw()); } - Err(Throw) + Err(Throw(())) } /// Creates a direct instance of the [`Error`](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Error) class. diff --git a/src/object/class/internal.rs b/src/object/class/internal.rs index f1dea4c58..9a0e0b2be 100644 --- a/src/object/class/internal.rs +++ b/src/object/class/internal.rs @@ -65,7 +65,7 @@ impl ConstructorCallCallback { ); } } - Err(Throw) + Err(Throw(())) } ConstructorCallCallback(callback::) diff --git a/src/object/class/mod.rs b/src/object/class/mod.rs index dff27a59e..42c94f582 100644 --- a/src/object/class/mod.rs +++ b/src/object/class/mod.rs @@ -168,7 +168,7 @@ pub(crate) trait ClassInternal: Class { ); if metadata_pointer.is_null() { - return Err(Throw); + return Err(Throw(())); } // NOTE: None of the error cases below need to delete the ClassMetadata object, since the @@ -181,7 +181,7 @@ pub(crate) trait ClassInternal: Class { class_name.as_ptr(), class_name.len() as u32, ) { - return Err(Throw); + return Err(Throw(())); } for (name, method) in descriptor.methods { @@ -196,7 +196,7 @@ pub(crate) trait ClassInternal: Class { name.len() as u32, method.to_raw(), ) { - return Err(Throw); + return Err(Throw(())); } } diff --git a/src/object/mod.rs b/src/object/mod.rs index cbc47e1ed..d330df0e3 100644 --- a/src/object/mod.rs +++ b/src/object/mod.rs @@ -115,7 +115,7 @@ mod traits { if unsafe { key.set_from(&mut result, self.to_raw(), val.to_raw()) } { Ok(result) } else { - Err(Throw) + Err(Throw(())) } } } @@ -265,7 +265,7 @@ mod traits { if unsafe { key.set_from(cx, &mut result, self.to_raw(), val.to_raw()) } { Ok(result) } else { - Err(Throw) + Err(Throw(())) } } diff --git a/src/result/mod.rs b/src/result/mod.rs index 0a8a72885..ce0f2bed7 100644 --- a/src/result/mod.rs +++ b/src/result/mod.rs @@ -47,7 +47,7 @@ use std::fmt::{Display, Formatter, Result as FmtResult}; /// /// [unit]: https://doc.rust-lang.org/book/ch05-01-defining-structs.html#unit-like-structs-without-any-fields #[derive(Debug)] -pub struct Throw; +pub struct Throw(pub(crate) ()); impl Display for Throw { fn fmt(&self, fmt: &mut Formatter) -> FmtResult { diff --git a/src/types/error.rs b/src/types/error.rs index 3b89589af..dd5947550 100644 --- a/src/types/error.rs +++ b/src/types/error.rs @@ -98,7 +98,7 @@ pub(crate) fn convert_panics NeonResult>( #[cfg(feature = "napi-1")] neon_runtime::error::clear_exception(env.to_raw()); neon_runtime::error::throw_error_from_utf8(env.to_raw(), data, len); - Err(Throw) + Err(Throw(())) } } } diff --git a/src/types/mod.rs b/src/types/mod.rs index 82ade11c9..26c241de8 100644 --- a/src/types/mod.rs +++ b/src/types/mod.rs @@ -117,7 +117,7 @@ pub(crate) fn build<'a, T: Managed, F: FnOnce(&mut raw::Local) -> bool>( if init(&mut local) { Ok(Handle::new_internal(T::from_raw(env, local))) } else { - Err(Throw) + Err(Throw(())) } } } diff --git a/test/dynamic/native/src/js/functions.rs b/test/dynamic/native/src/js/functions.rs index cafc80715..cbc7755ac 100644 --- a/test/dynamic/native/src/js/functions.rs +++ b/test/dynamic/native/src/js/functions.rs @@ -1,3 +1,5 @@ +use std::cell::RefCell; + use neon::object::This; use neon::prelude::*; use neon::result::Throw; @@ -143,8 +145,45 @@ pub fn panic_and_catch(mut cx: FunctionContext) -> JsResult { Ok(cx.try_catch(|_| panic!("oh no")).unwrap_or_else(|err| err)) } +thread_local! { + static FORGED_THROW: RefCell> = RefCell::new(None); +} + +fn forge_throw(mut cx: FunctionContext) -> JsResult { + // Force a random JS error to temporarily enter the throwing state. + let v: Handle = cx.null().upcast(); + match v.downcast_or_throw::(&mut cx) { + Ok(_) => panic!("failed to forge throw"), + Err(throw) => { + // Save the throw token in thread-local storage. + FORGED_THROW.with(|forged_throw| { + *forged_throw.borrow_mut() = Some(throw); + }) + } + } + panic!("get us out of here"); +} + pub fn unexpected_throw_and_catch(mut cx: FunctionContext) -> JsResult { - Ok(cx.try_catch(|_| Err(Throw)).unwrap_or_else(|err| err)) + // It's hard to forge a bogus throw token, but you can still do it by + // forcing a real throw, saving the token for later, and catching the + // throw to return the VM back out of its throwing state. + let _ = cx.try_catch::<(), _>(|cx| { + let forge: Handle = JsFunction::new(cx, forge_throw)?; + let null: Handle = cx.null().upcast(); + let args: Vec> = vec![]; + forge.call(cx, null, args)?; + Ok(()) + }); + + let retrieved_throw = FORGED_THROW.with(|forged_throw| { + forged_throw.borrow_mut().take() + }); + + match retrieved_throw { + Some(throw) => Ok(cx.try_catch(|_| Err(throw)).unwrap_or_else(|err| err)), + None => Ok(cx.string("failed to retrieve forged throw").upcast()), + } } pub fn downcast_error(mut cx: FunctionContext) -> JsResult { From eeeaf0694a01845d8a1ddd30f9eaafff51b352b3 Mon Sep 17 00:00:00 2001 From: David Herman Date: Sat, 18 Sep 2021 15:24:52 -0700 Subject: [PATCH 2/5] Kill unnecessary turbofish, and fix a doc test --- src/types/mod.rs | 2 +- test/dynamic/native/src/js/functions.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/types/mod.rs b/src/types/mod.rs index 26c241de8..50493c779 100644 --- a/src/types/mod.rs +++ b/src/types/mod.rs @@ -45,7 +45,7 @@ //! cx: &mut impl Context<'a>, //! object: Handle<'a, JsObject> //! ) -> JsResult<'a, JsArray> { -//! object.downcast(cx).or_throw(cx) +//! object.downcast().or_throw(cx) //! } //! # } //! ``` diff --git a/test/dynamic/native/src/js/functions.rs b/test/dynamic/native/src/js/functions.rs index cbc7755ac..1cb73fcb8 100644 --- a/test/dynamic/native/src/js/functions.rs +++ b/test/dynamic/native/src/js/functions.rs @@ -168,7 +168,7 @@ pub fn unexpected_throw_and_catch(mut cx: FunctionContext) -> JsResult // It's hard to forge a bogus throw token, but you can still do it by // forcing a real throw, saving the token for later, and catching the // throw to return the VM back out of its throwing state. - let _ = cx.try_catch::<(), _>(|cx| { + let _ = cx.try_catch(|cx| { let forge: Handle = JsFunction::new(cx, forge_throw)?; let null: Handle = cx.null().upcast(); let args: Vec> = vec![]; From 9c054af52e989107bff1bd8a70d5e64bf302f42a Mon Sep 17 00:00:00 2001 From: David Herman Date: Sun, 19 Sep 2021 00:29:33 -0700 Subject: [PATCH 3/5] Fix rustfmt errors and doctest issue with legacy/N-API incompatible change. --- src/types/mod.rs | 9 ++++++++- test/dynamic/native/src/js/functions.rs | 4 +--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/types/mod.rs b/src/types/mod.rs index 50493c779..e60064726 100644 --- a/src/types/mod.rs +++ b/src/types/mod.rs @@ -45,7 +45,14 @@ //! cx: &mut impl Context<'a>, //! object: Handle<'a, JsObject> //! ) -> JsResult<'a, JsArray> { -//! object.downcast().or_throw(cx) +//! # #[cfg(feature = "legacy-runtime")] +//! # return +//! # object.downcast().or_throw(cx) +//! # ; +//! # #[cfg(feature = "napi-1")] +//! # return +//! object.downcast(cx).or_throw(cx) +//! # ; //! } //! # } //! ``` diff --git a/test/dynamic/native/src/js/functions.rs b/test/dynamic/native/src/js/functions.rs index 1cb73fcb8..f3dd88b6c 100644 --- a/test/dynamic/native/src/js/functions.rs +++ b/test/dynamic/native/src/js/functions.rs @@ -176,9 +176,7 @@ pub fn unexpected_throw_and_catch(mut cx: FunctionContext) -> JsResult Ok(()) }); - let retrieved_throw = FORGED_THROW.with(|forged_throw| { - forged_throw.borrow_mut().take() - }); + let retrieved_throw = FORGED_THROW.with(|forged_throw| forged_throw.borrow_mut().take()); match retrieved_throw { Some(throw) => Ok(cx.try_catch(|_| Err(throw)).unwrap_or_else(|err| err)), From 91075699f33ac1e212917600cec9770da597c6d3 Mon Sep 17 00:00:00 2001 From: David Herman Date: Wed, 17 Nov 2021 09:13:17 -0800 Subject: [PATCH 4/5] Address review suggestions: - use private phantom !Send+!Sync data to make Throw even slightly more secure - use a crate-private constructor method to clean up construction code throughout the crate - simpler logic for forge test --- src/context/mod.rs | 2 +- src/object/class/internal.rs | 2 +- src/object/class/mod.rs | 6 +++--- src/object/mod.rs | 4 ++-- src/result/mod.rs | 9 ++++++++- src/types/error.rs | 2 +- src/types/mod.rs | 2 +- test/dynamic/native/src/js/functions.rs | 17 ++++++----------- 8 files changed, 23 insertions(+), 21 deletions(-) diff --git a/src/context/mod.rs b/src/context/mod.rs index 34f578d03..a5491439b 100644 --- a/src/context/mod.rs +++ b/src/context/mod.rs @@ -494,7 +494,7 @@ pub trait Context<'a>: ContextInternal<'a> { unsafe { neon_runtime::error::throw(self.env().to_raw(), v.to_raw()); } - Err(Throw(())) + Err(Throw::new()) } /// Creates a direct instance of the [`Error`](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Error) class. diff --git a/src/object/class/internal.rs b/src/object/class/internal.rs index 9a0e0b2be..7dea4f39f 100644 --- a/src/object/class/internal.rs +++ b/src/object/class/internal.rs @@ -65,7 +65,7 @@ impl ConstructorCallCallback { ); } } - Err(Throw(())) + Err(Throw::new()) } ConstructorCallCallback(callback::) diff --git a/src/object/class/mod.rs b/src/object/class/mod.rs index 42c94f582..be8b086da 100644 --- a/src/object/class/mod.rs +++ b/src/object/class/mod.rs @@ -168,7 +168,7 @@ pub(crate) trait ClassInternal: Class { ); if metadata_pointer.is_null() { - return Err(Throw(())); + return Err(Throw::new()); } // NOTE: None of the error cases below need to delete the ClassMetadata object, since the @@ -181,7 +181,7 @@ pub(crate) trait ClassInternal: Class { class_name.as_ptr(), class_name.len() as u32, ) { - return Err(Throw(())); + return Err(Throw::new()); } for (name, method) in descriptor.methods { @@ -196,7 +196,7 @@ pub(crate) trait ClassInternal: Class { name.len() as u32, method.to_raw(), ) { - return Err(Throw(())); + return Err(Throw::new()); } } diff --git a/src/object/mod.rs b/src/object/mod.rs index d330df0e3..8d8aafebd 100644 --- a/src/object/mod.rs +++ b/src/object/mod.rs @@ -115,7 +115,7 @@ mod traits { if unsafe { key.set_from(&mut result, self.to_raw(), val.to_raw()) } { Ok(result) } else { - Err(Throw(())) + Err(Throw::new()) } } } @@ -265,7 +265,7 @@ mod traits { if unsafe { key.set_from(cx, &mut result, self.to_raw(), val.to_raw()) } { Ok(result) } else { - Err(Throw(())) + Err(Throw::new()) } } diff --git a/src/result/mod.rs b/src/result/mod.rs index ce0f2bed7..7573edfbc 100644 --- a/src/result/mod.rs +++ b/src/result/mod.rs @@ -37,6 +37,7 @@ use crate::context::Context; use crate::handle::Handle; use crate::types::Value; use std::fmt::{Display, Formatter, Result as FmtResult}; +use std::marker::PhantomData; /// A [unit type][unit] indicating that the JavaScript thread is throwing an exception. /// @@ -47,7 +48,13 @@ use std::fmt::{Display, Formatter, Result as FmtResult}; /// /// [unit]: https://doc.rust-lang.org/book/ch05-01-defining-structs.html#unit-like-structs-without-any-fields #[derive(Debug)] -pub struct Throw(pub(crate) ()); +pub struct Throw(PhantomData<*mut ()>); // *mut is !Send + !Sync, making it harder to forge + +impl Throw { + pub(crate) fn new() -> Self { + Self(PhantomData) + } +} impl Display for Throw { fn fmt(&self, fmt: &mut Formatter) -> FmtResult { diff --git a/src/types/error.rs b/src/types/error.rs index dd5947550..a0b7164fa 100644 --- a/src/types/error.rs +++ b/src/types/error.rs @@ -98,7 +98,7 @@ pub(crate) fn convert_panics NeonResult>( #[cfg(feature = "napi-1")] neon_runtime::error::clear_exception(env.to_raw()); neon_runtime::error::throw_error_from_utf8(env.to_raw(), data, len); - Err(Throw(())) + Err(Throw::new()) } } } diff --git a/src/types/mod.rs b/src/types/mod.rs index e60064726..ba779f589 100644 --- a/src/types/mod.rs +++ b/src/types/mod.rs @@ -124,7 +124,7 @@ pub(crate) fn build<'a, T: Managed, F: FnOnce(&mut raw::Local) -> bool>( if init(&mut local) { Ok(Handle::new_internal(T::from_raw(env, local))) } else { - Err(Throw(())) + Err(Throw::new()) } } } diff --git a/test/dynamic/native/src/js/functions.rs b/test/dynamic/native/src/js/functions.rs index f3dd88b6c..41d1efce3 100644 --- a/test/dynamic/native/src/js/functions.rs +++ b/test/dynamic/native/src/js/functions.rs @@ -150,17 +150,12 @@ thread_local! { } fn forge_throw(mut cx: FunctionContext) -> JsResult { - // Force a random JS error to temporarily enter the throwing state. - let v: Handle = cx.null().upcast(); - match v.downcast_or_throw::(&mut cx) { - Ok(_) => panic!("failed to forge throw"), - Err(throw) => { - // Save the throw token in thread-local storage. - FORGED_THROW.with(|forged_throw| { - *forged_throw.borrow_mut() = Some(throw); - }) - } - } + // Force an arbitrary JS error to temporarily enter the throwing state. + let throw = cx.throw_error::<_, ()>("forced error").unwrap_err(); + // Save the throw token in thread-local storage. + FORGED_THROW.with(|forged_throw| { + *forged_throw.borrow_mut() = Some(throw); + }); panic!("get us out of here"); } From 074d0e226dcbfd121b950a587352246b9ba071ba Mon Sep 17 00:00:00 2001 From: David Herman Date: Thu, 3 Mar 2022 15:24:50 -0800 Subject: [PATCH 5/5] Tighten language in comment based on review suggestion: mut* makes `Throw` harder to store --- src/result/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/result/mod.rs b/src/result/mod.rs index 7573edfbc..7a2675609 100644 --- a/src/result/mod.rs +++ b/src/result/mod.rs @@ -48,7 +48,7 @@ use std::marker::PhantomData; /// /// [unit]: https://doc.rust-lang.org/book/ch05-01-defining-structs.html#unit-like-structs-without-any-fields #[derive(Debug)] -pub struct Throw(PhantomData<*mut ()>); // *mut is !Send + !Sync, making it harder to forge +pub struct Throw(PhantomData<*mut ()>); // *mut is !Send + !Sync, making it harder to accidentally store impl Throw { pub(crate) fn new() -> Self {