Re: lib/58453: endptr can be unitialized if an invalid base is passed to strto*(3)

[Robert Elz <kre%munnari.OZ.AU@localhost>]

The following reply was made to PR lib/58453; it has been noted by GNATS.

From: Robert Elz <kre%munnari.OZ.AU@localhost>
To: Christos Zoulas <christos%zoulas.com@localhost>
Cc: Taylor R Campbell <riastradh%NetBSD.org@localhost>,
        "gnats-bugs%netbsd.org@localhost" <gnats-bugs%NetBSD.org@localhost>,
        Christos Zoulas <christos%astron.com@localhost>,
        "netbsd-bugs%netbsd.org@localhost" <netbsd-bugs%NetBSD.org@localhost>
Subject: Re: lib/58453: endptr can be unitialized if an invalid base is passed to strto*(3)
Date: Tue, 23 Jul 2024 08:32:32 +0700

     Date:        Mon, 22 Jul 2024 20:13:51 -0400
     From:        Christos Zoulas <christos%zoulas.com@localhost>
     Message-ID:  <2984FE66-0D2E-41ED-A70E-0755EE1D1F46%zoulas.com@localhost>
 
   | Yes, when there is an invalid base. strtoi did not handle this properly
   | before the patch.
 
 Actually, as I understand it, unless you're looking at the POSIX
 definition of strtoimax(), there was no problem there on NetBSD,
 because of how our strtoimax() behaves.
 
 The actual problem that was fixed in the change cited related to
 distinguishing between the function being cancelled and it simply
 doing nothing (no number present I think might have been the alternative),
 which I guess we could have a test for, but I certainly have no idea how
 to write, and it isn't clear we ever had an actual issue there either.
 
   | > 3. christos@'s commit lifts this assumption so that the strtoi code we
   | >   use works in terms of either NetBSD's or glibc's
   | >   strtoimax/strtoumax.
 
 Yes, though there's not a lot of reason for that, other than strict
 correctness (no objections to dealing with that of course) - NetBSD's
 strtoi() is very unlikely to ever call a glibc strtoimax().   Someone
 else's copy of our strtoi() might of course.
 
 If we wanted to be absolutely correct (not just our libc vs glibc)
 and deal with other possible bizarre implementations of this issue,
 it would need to be more complex, or coded differently.
 
   | > I asked christos@ to commit an ATF test for strtoi that exercises a
   | > path that, _under glibc's implementation_ of strtoimax/strtoumax,
   | > would use uninitialized memory.  That way, we have a chance -- e.g.,
   | > via ubsan, or just by initializing it to some garbage pointer into
   | > unmapped oblivion -- of detecting the nonportable assumption in strtoi
   | > in case we ever change our strtoimax/strtoumax implementation.
 
 Since that assumption also got fixed, I believe, I doubt that a test
 for that would be easy to create, and while this
 
   | I just added a test that checks that strtoimax returns an initialized
   | pointer
 
 is reasonable, and harmless (verifying our implementation doesn't change),
 it isn't what was requested.
 
 Further, the actual undefined (unspecified) behaviour is what *entptr
 gets set to (if anything at all) - not whether it points to valid memory.
 Simply accessing *endptr is UB according to the POSIX (and I presume C)
 definition of strtoimax() (and the other basic strtoX() functions).
 
 That is, the implementation (of the strtoimax() etc functions) is permitted
 to place absolutely anything in the *endptr passed to it, if the base is
 invalid, including a pointer that will cause a trap if referenced (not
 just dereferenced).   I'm not aware of any of our supported ports where
 such a pointer value exists though.  ubsan (about which I know < zero)
 might possibly be able to detect such an undefined use however.
 
 I thought the changes to strtoi() were such that it dealt correctly
 with this possibility (even though it never occurs on NetBSD) but on
 a closer examination I'm not sure that's true.
 
 A simpler change to strtoi() to deal with all of this UB stuff would be to
 validate the base passed to it, rather than relying upon strtoimax()
 doing that - either by duplicating the code, or by all relevant functions
 calling an internal (ie _xxx()) function to do it, so they all remain
 consistent - that could be a static inline function from some #ifdef _LIBC
 section of some (internal) header file, so that the function could be
 defined just once, but avoid the function call overhead for the
 relatively trivial test that is needed.
 
 At least now, I think, there's enough information in this PR that
 some sense can be made of it, though I'm not sure that there's anything
 much required to handle it, and I doubt the ECANCELED change is really
 important enough to bother with a pullup, the other change certainly
 isn't.   After all, Alejandro's message containing the patch did say:
 
 alx%kernel.org@localhost said:
   | I'm not sure if this patch is wanted in NetBSD.  It doesn't fix any bugs
   | there. 
 
 Fixing it in HEAD makes sense however:
 
 alx%kernel.org@localhost said:
   | for those pasting this code in other systems (which is currently done by
   | libbsd).
 
 but we don't need it pulled up for that.
 
 kre