NetBSD Problem Report #25367
Received: (qmail 14639 invoked by uid 605); 28 Apr 2004 15:31:34 -0000
Message-Id: <200404281531.i3SFV1BY009211@apb.cequrux.com>
Date: Wed, 28 Apr 2004 17:31:01 +0200 (SAST)
From: apb@cequrux.com
Sender: gnats-bugs-owner@NetBSD.org
To: gnats-bugs@gnats.NetBSD.org
Subject: arc4random state is shared across forks
X-Send-Pr-Version: 3.95
>Number: 25367
>Category: lib
>Synopsis: arc4random state is shared across forks
>Confidential: no
>Severity: serious
>Priority: high
>Responsible: lib-bug-people
>State: closed
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Wed Apr 28 15:32:00 +0000 2004
>Closed-Date: Mon Jul 21 08:21:46 +0000 2014
>Last-Modified: Mon Jul 21 08:21:46 +0000 2014
>Originator: Alan Barrett
>Release: NetBSD 1.6ZK
>Organization:
Not much
>Environment:
System: NetBSD 1.6ZK
Architecture: i386
Machine: i386
>Description:
arc4random() returns the same stream of values in all children of
a common parent process. This violates the principle of least
astonishment.
>How-To-Repeat:
sh$ ./test_arc4random
top-level process 28045: toprand=0xb70ed1c5
child process 22848: childrand[0]=0x617d6e3e
child process 22848: childrand[1]=0xd9af1d32
child process 14579: childrand[0]=0x617d6e3e
child process 14579: childrand[1]=0xd9af1d32
child process 5365: childrand[0]=0x617d6e3e
child process 5365: childrand[1]=0xd9af1d32
child process 17748: childrand[0]=0x617d6e3e
child process 17748: childrand[1]=0xd9af1d32
child process 17560: childrand[0]=0x617d6e3e
child process 17560: childrand[1]=0xd9af1d32
sh$ cat ./test_arc4random.c
#include <unistd.h>
#include <stdlib.h>
#include <inttypes.h>
#include <sys/wait.h>
#define CHILDREN 5 /* number of children to run */
#define RAND_PER_CHILD 2 /* number of arc4random() calls per child */
int
main(int argc, char **argv)
{
int nchildren = 0;
int nerrors = 0;
pid_t toppid = getpid();
u_int32_t toprand;
/* initialise arc4random()'s private data in the parent */
toprand = arc4random();
printf("top-level process %lu: toprand=0x%" PRIx32 "\n",
(unsigned long)toppid, toprand);
fflush(stdout);
for (nerrors = nchildren = 0; nchildren < CHILDREN && nerrors <= 10; ) {
pid_t forkresult = fork();
if (forkresult < 0) {
nerrors++;
sleep(1);
} else if (forkresult == 0) {
pid_t childpid = getpid();
u_int32_t childrand;
int i;
for (i = 0; i < RAND_PER_CHILD; i++) {
childrand = arc4random();
printf("child process %lu: childrand[%d]=0x%" PRIx32 "\n",
(unsigned long)childpid, i, childrand);
}
fflush(stdout);
exit(0);
} else {
nchildren++;
(void) wait(NULL);
}
}
exit(0);
}
>Fix:
Somehow make arc4random re-seed itself after a fork().
Since there seems to be no fork_hooks infrastructure, I suppose we could
save the pid somewhere and re-seed if getpid() says the pid has changed.
>Release-Note:
>Audit-Trail:
From: David Laight <david@l8s.co.uk>
To: gnats@netbsd.org
Cc:
Subject: Re: lib/25367: arc4random state is shared across forks
Date: Sun, 19 Aug 2012 09:01:48 +0100
Just bumped into this one while looking at other arc4random bugs.
There are two freebsd threads from september 2008 explaining that
openbsd had added a getpid() call prior to every arc4random() one
in order to detect this - butthat is slow. See:
http://lists.freebsd.org/pipermail/freebsd-current/2008-September/088552.html
There is a more general problem of detecting fork() - arc4random()
isn't the only affected code.
Optimising getpid() (or having an optimised getpid() function) is one
possibility. All that requires is for libc to zero the cached pid
value in the child of fork().
Although vfork() and others? may need special consideration.
David
--
David Laight: david@l8s.co.uk
From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: lib/25367: arc4random state is shared across forks
Date: Wed, 22 Aug 2012 04:06:06 +0000
On Sun, Aug 19, 2012 at 04:25:02PM +0000, David Laight wrote:
> Optimising getpid() (or having an optimised getpid() function) is one
> possibility. All that requires is for libc to zero the cached pid
> value in the child of fork().
> Although vfork() and others? may need special consideration.
I think it's ok for a vfork child to share the arc4random state with
its parent, because it'll be shared read/write rather than copied.
--
David A. Holland
dholland@netbsd.org
From: Roy Marples <roy@marples.name>
To: gnats-bugs@netbsd.org
Cc: apb@cequrux.com, David Laight <david@l8s.co.uk>, David Holland
<dholland-bugs@netbsd.org>
Subject: Re: lib/25367: arc4random state is shared across forks
Date: Sun, 25 May 2014 08:30:35 +0100
--=_78a787e1cccc4ca0768aab6f423d85a8
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset=UTF-8;
format=flowed
Hi
I bumped into this as well.
I looked over the FreeBSD thread regarding potential speed issues and
despite any concerns they have actually merged the change in.
Attached is a patch to -current which does two things.
1) Re-stirs if the pid changes
2) Re-stirs after swallowing 1600000 bytes (which also addresses
PR/45952)
I've been running this on my laptop for a while with great success and
unless anyone objects I'll commit it shortly.
Thanks
Roy
--=_78a787e1cccc4ca0768aab6f423d85a8
Content-Transfer-Encoding: base64
Content-Type: text/x-diff;
name=arc4random-pid.diff
Content-Disposition: attachment;
filename=arc4random-pid.diff;
size=3981
PyB0ZXN0Cj8gdGVzdC5jCkluZGV4OiBnZW4vYXJjNHJhbmRvbS5jCj09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KUkNTIGZp
bGU6IC9jdnNyb290L3NyYy9saWIvbGliYy9nZW4vYXJjNHJhbmRvbS5jLHYKcmV0cmlldmluZyBy
ZXZpc2lvbiAxLjIxCmRpZmYgLXUgLXAgLXIxLjIxIGFyYzRyYW5kb20uYwotLS0gZ2VuL2FyYzRy
YW5kb20uYwkxNyBPY3QgMjAxMyAyMzo1NjoxNyAtMDAwMAkxLjIxCisrKyBnZW4vYXJjNHJhbmRv
bS5jCTI0IE1heSAyMDE0IDIxOjQwOjEzIC0wMDAwCkBAIC00OSwxMSArNDksMTEgQEAgX193ZWFr
X2FsaWFzKGFyYzRyYW5kb21fdW5pZm9ybSxfYXJjNHJhbgogI2VuZGlmCiAKIHN0cnVjdCBhcmM0
X3N0cmVhbSB7Ci0JdWludDhfdCBzdGlycmVkOwotCXVpbnQ4X3QgcGFkOwogCXVpbnQ4X3QgaTsK
IAl1aW50OF90IGo7CiAJdWludDhfdCBzWyh1aW50OF90KX4wdSArIDF1XTsJLyogMjU2IHRvIHlv
dSBhbmQgbWUgKi8KKwlzaXplX3QgY291bnQ7CisJcGlkX3Qgc3Rpcl9waWQ7CiAJbXV0ZXhfdCBt
dHg7CiB9OwogCkBAIC02Nyw3ICs2Nyw3IEBAIHN0cnVjdCBhcmM0X3N0cmVhbSB7CiAJCQltdXRl
eF91bmxvY2soJihycyktPm10eCk7ICAgICAgXAogCX0KICNlbHNlCi0jZGVmaW5lIExPQ0socnMp
IAorI2RlZmluZSBMT0NLKHJzKQogI2RlZmluZSBVTkxPQ0socnMpCiAjZW5kaWYKIApAQCAtNzgs
NyArNzgsNyBAQCBzdHJ1Y3QgYXJjNF9zdHJlYW0gewogI2RlZmluZSBTMjU2IFM2NCgwKSwgUzY0
KDY0KSwgUzY0KDEyOCksIFM2NCgxOTIpCiAKIHN0YXRpYyBzdHJ1Y3QgYXJjNF9zdHJlYW0gcnMg
PSB7IC5pID0gMHhmZiwgLmogPSAwLCAucyA9IHsgUzI1NiB9LAotCQkuc3RpcnJlZCA9IDAsIC5t
dHggPSBNVVRFWF9JTklUSUFMSVpFUiB9OworCQkuY291bnQgPSAwLCAuc3Rpcl9waWQgPSAwLCAu
bXR4ID0gTVVURVhfSU5JVElBTElaRVIgfTsKIAogI3VuZGVmIFMKICN1bmRlZiBTNApAQCAtODcs
MjAgKzg3LDEwIEBAIHN0YXRpYyBzdHJ1Y3QgYXJjNF9zdHJlYW0gcnMgPSB7IC5pID0gMHgKICN1
bmRlZiBTMjU2CiAKIHN0YXRpYyBpbmxpbmUgdm9pZCBhcmM0X2FkZHJhbmRvbShzdHJ1Y3QgYXJj
NF9zdHJlYW0gKiwgdV9jaGFyICosIGludCk7Ci1zdGF0aWMgX19ub2lubGluZSB2b2lkIGFyYzRf
c3RpcihzdHJ1Y3QgYXJjNF9zdHJlYW0gKik7CitzdGF0aWMgX19ub2lubGluZSB2b2lkIGFyYzRf
c3RpcihzdHJ1Y3QgYXJjNF9zdHJlYW0gKiwgcGlkX3QpOwogc3RhdGljIGlubGluZSB1aW50OF90
IGFyYzRfZ2V0Ynl0ZShzdHJ1Y3QgYXJjNF9zdHJlYW0gKik7CiBzdGF0aWMgaW5saW5lIHVpbnQz
Ml90IGFyYzRfZ2V0d29yZChzdHJ1Y3QgYXJjNF9zdHJlYW0gKik7CiAKLXN0YXRpYyBpbmxpbmUg
aW50Ci1hcmM0X2NoZWNrX2luaXQoc3RydWN0IGFyYzRfc3RyZWFtICphcykKLXsKLQlpZiAoX19w
cmVkaWN0X3RydWUocnMuc3RpcnJlZCkpCi0JCXJldHVybiAwOwotCi0JYXJjNF9zdGlyKGFzKTsK
LQlyZXR1cm4gMTsKLX0KLQogc3RhdGljIGlubGluZSB2b2lkCiBhcmM0X2FkZHJhbmRvbShzdHJ1
Y3QgYXJjNF9zdHJlYW0gKmFzLCB1X2NoYXIgKmRhdCwgaW50IGRhdGxlbikKIHsKQEAgLTExNyw3
ICsxMDcsNyBAQCBhcmM0X2FkZHJhbmRvbShzdHJ1Y3QgYXJjNF9zdHJlYW0gKmFzLCB1CiB9CiAK
IHN0YXRpYyBfX25vaW5saW5lIHZvaWQKLWFyYzRfc3RpcihzdHJ1Y3QgYXJjNF9zdHJlYW0gKmFz
KQorYXJjNF9zdGlyKHN0cnVjdCBhcmM0X3N0cmVhbSAqYXMsIHBpZF90IHBpZCkKIHsKIAlpbnQg
cmRhdFszMl07CiAJaW50IG1pYltdID0geyBDVExfS0VSTiwgS0VSTl9VUk5EIH07CkBAIC0xNDYs
MTAgKzEzNiwyNCBAQCBhcmM0X3N0aXIoc3RydWN0IGFyYzRfc3RyZWFtICphcykKIAkgKiBwYXBl
ciAiV2Vha25lc3NlcyBpbiB0aGUgS2V5IFNjaGVkdWxpbmcgQWxnb3JpdGhtIG9mIFJDNCIKIAkg
KiBieSBGbHVoZXIsIE1hbnRpbiwgYW5kIFNoYW1pci4gIChOID0gMjU2IGluIG91ciBjYXNlLikK
IAkgKi8KLQlmb3IgKGogPSAwOyBqIDwgX19hcnJheWNvdW50KGFzLT5zKSAqIDQ7IGorKykKKwlm
b3IgKGogPSAwOyBqIDwgX19hcnJheWNvdW50KGFzLT5zKSAqIHNpemVvZih1aW50MzJfdCk7IGor
KykKIAkJYXJjNF9nZXRieXRlKGFzKTsKIAotCWFzLT5zdGlycmVkID0gMTsKKwkvKiBTdGlyIGFn
YWluIGFmdGVyIHN3YWxsb3dpbmcgMTYwMDAwMCBieXRlcyBvciBpZiB0aGUgcGlkIGNoYW5nZXMg
Ki8KKwlhcy0+Y291bnQgPSAxNjAwMDAwOworCWFzLT5zdGlyX3BpZCA9IHBpZDsKK30KKworc3Rh
dGljIGlubGluZSB2b2lkCithcmM0X3N0aXJfaWZfbmVlZGVkKHN0cnVjdCBhcmM0X3N0cmVhbSAq
YXMsIHNpemVfdCBsZW4pCit7CisJcGlkX3QgcGlkOworCisJcGlkID0gZ2V0cGlkKCk7CisJaWYg
KGFzLT5jb3VudCA8PSBsZW4gfHwgYXMtPnN0aXJfcGlkICE9IHBpZCkKKwkJYXJjNF9zdGlyKGFz
LCBwaWQpOworCWVsc2UKKwkJYXMtPmNvdW50IC09IGxlbjsKIH0KIAogc3RhdGljIF9faW5saW5l
IHVpbnQ4X3QKQEAgLTE2OSw2ICsxNzMsNyBAQCBhcmM0X2dldGJ5dGVfaWooc3RydWN0IGFyYzRf
c3RyZWFtICphcywgCiBzdGF0aWMgaW5saW5lIHVpbnQ4X3QKIGFyYzRfZ2V0Ynl0ZShzdHJ1Y3Qg
YXJjNF9zdHJlYW0gKmFzKQogeworCiAJcmV0dXJuIGFyYzRfZ2V0Ynl0ZV9paihhcywgJmFzLT5p
LCAmYXMtPmopOwogfQogCkBAIC0xNzYsNiArMTgxLDcgQEAgc3RhdGljIGlubGluZSB1aW50MzJf
dAogYXJjNF9nZXR3b3JkKHN0cnVjdCBhcmM0X3N0cmVhbSAqYXMpCiB7CiAJdWludDMyX3QgdmFs
OworCiAJdmFsID0gYXJjNF9nZXRieXRlKGFzKSA8PCAyNDsKIAl2YWwgfD0gYXJjNF9nZXRieXRl
KGFzKSA8PCAxNjsKIAl2YWwgfD0gYXJjNF9nZXRieXRlKGFzKSA8PCA4OwpAQCAtMTg2LDE2ICsx
OTIsMTggQEAgYXJjNF9nZXR3b3JkKHN0cnVjdCBhcmM0X3N0cmVhbSAqYXMpCiB2b2lkCiBhcmM0
cmFuZG9tX3N0aXIodm9pZCkKIHsKKwogCUxPQ0soJnJzKTsKLQlhcmM0X3N0aXIoJnJzKTsKKwlh
cmM0X3N0aXIoJnJzLCBnZXRwaWQoKSk7CiAJVU5MT0NLKCZycyk7CiB9CiAKIHZvaWQKIGFyYzRy
YW5kb21fYWRkcmFuZG9tKHVfY2hhciAqZGF0LCBpbnQgZGF0bGVuKQogeworCiAJTE9DSygmcnMp
OwotCWFyYzRfY2hlY2tfaW5pdCgmcnMpOworCWFyYzRfc3Rpcl9pZl9uZWVkZWQoJnJzLCBkYXRs
ZW4pOwogCWFyYzRfYWRkcmFuZG9tKCZycywgZGF0LCBkYXRsZW4pOwogCVVOTE9DSygmcnMpOwog
fQpAQCAtMjA2LDcgKzIxNCw3IEBAIGFyYzRyYW5kb20odm9pZCkKIAl1aW50MzJfdCB2OwogCiAJ
TE9DSygmcnMpOwotCWFyYzRfY2hlY2tfaW5pdCgmcnMpOworCWFyYzRfc3Rpcl9pZl9uZWVkZWQo
JnJzLCBzaXplb2YodikpOwogCXYgPSBhcmM0X2dldHdvcmQoJnJzKTsKIAlVTkxPQ0soJnJzKTsK
IAlyZXR1cm4gdjsKQEAgLTIyMCw3ICsyMjgsNyBAQCBhcmM0cmFuZG9tX2J1Zih2b2lkICpidWYs
IHNpemVfdCBsZW4pCiAJdWludDhfdCBpLCBqOwogCiAJTE9DSygmcnMpOwotCWFyYzRfY2hlY2tf
aW5pdCgmcnMpOworCWFyYzRfc3Rpcl9pZl9uZWVkZWQoJnJzLCBsZW4pOwogCiAJLyogY2FjaGUg
aSBhbmQgaiAtIGNvbXBpbGVyIGNhbid0IGtub3cgJ2J1ZicgZG9lc24ndCBhbGlhcyB0aGVtICov
CiAJaSA9IHJzLmk7CkBAIC0yNjMsNyArMjcxLDcgQEAgYXJjNHJhbmRvbV91bmlmb3JtKHVpbnQz
Ml90IHVwcGVyX2JvdW5kKQogCW1pbiA9ICgweEZGRkZGRkZGVSAtIHVwcGVyX2JvdW5kICsgMSkg
JSB1cHBlcl9ib3VuZDsKIAogCUxPQ0soJnJzKTsKLQlhcmM0X2NoZWNrX2luaXQoJnJzKTsKKwlh
cmM0X3N0aXJfaWZfbmVlZGVkKCZycywgc2l6ZW9mKHIpKTsKIAogCS8qCiAJICogVGhpcyBjb3Vs
ZCB0aGVvcmV0aWNhbGx5IGxvb3AgZm9yZXZlciBidXQgZWFjaCByZXRyeSBoYXMK
--=_78a787e1cccc4ca0768aab6f423d85a8--
From: Joerg Sonnenberger <joerg@britannica.bec.de>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: lib/25367: arc4random state is shared across forks
Date: Sun, 25 May 2014 23:02:34 +0200
This is wrong, it should just use pthread_atfork without penalizing the
common case.
Joerg
From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@netbsd.org
Cc:
Subject: Re: lib/25367: arc4random state is shared across forks
Date: Fri, 30 May 2014 08:19:43 +0000
here's another copy of the mail above (that got sent to the wrong
place) in which the patch isn't base64'd:
------
From: Roy Marples <roy@marples.name>
To: gnats@netbsd.org
Subject: Re: lib/25367: arc4random state is shared across forks
Date: Sat, 24 May 2014 23:07:07 +0100
Hi
I bumped into this as well.
I looked over the FreeBSD thread regarding potential speed issues and despite
any concerns they have actually merged the change in.
Attached is a patch to -current which does two things.
1) Re-stirs if the pid changes
2) Re-stirs after swallowing 1600000 bytes (which also addresses PR/45952)
I've been running this on my laptop for a while with great success and unless
anyone objects I'll commit it shortly.
Thanks
Roy
? test
? test.c
Index: gen/arc4random.c
===================================================================
RCS file: /cvsroot/src/lib/libc/gen/arc4random.c,v
retrieving revision 1.21
diff -u -p -r1.21 arc4random.c
--- gen/arc4random.c 17 Oct 2013 23:56:17 -0000 1.21
+++ gen/arc4random.c 24 May 2014 21:40:13 -0000
@@ -49,11 +49,11 @@ __weak_alias(arc4random_uniform,_arc4ran
#endif
struct arc4_stream {
- uint8_t stirred;
- uint8_t pad;
uint8_t i;
uint8_t j;
uint8_t s[(uint8_t)~0u + 1u]; /* 256 to you and me */
+ size_t count;
+ pid_t stir_pid;
mutex_t mtx;
};
@@ -67,7 +67,7 @@ struct arc4_stream {
mutex_unlock(&(rs)->mtx); \
}
#else
-#define LOCK(rs)
+#define LOCK(rs)
#define UNLOCK(rs)
#endif
@@ -78,7 +78,7 @@ struct arc4_stream {
#define S256 S64(0), S64(64), S64(128), S64(192)
static struct arc4_stream rs = { .i = 0xff, .j = 0, .s = { S256 },
- .stirred = 0, .mtx = MUTEX_INITIALIZER };
+ .count = 0, .stir_pid = 0, .mtx = MUTEX_INITIALIZER };
#undef S
#undef S4
@@ -87,20 +87,10 @@ static struct arc4_stream rs = { .i = 0x
#undef S256
static inline void arc4_addrandom(struct arc4_stream *, u_char *, int);
-static __noinline void arc4_stir(struct arc4_stream *);
+static __noinline void arc4_stir(struct arc4_stream *, pid_t);
static inline uint8_t arc4_getbyte(struct arc4_stream *);
static inline uint32_t arc4_getword(struct arc4_stream *);
-static inline int
-arc4_check_init(struct arc4_stream *as)
-{
- if (__predict_true(rs.stirred))
- return 0;
-
- arc4_stir(as);
- return 1;
-}
-
static inline void
arc4_addrandom(struct arc4_stream *as, u_char *dat, int datlen)
{
@@ -117,7 +107,7 @@ arc4_addrandom(struct arc4_stream *as, u
}
static __noinline void
-arc4_stir(struct arc4_stream *as)
+arc4_stir(struct arc4_stream *as, pid_t pid)
{
int rdat[32];
int mib[] = { CTL_KERN, KERN_URND };
@@ -146,10 +136,24 @@ arc4_stir(struct arc4_stream *as)
* paper "Weaknesses in the Key Scheduling Algorithm of RC4"
* by Fluher, Mantin, and Shamir. (N = 256 in our case.)
*/
- for (j = 0; j < __arraycount(as->s) * 4; j++)
+ for (j = 0; j < __arraycount(as->s) * sizeof(uint32_t); j++)
arc4_getbyte(as);
- as->stirred = 1;
+ /* Stir again after swallowing 1600000 bytes or if the pid changes */
+ as->count = 1600000;
+ as->stir_pid = pid;
+}
+
+static inline void
+arc4_stir_if_needed(struct arc4_stream *as, size_t len)
+{
+ pid_t pid;
+
+ pid = getpid();
+ if (as->count <= len || as->stir_pid != pid)
+ arc4_stir(as, pid);
+ else
+ as->count -= len;
}
static __inline uint8_t
@@ -169,6 +173,7 @@ arc4_getbyte_ij(struct arc4_stream *as,
static inline uint8_t
arc4_getbyte(struct arc4_stream *as)
{
+
return arc4_getbyte_ij(as, &as->i, &as->j);
}
@@ -176,6 +181,7 @@ static inline uint32_t
arc4_getword(struct arc4_stream *as)
{
uint32_t val;
+
val = arc4_getbyte(as) << 24;
val |= arc4_getbyte(as) << 16;
val |= arc4_getbyte(as) << 8;
@@ -186,16 +192,18 @@ arc4_getword(struct arc4_stream *as)
void
arc4random_stir(void)
{
+
LOCK(&rs);
- arc4_stir(&rs);
+ arc4_stir(&rs, getpid());
UNLOCK(&rs);
}
void
arc4random_addrandom(u_char *dat, int datlen)
{
+
LOCK(&rs);
- arc4_check_init(&rs);
+ arc4_stir_if_needed(&rs, datlen);
arc4_addrandom(&rs, dat, datlen);
UNLOCK(&rs);
}
@@ -206,7 +214,7 @@ arc4random(void)
uint32_t v;
LOCK(&rs);
- arc4_check_init(&rs);
+ arc4_stir_if_needed(&rs, sizeof(v));
v = arc4_getword(&rs);
UNLOCK(&rs);
return v;
@@ -220,7 +228,7 @@ arc4random_buf(void *buf, size_t len)
uint8_t i, j;
LOCK(&rs);
- arc4_check_init(&rs);
+ arc4_stir_if_needed(&rs, len);
/* cache i and j - compiler can't know 'buf' doesn't alias them */
i = rs.i;
@@ -263,7 +271,7 @@ arc4random_uniform(uint32_t upper_bound)
min = (0xFFFFFFFFU - upper_bound + 1) % upper_bound;
LOCK(&rs);
- arc4_check_init(&rs);
+ arc4_stir_if_needed(&rs, sizeof(r));
/*
* This could theoretically loop forever but each retry has
From: Roy Marples <roy@marples.name>
To: gnats-bugs@netbsd.org
Cc: apb@cequrux.com, David Laight <david@l8s.co.uk>,
dholland-bugs@netbsd.org, Joerg Sonnenberger <joerg@britannica.bec.de>
Subject: Re: lib/25367: arc4random state is shared across forks
Date: Sat, 31 May 2014 01:15:34 +0100
Thanks Joerg!
I learned a new function :)
Posted inline (gnats sucks) is a new patch using pthread_atfork.
However, our man page and online documentation I can find is sadly
lacking in a few places so I'm not entirely sure the patch is correct as
no locking/unlocking is done at fork time and I don't know if it's
needed or not. Not really a threading expert :)
Saying that, this patch has been running on my server and workstation
for a few days now with no adverse effects.
Comments welcome.
Roy
Index: gen/arc4random.c
===================================================================
RCS file: /cvsroot/src/lib/libc/gen/arc4random.c,v
retrieving revision 1.21
diff -u -p -r1.21 arc4random.c
--- gen/arc4random.c 17 Oct 2013 23:56:17 -0000 1.21
+++ gen/arc4random.c 31 May 2014 00:03:55 -0000
@@ -33,6 +33,7 @@ __RCSID("$NetBSD: arc4random.c,v 1.21 20
#include "namespace.h"
#include "reentrant.h"
#include <fcntl.h>
+#include <pthread.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/types.h>
@@ -49,11 +50,12 @@ __weak_alias(arc4random_uniform,_arc4ran
#endif
struct arc4_stream {
- uint8_t stirred;
+ uint8_t inited;
uint8_t pad;
uint8_t i;
uint8_t j;
uint8_t s[(uint8_t)~0u + 1u]; /* 256 to you and me */
+ size_t count;
mutex_t mtx;
};
@@ -67,7 +69,7 @@ struct arc4_stream {
mutex_unlock(&(rs)->mtx); \
}
#else
-#define LOCK(rs)
+#define LOCK(rs)
#define UNLOCK(rs)
#endif
@@ -77,8 +79,8 @@ struct arc4_stream {
#define S64(n) S16(n), S16(n + 16), S16(n + 32), S16(n + 48)
#define S256 S64(0), S64(64), S64(128), S64(192)
-static struct arc4_stream rs = { .i = 0xff, .j = 0, .s = { S256 },
- .stirred = 0, .mtx = MUTEX_INITIALIZER };
+static struct arc4_stream rs = { .inited = 0, .i = 0xff, .j = 0, .s = {
S256 },
+ .count = 0, .mtx = MUTEX_INITIALIZER };
#undef S
#undef S4
@@ -91,14 +93,22 @@ static __noinline void arc4_stir(struct
static inline uint8_t arc4_getbyte(struct arc4_stream *);
static inline uint32_t arc4_getword(struct arc4_stream *);
-static inline int
+static void
+arc4_forked(void)
+{
+
+ /* Reset the counter to a force new stir after forking */
+ rs.count = 0;
+}
+
+static inline void
arc4_check_init(struct arc4_stream *as)
{
- if (__predict_true(rs.stirred))
- return 0;
- arc4_stir(as);
- return 1;
+ if (!__predict_true(as->inited)) {
+ as->inited = 1;
+ pthread_atfork(NULL, NULL, arc4_forked);
+ }
}
static inline void
@@ -124,6 +134,8 @@ arc4_stir(struct arc4_stream *as)
size_t len;
size_t i, j;
+ arc4_check_init(as);
+ */
- for (j = 0; j < __arraycount(as->s) * 4; j++)
+ for (j = 0; j < __arraycount(as->s) * sizeof(uint32_t); j++)
arc4_getbyte(as);
- as->stirred = 1;
+ /* Stir again after swallowing 1600000 bytes or if the pid
changes */
+ as->count = 1600000;
+}
+
+static inline void
+arc4_stir_if_needed(struct arc4_stream *as, size_t len)
+{
+
+ if (as->count <= len)
+ arc4_stir(as);
+ else
+ as->count -= len;
}
static __inline uint8_t
@@ -169,6 +192,7 @@ arc4_getbyte_ij(struct arc4_stream *as,
static inline uint8_t
arc4_getbyte(struct arc4_stream *as)
{
+
return arc4_getbyte_ij(as, &as->i, &as->j);
}
@@ -176,6 +200,7 @@ static inline uint32_t
arc4_getword(struct arc4_stream *as)
{
uint32_t val;
+
val = arc4_getbyte(as) << 24;
val |= arc4_getbyte(as) << 16;
val |= arc4_getbyte(as) << 8;
@@ -186,6 +211,7 @@ arc4_getword(struct arc4_stream *as)
void
arc4random_stir(void) UNLOCK(&rs);
@@ -194,8 +220,9 @@ arc4random_stir(void)
void
arc4random_addrandom(u_char *dat, int datlen)
{
+
LOCK(&rs);
- arc4_check_init(&rs);
+ arc4_stir_if_needed(&rs, datlen);
arc4_addrandom(&rs, dat, datlen);
UNLOCK(&rs);
}
@@ -206,7 +233,7 @@ arc4random(void)
uint32_t v;
LOCK(&rs);
- arc4_check_init(&rs);
+ arc4_stir_if_needed(&rs, sizeof(v));
v = arc4_getword(&rs);
UNLOCK(&rs);
return v;
@@ -220,7 +247,7 @@ arc4random_buf(void *buf, size_t len)
uint8_t i, j;
LOCK(&rs);
- arc4_check_init(&rs);
+ arc4_stir_if_needed(&rs, len);
/* cache i and j - compiler can't know 'buf' doesn't alias them
*/
i = rs.i;
@@ -263,7 +290,7 @@ arc4random_uniform(uint32_t upper_bound)
min = (0xFFFFFFFFU - upper_bound + 1) % upper_bound;
LOCK(&rs);
- arc4_check_init(&rs);
+ arc4_stir_if_needed(&rs, sizeof(r));
/*
* This could theoretically loop forever but each retry has
{
+
LOCK(&rs);
arc4_stir(&rs);
/*
* This code once opened and read /dev/urandom on each
* call. That causes repeated rekeying of the kernel stream
@@ -146,10 +158,21 @@ arc4_stir(struct arc4_stream *as)
* paper "Weaknesses in the Key Scheduling Algorithm of RC4"
* by Fluher, Mantin, and Shamir. (N = 256 in our case.)
From: "Roy Marples" <roy@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/25367 CVS commit: src/lib/libc/gen
Date: Sat, 7 Jun 2014 20:55:47 +0000
Module Name: src
Committed By: roy
Date: Sat Jun 7 20:55:47 UTC 2014
Modified Files:
src/lib/libc/gen: arc4random.c
Log Message:
Re-stir after forking, fixes PR lib/25367.
Re-stir after consuming 1600000 bytes, fixes PR lib/45952.
To generate a diff of this commit:
cvs rdiff -u -r1.21 -r1.22 src/lib/libc/gen/arc4random.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
State-Changed-From-To: open->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Mon, 21 Jul 2014 08:21:46 +0000
State-Changed-Why:
fixed, thanks
>Unformatted:
(Contact us)
$NetBSD: query-full-pr,v 1.39 2013/11/01 18:47:49 spz Exp $
$NetBSD: gnats_config.sh,v 1.8 2006/05/07 09:23:38 tsutsui Exp $
Copyright © 1994-2007
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.
Home