Re: git: b6ea2513f776 - main - tzcode: Limit TZ for setugid programs

In message <202508211859.57LIxlcm003786@gitrepo.freebsd.org>, Dag-Erling 
=?utf-
8?Q?Sm=C3=B8rgrav?= writes:
> The branch main has been updated by des:
>
> URL: https://cgit.FreeBSD.org/src/commit/?id=b6ea2513f7769ea9d9e4d342777111ad
> d2c903b0
>
> commit b6ea2513f7769ea9d9e4d342777111add2c903b0
> Author:     Dag-Erling Smørgrav <des@FreeBSD.org>
> AuthorDate: 2025-08-21 16:34:32 +0000
> Commit:     Dag-Erling Smørgrav <des@FreeBSD.org>
> CommitDate: 2025-08-21 18:59:37 +0000
>
>     tzcode: Limit TZ for setugid programs
>     
>     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.
>     
>     Reviewed by:    jhb
>     Differential Revision:  https://reviews.freebsd.org/D52029
> ---
>  contrib/tzcode/localtime.c | 70 +++++++++++++++++++++++++++++---------------
> --
>  1 file changed, 45 insertions(+), 25 deletions(-)
>
> diff --git a/contrib/tzcode/localtime.c b/contrib/tzcode/localtime.c
> index 6eabe0afe570..1a01db931cab 100644
> --- a/contrib/tzcode/localtime.c
> +++ b/contrib/tzcode/localtime.c
> @@ -408,13 +408,13 @@ scrub_abbrs(struct state *sp)
>   *	     1 if the time zone has changed
>   */
>  static int
> -tzfile_changed(const char *name)
> +tzfile_changed(const char *name, int fd)
>  {
>  	static char old_name[PATH_MAX];
>  	static struct stat old_sb;
>  	struct stat sb;
>  
> -	if (stat(name, &sb) != 0)
> +	if (_fstat(fd, &sb) != 0)
>  		return -1;
>  
>  	if (strcmp(name, old_name) != 0) {
> @@ -446,8 +446,10 @@ union input_buffer {
>  	   + 4 * TZ_MAX_TIMES];
>  };
>  
> +#ifndef __FreeBSD__
>  /* TZDIR with a trailing '/' rather than a trailing '\0'.  */
>  static char const tzdirslash[sizeof TZDIR] = TZDIR "/";
> +#endif /* !__FreeBSD__ */
>  
>  /* Local storage needed for 'tzloadbody'.  */
>  union local_storage {
> @@ -460,6 +462,7 @@ union local_storage {
>      struct state st;
>    } u;
>  
> +#ifndef __FreeBSD__
>    /* The name of the file to be opened.  Ideally this would have no
>       size limits, to support arbitrarily long Zone names.
>       Limiting Zone names to 1024 bytes should suffice for practical use.
> @@ -467,6 +470,7 @@ union local_storage {
>       file_analysis as that struct is allocated anyway, as the other
>       union member.  */
>    char fullname[max(sizeof(struct file_analysis), sizeof tzdirslash + 1024)]
> ;
> +#endif /* !__FreeBSD__ */
>  };
>  
>  /* Load tz data from the file named NAME into *SP.  Read extended
> @@ -480,7 +484,11 @@ tzloadbody(char const *name, struct state *sp, bool doex
> tend,
>  	register int			fid;
>  	register int			stored;
>  	register ssize_t		nread;
> +#ifdef __FreeBSD__
> +	int serrno;
> +#else /* !__FreeBSD__ */
>  	register bool doaccess;
> +#endif /* !__FreeBSD__ */
>  	register union input_buffer *up = &lsp->u.u;
>  	register int tzheadsize = sizeof(struct tzhead);
>  
> @@ -494,6 +502,7 @@ tzloadbody(char const *name, struct state *sp, bool doext
> end,
>  
>  	if (name[0] == ':')
>  		++name;
> +#ifndef __FreeBSD__
>  #ifdef SUPPRESS_TZDIR
>  	/* Do not prepend TZDIR.  This is intended for specialized
>  	   applications only, due to its security implications.  */
> @@ -502,9 +511,7 @@ tzloadbody(char const *name, struct state *sp, bool doext
> end,
>  	doaccess = name[0] == '/';
>  #endif
>  	if (!doaccess) {
> -#ifndef __FreeBSD__
>  		char const *dot;
> -#endif /* !__FreeBSD__ */
>  		if (sizeof lsp->fullname - sizeof tzdirslash <= strlen(name))
>  		  return ENAMETOOLONG;
>  
> @@ -514,7 +521,6 @@ tzloadbody(char const *name, struct state *sp, bool doext
> end,
>  		memcpy(lsp->fullname, tzdirslash, sizeof tzdirslash);
>  		strcpy(lsp->fullname + sizeof tzdirslash, name);
>  
> -#ifndef __FreeBSD__
>  		/* Set doaccess if NAME contains a ".." file name
>  		   component, as such a name could read a file outside
>  		   the TZDIR virtual subtree.  */
> @@ -524,35 +530,49 @@ tzloadbody(char const *name, struct state *sp, bool doe
> xtend,
>  		    doaccess = true;
>  		    break;
>  		  }
> -#endif /* !__FreeBSD__ */
>  
>  		name = lsp->fullname;
>  	}
> -#ifndef __FreeBSD__
>  	if (doaccess && access(name, R_OK) != 0)
>  	  return errno;
> -#endif /* !__FreeBSD__ */
> -#ifdef DETECT_TZ_CHANGES
> -	if (doextend) {
> -		/*
> -		 * Detect if the timezone file has changed.  Check
> -		 * 'doextend' to ignore TZDEFRULES; the tzfile_changed()
> -		 * function can only keep state for a single file.
> -		 */
> -		switch (tzfile_changed(name)) {
> -		case -1:
> -			return errno;
> -		case 0:
> -			return 0;
> -		case 1:
> -			break;
> -		}
> -	}
> -#endif /* DETECT_TZ_CHANGES */
> +#else /* __FreeBSD__ */
> +        if (issetugid()) {
> +          const char *relname = name;
> +          if (strncmp(relname, TZDIR "/", strlen(TZDIR) + 1) == 0)
> +            relname += strlen(TZDIR) + 1;
> +          int dd = _open(TZDIR, O_DIRECTORY | O_RDONLY);
> +          if (dd < 0)
> +            return errno;
> +          fid = _openat(dd, relname, O_RDONLY | O_BINARY, AT_RESOLVE_BENEATH
> );
> +          serrno = errno;
> +          _close(dd);
> +          errno = serrno;
> +        } else
> +#endif
>  	fid = _open(name, O_RDONLY | O_BINARY);
>  	if (fid < 0)
>  	  return errno;
>  
> +#ifdef DETECT_TZ_CHANGES
> +	if (doextend) {
> +          /*
> +           * Detect if the timezone file has changed.  Check 'doextend' to
> +           * ignore TZDEFRULES; the tzfile_changed() function can only
> +           * keep state for a single file.
> +           */
> +          switch (tzfile_changed(name, fid)) {
> +          case -1:
> +            serrno = errno;
> +            _close(fid);
> +            return serrno;
> +          case 0:
> +            _close(fid);
> +            return 0;
> +          case 1:
> +            break;
> +          }
> +	}
> +#endif /* DETECT_TZ_CHANGES */
>  	nread = _read(fid, up->buf, sizeof up->buf);
>  	if (nread < tzheadsize) {
>  	  int err = nread < 0 ? errno : EINVAL;
>

Further testing has shown that this commit has TZ America/Vancouver to UTC.

Setting TZ to PST8PDT still works but TZ=America/Vancouver does not.


-- 
Cheers,
Cy Schubert <Cy.Schubert@cschubert.com>
FreeBSD UNIX:  <cy@FreeBSD.org>   Web:  https://FreeBSD.org
NTP:           <cy@nwtime.org>    Web:  https://nwtime.org

			e**(i*pi)+1=0