From 7d7080b0152143876f900febe5030bac419bb555 Mon Sep 17 00:00:00 2001 From: Marcin S Date: Mon, 21 Aug 2023 15:37:35 +0200 Subject: [PATCH 1/5] PVF worker: random fixes - Fixes possible panic due to non-UTF-8 env vars (https://github.com/paritytech/polkadot/pull/7330#discussion_r1300101716) - Very small refactor of some duplicated code --- node/core/pvf/common/src/worker/mod.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/node/core/pvf/common/src/worker/mod.rs b/node/core/pvf/common/src/worker/mod.rs index d249007ec36e..d3792651ba15 100644 --- a/node/core/pvf/common/src/worker/mod.rs +++ b/node/core/pvf/common/src/worker/mod.rs @@ -121,19 +121,18 @@ pub fn worker_event_loop( "Node and worker version mismatch, node needs restarting, forcing shutdown", ); kill_parent_node_in_emergency(); - let err: io::Result = - Err(io::Error::new(io::ErrorKind::Unsupported, "Version mismatch")); - gum::debug!(target: LOG_TARGET, %worker_pid, "quitting pvf worker({}): {:?}", debug_id, err); + let err = io::Error::new(io::ErrorKind::Unsupported, "Version mismatch"); + worker_shutdown_message(debug_id, worker_pid, err); return } } // Delete all env vars to prevent malicious code from accessing them. - for (key, _) in std::env::vars() { + for (key, _) in std::env::vars_os() { // TODO: *theoretically* the value (or mere presence) of `RUST_LOG` can be a source of // randomness for malicious code. In the future we can remove it also and log in the host; // see . - if key != "RUST_LOG" { + if key.to_str() != Some("RUST_LOG") { std::env::remove_var(key); } } @@ -152,7 +151,7 @@ pub fn worker_event_loop( // It's never `Ok` because it's `Ok(Never)`. .unwrap_err(); - gum::debug!(target: LOG_TARGET, %worker_pid, "quitting pvf worker ({}): {:?}", debug_id, err); + worker_shutdown_message(debug_id, worker_pid, err); // We don't want tokio to wait for the tasks to finish. We want to bring down the worker as fast // as possible and not wait for stalled validation to finish. This isn't strictly necessary now, @@ -160,6 +159,11 @@ pub fn worker_event_loop( rt.shutdown_background(); } +/// Provide a consistent message on worker shutdown. +fn worker_shutdown_message(debug_id: &'static str, worker_pid: u32, err: io::Error) { + gum::debug!(target: LOG_TARGET, %worker_pid, "quitting pvf worker ({}): {:?}", debug_id, err); +} + /// Loop that runs in the CPU time monitor thread on prepare and execute jobs. Continuously wakes up /// and then either blocks for the remaining CPU time, or returns if we exceed the CPU timeout. /// From 041b4439b79e325deae97fbcb3b8164b1dc0a024 Mon Sep 17 00:00:00 2001 From: Marcin S Date: Mon, 21 Aug 2023 17:38:29 +0200 Subject: [PATCH 2/5] Don't need `to_str()` for comparison between OsString and str --- node/core/pvf/common/src/worker/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/core/pvf/common/src/worker/mod.rs b/node/core/pvf/common/src/worker/mod.rs index d3792651ba15..c36f433de7f1 100644 --- a/node/core/pvf/common/src/worker/mod.rs +++ b/node/core/pvf/common/src/worker/mod.rs @@ -132,7 +132,7 @@ pub fn worker_event_loop( // TODO: *theoretically* the value (or mere presence) of `RUST_LOG` can be a source of // randomness for malicious code. In the future we can remove it also and log in the host; // see . - if key.to_str() != Some("RUST_LOG") { + if key != "RUST_LOG" { std::env::remove_var(key); } } From 255aa9c60bf664e42f2ac6d2dccaaaa53c22ab68 Mon Sep 17 00:00:00 2001 From: Marcin S Date: Mon, 21 Aug 2023 17:55:18 +0200 Subject: [PATCH 3/5] Check edge cases that can cause env::remove_var to panic In case of a key or value that would cause env::remove_var to panic, we first log a warning and then proceed to attempt to remove the env var. --- node/core/pvf/common/src/worker/mod.rs | 51 +++++++++++++++++++++----- 1 file changed, 42 insertions(+), 9 deletions(-) diff --git a/node/core/pvf/common/src/worker/mod.rs b/node/core/pvf/common/src/worker/mod.rs index c36f433de7f1..802247cee6fb 100644 --- a/node/core/pvf/common/src/worker/mod.rs +++ b/node/core/pvf/common/src/worker/mod.rs @@ -127,15 +127,7 @@ pub fn worker_event_loop( } } - // Delete all env vars to prevent malicious code from accessing them. - for (key, _) in std::env::vars_os() { - // TODO: *theoretically* the value (or mere presence) of `RUST_LOG` can be a source of - // randomness for malicious code. In the future we can remove it also and log in the host; - // see . - if key != "RUST_LOG" { - std::env::remove_var(key); - } - } + remove_env_vars(); // Run the main worker loop. let rt = Runtime::new().expect("Creates tokio runtime. If this panics the worker will die and the host will detect that and deal with it."); @@ -159,6 +151,47 @@ pub fn worker_event_loop( rt.shutdown_background(); } +/// Delete all env vars to prevent malicious code from accessing them. +fn remove_env_vars() { + for (key, value) in std::env::vars_os() { + // TODO: *theoretically* the value (or mere presence) of `RUST_LOG` can be a source of + // randomness for malicious code. In the future we can remove it also and log in the host; + // see . + if key == "RUST_LOG" { + continue + } + + // In case of a key or value that would cause [`env::remove_var` to + // panic](https://doc.rust-lang.org/std/env/fn.remove_var.html#panics), we first log a + // warning and then proceed to attempt to remove the env var. + let mut err_reasons = vec![]; + let (key_str, value_str) = (key.to_str(), value.to_str()); + if key.is_empty() { + err_reasons.push("key is empty"); + } + if key_str.is_some_and(|s| s.contains('=')) { + err_reasons.push("key contains '='"); + } + if key_str.is_some_and(|s| s.contains('\0')) { + err_reasons.push("key contains '\0' (null character)"); + } + if value_str.is_some_and(|s| s.contains('\0')) { + err_reasons.push("value contains '\0' (null character)"); + } + if !err_reasons.is_empty() { + gum::warn!( + target: LOG_TARGET, + ?key, + ?value, + "Attempting to remove env var may fail: {:?}", + err_reasons + ); + } + + std::env::remove_var(key); + } +} + /// Provide a consistent message on worker shutdown. fn worker_shutdown_message(debug_id: &'static str, worker_pid: u32, err: io::Error) { gum::debug!(target: LOG_TARGET, %worker_pid, "quitting pvf worker ({}): {:?}", debug_id, err); From 862a1b5eb812467e917339513266e6b1b5ace4d2 Mon Sep 17 00:00:00 2001 From: Marcin S Date: Mon, 21 Aug 2023 18:01:03 +0200 Subject: [PATCH 4/5] Make warning message clearer for end users --- node/core/pvf/common/src/worker/mod.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/node/core/pvf/common/src/worker/mod.rs b/node/core/pvf/common/src/worker/mod.rs index 802247cee6fb..901f39e55424 100644 --- a/node/core/pvf/common/src/worker/mod.rs +++ b/node/core/pvf/common/src/worker/mod.rs @@ -127,7 +127,7 @@ pub fn worker_event_loop( } } - remove_env_vars(); + remove_env_vars(debug_id); // Run the main worker loop. let rt = Runtime::new().expect("Creates tokio runtime. If this panics the worker will die and the host will detect that and deal with it."); @@ -152,7 +152,7 @@ pub fn worker_event_loop( } /// Delete all env vars to prevent malicious code from accessing them. -fn remove_env_vars() { +fn remove_env_vars(debug_id: &'static str) { for (key, value) in std::env::vars_os() { // TODO: *theoretically* the value (or mere presence) of `RUST_LOG` can be a source of // randomness for malicious code. In the future we can remove it also and log in the host; @@ -181,9 +181,10 @@ fn remove_env_vars() { if !err_reasons.is_empty() { gum::warn!( target: LOG_TARGET, + %debug_id, ?key, ?value, - "Attempting to remove env var may fail: {:?}", + "Attempting to remove badly-formatted env var, this may cause the PVF worker to crash. Please remove it yourself. Reasons: {:?}", err_reasons ); } From 37856eae36f3e847883341d229cc948887641aec Mon Sep 17 00:00:00 2001 From: Marcin S Date: Mon, 21 Aug 2023 18:55:49 +0200 Subject: [PATCH 5/5] Backslash was unescaped, but can just remove it from error messages --- node/core/pvf/common/src/worker/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/node/core/pvf/common/src/worker/mod.rs b/node/core/pvf/common/src/worker/mod.rs index 901f39e55424..8b41cb82f73b 100644 --- a/node/core/pvf/common/src/worker/mod.rs +++ b/node/core/pvf/common/src/worker/mod.rs @@ -173,10 +173,10 @@ fn remove_env_vars(debug_id: &'static str) { err_reasons.push("key contains '='"); } if key_str.is_some_and(|s| s.contains('\0')) { - err_reasons.push("key contains '\0' (null character)"); + err_reasons.push("key contains null character"); } if value_str.is_some_and(|s| s.contains('\0')) { - err_reasons.push("value contains '\0' (null character)"); + err_reasons.push("value contains null character"); } if !err_reasons.is_empty() { gum::warn!(