Your proposal is junk. Not going to happen. >From owner-misc+M195331=deraadt=cvs.openbsd....@openbsd.org Sat Dec 31 >11:19:48 2022 >Delivered-To: dera...@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.manpa...@gmail.com> >X-Google-Original-From: Alejandro Colomar <a...@kernel.org> >To: misc@openbsd.org >Cc: Alejandro Colomar <a...@kernel.org>, > schwa...@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-...@kernel.org> >References: <20221231181856.13173-1-...@kernel.org> >MIME-Version: 1.0 >Content-Transfer-Encoding: 8bit >List-Help: <mailto:majord...@openbsd.org?body=help> >List-ID: <misc.openbsd.org> >List-Owner: <mailto:owner-m...@openbsd.org> >List-Post: <mailto:misc@openbsd.org> >List-Subscribe: <mailto:majord...@openbsd.org?body=sub%20misc> >List-Unsubscribe: <mailto:majord...@openbsd.org?body=unsub%20misc> >X-Loop: misc@openbsd.org >Precedence: list >Sender: owner-m...@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 <a...@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; > #endif >diff --git a/sys/dev/pci/drm/include/linux/random.h >b/sys/dev/pci/drm/include/linux/random.h >index 50ceed280c0..210c04fee21 100644 >--- a/sys/dev/pci/drm/include/linux/random.h >+++ b/sys/dev/pci/drm/include/linux/random.h >@@ -39,7 +39,7 @@ get_random_long(void) > static inline uint32_t > prandom_u32_max(uint32_t x) > { >- return arc4random_uniform(x + 1); >+ return arc4random_range(0, x); > } > > static inline void >diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c >index 31fb7d61877..b7ce754bb61 100644 >--- a/sys/kern/kern_fork.c >+++ b/sys/kern/kern_fork.c >@@ -645,7 +645,7 @@ allocpid(void) > * find an unused PID in the range [2, PID_MAX]. > */ > do { >- pid = 2 + arc4random_uniform(PID_MAX - 1); >+ pid = arc4random_range(2, PID_MAX); > } while (ispidtaken(pid)); > return pid; > } >diff --git a/sys/net/if_spppsubr.c b/sys/net/if_spppsubr.c >index 7254ee7b93d..61c622d2029 100644 >--- a/sys/net/if_spppsubr.c >+++ b/sys/net/if_spppsubr.c >@@ -3629,11 +3629,8 @@ sppp_chap_tlu(struct sppp *sp) > * Provide for an option to avoid rechallenges. > */ > if ((sp->hisauth.flags & AUTHFLAG_NORECHALLENGE) == 0) { >- /* >- * Compute the re-challenge timeout. This will yield >- * a number between 300 and 810 seconds. >- */ >- i = 300 + arc4random_uniform(1 + 810 - 300); >+ /* Compute the re-challenge timeout. */ >+ i = arc4random_uniform(300, 810); > > timeout_add_sec(&sp->ch[IDX_CHAP], i); > } >diff --git a/sys/net/pf.c b/sys/net/pf.c >index 80095c061f3..088bfad5c40 100644 >--- a/sys/net/pf.c >+++ b/sys/net/pf.c >@@ -4025,7 +4025,7 @@ retry: > PF_TEST_ATTRIB((r->tos && !(r->tos == ctx->pd->tos)), > TAILQ_NEXT(r, entries)); > PF_TEST_ATTRIB((r->prob && >- r->prob <= arc4random_uniform(UINT_MAX - 1) + 1), >+ r->prob <= arc4random_range(1, UINT_MAX - 1)), > TAILQ_NEXT(r, entries)); > PF_TEST_ATTRIB((r->match_tag && > !pf_match_tag(ctx->pd->m, r, &ctx->tag)), >diff --git a/sys/net/pf_lb.c b/sys/net/pf_lb.c >index 171242c71b6..2062e4d4c34 100644 >--- a/sys/net/pf_lb.c >+++ b/sys/net/pf_lb.c >@@ -221,7 +221,7 @@ pf_get_sport(struct pf_pdesc *pd, struct pf_rule *r, > high = tmp; > } > /* low < high */ >- cut = arc4random_uniform(1 + high - low) + low; >+ cut = arc4random_range(low, high); > /* low <= cut <= high */ > for (tmp = cut; tmp <= high && tmp <= 0xffff; ++tmp) { > key.port[sidx] = htons(tmp); >@@ -350,7 +350,7 @@ pf_rand_addr(uint32_t mask) > uint32_t addr; > > mask = ~ntohl(mask); >- addr = arc4random_uniform(mask + 1); >+ addr = arc4random_range(0, mask); > > return (htonl(addr)); > } >diff --git a/sys/netinet/ip_ipsp.c b/sys/netinet/ip_ipsp.c >index b76314b69f5..dd8b412e1df 100644 >--- a/sys/netinet/ip_ipsp.c >+++ b/sys/netinet/ip_ipsp.c >@@ -293,7 +293,7 @@ reserve_spi(u_int rdomain, u_int32_t sspi, u_int32_t tspi, > if (sspi == tspi) /* Specific SPI asked. */ > spi = tspi; > else /* Range specified */ >- spi = sspi + arc4random_uniform(tspi - sspi); >+ spi = arc4random_range(sspi, tspi - 1); > > /* Don't allocate reserved SPIs. */ > if (spi >= SPI_RESERVED_MIN && spi <= SPI_RESERVED_MAX) >diff --git a/usr.bin/nc/netcat.c b/usr.bin/nc/netcat.c >index 3ee1670053d..4b18cc2f9a6 100644 >--- a/usr.bin/nc/netcat.c >+++ b/usr.bin/nc/netcat.c >@@ -1480,7 +1480,7 @@ build_ports(char *p) > */ > if (rflag) { > for (x = 0; x <= hi - lo; x++) { >- cp = arc4random_uniform(x + 1); >+ cp = arc4random_range(0, x); > portlist[x] = portlist[cp]; > if (asprintf(&portlist[cp], "%d", x + lo) == -1) > err(1, "asprintf"); >diff --git a/usr.bin/skeyinit/skeyinit.c b/usr.bin/skeyinit/skeyinit.c >index d616d0dbdc1..2d3c80427d3 100644 >--- a/usr.bin/skeyinit/skeyinit.c >+++ b/usr.bin/skeyinit/skeyinit.c >@@ -201,7 +201,7 @@ main(int argc, char **argv) > *p++ = tolower((unsigned char)hostname[i]); > } > for (i = 0; i < 5; i++) >- *p++ = arc4random_uniform(10) + '0'; >+ *p++ = arc4random_range('0', '9'); > *p = '\0'; > > /* >diff --git a/usr.bin/ssh/auth.c b/usr.bin/ssh/auth.c >index 7d60d157068..9dbbfbd3743 100644 >--- a/usr.bin/ssh/auth.c >+++ b/usr.bin/ssh/auth.c >@@ -551,7 +551,7 @@ fakepw(void) > fake.pw_passwd = xstrdup("$2a$10$" > "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"); > for (cp = fake.pw_passwd + 7; *cp != '\0'; cp++) >- *cp = hashchars[arc4random_uniform(sizeof(hashchars) - 1)]; >+ *cp = hashchars[arc4random_range(0, sizeof(hashchars) - 1)]; > fake.pw_gecos = "NOUSER"; > fake.pw_uid = (uid_t)-1; > fake.pw_gid = (gid_t)-1; >diff --git a/usr.sbin/cron/entry.c b/usr.sbin/cron/entry.c >index ab683b8476a..bcce9a39852 100644 >--- a/usr.sbin/cron/entry.c >+++ b/usr.sbin/cron/entry.c >@@ -505,10 +505,7 @@ get_range(bitstr_t *bits, int low, int high, const char >*names[], > return (EOF); > } > >- /* get a random number in the interval [num1, num2] >- */ >- num3 = num1; >- num1 = arc4random_uniform(num2 - num3 + 1) + num3; >+ num1 = arc4random_range(num1, num2); > /* FALLTHROUGH */ > default: > /* not a range, it's a single number. >diff --git a/usr.sbin/ftp-proxy/ftp-proxy.c b/usr.sbin/ftp-proxy/ftp-proxy.c >index 9291aaba932..4e33e9b302e 100644 >--- a/usr.sbin/ftp-proxy/ftp-proxy.c >+++ b/usr.sbin/ftp-proxy/ftp-proxy.c >@@ -871,8 +871,7 @@ u_int16_t > pick_proxy_port(void) > { > /* Random should be good enough for avoiding port collisions. */ >- return (IPPORT_HIFIRSTAUTO + >- arc4random_uniform(IPPORT_HILASTAUTO - IPPORT_HIFIRSTAUTO)); >+ return arc4random_range(IPPORT_HIFIRSTAUTO, IPPORT_HILASTAUTO); > } > > void >diff --git a/usr.sbin/pppd/chap.c b/usr.sbin/pppd/chap.c >index 2e1c7e02e71..1d9aa0ecbd4 100644 >--- a/usr.sbin/pppd/chap.c >+++ b/usr.sbin/pppd/chap.c >@@ -751,10 +751,7 @@ ChapGenChallenge(cstate) > { > int chal_len; > >- /* pick a random challenge length >= MIN_CHALLENGE_LENGTH and >- <= MAX_CHALLENGE_LENGTH */ >- chal_len = MIN_CHALLENGE_LENGTH + >- arc4random_uniform(MAX_CHALLENGE_LENGTH - MIN_CHALLENGE_LENGTH + 1); >+ chal_len = arc4random_range(MIN_CHALLENGE_LENGTH, MAX_CHALLENGE_LENGTH); > > cstate->chal_len = chal_len; > cstate->chal_id = ++cstate->id; >diff --git a/usr.sbin/rad/engine.c b/usr.sbin/rad/engine.c >index ceb11d574e3..a61ea3835a6 100644 >--- a/usr.sbin/rad/engine.c >+++ b/usr.sbin/rad/engine.c >@@ -641,8 +641,7 @@ iface_timeout(int fd, short events, void *arg) > struct imsg_send_ra send_ra; > struct timeval tv; > >- 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); > tv.tv_usec = arc4random_uniform(1000000); > > log_debug("%s new timeout in %lld", __func__, tv.tv_sec); >diff --git a/usr.sbin/relayd/shuffle.c b/usr.sbin/relayd/shuffle.c >index e6173d69f24..02ce3e7b22d 100644 >--- a/usr.sbin/relayd/shuffle.c >+++ b/usr.sbin/relayd/shuffle.c >@@ -38,7 +38,7 @@ shuffle_init(struct shuffle *shuffle) > shuffle->isindex = 0; > /* Initialize using a Knuth shuffle */ > for (i = 0; i < 65536; ++i) { >- i2 = arc4random_uniform(i + 1); >+ i2 = arc4random_range(0, i); > shuffle->id_shuffle[i] = shuffle->id_shuffle[i2]; > shuffle->id_shuffle[i2] = i; > } >-- >2.39.0 > >
