Saturday, December 31, 2022

Re: [RFC v1 2/2] Use arc4random_range() instead of arc4random_uniform() when appropriate

Your proposal is junk. Not going to happen.

>From owner-misc+M195331=deraadt=cvs.openbsd.org@openbsd.org Sat Dec 31 11:19:48 2022
>Delivered-To: deraadt@cvs.openbsd.org
>DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; s=selector1; bh=/JVUSEqVR3
> /k8gFGm9V8QDDc/a7fMpZ1djd/RE+G3ho=; h=list-unsubscribe:list-subscribe:
> list-post:list-owner:list-id:list-help:references:in-reply-to:date:
> subject:cc:to:from; d=openbsd.org; b=9QbHsEP2Cs5GzwJFaRcKYtGgmTEdJNstk
> WG0moKjzSa7hfEBKsJsJdfiisq/xmQ6cDJ22kKl4rA0btSsjHthLFkJhvak9A132n8Pyv0
> wHmF/q53tJzAvwHHCPGNFmCMXdKVzN/k2IQUyrA/USls/x58VaRfuLYO2ooxbL3pPr9sSj
> Vi8PjayN3X1XksgfEJZj38LMOv8l2knvBBYrubQmLM21+bY1ilB6kq7yPUs/BneF7lG0Gt
> lgAGRJitijIAy8hZzgbgZEfs49Bx1Q5hT0Bw7T7OH6EkWtQ5ez59TiUzRWGXRkQSRAaTPy
> JyU3jjcTVd/NV3BmkdVTjMBONmpuQ==
>X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
> d=1e100.net; s=20210112;
> h=content-transfer-encoding:mime-version:references:in-reply-to
> :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc
> :subject:date:message-id:reply-to;
> bh=2UYUeZysQqtf/KOzlblpSVrwo2fzTbMjuWtz4tikOe0=;
> b=URJZ9YXpK7xe7laZ33ubdezrA7RmZ3aI+F8YaDpyZ8BuETxr7k+J6TzjjXbjvzj+vm
> kxI3RFU0zRcggCh/Xt6lIRYxViBYS2SuA7JB1VsDDAOrOwpRSq5s3ryoTggMtlFj4zD/
> HpWkBBML0zkmoLQHYmdk8jSx2gKUGpDdJaIA8uUD6mIkmZlxKEFMRagUyisXR8O5RH6W
> P23r5w9dlLu7sPfHCHDkwR76xUTp97+yVgHsKzcAc9OlLsKr0+aacNq8uuXQhztxNY3W
> JgQc5MhOOahuEI97xN8xI3L0e6BKFZmNS5vvUCLXEiFdt7jbx2JbZ9czZSeZwa87p6Wn
> Ru1g==
>X-Gm-Message-State: AFqh2krJps2B1CZ8kRCvS5Dm15CqO5h8/P3AzqvbHRPKj9lPOh8DWcFW
> ypZJNU0vfcYp+NudScdH0MILST5jvj8=
>X-Google-Smtp-Source: AMrXdXuh8Ked2AQA4K81PjC0dktKBv52w3jykbg/aF2xwJVEjOAuMNvGippMa6MPtJ+V948C79+izw==
>X-Received: by 2002:a05:600c:500a:b0:3d3:5b56:b834 with SMTP id n10-20020a05600c500a00b003d35b56b834mr25309242wmr.5.1672510739398;
> Sat, 31 Dec 2022 10:18:59 -0800 (PST)
>From: Alejandro Colomar <alx.manpages@gmail.com>
>X-Google-Original-From: Alejandro Colomar <alx@kernel.org>
>To: misc@openbsd.org
>Cc: Alejandro Colomar <alx@kernel.org>,
> schwarze@openbsd.org
>Subject: [RFC v1 2/2] Use arc4random_range() instead of arc4random_uniform() when appropriate
>Date: Sat, 31 Dec 2022 19:18:56 +0100
>X-Mailer: git-send-email 2.39.0
>In-Reply-To: <20221231181856.13173-1-alx@kernel.org>
>References: <20221231181856.13173-1-alx@kernel.org>
>MIME-Version: 1.0
>Content-Transfer-Encoding: 8bit
>List-Help: <mailto:majordomo@openbsd.org?body=help>
>List-ID: <misc.openbsd.org>
>List-Owner: <mailto:owner-misc@openbsd.org>
>List-Post: <mailto:misc@openbsd.org>
>List-Subscribe: <mailto:majordomo@openbsd.org?body=sub%20misc>
>List-Unsubscribe: <mailto:majordomo@openbsd.org?body=unsub%20misc>
>X-Loop: misc@openbsd.org
>Precedence: list
>Sender: owner-misc@openbsd.org
>
>This makes the code much more readable and self-documented. While doing
>this, I noticed a few bugs, and other cases which may be bugs or not.
>Switching to this specialized API makes it easier to spot such bugs, but
>since I'm not familiar with the code, I kept some bugs unfixed. The
>most obvious ones (although I may be wrong) I fixed them. And in some
>cases where it was very unclear, I didn't touch the old *_uniform() code.
>
>Below are the cases where I changed the behavior (I considered it a bug):
>
>* usr.bin/ssh/auth.c:
>
> - *cp = hashchars[arc4random_uniform(sizeof(hashchars) - 1)];
> + *cp = hashchars[arc4random_range(0, sizeof(hashchars) - 1)];
>
>* usr.sbin/ftp-proxy/ftp-proxy.c:
>
> - return (IPPORT_HIFIRSTAUTO +
> - arc4random_uniform(IPPORT_HILASTAUTO - IPPORT_HIFIRSTAUTO));
> + return arc4random_range(IPPORT_HIFIRSTAUTO, IPPORT_HILASTAUTO);
>
>* usr.sbin/rad/engine.c:
>
> - tv.tv_sec = MIN_RTR_ADV_INTERVAL +
> - arc4random_uniform(MAX_RTR_ADV_INTERVAL - MIN_RTR_ADV_INTERVAL);
> + tv.tv_sec = arc4random_range(MIN_RTR_ADV_INTERVAL, MAX_RTR_ADV_INTERVAL);
>
>In the following change, I didn't use the temporary variable 'num3'.
>AFAICS, this doesn't affect other uses of the variable in other places,
>because they set it before use. But please check carefully; I may have
>missed something:
>
>* usr.sbin/cron/entry.c:
>
> - /* get a random number in the interval [num1, num2]
> - */
> - num3 = num1;
> - num1 = arc4random_uniform(num2 - num3 + 1) + num3;
> + num1 = arc4random_range(num1, num2);
>
>Signed-off-by: Alejandro Colomar <alx@kernel.org>
>---
> games/boggle/boggle/bog.c | 2 +-
> games/canfield/canfield/canfield.c | 2 +-
> games/mille/init.c | 2 +-
> gnu/gcc/gcc/cfgexpand.c | 2 +-
> lib/libevent/select.c | 2 +-
> regress/lib/libc/malloc/malloc_general/malloc_general.c | 2 +-
> regress/sys/sys/tree/rb/rb-test.c | 3 +--
> regress/sys/sys/tree/splay/splay-test.c | 3 +--
> sbin/iked/ikev2.c | 2 +-
> sys/dev/pci/drm/drm_linux.c | 2 +-
> sys/dev/pci/drm/include/linux/random.h | 2 +-
> sys/kern/kern_fork.c | 2 +-
> sys/net/if_spppsubr.c | 7 ++-----
> sys/net/pf.c | 2 +-
> sys/net/pf_lb.c | 4 ++--
> sys/netinet/ip_ipsp.c | 2 +-
> usr.bin/nc/netcat.c | 2 +-
> usr.bin/skeyinit/skeyinit.c | 2 +-
> usr.bin/ssh/auth.c | 2 +-
> usr.sbin/cron/entry.c | 5 +----
> usr.sbin/ftp-proxy/ftp-proxy.c | 3 +--
> usr.sbin/pppd/chap.c | 5 +----
> usr.sbin/rad/engine.c | 3 +--
> usr.sbin/relayd/shuffle.c | 2 +-
> 24 files changed, 26 insertions(+), 39 deletions(-)
>
>diff --git a/games/boggle/boggle/bog.c b/games/boggle/boggle/bog.c
>index c0e19454a27..3ed4888fc43 100644
>--- a/games/boggle/boggle/bog.c
>+++ b/games/boggle/boggle/bog.c
>@@ -607,7 +607,7 @@ newgame(char *b)
> /* Shuffle the cubes using Fisher-Yates (aka Knuth P). */
> p = ncubes;
> while (--p) {
>- q = (int)arc4random_uniform(p + 1);
>+ q = (int)arc4random_range(0, p);
> tmp = cubes[p];
> cubes[p] = cubes[q];
> cubes[q] = tmp;
>diff --git a/games/canfield/canfield/canfield.c b/games/canfield/canfield/canfield.c
>index 346fd20a1d2..dec75f6531f 100644
>--- a/games/canfield/canfield/canfield.c
>+++ b/games/canfield/canfield/canfield.c
>@@ -531,7 +531,7 @@ shuffle(struct cardtype *deck[])
> deck[i]->paid = FALSE;
> }
> for (i = decksize - 1; i > 0; i--) {
>- j = arc4random_uniform(i + 1);
>+ j = arc4random_range(0, i);
> if (i != j) {
> temp = deck[i];
> deck[i] = deck[j];
>diff --git a/games/mille/init.c b/games/mille/init.c
>index a86157739dd..c0cc6ac1f02 100644
>--- a/games/mille/init.c
>+++ b/games/mille/init.c
>@@ -90,7 +90,7 @@ shuffle(void)
> CARD temp;
>
> for (i = DECK_SZ - 1; i > 0; i--) {
>- r = arc4random_uniform(i + 1);
>+ r = arc4random_range(0, i);
> temp = Deck[r];
> Deck[r] = Deck[i];
> Deck[i] = temp;
>diff --git a/gnu/gcc/gcc/cfgexpand.c b/gnu/gcc/gcc/cfgexpand.c
>index 17aff165f6d..0cb8a21289b 100644
>--- a/gnu/gcc/gcc/cfgexpand.c
>+++ b/gnu/gcc/gcc/cfgexpand.c
>@@ -438,7 +438,7 @@ partition_stack_vars (void)
> for (si = n - 1; si > 0; si--)
> {
> size_t tmp;
>- sj = arc4random_uniform(si + 1);
>+ sj = arc4random_range(0, si);
>
> tmp = stack_vars_sorted[si];
> stack_vars_sorted[si] = stack_vars_sorted[sj];
>diff --git a/lib/libevent/select.c b/lib/libevent/select.c
>index fb38b850155..8d5ff6ff34f 100644
>--- a/lib/libevent/select.c
>+++ b/lib/libevent/select.c
>@@ -155,7 +155,7 @@ select_dispatch(struct event_base *base, void *arg, struct timeval *tv)
> event_debug(("%s: select reports %d", __func__, res));
>
> check_selectop(sop);
>- i = arc4random_uniform(sop->event_fds + 1);
>+ i = arc4random_range(0, sop->event_fds);
> for (j = 0; j <= sop->event_fds; ++j) {
> struct event *r_ev = NULL, *w_ev = NULL;
> if (++i >= sop->event_fds+1)
>diff --git a/regress/lib/libc/malloc/malloc_general/malloc_general.c b/regress/lib/libc/malloc/malloc_general/malloc_general.c
>index b243787bcf7..3980ed5e277 100644
>--- a/regress/lib/libc/malloc/malloc_general/malloc_general.c
>+++ b/regress/lib/libc/malloc/malloc_general/malloc_general.c
>@@ -27,7 +27,7 @@
> size_t
> size(void)
> {
>- int p = arc4random_uniform(17) + 3;
>+ int p = arc4random_range(3, 19);
> return arc4random_uniform(1 << p);
> }
>
>diff --git a/regress/sys/sys/tree/rb/rb-test.c b/regress/sys/sys/tree/rb/rb-test.c
>index 409cc22393a..5bf54013d95 100644
>--- a/regress/sys/sys/tree/rb/rb-test.c
>+++ b/regress/sys/sys/tree/rb/rb-test.c
>@@ -67,8 +67,7 @@ main(int argc, char **argv)
> tmp = malloc(sizeof(struct node));
> if (tmp == NULL) err(1, "malloc");
> do {
>- tmp->key = arc4random_uniform(MAX-MIN);
>- tmp->key += MIN;
>+ tmp->key = arc4random_uniform(MIN, MAX - 1);
> } while (RB_FIND(tree, &root, tmp) != NULL);
> if (i == 0)
> max = min = tmp->key;
>diff --git a/regress/sys/sys/tree/splay/splay-test.c b/regress/sys/sys/tree/splay/splay-test.c
>index 56084a0c71e..69c9e87fa6c 100644
>--- a/regress/sys/sys/tree/splay/splay-test.c
>+++ b/regress/sys/sys/tree/splay/splay-test.c
>@@ -67,8 +67,7 @@ main(int argc, char **argv)
> tmp = malloc(sizeof(struct node));
> if (tmp == NULL) err(1, "malloc");
> do {
>- tmp->key = arc4random_uniform(MAX-MIN);
>- tmp->key += MIN;
>+ tmp->key = arc4random_range(MIN, MAX - 1);
> } while (SPLAY_FIND(tree, &root, tmp) != NULL);
> if (i == 0)
> max = min = tmp->key;
>diff --git a/sbin/iked/ikev2.c b/sbin/iked/ikev2.c
>index 9579b8a2ea5..3b4566bb3ae 100644
>--- a/sbin/iked/ikev2.c
>+++ b/sbin/iked/ikev2.c
>@@ -7186,7 +7186,7 @@ ikev2_cp_setaddr_pool(struct iked *env, struct iked_sa *sa,
> if (upper < lower)
> upper = lower;
> /* Randomly select start from [lower, upper-1] */
>- start = arc4random_uniform(upper - lower) + lower;
>+ start = arc4random_range(lower, upper - 1);
>
> for (host = start;;) {
> log_debug("%s: mask %x start %x lower %x host %x upper %x",
>diff --git a/sys/dev/pci/drm/drm_linux.c b/sys/dev/pci/drm/drm_linux.c
>index 2b975cda9fb..4e5aa29c228 100644
>--- a/sys/dev/pci/drm/drm_linux.c
>+++ b/sys/dev/pci/drm/drm_linux.c
>@@ -776,7 +776,7 @@ idr_alloc(struct idr *idr, void *ptr, int start, int end, gfp_t gfp_mask)
> end = INT_MAX;
>
> #ifdef notyet
>- id->id = begin = start + arc4random_uniform(end - start);
>+ id->id = begin = arc4random_range(start, end - 1);
> #else
> id->id = begin = start;
>

No comments:

Post a Comment