From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id BAB203858C50 for ; Tue, 12 Jul 2022 08:57:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org BAB203858C50 Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-196-oThEl9iPO3mVcq-8w2WmEA-1; Tue, 12 Jul 2022 04:57:21 -0400 X-MC-Unique: oThEl9iPO3mVcq-8w2WmEA-1 Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id F3371811E9B; Tue, 12 Jul 2022 08:57:20 +0000 (UTC) Received: from oldenburg.str.redhat.com (unknown [10.39.192.160]) by smtp.corp.redhat.com (Postfix) with ESMTPS id C0D2B492C3B; Tue, 12 Jul 2022 08:57:19 +0000 (UTC) From: Florian Weimer To: Adhemerval Zanella Cc: libc-alpha@sourceware.org, Yann Droneaud Subject: Re: [PATCH v8 1/9] stdlib: Add arc4random, arc4random_buf, and arc4random_uniform (BZ #4417) References: <20220629213428.3065430-1-adhemerval.zanella@linaro.org> <20220629213428.3065430-2-adhemerval.zanella@linaro.org> Date: Tue, 12 Jul 2022 10:57:18 +0200 In-Reply-To: <20220629213428.3065430-2-adhemerval.zanella@linaro.org> (Adhemerval Zanella's message of "Wed, 29 Jun 2022 18:34:20 -0300") Message-ID: <871quqvhch.fsf@oldenburg.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.85 on 10.11.54.10 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-10.9 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 12 Jul 2022 08:57:25 -0000 * Adhemerval Zanella: > diff --git a/NEWS b/NEWS > index b0a3d7e512..f9dc316ead 100644 > --- a/NEWS > +++ b/NEWS > @@ -46,6 +46,10 @@ Major new features: > to more flexibly configure and operate on filesystem mounts. The new > mount APIs are specifically designed to work with namespaces. > =20 > +* The functions arc4random, arc4random_buf, and arc4random_uniform have = been > + added. The functions use a cryptographic pseudo-random number generat= or > + based on ChaCha20 initilized with entropy from the kernel. This implies that the generator is cryptographically strong. Maybe it should not mention ChaCha20 and =E2=80=9Ccryptographic=E2=80=9D. > diff --git a/include/stdlib.h b/include/stdlib.h > index 1c6f70b082..c5f5628f22 100644 > --- a/include/stdlib.h > +++ b/include/stdlib.h > +/* Called from the fork function to reinitialize the internal lock in th= e > + child process. This avoids deadlocks if fork is called in multi-thre= aded > + processes. */ > +extern void __arc4random_fork_subprocess (void) attribute_hidden; Looks like the comment is outdated. There isn't a lock anymore. > diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c > index 01a282f3f6..ada65d40c2 100644 > --- a/nptl/allocatestack.c > +++ b/nptl/allocatestack.c > @@ -559,6 +560,8 @@ allocate_stack (const struct pthread_attr *attr, stru= ct pthread **pdp, > #endif > pd->robust_head.list =3D &pd->robust_head; > =20 > + __glibc_tls_internal_init (&pd->tls_state); I think this is a bit confusing because it's not aligned with the main thread. memset would probably be clearer because it mirrors setup for the main thread (zeroed implicitly). > diff --git a/stdlib/arc4random.c b/stdlib/arc4random.c > new file mode 100644 > index 0000000000..92d1da92cc > --- /dev/null > +++ b/stdlib/arc4random.c > @@ -0,0 +1,207 @@ > +/* Pseudo Random Number Generator based on ChaCha20. > + Copyright (C) 2020 Free Software Foundation, Inc. Copyright year 2022. Please check the other files, too. > +/* arc4random keeps two counters: 'have' is the current valid bytes not = yet > + consumed in 'buf' while 'count' is the maximum number of bytes until = a > + reseed. > + > + Both the initial seed and reseed try to obtain entropy from the kerne= l > + and abort the process if none could be obtained. > + > + The state 'buf' improves the usage of the cipher calls, allowing to c= all > + optimized implementations (if the architecture provides it) and optim= ize > + arc4random calls (since only multiple calls it will encrypt the next > + block). */ I don't understand the =E2=80=9Csince only multiple calls it will encrypt t= he next block=E2=80=9D part. > +/* Called from the fork function to reset the state. */ > +void > +__arc4random_fork_subprocess (void) > +{ > + struct arc4random_state_t *state =3D __glibc_tls_internal()->rand_stat= e; > + if (state !=3D NULL) > + { > + explicit_bzero (state, sizeof (struct arc4random_state_t)); sizeof (*state)? > + /* Force key init. */ > + state->count =3D -1; > + } > +} > + > +/* Return the current thread random state or try to create one if there = is > + none available. In the case malloc can not allocate a state, arc4ran= dom > + will try to get entropy with arc4random_getentropy. */ > +static struct arc4random_state_t * > +arc4random_get_state (void) > +{ > + struct arc4random_state_t *state =3D __glibc_tls_internal()->rand_stat= e; Missing space before (). > + if (state =3D=3D NULL) > + { > + state =3D malloc (sizeof (struct arc4random_state_t)); > + if (state !=3D NULL) > +=09{ > +=09 /* Force key initialization on first call. */ > +=09 state->count =3D -1; > +=09 __glibc_tls_internal()->rand_state =3D state; Missing space before (). > +=09} > + } > + return state; > +} > + > +static void > +arc4random_getrandom_failure (void) > +{ > + __libc_fatal ("Fatal glibc error: cannot get entropy for arc4random\n"= ); > +} > + > +static void > +arc4random_rekey (struct arc4random_state_t *state, uint8_t *rnd, size_t= rndlen) > +{ > + chacha20_crypt (state->ctx, state->buf, state->buf, sizeof state->buf)= ; > + > + /* Mix some extra entropy if provided. */ > + if (rnd !=3D NULL) > + { > + size_t m =3D MIN (rndlen, CHACHA20_KEY_SIZE + CHACHA20_IV_SIZE); > + for (size_t i =3D 0; i < m; i++) > +=09state->buf[i] ^=3D rnd[i]; > + } > + > + /* Immediately reinit for backtracking resistance. */ > + chacha20_init (state->ctx, state->buf, state->buf + CHACHA20_KEY_SIZE)= ; > + memset (state->buf, 0, CHACHA20_KEY_SIZE + CHACHA20_IV_SIZE); > + state->have =3D sizeof (state->buf) - (CHACHA20_KEY_SIZE + CHACHA20_IV= _SIZE); > +} Should that memset be explicit_bzero for clarity? > +static void > +arc4random_getentropy (void *rnd, size_t len) > +{ > + if (__getrandom_nocancel (rnd, len, GRND_NONBLOCK) =3D=3D len) > + return; > + > + int fd =3D __open64_nocancel ("/dev/urandom", O_RDONLY | O_CLOEXEC); This could use TEMP_FAILURE_RETRY, too. > + if (fd !=3D -1) > + { > + uint8_t *p =3D rnd; > + uint8_t *end =3D p + len; > + do > +=09{ > +=09 ssize_t ret =3D TEMP_FAILURE_RETRY (__read_nocancel (fd, p, end - p= )); > +=09 if (ret <=3D 0) > +=09 arc4random_getrandom_failure (); > +=09 p +=3D ret; > +=09} > + while (p < end); > + > + if (__close_nocancel (fd) =3D=3D 0) > +=09return; > + } > + arc4random_getrandom_failure (); > +} > + > +/* Reinit the thread context by reseeding the cipher state with kernel > + entropy. */ > +static void > +arc4random_check_stir (struct arc4random_state_t *state, size_t len) Could you add a comment describing the len parameter? > +{ > + if (state->count < len || state->count =3D=3D -1) > + { > + uint8_t rnd[CHACHA20_KEY_SIZE + CHACHA20_IV_SIZE]; > + arc4random_getentropy (rnd, sizeof rnd); > + > + if (state->count =3D=3D -1) > +=09chacha20_init (state->ctx, rnd, rnd + CHACHA20_KEY_SIZE); > + else > +=09arc4random_rekey (state, rnd, sizeof rnd); > + > + explicit_bzero (rnd, sizeof rnd); > + > + state->have =3D 0; > + memset (state->buf, 0, sizeof state->buf); > + state->count =3D CHACHA20_RESEED_SIZE; > + } > + if (state->count <=3D len) > + state->count =3D 0; > + else > + state->count -=3D len; > +} It's not clear to me if we want to enter the true branch of the second if statement if we already executed the true branch of the first if statement. > +void > +__arc4random_buf (void *buffer, size_t len) > +{ > + struct arc4random_state_t *state =3D arc4random_get_state (); > + if (__glibc_unlikely (state =3D=3D NULL)) > + { > + arc4random_getentropy (buffer, len); > + return; > + } > + > + arc4random_check_stir (state, len); > + while (len > 0) > + { > + if (state->have > 0) > +=09{ > +=09 size_t m =3D MIN (len, state->have); > +=09 uint8_t *ks =3D state->buf + sizeof (state->buf) - state->have; > +=09 memcpy (buffer, ks, m); > +=09 memset (ks, 0, m); Should be explicit_bzero, I think. > +=09 buffer +=3D m; > +=09 len -=3D m; > +=09 state->have -=3D m; > +=09} > + if (state->have =3D=3D 0) > +=09arc4random_rekey (state, NULL, 0); > + } > +} > +libc_hidden_def (__arc4random_buf) > +weak_alias (__arc4random_buf, arc4random_buf) > + > +uint32_t > +__arc4random (void) > +{ > + uint32_t r; > + > + struct arc4random_state_t *state =3D arc4random_get_state (); > + if (__glibc_unlikely (state =3D=3D NULL)) > + { > + arc4random_getentropy (&r, sizeof (uint32_t)); > + return r; > + } > + > + arc4random_check_stir (state, sizeof (uint32_t)); > + if (state->have < sizeof (uint32_t)) > + arc4random_rekey (state, NULL, 0); > + uint8_t *ks =3D state->buf + sizeof (state->buf) - state->have; > + memcpy (&r, ks, sizeof (uint32_t)); > + memset (ks, 0, sizeof (uint32_t)); > + state->have -=3D sizeof (uint32_t); > + > + return r; > +} Why not simply call __arc4random_buf? If you want to retain the optimization, turn the implementation of __arc4random_buf into an inline function and call it here and from __arc4random_buf. > +libc_hidden_def (__arc4random) > +weak_alias (__arc4random, arc4random) > diff --git a/stdlib/arc4random.h b/stdlib/arc4random.h > new file mode 100644 > index 0000000000..cdca639d9d > --- /dev/null > +++ b/stdlib/arc4random.h > @@ -0,0 +1,45 @@ > +/* Internal arc4random buffer, used on each feedback step so offer some > + backtracking protection and to allow better used of vectorized > + chacha20 implementations. */ > +#define CHACHA20_BUFSIZE (8 * CHACHA20_BLOCK_SIZE) Please add a _Static_assert that this is larger than CHACHA20_KEY_SIZE + CHACHA20_IV_SIZE because the implementation assumes this. > diff --git a/stdlib/chacha20.c b/stdlib/chacha20.c > new file mode 100644 > index 0000000000..4549fc780f > --- /dev/null > +++ b/stdlib/chacha20.c > @@ -0,0 +1,187 @@ > +/* 32-bit stream position, then 96-bit nonce. */ > +#define CHACHA20_IV_SIZE=0916 > +#define CHACHA20_KEY_SIZE=0932 > + > +#define CHACHA20_STATE_LEN=0916 > + > +/* The ChaCha20 implementation is based on RFC8439 [1], omitting the fin= al > + XOR of the keystream with the plaintext because the plaintext is a > + stream of zeros. */ You can also remove the byte swapping because the key is random and discarded, so we don't care about its precise value. You can do the swapping in the ChaCha20 test instead, to get reproducible test vectors. Hmm, given that this impacts the assembler implementations as well, maybe we can do this at a later stage. > diff --git a/stdlib/stdlib.h b/stdlib/stdlib.h > index bf7cd438e1..f02a713a7b 100644 > --- a/stdlib/stdlib.h > +++ b/stdlib/stdlib.h > @@ -485,6 +485,7 @@ extern unsigned short int *seed48 (unsigned short int= __seed16v[3]) > extern void lcong48 (unsigned short int __param[7]) __THROW __nonnull ((= 1)); > =20 > # ifdef __USE_MISC > +# include > /* Data structure for communication with thread safe versions. This > type is to be regarded as opaque. It's only exported because users > have to allocate objects of this type. */ > +/* Return a random number between zero (inclusive) and the specified > + limit (exclusive). */ > +extern uint32_t arc4random_uniform (uint32_t __upper_bound) > + __THROW __wur; > # endif=09/* Use misc. */ > #endif=09/* Use misc or X/Open. */ I'm a bit worried about that inclusion. It will be a bit confusing if these types become available via in some glibc versions. Maybe use __uint32_t instead? > diff --git a/sysdeps/generic/tls-internal-struct.h b/sysdeps/generic/tls-= internal-struct.h > index d76c715a96..81a71ac54b 100644 > --- a/sysdeps/generic/tls-internal-struct.h > +++ b/sysdeps/generic/tls-internal-struct.h > @@ -19,10 +19,13 @@ > #ifndef _TLS_INTERNAL_STRUCT_H > #define _TLS_INTERNAL_STRUCT_H 1 > =20 > +#include > + You can use a forward declaration instead. Minimizing dependencies seems desirable to me. > struct tls_internal_t > { > char *strsignal_buf; > char *strerror_l_buf; > + struct arc4random_state_t *rand_state; > }; > diff --git a/sysdeps/generic/tls-internal.c b/sysdeps/generic/tls-interna= l.c > index 898c20b61c..ec0ceeebd1 100644 > --- a/sysdeps/generic/tls-internal.c > +++ b/sysdeps/generic/tls-internal.c > @@ -16,6 +16,23 @@ > License along with the GNU C Library; if not, see > . */ > =20 > +#include > #include > =20 > __thread struct tls_internal_t __tls_internal; > + > +void > +__glibc_tls_internal_free (void) > +{ > + free (__tls_internal.strsignal_buf); > + free (__tls_internal.strerror_l_buf); > + > + if (__tls_internal.rand_state !=3D NULL) > + { > + /* Clear any lingering random state prior so if the thread stack i= s > +=09 cached it won't leak any data. */ > + explicit_bzero (__tls_internal.rand_state, > +=09=09 sizeof (struct arc4random_state_t)); > + free (__tls_internal.rand_state); > + } sizeof (*__tls_internal.rand_state)? > diff --git a/sysdeps/mach/hurd/_Fork.c b/sysdeps/mach/hurd/_Fork.c > index e60b86fab1..1c44b39c5b 100644 > --- a/sysdeps/mach/hurd/_Fork.c > +++ b/sysdeps/mach/hurd/_Fork.c > @@ -665,6 +665,8 @@ retry: > /* Run things that want to run in the child task to set up. */ > RUN_HOOK (_hurd_fork_child_hook, ()); > =20 > + call_function_static_weak (__arc4random_fork_subprocess); > + Hmm, is the ordering correct? Can _hurd_fork_child_hook run user code? (This probably needs review from Hurd developers.) > diff --git a/sysdeps/unix/sysv/linux/tls-internal.c b/sysdeps/unix/sysv/l= inux/tls-internal.c > index 6e25b021ab..045176197e 100644 > --- a/sysdeps/unix/sysv/linux/tls-internal.c > +++ b/sysdeps/unix/sysv/linux/tls-internal.c > @@ -1 +1,37 @@ > +void > +__glibc_tls_internal_free (void) > +{ > + struct pthread *self =3D THREAD_SELF; > + free (self->tls_state.strsignal_buf); > + free (self->tls_state.strerror_l_buf); > + > + if (self->tls_state.rand_state !=3D NULL) > + { > + /* Clear any lingering random state prior so if the thread stack i= s > + cached it won't leak any data. */ > + explicit_bzero (self->tls_state.rand_state, > +=09=09 sizeof (struct arc4random_state_t)); sizeof (self->tls_state.rand_state)? > diff --git a/sysdeps/unix/sysv/linux/tls-internal.h b/sysdeps/unix/sysv/l= inux/tls-internal.h > index f7a1a62135..f268a2d43b 100644 > --- a/sysdeps/unix/sysv/linux/tls-internal.h > +++ b/sysdeps/unix/sysv/linux/tls-internal.h > @@ -22,17 +22,21 @@ > #include > #include > =20 > +static inline void > +__glibc_tls_internal_init (struct tls_internal_t *tls_state) > +{ > + tls_state->strsignal_buf =3D NULL; > + tls_state->strerror_l_buf =3D NULL; > + tls_state->rand_state =3D NULL; > +} See the comment about using memset earlier. Thanks, Florian