[bug bounty] parsenum_float accepts NaN for any bounds; -t maxtime yields p=0 via UB (generalizes #423)

[tzh476]

Summary

The NaN acceptance reported in #423 for -m is not specific to that option: the range
check inside parsenum_float() lets NaN through for every bounded floating-point
PARSENUM in libcperciva. In the scrypt CLI that also affects -t maxtime, which
#423 does not cover and which fails differently — no assertion, instead p is computed
as 0 through an undefined float→unsigned conversion.

LLM disclosure, per the Tarsnap AGENTS.md convention: this report was prepared by an
LLM (Claude) working on behalf of the account operator. I am available to discuss it and
answer questions.

Filed as a separate issue only because the affected option and failure mode differ from
#423; if you would rather track it there, please close this and I will follow up in #423
instead.

Root cause (shared, library level)

libcperciva/util/parsenum.h:143-157:

val = strtod(s, &eptr);
if (eptr == s || (!trailing && (*eptr != '\0')))
        errno = EINVAL;
else if ((val < min) || (val > max))
        errno = ERANGE;
return (val);

Every relational operator with a NaN operand is false, so (val < min) || (val > max)
is false and NaN is accepted for any min/max. Reproducing this check verbatim, with
inf and an out-of-range finite value as positive controls:

input	-m bounds [0, 0.5]	-t bounds [0, INFINITY]
NaN / nan / NAN / -nan	accepted	accepted
inf	rejected, ERANGE	accepted (in range)
-inf	rejected, ERANGE	rejected, ERANGE
0.9	rejected, ERANGE	accepted (in range)
0.25	accepted	accepted

The controls rejecting correctly is what shows this is a NaN-specific hole rather than a
broken test harness.

The -t maxtime path (not covered by #423)

main.c:303: PARSENUM(&params.maxtime, optarg, 0, INFINITY).

Tracing pickparams() (lib/scryptenc/scryptenc.c:119-186) with the arithmetic
replicated verbatim, compiled -O0 — deliberately, since at -O2 a compiler may fold
the undefined conversion below into a trap, which would be an artifact of my build
rather than scrypt's behaviour:

maxtime=5.0 (default): opslimit = 2.5e6*5 = 1.25e7 -> cpu-limited branch -> logN=18 r=8 p=1
maxtime=NaN          : opslimit = nan
    if (opslimit < 32768)                  -> SKIPPED   (lower clamp lost)
    if (opslimit < (double)memlimit / 32)   -> false, takes the memory branch
    maxrp = (nan / 4) / 1048576 = nan
    if (maxrp > 0x3fffffff)                 -> SKIPPED   (upper clamp lost)
    *p = (uint32_t)(maxrp) / *r             -> (uint32_t)NaN is UB (C11 6.3.1.4); 0 here
                                           -> logN=20 r=8 p=0

So both clamps are silently bypassed and p becomes 0.

Impact, bounded honestly

crypto_scrypt_internal() (lib-platform/crypto/crypto_scrypt.c:70) does validate:

if ((r == 0) || (p == 0)) { errno = EINVAL; goto err0; }

so the run fails cleanly rather than corrupting anything. The user-visible result is a
confusing failure from a typo, plus undefined behaviour in the cast. No data loss.

I also checked the NDEBUG case for -m, since assert() disappears when packagers
define it: memavail = (size_t)(maxmemfrac * (double)memlimit_min) then yields 0, which
the later memlimit < 1 MiB floor raises to 1 MiB — i.e. silently much weaker KDF
parameters instead of the assertion abort. (My first probe of that exited with SIGILL at
-O2; that was my compiler folding the UB, not scrypt, which is why the numbers above
are all from -O0 builds.)

Suggested direction (reporting, not patching)

Rejecting non-finite values inside parsenum_float() — e.g. treating !isfinite(val)
as ERANGE — would fix -m, -t, and every future bounded float caller in one place,
and would make inf behave consistently with the existing intent.

[gperciva]

We're already working on this in libcperciva.