Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initialize tzcode early #1084

Merged
merged 1 commit into from
Nov 20, 2023
Merged

Conversation

dag-erling
Copy link
Contributor

An explicit tzset() call is usually not needed as it happens implicitly the first time we call localtime() or mktime(), but in some cases (sandboxing, chroot) this may be too late.

@fxlb
Copy link
Member

fxlb commented Sep 15, 2023

If there's a problem, can you give an example of how to reproduce it?

@dag-erling
Copy link
Contributor Author

  1. Build tcpdump with either Capsicum enabled or WITH_CHROOT set to an empty directory.
  2. Set your timezone to something other than UTC, either by creating /etc/localtime or by setting the TZ environment variable.
  3. Run tcpdump and observe that all timestamps are in UTC instead of your local timezone, because either the tzdb is not available in the chroot or Capsicum blocks you from reading it.

@dag-erling
Copy link
Contributor Author

dag-erling commented Sep 15, 2023

I didn't say “define WITH_CHROOT to an empty string”, I said set it to an empty directory.

(Strictly speaking, the directory doesn't have to be empty to demonstrate the problem; it just has to not contain /etc/localtime or a copy of the timezone referenced by TZ. But an empty directory, e.g. /var/empty, is simpler.)

@fxlb
Copy link
Member

fxlb commented Sep 15, 2023

I said set it to an empty directory.

Ok, thanks.

@fxlb
Copy link
Member

fxlb commented Sep 16, 2023

I suggest adding a comment, as in your commit body, with this patch:

diff --git a/tcpdump.c b/tcpdump.c
index b7eaad14d..19cdd1ab4 100644
--- a/tcpdump.c
+++ b/tcpdump.c
@@ -1550,6 +1550,11 @@ main(int argc, char **argv)
 	if (abort_on_misalignment(ebuf, sizeof(ebuf)) < 0)
 		error("%s", ebuf);
 
+	/*
+	 * An explicit tzset() call is usually not needed as it happens
+	 * implicitly the first time we call localtime() or mktime(),
+	 * but in some cases (sandboxing, chroot) this may be too late.
+	 */
 	tzset();
 
 	while (

@fxlb
Copy link
Member

fxlb commented Sep 16, 2023

It seems that in the chroot case, TZ setting is required (test on Debian/bookworm).
(Even with a valid /etc/localtime)

@guyharris
Copy link
Member

I suggest adding a comment, as in your commit body, with this patch:

+1 - if it wasn't obvious to whoever put in the chroot code and the Capsicum code, there might be others to whom it wasn't obvious.

(In a better world, there would be ways in which a program running with no elevated privileges could capture traffic via, for example, libpcap running/opening a UNIX-domain socket to/etc. a program running with sufficient privileges to open {a BPF device, a PF_SOCKET socket, etc.} and be handed back a suitably-protected FD if, by some mechanism, the process making the request is deemed worthy of that privilege.

That requires, of course, a way to determine what renders the process worthy. Process credentials? Some extended attribute that can only be set by somebody granted some privilege? Signed code? Signed code plus some form of extended attribute granting that privilege? That's sort of how Npcap works if you install it in "admin permissions required" mode; it runs a helper program, with a UAC prompt involved in the process of that helper program ending up with admin permissions, and the helper program uses the Windows "hand this process a HANDLE" API.

Wireshark's use of a separate process to do the capturing gets you some of this, at the expense of the arm's-length relationship between the Wireshark/TShark process and dumpcap making some things awkward.)

An explicit tzset() call is usually not needed as it happens implicitly
the first time we call localtime() or mktime(), but in some cases
(sandboxing, chroot) this may be too late.
@fxlb
Copy link
Member

fxlb commented Sep 16, 2023

It seems that in the chroot case, TZ setting is required (test on Debian/bookworm).
(Even with a valid /etc/localtime)

There is an answer here:
https://bugzilla.redhat.com/show_bug.cgi?id=217317#c2

Is it the same with Capsicum?

@fxlb
Copy link
Member

fxlb commented Sep 16, 2023

The comment should perhaps be updated with:
(And also the body of the commit message)

diff --git a/tcpdump.c b/tcpdump.c
index 19cdd1ab4..fef702991 100644
--- a/tcpdump.c
+++ b/tcpdump.c
@@ -1554,6 +1554,8 @@ main(int argc, char **argv)
 	 * An explicit tzset() call is usually not needed as it happens
 	 * implicitly the first time we call localtime() or mktime(),
 	 * but in some cases (sandboxing, chroot) this may be too late.
+	 * In these cases, you need to set the TZ environment variable
+	 * even if you have a valid /etc/localtime.
 	 */
 	tzset();
 

@dag-erling
Copy link
Contributor Author

No, because it's not true.

@fxlb
Copy link
Member

fxlb commented Sep 16, 2023

No, because it's not true.

Could you explain?
Without TZ setting, I cannot make it work on Debian/bookworm or Ubuntu, even with a valid /etc/localtime)

@dag-erling
Copy link
Contributor Author

If that is true (which I doubt but will not be able to investigate closer until next week) then that's a bug in Debian that needs to be fixed there, rather than enshrined here.

@guyharris
Copy link
Member

Without TZ setting, I cannot make it work on Debian/bookworm or Ubuntu, even with a valid /etc/localtime)

So do you mean that, if you call tzset() before doing chroot(), and there's a valid /etc/localtime in the non-chroot environment, tcpdump gets the time zone wrong if you don't set TZ?

If so, is there also a valid /etc/localtime in the chrooted environment?

@fxlb
Copy link
Member

fxlb commented Sep 16, 2023

Without TZ setting, I cannot make it work on Debian/bookworm or Ubuntu, even with a valid /etc/localtime)

So do you mean that, if you call tzset() before doing chroot(), and there's a valid /etc/localtime in the non-chroot environment, tcpdump gets the time zone wrong if you don't set TZ?

Yes.

If so, is there also a valid /etc/localtime in the chrooted environment?

No.

@guyharris
Copy link
Member

If so, is there also a valid /etc/localtime in the chrooted environment?

No.

I suspect that's the problem. As per the bug comment you cited, POSIX specifies that some functions, such as localtime() "behave as if they use tzset internally", so, if localtime() is called after the chroot(), and if it checks /etc/localtime and falls back on UTC if that fails, it'll fall back on UTC if there's no /etc/localtime in the chroot. As that bug comment says:

So, when using the chroot, preferrably link or bind mount /etc/localtime into the chroot (e.g. system-config-date and /usr/sbin/tzdata-update copy /etc/localtime over to /var/spool/postfix/etc/localtime if it existed previously and had the same content as /etc/localtime), or tzset () before chrooting and make sure you don't use any functions where documentation says it behaves as if tzset () was called.

so you need the /etc/localtime link in the chrooted environment. A hard link to the appropriate file should exist even if the paths to the tz database files aren't accessible from the chrooted environment; a symbolic link would require that the files be present in the chrooted environment.

@fxlb
Copy link
Member

fxlb commented Sep 17, 2023

so you need the /etc/localtime link in the chrooted environment.

Having some problems to do a hard link ("Operation not permitted") perhaps because /etc/localtime is a symbolic link (I will do more tests), I just copy /etc/localtime and the relevant /usr/share/zoneinfo/X/Y in the chroot directory. It's works but also shows that adding the tzset() call is useless. This call seems useful only when using TZ.

@dag-erling
Copy link
Contributor Author

Adding the tzset() call is not useless. It fixes the Capsicum case (no chroot, but the tzdb cannot be read after entering capabilities mode), it fixes the TZ chroot case on glibc systems, and it probably fixes both the TZ and non-TZ chroot cases on non-glibc systems. I'll look into the glibc issue separately.

@fxlb
Copy link
Member

fxlb commented Sep 19, 2023

The comment should perhaps be updated with:
(And also the body of the commit message)

diff --git a/tcpdump.c b/tcpdump.c
index 19cdd1ab4..b39c25682 100644
--- a/tcpdump.c
+++ b/tcpdump.c
@@ -1554,6 +1554,10 @@ main(int argc, char **argv)
 	 * An explicit tzset() call is usually not needed as it happens
 	 * implicitly the first time we call localtime() or mktime(),
 	 * but in some cases (sandboxing, chroot) this may be too late.
+	 * In the chroot case, at least with a glibc build, if
+	 * /etc/localtime (and eventually the file to which it is linked)
+	 * don't exist in the chroot directory to give a valid timezone data,
+	 * you need to set the TZ environment variable.
 	 */
 	tzset();
 

@dag-erling
Copy link
Contributor Author

Is that really the place for this information? It is neither specific to tcpdump nor universally applicable to the platforms it supports, and may very well be overcome by events in the near future.

@fxlb
Copy link
Member

fxlb commented Oct 2, 2023

Is that really the place for this information?

We need to document what has been fixed in the current state (and in the commit message), like:

diff --git a/tcpdump.c b/tcpdump.c
index 19cdd1ab4..1d7f60272 100644
--- a/tcpdump.c
+++ b/tcpdump.c
@@ -1554,6 +1554,8 @@ main(int argc, char **argv)
 	 * An explicit tzset() call is usually not needed as it happens
 	 * implicitly the first time we call localtime() or mktime(),
 	 * but in some cases (sandboxing, chroot) this may be too late.
+	 * This fixes the Capsicum case and the TZ chroot case on glibc
+	 * systems.
 	 */
 	tzset();
 

@fxlb fxlb merged commit 54d5fbe into the-tcpdump-group:master Nov 20, 2023
4 checks passed
@fxlb
Copy link
Member

fxlb commented Nov 20, 2023

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants