bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems

[Paul Eggert <eggert@cs.ucla.edu>]

[-- Attachment #1: Type: text/plain, Size: 2551 bytes --]

Eli Zaretskii wrote:
> I wish we'd discuss such things before committing and not after.

Sorry, I missed the part of the discussion that talked about reading 
/dev/urandom in the first place.

>    . I see nothing wrong with having 2 (or more) independent reads from
>      /dev/urandom:

It's one more thing to worry about when auditing an Emacs trace. Also, it's two 
file descriptors (both open at the same time) when we can get by with just one.

>      . GnuTLS is a separate library, so it's free to do that for its
>        own purposes; we shouldn't care.

Yes and no. Yes, we shouldn't care how GnuTLS gets entropy; and no, because if 
GnuTLS is available we should be better off using its entropy source than 
rolling our own. The GnuTLS guys are far more expert in this stuff; why reinvent 
the wheel? And if the GnuTLS entropy source is busted, Emacs is already insecure 
in dozens of important ways, so using that source here shouldn't make matters 
significantly worse.

>	 Besides, what if tomorrow
>        there will be a 3rd library that would need to access
>        /dev/urandom?

Not our problem. As you say, libraries are free to do that for their own 
purposes, and we shouldn't care.

>    . GnuTLS is a library for TLS, not for cryptography.

GnuTLS is not just for TLS, it's for secure communications. Getting a nonce is a 
basic building block for such a library. They're not going to remove a basic 
building block.

>      What if tomorrow GnuTLS changes its implementation?

That's fine. We don't need to know the details of how GnuTLS gets its nonces. 
For example, if it starts using the RDRAND instruction available on Ivy Bridge 
and later Intel processors, more power to them. We shouldn't care.

>    . This change means that we now load GnuTLS at startup, even if no
>      TLS connections are or will be used.

That's already true on GNU and POSIXish platforms, so it's not a problem there. 
It is an issue on MS-Windows, though, so your change to avoid GnuTLS here on 
MS-Windows makes sense.

> Why is it suddenly a concern that users will know that we use time and
> PID as fallback?

Merely because we're in the neighborhood anyway and it's the first time I 
noticed that this detail was documented. The detail doesn't belong in the 
documentation; Emacs shouldn't promise that it'll use the PID, for example.

One other thing, while we're nearby: the doc shouldn't assume that readers know 
that time-of-day etc. is less random.

How about the attached patch? It should address these documentation concerns.

[-- Attachment #2: t.diff --]
[-- Type: text/x-diff, Size: 1316 bytes --]

diff --git a/doc/lispref/numbers.texi b/doc/lispref/numbers.texi
index 3a9483a..28eb6b1 100644
--- a/doc/lispref/numbers.texi
+++ b/doc/lispref/numbers.texi
@@ -1252,9 +1252,9 @@ Random Numbers
 (@pxref{Integer Basics}).
 
 If @var{limit} is @code{t}, it means to choose a new seed as if Emacs
-were restarting.  The new seed will be set from the system entropy, if
-that is available, or from the current time and Emacs process's PID
-(@pxref{System Environment, emacs-pid}) if not.
+were restarting, typically from the system entropy.  On systems
+lacking entropy pools, choose the seed from less-random volatile data
+such as the current time.
 
 If @var{limit} is a string, it means to choose a new seed based on the
 string's contents.
diff --git a/src/fns.c b/src/fns.c
index 19fa440..86ad333 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -51,7 +51,7 @@ and `most-positive-fixnum', inclusive, are equally likely.
 
 With positive integer LIMIT, return random number in interval [0,LIMIT).
 With argument t, set the random number seed from the system's entropy
-pool, or from the current time and pid if entropy is unavailable.
+pool if available, otherwise from less-random volatile data such as the time.
 With a string argument, set the seed based on the string's contents.
 Other values of LIMIT are ignored.