Skip to content

Commit 11b0bb9

Browse files
committed
fix(runtime): harden marshaling, release logging and remote loading
Marshaling (both the classic and napi engines): - A pointer comes only from an External, a BigInt or null. A Number or string `handle` or `__native_ptr` is refused, since JSON can carry those: `{"handle": 1094795585}` passed to an Object parameter used to be AddRef'd as a COM pointer. - Interface and class parameters QueryInterface every pointer, boxed primitives included, and no longer fall back to the raw pointer when that fails, so a string passed as a Uri can't be called through the wrong vtable. Delegate parameters accept JS delegates the runtime made and QueryInterface anything else; GenericMethodCall fails instead of calling a slot on the wrong interface. - Shapes the call layout can't express (non-byte arrays, received arrays, array returns, structs larger than the return buffer, JS delegates taking arrays) fail at construction instead of corrupting the libffi call. - Struct buffers must cover the struct, serialized structs are padded to their ABI size, and IReference<T> struct fields are pointer-sized. - Buffer arguments are rechecked before the call, since parsing a later argument can run JS that detaches or resizes them. - Only two-field wrappers are read as a DeclarationFFI, and URL accessors check their receiver, so a URL passed to WinRT or `instanceof` is no longer read as the wrong type. Release builds keep app console output out of the Windows Event Log and the on-disk trace log, and the trace log no longer falls back to a hardcoded user directory. Remote modules: allowlist entries match by origin and path boundary rather than string prefix, the response's final URL must also pass the gate after redirects, the gate reads the entry's package.json for script entries too, and NSWinRT.dynamicImport goes through the native loader. WebSocket subprotocols must be HTTP tokens and a message is capped at 100 MiB. Also: crypto.getRandomValues and crypto.randomUUID backed by the OS RNG, and NSWinRT.win32.bind loads a bare DLL name from the app directory and System32 only.
1 parent 7f82c69 commit 11b0bb9

24 files changed

Lines changed: 1004 additions & 395 deletions

‎packages/test/security-check.js‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
// Standalone-engine check that untrusted-shaped values can't reach native code as pointers or
2+
// mistyped objects, and that the safe-looking APIs handle views, NULs and randomness correctly:
3+
// nativescript-windows packages/test/security-check.js
4+
// Prints "security-check: N passed, M failed"; throws (nonzero exit) on any failure.
5+
'use strict';
6+
7+
var pass = 0, fail = 0;
8+
function check(name, ok) {
9+
if (ok) { pass += 1; }
10+
else { fail += 1; console.error('FAIL ' + name); }
11+
}
12+
function throws(fn) {
13+
try { fn(); } catch (_) { return true; }
14+
return false;
15+
}
16+
17+
// JSON can carry numbers and strings, which must never be read as native pointers.
18+
var ps = new Windows.Foundation.Collections.PropertySet();
19+
ps.Insert('ok', 'a string boxes fine');
20+
check('string boxes', ps.Lookup('ok') === 'a string boxes fine');
21+
['{"handle": 1094795585}', '{"handle": "0x41414141"}', '{"__native_ptr": "0x4141414141"}',
22+
'{"__native_ptr": 1094795585}'].forEach(function (json) {
23+
check('rejects ' + json, throws(function () { ps.Insert('k', JSON.parse(json)); }));
24+
});
25+
26+
// Interface parameters only take objects implementing the interface.
27+
var client = new Windows.Web.Http.HttpClient();
28+
client.GetStringAsync(new Windows.Foundation.Uri('https://example.invalid/')).Cancel();
29+
check('string is not a Uri', throws(function () { client.GetStringAsync('https://example.com'); }));
30+
check('number is not a Uri', throws(function () { client.GetStringAsync(42); }));
31+
check('PropertySet is not a Uri', throws(function () { client.GetStringAsync(new Windows.Foundation.Collections.PropertySet()); }));
32+
33+
// Struct buffers must cover the struct.
34+
var PV = Windows.Foundation.PropertyValue;
35+
check('8-byte Point accepted', !throws(function () { PV.CreatePoint(new Float32Array([1, 2])); }));
36+
check('2-byte Point rejected', throws(function () { PV.CreatePoint(new Uint8Array(2)); }));
37+
38+
// Delegate parameters only take delegates.
39+
var pool = Windows.System.Threading.ThreadPool;
40+
check('function delegate', !throws(function () { pool.RunAsync(function () {}); }));
41+
check('asDelegate handle', !throws(function () {
42+
pool.RunAsync(NSWinRT.asDelegate('Windows.System.Threading.WorkItemHandler', function () {}));
43+
}));
44+
check('PropertySet is not a delegate', throws(function () { pool.RunAsync(new Windows.Foundation.Collections.PropertySet()); }));
45+
46+
// A view's bytes start at its byteOffset.
47+
var CB = Windows.Security.Cryptography.CryptographicBuffer;
48+
var bytes = new Uint8Array([1, 2, 3, 4, 5, 6, 7, 8]);
49+
var hex = CB.EncodeToHexString(CB.CreateFromByteArray(bytes.subarray(4)));
50+
check('view offset honored (' + hex + ')', hex === '05060708');
51+
52+
// Strings keep embedded NULs.
53+
var withNul = Windows.Data.Json.JsonValue.CreateStringValue('a\u0000b').GetString();
54+
check('embedded NUL kept', withNul.length === 3);
55+
56+
// Web Crypto randomness.
57+
var r1 = crypto.getRandomValues(new Uint8Array(32));
58+
var r2 = crypto.getRandomValues(new Uint8Array(32));
59+
check('random fills differ', r1.some(function (v, i) { return v !== r2[i]; }));
60+
check('float array rejected', throws(function () { crypto.getRandomValues(new Float32Array(2)); }));
61+
check('quota enforced', throws(function () { crypto.getRandomValues(new Uint8Array(65537)); }));
62+
check('randomUUID v4', /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/.test(crypto.randomUUID()));
63+
64+
console.log('security-check: ' + pass + ' passed, ' + fail + ' failed');
65+
if (fail) { throw new Error('security-check: ' + fail + ' failed'); }

‎runtime/src/esm_http.rs‎

Lines changed: 54 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -258,13 +258,27 @@ pub(crate) fn mark_keys_for_cache_bust(keys: &[String]) {
258258

259259
/// Remote-module gate: debug hosts (`set_debug_build`) always allow; release builds need
260260
/// `security.allowRemoteModules` in the app's package.json, then an allowlist match
261-
/// (`security.remoteModuleAllowlist` URL prefixes; empty allows every URL).
261+
/// (`security.remoteModuleAllowlist` URLs; empty allows every URL).
262262
pub(crate) fn remote_url_allowed(url: &str) -> bool {
263263
if DEBUG_BUILD.load(std::sync::atomic::Ordering::Relaxed) || cfg!(test) {
264264
return true;
265265
}
266266
let (allowed, allowlist) = SECURITY.get().cloned().unwrap_or((false, Vec::new()));
267-
allowed && (allowlist.is_empty() || allowlist.iter().any(|p| url.starts_with(p.as_str())))
267+
allowed && (allowlist.is_empty() || allowlist.iter().any(|entry| allowlist_matches(url, entry)))
268+
}
269+
270+
/// An allowlist entry admits URLs with its scheme, host and port whose path is the entry's path
271+
/// or lies under it, so `https://cdn.example.com` doesn't admit `https://cdn.example.com.evil`.
272+
pub(crate) fn allowlist_matches(url: &str, entry: &str) -> bool {
273+
let (Ok(url), Ok(entry)) = (url::Url::parse(url), url::Url::parse(entry)) else {
274+
return false;
275+
};
276+
if url.origin() != entry.origin() {
277+
return false;
278+
}
279+
let prefix = entry.path().trim_end_matches('/');
280+
let path = url.path();
281+
path == prefix || (path.starts_with(prefix) && path[prefix.len()..].starts_with('/'))
268282
}
269283

270284
static SECURITY: OnceLock<(bool, Vec<String>)> = OnceLock::new();
@@ -277,6 +291,11 @@ pub fn set_debug_build(debug: bool) {
277291
DEBUG_BUILD.store(debug, std::sync::atomic::Ordering::Relaxed);
278292
}
279293

294+
/// Whether the host marked itself as a debug build via [`set_debug_build`].
295+
pub(crate) fn is_debug_build() -> bool {
296+
DEBUG_BUILD.load(std::sync::atomic::Ordering::Relaxed)
297+
}
298+
280299
/// Read the release-build security settings from the app's package.json, once.
281300
pub(crate) fn init_security_from_app_dir(app_dir: &std::path::Path) {
282301
let _ = SECURITY.get_or_init(|| {
@@ -340,6 +359,13 @@ pub(crate) fn fetch_module(url: &str, key: &str) -> Result<FetchedModule, String
340359
}
341360
let response = fetch_once(url, key).or_else(|_| fetch_once(url, key))
342361
.map_err(|e| format!("HTTP import failed: {url} ({e})"))?;
362+
// WinHTTP follows redirects, so the gate must also admit where the response came from.
363+
if !remote_url_allowed(&response.final_url) {
364+
return Err(format!(
365+
"HTTP import blocked: {url} redirected to {}, which is not allowed",
366+
response.final_url
367+
));
368+
}
343369
let status = response.status;
344370
if status == 204 || status == 205 {
345371
return Err(format!("HTTP import failed: {url} (status={status}, no content)"));
@@ -403,6 +429,32 @@ pub(crate) fn json_module_source(body: &str) -> String {
403429
mod tests {
404430
use super::*;
405431

432+
#[test]
433+
fn allowlist_matches_origin_and_path_boundary() {
434+
let cdn = "https://cdn.example.com";
435+
assert!(allowlist_matches("https://cdn.example.com/a.js", cdn));
436+
assert!(allowlist_matches("https://cdn.example.com:443/a.js", cdn));
437+
assert!(!allowlist_matches("https://cdn.example.com.evil.net/a.js", cdn));
438+
assert!(!allowlist_matches("https://cdn.example.com@evil.net/a.js", cdn));
439+
assert!(!allowlist_matches("http://cdn.example.com/a.js", cdn));
440+
assert!(!allowlist_matches("https://cdn.example.com:8443/a.js", cdn));
441+
let lib = "https://cdn.example.com/lib/";
442+
assert!(allowlist_matches("https://cdn.example.com/lib/a.js", lib));
443+
assert!(allowlist_matches("https://cdn.example.com/lib", lib));
444+
assert!(!allowlist_matches("https://cdn.example.com/library.js", "https://cdn.example.com/lib"));
445+
assert!(!allowlist_matches("https://cdn.example.com/lib/../evil.js", lib));
446+
assert!(!allowlist_matches("", lib));
447+
}
448+
449+
#[test]
450+
fn websocket_subprotocols_must_be_tokens() {
451+
assert!(crate::winhttp::is_http_token("graphql-ws"));
452+
assert!(crate::winhttp::is_http_token("v1.chat"));
453+
assert!(!crate::winhttp::is_http_token(""));
454+
assert!(!crate::winhttp::is_http_token("a\r\nX-Injected: 1"));
455+
assert!(!crate::winhttp::is_http_token("a, b"));
456+
}
457+
406458
#[test]
407459
fn canonical_keys_follow_the_configured_vocabulary() {
408460
clear_thread_vocabulary();

‎runtime/src/esm_http_test.rs‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -294,3 +294,19 @@ fn native_websocket_round_trip() {
294294
);
295295
assert_eq!(refused, "ok:1006:false");
296296
}
297+
298+
#[test]
299+
fn legacy_dynamic_import_uses_the_module_loader() {
300+
let server = TestServer::start();
301+
let o = server.origin.clone();
302+
server.route("/remote/mod.js", "text/javascript", "export default 'remote'; export const n = 2;");
303+
server.route("/remote/style.css", "text/css", "body{}");
304+
305+
let mut runtime = Runtime::new(".");
306+
runtime.register_delegate_isolate_ptr();
307+
let got = await_js(&mut runtime, &format!("NSWinRT.dynamicImport('{o}/remote/mod.js').then(m => m.default + m.n)"));
308+
assert_eq!(got, "ok:remote2");
309+
// The loader's MIME check applies too.
310+
let css = await_js(&mut runtime, &format!("NSWinRT.dynamicImport('{o}/remote/style.css')"));
311+
assert!(css.starts_with("err:"), "{css}");
312+
}

‎runtime/src/generic_method_call.rs‎

Lines changed: 40 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -44,9 +44,14 @@ pub struct GenericMethodCall {
4444
/// Pre-computed interface IIDs for pointer-typed parameters (one per parse param, None if no QI needed).
4545
parameter_arg_iids: Vec<Option<GUID>>,
4646
/// Scratch buffer for value-type returns (same role as in MethodCall).
47-
return_value_buf: [u8; 128],
47+
return_value_buf: [u8; crate::helpers::RETURN_BUFFER_SIZE],
48+
/// Set when the call can't be made safely; `call` then fails without touching the vtable.
49+
init_error: Option<HRESULT>,
4850
}
4951

52+
const E_NOINTERFACE: HRESULT = HRESULT(0x8000_4002u32 as i32);
53+
const E_NOTIMPL: HRESULT = HRESULT(0x8000_4001u32 as i32);
54+
5055
#[inline]
5156
fn call_failure() -> HRESULT {
5257
// E_FAIL
@@ -93,9 +98,12 @@ impl GenericMethodCall {
9398
)
9499
};
95100

96-
// Generic interface QI is best-effort: fall back to the original interface
97-
// when the object doesn't implement the expected instantiation.
101+
// Without the interface the vtable slot belongs to some other method, so the call fails.
98102
let qi_ok = result.is_ok() && !interface_ptr.is_null();
103+
let mut init_error = (!qi_ok).then_some(E_NOINTERFACE);
104+
if !crate::helpers::return_layout_supported(&return_type) {
105+
init_error = Some(E_NOTIMPL);
106+
}
99107

100108
let is_composition = !is_sealed;
101109

@@ -126,6 +134,9 @@ impl GenericMethodCall {
126134
let metadata = parameter.metadata().unwrap();
127135

128136
let signature = Signature::to_string(metadata, &type_);
137+
if !crate::helpers::parameter_layout_supported(&signature) {
138+
init_error = Some(E_NOTIMPL);
139+
}
129140

130141
let parse_native_type = NativeType::try_from(signature.as_str());
131142
assert!(parse_native_type.is_ok());
@@ -207,19 +218,26 @@ impl GenericMethodCall {
207218
.iter()
208219
.cloned()
209220
.map(libffi::middle::Type::try_from)
210-
.collect::<std::result::Result<Vec<Type>, AnyError>>();
221+
.collect::<std::result::Result<Vec<Type>, AnyError>>()
222+
.unwrap_or_else(|_| {
223+
init_error = Some(E_NOTIMPL);
224+
Vec::new()
225+
});
211226

212-
assert!(params.is_ok());
213-
214-
let cif = Cif::new(params.unwrap(), Type::i32());
227+
let cif = Cif::new(params, Type::i32());
215228

216229
let effective_interface = if qi_ok {
217230
unsafe { IUnknown::from_raw(interface_ptr) }
218231
} else {
219232
interface.clone()
220233
};
221-
let vtable: *mut *mut c_void = unsafe { std::mem::transmute(effective_interface.vtable()) };
222-
let func = unsafe { *vtable.offset(index as isize) };
234+
let func = if qi_ok {
235+
let vtable: *mut *mut c_void =
236+
unsafe { std::mem::transmute(effective_interface.vtable()) };
237+
unsafe { *vtable.offset(index as isize) }
238+
} else {
239+
std::ptr::null_mut()
240+
};
223241

224242
Self {
225243
cif,
@@ -237,7 +255,8 @@ impl GenericMethodCall {
237255
parameters: method.parameters().to_vec(),
238256
return_type,
239257
parameter_arg_iids,
240-
return_value_buf: [0u8; 128],
258+
return_value_buf: [0u8; crate::helpers::RETURN_BUFFER_SIZE],
259+
init_error,
241260
}
242261
}
243262

@@ -246,6 +265,9 @@ impl GenericMethodCall {
246265
scope: &mut v8::PinScope<'s, '_>,
247266
args: &v8::FunctionCallbackArguments,
248267
) -> (HRESULT, *mut c_void, Vec<v8::Local<'s, v8::Value>>) {
268+
if let Some(hr) = self.init_error {
269+
return (hr, std::ptr::null_mut(), Vec::new());
270+
}
249271
let number_of_abi_parameters = self.number_of_abi_parameters;
250272
let mut arguments: Vec<NativeValue> = Vec::with_capacity(number_of_abi_parameters);
251273
// Track parse-level types for each ABI argument slot.
@@ -267,6 +289,7 @@ impl GenericMethodCall {
267289
});
268290
argument_parse_types.push(None);
269291

292+
let mut buffer_args = Vec::new();
270293
for (i, native_type) in self.parse_parameter_types.iter().enumerate() {
271294
let parameter = &self.parameters[i];
272295
let param_sig_opt = parameter
@@ -340,7 +363,7 @@ impl GenericMethodCall {
340363
Ok(pointer)
341364
}
342365
Ok((pointer, None)) => Ok(pointer),
343-
Err(_) => ffi_parse_pointer_arg(scope, value),
366+
Err(error) => Err(error),
344367
}
345368
} else {
346369
ffi_parse_pointer_arg(scope, value)
@@ -352,6 +375,7 @@ impl GenericMethodCall {
352375
Ok(value) => value,
353376
Err(_) => return (call_failure(), std::ptr::null_mut(), Vec::new()),
354377
};
378+
buffer_args.push((value, unsafe { buffer_value.pointer }, byte_length));
355379

356380
arguments.push(NativeValue {
357381
u32_value: byte_length,
@@ -362,7 +386,7 @@ impl GenericMethodCall {
362386
continue;
363387
}
364388
NativeType::Function => ffi_parse_function_arg(scope, value),
365-
NativeType::Struct(_) => ffi_parse_struct_arg(scope, value),
389+
NativeType::Struct(_) => ffi_parse_struct_arg(scope, value, native_type.size()),
366390
NativeType::String => ffi_parse_string_arg(scope, value),
367391
};
368392

@@ -430,6 +454,10 @@ impl GenericMethodCall {
430454
}
431455
}
432456

457+
if !crate::value::buffers_unchanged(scope, &buffer_args) {
458+
return (call_failure(), std::ptr::null_mut(), Vec::new());
459+
}
460+
433461
let prep = match crate::ffi::prepare_string_storage(
434462
&arguments,
435463
&self.parameter_types,

0 commit comments

Comments
 (0)