Hi, Jan Stary wrote on Thu, Jan 10, 2019 at 08:24:58PM +0100:
> calendar imho doesn't need to setlocale(LC_TIME, ...) > before and after strftime(3), as LC_TIME is ignored. As it stands, this diff is not OK. The calendar(1) program is also calling setlocale(LC_ALL, "") from main, both in the parent and in the child. And it is calling setlocale(LC_ALL, ...) with a non-trivial argument read from the calendar file in io.c, function cal(). I believe a program should either not use setlocale(3) at all, or it should use setlocale(3) correctly, in a safe way that is portable, that is safe even when compiled on an operating system other than OpenBSD. So, as long as the program runs under setlocale(LC_ALL, "") in general, you cannot remove setting of a safe locale around strftime(3) just like that. That said, i see at least three issues: * In general, we don't want to use LC_ALL. Even if some locale(1) usage is called for here, we probably want to reduce that to the categorie(s) that actually matter. Finding out which those are requires actual understanding of the program. * In general, we don't want to run with a locale unless there is a complling reason to. It appears locale(1) functionality is at least used for the BODUN= date calculation mode for Cyrillic calendars (whatever that is), so i doubt setlocale(3) can be removed completely without loss of functionality. But maybe it would be better to call setlocale(3) only at the one place where it is actually used (unless there are others) and restore to "C" just afterwards, rather than having it set during the whole program and having to worry about security at places like the one you are just proposing to make insecure? Not sure, it depends on what the program is doing throughout. * Using setlocale(LC_TIME, "") to restore the previous locale looks fragile. Normally, to save and restore a locale, you have to call setlocale(category, NULL), copy the return value, and pass it back to setlocale(3) to restore. It may not matter too much here because the program may not modify its environment, but this is still a dangerous idiom. * The fact that part of the program runs under a locale taken from the environment and other parts run under a locale taken from the calendar file looks fishy. What are the consequences if both locales conflict? Are there bugs hiding in there? So, if someone wants to improve this program, there is some real work to do. I'm not sure i want to tackle it right now - i rarely use this program, and at this point, even though i see some ugliness, i see no strong indication that what there is now might constitute a security risk. Yours, Ingo > Index: day.c > =================================================================== > RCS file: /cvs/src/usr.bin/calendar/day.c,v > retrieving revision 1.34 > diff -u -p -r1.34 day.c > --- day.c 14 Sep 2016 15:09:46 -0000 1.34 > +++ day.c 10 Jan 2019 15:23:45 -0000 > @@ -169,11 +169,7 @@ settime(time_t *now) > if (f_SetdayAfter) > offset = 0; /* Except not when range is set explicitly */ > header[5].iov_base = dayname; > - > - (void) setlocale(LC_TIME, "C"); > header[5].iov_len = strftime(dayname, sizeof(dayname), "%A", tp); > - (void) setlocale(LC_TIME, ""); > - > setnnames(); > }
