Safe methods in std::io allow reading uninit memory

[sivadeilra]

\src\libstd\io\mod.rs contains two methods that allow attacking code to read memory that it should otherwise not have access to. The methods are Reader.push() and Reader.push_at_least(). An attacker could write (or exploit) an implementation of Reader, by implementing a read() method that reads from the given buffer, rather than writing to it. Or by not writing at all, returning a non-zero byte count, and then calling push() and seeing what memory was returned.

The push() and push_at_least() methods should probably just be deleted entirely. The support function slice_vec_capacity() could also be deleted.

[kmcallister]

Nominating. Regardless of whether this technically violates Rust's memory safety guarantees, it's extremely surprising and has major security impact.

[huonw]

It seems this was already noticed and the functions are suggested for deletion when std::io gets overhauled for 1.0: https://github.com/aturon/rfcs/blob/io-os-reform/text/0000-io-os-reform.md#removed-methods

[pythonesque]

As a side note this feels like what unsafe traits would be useful for.

[aturon]

This was discussed as part of the IO reform RFC process, and it is believed not to lead to undefined behavior, at least if we follow a certain pattern of intrinsics use. Reading an uninitialized but alloced block of u8 data is type and memory safe.

There doesn't seem to be widespread agreement elsewhere (on IRC, etc) as to how serious a security risk this poses. However, note that we can close this potential hole easily by pre-zeroing the memory in these convenience functions, without causing any API breakage.

It would be helpful to see a more detailed example of an exploit based on the current behavior.

[sivadeilra]

I'm frankly astonished. There is no way my team will accept Rust with this
kind of information leakage within its "safe" subset.

How can you make any claim about type safety and security, with such a
gaping hole?

On Thu, Feb 12, 2015 at 2:07 PM, Aaron Turon notifications@github.com
wrote:

This was discussed as part of the IO reform RFC process, and it is
believed not to lead to undefined behavior, at least if we follow a
certain pattern of intrinsics use. Reading an uninitialized but alloced
block of u8 data is type and memory safe.

There doesn't seem to be widespread agreement elsewhere (on IRC, etc) as
to how serious a security risk this poses. However, note that we can close
this potential hole easily by pre-zeroing the memory in these convenience
functions, without causing any API breakage.

It would be helpful to see a more detailed example of an exploit based on
the current behavior.

—
Reply to this email directly or view it on GitHub
#20314 (comment).

[aturon]

@sivadeilra

First, I'm sorry if I wasn't clear, but no decision has been made here. I was asking for clarification.

I was trying to clarify first of all that there is not a type or memory safety hole here, see this comment thread. If you believe this can lead to a type or memory safety violation, can you please spell out the details? (cc @mahkoh)

Second, I was hoping you could spell out in more concrete detail an exploit based on this behavior alone. It would require a Reader implementation to read from the buffer it's supposed to write into, afaict.

As I said in my comment, we always have the option of zeroing the memory in these convenience methods, which doesn't require any API change.

[kmcallister]

I'm basically with @sivadeilra here. This needs to be fixed for 1.0.

The exploit seems clear. The memory you read in read is uninitialized, fresh from the allocator. It will contain whatever was there last — passwords, encryption keys (yeah, you should zero these on drop, but...), addresses of internal structures useful for defeating ASLR, etc.

You may not be worried about malicious read impls but even an inept one may leak information. Exploits for systems like kernels or sandboxed browers are very complex. They often combine information leaks from a number of "minor" bugs.

... not lead to undefined behavior, at least if we follow a certain pattern of intrinsics use. Reading an uninitialized but alloced block of u8 data is type and memory safe.

Sure, it's not UB. I'll even accept that it's not a "type and memory safety" issue. But this is a major lurking security risk in a language that otherwise goes out of its way to prevent you from reading uninitialized memory.

[aturon]

@kmcallister

The exploit seems clear. The memory you read in read is uninitialized, fresh from the allocator. It will contain whatever was there last — passwords, encryption keys (yeah, you should zero these on drop, but...), addresses of internal structures useful for defeating ASLR, etc.

But a read implementation doesn't read from the buffer, it writes into it. It would have to be very inept indeed to read from the buffer.