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

-G drops packets if not enough traffic is recorded #1170

Closed
Trolldemorted opened this issue Apr 6, 2024 · 7 comments
Closed

-G drops packets if not enough traffic is recorded #1170

Trolldemorted opened this issue Apr 6, 2024 · 7 comments

Comments

@Trolldemorted
Copy link

Trolldemorted commented Apr 6, 2024

We are invoking tcpdump as follows:

tcpdump -i router -G 30 -w '%Y_%m_%d-%H_%M_%S.pcap' -s 0 -z touch src net 10.0.0.0/16 and dst net 10.0.0.0/16

After causing some traffic, we get an (unfinished) pcap file as we'd expect:

root@router1:/services/BambiArkime# ls -alh /pcaps
total 8.0K
drwxr-xr-x  2 root    root    4.0K Apr  6 12:53 .
drwxr-xr-x 20 root    root    4.0K Apr  6 11:16 ..
-rw-r--r--  1 tcpdump tcpdump    0 Apr  6 12:53 2024_04_06-12_53_30.pcap

If we now wait some time (probably >30s, we waited about a minute in our tests) before causing some more traffic, then the previous file is finished without the packets that tcpdump has received, and a new (unfinished) pcap file is created:

root@router1:/services/BambiArkime# ls -alh /pcaps
total 12K
drwxr-xr-x  2 root    root    4.0K Apr  6 12:55 .
drwxr-xr-x 20 root    root    4.0K Apr  6 11:16 ..
-rw-r--r--  1 tcpdump tcpdump   24 Apr  6 12:55 2024_04_06-12_53_30.pcap
-rw-r--r--  1 tcpdump tcpdump    0 Apr  6 12:55 2024_04_06-12_55_12.pcap

As you can see, 2024_04_06-12_53_30.pcap is only 24 bytes long, which should be the pcap file header size without any packets.

The problem perpetuates, as long as our traffic is spurious we never see any packets in any of our files:

root@router1:/services/BambiArkime# ls -alh /pcaps
total 16K
drwxr-xr-x  2 root    root    4.0K Apr  6 12:57 .
drwxr-xr-x 20 root    root    4.0K Apr  6 11:16 ..
-rw-r--r--  1 tcpdump tcpdump   24 Apr  6 12:55 2024_04_06-12_53_30.pcap
-rw-r--r--  1 tcpdump tcpdump   24 Apr  6 12:57 2024_04_06-12_55_12.pcap
-rw-r--r--  1 tcpdump tcpdump    0 Apr  6 12:57 2024_04_06-12_57_27.pcap
root@router1:/services/BambiArkime# tcpdump --version
tcpdump version 4.99.1
libpcap version 1.10.1 (with TPACKET_V3)
OpenSSL 3.0.2 15 Mar 2022
root@router1:/services/BambiArkime# uname -a
Linux router1 5.15.0-101-generic #111-Ubuntu SMP Tue Mar 5 20:16:58 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux

cc @ldruschk

@infrastation
Copy link
Member

Does tcpdump -n -i router -s 0 src net 10.0.0.0/16 and dst net 10.0.0.0/16 capture the packets you expect to appear in the files?

@Trolldemorted
Copy link
Author

yes

@Trolldemorted
Copy link
Author

Trolldemorted commented Apr 7, 2024

Had a look at the code: You don't execute pcap_dump_flush before pcap_dump_close, which you have to if you don't want to lose packets.

#1171 fixes the problem. Tested on Ubuntu 24.04.4 22.04.4

@guyharris
Copy link
Member

You don't execute pcap_dump_flush before pcap_dump_close, which you have to if you don't want to lose packets.

Given that pcap_dump_flush() just does an fflush() and returns an error indication if it fails, and pcap_dump_close() just does an fclose(), the only reason why you would have to call pcap_dump_flush() before calling pcap_dump_close() in order not to lose data would be that, in your C library's implementation of fclose(), the remaining data in the buffer isn't flushed out.

Given that, according to C90, section 7.9.5.1 "The fclose function":

The fclose function causes the stream pointed to by stream to be flushed and the associated file to be closed. Any unwritten buffered data for the stream are delivered to the host environment to be written to the file...

that would indicate a bug in your C library.

What am I missing here? Is there a bug in the GNU libc in Ubuntu 24.04.4, such that fclose() on a FILE * open for writing causes data not yet written to the FILE *'s file descriptor to be discarded rather than written to the file to be discarded rather than being written to the file descriptor? If so, I'd expect a lot more than just tcpdump to break.

@Trolldemorted
Copy link
Author

Thanks for your elaborate response! After browsing through more libpcap and Ubuntu's glibc code (which does indeed correctly flush), a colleague noticed that we were seeing 2 distinct "permission denied" errors in our strace output, one from -z (which failed on some systems due to apparmor fun, which is tolerated), and one from tcpdump proper, which failed to create the subsequent files due to insufficient permissions on the directory. Last night I probably did chmod 777 out of desparation at some point, "fixing" the issue without realizing it, and attributed it to my patch.

Is the first .pcap file created before the privilege drop, unlike the subsequent files? We have a systemd unit that happily restarted tcpdump, rendering us oblivious to the fact that it kept crashing every 30s, because every new tcpdump process did create a new .pcap file, and thus only a few packets went missing (either during super low load, or during rotation while tcpdump crashed).

tl;dr I am a fool and should catch more sleep, sorry for bothering you :( I did read through other code that uses libpcap directly, and it called flush before close, and thus I jumped to conclusions.

@guyharris
Copy link
Member

and one from tcpdump proper, which failed to create the subsequent files due to insufficient permissions on the directory.

If that didn't get printed to the standard error by tcpdump, with tcpdump then exiting, that's a tcpdump bug.

Is the first .pcap file created before the privilege drop, unlike the subsequent files?

If so, that's a tcpdump bug, too; the only thing that should be done with elevated privileges is opening the capture device, as that's the only thing that should require elevated privileges.

@Trolldemorted
Copy link
Author

If that didn't get printed to the standard error by tcpdump, with tcpdump then exiting, that's a tcpdump bug.

Don't worry, it did, that was just my tunnel vision after too many hours of development.

If so, that's a tcpdump bug, too; the only thing that should be done with elevated privileges is opening the capture device, as that's the only thing that should require elevated privileges.

That's interesting, the behaviour on ubuntu is somewhat known (cc @ali-foroughi). I had a closer look today:

  • Self-built tcpdumps (from this repo) works as you say and we expect
  • Ubuntu's shipped tcpdumps don't
  • My ubuntu22 has the apt package tcpdump/now 4.99.1-3ubuntu0.1 installed
  • I cloned tcpdump from launchpad , had a look for matching branches and found 2: applied/4.99.1-3ubuntu0.1 and import/4.99.1-3ubuntu0.1.
  • Assuming that the "applied" one is correct, I checked out the branch, ./configure, make, started it just to see it segfault:
root@majorpurpose:/tcpdumptests# /home/benni/repositories/tcpdump_ubuntu/tcpdump -i any -G 5 -w '%Y_%m_%d-%H_%M_%S.pcap' -s 0
tcpdump: data link type LINUX_SLL2
Segmentation fault (core dumped)
  • gdb says it crashes in getpwnam, and printf debugging tells me that it invokes getpwnam with a nullptr. I don't know anything about Ubuntu's arcane build systems and whether I would need to set more flags or config options to get the real ubuntuesque experience, so I added -Z tcpdump and it stopped crashing, and exhibited the behaviour described above.
  • More debugging reveals that Ubuntu's tcpdump indeed does create the file before the permissions drop. Luckily they have their patches in separate files:
Description: Drop root privileges after opening savefile
Forwarded: no
Bug-Debian: https://bugs.debian.org/935112
Origin: https://src.fedoraproject.org/rpms/tcpdump/raw/master/f/0003-Drop-root-priviledges-before-opening-first-savefile-.patch
---
 tcpdump.1.in |  7 ++++++-
 tcpdump.c    | 30 ++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 1 deletion(-)

--- a/tcpdump.1.in
+++ b/tcpdump.1.in
@@ -267,6 +267,9 @@
 flag, with a number after it, starting at 1 and continuing upward.
 The units of \fIfile_size\fP are millions of bytes (1,000,000 bytes,
 not 1,048,576 bytes).
+
+Note that when used with \fB\-Z\fR option (enabled by default), privileges
+are dropped before opening first savefile.
 .TP
 .B \-d
 Dump the compiled packet-matching code in a human readable form to
@@ -962,12 +965,14 @@
 If
 .I tcpdump
 is running as root, after opening the capture device or input savefile,
-but before opening any savefiles for output, change the user ID to
+change the user ID to
 .I user
 and the group ID to the primary group of
 .IR user .
 .IP
-This behavior can also be enabled by default at compile time.
+This behavior is enabled by default (\fB\-Z tcpdump\fR), and can
+be disabled by \fB\-Z root\fR.
+
 .IP "\fI expression\fP"
 .RS
 selects which packets will be dumped.
--- a/tcpdump.c
+++ b/tcpdump.c
@@ -1492,6 +1492,7 @@
 	cap_rights_t rights;
 	int cansandbox;
 #endif	/* HAVE_CAPSICUM */
+	int chown_flag = 0;
 	int Oflag = 1;			/* run filter code optimizer */
 	int yflag_dlt = -1;
 	const char *yflag_dlt_name = NULL;
@@ -2320,6 +2321,19 @@
 		}
 		capng_apply(CAPNG_SELECT_BOTH);
 #endif /* HAVE_LIBCAP_NG */
+	/* If user is running tcpdump as root and wants to write to the savefile,
+	 * we will check if -C is set and if it is, we will drop root
+	 * privileges right away and consequent call to>pcap_dump_open()
+	 * will most likely fail for the first file. If -C flag is not set we
+	 * will create file as root then change ownership of file to proper
+	 * user(default tcpdump) and drop root privileges.
+	 */
+	if (WFileName)
+		if (Cflag && (username || chroot_dir))
+			droproot(username, chroot_dir);
+		else
+			chown_flag = 1;
+	else
 		if (username || chroot_dir)
 			droproot(username, chroot_dir);
 
@@ -2377,6 +2391,22 @@
 #endif /* HAVE_LIBCAP_NG */
 		if (pdd == NULL)
 			error("%s", pcap_geterr(pd));
+
+		/* Change ownership of file and drop root privileges */
+		if (chown_flag) {
+			struct passwd *pwd;
+
+			pwd = getpwnam(username);
+			if (!pwd)
+				error("Couldn't find user '%s'", username);
+
+			if (strcmp(WFileName, "-") && chown(dumpinfo.CurrentFileName, pwd->pw_uid, pwd->pw_gid) < 0)
+				error("Couldn't change ownership of savefile");
+
+			if (username || chroot_dir)
+				droproot(username, chroot_dir);
+	    }
+
 #ifdef HAVE_CAPSICUM
 		set_dumper_capsicum_rights(pdd);
 #endif
  • So someone (can't really say whether they are Debian or Ubuntu maintainers at this point)
    • indeed implemented an "open file before priv drop" "feature"
    • knew that tcpdump can rotate files, and that their feature won't work then
    • implemented custom handling for -C (if -C is set, do an early priv drop)
    • forgot that -G also rotates (so even if -G is set, it does a late priv drop)
  • Their if (WFileName)... block is not properly indented, which makes the diff harder to read and understand if you are using a simple text editor

Sorry again for bothering you with problems not caused by you. I'll just raise an issue there, and hope for the best :(

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

Successfully merging a pull request may close this issue.

3 participants