3

So, it's been just over eleven years since a Debian maintainer infamously caused RNG seeds to become predictable by commenting out a usage of uninitialized data.

The issue generated a lot of heated discussion in online circles, with most of the focus seemingly being on criticizing the review process or attacking the developers in question.

However, I haven't been able to find any information on the actual thought process behind the segment being there in the first place. A lot of users argue that "worst case, it can't hurt" - however, that seems utterly counter-intuitive to me.

After all, reading from uninitialized memory invokes undefined behavior which, infamously, can cause nasal demons, run nethack or format your hard drive. Thus, it seems to me that introducing this sort of logic into any program - let alone a crypto library - puts you one aggressive compiler optimization away from utter disaster.

Thus, my question(s):

  • Am I misunderstanding something? Is there a reason why this is actually not invoking UB, but well-defined under the standard?
  • If it does in fact invoke UB, why was this behavior originally included in OpenSSL?
jww
  • 97,681
  • 90
  • 411
  • 885
Treeston
  • 390
  • 1
  • 7
  • 1
    I wasn't involved, so regard this as moderately well informed speculation. There's the theoretical UB (for which nasal demons are one possible consequence) and the practical UB (you get somewhat unpredictable results). It wasn't a good use of UB, but the practical result of using it was that things were more random before than after the fix. It was a case of broken code being 'fixed' with more but differently broken code — and it wasn't obvious at first glance that the second lot of code was also broken. – Jonathan Leffler Jul 30 '17 at 01:19
  • See also [nasal demons](https://stackoverflow.com/a/45395334/15168). – Jonathan Leffler Jul 30 '17 at 01:52
  • It might also be worth mentioning the Debian folks asked the OpenSSL folks if it was OK to do it. The problem was, they did not get a straight Yes/No answer. Getting a straight answer from some dev-teams is like trying to get an honest answer from a lawyer or pin jello to a wall. – jww Jul 30 '17 at 07:47

2 Answers2

3

The Debian "fix" was completely incorrect. ssleay_rand_add was the only function adding entropy to the pool. The real culprit was in the site of invocation, and it should have been fixed there - as there was nothing wrong with the function itself.

So basically it was the case of:

void add_entropy(void *buf, size_t length) {
    actually_add_entropy(buf, length);
}

int main(void) {
    char buf[256];

    // uninitialized local variable
    add_entropy(buf, length);

    // calculate more entropy to buf
    // ...
    add_entropy(buf, length);
}

If you look at the code, the problem is that the argument passed in in the first invocation is the real culprit, the second case is OK, as the buf was now initialized. The proper fix is not to "fix" add_entropy as follows:

void add_entropy(void *buf, size_t length) {
    // valgrind complains about buf being uninitialized so
    // actually_add_entropy(buf, length);
}

However they were right in attempting to remove the use of uninitialized array - UB and USB aside, it might have been masking serious problems in the code - it might have very well been that the entropy been initialized only from the uninitialized data and therefore possibly still guessable or controllable - but only this time harder to notice.

Antti Haapala -- Слава Україні
  • 129,958
  • 22
  • 279
  • 321
  • So the buffer wasn't always uninitialized, just in some invocations - and they stopped it being added in _all_ invocations? That would explain why "removing one source of entropy" actually meant removing (almost) _all_ sources of entropy. – Treeston Jul 30 '17 at 16:38
  • @Treeston exactly. In `ssh-keygen` only the *initial* value, the process id of the running process was used to seed the random generator. – Antti Haapala -- Слава Україні Jul 30 '17 at 17:31
  • Sadly, from the Crypto Engineering point of view, there were two high level failures. First, users were not using the library correctly because they did not seed the generator. OpenSSL's wiki now tries to be very clear they should seed the generator. Second, the library was easy to use incorrectly, and more difficult to correctly. I believe this was addressed by pulling seeds from OS generators. The generators still mix-in things like time and PIDs, but they are not the only source of entropy. The changes make the generators easier to use correctly. – jww Jul 30 '17 at 18:03
1

It wasn't, and just happened (one manifestation of nasal demons) to work well enough. Debian was right to remove the UB. OpenSSL was wrong to have such broken code to begin with.

R.. GitHub STOP HELPING ICE
  • 208,859
  • 35
  • 376
  • 711