tzcode: Limit TZ for setugid programs

[des]

Summary

The zoneinfo parser can be told to read any file the program can access
by setting TZ to either an absolute path, or a path relative to the
zoneinfo directory. For setugid programs, we previously had a hack from
OpenBSD which rejects values of TZ deemed unsafe, but that was rather
arbitrary (anything containing a dot, for instance). Leverage openat()
with AT_RESOLVE_BENEATH instead.

For simplicity, move the TZ change detection code to after we've opened
the file, and stat the file descriptor rather than the name.

[jhb]

contrib/tzcode/localtime.c

537–538

Couldn't this now be an #else?

540

you can use strlen(TZDIR) here if that is more readable, modern compilers will compute that value at compile-time so you still end up with a constant in the machine code. You could also rewrite this as:

if (strncmp(relname, TZDIR "/", strlen(TZDIR) + 1) == 0)
    relname += strlen(TZDIR) + 1;

Or maybe even keep tzdirslash and write it as this:

if (strncmp(rename, tzdirslash, strlen(tzdirslash) == 0)
    relname += strlen(tzdirslash);

I suspect the compiler will compute the strlen()'s as a compile time constant still and that last version is more readable to me.

[imp]

contrib/tzcode/localtime.c

417

why _fstat? _close, etc? I'd note that you're changing that as well and the reason in the commit (and that's likely the only mention you'll need to make for the next person...)

[des]

contrib/tzcode/localtime.c

417

I'm not sure what you mean, the reason for replacing stat() with fstat() is already explained in the commit.

[jhb]

contrib/tzcode/localtime.c

417

I think Warner is asking about the leading underscore. We already have a local patch in this code to not use the preemptible libc symbols like open but to use _open instead. It seems stat here was just missed previously in that other local change.

[des]

contrib/tzcode/localtime.c

417

Right, I noticed that when I took over tzcode, but there is no _stat in namespace.h and it didn't occur to me to add it.

[imp]

Yes. I was thinking

"switch to _close, _open, blah, blah blah, since we have to use the internal symbols to avoid problems when the user redefines open, close etc" or something similar, but maybe a little less verbose.

[des]

In D52029#1189170, @imp wrote:

Yes. I was thinking

"switch to _close, _open, blah, blah blah, since we have to use the internal symbols to avoid problems when the user redefines open, close etc" or something similar, but maybe a little less verbose.

First of all, I'm not switching. This code has been using prefixed syscalls since 2001.

Second, the reason for using prefixed syscalls has nothing to do with “the user redefining them”, it was done because libpthread wrapped them, and it's no longer needed.

[imp]

In D52029#1189175, @des wrote:

In D52029#1189170, @imp wrote:

Yes. I was thinking

"switch to _close, _open, blah, blah blah, since we have to use the internal symbols to avoid problems when the user redefines open, close etc" or something similar, but maybe a little less verbose.

First of all, I'm not switching. This code has been using prefixed syscalls since 2001.

Second, the reason for using prefixed syscalls has nothing to do with “the user redefining them”, it was done because libpthread wrapped them, and it's no longer needed.

Ah, I got bad info a million years ago. I thought the sys_CALL versions were for that.

But you did change a couple from one to the other to make them consistent with the rest of the code. Or so I remembered, but it looks to only be stat -> _fstat now that I re-review things. So never mind.

[des]

contrib/tzcode/localtime.c

540

strlen(tzdirslash) invokes undefined behavior.

[des]

update

[jhb]

contrib/tzcode/localtime.c

505

I usually prefer #ifdef over #ifndef, but I think the trailing } else in the FreeBSD block does justify ordering the FreeBSD block second in this case.