Possible bug with account/password expiration

Wall, Stephen swall at redcom.com
Thu Aug 17 21:41:18 UTC 2017


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



More information about the freebsd-bugs mailing list