LC_TIME in calendar(1)

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

LC_TIME in calendar(1)

Jan Stary
calendar imho doesn't need to setlocale(LC_TIME, ...)
before and after strftime(3), as LC_TIME is ignored.

        Jan

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();
 }
 

Reply | Threaded
Open this post in threaded view
|

Re: LC_TIME in calendar(1)

Ted Unangst-6
Jan Stary wrote:
> calendar imho doesn't need to setlocale(LC_TIME, ...)
> before and after strftime(3), as LC_TIME is ignored.

iirc, our current practice is to delete locale related calls where the
functionality isn't implemented, so this and the other diffs are ok.

Reply | Threaded
Open this post in threaded view
|

Re: LC_TIME in calendar(1)

Ingo Schwarze
In reply to this post by Jan Stary
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();
>  }