* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2015-12-18 10:05 bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems Demetri Obenour
@ 2015-12-18 10:46 ` Eli Zaretskii
2015-12-29 15:36 ` Richard Copley
` (5 subsequent siblings)
6 siblings, 0 replies; 48+ messages in thread
From: Eli Zaretskii @ 2015-12-18 10:46 UTC (permalink / raw)
To: Demetri Obenour; +Cc: 22202
> From: Demetri Obenour <demetriobenour@gmail.com>
> Date: Fri, 18 Dec 2015 05:05:09 -0500
>
>
> 1. Be logged into the same Windows computer as someone else.
> 2. Have a process running that is notified whenever a process starts up
> 3. Have them run `emacs --daemon' or invoke `server-start'.
> 4. Use the knowledge of the current time and the server's PID to guess
> the authentication key.
> 5. Connect to the other user's Emacs server.
> 6. Execute arbitrary code with the other user's privileges.
Please provide the necessary details for reproducing this problem and
verifying the solution. What I'm missing:
> 1. Be logged into the same Windows computer as someone else.
How do you do that? I understand you are describing a situation where
2 users are logged into the same Windows system simultaneously using
the same credentials, is that true? If so, how to create such a
situation?
> 2. Have a process running that is notified whenever a process starts up
> 3. Have them run `emacs --daemon' or invoke `server-start'.
> 4. Use the knowledge of the current time and the server's PID to guess
> the authentication key.
I don't think we use the current time and PID for that, but even if we
do, how do you get a hold of the time at the moment of the server
creation to nanosecond resolution? Please tell how to do that.
Thanks.
^ permalink raw reply [flat|nested] 48+ messages in thread* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2015-12-18 10:05 bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems Demetri Obenour
2015-12-18 10:46 ` Eli Zaretskii
@ 2015-12-29 15:36 ` Richard Copley
2015-12-29 16:21 ` Eli Zaretskii
2016-01-17 20:26 ` Paul Eggert
` (4 subsequent siblings)
6 siblings, 1 reply; 48+ messages in thread
From: Richard Copley @ 2015-12-29 15:36 UTC (permalink / raw)
To: Eli Zaretskii, Demetri Obenour, 22202
> Please provide the necessary details for reproducing this problem and
> verifying the solution. What I'm missing:
>
> > 1. Be logged into the same Windows computer as someone else.
>
> How do you do that? I understand you are describing a situation where
> 2 users are logged into the same Windows system simultaneously using
> the same credentials, is that true? If so, how to create such a
> situation?
I don't think that is possible; however, two /different/ accounts can
be logged in to a computer at the same time, via Remote Desktop or
Fast User Switching. (If the computer is a Remote Desktop server then
two users can be simultaneously interacting with their desktops, in
separate sessions. That's not at all uncommon in a business
environment, but I don't think it's relevant to this question.)
> > 2. Have a process running that is notified whenever a process starts up
> > 3. Have them run `emacs --daemon' or invoke `server-start'.
> > 4. Use the knowledge of the current time and the server's PID to guess
> > the authentication key.
>
> I don't think we use the current time and PID for that, but even if we
> do, how do you get a hold of the time at the moment of the server
> creation to nanosecond resolution? Please tell how to do that.
We use function "random" (see function "server-generate-key"); its
seed is typically set at startup using the current time and PID (see
"init_random()" in sysdep.c), so it's the time Emacs started that you
would want to know, not the time the server started. You can get the
start time (to the nearest second at least) and PID of any user's
processes using, e.g., Process Explorer.
I'm not sure what resolution timestamp we end up using as the seed.
gettime() might return microsecond timestamps in certain configurations.
I can't speak for Demetri but it seems to me he's imagining an attacker
who is prepared to use a certain amount of brute force. Knowing or
guessing the Emacs start time within a few seconds would reduce the
search space.
^ permalink raw reply [flat|nested] 48+ messages in thread* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2015-12-29 15:36 ` Richard Copley
@ 2015-12-29 16:21 ` Eli Zaretskii
2015-12-29 17:44 ` Richard Copley
0 siblings, 1 reply; 48+ messages in thread
From: Eli Zaretskii @ 2015-12-29 16:21 UTC (permalink / raw)
To: Richard Copley; +Cc: 22202, demetriobenour
> Date: Tue, 29 Dec 2015 15:36:12 +0000
> From: Richard Copley <rcopley@gmail.com>
>
> > Please provide the necessary details for reproducing this problem and
> > verifying the solution. What I'm missing:
> >
> > > 1. Be logged into the same Windows computer as someone else.
> >
> > How do you do that? I understand you are describing a situation where
> > 2 users are logged into the same Windows system simultaneously using
> > the same credentials, is that true? If so, how to create such a
> > situation?
>
> I don't think that is possible; however, two /different/ accounts can
> be logged in to a computer at the same time, via Remote Desktop or
> Fast User Switching.
Logging in via Remote Desktop usurps the system, AFAIK. So these
possibilities are not relevant to the issue at hand.
> > > 2. Have a process running that is notified whenever a process starts up
> > > 3. Have them run `emacs --daemon' or invoke `server-start'.
> > > 4. Use the knowledge of the current time and the server's PID to guess
> > > the authentication key.
> >
> > I don't think we use the current time and PID for that, but even if we
> > do, how do you get a hold of the time at the moment of the server
> > creation to nanosecond resolution? Please tell how to do that.
>
> We use function "random" (see function "server-generate-key"); its
> seed is typically set at startup using the current time and PID (see
> "init_random()" in sysdep.c), so it's the time Emacs started that you
> would want to know, not the time the server started. You can get the
> start time (to the nearest second at least) and PID of any user's
> processes using, e.g., Process Explorer.
You need the time to nanosecond resolution to compute the seed. How
do you do that?
> I'm not sure what resolution timestamp we end up using as the seed.
> gettime() might return microsecond timestamps in certain configurations.
On MS-Windows, gettime calls gettimeofday, which returns the system
clock in 100 nanosecond units. The actual resolution of the clock is
between 1 ms and 10 ms, but I think it's still an impossible task to
get the exact time we sample the clock during startup with such a high
accuracy.
> I can't speak for Demetri but it seems to me he's imagining an attacker
> who is prepared to use a certain amount of brute force. Knowing or
> guessing the Emacs start time within a few seconds would reduce the
> search space.
As I said, I don't see how such a user could even get access to a
machine without my paying attention. And that if the services
required for remote access have not been turned off to begin with.
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2015-12-29 16:21 ` Eli Zaretskii
@ 2015-12-29 17:44 ` Richard Copley
2015-12-29 20:00 ` David Engster
0 siblings, 1 reply; 48+ messages in thread
From: Richard Copley @ 2015-12-29 17:44 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 22202, Demetri Obenour
On 29 December 2015 at 16:21, Eli Zaretskii <eliz@gnu.org> wrote:
>> Date: Tue, 29 Dec 2015 15:36:12 +0000
>> From: Richard Copley <rcopley@gmail.com>
>>
>> > Please provide the necessary details for reproducing this problem and
>> > verifying the solution. What I'm missing:
>> >
>> > > 1. Be logged into the same Windows computer as someone else.
>> >
>> > How do you do that? I understand you are describing a situation where
>> > 2 users are logged into the same Windows system simultaneously using
>> > the same credentials, is that true? If so, how to create such a
>> > situation?
>>
>> I don't think that is possible; however, two /different/ accounts can
>> be logged in to a computer at the same time, via Remote Desktop or
>> Fast User Switching.
>
> Logging in via Remote Desktop usurps the system, AFAIK. So these
> possibilities are not relevant to the issue at hand.
That is definitely not correct. In some configurations several users
can connect via remote desktop. I do this every day. It /might/ be
necessary to have a "Professional" and/or Server edition of Windows.
A licensed Terminal Server supports dozens of sessions at once.
Fast User Switching is a different thing. (Type CTRL-ALT-DEL and click
"Switch User".) That, too, might require "Professional".
>> > > 2. Have a process running that is notified whenever a process starts up
>> > > 3. Have them run `emacs --daemon' or invoke `server-start'.
>> > > 4. Use the knowledge of the current time and the server's PID to guess
>> > > the authentication key.
>> >
>> > I don't think we use the current time and PID for that, but even if we
>> > do, how do you get a hold of the time at the moment of the server
>> > creation to nanosecond resolution? Please tell how to do that.
>>
>> We use function "random" (see function "server-generate-key"); its
>> seed is typically set at startup using the current time and PID (see
>> "init_random()" in sysdep.c), so it's the time Emacs started that you
>> would want to know, not the time the server started. You can get the
>> start time (to the nearest second at least) and PID of any user's
>> processes using, e.g., Process Explorer.
>
> You need the time to nanosecond resolution to compute the seed. How
> do you do that?
I haven't tried, but the MSDN docs for GetProcessTimes say it returns the
start time in 100 ns units. I'd guess that's what Process Explorer uses.
>> I'm not sure what resolution timestamp we end up using as the seed.
>> gettime() might return microsecond timestamps in certain configurations.
>
> On MS-Windows, gettime calls gettimeofday, which returns the system
> clock in 100 nanosecond units. The actual resolution of the clock is
> between 1 ms and 10 ms, but I think it's still an impossible task to
> get the exact time we sample the clock during startup with such a high
> accuracy.
Perhaps you don't need to. Brute force. (Maybe that's ridiculous. I haven't
tried to do the sums. Trying 100 to 1000 different values doesn't sound too
hard.)
>> I can't speak for Demetri but it seems to me he's imagining an attacker
>> who is prepared to use a certain amount of brute force. Knowing or
>> guessing the Emacs start time within a few seconds would reduce the
>> search space.
>
> As I said, I don't see how such a user could even get access to a
> machine without my paying attention.
With respect, that's not correct (explained above).
> And that if the services
> required for remote access have not been turned off to begin with.
Yes obviously, but many organizations do have Remote Desktop
servers their staff can (or must) connect to.
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2015-12-29 17:44 ` Richard Copley
@ 2015-12-29 20:00 ` David Engster
2015-12-29 21:22 ` Richard Copley
0 siblings, 1 reply; 48+ messages in thread
From: David Engster @ 2015-12-29 20:00 UTC (permalink / raw)
To: Richard Copley; +Cc: 22202, Demetri Obenour
Richard Copley writes:
> On 29 December 2015 at 16:21, Eli Zaretskii <eliz@gnu.org> wrote:
>>> Date: Tue, 29 Dec 2015 15:36:12 +0000
>>> From: Richard Copley <rcopley@gmail.com>
>>>
>
>>> > Please provide the necessary details for reproducing this problem and
>>> > verifying the solution. What I'm missing:
>>> >
>>> > > 1. Be logged into the same Windows computer as someone else.
>>> >
>>> > How do you do that? I understand you are describing a situation where
>>> > 2 users are logged into the same Windows system simultaneously using
>>> > the same credentials, is that true? If so, how to create such a
>>> > situation?
>>>
>>> I don't think that is possible; however, two /different/ accounts can
>>> be logged in to a computer at the same time, via Remote Desktop or
>>> Fast User Switching.
>>
>> Logging in via Remote Desktop usurps the system, AFAIK. So these
>> possibilities are not relevant to the issue at hand.
>
> That is definitely not correct. In some configurations several users
> can connect via remote desktop. I do this every day. It /might/ be
> necessary to have a "Professional" and/or Server edition of Windows.
> A licensed Terminal Server supports dozens of sessions at once.
That's correct (it requires a Windows Server with enabled terminal
services), but each user session has of course its own process space, so
I don't see how the described attack could work there.
-David
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2015-12-29 20:00 ` David Engster
@ 2015-12-29 21:22 ` Richard Copley
2015-12-29 22:02 ` David Engster
2015-12-30 15:58 ` Eli Zaretskii
0 siblings, 2 replies; 48+ messages in thread
From: Richard Copley @ 2015-12-29 21:22 UTC (permalink / raw)
To: David Engster; +Cc: 22202, Demetri Obenour
>> [...]
>
> That's correct (it requires a Windows Server with enabled terminal
> services), but each user session has of course its own process space, so
> I don't see how the described attack could work there.
Not sure what you mean by process space. As an unprivileged user
you can find other users' Emacs processes without any effort (using
tasklist.exe, for example). If you know on what port an Emacs server
is listening (which is admittedly a difficulty), you can send bytes to it.
I've just done so as an experiment. (I was driving both sessions so I
knew the server port.)
I haven't reproduced the whole attack scenario and I don't pretend
know whether it could work. I don't claim any expertise in software
security. I just wanted to help out by answering Eli's questions.
To get back to the OP's main point, given that we already go to the
trouble of creating this secret, it wouldn't hurt to do it better (on all
systems, for preference). On Windows it really doesn't seem hard.
Sorry, no patch, for legal reasons, but there's a simple example on
the MSDN page for CryptGenRandom.
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2015-12-29 21:22 ` Richard Copley
@ 2015-12-29 22:02 ` David Engster
2015-12-29 23:13 ` Richard Copley
2015-12-30 15:58 ` Eli Zaretskii
1 sibling, 1 reply; 48+ messages in thread
From: David Engster @ 2015-12-29 22:02 UTC (permalink / raw)
To: Richard Copley; +Cc: 22202, Demetri Obenour
Richard Copley writes:
>>> [...]
>>
>> That's correct (it requires a Windows Server with enabled terminal
>> services), but each user session has of course its own process space, so
>> I don't see how the described attack could work there.
>
> Not sure what you mean by process space. As an unprivileged user
> you can find other users' Emacs processes without any effort (using
> tasklist.exe, for example). If you know on what port an Emacs server
> is listening (which is admittedly a difficulty), you can send bytes to it.
> I've just done so as an experiment. (I was driving both sessions so I
> knew the server port.)
You logged in with two different user accounts? I always thought
sessions from different users were better isolated from one another and
more similar to Linux containers. If that is not the case, then I agree
the attack scenario looks feasible.
-David
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2015-12-29 21:22 ` Richard Copley
2015-12-29 22:02 ` David Engster
@ 2015-12-30 15:58 ` Eli Zaretskii
2015-12-30 20:47 ` Richard Copley
1 sibling, 1 reply; 48+ messages in thread
From: Eli Zaretskii @ 2015-12-30 15:58 UTC (permalink / raw)
To: Richard Copley; +Cc: 22202, demetriobenour, deng
> Date: Tue, 29 Dec 2015 21:22:55 +0000
> From: Richard Copley <rcopley@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, 22202@debbugs.gnu.org,
> Demetri Obenour <demetriobenour@gmail.com>
>
> I haven't reproduced the whole attack scenario and I don't pretend
> know whether it could work. I don't claim any expertise in software
> security. I just wanted to help out by answering Eli's questions.
>
> To get back to the OP's main point, given that we already go to the
> trouble of creating this secret, it wouldn't hurt to do it better (on all
> systems, for preference). On Windows it really doesn't seem hard.
> Sorry, no patch, for legal reasons, but there's a simple example on
> the MSDN page for CryptGenRandom.
Can you audit the patch below? I know next to nothing about
cryptography, and I'm not sure I understood all the flags involved in
these APIs.
Also, generating random numbers with these APIs is significantly
slower -- about 2.5 msec per number, as opposed to something like
175 microsec using 'rand'. Should we perhaps provide an option
(by default off) to force using the old, weaker, but faster method?
Thanks.
--- src/w32.c~0 2015-11-29 06:48:07.000000000 +0200
+++ src/w32.c 2015-12-30 17:48:19.297251800 +0200
@@ -224,6 +224,8 @@ typedef struct _REPARSE_DATA_BUFFER {
#include <iphlpapi.h> /* should be after winsock2.h */
+#include <wincrypt.h>
+
#include <c-strcase.h>
#include "w32.h"
@@ -2093,9 +2095,34 @@ init_user_info (void)
CloseHandle (token);
}
+static HCRYPTPROV w32_crypto_hprov;
+static int
+w32_init_crypt_random (void)
+{
+ if (!CryptAcquireContext (&w32_crypto_hprov, NULL, NULL, PROV_RSA_FULL,
+ CRYPT_VERIFYCONTEXT | CRYPT_SILENT))
+ {
+ DebPrint (("CryptAcquireContext failed with error %x\n",
+ GetLastError ()));
+ return -1;
+ }
+ return 0;
+}
+
int
random (void)
{
+ if (w32_crypto_hprov)
+ w32_init_crypt_random ();
+ if (w32_crypto_hprov)
+ {
+ const DWORD nbytes = 4; /* see RAND_BITS in sysdep.c */
+ BYTE rand_buffer[nbytes];
+
+ if (CryptGenRandom (w32_crypto_hprov, nbytes, rand_buffer))
+ return *(int *)&rand_buffer[0];
+ }
+ /* Else fall back on rand () */
/* rand () on NT gives us 15 random bits...hack together 30 bits. */
return ((rand () << 15) | rand ());
}
@@ -2103,6 +2130,18 @@ random (void)
void
srandom (int seed)
{
+ if (!w32_crypto_hprov)
+ w32_init_crypt_random ();
+ if (w32_crypto_hprov)
+ {
+ const DWORD nbytes = 4; /* see RAND_BITS in sysdep.c */
+ BYTE buf[nbytes];
+
+ memcpy (buf, &seed, sizeof buf);
+ CryptGenRandom (w32_crypto_hprov, nbytes, buf);
+ }
+ /* Always seed rand () as well, in case some future call to
+ CryptGenRandom fails and we need to fall back to rand () */
srand (seed);
}
@@ -9386,6 +9425,8 @@ globals_of_w32 (void)
extern void dynlib_reset_last_error (void);
dynlib_reset_last_error ();
#endif
+
+ w32_crypto_hprov = (HCRYPTPROV)0;
}
/* For make-serial-process */
^ permalink raw reply [flat|nested] 48+ messages in thread* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2015-12-30 15:58 ` Eli Zaretskii
@ 2015-12-30 20:47 ` Richard Copley
2015-12-30 20:56 ` Richard Copley
` (2 more replies)
0 siblings, 3 replies; 48+ messages in thread
From: Richard Copley @ 2015-12-30 20:47 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 22202, Demetri Obenour, David Engster
> Can you audit the patch below? I know next to nothing about
> cryptography, and I'm not sure I understood all the flags involved in
> these APIs.
Sure! But please bear in mind I'm not experienced in crypto
either.
With regard to API usage: The call to CryptAcquireContext looks good
to me. (The comment about interoperability in the documentation for
the parameter "pszProvider" does not apply as we are not inter-
operating with anything. Setting "pszContainer" to NULL, as you have
done, is explicitly recommended. The docs for the individual flags
entail the very value of "dwFlags" that you use.) I can see nothing
else to comment on.
Re performance: using CryptGenRandom to provide a seed for srand
is enough to address Demetri's concern. For performance reasons,
as you said, implementing random() with CryptGenRandom is
potentially bad. I think random() itself should not be changed.
That said, rand() makes me uncomfortable (mostly because of bugs in
long-gone implementations, and superstition). Given the chance I would
replace it with an xorshift* generator. The generator at [1] seeded
with 64 bits from CryptGenRandom should give good performance for
random() and (I guess!) an effectively unassailable server secret.
But I have no good reason to claim that rand() is not good enough.
Thank you Eli.
[1]: https://en.wikipedia.org/w/index.php?title=Xorshift&oldid=697235156#xorshift.2A
^ permalink raw reply [flat|nested] 48+ messages in thread* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2015-12-30 20:47 ` Richard Copley
@ 2015-12-30 20:56 ` Richard Copley
2015-12-30 20:56 ` Eli Zaretskii
2015-12-31 17:04 ` Demetrios Obenour
2 siblings, 0 replies; 48+ messages in thread
From: Richard Copley @ 2015-12-30 20:56 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 22202, Demetri Obenour, David Engster
Ah, I forgot to mention one other thing that had occurred to me.
It might not be a good idea to pass the current time to CryptGenRandom
for the optional initial seed. The current time (in various forms) is
already used as seed entropy by the system, and it's conceivable
(though implausible) that we could be destroying entropy by doing
this. It's probably better (and "acceptable" according to the
documentation) not to pass any seed at all.
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2015-12-30 20:47 ` Richard Copley
2015-12-30 20:56 ` Richard Copley
@ 2015-12-30 20:56 ` Eli Zaretskii
2015-12-30 21:15 ` Richard Copley
2015-12-31 17:04 ` Demetrios Obenour
2 siblings, 1 reply; 48+ messages in thread
From: Eli Zaretskii @ 2015-12-30 20:56 UTC (permalink / raw)
To: Richard Copley; +Cc: 22202, demetriobenour, deng
> Date: Wed, 30 Dec 2015 20:47:40 +0000
> From: Richard Copley <rcopley@gmail.com>
> Cc: David Engster <deng@randomsample.de>, 22202@debbugs.gnu.org,
> Demetri Obenour <demetriobenour@gmail.com>
>
> Re performance: using CryptGenRandom to provide a seed for srand
> is enough to address Demetri's concern. For performance reasons,
> as you said, implementing random() with CryptGenRandom is
> potentially bad. I think random() itself should not be changed.
That'd be the worst of both worlds, IMO: a not-so-good PRNG with no
way whatsoever to get a repeatable sequence. Am I right?
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2015-12-30 20:56 ` Eli Zaretskii
@ 2015-12-30 21:15 ` Richard Copley
2015-12-31 14:14 ` Eli Zaretskii
0 siblings, 1 reply; 48+ messages in thread
From: Richard Copley @ 2015-12-30 21:15 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 22202, Demetri Obenour, David Engster
> That'd be the worst of both worlds, IMO: a not-so-good PRNG with no
> way whatsoever to get a repeatable sequence. Am I right?
Oh dear. Yes. Worse than that, repeatability is part of the contract for
the lisp "random" function, so seed_random() and get_random() are
constrained to be deterministic. Right?
Seems as though init_random() is the only place that could use
CryptGenRandom, which is a pity if you're trying to confine the
changes to w32.c.
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2015-12-30 21:15 ` Richard Copley
@ 2015-12-31 14:14 ` Eli Zaretskii
0 siblings, 0 replies; 48+ messages in thread
From: Eli Zaretskii @ 2015-12-31 14:14 UTC (permalink / raw)
To: Richard Copley; +Cc: 22202, demetriobenour, deng
> Date: Wed, 30 Dec 2015 21:15:13 +0000
> From: Richard Copley <rcopley@gmail.com>
> Cc: David Engster <deng@randomsample.de>, 22202@debbugs.gnu.org,
> Demetri Obenour <demetriobenour@gmail.com>
>
> > That'd be the worst of both worlds, IMO: a not-so-good PRNG with no
> > way whatsoever to get a repeatable sequence. Am I right?
>
> Oh dear. Yes. Worse than that, repeatability is part of the contract for
> the lisp "random" function, so seed_random() and get_random() are
> constrained to be deterministic. Right?
>
> Seems as though init_random() is the only place that could use
> CryptGenRandom, which is a pity if you're trying to confine the
> changes to w32.c.
How about the following, then?
(Not sure it's a good idea to read from /dev/random, as that could
block; perhaps we should use /dev/urandom instead.)
--- src/sysdep.c~2 2015-11-11 07:57:41.000000000 +0200
+++ src/sysdep.c 2015-12-31 16:09:46.987229300 +0200
@@ -2095,8 +2095,35 @@ seed_random (void *seed, ptrdiff_t seed_
void
init_random (void)
{
- struct timespec t = current_timespec ();
- uintmax_t v = getpid () ^ t.tv_sec ^ t.tv_nsec;
+ uintmax_t v;
+ struct timespec t;
+ bool success = false;
+
+#if HAVE_DEV_RANDOM
+ FILE *fp = fopen ("/dev/random", "rb");
+
+ if (fp)
+ {
+ int i;
+
+ for (i = 0, v = 0; i < sizeof (uintmax_t); i++)
+ {
+ v <<= 8;
+ v |= fgetc (fp);
+ }
+ fclose (fp);
+ success = true;
+ }
+#elif defined WINDOWSNT
+ if (w32_init_random (&v, sizeof v) == 0)
+ success = true;
+#endif /* HAVE_DEV_RANDOM || WINDOWSNT */
+ if (!success)
+ {
+ /* Fall back to current time value + PID. */
+ t = current_timespec ();
+ v = getpid () ^ t.tv_sec ^ t.tv_nsec;
+ }
seed_random (&v, sizeof v);
}
--- src/w32.c~0 2015-11-29 06:48:07.000000000 +0200
+++ src/w32.c 2015-12-31 16:05:06.775707600 +0200
@@ -224,6 +224,8 @@ typedef struct _REPARSE_DATA_BUFFER {
#include <iphlpapi.h> /* should be after winsock2.h */
+#include <wincrypt.h>
+
#include <c-strcase.h>
#include "w32.h"
@@ -2093,6 +2095,34 @@ init_user_info (void)
CloseHandle (token);
}
+static HCRYPTPROV w32_crypto_hprov;
+static int
+w32_init_crypt_random (void)
+{
+ if (!CryptAcquireContext (&w32_crypto_hprov, NULL, NULL, PROV_RSA_FULL,
+ CRYPT_VERIFYCONTEXT | CRYPT_SILENT))
+ {
+ DebPrint (("CryptAcquireContext failed with error %x\n",
+ GetLastError ()));
+ w32_crypto_hprov = 0;
+ return -1;
+ }
+ return 0;
+}
+
+int
+w32_init_random (void *buf, ptrdiff_t buflen)
+{
+ if (w32_crypto_hprov)
+ w32_init_crypt_random ();
+ if (w32_crypto_hprov)
+ {
+ if (CryptGenRandom (w32_crypto_hprov, buflen, (BYTE *)buf))
+ return 0;
+ }
+ return -1;
+}
+
int
random (void)
{
@@ -9386,6 +9416,8 @@ globals_of_w32 (void)
extern void dynlib_reset_last_error (void);
dynlib_reset_last_error ();
#endif
+
+ w32_crypto_hprov = (HCRYPTPROV)0;
}
/* For make-serial-process */
--- src/w32.h~2 2015-11-29 06:48:07.000000000 +0200
+++ src/w32.h 2015-12-31 16:09:21.960654500 +0200
@@ -222,6 +222,9 @@ extern int w32_memory_info (unsigned lon
/* Compare 2 UTF-8 strings in locale-dependent fashion. */
extern int w32_compare_strings (const char *, const char *, char *, int);
+/* Return a cryptographically secure seed for PRNG. */
+extern int w32_init_random (void *, ptrdiff_t);
+
#ifdef HAVE_GNUTLS
#include <gnutls/gnutls.h>
--- src/fns.c~ 2015-11-22 07:57:20.000000000 +0200
+++ src/fns.c 2015-12-31 16:57:43.607286800 +0200
@@ -50,7 +50,8 @@ All integers representable in Lisp, i.e.
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 current time and pid.
+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.
With a string argument, set the seed based on the string's contents.
Other values of LIMIT are ignored.
--- configure.ac~2 2015-12-20 06:45:33.000000000 +0200
+++ configure.ac 2015-12-31 16:06:48.959511800 +0200
@@ -4145,6 +4145,22 @@
AC_TYPE_MBSTATE_T
+AC_MSG_CHECKING([whether "/dev/random" is available])
+dev_random=no
+dnl MSYS, being a Cygwin fork, thinks "/dev/random" does exist, so
+dnl don't check this for the MinGW builds.
+if test "${opsys}" != "mingw32"; then
+ if test -r "/dev/random"; then
+ AC_DEFINE(HAVE_DEV_RANDOM, 1, [Define if the system supports the "/dev/random" device.])
+ dev_random=yes
+ fi
+fi
+if test $dev_random = yes; then
+ AC_MSG_RESULT(yes)
+else
+ AC_MSG_RESULT(no)
+fi
+
dnl Fixme: AC_SYS_POSIX_TERMIOS should probably be used, but it's not clear
dnl how the tty code is related to POSIX and/or other versions of termios.
dnl The following looks like a useful start.
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2015-12-30 20:47 ` Richard Copley
2015-12-30 20:56 ` Richard Copley
2015-12-30 20:56 ` Eli Zaretskii
@ 2015-12-31 17:04 ` Demetrios Obenour
2015-12-31 17:24 ` Eli Zaretskii
2015-12-31 19:20 ` Eli Zaretskii
2 siblings, 2 replies; 48+ messages in thread
From: Demetrios Obenour @ 2015-12-31 17:04 UTC (permalink / raw)
To: Richard Copley, Eli Zaretskii; +Cc: 22202, David Engster
On Wed, 2015-12-30 at 20:47 +0000, Richard Copley wrote:
> > Can you audit the patch below? I know next to nothing about
> > cryptography, and I'm not sure I understood all the flags involved
> > in
> > these APIs.
>
> Sure! But please bear in mind I'm not experienced in crypto
> either.
>
> With regard to API usage: The call to CryptAcquireContext looks good
> to me. (The comment about interoperability in the documentation for
> the parameter "pszProvider" does not apply as we are not inter-
> operating with anything. Setting "pszContainer" to NULL, as you have
> done, is explicitly recommended. The docs for the individual flags
> entail the very value of "dwFlags" that you use.) I can see nothing
> else to comment on.
>
> Re performance: using CryptGenRandom to provide a seed for srand
> is enough to address Demetri's concern. For performance reasons,
> as you said, implementing random() with CryptGenRandom is
> potentially bad. I think random() itself should not be changed.
>
> That said, rand() makes me uncomfortable (mostly because of bugs in
> long-gone implementations, and superstition). Given the chance I
> would
> replace it with an xorshift* generator. The generator at [1] seeded
> with 64 bits from CryptGenRandom should give good performance for
> random() and (I guess!) an effectively unassailable server secret.
>
> But I have no good reason to claim that rand() is not good enough.
>
> Thank you Eli.
>
> [1]: https://en.wikipedia.org/w/index.php?title=Xorshift&oldid=697235
> 156#xorshift.2A
>
The server secret should be entirely obtained from CryptGenRandom (or
the function RtlGenRandom on which it is based). The server secret is
a cryptographic key and should be generated as such. Using the same
entropy to seed an insecure PRNG and the server secret is a bad idea --
the server secret could be guessed based on PRNG output.
It would also be nice to expose a CSPRNG to Lisp on all platforms. I
know that SLIME could use it on Windows, and it would be nice if one
could have a just-do-it API for this purpose. Speed does not matter
much here.
Demetri
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2015-12-31 17:04 ` Demetrios Obenour
@ 2015-12-31 17:24 ` Eli Zaretskii
2015-12-31 17:47 ` Richard Copley
2015-12-31 19:20 ` Eli Zaretskii
1 sibling, 1 reply; 48+ messages in thread
From: Eli Zaretskii @ 2015-12-31 17:24 UTC (permalink / raw)
To: Demetrios Obenour; +Cc: rcopley, 22202, deng
> From: Demetrios Obenour <demetriobenour@gmail.com>
> Cc: David Engster <deng@randomsample.de>, 22202@debbugs.gnu.org
> Date: Thu, 31 Dec 2015 12:04:38 -0500
>
> The server secret should be entirely obtained from CryptGenRandom (or
> the function RtlGenRandom on which it is based). The server secret is
> a cryptographic key and should be generated as such. Using the same
> entropy to seed an insecure PRNG and the server secret is a bad idea --
> the server secret could be guessed based on PRNG output.
I don't understand what you are saying. server.el doesn't use the
secret, it simply invokes the 'random' function several time to
generate the authentication key. The secret is used to seed the PRNG
during Emacs startup, and it is used only once.
Given this description, how can the secret be guessed, and what are
the implications of that guess (if indeed it's possible) on the
ability of an attacker to control Emacs via the client socket?
> It would also be nice to expose a CSPRNG to Lisp on all platforms. I
> know that SLIME could use it on Windows, and it would be nice if one
> could have a just-do-it API for this purpose. Speed does not matter
> much here.
Patches are welcome, but they should include the same feature for
Posix hosts, probably using /dev/random.
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2015-12-31 17:24 ` Eli Zaretskii
@ 2015-12-31 17:47 ` Richard Copley
2015-12-31 18:22 ` Eli Zaretskii
0 siblings, 1 reply; 48+ messages in thread
From: Richard Copley @ 2015-12-31 17:47 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 22202, Demetrios Obenour, David Engster
That last patch would still improve matters. The user would have
to be publishing the output of their PRNG to begin with in order
for the attacker to analyse it and guess the seed. (I don't know
how one could do that but that's no proof that it's impossible.)
What Demetri has just described is what I would do. (Sorry
again that I can't assist with a patch.)
+ if (w32_crypto_hprov)
+ w32_init_crypt_random ();
should be
+ if (! w32_crypto_hprov)
+ w32_init_crypt_random ();
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2015-12-31 17:47 ` Richard Copley
@ 2015-12-31 18:22 ` Eli Zaretskii
0 siblings, 0 replies; 48+ messages in thread
From: Eli Zaretskii @ 2015-12-31 18:22 UTC (permalink / raw)
To: Richard Copley; +Cc: 22202, demetriobenour, deng
> From: Richard Copley <rcopley@gmail.com>
> Date: Thu, 31 Dec 2015 17:47:18 +0000
> Cc: Demetrios Obenour <demetriobenour@gmail.com>, David Engster <deng@randomsample.de>,
> 22202@debbugs.gnu.org
>
> That last patch would still improve matters. The user would have
> to be publishing the output of their PRNG to begin with in order
> for the attacker to analyse it and guess the seed. (I don't know
> how one could do that but that's no proof that it's impossible.)
I don't even understand how that could be possible.
> What Demetri has just described is what I would do.
Now I'm confused: do what? We still need to support 'random' with an
argument, so we cannot get rid of seeding a PRNG with a known value.
And I didn't want to remove srandom.
> + if (w32_crypto_hprov)
> + w32_init_crypt_random ();
>
> should be
>
> + if (! w32_crypto_hprov)
> + w32_init_crypt_random ();
Ah, that's a left-over from debugging. Thanks.
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2015-12-31 17:04 ` Demetrios Obenour
2015-12-31 17:24 ` Eli Zaretskii
@ 2015-12-31 19:20 ` Eli Zaretskii
2015-12-31 19:49 ` Richard Copley
1 sibling, 1 reply; 48+ messages in thread
From: Eli Zaretskii @ 2015-12-31 19:20 UTC (permalink / raw)
To: Demetrios Obenour; +Cc: rcopley, 22202, deng
> From: Demetrios Obenour <demetriobenour@gmail.com>
> Cc: David Engster <deng@randomsample.de>, 22202@debbugs.gnu.org
> Date: Thu, 31 Dec 2015 12:04:38 -0500
>
> It would also be nice to expose a CSPRNG to Lisp on all platforms. I
> know that SLIME could use it on Windows, and it would be nice if one
> could have a just-do-it API for this purpose. Speed does not matter
> much here.
Btw, for the record: my speed measurements were wrong: it turned out I
compared an optimized build with an unoptimized one. When compared
correctly, there's no perceptible performance difference between using
CRT's 'rand' and using the Windows Crypto API to generate random
numbers.
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2015-12-31 19:20 ` Eli Zaretskii
@ 2015-12-31 19:49 ` Richard Copley
2015-12-31 20:13 ` Eli Zaretskii
2016-01-15 9:55 ` Eli Zaretskii
0 siblings, 2 replies; 48+ messages in thread
From: Richard Copley @ 2015-12-31 19:49 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 22202, Demetrios Obenour, David Engster
>> That last patch would still improve matters. The user would have
>> to be publishing the output of their PRNG to begin with in order
>> for the attacker to analyse it and guess the seed. (I don't know
>> how one could do that but that's no proof that it's impossible.)
>
>I don't even understand how that could be possible.
Me either, but that doesn't make it impossible. (There are articles
on the web demonstrating such feats, if you're interested.)
>> What Demetri has just described is what I would do.
>
>Now I'm confused: do what?
As I understand it: Provide a function callable from lisp that returns
a cryptographically secure sequence of random bytes, of a specified
length. Use that function to generate the server secret.
>We still need to support 'random' with an
>argument, so we cannot get rid of seeding a PRNG with a known value.
>And I didn't want to remove srandom.
Given the above, we could leave "random", etc., as they are, or we
could use a better PRNG and/or seed with system entropy. It would
no longer be tied up with this issue report.
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2015-12-31 19:49 ` Richard Copley
@ 2015-12-31 20:13 ` Eli Zaretskii
2015-12-31 20:44 ` Richard Copley
2016-01-15 9:55 ` Eli Zaretskii
1 sibling, 1 reply; 48+ messages in thread
From: Eli Zaretskii @ 2015-12-31 20:13 UTC (permalink / raw)
To: Richard Copley; +Cc: 22202, demetriobenour, deng
> From: Richard Copley <rcopley@gmail.com>
> Date: Thu, 31 Dec 2015 19:49:42 +0000
> Cc: Demetrios Obenour <demetriobenour@gmail.com>, David Engster <deng@randomsample.de>,
> 22202@debbugs.gnu.org
>
> >> What Demetri has just described is what I would do.
> >
> >Now I'm confused: do what?
>
> As I understand it: Provide a function callable from lisp that returns
> a cryptographically secure sequence of random bytes, of a specified
> length. Use that function to generate the server secret.
That's what my patch does.
> >We still need to support 'random' with an
> >argument, so we cannot get rid of seeding a PRNG with a known value.
> >And I didn't want to remove srandom.
>
> Given the above, we could leave "random", etc., as they are, or we
> could use a better PRNG and/or seed with system entropy. It would
> no longer be tied up with this issue report.
Patches welcome, as I said already.
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2015-12-31 20:13 ` Eli Zaretskii
@ 2015-12-31 20:44 ` Richard Copley
0 siblings, 0 replies; 48+ messages in thread
From: Richard Copley @ 2015-12-31 20:44 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 22202, Demetri Obenour, David Engster
>> >> What Demetri has just described is what I would do.
>> >
>> >Now I'm confused: do what?
>>
>> As I understand it: Provide a function callable from lisp that returns
>> a cryptographically secure sequence of random bytes, of a specified
>> length. Use that function to generate the server secret.
>
> That's what my patch does.
A separate function from "random".
>> >We still need to support 'random' with an
>> >argument, so we cannot get rid of seeding a PRNG with a known value.
>> >And I didn't want to remove srandom.
>>
>> Given the above, we could leave "random", etc., as they are, or we
>> could use a better PRNG and/or seed with system entropy. It would
>> no longer be tied up with this issue report.
>
> Patches welcome, as I said already.
You asked me a question.
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2015-12-31 19:49 ` Richard Copley
2015-12-31 20:13 ` Eli Zaretskii
@ 2016-01-15 9:55 ` Eli Zaretskii
1 sibling, 0 replies; 48+ messages in thread
From: Eli Zaretskii @ 2016-01-15 9:55 UTC (permalink / raw)
To: Richard Copley; +Cc: demetriobenour, deng, 22202-done
> From: Richard Copley <rcopley@gmail.com>
> Date: Thu, 31 Dec 2015 19:49:42 +0000
> Cc: Demetrios Obenour <demetriobenour@gmail.com>, David Engster <deng@randomsample.de>,
> 22202@debbugs.gnu.org
>
> >> That last patch would still improve matters. The user would have
> >> to be publishing the output of their PRNG to begin with in order
> >> for the attacker to analyse it and guess the seed. (I don't know
> >> how one could do that but that's no proof that it's impossible.)
> >
> >I don't even understand how that could be possible.
>
> Me either, but that doesn't make it impossible. (There are articles
> on the web demonstrating such feats, if you're interested.)
>
> >> What Demetri has just described is what I would do.
> >
> >Now I'm confused: do what?
>
> As I understand it: Provide a function callable from lisp that returns
> a cryptographically secure sequence of random bytes, of a specified
> length. Use that function to generate the server secret.
That'd be an enhancement, not a bug. Patches to provide such an API
are welcome, now that the infrastructure exists both on Posix hosts
and on MS-Windows (see below), the rest should be easy: one just needs
to follow the established APIs in other Lisp-like environments, I
think.
> >We still need to support 'random' with an
> >argument, so we cannot get rid of seeding a PRNG with a known value.
> >And I didn't want to remove srandom.
>
> Given the above, we could leave "random", etc., as they are, or we
> could use a better PRNG and/or seed with system entropy. It would
> no longer be tied up with this issue report.
I preferred to make it possible to pass a cryptographically secure
byte stream to 'srandom' instead. See commit 3ffe81e on the emacs-25
branch. This leaves the basic 'random' functionality intact, so no
Lisp packages should be affected.
I'm therefore marking this bug as done. Thanks for the feedback.
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2015-12-18 10:05 bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems Demetri Obenour
2015-12-18 10:46 ` Eli Zaretskii
2015-12-29 15:36 ` Richard Copley
@ 2016-01-17 20:26 ` Paul Eggert
2016-01-18 1:42 ` Paul Eggert
2016-01-18 15:45 ` Eli Zaretskii
2016-01-18 12:04 ` Andy Moreton
` (3 subsequent siblings)
6 siblings, 2 replies; 48+ messages in thread
From: Paul Eggert @ 2016-01-17 20:26 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: Richard Copley, 22202, demetriobenour, deng
[-- Attachment #1: Type: text/plain, Size: 743 bytes --]
Eli, thanks for improving the initial seed for (random t) in Emacs. I noticed
that with this change, my Emacs was opening /dev/urandom twice, because GnuTLS
does something similar during startup. Also, it was reading more data from
/dev/urandom than it needed, due to stdio buffering. So I installed the attached
patch, which defers to GnuTLS and falls back on doing things by hand (without
stdio) only if GnuTLS is not available or fails. I assume this approach works
under MS-Windows; if not please let me know and I'll try to fix it.
Would you mind if I removed the newly-added details about current time and
process ID from the documentation? The idea is that this is internal
implementation detail that users should not rely on.
[-- Attachment #2: 0001-Prefer-GnuTLS-when-acquiring-random-seed.patch --]
[-- Type: text/x-diff, Size: 4807 bytes --]
From 05e8148a24ebe51fbe758dd16265e8fb81f85953 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 17 Jan 2016 12:12:08 -0800
Subject: [PATCH] Prefer GnuTLS when acquiring random seed
This attempts to improve on the fix for Bug#22202.
* configure.ac (HAVE_DEV_URANDOM): Remove.
Check /dev/urandom existence at run time, not at build time,
since the device could exist in the former but not the latter.
* src/sysdep.c [HAVE_GNUTLS]: Include gnutls/gnutls.h.
(gnutls_rnd) [GNUTLS_VERSION_NUMBER < 0x020c00]: New fallback macro.
(random_seed): New typedef.
(set_random_seed): New static function.
(seed_random): Use them.
(init_random): Use random_seed instead of uintmax_t, so as to
not consume more entropy than needed. Prefer gnutls_rnd if it
works; this avoids a redundant open of /dev/urandom on
GNU/Linux with modern GnuTLS.
---
configure.ac | 16 ------------
src/sysdep.c | 85 ++++++++++++++++++++++++++++++------------------------------
2 files changed, 43 insertions(+), 58 deletions(-)
diff --git a/configure.ac b/configure.ac
index 6c9b621..8c01aba 100644
--- a/configure.ac
+++ b/configure.ac
@@ -4153,22 +4153,6 @@ fi
AC_TYPE_MBSTATE_T
-AC_MSG_CHECKING([whether "/dev/urandom" is available])
-dev_urandom=no
-dnl MSYS, being a Cygwin fork, thinks "/dev/urandom" does exist, so
-dnl don't check this for the MinGW builds.
-if test "${opsys}" != "mingw32"; then
- if test -r "/dev/urandom"; then
- AC_DEFINE(HAVE_DEV_URANDOM, 1, [Define if the system supports the "/dev/urandom" device.])
- dev_urandom=yes
- fi
-fi
-if test $dev_urandom = yes; then
- AC_MSG_RESULT(yes)
-else
- AC_MSG_RESULT(no)
-fi
-
dnl Fixme: AC_SYS_POSIX_TERMIOS should probably be used, but it's not clear
dnl how the tty code is related to POSIX and/or other versions of termios.
dnl The following looks like a useful start.
diff --git a/src/sysdep.c b/src/sysdep.c
index 1fa4229..635443c 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -99,6 +99,15 @@ along with GNU Emacs. If not, see <http://www.gnu.org/licenses/>. */
#include "process.h"
#include "cm.h"
+#ifdef HAVE_GNUTLS
+# include <gnutls/gnutls.h>
+#endif
+#if 0x020c00 <= GNUTLS_VERSION_NUMBER
+# include <gnutls/crypto.h>
+#else
+# define gnutls_rnd(level, data, len) (-1)
+#endif
+
#ifdef WINDOWSNT
#include <direct.h>
/* In process.h which conflicts with the local copy. */
@@ -2068,63 +2077,55 @@ init_signals (bool dumping)
# endif /* !HAVE_RANDOM */
#endif /* !RAND_BITS */
+#ifdef HAVE_RANDOM
+typedef unsigned int random_seed;
+static void set_random_seed (random_seed arg) { srandom (arg); }
+#elif defined HAVE_LRAND48
+/* Although srand48 uses a long seed, this is unsigned long to avoid
+ undefined behavior on signed integer overflow in init_random. */
+typedef unsigned long int random_seed;
+static void set_random_seed (random_seed arg) { srand48 (arg); }
+#else
+typedef unsigned int random_seed;
+static void set_random_seed (random_seed arg) { srand (arg); }
+#endif
+
void
seed_random (void *seed, ptrdiff_t seed_size)
{
-#if defined HAVE_RANDOM || ! defined HAVE_LRAND48
- unsigned int arg = 0;
-#else
- long int arg = 0;
-#endif
+ random_seed arg = 0;
unsigned char *argp = (unsigned char *) &arg;
unsigned char *seedp = seed;
- ptrdiff_t i;
- for (i = 0; i < seed_size; i++)
+ for (ptrdiff_t i = 0; i < seed_size; i++)
argp[i % sizeof arg] ^= seedp[i];
-#ifdef HAVE_RANDOM
- srandom (arg);
-#else
-# ifdef HAVE_LRAND48
- srand48 (arg);
-# else
- srand (arg);
-# endif
-#endif
+ set_random_seed (arg);
}
void
init_random (void)
{
- uintmax_t v;
- struct timespec t;
- bool success = false;
-
-#if HAVE_DEV_URANDOM
- FILE *fp = fopen ("/dev/urandom", "rb");
-
- if (fp)
+ random_seed v;
+ if (gnutls_rnd (GNUTLS_RND_NONCE, &v, sizeof v) != 0)
{
- int i;
-
- for (i = 0, v = 0; i < sizeof (uintmax_t); i++)
+ bool success = false;
+#ifndef WINDOWSNT
+ int fd = emacs_open ("/dev/urandom", O_RDONLY | O_BINARY, 0);
+ if (0 <= fd)
{
- v <<= 8;
- v |= fgetc (fp);
+ success = emacs_read (fd, &v, sizeof v) == sizeof v;
+ emacs_close (fd);
+ }
+#else
+ success = w32_init_random (&v, sizeof v) == 0;
+#endif
+ if (! success)
+ {
+ /* Fall back to current time value + PID. */
+ struct timespec t = current_timespec ();
+ v = getpid () ^ t.tv_sec ^ t.tv_nsec;
}
- fclose (fp);
- success = true;
- }
-#elif defined WINDOWSNT
- if (w32_init_random (&v, sizeof v) == 0)
- success = true;
-#endif /* HAVE_DEV_URANDOM || WINDOWSNT */
- if (!success)
- {
- /* Fall back to current time value + PID. */
- t = current_timespec ();
- v = getpid () ^ t.tv_sec ^ t.tv_nsec;
}
- seed_random (&v, sizeof v);
+ set_random_seed (v);
}
/*
--
2.5.0
^ permalink raw reply related [flat|nested] 48+ messages in thread* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2016-01-17 20:26 ` Paul Eggert
@ 2016-01-18 1:42 ` Paul Eggert
2016-01-18 14:40 ` Richard Copley
2016-01-18 15:45 ` Eli Zaretskii
1 sibling, 1 reply; 48+ messages in thread
From: Paul Eggert @ 2016-01-18 1:42 UTC (permalink / raw)
To: 22202; +Cc: Richard Copley, Andreas Schwab, demetriobenour, deng
Andreas Schwab discovered a problem with my patch in that GnuTLS wasn't
initialized, and reverted the GnuTLS part of it. As I understand it, newer
versions of GnuTLS initialize themselves when they are loaded and so do not run
into the issue; I tested with GnuTLS 3.3.15, which I suppose is new enough. I
attempted to fix this problem in the followup commit
130d512045aa376333b664d58c501b3884187592.
Andreas's commit also changed some unrelated style issues, which I reverted;
that is merely a longrunning stylistic disagreement, and right now is not a good
time to be changing style in code unrelated to fixes.
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2016-01-18 1:42 ` Paul Eggert
@ 2016-01-18 14:40 ` Richard Copley
2016-01-18 16:05 ` Eli Zaretskii
0 siblings, 1 reply; 48+ messages in thread
From: Richard Copley @ 2016-01-18 14:40 UTC (permalink / raw)
To: Paul Eggert; +Cc: 22202, Andreas Schwab, Demetri Obenour, David Engster
On 18 January 2016 at 01:42, Paul Eggert <eggert@cs.ucla.edu> wrote:
> Andreas Schwab discovered a problem with my patch in that GnuTLS wasn't
> initialized, and reverted the GnuTLS part of it. As I understand it, newer
> versions of GnuTLS initialize themselves when they are loaded and so do not
> run into the issue; I tested with GnuTLS 3.3.15, which I suppose is new
> enough. I attempted to fix this problem in the followup commit
> 130d512045aa376333b664d58c501b3884187592.
>
> Andreas's commit also changed some unrelated style issues, which I reverted;
> that is merely a longrunning stylistic disagreement, and right now is not a
> good time to be changing style in code unrelated to fixes.
I can't build from the current sources; the error is:
CCLD temacs.exe
sysdep.o: In function `init_random':
C:/emacs/repo/emacs/src/sysdep.c:2108: undefined reference to `gnutls_rnd'
C:/emacs/repo/emacs/src/sysdep.c:2108:(.text+0xf38): relocation
truncated to fit: R_X86_64_PC32 against undefined symbol `gnutls_rnd'
collect2.exe: error: ld returned 1 exit status
Configuration details (from last good build):
In GNU Emacs 25.0.50.1 (x86_64-w64-mingw32)
of 2016-01-14 built on 60678UHB
Repository revision: dadb841a06aa1ffd6d17c04ef83140dbd1ad7307
Windowing system distributor 'Microsoft Corp.', version 6.1.7601
Configured using:
'configure --prefix /c/emacs/emacs-20160114-182403
--without-imagemagick --disable-dependency-tracking
--enable-locallisppath=%emacs_dir%/../site-lisp 'CFLAGS=-Og -g -ggdb''
Configured features:
XPM JPEG TIFF GIF PNG RSVG SOUND DBUS NOTIFY ACL GNUTLS LIBXML2 ZLIB
TOOLKIT_SCROLL_BARS
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2016-01-18 14:40 ` Richard Copley
@ 2016-01-18 16:05 ` Eli Zaretskii
2016-01-18 16:20 ` Richard Copley
0 siblings, 1 reply; 48+ messages in thread
From: Eli Zaretskii @ 2016-01-18 16:05 UTC (permalink / raw)
To: Richard Copley; +Cc: 22202, schwab, eggert
> From: Richard Copley <rcopley@gmail.com>
> Date: Mon, 18 Jan 2016 14:40:05 +0000
> Cc: 22202@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>,
> Demetri Obenour <demetriobenour@gmail.com>, David Engster <deng@randomsample.de>,
> Andreas Schwab <schwab@linux-m68k.org>
>
> I can't build from the current sources; the error is:
>
> CCLD temacs.exe
> sysdep.o: In function `init_random':
> C:/emacs/repo/emacs/src/sysdep.c:2108: undefined reference to `gnutls_rnd'
> C:/emacs/repo/emacs/src/sysdep.c:2108:(.text+0xf38): relocation
> truncated to fit: R_X86_64_PC32 against undefined symbol `gnutls_rnd'
> collect2.exe: error: ld returned 1 exit status
Please try again.
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2016-01-18 16:05 ` Eli Zaretskii
@ 2016-01-18 16:20 ` Richard Copley
0 siblings, 0 replies; 48+ messages in thread
From: Richard Copley @ 2016-01-18 16:20 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 22202, Andreas Schwab, Paul Eggert
On 18 January 2016 at 16:05, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Richard Copley <rcopley@gmail.com>
>> Date: Mon, 18 Jan 2016 14:40:05 +0000
>> Cc: 22202@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>,
>> Demetri Obenour <demetriobenour@gmail.com>, David Engster <deng@randomsample.de>,
>> Andreas Schwab <schwab@linux-m68k.org>
>>
>> I can't build from the current sources; the error is:
>>
>> CCLD temacs.exe
>> sysdep.o: In function `init_random':
>> C:/emacs/repo/emacs/src/sysdep.c:2108: undefined reference to `gnutls_rnd'
>> C:/emacs/repo/emacs/src/sysdep.c:2108:(.text+0xf38): relocation
>> truncated to fit: R_X86_64_PC32 against undefined symbol `gnutls_rnd'
>> collect2.exe: error: ld returned 1 exit status
>
> Please try again.
Fixed, thanks.
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2016-01-17 20:26 ` Paul Eggert
2016-01-18 1:42 ` Paul Eggert
@ 2016-01-18 15:45 ` Eli Zaretskii
2016-01-18 20:50 ` Paul Eggert
1 sibling, 1 reply; 48+ messages in thread
From: Eli Zaretskii @ 2016-01-18 15:45 UTC (permalink / raw)
To: Paul Eggert; +Cc: rcopley, 22202, deng
> From: Paul Eggert <eggert@cs.ucla.edu>
> Cc: 22202@debbugs.gnu.org, Richard Copley <rcopley@gmail.com>,
> demetriobenour@gmail.com, deng@randomsample.de
> Date: Sun, 17 Jan 2016 12:26:31 -0800
>
> Eli, thanks for improving the initial seed for (random t) in Emacs. I noticed that with this change, my Emacs was opening /dev/urandom twice, because GnuTLS does something similar during startup. Also, it was reading more data from /dev/urandom than it needed, due to stdio buffering. So I installed the attached patch, which defers to GnuTLS and falls back on doing things by hand (without stdio) only if GnuTLS is not available or fails. I assume this approach works under MS-Windows; if not please let me know and I'll try to fix it.
I wish we'd discuss such things before committing and not after.
What's the rush? It's not like Emacs was broken or something. It
just isn't worth it.
More to to the point, the more I think about this, the less I like
this change. Here's why:
. I see nothing wrong with having 2 (or more) independent reads from
/dev/urandom:
. GnuTLS is a separate library, so it's free to do that for its
own purposes; we shouldn't care. Besides, what if tomorrow
there will be a 3rd library that would need to access
/dev/urandom?
. Reading from /dev/urandom never blocks, so doing this one more
time during startup should be a non-issue.
. Calling gnutls_rnd makes Emacs dependent on GnuTLS in ways that we
don't necessarily want: GnuTLS is a library for TLS, not for
cryptography. What that function does on each platform is not
documented anywhere in the GnuTLS manual that I could see; I
needed to read the sources to find out. What if tomorrow GnuTLS
changes its implementation? They are a separate project, they are
not under any obligation to tell us.
. This change means that we now load GnuTLS at startup, even if no
TLS connections are or will be used. Why should we unnecessarily
bloat our memory footprint, even in minor ways?
. It breaks the Windows build -- since GnuTLS is dynamically loaded
on Windows, any GnuTLS function must be called through macros set
up in gnutls.c, which sysdep.c knows nothing about. (This is
relatively simple to fix, but doing so requires adding yet more
ugly #ifdef's to an already not-so-pretty little mess.)
So I tend to just say NO here. Let's use the relatively simple code
we had before (with your I/O changes, I don't object to those, of
course), and leave GnuTLS to its own devices. WDYT?
> Would you mind if I removed the newly-added details about current time and process ID from the documentation? The idea is that this is internal implementation detail that users should not rely on.
I didn't really add anything: the fact that we used time and PID was
spelled out in the function's doc string for the last 20-odd years. I
just added that to the ELisp manual, to make it more consistent with
the doc string, and also because it's hard to tell that we are using
system entropy when available without saying anything about what we do
when it isn't. (And IMO we should mention the system entropy because
that's an important feature users should know about.)
Why is it suddenly a concern that users will know that we use time and
PID as fallback?
^ permalink raw reply [flat|nested] 48+ messages in thread* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2016-01-18 15:45 ` Eli Zaretskii
@ 2016-01-18 20:50 ` Paul Eggert
2016-01-18 21:09 ` Eli Zaretskii
0 siblings, 1 reply; 48+ messages in thread
From: Paul Eggert @ 2016-01-18 20:50 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: rcopley, 22202, deng
[-- 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.
^ permalink raw reply related [flat|nested] 48+ messages in thread* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2016-01-18 20:50 ` Paul Eggert
@ 2016-01-18 21:09 ` Eli Zaretskii
2016-01-19 5:34 ` Paul Eggert
0 siblings, 1 reply; 48+ messages in thread
From: Eli Zaretskii @ 2016-01-18 21:09 UTC (permalink / raw)
To: Paul Eggert; +Cc: rcopley, 22202, deng
> Cc: 22202@debbugs.gnu.org, rcopley@gmail.com, deng@randomsample.de
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Mon, 18 Jan 2016 12:50:12 -0800
>
> > 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.
That's not what I meant. You saw the code I committed just a few days
ago; it would be prudent to discuss your plans to change that. We all
silently fix blunders and other trivial problems; this wasn't one of
them. It wasn't a bug fix, either. So there was no need to rush with
the commit. Such conduct doesn't make us feel like a community.
> > . 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.
Why is that a worry? We use many libraries, some of them open and
read files. What's to worry about?
> Also, it's two file descriptors (both open at the same time) when we
> can get by with just one.
AFAICS, we close the file descriptor as soon as we finished reading.
So unless GnuTLS initialization is run in another thread, there won't
be 2 descriptors at the same time.
> > . 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?
It's a very simple wheel. If you've seen their sources (I have), you
know that they do exactly the same, both on GNU/Linux and on
MS-Windows.
> 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.
I don't think we should become experts on external libraries, and I
don't think we should track their development. Where GnuTLS needs
security for its internal use, we should let it do what they see fit;
if they do that wrongly, the word will spread, and we will upgrade or
switch to another library.
But where we need to seed our own PRNG, we better had a good idea of
what we do and what kind of randomness we get. This is the core of
this bug report. I don't think we should delegate that to an external
library whose main business is secure communications. It's a
different discipline, different trade-offs, etc.
> > 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.
So what is special about GnuTLS?
> > 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.
We did that since 1993. Isn't it a tad too late to worry about it?
> One other thing, while we're nearby: the doc shouldn't assume that readers know
> that time-of-day etc. is less random.
A fallback is always worse than Plan A. Everyone understands that,
even if they know nothing about randomness and PRNGs.
> How about the attached patch? It should address these documentation concerns.
Frankly, it feels silly, but since you are so enthusiastic, who am I
to stand on your way?
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2016-01-18 21:09 ` Eli Zaretskii
@ 2016-01-19 5:34 ` Paul Eggert
2016-01-19 16:24 ` Eli Zaretskii
0 siblings, 1 reply; 48+ messages in thread
From: Paul Eggert @ 2016-01-19 5:34 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: rcopley, 22202, deng
Eli Zaretskii wrote:
> We all silently fix blunders and other trivial problems; this wasn't one of
> them.
I thought it a trivial matter; evidently I was mistaken. My apologies.
> AFAICS, we close the file descriptor as soon as we finished reading.
> So unless GnuTLS initialization is run in another thread, there won't
> be 2 descriptors at the same time.
GnuTLS keeps /dev/urandom open indefinitely. If Emacs opens /dev/urandom
independently it can have two file descriptors open to the same file. Yes, it's
not a huge deal performance-wise; but it is strange, and when doing security
audits it will be one more thing to explain.
> But where we need to seed our own PRNG, we better had a good idea of
> what we do and what kind of randomness we get.
Any worries we might have about GnuTLS's randomness apply with equal force to
/dev/urandom's. After all, /dev/urandom is not guaranteed to be random.
Really, though, if we can't trust GnuTLS to give us random data, we should not
trust it for communications security at all. Nonces are that basic.
> So what is special about GnuTLS?
GnuTLS already has the random data we need; other libraries don't.
I installed the documentation patch, since it does seem a minor improvement.
Yes, the doc could have been improved ages ago, but late is better than never.
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2016-01-19 5:34 ` Paul Eggert
@ 2016-01-19 16:24 ` Eli Zaretskii
2016-01-19 17:03 ` John Wiegley
2016-01-19 17:07 ` Paul Eggert
0 siblings, 2 replies; 48+ messages in thread
From: Eli Zaretskii @ 2016-01-19 16:24 UTC (permalink / raw)
To: Paul Eggert, John Wiegley; +Cc: rcopley, 22202, deng
> Cc: 22202@debbugs.gnu.org, rcopley@gmail.com, deng@randomsample.de
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Mon, 18 Jan 2016 21:34:12 -0800
>
> AFAICS, we close the file descriptor as soon as we finished reading.
> So unless GnuTLS initialization is run in another thread, there won't
> be 2 descriptors at the same time.
>
> GnuTLS keeps /dev/urandom open indefinitely.
So it's a bug or misfeature in GnuTLS. Not our concern.
> If Emacs opens /dev/urandom independently it can have two file descriptors open to the same file. Yes, it's not a huge deal performance-wise; but it is strange, and when doing security audits it will be one more thing to explain.
GnuTLS guys need to explain this, not us.
> But where we need to seed our own PRNG, we better had a good idea of
> what we do and what kind of randomness we get.
>
> Any worries we might have about GnuTLS's randomness apply with equal force to /dev/urandom's. After all, /dev/urandom is not guaranteed to be random.
No, /dev/urandom is random enough for our purposes.
> Really, though, if we can't trust GnuTLS to give us random data, we should not trust it for communications security at all. Nonces are that basic.
We could stop trusting GnuTLS for communications security, but we
still need the secure random seed for server-start. I see no reasons
to tie these two together. The Emacs server is not about TLS
communications, at least not by default.
If GnuTLS were a library of RNGs, it would have been a different
matter. But it isn't. We shouldn't depend so critically on 3rd party
libraries for functionality that is unrelated or secondary to their
goal.
> So what is special about GnuTLS?
>
> GnuTLS already has the random data we need; other libraries don't.
We have what we need; calling gnutls_rnd changes nothing in this
regard. It's just a more complex way of issuing the same system
calls. It buys us nothing in terms of security and performance, while
we sustain the price of having core functionality that must run at
startup crucially depending on a 3rd party library we don't control.
John, I feel this decision is wrong and the changes that prefer
gnutls_rnd should be reverted. Maybe I'm the only one who cares, but
then Paul is the only one who felt the need to make that change. I'd
like to hear your take on this, please.
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2016-01-19 16:24 ` Eli Zaretskii
@ 2016-01-19 17:03 ` John Wiegley
2016-01-19 17:38 ` Paul Eggert
2016-01-19 17:07 ` Paul Eggert
1 sibling, 1 reply; 48+ messages in thread
From: John Wiegley @ 2016-01-19 17:03 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: rcopley, 22202, deng, Paul Eggert
[-- Attachment #1: Type: text/plain, Size: 1044 bytes --]
>>>>> Eli Zaretskii <eliz@gnu.org> writes:
> We have what we need; calling gnutls_rnd changes nothing in this regard.
> It's just a more complex way of issuing the same system calls. It buys us
> nothing in terms of security and performance, while we sustain the price of
> having core functionality that must run at startup crucially depending on a
> 3rd party library we don't control.
> John, I feel this decision is wrong and the changes that prefer gnutls_rnd
> should be reverted. Maybe I'm the only one who cares, but then Paul is the
> only one who felt the need to make that change. I'd like to hear your take
> on this, please.
From what I've read, I agree with you Eli. If we can open /dev/urandom, why do
we need a dependency on GnuTLS to effectively do the same thing?
What critical feature is GnuTLS buying for us that would make this worthwhile,
Paul?
--
John Wiegley GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com 60E1 46C4 BD1A 7AC1 4BA2
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 629 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2016-01-19 17:03 ` John Wiegley
@ 2016-01-19 17:38 ` Paul Eggert
2016-01-19 18:44 ` Eli Zaretskii
0 siblings, 1 reply; 48+ messages in thread
From: Paul Eggert @ 2016-01-19 17:38 UTC (permalink / raw)
To: John Wiegley, Eli Zaretskii; +Cc: rcopley, 22202, deng
On 01/19/2016 09:03 AM, John Wiegley wrote:
> What critical feature is GnuTLS buying for us that would make this worthwhile,
> Paul?
There is nothing "critical" here. This is just a minor issue, one that
has been blown all out of proportion. Using GnuTLS when available
lessens use of system resources and simplifies auditing, but Emacs could
get by without this minor bugfix-improvement.
> why do we need a dependency on GnuTLS
There isn't a dependency on GnuTLS in the usual sense: that is, if
GnuTLS is absent, the code still works as before. The only dependency is
that we trust the GnuTLS library to work when it is present, and to
report an error if one occurs. This is a reasonable assumption, both
here and elsewhere in Emacs.
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2016-01-19 17:38 ` Paul Eggert
@ 2016-01-19 18:44 ` Eli Zaretskii
0 siblings, 0 replies; 48+ messages in thread
From: Eli Zaretskii @ 2016-01-19 18:44 UTC (permalink / raw)
To: Paul Eggert; +Cc: rcopley, 22202, deng, johnw
> Cc: 22202@debbugs.gnu.org, rcopley@gmail.com, deng@randomsample.de
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 19 Jan 2016 09:38:23 -0800
>
> This is just a minor issue, one that has been blown all out of
> proportion.
This is unfair: if this minor issue was important enough for you to
make those changes, then objecting to them cannot be considered
"blowing it all out of proportion".
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2016-01-19 16:24 ` Eli Zaretskii
2016-01-19 17:03 ` John Wiegley
@ 2016-01-19 17:07 ` Paul Eggert
2016-01-19 18:16 ` Eli Zaretskii
1 sibling, 1 reply; 48+ messages in thread
From: Paul Eggert @ 2016-01-19 17:07 UTC (permalink / raw)
To: Eli Zaretskii, John Wiegley; +Cc: rcopley, 22202, deng
On 01/19/2016 08:24 AM, Eli Zaretskii wrote:
> So it's a bug or misfeature in GnuTLS.
GnuTLS has been operating that way for a while, and it works. Calling
its behavior a "bug or misfeature" seems a stretch.
If we change Emacs back to always read /dev/urandom by hand as well has
have GnuTLS read /dev/urandom at startup, this will cause Emacs to
exhaust the GNU/Linux entropy pool more quickly. This may slow down
other programs that read /dev/random (a device that blocks until entropy
is available). So there is an overall system benefit to minimizing the
use of /dev/urandom, which was the point of my original patch.
>> If Emacs opens /dev/urandom independently it can have two file descriptors open to the same file. Yes, it's not a huge deal performance-wise; but it is strange, and when doing security audits it will be one more thing to explain.
> GnuTLS guys need to explain this, not us.
Any explanation they come up with will have to be part of our
explanation, since we're responsible for Emacs. Our explanation will
also have to cover Emacs's added accesses, so minimizing them will be a win.
>> But where we need to seed our own PRNG, we better had a good idea of
>> what we do and what kind of randomness we get.
>>
>> Any worries we might have about GnuTLS's randomness apply with equal force to /dev/urandom's. After all, /dev/urandom is not guaranteed to be random.
> No, /dev/urandom is random enough for our purposes.
In that case GnuTLS's nonce generator is random enough for our purposes,
and we have a good idea of what kind of randomness we get.
>
>> Really, though, if we can't trust GnuTLS to give us random data, we should not trust it for communications security at all. Nonces are that basic.
> We could stop trusting GnuTLS for communications security, but we
> still need the secure random seed for server-start.
If we stop trusting or using GnuTLS, the code will still get a secure
random seed by hand, so that's not a problem. But currently we do trust
and use GnuTLS by default, and there are no plans to change this.
> We have what we need; calling gnutls_rnd changes nothing in this
> regard. It's just a more complex way of issuing the same system calls.
They are not the same system calls. If they were the same, you would be
right and we shouldn't bother with GnuTLS here. They are different
sequences of system calls, and the sequence that uses GnuTLS lessens
entropy consumption and simplifies audits.
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2016-01-19 17:07 ` Paul Eggert
@ 2016-01-19 18:16 ` Eli Zaretskii
2016-01-20 0:39 ` Paul Eggert
0 siblings, 1 reply; 48+ messages in thread
From: Eli Zaretskii @ 2016-01-19 18:16 UTC (permalink / raw)
To: Paul Eggert; +Cc: rcopley, 22202, deng, johnw
> Cc: 22202@debbugs.gnu.org, rcopley@gmail.com, deng@randomsample.de
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 19 Jan 2016 09:07:15 -0800
>
> On 01/19/2016 08:24 AM, Eli Zaretskii wrote:
> > So it's a bug or misfeature in GnuTLS.
>
> GnuTLS has been operating that way for a while, and it works. Calling
> its behavior a "bug or misfeature" seems a stretch.
You said it was a problem to have one more handle open, not I. So
it's you who maintains this is a problem (a.k.a. "misfeature"). If
you are now saying it's okay to have the device open, then it's not a
problem to have it open twice for a short while.
IOW, please make up your mind whether this is an issue or not.
> If we change Emacs back to always read /dev/urandom by hand as well has
> have GnuTLS read /dev/urandom at startup, this will cause Emacs to
> exhaust the GNU/Linux entropy pool more quickly. This may slow down
> other programs that read /dev/random (a device that blocks until entropy
> is available). So there is an overall system benefit to minimizing the
> use of /dev/urandom, which was the point of my original patch.
Now, _that's_ a stretch. We only read /dev/urandom once, that's all.
You cannot be serious saying that a single call will deplete the
system entropy.
> >> If Emacs opens /dev/urandom independently it can have two file descriptors open to the same file. Yes, it's not a huge deal performance-wise; but it is strange, and when doing security audits it will be one more thing to explain.
> > GnuTLS guys need to explain this, not us.
>
> Any explanation they come up with will have to be part of our
> explanation
No, it doesn't. We cannot be responsible for the internals of
3rd-party libraries.
> >> But where we need to seed our own PRNG, we better had a good idea of
> >> what we do and what kind of randomness we get.
> >>
> >> Any worries we might have about GnuTLS's randomness apply with equal force to /dev/urandom's. After all, /dev/urandom is not guaranteed to be random.
> > No, /dev/urandom is random enough for our purposes.
>
> In that case GnuTLS's nonce generator is random enough for our purposes,
> and we have a good idea of what kind of randomness we get.
Today, we do. Tomorrow, it's anyone's guess. Unless we audit their
code all the time. Why should we?
> >> Really, though, if we can't trust GnuTLS to give us random data, we should not trust it for communications security at all. Nonces are that basic.
> > We could stop trusting GnuTLS for communications security, but we
> > still need the secure random seed for server-start.
>
> If we stop trusting or using GnuTLS, the code will still get a secure
> random seed by hand, so that's not a problem. But currently we do trust
> and use GnuTLS by default, and there are no plans to change this.
That trust needs now to be ascertained even if we don't use secure
communications from Emacs. So things got worse.
> > We have what we need; calling gnutls_rnd changes nothing in this
> > regard. It's just a more complex way of issuing the same system calls.
>
> They are not the same system calls. If they were the same, you would be
> right and we shouldn't bother with GnuTLS here. They are different
> sequences of system calls, and the sequence that uses GnuTLS lessens
> entropy consumption and simplifies audits.
I've read the code and saw no differences.
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2016-01-19 18:16 ` Eli Zaretskii
@ 2016-01-20 0:39 ` Paul Eggert
0 siblings, 0 replies; 48+ messages in thread
From: Paul Eggert @ 2016-01-20 0:39 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: rcopley, 22202, deng, johnw
On 01/19/2016 10:16 AM, Eli Zaretskii wrote:
>> Cc: 22202@debbugs.gnu.org, rcopley@gmail.com, deng@randomsample.de
>> From: Paul Eggert <eggert@cs.ucla.edu>
>> Date: Tue, 19 Jan 2016 09:07:15 -0800
>>
>> On 01/19/2016 08:24 AM, Eli Zaretskii wrote:
>>> So it's a bug or misfeature in GnuTLS.
>> GnuTLS has been operating that way for a while, and it works. Calling
>> its behavior a "bug or misfeature" seems a stretch.
> You said it was a problem to have one more handle open, not I. So
> it's you who maintains this is a problem (a.k.a. "misfeature"). If
> you are now saying it's okay to have the device open, then it's not a
> problem to have it open twice for a short while.
There is no contradiction here. Although having multiple file
descriptors for the same file is only a minor problem, that does not
mean it is not a problem at all, nor does it mean that the minor problem
is entirely in GnuTLS as opposed to being in the combination of GnuTLS
and Emacs. I did not say GnuTLS's behavior is a misfeature or bug; such
a characterization would go too far.
> You cannot be serious saying that a single call will deplete the
> system entropy.
Although the entropy will not be completely depleted, reading excess
bytes from /dev/urandom can exhaust the entropy pool more quickly. On
GNU/Linux, processes reading /dev/urandom can fairly quickly drain
system entropy down to a couple hundred bits. Reads of /dev/random can
then drain the remaining bits and subsequent reads will hang. Although
the kernel eventually will reacquire system entropy, acquisition rates
can be less than one bit per second, so reading excess data from
/dev/urandom is not cost-free here.
>
>>>> If Emacs opens /dev/urandom independently it can have two file descriptors open to the same file. Yes, it's not a huge deal performance-wise; but it is strange, and when doing security audits it will be one more thing to explain.
>>> GnuTLS guys need to explain this, not us.
>> Any explanation they come up with will have to be part of our
>> explanation
> No, it doesn't. We cannot be responsible for the internals of
> 3rd-party libraries.
We are not responsible for GnuTLS internals, but for audits we are
responsible for explaining unusual behaviors. It's like many other
aspects of libraries that Emacs uses: for example, although we are not
responsible for ImageMagick internals, if those internals allocate a lot
of memory and cause Emacs to hang or crash, we are responsible for the
overall behavior.
>>>> But where we need to seed our own PRNG, we better had a good idea of
>>>> what we do and what kind of randomness we get.
>>>>
>>>> Any worries we might have about GnuTLS's randomness apply with equal force to /dev/urandom's. After all, /dev/urandom is not guaranteed to be random.
>>> No, /dev/urandom is random enough for our purposes.
>> In that case GnuTLS's nonce generator is random enough for our purposes,
>> and we have a good idea of what kind of randomness we get.
> Today, we do. Tomorrow, it's anyone's guess. Unless we audit their
> code all the time. Why should we?
We never really needed to audit the GnuTLS code in the first place. We
went into it only because of concerns that GnuTLS might not do the right
thing, due to lack of understanding about GnuTLS. These concerns have
been addressed, and there should be no need to keep revisiting the issue.
> That trust needs now to be ascertained even if we don't use secure
> communications from Emacs.
Sorry, I'm not following. If the point is that GnuTLS should be trusted
for secure communications (which is relatively complicated) but not for
generating nonces (which is relatively simple) then I don't see a sound
basis for this worry. If GnuTLS can't be trusted for simple building
blocks, why trust it for complex things? Especially when the complex
things are based on the simple building blocks?
>>> We have what we need; calling gnutls_rnd changes nothing in this
>>> regard. It's just a more complex way of issuing the same system calls.
>> They are not the same system calls. If they were the same, you would be
>> right and we shouldn't bother with GnuTLS here. They are different
>> sequences of system calls, and the sequence that uses GnuTLS lessens
>> entropy consumption and simplifies audits.
> I've read the code and saw no differences.
You can try running it both ways under "strace" in GNU/Linux. You should
see different sequences of system calls involving /dev/urandom. That's
what happened for me.
>> This is just a minor issue, one that has been blown all out of
>> proportion.
> This is unfair: if this minor issue was important enough for you to
> make those changes, then objecting to them cannot be considered
> "blowing it all out of proportion".
Yes, a simple objection to a minor change would not blow it all out of
proportion. But this long thread is more than that.
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2015-12-18 10:05 bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems Demetri Obenour
` (2 preceding siblings ...)
2016-01-17 20:26 ` Paul Eggert
@ 2016-01-18 12:04 ` Andy Moreton
2016-01-18 15:57 ` Eli Zaretskii
2016-01-18 23:03 ` John Wiegley
2016-01-19 21:48 ` Andy Moreton
` (2 subsequent siblings)
6 siblings, 2 replies; 48+ messages in thread
From: Andy Moreton @ 2016-01-18 12:04 UTC (permalink / raw)
To: 22202
On Sun 17 Jan 2016, Paul Eggert wrote:
> Andreas Schwab discovered a problem with my patch in that GnuTLS wasn't
> initialized, and reverted the GnuTLS part of it. As I understand it, newer
> versions of GnuTLS initialize themselves when they are loaded and so do not
> run into the issue; I tested with GnuTLS 3.3.15, which I suppose is new
> enough. I attempted to fix this problem in the followup commit
> 130d512045aa376333b664d58c501b3884187592.
Your patch will break builds configured using --without-gnutls, as the
gnutls headers may not be installed, and should not be included.
In addition, these changes require static or dynamic linking of gnutls,
which breaks the Windows builds (which use runtime imports for the gnutls DLLs).
> Andreas's commit also changed some unrelated style issues, which I reverted;
> that is merely a longrunning stylistic disagreement, and right now is not a
> good time to be changing style in code unrelated to fixes.
This is rudeness from both of you: please solve this disagreement by
talking to each other instead of adding pointless churn to the source tree.
AndyM
^ permalink raw reply [flat|nested] 48+ messages in thread* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2015-12-18 10:05 bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems Demetri Obenour
` (3 preceding siblings ...)
2016-01-18 12:04 ` Andy Moreton
@ 2016-01-19 21:48 ` Andy Moreton
2016-01-20 3:31 ` Glenn Morris
2016-01-20 14:06 ` Andy Moreton
2016-01-20 15:15 ` Andy Moreton
6 siblings, 1 reply; 48+ messages in thread
From: Andy Moreton @ 2016-01-19 21:48 UTC (permalink / raw)
To: 22202
On Tue 19 Jan 2016, John Wiegley wrote:
>>>>>> Eli Zaretskii <eliz@gnu.org> writes:
>
>> We have what we need; calling gnutls_rnd changes nothing in this regard.
>> It's just a more complex way of issuing the same system calls. It buys us
>> nothing in terms of security and performance, while we sustain the price of
>> having core functionality that must run at startup crucially depending on a
>> 3rd party library we don't control.
>
>> John, I feel this decision is wrong and the changes that prefer gnutls_rnd
>> should be reverted. Maybe I'm the only one who cares, but then Paul is the
>> only one who felt the need to make that change. I'd like to hear your take
>> on this, please.
>
> From what I've read, I agree with you Eli. If we can open /dev/urandom, why do
> we need a dependency on GnuTLS to effectively do the same thing?
>
> What critical feature is GnuTLS buying for us that would make this worthwhile,
> Paul?
As far as I can see, this set of patches attempted to fix a minor
problem, but in doing so:
- added unnecessary dependencies on gnutls libraries
- broke the Windows builds (which use runtime linking for gnutls)
- broke all builds configured with "--without-gnutls"
I am happy for the original issue to be addressed, but only if all of
the issues listed above are addressed.
In particular, it must remain possible to build on a system that does
not have gnutls headers and libraries installed, or to disable gnutls
support even if the headers and libraries are present.
AndyM
^ permalink raw reply [flat|nested] 48+ messages in thread* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2015-12-18 10:05 bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems Demetri Obenour
` (4 preceding siblings ...)
2016-01-19 21:48 ` Andy Moreton
@ 2016-01-20 14:06 ` Andy Moreton
2016-01-20 14:12 ` Eli Zaretskii
2016-01-20 15:15 ` Andy Moreton
6 siblings, 1 reply; 48+ messages in thread
From: Andy Moreton @ 2016-01-20 14:06 UTC (permalink / raw)
To: 22202
On Tue 19 Jan 2016, Glenn Morris wrote:
> Andy Moreton wrote:
>
>> - broke all builds configured with "--without-gnutls"
>
> AFAICS, you are mistaken.
I may well be, but please explain.
src/sysdep.c (at emacs-25 commit c5ee6de21d4b) contains:
#include "gnutls.h"
#if 0x020c00 <= GNUTLS_VERSION_NUMBER && !defined WINDOWSNT
# include <gnutls/crypto.h>
#else
# define emacs_gnutls_global_init() Qnil
# define gnutls_rnd(level, data, len) (-1)
#endif
How can this build properly on a non-Windows system which does not
contain an installed "gnutls.h" header ?
AndyM
^ permalink raw reply [flat|nested] 48+ messages in thread* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2016-01-20 14:06 ` Andy Moreton
@ 2016-01-20 14:12 ` Eli Zaretskii
0 siblings, 0 replies; 48+ messages in thread
From: Eli Zaretskii @ 2016-01-20 14:12 UTC (permalink / raw)
To: Andy Moreton; +Cc: 22202
> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Wed, 20 Jan 2016 14:06:42 +0000
>
> On Tue 19 Jan 2016, Glenn Morris wrote:
>
> > Andy Moreton wrote:
> >
> >> - broke all builds configured with "--without-gnutls"
> >
> > AFAICS, you are mistaken.
>
> I may well be, but please explain.
> src/sysdep.c (at emacs-25 commit c5ee6de21d4b) contains:
>
> #include "gnutls.h"
> #if 0x020c00 <= GNUTLS_VERSION_NUMBER && !defined WINDOWSNT
> # include <gnutls/crypto.h>
> #else
> # define emacs_gnutls_global_init() Qnil
> # define gnutls_rnd(level, data, len) (-1)
> #endif
>
> How can this build properly on a non-Windows system which does not
> contain an installed "gnutls.h" header ?
That's src/gnutls.h header that comes with Emacs sources.
^ permalink raw reply [flat|nested] 48+ messages in thread
* bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems
2015-12-18 10:05 bug#22202: 24.5; SECURITY ISSUE -- Emacs Server vulnerable to random number generator attack on Windows systems Demetri Obenour
` (5 preceding siblings ...)
2016-01-20 14:06 ` Andy Moreton
@ 2016-01-20 15:15 ` Andy Moreton
6 siblings, 0 replies; 48+ messages in thread
From: Andy Moreton @ 2016-01-20 15:15 UTC (permalink / raw)
To: 22202
On Wed 20 Jan 2016, Eli Zaretskii wrote:
>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Wed, 20 Jan 2016 14:06:42 +0000
>>
>> On Tue 19 Jan 2016, Glenn Morris wrote:
>>
>> > Andy Moreton wrote:
>> >
>> >> - broke all builds configured with "--without-gnutls"
>> >
>> > AFAICS, you are mistaken.
>>
>> I may well be, but please explain.
>> src/sysdep.c (at emacs-25 commit c5ee6de21d4b) contains:
>>
>> #include "gnutls.h"
>> #if 0x020c00 <= GNUTLS_VERSION_NUMBER && !defined WINDOWSNT
>> # include <gnutls/crypto.h>
>> #else
>> # define emacs_gnutls_global_init() Qnil
>> # define gnutls_rnd(level, data, len) (-1)
>> #endif
>>
>> How can this build properly on a non-Windows system which does not
>> contain an installed "gnutls.h" header ?
>
> That's src/gnutls.h header that comes with Emacs sources.
Thanks - I knew it had to be something obvious. Sorry for the noise.
AndyM
^ permalink raw reply [flat|nested] 48+ messages in thread