Fix handling of malicious Readers in read_to_end

[sfackler]

A malicious Read impl could return overly large values from read, which would result in the guard's drop impl setting the buffer's length to greater than its capacity! To fix this, the drop impl now uses the safe truncate function instead of set_len which ensures that this will not happen. The result of calling the function will be nonsensical, but that's fine given the contract violation of the Read impl.

The Guard type is also used by append_to_string which does not pass untrusted values into the length field, so I've copied the guard type into each function and only modified the one used by read_to_end. We could just keep a single one and modify it, but it seems a bit cleaner to keep the guard code close to the functions and related specifically to them.

To fix this, we now assert that the returned length is not larger than the buffer passed to the method.

For reference, this bug has been present for ~2.5 years since 1.20: ecbb896.

Closes #80894.

[rust-highfive]

r? @shepmaster

(rust-highfive has picked a reviewer for you, use r? to override)

[rust-log-analyzer]

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

    Checking addr2line v0.14.0
error[E0282]: type annotations needed
   --> library/std/src/io/mod.rs:381:9
    |
381 |     let ret;
    |         ^^^ consider giving `ret` a type
error: aborting due to previous error

For more information about this error, try `rustc --explain E0282`.
error: could not compile `std`
error: could not compile `std`

To learn more, run the command again with --verbose.
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--color" "always" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/checkout/library/test/Cargo.toml" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check
Build completed unsuccessfully in 0:01:46

[Qwaz]

Could we check for an overflow here?

[sfackler]

We could, but I would rather keep the codepath oriented as much as possible towards non-malicious implementations. Overflow can only be caused by bogus return values, and just means that a nonsensical result will be returned.

[Qwaz]

Malicious Read can shrink g.len with an overflow, which in turn can lead to a violation of append_to_string()'s second requirement. (and append_to_string() should probably be marked as an unsafe function)

rust/library/std/src/io/mod.rs

Lines 320 to 329 in c97f11a

	// The unsafety in this function is twofold:
	//
	// 1. We're looking at the raw bytes of `buf`, so we take on the burden of UTF-8
	// checks.
	// 2. We're passing a raw buffer to the function `f`, and it is expected that
	// the function only *appends* bytes to the buffer. We'll get undefined
	// behavior if existing bytes are overwritten to have non-UTF-8 data.
	fn append_to_string<F>(buf: &mut String, f: F) -> Result<usize>
	where
	F: FnOnce(&mut Vec<u8>) -> Result<usize>,

[Qwaz]

This is the context:

rust/library/std/src/io/mod.rs

Lines 707 to 718 in c97f11a

	fn read_to_string(&mut self, buf: &mut String) -> Result<usize> {
	// Note that we do *not* call `.read_to_end()` here. We are passing
	// `&mut Vec<u8>` (the raw contents of `buf`) into the `read_to_end`
	// method to fill it up. An arbitrary implementation could overwrite the
	// entire contents of the vector, not just append to it (which is what
	// we are expecting).
	//
	// To prevent extraneously checking the UTF-8-ness of the entire buffer
	// we pass it to our hardcoded `read_to_end` implementation which we
	// know is guaranteed to only read data into the end of the buffer.
	append_to_string(buf, |b| read_to_end(self, b))
	}

[sfackler]

Ah right. We'll need to replace that with a checked_add then.

This kind of mess is one of the things that the ReadBuf proposal is designed to avoid, so things should get less janky when that's implemented.

[sfackler]

Updated to assert that the returned length is actually valid for the input buffer.

[nagisa]

Would it make sense to slice the slice that's been passed into the read call in the first place and then take its length? As in:

let buffer = &mut g.buf[g.len..];
match r.read(buffer) { 
    // ...
    Ok(n) => g.len += buffer[..n].len(),
}

or something along those lines? It has an added benefit of also detecting when returned n is greater than the length of the length of the buffer provided, and may result in slightly less code generated overall.

[sfackler]

This check already is looking for n being greater than the length of the buffer, but agreed that structuring it like that is a bit more clear. Will update later today.

[sfackler]

r? @m-ou-se

[sfackler]

cc @Mark-Simulacrum not sure if you think we should to backport to beta?

[Mark-Simulacrum]

I think that seems fine - it's not really necessary IMO given this has been like this for a long time now, but giving it's a soundness fix seems appropriate. I'm going to unilaterally beta-accept but will wait till it lands on master before backporting.

[sfackler]

Yeah, makes sense to me, particularly since the patch is so small.

[m-ou-se]

@bors r+

[bors]

📌 Commit e6c07b0 has been approved by m-ou-se