From: Todd C. Miller <millert@openbsd.org>
Subject: Re: localtime(3) / gmtime(3) error checking for bin / libexec / sbin
To: Florian Obser <florian@openbsd.org>
Cc: tech <tech@openbsd.org>
Date: Sun, 28 Apr 2024 08:48:45 -0600

On Sun, 28 Apr 2024 12:03:04 +0200, Florian Obser wrote:

> Mostly compile tested only. Most of these probably can't happen. At
> least I don't expect the kernel to work well if the time is so far in
> the future that the year can't be represented by a struct tm. But I
> suppose just adding the error check is easier than arguing why it's not
> needed.

This looks good to me.  I think it is worth adding the checks even
if failure should be impossible.  Some comments inline.

 - todd

> diff --git sbin/isakmpd/x509.c sbin/isakmpd/x509.c
> index 989553897e2..9a83d5d743e 100644
> --- sbin/isakmpd/x509.c
> +++ sbin/isakmpd/x509.c
> @@ -222,8 +222,15 @@ x509_generate_kn(int id, X509 *cert)
>  	if (((tm = X509_get_notBefore(cert)) == NULL) ||
>  	    (tm->type != V_ASN1_UTCTIME &&
>  		tm->type != V_ASN1_GENERALIZEDTIME)) {
> -		tt = time(0);
> -		strftime(before, 14, "%Y%m%d%H%M%S", localtime(&tt));
> +		struct tm *ltm;
> +
> +		tt = time(NULL);
> +		if ((ltm = localtime(&tt)) == NULL) {
> +			LOG_DBG((LOG_POLICY, 30,
> +			    "x509_generate_kn: invalid local time"));
> +			goto fail;
> +		}
> +		strftime(before, 14, "%Y%m%d%H%M%S", ltm);
>  		timecomp = "LocalTimeOfDay";
>  	} else {
>  		if (tm->data[tm->length - 1] == 'Z') {
> @@ -312,7 +319,14 @@ x509_generate_kn(int id, X509 *cert)
>  	if (tm == NULL ||
>  	    (tm->type != V_ASN1_UTCTIME &&
>  		tm->type != V_ASN1_GENERALIZEDTIME)) {
> +		struct tm *ltm;
> +
>  		tt = time(0);
> +		if ((ltm = localtime(&tt)) == NULL) {
> +			LOG_DBG((LOG_POLICY, 30,
> +			    "x509_generate_kn: invalid local time"));
> +			goto fail;
> +		}
>  		strftime(after, 14, "%Y%m%d%H%M%S", localtime(&tt));

In the strftime() call this should now be ltm.

>  		timecomp2 = "LocalTimeOfDay";
>  	} else {
> diff --git sbin/shutdown/shutdown.c sbin/shutdown/shutdown.c
> index 2c74ae3d25a..84f26575b26 100644
> --- sbin/shutdown/shutdown.c
> +++ sbin/shutdown/shutdown.c
> @@ -345,8 +345,11 @@ timewarn(time_t timeleft)
>  	if (timeleft > 10 * 60) {
>  		shuttime = time(NULL) + timeleft;
>  		lt = localtime(&shuttime);
> -		strftime(when, sizeof(when), "%H:%M %Z", lt);
> -		dprintf(fd[1], "System going down at %s\n\n", when);
> +		if (lt != NULL) {
> +			strftime(when, sizeof(when), "%H:%M %Z", lt);
> +			dprintf(fd[1], "System going down at %s\n\n", when);
> +		} else
> +			dprintf(fd[1], "System going down soon\n\n");

Maybe just use the timeleft > 59 case if localtime() fails?

>  	} else if (timeleft > 59) {
>  		dprintf(fd[1], "System going down in %lld minute%s\n\n",
>  		    (long long)(timeleft / 60), (timeleft > 60) ? "s" : "");
> @@ -525,6 +528,9 @@ getoffset(char *timearg)
>  	time(&now);
>  	lt = localtime(&now);				/* current time val */
>  
> +	if (lt == NULL)
> +		badtime();
> +
>  	switch (strlen(timearg)) {
>  	case 10:
>  		this_year = lt->tm_year;
> @@ -597,8 +603,13 @@ nolog(time_t timeleft)
>  	tm = localtime(&shuttime);
>  	if (tm && (logfd = open(_PATH_NOLOGIN, O_WRONLY|O_CREAT|O_TRUNC,
>  	    0664)) >= 0) {
> -		strftime(when, sizeof(when), "at %H:%M %Z", tm);
> -		dprintf(logfd, "\n\nNO LOGINS: System going down %s\n\n", when)
> ;
> +		if (tm) {
> +			strftime(when, sizeof(when), "at %H:%M %Z", tm);
> +			dprintf(logfd,
> +			    "\n\nNO LOGINS: System going down %s\n\n", when);
> +		} else
> +			dprintf(logfd,
> +			    "\n\nNO LOGINS: System going down soon\n\n");

How about this instead?
	dprintf(logfd,
	    "\n\nNO LOGINS: System going down in %lld minute%s\n\n",
	    (long long)(timeleft / 60), (timeleft > 60) ? "s" : "");

>  		close(logfd);
>  	}
>  }