Skip to content

Commit dc740c9

Browse files
authored
Fix wasmtime-wasi path_open(TRUNCATE) bypass of FilePerms::WRITE check (#13429)
* NFC: refactor wasi testing to make WasiCtxBuilder changes with closure * fix and test GHSA-2r75-cxrj-cmph In wasmtime-wasi, when a filesystem preopen is given DirPerms::all() and FilePerms::READ without FilePerms::WRITE, this wasmtime-wasi enforced access control mechanism can be bypassed by using the wasip2 descriptor.open-at or wasip1 path_open interfaces by opening a file with OpenFlags::TRUNCATE oflag only, for example: ```rust dir_descriptor.open_at( PathFlags::empty(), FILENAME, OpenFlags::TRUNCATE, DescriptorFlags::READ, ) ``` or ```rust wasip1::path_open( dir_fd, 0, FILENAME, wasip1::OFLAGS_TRUNC, wasip1::RIGHTS_FD_READ, 0, 0 ) ``` The root cause is that the clause that considered OpenFlags::TRUNCATE did not set open_mode |= OpenMode::WRITE;, used later in that function for the access control check against FilePerms for whether opening that file is permitted. With the bug corrected, these calls to open-at and path_open fail with error-code.not-permitted and ERRNO_PERM respectively. This commit contains the fix for the above bug, and tests for the fix. * wasi-common: test stubs
1 parent a9a2712 commit dc740c9

8 files changed

Lines changed: 541 additions & 215 deletions

File tree

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
#![expect(unsafe_op_in_unsafe_fn, reason = "old code, not worth updating yet")]
2+
3+
use std::process;
4+
use test_programs::preview1::{BlockingMode, open_scratch_directory};
5+
6+
const FILENAME: &str = "test.txt";
7+
unsafe fn test_file_has_expected_contents(dir_fd: wasip1::Fd, blocking_mode: &BlockingMode) {
8+
// Open a file for reading
9+
let file_fd = wasip1::path_open(
10+
dir_fd,
11+
0,
12+
FILENAME,
13+
0,
14+
wasip1::RIGHTS_FD_READ,
15+
0,
16+
blocking_mode.fd_flags(),
17+
)
18+
.expect("opening test.txt for reading");
19+
20+
// Read the file's contents
21+
let buffer = &mut [0u8; 100];
22+
let nread = blocking_mode
23+
.read(
24+
file_fd,
25+
&[wasip1::Iovec {
26+
buf: buffer.as_mut_ptr(),
27+
buf_len: buffer.len(),
28+
}],
29+
)
30+
.expect("reading file content");
31+
32+
const EXPECTED_CONTENTS: &[u8] = b"truncation test file\n";
33+
// The file should be as created by the test harness, not truncated.
34+
assert_eq!(nread, EXPECTED_CONTENTS.len(), "expected untouched file");
35+
assert_eq!(
36+
&buffer[..nread],
37+
EXPECTED_CONTENTS,
38+
"expected untouched file contents"
39+
);
40+
41+
wasip1::fd_close(file_fd).expect("closing the file");
42+
}
43+
44+
unsafe fn test_file_truncation_readonly(dir_fd: wasip1::Fd, blocking_mode: BlockingMode) {
45+
// Check test preconditions.
46+
test_file_has_expected_contents(dir_fd, &blocking_mode);
47+
48+
// Opening the file for truncation should fail.
49+
let err = wasip1::path_open(
50+
dir_fd,
51+
0,
52+
FILENAME,
53+
wasip1::OFLAGS_TRUNC,
54+
wasip1::RIGHTS_FD_READ,
55+
0,
56+
blocking_mode.fd_flags(),
57+
);
58+
assert!(err.is_err(), "opening file for truncation should fail");
59+
assert_eq!(
60+
err.err().unwrap(),
61+
wasip1::ERRNO_PERM,
62+
"opening file for truncation should fail with PERM"
63+
);
64+
65+
// Check that truncation did not occur.
66+
test_file_has_expected_contents(dir_fd, &blocking_mode);
67+
}
68+
69+
fn main() {
70+
// This test program requires a special preopen at the path "readonly",
71+
// which the host enforces as read-only. Unlike other test programs, this
72+
// directory's path not passed in as an argument, because modifications to
73+
// the testing harness would be too invasive.
74+
let dir_fd = match open_scratch_directory("readonly") {
75+
Ok(dir_fd) => dir_fd,
76+
Err(err) => {
77+
eprintln!("{err}");
78+
process::exit(1)
79+
}
80+
};
81+
82+
// Run the tests.
83+
unsafe {
84+
test_file_truncation_readonly(dir_fd, BlockingMode::Blocking);
85+
test_file_truncation_readonly(dir_fd, BlockingMode::NonBlocking);
86+
}
87+
}
Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
use test_programs::wasi::filesystem::preopens;
2+
use test_programs::wasi::filesystem::types::{
3+
Descriptor, DescriptorFlags, ErrorCode, OpenFlags, PathFlags,
4+
};
5+
6+
const FILENAME: &str = "test.txt";
7+
fn test_file_has_expected_contents(dir: &Descriptor) {
8+
// Open a file for reading
9+
let file = dir
10+
.open_at(
11+
PathFlags::empty(),
12+
FILENAME,
13+
OpenFlags::empty(),
14+
DescriptorFlags::READ,
15+
)
16+
.expect("open test.txt for reading");
17+
18+
// Read the file's contents
19+
let stream = file.read_via_stream(0).unwrap();
20+
let read = stream.blocking_read(100).expect("reading test.txt content");
21+
drop(stream);
22+
drop(file);
23+
24+
const EXPECTED_CONTENTS: &[u8] = b"truncation test file\n";
25+
// The file should not be empty due to truncation
26+
assert_eq!(read, EXPECTED_CONTENTS, "expected untouched file contents");
27+
}
28+
29+
fn test_file_truncation_readonly(dir: &Descriptor) {
30+
// Check test preconditions.
31+
test_file_has_expected_contents(dir);
32+
33+
// Opening the file for truncation should fail.
34+
let err = dir.open_at(
35+
PathFlags::empty(),
36+
FILENAME,
37+
OpenFlags::TRUNCATE,
38+
DescriptorFlags::READ,
39+
);
40+
assert!(err.is_err(), "opening file for truncation should fail");
41+
assert_eq!(
42+
err.err().unwrap(),
43+
ErrorCode::NotPermitted,
44+
"opening file for truncation should fail with ErrorCode::NotPermitted"
45+
);
46+
47+
// Check that truncation did not occur.
48+
test_file_has_expected_contents(dir);
49+
}
50+
51+
fn main() {
52+
// This test program requires a special preopen at the path "readonly",
53+
// which the host enforces as read-only. Unlike other test programs, this
54+
// directory's path not passed in as an argument, because modifications to
55+
// the testing harness would be too invasive.
56+
let preopens = preopens::get_directories();
57+
let (dir, _) = preopens
58+
.iter()
59+
.find(|(_, path)| path == "readonly")
60+
.expect("find preopen named readonly");
61+
62+
// Run the test
63+
test_file_truncation_readonly(dir);
64+
}

crates/wasi-common/tests/all/async_.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -291,3 +291,9 @@ async fn p1_path_open_lots() {
291291
async fn p1_sleep_quickly_but_lots() {
292292
run(P1_SLEEP_QUICKLY_BUT_LOTS, true).await.unwrap()
293293
}
294+
#[test]
295+
fn p1_file_truncation_readonly() {
296+
println!(
297+
"blank placeholder test to satisfy assert_test_exists. This test exercises wasmtime-wasi functionality is not relevant to wasi-common"
298+
);
299+
}

crates/wasi-common/tests/all/sync.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -285,3 +285,9 @@ fn p1_path_open_lots() {
285285
fn p1_sleep_quickly_but_lots() {
286286
run(P1_SLEEP_QUICKLY_BUT_LOTS, true).unwrap()
287287
}
288+
#[test]
289+
fn p1_file_truncation_readonly() {
290+
println!(
291+
"blank placeholder test to satisfy assert_test_exists. This test exercises wasmtime-wasi functionality is not relevant to wasi-common"
292+
);
293+
}

crates/wasi/src/filesystem.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -966,6 +966,7 @@ impl Dir {
966966

967967
if oflags.contains(OpenFlags::TRUNCATE) {
968968
opts.truncate(true).write(true);
969+
open_mode |= OpenMode::WRITE;
969970
}
970971
if flags.contains(DescriptorFlags::READ) {
971972
opts.read(true);

0 commit comments

Comments
 (0)