Skip to content

Commit 0cabc6f

Browse files
tbu-tbu-
committed
Validate environment variable names in std::process
Make sure that they're not empty and do not contain `=` signs beyond the first character. This prevents environment variable injection, because previously, setting the `PATH=/opt:` variable to `foobar` would lead to the `PATH` variable being overridden. Fixes #122335.
1 parent 56e112a commit 0cabc6f

File tree

6 files changed

+113
-41
lines changed

6 files changed

+113
-41
lines changed

‎library/std/src/process/tests.rs

+29
Original file line numberDiff line numberDiff line change
@@ -378,6 +378,35 @@ fn test_interior_nul_in_env_value_is_error() {
378378
}
379379
}
380380

381+
#[test]
382+
fn test_empty_env_key_is_error() {
383+
match env_cmd().env("", "value").spawn() {
384+
Err(e) => assert_eq!(e.kind(), ErrorKind::InvalidInput),
385+
Ok(_) => panic!(),
386+
}
387+
}
388+
389+
#[test]
390+
fn test_interior_equals_in_env_key_is_error() {
391+
match env_cmd().env("has=equals", "value").spawn() {
392+
Err(e) => assert_eq!(e.kind(), ErrorKind::InvalidInput),
393+
Ok(_) => panic!(),
394+
}
395+
}
396+
397+
#[test]
398+
fn test_env_leading_equals() {
399+
let mut cmd = env_cmd();
400+
cmd.env("=RUN_TEST_LEADING_EQUALS", "789=012");
401+
let result = cmd.output().unwrap();
402+
let output = String::from_utf8_lossy(&result.stdout).to_string();
403+
404+
assert!(
405+
output.contains("=RUN_TEST_LEADING_EQUALS=789=012"),
406+
"didn't find =RUN_TEST_LEADING_EQUALS inside of:\n\n{output}",
407+
);
408+
}
409+
381410
/// Tests that process creation flags work by debugging a process.
382411
/// Other creation flags make it hard or impossible to detect
383412
/// behavioral changes in the process.

‎library/std/src/sys/pal/unix/process/process_common.rs

+36-4
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,7 @@ pub struct Command {
100100
uid: Option<uid_t>,
101101
gid: Option<gid_t>,
102102
saw_nul: bool,
103+
saw_invalid_env_key: bool,
103104
closures: Vec<Box<dyn FnMut() -> io::Result<()> + Send + Sync>>,
104105
groups: Option<Box<[gid_t]>>,
105106
stdin: Option<Stdio>,
@@ -193,6 +194,7 @@ impl Command {
193194
uid: None,
194195
gid: None,
195196
saw_nul,
197+
saw_invalid_env_key: false,
196198
closures: Vec::new(),
197199
groups: None,
198200
stdin: None,
@@ -217,6 +219,7 @@ impl Command {
217219
uid: None,
218220
gid: None,
219221
saw_nul,
222+
saw_invalid_env_key: false,
220223
closures: Vec::new(),
221224
groups: None,
222225
stdin: None,
@@ -279,8 +282,21 @@ impl Command {
279282
self.create_pidfd
280283
}
281284

282-
pub fn saw_nul(&self) -> bool {
283-
self.saw_nul
285+
pub fn validate_input(&self) -> io::Result<()> {
286+
if self.saw_invalid_env_key {
287+
Err(io::const_io_error!(
288+
io::ErrorKind::InvalidInput,
289+
"env key empty or equals sign found in env key",
290+
))
291+
} else if self.saw_nul {
292+
Err(io::const_io_error!(io::ErrorKind::InvalidInput, "nul byte found in provided data"))
293+
} else {
294+
Ok(())
295+
}
296+
}
297+
298+
pub fn saw_invalid_env_key(&self) -> bool {
299+
self.saw_invalid_env_key
284300
}
285301

286302
pub fn get_program(&self) -> &OsStr {
@@ -361,7 +377,7 @@ impl Command {
361377

362378
pub fn capture_env(&mut self) -> Option<CStringArray> {
363379
let maybe_env = self.env.capture_if_changed();
364-
maybe_env.map(|env| construct_envp(env, &mut self.saw_nul))
380+
maybe_env.map(|env| construct_envp(env, &mut self.saw_nul, &mut self.saw_invalid_env_key))
365381
}
366382

367383
#[allow(dead_code)]
@@ -426,9 +442,25 @@ impl CStringArray {
426442
}
427443
}
428444

429-
fn construct_envp(env: BTreeMap<OsString, OsString>, saw_nul: &mut bool) -> CStringArray {
445+
fn construct_envp(
446+
env: BTreeMap<OsString, OsString>,
447+
saw_nul: &mut bool,
448+
saw_invalid_env_key: &mut bool,
449+
) -> CStringArray {
430450
let mut result = CStringArray::with_capacity(env.len());
431451
for (mut k, v) in env {
452+
{
453+
let mut iter = k.as_bytes().iter();
454+
if iter.next().is_none() {
455+
*saw_invalid_env_key = true;
456+
continue;
457+
}
458+
if iter.any(|&b| b == b'=') {
459+
*saw_invalid_env_key = true;
460+
continue;
461+
}
462+
}
463+
432464
// Reserve additional space for '=' and null terminator
433465
k.reserve_exact(v.len() + 2);
434466
k.push("=");

‎library/std/src/sys/pal/unix/process/process_fuchsia.rs

+3-11
Original file line numberDiff line numberDiff line change
@@ -21,12 +21,7 @@ impl Command {
2121
) -> io::Result<(Process, StdioPipes)> {
2222
let envp = self.capture_env();
2323

24-
if self.saw_nul() {
25-
return Err(io::const_io_error!(
26-
io::ErrorKind::InvalidInput,
27-
"nul byte found in provided data",
28-
));
29-
}
24+
self.validate_input()?;
3025

3126
let (ours, theirs) = self.setup_io(default, needs_stdin)?;
3227

@@ -41,11 +36,8 @@ impl Command {
4136
}
4237

4338
pub fn exec(&mut self, default: Stdio) -> io::Error {
44-
if self.saw_nul() {
45-
return io::const_io_error!(
46-
io::ErrorKind::InvalidInput,
47-
"nul byte found in provided data",
48-
);
39+
if let Err(err) = self.validate_input() {
40+
return err;
4941
}
5042

5143
match self.setup_io(default, true) {

‎library/std/src/sys/pal/unix/process/process_unix.rs

+9-14
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use crate::fmt;
2-
use crate::io::{self, Error, ErrorKind};
2+
use crate::io::{self, Error};
33
use crate::mem;
44
use crate::num::NonZero;
55
use crate::sys;
@@ -66,12 +66,7 @@ impl Command {
6666

6767
let envp = self.capture_env();
6868

69-
if self.saw_nul() {
70-
return Err(io::const_io_error!(
71-
ErrorKind::InvalidInput,
72-
"nul byte found in provided data",
73-
));
74-
}
69+
self.validate_input()?;
7570

7671
let (ours, theirs) = self.setup_io(default, needs_stdin)?;
7772

@@ -182,7 +177,7 @@ impl Command {
182177
// way to avoid that all-together.
183178
#[cfg(any(target_os = "tvos", target_os = "watchos"))]
184179
const ERR_APPLE_TV_WATCH_NO_FORK_EXEC: Error = io::const_io_error!(
185-
ErrorKind::Unsupported,
180+
io::ErrorKind::Unsupported,
186181
"`fork`+`exec`-based process spawning is not supported on this target",
187182
);
188183

@@ -223,7 +218,7 @@ impl Command {
223218
thread::sleep(delay);
224219
} else {
225220
return Err(io::const_io_error!(
226-
ErrorKind::WouldBlock,
221+
io::ErrorKind::WouldBlock,
227222
"forking returned EBADF too often",
228223
));
229224
}
@@ -238,8 +233,8 @@ impl Command {
238233
pub fn exec(&mut self, default: Stdio) -> io::Error {
239234
let envp = self.capture_env();
240235

241-
if self.saw_nul() {
242-
return io::const_io_error!(ErrorKind::InvalidInput, "nul byte found in provided data",);
236+
if let Err(err) = self.validate_input() {
237+
return err;
243238
}
244239

245240
match self.setup_io(default, true) {
@@ -499,7 +494,7 @@ impl Command {
499494
thread::sleep(delay);
500495
} else {
501496
return Err(io::const_io_error!(
502-
ErrorKind::WouldBlock,
497+
io::ErrorKind::WouldBlock,
503498
"posix_spawnp returned EBADF too often",
504499
));
505500
}
@@ -1111,14 +1106,14 @@ impl crate::os::linux::process::ChildExt for crate::process::Child {
11111106
self.handle
11121107
.pidfd
11131108
.as_ref()
1114-
.ok_or_else(|| Error::new(ErrorKind::Uncategorized, "No pidfd was created."))
1109+
.ok_or_else(|| Error::new(io::ErrorKind::Uncategorized, "No pidfd was created."))
11151110
}
11161111

11171112
fn take_pidfd(&mut self) -> io::Result<PidFd> {
11181113
self.handle
11191114
.pidfd
11201115
.take()
1121-
.ok_or_else(|| Error::new(ErrorKind::Uncategorized, "No pidfd was created."))
1116+
.ok_or_else(|| Error::new(io::ErrorKind::Uncategorized, "No pidfd was created."))
11221117
}
11231118
}
11241119

‎library/std/src/sys/pal/unix/process/process_vxworks.rs

+2-6
Original file line numberDiff line numberDiff line change
@@ -21,12 +21,8 @@ impl Command {
2121
use crate::sys::cvt_r;
2222
let envp = self.capture_env();
2323

24-
if self.saw_nul() {
25-
return Err(io::const_io_error!(
26-
ErrorKind::InvalidInput,
27-
"nul byte found in provided data",
28-
));
29-
}
24+
self.validate_input()?;
25+
3026
let (ours, theirs) = self.setup_io(default, needs_stdin)?;
3127
let mut p = Process { pid: 0, status: None };
3228

‎library/std/src/sys/pal/windows/process.rs

+34-6
Original file line numberDiff line numberDiff line change
@@ -149,12 +149,40 @@ impl AsRef<OsStr> for EnvKey {
149149
}
150150
}
151151

152-
pub(crate) fn ensure_no_nuls<T: AsRef<OsStr>>(str: T) -> io::Result<T> {
153-
if str.as_ref().encode_wide().any(|b| b == 0) {
154-
Err(io::const_io_error!(ErrorKind::InvalidInput, "nul byte found in provided data"))
155-
} else {
156-
Ok(str)
152+
/// Returns an error if the provided string has a NUL byte anywhere or a `=`
153+
/// after the first character.
154+
fn ensure_env_var_name<T: AsRef<OsStr>>(s: T) -> io::Result<T> {
155+
fn inner(s: &OsStr) -> io::Result<()> {
156+
let err = || {
157+
Err(io::const_io_error!(
158+
ErrorKind::InvalidInput,
159+
"nul or '=' byte found in provided data",
160+
))
161+
};
162+
let mut iter = s.as_encoded_bytes().iter();
163+
if iter.next() == Some(&0) {
164+
return err();
165+
}
166+
if iter.any(|&b| b == 0 || b == b'=') {
167+
return err();
168+
}
169+
Ok(())
170+
}
171+
inner(s.as_ref())?;
172+
Ok(s)
173+
}
174+
175+
/// Returns an error if the provided string has a NUL byte anywhere.
176+
pub(crate) fn ensure_no_nuls<T: AsRef<OsStr>>(s: T) -> io::Result<T> {
177+
fn inner(s: &OsStr) -> io::Result<()> {
178+
if s.as_encoded_bytes().iter().any(|&b| b == 0) {
179+
Err(io::const_io_error!(ErrorKind::InvalidInput, "nul byte found in provided data"))
180+
} else {
181+
Ok(())
182+
}
157183
}
184+
inner(s.as_ref())?;
185+
Ok(s)
158186
}
159187

160188
pub struct Command {
@@ -857,7 +885,7 @@ fn make_envp(maybe_env: Option<BTreeMap<EnvKey, OsString>>) -> io::Result<(*mut
857885
}
858886

859887
for (k, v) in env {
860-
ensure_no_nuls(k.os_string)?;
888+
ensure_env_var_name(k.os_string)?;
861889
blk.extend(k.utf16);
862890
blk.push('=' as u16);
863891
blk.extend(ensure_no_nuls(v)?.encode_wide());

0 commit comments

Comments
 (0)