While trying to determine the cause of a problem a customer had with being
unable to reset their root password, I came across this piece of code in
usr.sbin/pwd_mkdb/pwd_mkdb.c:
#define SCALAR(e) store = htonl((uint32_t)(e)); \
memmove(p, &store, sizeof(store)); \
p += sizeof(store);
#define LSCALAR(e) store = HTOL((uint32_t)(e)); \
memmove(p, &store, sizeof(store)); \
p += sizeof(store);
#define HTOL(e) (openinfo.lorder == BYTE_ORDER ? \
(uint32_t)(e) : \
bswap32((uint32_t)(e)))
if (!is_comment &&
(!username || (strcmp(username, pwd.pw_name) == 0))) {
/* Create insecure data. */
p = buf;
COMPACT(pwd.pw_name);
COMPACT("*");
SCALAR(pwd.pw_uid);
SCALAR(pwd.pw_gid);
SCALAR(pwd.pw_change);
COMPACT(pwd.pw_class);
COMPACT(pwd.pw_gecos);
COMPACT(pwd.pw_dir);
COMPACT(pwd.pw_shell);
SCALAR(pwd.pw_expire);
SCALAR(pwd.pw_fields);
data.size = p - buf;
Note the cast to uint32_t in the SCALAR macro, then the use of that macro
further down with pwd.pw_change and pwd.pw_expire. These fields are declared
as time_t, which is a 64-bit value on x86. It seems to me this will incorrectly
truncate the values when passing them to htonl().
Am I missing something here?
On a side note, use of these fields is pretty inconsistent throughout the code.
They are passed around variously as time_t, intmax_t, and long. While these
do happen to all be the same size on x86 (I did not investigate other
platforms), it's not good practice and could lead to problems if these types
diverge.
Thanks
-spw
_______________________________________________
[email protected] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-bugs
To unsubscribe, send any mail to "[email protected]"
