From a1457de50fc39d35c585d5196aa6d9e7585a3590 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Mon, 26 Oct 2020 15:56:50 +0100 Subject: [PATCH 1/6] feat(wasi) Allow the `=` sign in the environment variable value. Having `a=b` means that the environment variable key is `a`, and its value is `b`. Having `a=b=c` means that the key is `a` and its value is `b=c`. It's OK to get this, e.g. within a CGI context where values can hold strings such as `sec-ch-ua: "Chromium;v=86"` etc. See https://github.com/wasmerio/wasmer/issues/1708 for a longer explanation. --- lib/wasi/src/state/builder.rs | 34 ++++++++-------------------------- 1 file changed, 8 insertions(+), 26 deletions(-) diff --git a/lib/wasi/src/state/builder.rs b/lib/wasi/src/state/builder.rs index f6694bb021a..74f4676eda6 100644 --- a/lib/wasi/src/state/builder.rs +++ b/lib/wasi/src/state/builder.rs @@ -328,33 +328,15 @@ impl WasiStateBuilder { } } } + for env in self.envs.iter() { - let mut eq_seen = false; - for b in env.iter() { - match *b { - b'=' => { - if eq_seen { - return Err(WasiStateCreationError::EnvironmentVariableFormatError( - format!( - "found '=' in env var string \"{}\" (key=value)", - std::str::from_utf8(env) - .unwrap_or("Inner error: env var is invalid_utf8!") - ), - )); - } - eq_seen = true; - } - 0 => { - return Err(WasiStateCreationError::EnvironmentVariableFormatError( - format!( - "found nul byte in env var string \"{}\" (key=value)", - std::str::from_utf8(env) - .unwrap_or("Inner error: env var is invalid_utf8!") - ), - )); - } - _ => (), - } + if env.iter().find(|&&ch| ch == 0).is_some() { + return Err(WasiStateCreationError::EnvironmentVariableFormatError( + format!( + "found nul byte in env var string \"{}\" (key=value)", + std::str::from_utf8(env).unwrap_or("Inner error: env var is invalid_utf8!") + ), + )); } } From 32c08b5c6794149a7a7127320168070b8640b381 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Mon, 26 Oct 2020 15:59:23 +0100 Subject: [PATCH 2/6] test(wasi) Update test cases according to the previous commit. --- lib/wasi/src/state/builder.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/wasi/src/state/builder.rs b/lib/wasi/src/state/builder.rs index 74f4676eda6..ed59ecb2605 100644 --- a/lib/wasi/src/state/builder.rs +++ b/lib/wasi/src/state/builder.rs @@ -490,17 +490,21 @@ mod test { #[test] fn env_var_errors() { + // `a=b` means key is `a` and value is `b`, which is OK. + // `a=b=c` means key is `a` and value is `b=c`, which is OK too. let output = create_wasi_state("test_prog") - .env("HOM=E", "/home/home") + .env("HOME", "/home/home=foo") .build(); + match output { - Err(WasiStateCreationError::EnvironmentVariableFormatError(_)) => assert!(true), - _ => assert!(false), + Err(WasiStateCreationError::EnvironmentVariableFormatError(_)) => assert!(false), + _ => assert!(true), } let output = create_wasi_state("test_prog") .env("HOME\0", "/home/home") .build(); + match output { Err(WasiStateCreationError::EnvironmentVariableFormatError(_)) => assert!(true), _ => assert!(false), From 41c22d2369fa50bfd9fa4d91bb2a22e6389a8185 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Mon, 26 Oct 2020 16:01:49 +0100 Subject: [PATCH 3/6] doc(changelog) Add #1762. --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4621db71a3e..a9c7b93ef7c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,10 @@ - [#1709](https://github.com/wasmerio/wasmer/pull/1709) Implement `wasm_module_name` and `wasm_module_set_name` in the Wasm(er) C API. - [#1700](https://github.com/wasmerio/wasmer/pull/1700) Implement `wasm_externtype_copy` in the Wasm C API. +### Changed + +- [#1762](https://github.com/wasmerio/wasmer/pull/1762) Allow the `=` sign in a WASI environment variable value. + ### Fixed - [#1718](https://github.com/wasmerio/wasmer/pull/1718) Fix panic in the API in some situations when the memory's min bound was greater than the memory's max bound. From 66cdd7b72b746f742d977bb8c8dc2abb5c89282a Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Mon, 26 Oct 2020 16:35:14 +0100 Subject: [PATCH 4/6] doc(wasi) Update API documentation. --- lib/wasi/src/state/builder.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/wasi/src/state/builder.rs b/lib/wasi/src/state/builder.rs index ed59ecb2605..8fcf7c84535 100644 --- a/lib/wasi/src/state/builder.rs +++ b/lib/wasi/src/state/builder.rs @@ -93,7 +93,7 @@ fn validate_mapped_dir_alias(alias: &str) -> Result<(), WasiStateCreationError> // return stdout somehow, it's unclear what that API should look like) impl WasiStateBuilder { /// Add an environment variable pair. - /// Environment variable keys and values must not contain the byte `=` (0x3d) + /// Environment variable keys must not contain the byte `=` (0x3d) /// or nul (0x0). pub fn env(&mut self, key: Key, value: Value) -> &mut Self where @@ -130,7 +130,7 @@ impl WasiStateBuilder { } /// Add multiple environment variable pairs. - /// Keys and values must not contain the `=` (0x3d) or nul (0x0) byte. + /// Keys must not contain the `=` (0x3d) or nul (0x0) byte. pub fn envs(&mut self, env_pairs: I) -> &mut Self where I: IntoIterator, From af9b9bd326f560ae0ed566e5ad1c5c644fb15bd2 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Tue, 27 Oct 2020 10:59:49 +0100 Subject: [PATCH 5/6] feat(wasi) Replace `unwrap_or` by `from_utf8_lossy`. Since `env` contains a nul byte, it's better to get a lossy version of it. Moreover, it removes one more `unwrap`. --- lib/wasi/src/state/builder.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/wasi/src/state/builder.rs b/lib/wasi/src/state/builder.rs index 8fcf7c84535..cc7cfc7801a 100644 --- a/lib/wasi/src/state/builder.rs +++ b/lib/wasi/src/state/builder.rs @@ -334,7 +334,7 @@ impl WasiStateBuilder { return Err(WasiStateCreationError::EnvironmentVariableFormatError( format!( "found nul byte in env var string \"{}\" (key=value)", - std::str::from_utf8(env).unwrap_or("Inner error: env var is invalid_utf8!") + String::from_utf8_lossy(env) ), )); } From 89c62e7ae39ed8c4159a67242e235a8cc9eaba4f Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Tue, 27 Oct 2020 11:33:23 +0100 Subject: [PATCH 6/6] feat(wasi) Seperate env's key and value and do different validations. This patch keeps the env's key and value separate until the `build()` method is called. It allows to apply different validations resp. for the key and the value: Both must not contain a nul byte, but only the key cannot contain an equal sign. This patch also updates the test cases accordingly. Finally this patch updates the `envs` and `args` methods to reuse respectively `env` and `arg` to avoid code repetition. --- lib/wasi/src/state/builder.rs | 155 +++++++++++++++++++++------------- 1 file changed, 96 insertions(+), 59 deletions(-) diff --git a/lib/wasi/src/state/builder.rs b/lib/wasi/src/state/builder.rs index cc7cfc7801a..68a233ccd60 100644 --- a/lib/wasi/src/state/builder.rs +++ b/lib/wasi/src/state/builder.rs @@ -35,7 +35,7 @@ pub(crate) fn create_wasi_state(program_name: &str) -> WasiStateBuilder { #[derive(Default)] pub struct WasiStateBuilder { args: Vec>, - envs: Vec>, + envs: Vec<(Vec, Vec)>, preopens: Vec, #[allow(clippy::type_complexity)] setup_fs_fn: Option Result<(), String> + Send>>, @@ -100,17 +100,8 @@ impl WasiStateBuilder { Key: AsRef<[u8]>, Value: AsRef<[u8]>, { - let key_b = key.as_ref(); - let val_b = value.as_ref(); - - let length = key_b.len() + val_b.len() + 1; - let mut byte_vec = Vec::with_capacity(length); - - byte_vec.extend_from_slice(&key_b); - byte_vec.push(b'='); - byte_vec.extend_from_slice(&val_b); - - self.envs.push(byte_vec); + self.envs + .push((key.as_ref().to_vec(), value.as_ref().to_vec())); self } @@ -121,10 +112,7 @@ impl WasiStateBuilder { where Arg: AsRef<[u8]>, { - let arg_b = arg.as_ref(); - let mut byte_vec = Vec::with_capacity(arg_b.len()); - byte_vec.extend_from_slice(&arg_b); - self.args.push(byte_vec); + self.args.push(arg.as_ref().to_vec()); self } @@ -137,19 +125,9 @@ impl WasiStateBuilder { Key: AsRef<[u8]>, Value: AsRef<[u8]>, { - for (key, value) in env_pairs { - let key_b = key.as_ref(); - let val_b = value.as_ref(); - - let length = key_b.len() + val_b.len() + 1; - let mut byte_vec = Vec::with_capacity(length); - - byte_vec.extend_from_slice(&key_b); - byte_vec.push(b'='); - byte_vec.extend_from_slice(&val_b); - - self.envs.push(byte_vec); - } + env_pairs.into_iter().for_each(|(key, value)| { + self.env(key, value); + }); self } @@ -161,12 +139,9 @@ impl WasiStateBuilder { I: IntoIterator, Arg: AsRef<[u8]>, { - for arg in args { - let arg_b = arg.as_ref(); - let mut byte_vec = Vec::with_capacity(arg_b.len()); - byte_vec.extend_from_slice(&arg_b); - self.args.push(byte_vec); - } + args.into_iter().for_each(|arg| { + self.arg(arg); + }); self } @@ -329,12 +304,47 @@ impl WasiStateBuilder { } } - for env in self.envs.iter() { - if env.iter().find(|&&ch| ch == 0).is_some() { + enum InvalidCharacter { + Nul, + Equal, + } + + for (env_key, env_value) in self.envs.iter() { + match env_key.iter().find_map(|&ch| { + if ch == 0 { + Some(InvalidCharacter::Nul) + } else if ch == b'=' { + Some(InvalidCharacter::Equal) + } else { + None + } + }) { + Some(InvalidCharacter::Nul) => { + return Err(WasiStateCreationError::EnvironmentVariableFormatError( + format!( + "found nul byte in env var key \"{}\" (key=value)", + String::from_utf8_lossy(env_key) + ), + )) + } + + Some(InvalidCharacter::Equal) => { + return Err(WasiStateCreationError::EnvironmentVariableFormatError( + format!( + "found equal sign in env var key \"{}\" (key=value)", + String::from_utf8_lossy(env_key) + ), + )) + } + + None => (), + } + + if env_value.iter().find(|&&ch| ch == 0).is_some() { return Err(WasiStateCreationError::EnvironmentVariableFormatError( format!( - "found nul byte in env var string \"{}\" (key=value)", - String::from_utf8_lossy(env) + "found nul byte in env var value \"{}\" (key=value)", + String::from_utf8_lossy(env_value) ), )); } @@ -368,7 +378,18 @@ impl WasiStateBuilder { Ok(WasiState { fs: wasi_fs, args: self.args.clone(), - envs: self.envs.clone(), + envs: self + .envs + .iter() + .map(|(key, value)| { + let mut env = Vec::with_capacity(key.len() + value.len() + 1); + env.extend_from_slice(&key); + env.push(b'='); + env.extend_from_slice(&value); + + env + }) + .collect(), }) } @@ -490,25 +511,41 @@ mod test { #[test] fn env_var_errors() { - // `a=b` means key is `a` and value is `b`, which is OK. - // `a=b=c` means key is `a` and value is `b=c`, which is OK too. - let output = create_wasi_state("test_prog") - .env("HOME", "/home/home=foo") - .build(); - - match output { - Err(WasiStateCreationError::EnvironmentVariableFormatError(_)) => assert!(false), - _ => assert!(true), - } - - let output = create_wasi_state("test_prog") - .env("HOME\0", "/home/home") - .build(); - - match output { - Err(WasiStateCreationError::EnvironmentVariableFormatError(_)) => assert!(true), - _ => assert!(false), - } + // `=` in the key is invalid. + assert!( + create_wasi_state("test_prog") + .env("HOM=E", "/home/home") + .build() + .is_err(), + "equal sign in key must be invalid" + ); + + // `\0` in the key is invalid. + assert!( + create_wasi_state("test_prog") + .env("HOME\0", "/home/home") + .build() + .is_err(), + "nul in key must be invalid" + ); + + // `=` in the value is valid. + assert!( + create_wasi_state("test_prog") + .env("HOME", "/home/home=home") + .build() + .is_ok(), + "equal sign in the value must be valid" + ); + + // `\0` in the value is invalid. + assert!( + create_wasi_state("test_prog") + .env("HOME", "/home/home\0") + .build() + .is_err(), + "nul in value must be invalid" + ); } #[test]