[Pkg-openssl-devel] Valgrind patch leftovers

[Ernst-Udo Wallenborn]

Luca Bruno writes:

>Hi,
>I'm not sure if it has been already discussed here, but I see that
>after the latest Valgrind related problem, not the whole patch has been
>dropped [0].

Hi,

i'm curious about that, too. I looked into the issue on a debian system

me at debian-vm: ~$ uname -a
Linux debian-vm 3.2.0-4-amd64 #1 SMP Debian 3.2.46-1 x86_64 GNU/Linux

both with the stock openssl and with a fresh build of the library, sources obtained using

me at debian-vm:~$ sudo apt-get -b source openssl

Eric Wong's proof-of-concept mentioned by Martin Boßlet on his blog [1] consists of a parent process that calls RAND_bytes once and then starts forking child processes until the system begins to recycle pids. The first child prints its pid and 4 bytes from the rnd stream, and a while later the next child that gets the same pid does the same. The program then exits. With Debian's libssl the output is:

me at debian-vm:~$ gcc -o random random.c -lssl
me at debian-vm:~$ ./random
pid=4904 \x12\x78\x03\x3b
pid=4904 \x12\x78\x03\x3b

So both children produce the same sequence of random numbers. Debian seems to be the only linux to do that, I tried it on a SLES 11p2 and on an OpenSuSE and got differing random number sequences. From SLES sources I can tell that they don't apply anything like the valgrind.patch. When I rebuild the Debian openssl library without the valgrind.patch, I get

me at debian-vm:~$ ./random
pid=5419 \x81\xc7\x69\x6f
pid=5419 \x83\xa4\x39\x8c

as expected.

Now I wasn't around in 2008 when the problem first surfaced, so I don't know exactly what the fix was, but from what I read on Slashdot and elsewhere, I think the issue was that MD_Update was flagged by valgrind, and commented out in two places, ssleay_rand_bytes and then again in ssleay_rand_add. Afaik the latter was what caused the fracas, so it was removed in 2008, and the code now carries a big red warning sign (md_rand.c:279):

                               /* DO NOT REMOVE THE FOLLOWING CALL TO MD_Update()! */
                               MD_Update(&m,buf,j);
                               /* We know that line may cause programs such as
                                  purify and valgrind to complain about use of
                                  uninitialized data.  The problem is not, it's
                                  with the caller.  Removing that line will make
                                  sure you get really bad randomness and thereby
                                  other problems such as very insecure keys. */

                               MD_Update(&m,(unsigned char *)&(md_c[0]),sizeof(md_c));




The former, though, stayed in the code, slightly modified (#if 0 instead of a straight comment), and that is the valgrind.patch of today.

Index: openssl-1.0.0c/crypto/rand/md_rand.c
===================================================================
--- openssl-1.0.0c.orig/crypto/rand/md_rand.c   2010-06-16 15:17:22.000000000 +0
200
+++ openssl-1.0.0c/crypto/rand/md_rand.c        2010-12-12 17:02:50.000000000 +0
100
@@ -476,6 +476,7 @@
                MD_Update(&m,(unsigned char *)&(md_c[0]),sizeof(md_c));
 #ifndef PURIFY /* purify complains */
+#if 0
                /* The following line uses the supplied buffer as a small
                 * source of entropy: since this buffer is often uninitialised
                 * it may cause programs such as purify or valgrind to
@@ -485,6 +486,7 @@
                 */
                MD_Update(&m,buf,j);
#endif
+#endif
                k=(st_idx+MD_DIGEST_LENGTH/2)-st_num;
                if (k > 0)



Why is this a problem? Well, in Eric's code, the parent calls RAND_bytes *before* the first fork, causing ssleay_rand_bytes to call RAND_poll (md_rand.c:392), thus initializing the RNG:

                if (!initialized)
                               {
                               RAND_poll();
                               initialized = 1;
                               }
.
As a side effect, initialized=1 blocks RAND_poll from *ever being called again*. Now the parent forks, and the children call RAND_bytes again. Now ssleay_rand_bytes skips RAND_poll, updates the entropy pool with the PID, and then there is only one other chance for individual entropy, the MD_Update call that valgrind.patch just removed. Result: all children with the same PID get the same random numbers. If the parent hadn't called ssleay_rand_bytes each child would call RAND_poll individually, and all would be well.

My twocent is: Commenting out the MD_Update call in ssleay_rand_bytes is just as incorrect, the problem just shows up less often than the original bug, that's it why took a while before someone stumbled upon it [2]. Valgrind.patch should be removed.


Ernst-Udo Wallenborn



[1] http://emboss.github.io/blog/2013/08/21/openssl-prng-is-not-really-fork-safe/
[2] https://bugs.ruby-lang.org/issues/4579


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.alioth.debian.org/pipermail/pkg-openssl-devel/attachments/20130829/64d2ea91/attachment.html>