Improving tzcode localtime.c security a la FreeBSD
The first difference between FreeBSD and tzcode localtime.c that I'd like to address is that FreeBSD is more cautious about handling outlandish TZ settings like "../foo". It seems to me that tzcode should largely mimic FreeBSD's extra caution. In the long run I'd like tzcode localtime.c to be byte-for-byte identical to FreeBSD localtime.c but it's better to do this one step at a time and this security caution is a good first step. I noticed some minor issues with the FreeBSD implementation of this extra checking, so I propose implementing it in a somewhat different way in tzcode. I'm attaching proposed patches to do that, and have installed them in the tzcode development repository on GitHub. Here are issues I noticed: * With an environment variable setting like TZ="/usr/share/zoneinfo/America/Los_Angeles", in a privileged program FreeBSD first opens the directory /usr/share/zoneinfo to get a directory file descriptor, and then uses fstatat and openat, both with AT_RESOLVE_BENEATH. However, it passes the full name to fstatat which must be a typo; I can't see how that would work reliably for a setuid program, even though the neighboring code suggests it was intended to work. I assume it was intended to pass a relative name to fstatat. * As near as I can make out, in privileged programs FreeBSD rejects adjacent slashes in settings like TZ="/usr/share/zoneinfo//America/Los_Angeles", where there are two slashes after "zoneinfo". It would be nicer to treat those two slashes as if they were one, as the kernel does. * tzcode is intended to be portable to platforms lacking openat and AT_RESOLVE_BENEATH, so for now the attached patches mimic FreeBSD's extra checking without using these two features. There are efficiency motivations for using the two features if available, but I'd like to defer that to a later patchset. * Although FreeBSD takes some steps to treat TZ settings the same regardless of whether they come from the TZ environment variable, it doesn't take this idea to its logical conclusion. It seems to me that it's better to not care where the TZ setting came from, as that's easier to document, understand, and maintain, so the attached patches do that. * FreeBSD uses issetugid, which is not universally available. The attached patches work around this by supplying an issetugid substitute for platforms like GNU/Linux that lack it. The first attached patch is a minor indenting fixup. The second one is a minor efficiency improvement, to use strnlen instead of strlen when that improves asymptotic efficiency. The third patch adopts FreeBSD's idea of treating a TZ setting starting with "/usr/share/zoneinfo/" as if it lacked that prefix. And the fourth and main patch changes the tzcode security checks as described above. These tzcode patches do not address other differences between tzcode and FreeBSD, such as the multithreading differences or the check-for-changes differences. These can wait for later patchsets. Comments and suggestions are welcome as usual.
Paul Eggert <eggert@cs.ucla.edu> writes:
* With an environment variable setting like TZ="/usr/share/zoneinfo/America/Los_Angeles", in a privileged program
This is... imprecise. We perform additional checks if the process is setugid because it sits at a privilege boundary: it has elevated privileges but its environment is controlled by an unprivileged user.
FreeBSD first opens the directory /usr/share/zoneinfo to get a directory file descriptor, and then uses fstatat and openat, both with AT_RESOLVE_BENEATH. However, it passes the full name to fstatat which must be a typo; I can't see how that would work reliably for a setuid program, even though the neighboring code suggests it was intended to work. I assume it was intended to pass a relative name to fstatat.
Yes, that's a typo in the code and a hole in our test suite, thank you.
* As near as I can make out, in privileged programs FreeBSD rejects adjacent slashes in settings like TZ="/usr/share/zoneinfo//America/Los_Angeles", where there are two slashes after "zoneinfo". It would be nicer to treat those two slashes as if they were one, as the kernel does.
True. This is an oversight. We strip TZDIR "/" but don't strip additional slashes.
* tzcode is intended to be portable to platforms lacking openat and AT_RESOLVE_BENEATH, so for now the attached patches mimic FreeBSD's extra checking without using these two features. There are efficiency motivations for using the two features if available, but I'd like to defer that to a later patchset.
This is a matter of security, not efficieny. You simply cannot get the same security guarantees without AT_RESOLVE_BENEATH. You can achieve the same result, but your code will be vulnerable to TOCTTOU races.
* Although FreeBSD takes some steps to treat TZ settings the same regardless of whether they come from the TZ environment variable, it doesn't take this idea to its logical conclusion. It seems to me that it's better to not care where the TZ setting came from, as that's easier to document, understand, and maintain, so the attached patches do that.
That's just wrong. As a setugid program, I can't trust TZ because it was provided by an unprivileged and untrusted user, so I must defend against attempts to break out from TZDIR. I should however be able to trust TZDEFAULT, even if it points to something outside TZDIR, because TZDEFAULT is controlled by the administrator, not by an untrusted user. DES -- Dag-Erling Smørgrav - des@des.no
On 2025-09-26 10:27, Dag-Erling Smørgrav wrote:
This is a matter of security, not efficieny. You simply cannot get the same security guarantees without AT_RESOLVE_BENEATH. You can achieve the same result, but your code will be vulnerable to TOCTTOU races.
Yes and no. First, I'm planning a future patch to use AT_RESOLVE_BENEATH if available, and this won't be an issue on FreeBSD once that happens. Second, even platforms lacking AT_RESOLVE_BENEATH get equivalent security guarantees, because we can trust the contents of /usr/share/zoneinfo (if we can't trust that directory, we have bigger problems than those fixable by AT_RESOLVE_BENEATH!) and tzcode's localtime checks for ".." in relative pathnames (I'll arrange for this check to be optimized away on platforms with AT_RESOLVE_BENEATH).
* Although FreeBSD takes some steps to treat TZ settings the same regardless of whether they come from the TZ environment variable, it doesn't take this idea to its logical conclusion. It seems to me that it's better to not care where the TZ setting came from, as that's easier to document, understand, and maintain, so the attached patches do that.
That's just wrong. As a setugid program, I can't trust TZ because it was provided by an unprivileged and untrusted user, so I must defend against attempts to break out from TZDIR. I should however be able to trust TZDEFAULT, even if it points to something outside TZDIR, because TZDEFAULT is controlled by the administrator, not by an untrusted user.
Ah, I should have mentioned that TZDEFAULT (default "/etc/localtime") is a special case: its is trusted regardless of whether it came from the TZ environment variable or from tzalloc. This is true in both current tzcode localtime.c (which has tzalloc) and in FreeBSD localtime.c (which doesn't).
On 2025-09-26 11:55, Paul Eggert via tz wrote:
On 2025-09-26 10:27, Dag-Erling Smørgrav wrote: Second, even platforms lacking AT_RESOLVE_BENEATH get equivalent security guarantees, because we can trust the contents of /usr/share/zoneinfo (if we can't trust that directory, we have bigger problems than those fixable by AT_RESOLVE_BENEATH!) and tzcode's localtime checks for ".." in relative pathnames (I'll arrange for this check to be optimized away on platforms with AT_RESOLVE_BENEATH).
* Although FreeBSD takes some steps to treat TZ settings the same regardless of whether they come from the TZ environment variable, it doesn't take this idea to its logical conclusion. It seems to me that it's better to not care where the TZ setting came from, as that's easier to document, understand, and maintain, so the attached patches do that.
That's just wrong. As a setugid program, I can't trust TZ because it was provided by an unprivileged and untrusted user, so I must defend against attempts to break out from TZDIR. I should however be able to trust TZDEFAULT, even if it points to something outside TZDIR, because TZDEFAULT is controlled by the administrator, not by an untrusted user.
Ah, I should have mentioned that TZDEFAULT (default "/etc/localtime") is a special case: its is trusted regardless of whether it came from the TZ environment variable or from tzalloc. If you do not allow data file paths outside TZDIR, how do we test zones in the packaged build or staging directories, or custom or patched zones in our dev directories?
Is it not better to apply the same untrusting attitude about TZ to all external data, regardless of what you believe about the source, which may have been compromised? This seems to be how exploits are elevated from messing with relatively obscure files which are misconfigured, accidentally or maliciously! I never understood why effectively constant data files are installed with user write privileges, where read only might slow attackers a smidge, and may have to be used on systems which do not really support POSIX permissions; more especially if that is made a checked security condition! -- Take care. Thanks, Brian Inglis Calgary, Alberta, Canada La perfection est atteinte Perfection is achieved non pas lorsqu'il n'y a plus rien à ajouter not when there is no more to add mais lorsqu'il n'y a plus rien à retrancher but when there is no more to cut -- Antoine de Saint-Exupéry
On 2025-09-26 12:28, Brian Inglis via tz wrote:
On 2025-09-26 11:55, Paul Eggert via tz wrote:
If you do not allow data file paths outside TZDIR, how do we test zones in the packaged build or staging directories, or custom or patched zones in our dev directories?
We do that by not using setuid/setgid programs to test out-of-TZDIR data. The behavior hasn't changed for ordinary programs. What's changed is that tzcode is now more cautious when in a setuid/setgid program. Caution does seems warranted for these programs, and it's not like we're inventing the caution (FreeBSD is similarly cautious).
Is it not better to apply the same untrusting attitude about TZ to all external data
If we did that, we couldn't use ordinary programs to test non-TZDIR data files, right?
I never understood why effectively constant data files are installed with user write privileges You mean like this file on Fedora 42?
$ ls -l /usr/share/zoneinfo/America/Los_Angeles -rw-r--r--. 3 root root 2852 Mar 26 2025 /usr/share/zoneinfo/America/Los_Angeles There's would be no security benefit to making the file "-r--r--r--" instead of "-rw-r--r--", as root can write to readonly files.
On 2025-09-26 10:55, Paul Eggert wrote:
I'm planning a future patch to use AT_RESOLVE_BENEATH if available, and this won't be an issue on FreeBSD once that happens.
I did this by installing the attached patches. Comments welcome. In doing it I noticed one tiny glitch in the FreeBSD implementation: it opens /usr/share/zoneinfo with O_RDONLY. Surely that should be O_SEARCH. This makes no practical difference since the directory is always both readable and searchable, but I thought I'd mention it.
Second, even platforms lacking AT_RESOLVE_BENEATH get equivalent security guarantees, because we can trust the contents of /usr/share/ zoneinfo (if we can't trust that directory, we have bigger problems than those fixable by AT_RESOLVE_BENEATH!) and tzcode's localtime checks for ".." in relative pathnames (I'll arrange for this check to be optimized away on platforms with AT_RESOLVE_BENEATH).
On thinking about it more, I still don't see how bleeding-edge FreeBSD's recently-added openat+open approach is a win for localtime.c. That approach slows things down (because we now have two extra syscalls: openat and close); it introduces new failure modes (e.g., EMFILE); and it provides no extra security because it does not fix any TOCTTOU races in practice. Although the attached patches implement the approach anyway (as an option), I'm hoping that upon further reflection the FreeBSD maintainers agree that it's unnecessary; if so, we can remove the option from tzcode. If I'm not mistaken, the two remaining features in FreeBSD but not tzcode are support for intermittent polling for TZif file changes, and support for poorly written multithreaded programs that use localtime/gmtime/offtime instead of localtime_r/gmtime_r/offtime_r. I plan to look at these two features in that order.
On 2025-09-29 04:23, Dag-Erling Smørgrav wrote:
We never asked you to add it.
That's OK, you don't need to use it: you can continue to maintain a separate version of the code, a version that has exactly the same behavior as what's in tzcode. Though I still hope for an explanation that gives a realistic scenario showing why the openat + O_RESOLVE_BENEATH approach, which is less efficient, provides extra security in practice. I would like to put such an explanation into tzcode as a comment, as it's not obvious.
Paul Eggert via tz <tz@iana.org> writes:
Dag-Erling Smørgrav <des@FreeBSD.org> writes:
We never asked you to add it. That's OK, you don't need to use it: you can continue to maintain a separate version of the code, a version that has exactly the same behavior as what's in tzcode.
I'm starting to doubt your good faith here, Paul.
Though I still hope for an explanation that gives a realistic scenario showing why the openat + O_RESOLVE_BENEATH approach, which is less efficient, provides extra security in practice. I would like to put such an explanation into tzcode as a comment, as it's not obvious.
The goal is to allow TZ to point to anything inside TZDIR, either absolutely or relatively, and nothing else, in the setugid case; there's also a check that handles the weird-but-not-unheard-of case where TZ points to TZDEFAULT (I've encountered downstream code that sets TZ to ":/etc/localtime"; I don't know why, and I'm afraid to ask, but it needs to work). We previously had something lifted from OpenBSD which would simply reject anything that contained a dot. This seemed overly restrictive, so I wrote what we currently have instead. It does add a couple of syscalls, but I can't think of an alternative that doesn't add even more. DES -- Dag-Erling Smørgrav - des@des.no
On 2025-09-29 09:05, Dag-Erling Smørgrav wrote:
The goal is to allow TZ to point to anything inside TZDIR, either absolutely or relatively, and nothing else, in the setugid case; there's also a check that handles the weird-but-not-unheard-of case where TZ points to TZDEFAULT (I've encountered downstream code that sets TZ to ":/etc/localtime"; I don't know why, and I'm afraid to ask, but it needs to work). We previously had something lifted from OpenBSD which would simply reject anything that contained a dot. This seemed overly restrictive, so I wrote what we currently have instead.
The old OpenBSD approach of rejecting anything containing a dot was inspired by a tzcode change contributed by Arthur David Olson in 1986[1]. In 2018 I noticed the same issue you did - that rejecting anything containing '.' was too restrictive - and changed tzcode to reject only pathnames containing a ".." file name component[2]; this was later picked up by NetBSD. The OpenBSD maintainers noticed the same issue in 2022, and changed their implementation to reject all strings containing "../"[3]. The tzcode and OpenBSD approaches both work well enough in practice; the tzcode approach is slightly more generous, as it allows bizarre settings like TZ="Hello../World", but that's not a significant difference. The avoid-".." approach has been used for quite some time and I know of no security problem with it. I keep asking about this because if there is a practical security problem with the approach, it's important for OpenBSD, NetBSD, tzcode etc. to know it so that we can fix our code.
It does add a couple of syscalls, but I can't think of an alternative that doesn't add even more. The alternative used in current tzcode[4] (assuming OPENAT_TZDIR is 0) handles all the cases mentioned in the above-quoted remarks (setugid, TZ=":/etc/localtime", TZ="/etc/localtime", TZ="/usr/share/zoneinfo/America/Los_Angeles", etc.), and it does so without using openat + AT_RESOLVE_BENEATH. Again, if there's a problem with it I'd like to know.
[1]: https://github.com/eggert/tz/commit/2c592c94b83514b80aabeca638ea7afde5f69436... [2]: https://github.com/eggert/tz/commit/6c9af485e113f60d32e47ff64ef22423ccbdc496 [3]: https://github.com/openbsd/src/commit/a88d58b8033c36ffc70b16c55fd0624a20c4f0... [4]: https://github.com/eggert/tz/blob/997441bed2d4832e0ee5f9effc037986b725059c/l...
I'm starting to doubt your good faith here, Paul.
I don't see why, as the questions are genuine and are based on some research. It sounds like there's been some miscommunication on my end. If so, I apologize.
On 2025-09-26 02:47, Paul Eggert via tz wrote:
The third patch adopts FreeBSD's idea of treating a TZ setting starting with "/usr/share/zoneinfo/" as if it lacked that prefix.
Hopefully that is implemented as checking for the configured TZDIR rather than the literal absolute path? -- Take care. Thanks, Brian Inglis Calgary, Alberta, Canada La perfection est atteinte Perfection is achieved non pas lorsqu'il n'y a plus rien à ajouter not when there is no more to add mais lorsqu'il n'y a plus rien à retrancher but when there is no more to cut -- Antoine de Saint-Exupéry
Yesterday’s changes inadvertently removed some support for the rarely-used SUPPRESS_TZDIR option. * localtime.c (SUPPRESS_TZDIR): Default to 0. (tzloadbody, tzset_unlocked) [SUPPRESS_TZDIR]: Suppress TZDIR in a few recently-added places. Prefer if to #if if either will do. --- localtime.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/localtime.c b/localtime.c index 32013247..67500312 100644 --- a/localtime.c +++ b/localtime.c @@ -209,6 +209,13 @@ static char const *utc = etc_utc + sizeof "Etc/" - 1; # define TZDEFRULESTRING ",M3.2.0,M11.1.0" #endif +/* If compiled with -DSUPPRESS_TZDIR, do not prepend TZDIR to relative TZ. + This is intended for specialized applications only, due to its + security implications. */ +#ifndef SUPPRESS_TZDIR +# define SUPPRESS_TZDIR 0 +#endif + /* Limit to time zone abbreviation length in proleptic TZ strings. This is distinct from TZ_MAX_CHARS, which limits TZif file contents. It defaults to 254, not 255, so that desigidx_type can be an unsigned char. @@ -581,7 +588,8 @@ tzloadbody(char const *name, struct state *sp, char tzloadflags, /* If the program is privileged, NAME is TZDEFAULT or subsidiary to TZDIR. Also, NAME is not a device. */ if (name[0] == '/' && strcmp(name, TZDEFAULT) != 0) { - if (strncmp(relname, tzdirslash, sizeof tzdirslash) == 0) + if (!SUPPRESS_TZDIR + && strncmp(relname, tzdirslash, sizeof tzdirslash) == 0) for (relname += sizeof tzdirslash; *relname == '/'; relname++) continue; else if (issetugid()) @@ -615,11 +623,7 @@ tzloadbody(char const *name, struct state *sp, char tzloadflags, } } -#ifdef SUPPRESS_TZDIR - /* Do not prepend TZDIR. This is intended for specialized - applications only, due to its security implications. */ -#else - if (name[0] != '/') { + if (!SUPPRESS_TZDIR && name[0] != '/') { if (sizeof lsp->fullname - sizeof tzdirslash <= strnlen(name, sizeof lsp->fullname - sizeof tzdirslash)) return ENAMETOOLONG; @@ -631,7 +635,6 @@ tzloadbody(char const *name, struct state *sp, char tzloadflags, strcpy(lsp->fullname + sizeof tzdirslash, name); name = lsp->fullname; } -#endif fid = open(name, (O_RDONLY | O_BINARY | O_CLOEXEC | O_CLOFORK | O_IGNORE_CTTY | O_NOCTTY)); @@ -1529,7 +1532,7 @@ tzset_unlocked(void) /* Abbreviate a string like "/usr/share/zoneinfo/America/Los_Angeles" to its shorter equivalent "America/Los_Angeles". */ - if (sizeof tzdirslash < namelen + if (!SUPPRESS_TZDIR && sizeof tzdirslash < namelen && memcmp(name, tzdirslash, sizeof tzdirslash) == 0) { char const *p = name + sizeof tzdirslash; while (*p == '/') -- 2.48.1
participants (3)
-
Brian Inglis -
Dag-Erling Smørgrav -
Paul Eggert