Uninitialized memory

[carllerche]

This is very true. A concrete example is mio. I plan on adding a read_buf function (see https://github.com/carllerche/mio/blob/mio-reform/src/io.rs#L40). If Rust adds Read::read_buf, it would definitely break any code that uses mio + Read (which would be most mio code)

[nikomatsakis]

If a method of a different trait has the name of the newly added function, then name resolving will fail if an objects implements both traits, both are in scope and the function is called.

Sorry, I should have been more clear. We’ve been meaning to write up an RFC/guidelines documenting what kinds of changes are permitted, and the plan was to permit adding methods to traits. There is some risk of ambiguity arising, yes, which can be resolved by clients in a trivial way using UFCS. This would be one of a small number of “permitted” compilation failures that can accompany a minor release. (Similar hazards arise in other languages as well.) Otherwise our ability to grow APIs would be very hampered.

[alexcrichton]

Accidentally reading from uninitialized memory is very easy and have the potential of having very serious implications (see heart bleed, but it is probably one of main vulnerability vectors). If Rust decides that it is ok to leak uninitialized memory into safe Rust code, I would consider that a serious hit against the advantages that Rust provides.

I would personally like to keep concerns as concrete as possible. 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?

First of all, it’s not an and situation, it is an or situation

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.

Now, the exact same argument could be made against the borrow checker.

I see the borrow checker as quite distinct from this. The borrow checker is preventing me from segfaulting. Initializing a byte buffer does not prevent me from segfaulting.

Perhaps it is reasonable that security-sensitive code would wind up with its own set of abstractions. (In particular I don’t know how much of an additional burden this would be on top of whatever add’l requirements security-sensitive code already has.)

I have not personally written much security-sensitive code, but I would expect a ZeroVec<T> which is largely what Vec<T> is today except that Drop is implemented differently by dropping each individual element and then zeroing out the entire allocation to ensure all data is erased (or perhaps a ZeroBox<T> abstraction as well). Again though, I do not personally know if this is sufficient (and would be curious if others know whether it would be).

It just doesn’t feel like the stdlib should be in the business of exposing uninitialized memory without an unsafe keyword

I personally find it hard to buy into this though without concrete evidence saying that we shouldn’t (e.g. justifying the 20-25% loss in performance for read_to_end). I, too, am somewhat uneasy about it but I am unable to convince myself that we’re wrong for exposing uninitialized byte buffers.

[iopq]

While Heartbleed was an out-of bounds buffer access, if I recall correctly. But if a Rust server has some kind of vulnerability with uninitialized buffers, it would be potentially just as bad.

I don’t believe it’s possible to do this kind of thing in Java, for example.

[nikomatsakis]

I have not personally written much security-sensitive code, but I would expect a ZeroVec<T> which is largely what Vec<T> is today except that Drop is implemented differently by dropping each individual element and then zeroing out the entire allocation to ensure all data is erased (or perhaps a ZeroBox<T> abstraction as well). Again though, I do not personally know if this is sufficient (and would be curious if others know whether it would be).

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.

[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.