File implementation on Windows has unsound methods

[jstarks]

File's read, write, seek_read, and seek_write all have soundness issues in Windows under some conditions. In the below, I describe the problem and possible fixes for seek_read and read; the same problems and solutions apply to seek_write and write.

seek_read

Using std::os::windows::fs::OpenOptionsExt::custom_flags, we can open a File with FILE_FLAG_OVERLAPPED, which is a Win32 flag that indicates that IO should be performed asynchronously using OVERLAPPED structures to track the state of outstanding IO.

We can then use std::os::windows::fs::FileExt::seek_read to initiate an IO. Internally, this uses ReadFile and specifies an OVERLAPPED structure on the stack in order to specify the offset to read from. If the IO does not completely synchronously in the kernel, this call will return the error ERROR_IO_PENDING, indicating that the buffer and the OVERLAPPED structure are still referenced by the kernel. The kernel can write to these buffers at any time, but seek_read has already returned so the lifetime of both objects has expired.

This example shows how

#[test]
fn unsound() {
    use std::os::windows::prelude::*;
    const FILE_FLAG_OVERLAPPED: u32 = 0x40000000;
    const ERROR_IO_PENDING: i32 = 997;

    let f = std::fs::OpenOptions::new()
        .custom_flags(FILE_FLAG_OVERLAPPED)
        .read(true)
        .open("con") // Open the console to guarantee the IO will pend.
        .unwrap();

    let mut buf = [0; 1];
    // If this assertion fails, then the IO is still in flight has references to
    // `buf` and to an already-gone `OVERLAPPED` value that was on the stack.
    assert_ne!(
        f.seek_read(&mut buf, 0)
            .unwrap_err()
            .raw_os_error()
            .unwrap(),
        ERROR_IO_PENDING
    )
}

read

This can also be achieved with read, too, but it is harder to set up. read uses ReadFile without passing an OVERLAPPED structure, so ReadFile allocates one on the stack and handles ERROR_IO_PENDING internally by waiting on the file object for an IO to complete. However, ReadFile can still return with the IO in flight if there are multiple reads outstanding concurrently, since the IO completion notification will not necessarily wake up the right caller. Worse, this condition is not detectable--ReadFile returns a successful completion in this case (though with zero bytes written).

To demonstrate this, we need some unsafe code to create a named pipe server so that we can control IO completion timing. But this could be in a separate process or via an otherwise safe named pipe wrapper, so this does not mitigate the soundness issue in read itself. If you ignore this aspect, the result is more dramatic since we can demonstrate the unsound behavior in the form of a buffer magically changing contents across a sleep:

#[test]
fn unsound_2() {
    use std::io::{Read, Write};
    use std::os::windows::prelude::*;
    use std::sync::Arc;
    use std::thread::sleep;
    use std::time::Duration;

    const FILE_FLAG_OVERLAPPED: u32 = 0x40000000;

    fn create_pipe_server(path: &str) -> std::fs::File {
        let mut path0 = path.as_bytes().to_owned();
        path0.push(0);
        extern "system" {
            fn CreateNamedPipeA(
                lpName: *const u8,
                dwOpenMode: u32,
                dwPipeMode: u32,
                nMaxInstances: u32,
                nOutBufferSize: u32,
                nInBufferSize: u32,
                nDefaultTimeOut: u32,
                lpSecurityAttributes: *mut u8,
            ) -> RawHandle;
        }

        unsafe {
            let h = CreateNamedPipeA(
                "\\\\.\\pipe\\repro\0".as_ptr(),
                3,
                0,
                1,
                0,
                0,
                0,
                std::ptr::null_mut(),
            );
            assert_ne!(h as isize, -1);
            std::fs::File::from_raw_handle(h)
        }
    }

    let path = "\\\\.\\pipe\\repro";
    let mut server = create_pipe_server(path);

    let client = Arc::new(
        std::fs::OpenOptions::new()
            .custom_flags(FILE_FLAG_OVERLAPPED)
            .read(true)
            .open(path)
            .unwrap(),
    );

    let spawn_read = |is_first: bool| {
        std::thread::spawn({
            let f = client.clone();
            move || {
                let mut buf = [0xcc; 1];
                let mut f = f.as_ref();
                f.read(&mut buf).unwrap();
                if is_first {
                    assert_ne!(buf[0], 0xcc);
                } else {
                    let b = buf[0]; // capture buf[0]
                    sleep(Duration::from_millis(200));
                    assert_eq!(buf[0], b, "whoa-oaaa, buf changed after sleeping!");
                }
            }
        })
    };

    let t1 = spawn_read(true);
    sleep(Duration::from_millis(20));
    let t2 = spawn_read(false);
    sleep(Duration::from_millis(100));
    server.write(b"x").unwrap();
    sleep(Duration::from_millis(100));
    server.write(b"y").unwrap();
    t1.join().unwrap();
    t2.join().unwrap();
}

Possible fixes

seek_read

There are multiple ways to fix seek_read. The options I can think of: if ReadFile fails with ERROR_IO_PENDING:

1. Call GetOverlappedResult() in a loop until it succeeds or fails with something other than ERROR_IO_INCOMPLETE, or;
2. Call GetOverlappedResult() once. If this fails with ERROR_IO_INCOMPLETE, call abort(), or;
3. Call abort().

My recommendation is to implement option 2. This is what ReadFile does if you don't pass an OVERLAPPED structure and so is what read does today (except ReadFile doesn't abort, it just returns--more about that below). Option 1 sounds appealing but it can easily lead to deadlocks due to some peculiarities about how IO completion signaling works in Windows. Fixing this would rely on Rust allocating a separate Windows event object for each thread issuing IO--I'm not certain this is worth the overhead.

We also need to fix read. Since it calls ReadFile without an OVERLAPPED structure, it can hit this same condition but with the kernel-referenced OVERLAPPED structure (actually an IO_STATUS_BLOCK at this layer of the stack) already popped of the stack. And since ReadFile returns success in this case, there's no way to reliably detect this and abort after the fact. You could argue this is a bug in Windows, but it's a bit late to fix it, especially in all the in-market releases. Calling ReadFile without an OVERLAPPED structure is not really safe unless you carefully control the handle's source.

To fix it, it's tempting to switch to allocating the OVERLAPPED structure on the stack and passing it to ReadFile, as is done in seek_read. Unfortunately, this is not possible to do with perfect fidelity. When you pass an OVERLAPPED structure, you have to provide an offset, but for read we want the current file offset. When a file is opened without FILE_FLAG_OVERLAPPED, -2 means to write at the current file offset, so we could pass that. But when the file is opened with FILE_FLAG_OVERLAPPED, there is no current file position (in this case read only makes sense for streams, such as named pipes, where the offset is meaningless). In this case, -2 is considered an invalid value by the kernel, so we would probably want to pass 0 for most cases. Since we don't know which case we're in, we're stuck.

But ReadFile doesn't know what case it's in, either, so how does it solve this problem? Simple: it calls the lower-level system call NtReadFile, which allows NULL to be passed for the offset. This has exactly the behavior we want. read should be updated to use NtReadFile directly.

As an alternative to all this, OpenOptionsExt::custom_flags could strip or otherwise prohibit the use of FILE_FLAG_OVERLAPPED, but I suspect this would be a breaking change. This interface is probably used in various places to open async file handles for other uses (via into_raw_handle()). I'm pretty sure I've written such code. And of course using (unsafe) from_raw_handle it's still possible to get a File whose underlying file object has this flag set, so there's still a trap set for unsuspecting devs.

Sorry this is such a mess. The Windows IO model has some rough edges. We (the Windows OS team) have tried to smooth some of them out over the releases, but doing so while maintaining app compat has proven challenging.

Meta

cargo +nightly --version --verbose:

cargo 1.51.0-nightly (a73e5b7d5 2021-01-12)
release: 1.51.0
commit-hash: a73e5b7d567c3036b296fc6b33ed52c5edcd882e
commit-date: 2021-01-12

This issue has been assigned to @sivadeilra via this comment.

[KamilaBorowska]

I think this is one of those cases where breaking backwards compatibility after fixing mio to use CreateFileW instead of OpenOptions would make sense.

[jstarks]

I'm not so sure. I think there are reasonable Rust programs, perhaps yet to be written, that will necessarily want to accept file handles from untrusted sources, possibly controlled by an attacker, via some IPC mechanism. A reasonable thing for such a program to do would be to take the handle and create a File from it using from_raw_handle. If an attacker can cause memory corruption in the target process, this type of program is harder to write safely in Rust.

The safety requirements for from_raw_handle mention sole ownership of the handle as a requirement, but they do not mention anything about the properties of the handle that's being consumed.

[KodrAus]

Thanks for the detailed report @jstarks! We discussed this in our recent Libs meeting and after a quick survey found that passing FILE_FLAG_OVERLAPPED to OpenOptions::custom_flags is a common enough pattern in the wild that we couldn't try catch it there without breaking legitimate use-cases.

We felt the best path forward would be effectively option 2, which is also what you're recommending:

Call GetOverlappedResult() once. If this fails with ERROR_IO_INCOMPLETE, call abort()

We were also comfortable using NtReadFile instead of ReadFile, knowing that backwards compatibility for the lower-level APIs is best-effort (but Windows normally bends over pretty hard to maintain it).

[apiraino]

Assigning P-critical as it requires attention. Discussed as part of the Prioritization Working Group procedure and removing I-prioritize.

[nikomatsakis]

I just wanted to make folks in the windows notification group aware of this, it seems like everything is in hand:

@rustbot ping windows

[rustbot]

Hey Windows Group! This bug has been identified as a good "Windows candidate".
In case it's useful, here are some instructions for tackling these sorts of
bugs. Maybe take a look?
Thanks! <3

cc @arlosi @danielframpton @gdr-at-ms @kennykerr @luqmana @lzybkr @nico-abram @retep998 @rylev @sivadeilra

[nikomatsakis]

@rustbot assign @sivadeilra

[camelid]

Assigning P-critical as it requires attention. Discussed as part of the Prioritization Working Group procedure and removing I-prioritize.

@apiraino You removed I-nominated instead of I-prioritize. Removing I-prioritize and adding I-nominated now: @rustbot label: +I-nominated -I-prioritize