From a041b0c76ed3c836c074832b1720a352a9b78b00 Mon Sep 17 00:00:00 2001 From: Marcin S Date: Sun, 4 Jun 2023 16:16:28 -0400 Subject: [PATCH 1/4] PVF worker: Prevent access to env vars --- node/core/pvf/common/src/worker.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/node/core/pvf/common/src/worker.rs b/node/core/pvf/common/src/worker.rs index debe18985b37..b9be1a34662b 100644 --- a/node/core/pvf/common/src/worker.rs +++ b/node/core/pvf/common/src/worker.rs @@ -101,6 +101,11 @@ pub fn worker_event_loop( } } + // Delete env vars to prevent malicious code from accessing them. + for (key, _) in std::env::vars() { + std::env::remove_var(key); + } + // 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."); let err = rt From 7d60c2eb259fa27dbd5cc7d215b22497f13766db Mon Sep 17 00:00:00 2001 From: Marcin S Date: Sun, 4 Jun 2023 16:39:17 -0400 Subject: [PATCH 2/4] Clear env vars at time of spawning process --- node/core/pvf/common/src/worker.rs | 5 ----- node/core/pvf/src/worker_intf.rs | 10 +++++++++- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/node/core/pvf/common/src/worker.rs b/node/core/pvf/common/src/worker.rs index b9be1a34662b..debe18985b37 100644 --- a/node/core/pvf/common/src/worker.rs +++ b/node/core/pvf/common/src/worker.rs @@ -101,11 +101,6 @@ pub fn worker_event_loop( } } - // Delete env vars to prevent malicious code from accessing them. - for (key, _) in std::env::vars() { - std::env::remove_var(key); - } - // 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."); let err = rt diff --git a/node/core/pvf/src/worker_intf.rs b/node/core/pvf/src/worker_intf.rs index 33144616601d..34fb1d79aafc 100644 --- a/node/core/pvf/src/worker_intf.rs +++ b/node/core/pvf/src/worker_intf.rs @@ -217,7 +217,15 @@ impl WorkerHandle { extra_args: &[&str], socket_path: impl AsRef, ) -> io::Result { - let mut child = process::Command::new(program.as_ref()) + // Clear all env vars from the spawned process. + let mut command = process::Command::new(program.as_ref()); + command.env_clear(); + // Add back any env vars we want to keep. + if let Ok(env) = std::env::var("RUST_LOG") { + command.env("RUST_LOG", env); + } + + let mut child = command .args(extra_args) .arg("--socket-path") .arg(socket_path.as_ref().as_os_str()) From e0cfab7090a8ca8a1a25345c974cf7dce5fb5689 Mon Sep 17 00:00:00 2001 From: Marcin S Date: Sun, 6 Aug 2023 16:47:23 +0200 Subject: [PATCH 3/4] Move env var clearage back to child...! Exception for RUST_LOG Clearing env vars with the `std::process::Command` API didn't get everything on Mac, namely `__CF_USER_TEXT_ENCODING` was still present. While we don't support Mac itself as a secure system, the same issue could exist on some Linux systems either now or in the future. So it is better to just clear it on the child-side and not worry about it. We may not use the `Command` API in the future, anyway: https://github.com/paritytech/polkadot/issues/4721 --- node/core/pvf/common/src/worker.rs | 10 ++++++++++ node/core/pvf/src/worker_intf.rs | 10 +--------- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/node/core/pvf/common/src/worker.rs b/node/core/pvf/common/src/worker.rs index debe18985b37..4bf61c61a5cb 100644 --- a/node/core/pvf/common/src/worker.rs +++ b/node/core/pvf/common/src/worker.rs @@ -101,6 +101,16 @@ pub fn worker_event_loop( } } + // Delete all env vars to prevent malicious code from accessing them. + for (key, _) in std::env::vars() { + // 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); + } + } + // 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."); let err = rt diff --git a/node/core/pvf/src/worker_intf.rs b/node/core/pvf/src/worker_intf.rs index 34fb1d79aafc..33144616601d 100644 --- a/node/core/pvf/src/worker_intf.rs +++ b/node/core/pvf/src/worker_intf.rs @@ -217,15 +217,7 @@ impl WorkerHandle { extra_args: &[&str], socket_path: impl AsRef, ) -> io::Result { - // Clear all env vars from the spawned process. - let mut command = process::Command::new(program.as_ref()); - command.env_clear(); - // Add back any env vars we want to keep. - if let Ok(env) = std::env::var("RUST_LOG") { - command.env("RUST_LOG", env); - } - - let mut child = command + let mut child = process::Command::new(program.as_ref()) .args(extra_args) .arg("--socket-path") .arg(socket_path.as_ref().as_os_str()) From 2df975587532909c903f758e8b75887617df0be8 Mon Sep 17 00:00:00 2001 From: Marcin S Date: Tue, 8 Aug 2023 11:03:34 +0200 Subject: [PATCH 4/4] Update impl guide --- .../src/node/utility/pvf-host-and-workers.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/roadmap/implementers-guide/src/node/utility/pvf-host-and-workers.md b/roadmap/implementers-guide/src/node/utility/pvf-host-and-workers.md index 017b7fc025cc..bcf01b61f217 100644 --- a/roadmap/implementers-guide/src/node/utility/pvf-host-and-workers.md +++ b/roadmap/implementers-guide/src/node/utility/pvf-host-and-workers.md @@ -125,3 +125,10 @@ A basic security mechanism is to make sure that any thread directly interfacing with untrusted code does not have access to the file-system. This provides some protection against attackers accessing sensitive data or modifying data on the host machine. + +### Clearing env vars + +We clear environment variables before handling untrusted code, because why give +attackers potentially sensitive data unnecessarily? And even if everything else +is locked down, env vars can potentially provide a source of randomness (see +point 1, "Consensus faults" above).