misc/17441: sysctl -w kern.hostid can't set host ID to my IP address

[John F. Woods]

>Number: 17441
>Category: misc
>Synopsis: sysctl -w kern.hostid can't set host ID to my IP address
>Confidential: no
>Severity: non-critical
>Priority: low
>Responsible: misc-bug-people
>State: open
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Sun Jun 30 06:38:00 PDT 2002
>Closed-Date:
>Last-Modified:
>Originator: John F. Woods
>Release: NetBSD 1.6B
>Organization:
Misanthropes-R-Us
>Environment:
System: NetBSD jfwhome.funhouse.com 1.6B NetBSD 1.6B (JFW) #4: Sat Jun 29 20:39:52 EDT 2002 root@:/usr/src/sys/arch/i386/compile/JFW i386
Architecture: i386
Machine: i386
>Description:
If I recall correctly, the "hostid" value is traditionally the IP address
of a system (although I do seem to recall that for some old Sun 2 systems
I used to use, it was an IP address assigned by Sun from Sun's IP space as
a unique system id...). At any rate, I wanted to set the hostid to
correspond to 146.115.236.216; sysctl can't cope with the corresponding 32-bit
value (which has the 2^31 bit set). Sysctl uses atoi() to parse the number,
and therefore on 31-bit overflow clamps at 2^31-1. Either sysctl should use
strtoul(), or a separate CTLTYPE_UINT MIB type is needed. At any rate,
the sysctl manual page should document whatever kind of misbehavior it is
going to offer on overflow.

And while I'm here'd I'd like to complain about the following coding error
in sysctl.c (around line 335, in parse()):

snprintf(buf, BUFSIZ, "%s", string);
if ((cp = strchr(string, '=')) != NULL) {
if (!wflag)
errx(2, "Must specify -w to set variables");
*strchr(buf, '=') = '\0';

If the length of "string" is over BUFSIZE, and if the = sign is past BUFSIZ,
buf[] will neither contain the character '=' nor will it be NUL terminated,
so strchr() will either return NULL (thus dereferencing a null pointer), or
it will return a pointer to some accidental 61-valued byte on the stack,
allowing it to be smashed.

>How-To-Repeat:
# sysctl -w kern.hostid=2457070808
kern.hostid: 146 -> 2147483647
>Fix:
Several potential fixes occur to me.

The simplest is to change
intval = atoi(newval);
to
intval = (int)strtoul(newval, 0, 10);

This would allow me to set the value I want.

A slightly better change would be
char *eptr; /* at the top of parse() */
unsigned long ctmp;
...
errno = 0;
ctmp = strtoul(newval, &eptr, 10);
if (*eptr || (ctmp == ULONG_MAX && errno == ERANGE)) {
fprintf(stderr, "%s: invalid integer\n", newval);
return -1;
}
newtmp = (int)ctmp;
...

This will allow the value I want *and* issue an error message on conversion
failure, rather than mysteriously setting a different value. (The quad parsing
code around line 573 could benefit from a similar change from sscanf to
strtouq().)

The above changes assume that it would be OK to set MIB variables of type
"integer" to extremely large supposedly positive values, which might be
undesirable. A more tasteful (but much more extensive) change would be
to add to <sys/sysctl.h>
#define CTLTYPE_UINT 6 /* name describes an unsigned 32-bit number */
#define CTLTYPE_UQUAD 7 /* name describes an unsigned 64-bit number */
change /usr/src/sbin/sysctl/sysctl.c to do the right thing for these types,
and then change those variables in <sys/sysctl.h> and other places which
would benefit from this.

>Release-Note:
>Audit-Trail:
>Unformatted:
date of cvs update: 28 June 2002 around 8pm (I think)