From: Florian Obser Subject: Re: localtime(3) / gmtime(3) error checking for bin / libexec / sbin To: "Todd C. Miller" Cc: tech Date: Sun, 28 Apr 2024 17:12:59 +0200 On 2024-04-28 08:48 -06, "Todd C. Miller" wrote: > 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. fixed > >> 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? I like it > >> } 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" : ""); > this as well, while there is precedent for "soon", I think it's better to go with minutes. I've also spotted that this one actually had a check for localtime failing (it would just be silent). So I've removed the outer check and added the minutes output. >> close(logfd); >> } >> } updated diff: diff --git bin/date/date.c bin/date/date.c index 0330ffc5f57..43553bf3de1 100644 --- bin/date/date.c +++ bin/date/date.c @@ -151,6 +151,8 @@ setthetime(char *p, const char *pformat) err(1, "pledge"); lt = localtime(&tval); + if (lt == NULL) + errx(1, "conversion error"); lt->tm_isdst = -1; /* correct for DST */ diff --git bin/ksh/lex.c bin/ksh/lex.c index 41178694fa6..51128a31bb1 100644 --- bin/ksh/lex.c +++ bin/ksh/lex.c @@ -1251,7 +1251,11 @@ dopprompt(const char *sp, int ntruncate, const char **spp, int doprint) case 'd': /* '\' 'd' Dow Mon DD */ time(&t); tm = localtime(&t); - strftime(strbuf, sizeof strbuf, "%a %b %d", tm); + if (tm) + strftime(strbuf, sizeof strbuf, + "%a %b %d", tm); + else + strbuf[0] = '\0'; break; case 'D': /* '\' 'D' '{' strftime format '}' */ p = strchr(cp + 2, '}'); @@ -1266,7 +1270,11 @@ dopprompt(const char *sp, int ntruncate, const char **spp, int doprint) *p = '\0'; time(&t); tm = localtime(&t); - strftime(strbuf, sizeof strbuf, tmpbuf, tm); + if (tm) + strftime(strbuf, sizeof strbuf, tmpbuf, + tm); + else + strbuf[0] = '\0'; cp = strchr(cp + 2, '}'); break; case 'e': /* '\' 'e' escape */ @@ -1315,22 +1323,38 @@ dopprompt(const char *sp, int ntruncate, const char **spp, int doprint) case 't': /* '\' 't' 24 hour HH:MM:SS */ time(&t); tm = localtime(&t); - strftime(strbuf, sizeof strbuf, "%T", tm); + if (tm) + strftime(strbuf, sizeof strbuf, "%T", + tm); + else + strbuf[0] = '\0'; break; case 'T': /* '\' 'T' 12 hour HH:MM:SS */ time(&t); tm = localtime(&t); - strftime(strbuf, sizeof strbuf, "%l:%M:%S", tm); + if (tm) + strftime(strbuf, sizeof strbuf, + "%l:%M:%S", tm); + else + strbuf[0] = '\0'; break; case '@': /* '\' '@' 12 hour am/pm format */ time(&t); tm = localtime(&t); - strftime(strbuf, sizeof strbuf, "%r", tm); + if (tm) + strftime(strbuf, sizeof strbuf, "%r", + tm); + else + strbuf[0] = '\0'; break; case 'A': /* '\' 'A' 24 hour HH:MM */ time(&t); tm = localtime(&t); - strftime(strbuf, sizeof strbuf, "%R", tm); + if (tm) + strftime(strbuf, sizeof strbuf, "%R", + tm); + else + strbuf[0] = '\0'; break; case 'u': /* '\' 'u' username */ strlcpy(strbuf, username, sizeof strbuf); diff --git bin/pax/sel_subs.c bin/pax/sel_subs.c index bd0138990b3..f825f2d719b 100644 --- bin/pax/sel_subs.c +++ bin/pax/sel_subs.c @@ -572,7 +572,8 @@ str_sec(const char *p, time_t *tval) return(-1); } - lt = localtime(tval); + if ((lt = localtime(tval)) == NULL) + return (-1); if (dot != NULL) { /* .SS */ if (strlen(++dot) != 2) diff --git bin/ps/print.c bin/ps/print.c index 7c7f75d74b8..a87a9f3ae67 100644 --- bin/ps/print.c +++ bin/ps/print.c @@ -524,6 +524,10 @@ started(const struct pinfo *pi, VARENT *ve) startt = kp->p_ustart_sec; tp = localtime(&startt); + if (tp == NULL) { + (void)printf("%-*s", v->width, "-"); + return; + } if (!now) (void)time(&now); if (now - kp->p_ustart_sec < 12 * SECSPERHOUR) { diff --git libexec/ftpd/ftpcmd.y libexec/ftpd/ftpcmd.y index e85d7ccb72f..4222dd2e253 100644 --- libexec/ftpd/ftpcmd.y +++ libexec/ftpd/ftpcmd.y @@ -613,6 +613,11 @@ cmd } else { struct tm *t; t = gmtime(&stbuf.st_mtime); + if (t == NULL) { + /* invalid time, use epoch */ + stbuf.st_mtime = 0; + t = gmtime(&stbuf.st_mtime); + } reply(213, "%04d%02d%02d%02d%02d%02d", 1900 + t->tm_year, diff --git libexec/getty/main.c libexec/getty/main.c index 3ac0939d8c8..0a77d14ad04 100644 --- libexec/getty/main.c +++ libexec/getty/main.c @@ -562,10 +562,12 @@ putf(char *cp) break; case 'd': { - (void)time(&t); - (void)strftime(db, sizeof(db), - "%l:%M%p on %A, %d %B %Y", localtime(&t)); - xputs(db); + struct tm *tm; + time(&t); + if ((tm = localtime(&t)) != NULL) + if (strftime(db, sizeof(db), + "%l:%M%p on %A, %d %B %Y", tm) != 0) + xputs(db); break; } diff --git libexec/snmpd/snmpd_metrics/mib.c libexec/snmpd/snmpd_metrics/mib.c index a3b80ddf9ac..87116770a16 100644 --- libexec/snmpd/snmpd_metrics/mib.c +++ libexec/snmpd/snmpd_metrics/mib.c @@ -296,27 +296,29 @@ mib_hrsystemdate(struct agentx_varbind *vb) int tzoffset; unsigned short year; + memset(s, 0, sizeof(s)); (void)time(&now); ptm = localtime(&now); - year = htons(ptm->tm_year + 1900); - memcpy(s, &year, 2); - s[2] = ptm->tm_mon + 1; - s[3] = ptm->tm_mday; - s[4] = ptm->tm_hour; - s[5] = ptm->tm_min; - s[6] = ptm->tm_sec; - s[7] = 0; - - tzoffset = ptm->tm_gmtoff; - if (tzoffset < 0) - s[8] = '-'; - else - s[8] = '+'; - - s[9] = abs(tzoffset) / 3600; - s[10] = (abs(tzoffset) - (s[9] * 3600)) / 60; + if (ptm != NULL) { + year = htons(ptm->tm_year + 1900); + memcpy(s, &year, 2); + s[2] = ptm->tm_mon + 1; + s[3] = ptm->tm_mday; + s[4] = ptm->tm_hour; + s[5] = ptm->tm_min; + s[6] = ptm->tm_sec; + s[7] = 0; + + tzoffset = ptm->tm_gmtoff; + if (tzoffset < 0) + s[8] = '-'; + else + s[8] = '+'; + s[9] = abs(tzoffset) / 3600; + s[10] = (abs(tzoffset) - (s[9] * 3600)) / 60; + } agentx_varbind_nstring(vb, s, sizeof(s)); } diff --git libexec/talkd/announce.c libexec/talkd/announce.c index 00bdd0b8c96..66ad22157cb 100644 --- libexec/talkd/announce.c +++ libexec/talkd/announce.c @@ -102,9 +102,14 @@ print_mesg(FILE *tf, CTL_MSG *request, char *remote_machine) sizes[i] = strlen(line_buf[i]); max_size = max(max_size, sizes[i]); i++; - (void)snprintf(line_buf[i], N_CHARS, - "Message from Talk_Daemon@%s at %d:%02d ...", - hostname, localclock->tm_hour , localclock->tm_min ); + if (localclock) { + (void)snprintf(line_buf[i], N_CHARS, + "Message from Talk_Daemon@%s at %d:%02d ...", + hostname, localclock->tm_hour , localclock->tm_min ); + } else { + (void)snprintf(line_buf[i], N_CHARS, + "Message from Talk_Daemon@%s ...", hostname); + } sizes[i] = strlen(line_buf[i]); max_size = max(max_size, sizes[i]); i++; diff --git sbin/dhclient/dhclient.c sbin/dhclient/dhclient.c index 97fc9b5b2fe..dc65bdf18e0 100644 --- sbin/dhclient/dhclient.c +++ sbin/dhclient/dhclient.c @@ -2079,6 +2079,7 @@ lease_as_string(char *type, struct client_lease *lease) static char string[8192]; char timebuf[27]; /* 6 2017/04/08 05:47:50 UTC; */ struct option_data *opt; + struct tm *tm; char *buf, *name; time_t t; size_t rslt; @@ -2138,19 +2139,25 @@ lease_as_string(char *type, struct client_lease *lease) free(buf); t = lease->epoch + lease_renewal(lease); - rslt = strftime(timebuf, sizeof(timebuf), DB_TIMEFMT, gmtime(&t)); + if ((tm = gmtime(&t)) == NULL) + return NULL; + rslt = strftime(timebuf, sizeof(timebuf), DB_TIMEFMT, tm); if (rslt == 0) return NULL; append_statement(string, sizeof(string), " renew ", timebuf); t = lease->epoch + lease_rebind(lease); - rslt = strftime(timebuf, sizeof(timebuf), DB_TIMEFMT, gmtime(&t)); + if ((tm = gmtime(&t)) == NULL) + return NULL; + rslt = strftime(timebuf, sizeof(timebuf), DB_TIMEFMT, tm); if (rslt == 0) return NULL; append_statement(string, sizeof(string), " rebind ", timebuf); t = lease->epoch + lease_expiry(lease); - rslt = strftime(timebuf, sizeof(timebuf), DB_TIMEFMT, gmtime(&t)); + if ((tm = gmtime(&t)) == NULL) + return NULL; + rslt = strftime(timebuf, sizeof(timebuf), DB_TIMEFMT, tm); if (rslt == 0) return NULL; append_statement(string, sizeof(string), " expire ", timebuf); diff --git sbin/isakmpd/log.c sbin/isakmpd/log.c index 5a0df1df5e9..aeab7b085c4 100644 --- sbin/isakmpd/log.c +++ sbin/isakmpd/log.c @@ -182,7 +182,11 @@ _log_print(int error, int syslog_level, const char *fmt, va_list ap, if (log_output) { gettimeofday(&now, 0); t = now.tv_sec; - tm = localtime(&t); + if ((tm = localtime(&t)) == NULL) { + /* Invalid time, use the epoch. */ + t = 0; + tm = localtime(&t); + } if (class >= 0) snprintf(nbuf, sizeof nbuf, "%02d%02d%02d.%06ld %s %02d ", diff --git sbin/isakmpd/policy.c sbin/isakmpd/policy.c index 9dbf339c0db..1df46dca0fc 100644 --- sbin/isakmpd/policy.c +++ sbin/isakmpd/policy.c @@ -1728,13 +1728,23 @@ policy_callback(char *name) return phase_1; if (strcmp(name, "GMTTimeOfDay") == 0) { + struct tm *tm; tt = time(NULL); - strftime(mytimeofday, 14, "%Y%m%d%H%M%S", gmtime(&tt)); + if ((tm = gmtime(&tt)) == NULL) { + log_error("policy_callback: invalid time %lld", tt); + goto bad; + } + strftime(mytimeofday, 14, "%Y%m%d%H%M%S", tm); return mytimeofday; } if (strcmp(name, "LocalTimeOfDay") == 0) { + struct tm *tm; tt = time(NULL); - strftime(mytimeofday, 14, "%Y%m%d%H%M%S", localtime(&tt)); + if ((tm = localtime(&tt)) == NULL) { + log_error("policy_callback: invalid time %lld", tt); + goto bad; + } + strftime(mytimeofday, 14, "%Y%m%d%H%M%S", tm); return mytimeofday; } if (strcmp(name, "initiator") == 0) diff --git sbin/isakmpd/x509.c sbin/isakmpd/x509.c index 989553897e2..ffeff089967 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,8 +319,15 @@ 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); - strftime(after, 14, "%Y%m%d%H%M%S", localtime(&tt)); + 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", ltm); timecomp2 = "LocalTimeOfDay"; } else { if (tm->data[tm->length - 1] == 'Z') { diff --git sbin/newfs_msdos/newfs_msdos.c sbin/newfs_msdos/newfs_msdos.c index dae0a704daa..f170a70399f 100644 --- sbin/newfs_msdos/newfs_msdos.c +++ sbin/newfs_msdos/newfs_msdos.c @@ -550,7 +550,9 @@ main(int argc, char *argv[]) if (!opt_N) { gettimeofday(&tv, NULL); now = tv.tv_sec; - tm = localtime(&now); + if ((tm = localtime(&now)) == NULL) + errx(1, "Invalid time"); + if (!(img = malloc(bpb.bps))) err(1, NULL); dir = bpb.res + (bpb.spf ? bpb.spf : bpb.bspf) * bpb.nft; diff --git sbin/route/route.c sbin/route/route.c index 755e548e9cc..f4e45584234 100644 --- sbin/route/route.c +++ sbin/route/route.c @@ -1878,7 +1878,10 @@ bfd_calc_uptime(time_t time) fmt = "%Ss"; tp = localtime(&time); - (void)strftime(buf, sizeof(buf), fmt, tp); + if (tp) + (void)strftime(buf, sizeof(buf), fmt, tp); + else + buf[0] = '\0'; return (buf); } diff --git sbin/shutdown/shutdown.c sbin/shutdown/shutdown.c index 2c74ae3d25a..618edd20776 100644 --- sbin/shutdown/shutdown.c +++ sbin/shutdown/shutdown.c @@ -345,8 +345,13 @@ 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 in %lld minute%s\n\n", + (long long)(timeleft / 60), + (timeleft > 60) ? "s" : ""); } 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 +530,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; @@ -595,10 +603,17 @@ nolog(time_t timeleft) (void)signal(SIGTERM, finish); shuttime = time(NULL) + timeleft; tm = localtime(&shuttime); - if (tm && (logfd = open(_PATH_NOLOGIN, O_WRONLY|O_CREAT|O_TRUNC, + if ((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 in %lld minute%s\n\n", + (long long)(timeleft / 60), + (timeleft > 60) ? "s" : ""); close(logfd); } } diff --git sbin/unwind/libunbound/util/log.c sbin/unwind/libunbound/util/log.c index a15ee920c0f..86a3acee43c 100644 --- sbin/unwind/libunbound/util/log.c +++ sbin/unwind/libunbound/util/log.c @@ -271,8 +271,8 @@ log_vmsg(int pri, const char* type, } now = (time_t)time(NULL); #if defined(HAVE_STRFTIME) && defined(HAVE_LOCALTIME_R) - if(log_time_asc && strftime(tmbuf, sizeof(tmbuf), "%b %d %H:%M:%S", - localtime_r(&now, &tm))%(sizeof(tmbuf)) != 0) { + if(log_time_asc && localtime_r(&now, &tm) && strftime(tmbuf, + sizeof(tmbuf), "%b %d %H:%M:%S", &tm)%(sizeof(tmbuf)) != 0) { /* %sizeof buf!=0 because old strftime returned max on error */ fprintf(logfile, "%s %s[%d:%x] %s: %s\n", tmbuf, ident, (int)getpid(), tid?*tid:0, type, message); -- In my defence, I have been left unsupervised.