Uninitialized memory

[carllerche]

For example I think it is unfair to say that this is akin to heartbleed as I am under the impression that it resulted as a result of an out-of-bounds access, not reading uninitialized data (please correct me if I’m wrong though).

You say that you strongly believe that Rust should protect you from doing this, but can you clarify why? What cases are you worried about?

The reason why I compared it to heart bleed is that you probably heard of it AND the outcome of reading from uninitialized memory is for all intents and purposes the same.

“Security-sensitive code” is not a small subset of code. Any bit of code that is potentially exposed to an attacker is “security-sensitive.” This includes a significant chunk of code that is written (and virtually all server code). A “read from uninitialized memory” bug can be exploited to leak secret keys, etc…

For example, writing a server app that returns signed cookies? An attacker could exploit an uninitialized memory read bug to access the secret key and then be able to get full access to the system. The list goes on.

I think this all hinges on your later statement that the buffer abstraction is not “API overhead”. I personally disagree because we do not have a write-only buffer and it would take time to stabilize and rationalize with existing read methods (aka what @aturon and @nikomatsakis have been talking about). This to me definitely seems somewhat of an overhead as it must be considered.

The buffer API can be punted post 1.0. Zeroing memory in the read_to_end / read_to_string functions are acceptable trade-offs for 1.0. The performance hit is not huge and those two functions in question should not be used in performance sensitive situations anyway.

Honestly, I have always thought that Rust would have protected me against reading from uninitialized memory, and the fact that this is in question is (still) very surprising to me given how bad the outcome of such bugs can be.

[NawfelBengherbia]

Seriously people! I know that you have promised, but it’s not 1.0 yet. Just break backwards compatibility!

+1 for the buffer object. It allows for more optimizations than slices.

[glaebhoerl]

Is there any contention about the point that the problem can be solved performantly and safely with new abstractions like &out, or even just library types in the existing language as suggested by @carllerche and @reem? Because if that’s the case, then the choices appear to boil down to:

1. Some use cases written in a certain way will be somewhat slower out of gate from 1.0, up until we add those abstractions (which we can make our first priority post-1.0), and after that point they can be both fast and safe.

2. They will be fast right from the start, but Rust’s safety and security guarantees will be weaker forever.

In other words, a temporary performance hit versus a permanent safety/security hit. This doesn’t feel like a difficult decision to me.

(This is not about the meaning of the words “safe” and “secure” in a boolean sense, like where exactly you would draw the line - rather about stronger vs. weaker guarantees, on a continuum.)

[nikomatsakis]

In other words, a temporary performance hit versus a permanent safety/security hit. This doesn’t feel like a difficult decision to me.

I’m inclined to agree. Perhaps a compromise is simply to say that we intend to enforce the “no leaking access to uninitialized memory” guideline so long as we can, but we’ll possibly revisit this question in the future as new APIs arise. In judicial terms, basically we set a narrow precedent here, since it is clear we can solve this case but perhaps there are other cases that will be less clear that we are not thinking of.

[carllerche]

Well, a guideline would require that an API that leaks uninitialized memory is marked as unsafe.

[comex]

I’m basically just restating what’s been already said in the thread, but -

The thing about reading uninitialized memory is that it only takes one such read to potentially get at any freed data, which breaks what is otherwise a promise that avoids a whole class of bugs in safe code - the promise that freeing data makes it go away. For some especially sensitive pieces of data, such as passwords, it is advisable to zero out the memory after use anyway, as existing C applications do, because the kernel might somehow leak that memory to other processes; but it is impractical to do this for all data which might be valuable to an attacker (which in networked applications could be just about anything - any private data stored on behalf of users), and attacks against the kernel are usually local only, so in general that attack scenario is far less scary than direct leaks by the application. Heartbleed is quite relevant whether or not that specific bug was caused by reading from an uninitialized buffer: it is an example of how dangerous information disclosure can be even if it’s not in the traditional ‘arbitrary code execution’ vulnerability category.

I don’t think the overhead of stabilizing a buffer API post 1.0, and temporary slowdown before that, is very significant compared to that risk. I agree that a narrow precedent would be best, but I can’t actually think of any scenarios, where uninitialized memory access is truly required to avoid a significant usability or performance overhead, that aren’t already highly unsafe; can anyone else?

[bascule]

Just wanted to +1 these comments by comex:

The thing about reading uninitialized memory is that it only takes one such read to potentially get at any freed data, which breaks what is otherwise a promise that avoids a whole class of bugs in safe code

I don’t think the overhead of stabilizing a buffer API post 1.0, and temporary slowdown before that, is very significant compared to that risk.

I strongly agree with these sentiments (and as a separate issue, Rust should definitely stabilize the volatile_set_memory intrinsic!). Reading uninitialized memory poses a huge security danger, and I’d hate to expose that danger to the safe subset of Rust “for speed” if there are better alternatives which can still provide safety.

The buffer traits proposed by @carllerche seem like the best path forward to me. They emphasize safety while still providing a good foundation for performance in the future.

[llogiq]

So to sum this up, we have three possible solutions:

1. Leave things as they are. The security minded folks gnaw their teeth, but what’s the worst that could happen between consenting adult implementors? Performance would be good, but valgrind may show strange things.

2. Zero out all memory on allocation (or perhaps at program start and on drop). This would imply a serious performance hit, but keep the secutiry folks happy. However, Rust is about zero-cost abstraction, so I’m quite sure this is not the right way.

3. Leave it to the type system to figure it out (e.g. wrap to disallow reads). No performance hit, no valid insecure code (as long as the library defines the right signature), benefits in other areas (e.g. OpenGL).

I for one am inclined to give my vote to option 3, but I don’t know what it’s implementation would entail.

[tbu]

consenting adult implementors

What could possibly happen between consenting adult implementors of something in C? This is an area where Rust tries to improve.

[llogiq]

I may have missed putting a <sarcasm> tag somewhere.

Note that option 3 (which is basically what reem proposed) would shift the responsibility for keeping the buffer write-only to the caller (because it would need to wrap the buffer in a write-only type before handing it to the callee).

Why? I don’t think it is feasible (as of now) to fully implement this scheme for all allocations. This would mean that allocated but uninitialized memory would be WriteOnly until initialization. It would then need to automatically change its type to ReadWrite upon writing to it. There are fairly good heuristics to deal with a large range of this (e.g. see how FindBugs tracks NullPointerExceptions in Java code), but the problem remains unsolvable for the general case. Also things like entropy pools (which are, admittedly, not that common) would then need another escape hatch.

Oh, another thing regarding safety: While it is true that reading from uninitialized memory could potentially be risky, not cleaning security-relevant data from memory is the real culprit. If you have a scenario where an attacker is already running code within your process, the compiler cannot keep them from combing through the heap.

[nikomatsakis]

Zero out all memory on allocation (or perhaps at program start and on drop). This would imply a serious performance hit, but keep the secutiry folks happy. However, Rust is about zero-cost abstraction, so I’m quite sure this is not the right way.

This is not about zeroing all memory on allocation. Most of the time, we immediately initialize memory after allocating it, and zeroing is not necessary. It’s only in the case of the helper fn read_to_end and similar patterns, where we want to allocate a buffer and then gradually write into it, where problems arise.

[vojtechkral]

Right, that’s what I was talking about. Probably security-sensitive code would want to practice security in depth, meaning that they would want to use ZeroVec and not have safe code being given access to uninitialized data.

Sidenote/FYI: for example Botan (a C++ crypto lib) has a vector type like that. Besides zeroing memory before freeing it also tries to lock the memory in RAM so that it cannot be swapped out, that’s another issue. See http://botan.randombit.net/manual/secmem.html if you’re interested in that…

[llogiq]

Ok, so the implications are less pronounced. Also this would just include one API function that allocates, but doesn’t initialize memory. Could a lint be produced that checks if the resulting memory of this API function is read before initialization?

Yes, I know that it’s intractable in general, but I’d assume that usage would occur fairly linear (those could be checked to reliably produce an error by a kind of bounds-check), and more complex usages could issue a warning.

This could be done in a backwards-compatible way, would allow more correct programs than the WriteOnly implementation (I’m not yet sure if this is a good thing) and won’t even require type system shenanigans.

Still, the OpenGL use case is a strong motivator for implementing WriteOnly anyway.

[bascule]

@vojtechkral there’s already a pretty nice library for memory protected buffers in Rust:

seb-m/tars

[llogiq]

Taking a step back and thinking about easy solutions, I came up with the idea of using a marker type + lint for this, like:

fn read(&mut self, buf: &mut [u8] + WriteOnly) -> Result<usize>;

The lint would then need to get a typed AST and err on any deref outside of an lvalue. The marker type would not even need to be generic, and could be defined now, even if we haven’t implemented the lint yet.

If that works, it would obviate the need to have a wrapper which would either be specific to one type, e.g. &mut [u8] (which would limit its function to byte buffers), or would need to be generic over the wrapped type (which, barring HKT could entail its own set of problems).

Would that be feasible or do I give Rust’s type system too much credit? Did I get the syntax right? How hard is it to get a typed AST within a lint?

Edit: No, that apparently does not work, because we cannot add trait bounds to concrete types. Too bad. It would have been a cool possibility.

[tbu]

This also doesn’t work because it’d have to check that the return value matches the number of bytes written.

[llogiq]

Then a wrapper trait that ensures the invariants (perhaps even by API design) would be the right solution. The interface could ensure that the right thing is done.

// would need to keep a pointer to the current head, could also do len-check assertions
fn append(data ; &[u8], bytes : usize)

// get the bytes written
fn bytes_written()

If needed, a Deref / DerefMut implementation could use asserts to ensure that only already written bytes are deref’d.

For the OpenGL use case, perhaps some struct could implement DerefMut, but not Deref? Otherwise, a lint could check that the struct is not Deref’d.

[gnzlbg]

I want to contribute a real world example of reading uninitialized data from multiple implementations of the MPI standard in C.

On initialization, MPI processes spawn other processes. Before spawning the children procs, each process allocates a fixed-size buffer of run-time size (same for all processes). Without going into details, this buffer is larger than the amount of data to be sent and contains the bounds of the data actually sent. The process fills it partially with data, and sends it over the network, including the uninitialized data. This is faster than just sending the data that is initialized. On the receiver process end, the bounds are read, and only the initialized part of the buffer is read.

Two things happen. On the reader process, valgrind complains on the read of uninitialized memory for sending it over the network. On the receiver end, valgrind does not complain if the child process reads out of bounds, because the out-of-bounds part of the buffer has been rightfully initialized, albeit with garbage. valgrind, of course, doesn’t know that.

A lot of people have argued about zeroing the buffer for removing this spurious valgrind error (on the sender end). It was, however, profiled than zeroing the buffer had a non-acceptable cost, and valgrind suppression files are provided instead.

[DaGenix]

This seems kinda related to https://github.com/rust-lang/rfcs/issues/926, at least in the respect that unsafe code that relies on safe code doing the right thing can result in memory safety violations.

[rkjnsn]

I agree that we should try to prevent reading from uninitialized memory in safe code, and we should leverage the type system to do so.

I want to point out that &out is not what we want, here (read would be required to fully initialize the buffer before returning). Also, the OpenGL example and the read_to_end example want different things. The same solution will not work for both. read_to_end needs to ensure that a fully safe read doesn’t look at any uninitialized bytes from the buffer (reading bytes that have already been written is theoretically fine, but probably not necessary), writes data into the buffer sequentially, and correctly returns the number of bytes written to the buffer (otherwise uninitialized bytes could end up in the final vector).

It seems like one would want something like this (modulo optimizations):

struct OutBuf<'a> {
    buf: &'a mut[u8],
    len: usize,
}

impl<'a> OutBuf<'a> {
    fn push(&mut self, byte: u8) { self.buf[self.len] = byte; self.len += 1; }
    fn push_all(&mut self, slice: &[u8]) { for &byte in slice {self.push(byte);} }
    fn capacity(&mut self)->usize { self.buf.len() }
    fn len(&mut self)->usize { self.len }

    unsafe fn as_mut_ptr(&mut self)->*mut u8 { self.buf.as_mut_ptr() }
    unsafe fn set_len(&mut self, len: usize) { self.len = len; }
}

For (my understanding of) the OpenGL example, you want something very different: code can write anywhere in the buffer, it can never read from the buffer, and there’s no need to record how many bytes have been written.