OpenSSH 2.9p1: lastlog_get_entry() should be more careful

[Patrick J. LoPresti]

In OpenSSH 2.9p1, the function loginrec.c:lastlog_get_entry() looks
like this:

int
lastlog_get_entry(struct logininfo *li)
{
	struct lastlog last;
	int fd;

	if (lastlog_openseek(li, &fd, O_RDONLY)) {
		if (atomicio(read, fd, &last, sizeof(last)) != sizeof(last)) {
			log("lastlog_get_entry: Error reading from %s: %s",
			    LASTLOG_FILE, strerror(errno));
			return 0;
		} else {
			lastlog_populate_entry(li, &last);
			return 1;
		}
	} else {
		return 0;
	}
}


On Linux (at least version 2.2.19), the call to lastlog_openseek() can
succeed even when the /var/log/lastlog file is short (e.g., 0 bytes).
Subsequent calls to read() on the file descriptor return zero (EOF).
Consequently, the call to atomicio in the function above returns zero,
causing OpenSSH to log a bogus error message by examining a bogus
errno value.

I suggest that the call to atomicio() be checked to see if it returns
zero, with no error message generated for this case.  That is, I
suggest the function be rewritten something like this:

int
lastlog_get_entry(struct logininfo *li)
{
	struct lastlog last;
	int fd;

	if (lastlog_openseek(li, &fd, O_RDONLY)) {
		int ret = atomicio(read, fd, &last, sizeof(last));

		if (ret == sizeof(last)) {
			lastlog_populate_entry(li, &last);
			return 1;
                } else if (ret == 0) {
			return 0;
		} else {
			log("lastlog_get_entry: Error reading from %s: %s",
			    LASTLOG_FILE, strerror(errno));
			return 0;
		}
	} else {
		return 0;
	}
}


The above is just for illustration; it is still not quite correct.
Strictly speaking, you should not examine errno unless atomicio()
returns a negative value, because a short read will not set errno
either.

 - Pat