3 ms·

Yes, this misunderstanding does seem to be very popular. No, that's what was going on here, the OpenSSL code had two places where it copies of a bunch of bytes,

Yes, this misunderstanding does seem to be very popular. No, that's what was going on here, the OpenSSL code had two places where it copies of a bunch of bytes, and one of those copies can end up with uninitialized junk, so that's wrong. Somebody wrote a patch which fixes this, and then - even without assistance from an LLM just using normal human incompetence, somebody said "There's another similar copy nearby, we should get rid of that one too" and Debian landed the patch to apply both changes. The result is now OpenSSL doesn't copy any bytes. It doesn't copy the uninitialized data, which is good, and it also doesn't copy the truly random entropy into the pool. Oops.

orra 3y ago
> and one of those copies can end up with uninitialized junk, so that's wrong. To be fair to the Debian patch, reading from uninitialized junk doesn't sprinkle randomness into the desired buffer: it invokes undefined behaviour. The Debian patch was lawful!
- tedunangst 3y ago
  The part where they also zeroed the buffer with intentionally random data may have been lawful, but it wasn't good.
ptx 3y ago
From Russ Cox's post[0] about this, it sounds like the place in the code that initializes the buffer with actual random data also uses the parts of the buffer that haven't been initialized. So there weren't two separate pieces of code, one copying junk and one copying actual random entropy, but rather "at least three different places" that did both, if I've understood this correctly. [0] https://research.swtch.com/openssl https://research.swtch.com/openssl
- tialaramex 3y ago
  Cox had the advantage (I presume) when writing this of looking at the original source, for which all the links I found are long dead (because presumably this code moved to git years later) and he's a smart guy so we should definitely believe what he describes over my recollections. But I think you're slightly wrong here, I think Cox is describing three cases where OpenSSL takes a bunch of bytes and says here you go entropy pool, here's some potential entropy to add, if it wasn't really entropy that's fine, stir it in anyway. A. Buffers you've provided and are asking to pour random data into. OpenSSL thinks this is clever - best case the data is unpredictable and it helps, worst case it's just zeroes and the entropy pool will handle that anyway. Valgrind and similar tools will be very angry when you do this to actual uninitialized buffers, which in C is really easy to write and so likely to be common. This is the case that already had a comment about such problems and is the second of two places Debian removed. B. Buffers OpenSSL tried to put entropy from the Linux random device into. OpenSSL doesn't bother checking that the randomness went into the buffer, after all if it didn't the buffer is uninitialized... This is the first of the two places Debian removed (in terms of line number). This is where "Debian Weak Keys" come from because now the "random" numbers are not random. In almost any real world scenario this code was unproblematic although very stupid, until it was patched. C. A Buffer read from a file the user provided with entropy in it. This only ends up with uninitialized data if the file shrinks while OpenSSL is trying to read it, which can happen but would probably be some sort of attack. Again though OpenSSL doesn't care, Debian did not patch this code AFAIK but very few people care about it. Edited to add: One thing I really like in the discussion below the actual post you linked is the idea that the OpenSSL people are thinking the Debian dev wants to debug their software and are saying it's fine to comment out the randomness in order to help them do debugging not realising the developer actually intends to remove the randomness because they believe it's a bug. When you re-read the mailing list posts with this understanding and forgetting the Debian Weak Keys bugs, it snaps into place - we can't know if that's really what happened of course, but it makes sense.

orra 3y ago

> and one of those copies can end up with uninitialized junk, so that's wrong. To be fair to the Debian patch, reading from uninitialized junk doesn't sprinkle randomness into the desired buffer: it invokes undefined behaviour. The Debian patch was lawful!

tedunangst 3y ago
The part where they also zeroed the buffer with intentionally random data may have been lawful, but it wasn't good.

ptx 3y ago

From Russ Cox's post[0] about this, it sounds like the place in the code that initializes the buffer with actual random data also uses the parts of the buffer that haven't been initialized. So there weren't two separate pieces of code, one copying junk and one copying actual random entropy, but rather "at least three different places" that did both, if I've understood this correctly. [0] https://research.swtch.com/openssl https://research.swtch.com/openssl

tialaramex 3y ago
Cox had the advantage (I presume) when writing this of looking at the original source, for which all the links I found are long dead (because presumably this code moved to git years later) and he's a smart guy so we should definitely believe what he describes over my recollections. But I think you're slightly wrong here, I think Cox is describing three cases where OpenSSL takes a bunch of bytes and says here you go entropy pool, here's some potential entropy to add, if it wasn't really entropy that's fine, stir it in anyway. A. Buffers you've provided and are asking to pour random data into. OpenSSL thinks this is clever - best case the data is unpredictable and it helps, worst case it's just zeroes and the entropy pool will handle that anyway. Valgrind and similar tools will be very angry when you do this to actual uninitialized buffers, which in C is really easy to write and so likely to be common. This is the case that already had a comment about such problems and is the second of two places Debian removed. B. Buffers OpenSSL tried to put entropy from the Linux random device into. OpenSSL doesn't bother checking that the randomness went into the buffer, after all if it didn't the buffer is uninitialized... This is the first of the two places Debian removed (in terms of line number). This is where "Debian Weak Keys" come from because now the "random" numbers are not random. In almost any real world scenario this code was unproblematic although very stupid, until it was patched. C. A Buffer read from a file the user provided with entropy in it. This only ends up with uninitialized data if the file shrinks while OpenSSL is trying to read it, which can happen but would probably be some sort of attack. Again though OpenSSL doesn't care, Debian did not patch this code AFAIK but very few people care about it. Edited to add: One thing I really like in the discussion below the actual post you linked is the idea that the OpenSSL people are thinking the Debian dev wants to debug their software and are saying it's fine to comment out the randomness in order to help them do debugging not realising the developer actually intends to remove the randomness because they believe it's a bug. When you re-read the mailing list posts with this understanding and forgetting the Debian Weak Keys bugs, it snaps into place - we can't know if that's really what happened of course, but it makes sense.