PVF: Move landlock out of thread into process; add landlock exceptions

node/core/pvf/common/src/worker/mod.rs

@@ -70,17 +70,21 @@ macro_rules! decl_worker_main {
 			}
 
 			let mut node_version = None;
-			let mut socket_path: &str = "";
+			let mut socket_path = None;
+			let mut cache_path = None;
 
 			for i in (2..args.len()).step_by(2) {
 				match args[i].as_ref() {
-					"--socket-path" => socket_path = args[i + 1].as_str(),
+					"--socket-path" => socket_path = Some(args[i + 1].as_str()),
 					"--node-impl-version" => node_version = Some(args[i + 1].as_str()),
+					"--cache-path" => cache_path = Some(args[i + 1].as_str()),
 					arg => panic!("Unexpected argument found: {}", arg),
 				}
 			}
+			let socket_path = socket_path.expect("the --socket-path argument is required");
+			let cache_path = cache_path.expect("the --cache-path argument is required");
 
-			$entrypoint(&socket_path, node_version, Some($worker_version));
+			$entrypoint(&socket_path, node_version, Some($worker_version), cache_path);
 		}
 	};
 }

node/core/pvf/common/src/worker/security.rs

@@ -52,14 +52,19 @@ impl LandlockStatus {
 /// [landlock]: https://docs.rs/landlock/latest/landlock/index.html
 #[cfg(target_os = "linux")]
 pub mod landlock {
-	use landlock::{Access, AccessFs, Ruleset, RulesetAttr, RulesetError, RulesetStatus, ABI};
+	pub use landlock::{path_beneath_rules, Access, AccessFs};
+
+	use landlock::{
+		PathBeneath, PathFd, Ruleset, RulesetAttr, RulesetCreatedAttr, RulesetError, RulesetStatus,
+		ABI,
+	};
 
 	/// Landlock ABI version. We use ABI V1 because:
 	///
 	/// 1. It is supported by our reference kernel version.
 	/// 2. Later versions do not (yet) provide additional security.
 	///
-	/// # Versions (June 2023)
+	/// # Versions (as of June 2023)
 	///
 	/// - Polkadot reference kernel version: 5.16+
 	/// - ABI V1: 5.13 - introduces	landlock, including full restrictions on file reads
@@ -87,7 +92,7 @@ pub mod landlock {
 	/// Returns to what degree landlock is enabled with the given ABI on the current Linux
 	/// environment.
 	pub fn get_status() -> Result<RulesetStatus, Box<dyn std::error::Error>> {
-		match std::thread::spawn(|| try_restrict_thread()).join() {
+		match std::thread::spawn(|| try_restrict_thread(std::iter::empty())).join() {
 			Ok(Ok(status)) => Ok(status),
 			Ok(Err(ruleset_err)) => Err(ruleset_err.into()),
 			Err(_err) => Err("a panic occurred in try_restrict_thread".into()),
@@ -110,18 +115,21 @@ pub mod landlock {
 
 	/// Tries to restrict the current thread with the following landlock access controls:
 	///
-	/// 1. all global filesystem access
-	/// 2. ... more may be supported in the future.
+	/// 1. all global filesystem access restricted, with optional exceptions
+	/// 2. ... more sandbox types (e.g. networking) may be supported in the future.
 	///
 	/// If landlock is not supported in the current environment this is simply a noop.
 	///
 	/// # Returns
 	///
 	/// The status of the restriction (whether it was fully, partially, or not-at-all enforced).
-	pub fn try_restrict_thread() -> Result<RulesetStatus, RulesetError> {
+	pub fn try_restrict_thread(
+		fs_exceptions: impl Iterator<Item = Result<PathBeneath<PathFd>, RulesetError>>,
+	) -> Result<RulesetStatus, RulesetError> {
 		let status = Ruleset::new()
 			.handle_access(AccessFs::from_all(LANDLOCK_ABI))?
 			.create()?
+			.add_rules(fs_exceptions)?
 			.restrict_self()?;
 		Ok(status.ruleset)
 	}
@@ -132,50 +140,115 @@ pub mod landlock {
 		use std::{fs, io::ErrorKind, thread};
 
 		#[test]
-		fn restricted_thread_cannot_access_fs() {
+		fn restricted_thread_cannot_read_file() {
 			// TODO: This would be nice: <https://github.com/rust-lang/rust/issues/68007>.
 			if !check_is_fully_enabled() {
 				return
 			}
 
 			// Restricted thread cannot read from FS.
 			let handle = thread::spawn(|| {
-				// Write to a tmp file, this should succeed before landlock is applied.
-				let text = "foo";
-				let tmpfile = tempfile::NamedTempFile::new().unwrap();
-				let path = tmpfile.path();
-				fs::write(path, text).unwrap();
-				let s = fs::read_to_string(path).unwrap();
-				assert_eq!(s, text);
-
-				let status = try_restrict_thread().unwrap();
+				// Create, write, and read two tmp files. This should succeed before any landlock
+				// restrictions are applied.
+				const TEXT: &str = "foo";
+				let tmpfile1 = tempfile::NamedTempFile::new().unwrap();
+				let path1 = tmpfile1.path();
+				let tmpfile2 = tempfile::NamedTempFile::new().unwrap();
+				let path2 = tmpfile2.path();
+
+				fs::write(path1, TEXT).unwrap();
+				let s = fs::read_to_string(path1).unwrap();
+				assert_eq!(s, TEXT);
+				fs::write(path2, TEXT).unwrap();
+				let s = fs::read_to_string(path2).unwrap();
+				assert_eq!(s, TEXT);
+
+				// Apply Landlock with a read exception for only one of the files.
+				let status = try_restrict_thread(path_beneath_rules(
+					&[path1],
+					AccessFs::from_read(LANDLOCK_ABI),
+				))
+				.unwrap();
+				if !matches!(status, RulesetStatus::FullyEnforced) {
+					panic!("Ruleset should be enforced since we checked if landlock is enabled");
+				}
+
+				// Try to read from both files, only tmpfile1 should succeed.
+				let result = fs::read_to_string(path1);
+				assert!(matches!(
+					result,
+					Ok(s) if s == TEXT
+				));
+				let result = fs::read_to_string(path2);
+				assert!(matches!(
+					result,
+					Err(err) if matches!(err.kind(), ErrorKind::PermissionDenied)
+				));
+
+				// Apply Landlock for all files.
+				let status = try_restrict_thread(std::iter::empty()).unwrap();
 				if !matches!(status, RulesetStatus::FullyEnforced) {
 					panic!("Ruleset should be enforced since we checked if landlock is enabled");
 				}
 
-				// Try to read from the tmp file after landlock.
-				let result = fs::read_to_string(path);
+				// Try to read from tmpfile1 after landlock, it should fail.
+				let result = fs::read_to_string(path1);
 				assert!(matches!(
 					result,
 					Err(err) if matches!(err.kind(), ErrorKind::PermissionDenied)
 				));
 			});
 
 			assert!(handle.join().is_ok());
+		}
+
+		#[test]
+		fn restricted_thread_cannot_write_file() {
+			// TODO: This would be nice: <https://github.com/rust-lang/rust/issues/68007>.
+			if !check_is_fully_enabled() {
+				return
+			}
 
 			// Restricted thread cannot write to FS.
 			let handle = thread::spawn(|| {
-				let text = "foo";
-				let tmpfile = tempfile::NamedTempFile::new().unwrap();
-				let path = tmpfile.path();
+				// Create and write two tmp files. This should succeed before any landlock
+				// restrictions are applied.
+				const TEXT: &str = "foo";
+				let tmpfile1 = tempfile::NamedTempFile::new().unwrap();
+				let path1 = tmpfile1.path();
+				let tmpfile2 = tempfile::NamedTempFile::new().unwrap();
+				let path2 = tmpfile2.path();
+
+				fs::write(path1, TEXT).unwrap();
+				fs::write(path2, TEXT).unwrap();
+
+				// Apply Landlock with a write exception for only one of the files.
+				let status = try_restrict_thread(path_beneath_rules(
+					&[path1],
+					AccessFs::from_write(LANDLOCK_ABI),
+				))
+				.unwrap();
+				if !matches!(status, RulesetStatus::FullyEnforced) {
+					panic!("Ruleset should be enforced since we checked if landlock is enabled");
+				}
+
+				// Try to write to both files, only tmpfile1 should succeed.
+				let result = fs::write(path1, TEXT);
+				assert!(matches!(result, Ok(_)));
+				let result = fs::write(path2, TEXT);
+				assert!(matches!(
+					result,
+					Err(err) if matches!(err.kind(), ErrorKind::PermissionDenied)
+				));
 
-				let status = try_restrict_thread().unwrap();
+				// Apply Landlock for all files.
+				let status = try_restrict_thread(std::iter::empty()).unwrap();
 				if !matches!(status, RulesetStatus::FullyEnforced) {
 					panic!("Ruleset should be enforced since we checked if landlock is enabled");
 				}
 
-				// Try to write to the tmp file after landlock.
-				let result = fs::write(path, text);
+				// Try to write to tmpfile1 after landlock, it should fail.
+				let result = fs::write(path1, TEXT);
 				assert!(matches!(
 					result,
 					Err(err) if matches!(err.kind(), ErrorKind::PermissionDenied)

node/core/pvf/execute-worker/src/lib.rs

@@ -37,7 +37,7 @@ use polkadot_node_core_pvf_common::{
 };
 use polkadot_parachain::primitives::ValidationResult;
 use std::{
-	path::PathBuf,
+	path::{Path, PathBuf},
 	sync::{mpsc::channel, Arc},
 	time::Duration,
 };
@@ -117,14 +117,21 @@ async fn send_response(stream: &mut UnixStream, response: Response) -> io::Resul
 ///
 /// # Parameters
 ///
-/// The `socket_path` specifies the path to the socket used to communicate with the host. The
-/// `node_version`, if `Some`, is checked against the worker version. A mismatch results in
-/// immediate worker termination. `None` is used for tests and in other situations when version
-/// check is not necessary.
+/// - `socket_path` specifies the path to the socket used to communicate with the host.
+///
+/// - `node_version`, if `Some`, is checked against the `worker_version`. A mismatch results in
+///   immediate worker termination. `None` is used for tests and in other situations when version
+///   check is not necessary.
+///
+/// - `worker_version`: see above
+///
+/// - `cache_path` contains the expected cache path for artifacts and is used to provide a sandbox
+///   exception for landlock.
 pub fn worker_entrypoint(
 	socket_path: &str,
 	node_version: Option<&str>,
 	worker_version: Option<&str>,
+	cache_path: &Path,
 ) {
 	worker_event_loop(
 		"execute",
@@ -134,6 +141,36 @@ pub fn worker_entrypoint(
 		|mut stream| async move {
 			let worker_pid = std::process::id();
 
+			// Try to enable landlock.
+			{
+				#[cfg(target_os = "linux")]
+				let landlock_status = {
+					use polkadot_node_core_pvf_common::worker::security::landlock::{
+						path_beneath_rules, try_restrict_thread, Access, AccessFs, LANDLOCK_ABI,
+					};
+
+					try_restrict_thread(path_beneath_rules(
+						&[cache_path],
+						AccessFs::from_read(LANDLOCK_ABI),
+					))
+					.map(LandlockStatus::from_ruleset_status)
+					.map_err(|e| e.to_string())
+				};
+				#[cfg(not(target_os = "linux"))]
+				let landlock_status: Result<LandlockStatus, String> = Ok(LandlockStatus::NotEnforced);
+
+				// TODO: return an error?
+				// Log if landlock threw an error.
+				if let Err(err) = landlock_status {
+					gum::warn!(
+						target: LOG_TARGET,
+						%worker_pid,
+						"error enabling landlock: {}",
+						err
+					);
+				}
+			}
+
 			let handshake = recv_handshake(&mut stream).await?;
 			let executor = Executor::new(handshake.executor_params).map_err(|e| {
 				io::Error::new(io::ErrorKind::Other, format!("cannot create executor: {}", e))
@@ -148,9 +185,11 @@ pub fn worker_entrypoint(
 					artifact_path.display(),
 				);
 
+				if !artifact_path.starts_with(cache_path) {
+					return Err(io::Error::new(io::ErrorKind::Other, format!("received an artifact path {artifact_path:?} that does not belong to expected artifact dir {cache_path:?}")))
+				}
+
 				// Get the artifact bytes.
-				//
-				// We do this outside the thread so that we can lock down filesystem access there.
 				let compiled_artifact_blob = match std::fs::read(artifact_path) {
 					Ok(bytes) => bytes,
 					Err(err) => {
@@ -185,22 +224,11 @@ pub fn worker_entrypoint(
 				let execute_thread = thread::spawn_worker_thread_with_stack_size(
 					"execute thread",
 					move || {
-						// Try to enable landlock.
-						#[cfg(target_os = "linux")]
-					let landlock_status = polkadot_node_core_pvf_common::worker::security::landlock::try_restrict_thread()
-						.map(LandlockStatus::from_ruleset_status)
-						.map_err(|e| e.to_string());
-						#[cfg(not(target_os = "linux"))]
-						let landlock_status: Result<LandlockStatus, String> = Ok(LandlockStatus::NotEnforced);
-
-						(
-							validate_using_artifact(
-								&compiled_artifact_blob,
-								&params,
-								executor_2,
-								cpu_time_start,
-							),
-							landlock_status,
+						validate_using_artifact(
+							&compiled_artifact_blob,
+							&params,
+							executor_2,
+							cpu_time_start,
 						)
 					},
 					Arc::clone(&condvar),
@@ -213,24 +241,9 @@ pub fn worker_entrypoint(
 				let response = match outcome {
 					WaitOutcome::Finished => {
 						let _ = cpu_time_monitor_tx.send(());
-						let (result, landlock_status) = execute_thread.join().unwrap_or_else(|e| {
-							(
-								Response::Panic(stringify_panic_payload(e)),
-								Ok(LandlockStatus::Unavailable),
-							)
-						});
-
-						// Log if landlock threw an error.
-						if let Err(err) = landlock_status {
-							gum::warn!(
-								target: LOG_TARGET,
-								%worker_pid,
-								"error enabling landlock: {}",
-								err
-							);
-						}
-
-						result
+						execute_thread
+							.join()
+							.unwrap_or_else(|e| Response::Panic(stringify_panic_payload(e)))
 					},
 					// If the CPU thread is not selected, we signal it to end, the join handle is
 					// dropped and the thread will finish in the background.