From fe835ae95cb147e0d7cab7721c8ce4b3f292eba8 Mon Sep 17 00:00:00 2001 From: Guy Harris Date: Sat, 9 Sep 2023 22:14:08 -0700 Subject: [PATCH] capture child: send an errno message if exec of dumpcap fails. On at least some Linux distributions, dumpcap is either installed with elevated privileges sufficient to support traffic capture by default or can optionally be given those privileges. If it has those privileges, it's typically made group-executable but not world-executable and owned by a special group, e.g. "wireshark", so that only users in that group can use dumpcap to capture traffic. The user installing the Wireshark package is *not* necessarily put into that group by default; this means that any attempt by Wireshark or TShark to run dumpcap will fail with EACCES. If the exec call in the child process sends text error mesages, intended for end users, up the message pipe, as we had been doing, then figuring out *why* the exec failed would require some heuristic parsing to figure out whether it's a permissions problem or not. Instead of doing that, just send a message giving the errno for exec failing. For now, we just format an error message for that in the parent process, but this leaves room to do a better job. While we're at it, fix some cases where an empty error message could be printed. --- capture/capture_sync.c | 78 +++++++++++++++++++++++++++++++++++++++--- sync_pipe.h | 14 ++++---- sync_pipe_write.c | 13 +++++++ tshark.c | 7 ++-- ui/capture.c | 2 +- 5 files changed, 99 insertions(+), 15 deletions(-) diff --git a/capture/capture_sync.c b/capture/capture_sync.c index a87c81ca4d7..40213b8d647 100644 --- a/capture/capture_sync.c +++ b/capture/capture_sync.c @@ -305,7 +305,6 @@ sync_pipe_open_command(char* const argv[], int *data_read_fd, unsigned j; interface_options *interface_opts; #else - char errmsg[1024+1]; int sync_pipe[2]; /* pipe used to send messages from child to parent */ int data_pipe[2]; /* pipe used to send data from child to parent */ #endif @@ -535,9 +534,7 @@ sync_pipe_open_command(char* const argv[], int *data_read_fd, ws_close(sync_pipe[PIPE_READ]); ws_close(sync_pipe[PIPE_WRITE]); execv(argv[0], argv); - snprintf(errmsg, sizeof errmsg, "Couldn't run %s in child process: %s", - argv[0], g_strerror(errno)); - sync_pipe_write_errmsgs_to_parent(2, errmsg, ""); + sync_pipe_write_int_msg(2, SP_EXEC_FAILED, errno); /* Exit with "_exit()", so that we don't close the connection to the X server (and cause stuff buffered up by our parent but @@ -970,6 +967,7 @@ sync_pipe_run_command_actual(char* const argv[], char **data, char **primary_msg char buffer[PIPE_BUF_SIZE+1] = {0}; ssize_t nread; char indicator; + int32_t exec_errno = 0; int primary_msg_len; char *primary_msg_text; int secondary_msg_len; @@ -1034,6 +1032,39 @@ sync_pipe_run_command_actual(char* const argv[], char **data, char **primary_msg /* we got a valid message block from the child, process it */ switch(indicator) { + case SP_EXEC_FAILED: + /* + * Exec of dumpcap failed. Get the errno for the failure. + */ + if (!ws_strtoi32(buffer, NULL, &exec_errno)) { + ws_warning("Invalid errno: %s", buffer); + } + + /* + * Pick up the child status. + */ + ret = sync_pipe_close_command(&data_pipe_read_fd, sync_pipe_read_io, + &fork_child, &msg); + if (ret == -1) { + /* + * Child process failed unexpectedly, or wait failed; msg is the + * error message. + */ + *primary_msg = msg; + *secondary_msg = NULL; + } else { + /* + * Child process failed, but returned the expected exit status. + * Return the messages it gave us, and indicate failure. + */ + *primary_msg = ws_strdup_printf("Couldn't run dumpcap in child process: %s", + g_strerror(exec_errno)); + *secondary_msg = NULL; + ret = -1; + } + *data = NULL; + break; + case SP_ERROR_MSG: /* * Error from dumpcap; there will be a primary message and a @@ -1336,6 +1367,7 @@ sync_interface_stats_open(int *data_read_fd, ws_process_id *fork_child, char **m char buffer[PIPE_BUF_SIZE+1] = {0}; ssize_t nread; char indicator; + int32_t exec_errno = 0; int primary_msg_len; char *primary_msg_text; int secondary_msg_len; @@ -1416,6 +1448,24 @@ sync_interface_stats_open(int *data_read_fd, ws_process_id *fork_child, char **m /* we got a valid message block from the child, process it */ switch(indicator) { + case SP_EXEC_FAILED: + /* + * Exec of dumpcap failed. Get the errno for the failure. + */ + if (!ws_strtoi32(buffer, NULL, &exec_errno)) { + ws_warning("Invalid errno: %s", buffer); + } + *msg = ws_strdup_printf("Couldn't run dumpcap in child process: %s", + g_strerror(exec_errno)); + + /* + * Pick up the child status. + */ + ret = sync_pipe_close_command(data_read_fd, message_read_io, + fork_child, msg); + ret = -1; + break; + case SP_ERROR_MSG: /* * Error from dumpcap; there will be a primary message and a @@ -1663,6 +1713,7 @@ sync_pipe_input_cb(GIOChannel *pipe_io, capture_session *cap_session) char buffer[SP_MAX_MSG_LEN+1] = {0}; ssize_t nread; char indicator; + int32_t exec_errno = 0; int primary_len; char *primary_msg; int secondary_len; @@ -1755,6 +1806,19 @@ sync_pipe_input_cb(GIOChannel *pipe_io, capture_session *cap_session) cap_session->count += npackets; cap_session->new_packets(cap_session, npackets); break; + case SP_EXEC_FAILED: + /* + * Exec of dumpcap failed. Get the errno for the failure. + */ + if (!ws_strtoi32(buffer, NULL, &exec_errno)) { + ws_warning("Invalid errno: %s", buffer); + } + primary_msg = ws_strdup_printf("Couldn't run dumpcap in child process: %s", + g_strerror(exec_errno)); + cap_session->error(cap_session, primary_msg, NULL); + /* the capture child will close the sync_pipe, nothing to do for now */ + /* (an error message doesn't mean we have to stop capturing) */ + break; case SP_ERROR_MSG: /* convert primary message */ pipe_convert_header((unsigned char*)buffer, 4, &indicator, &primary_len); @@ -1793,7 +1857,11 @@ sync_pipe_input_cb(GIOChannel *pipe_io, capture_session *cap_session) break; } default: - ws_assert_not_reached(); + if (g_ascii_isprint(indicator)) + ws_warning("Unknown indicator '%c'", indicator); + else + ws_warning("Unknown indicator '\\x%02x", indicator); + break; } return true; diff --git a/sync_pipe.h b/sync_pipe.h index 7db57d42551..4b54c6222c5 100644 --- a/sync_pipe.h +++ b/sync_pipe.h @@ -36,6 +36,7 @@ * (http://code.google.com/p/protobuf-c/) if we ever need to use more * complex messages. */ +#define SP_EXEC_FAILED 'X' /* errno value for the exec failing */ #define SP_FILE 'F' /* the name of the recently opened file */ #define SP_ERROR_MSG 'E' /* error message */ #define SP_BAD_FILTER 'B' /* error message for bad capture filter */ @@ -63,14 +64,13 @@ sync_pipe_write_string_msg(int pipe_fd, char indicator, const char *msg); extern void sync_pipe_write_uint_msg(int pipe_fd, char indicator, unsigned int num); +/* Write a message, with an integer body, to the recipient pipe in the + standard format (1-byte message indicator, 3-byte message length + (excluding length and indicator field), and the integer, as a string. */ +extern void +sync_pipe_write_int_msg(int pipe_fd, char indicator, int num); + /** the child encountered an error, notify the parent */ -/* Write a message, with two message strings as the body, to the - recipient pipe. The header is an SP_ERROR_MSG header, with the - length being the length of two string submessages; the submessages - are the body of the message, with each submessage being a message - with an indicator of SP_ERROR_MSG, the first message having - first message string and the second message having the second message - string. */ extern void sync_pipe_write_errmsgs_to_parent(int pipe_fd, const char *error_msg, const char *secondary_error_msg); diff --git a/sync_pipe_write.c b/sync_pipe_write.c index 367102e02d4..c279e5765f4 100644 --- a/sync_pipe_write.c +++ b/sync_pipe_write.c @@ -97,6 +97,19 @@ sync_pipe_write_uint_msg(int pipe_fd, char indicator, unsigned int num) sync_pipe_write_string_msg(pipe_fd, indicator, count_str); } +/* Write a message, with an integer body, to the recipient pipe in the + standard format (1-byte message indicator, 3-byte message length + (excluding length and indicator field), and the unsigned integer, + as a string. */ +void +sync_pipe_write_int_msg(int pipe_fd, char indicator, int num) +{ + char count_str[SP_DECISIZE+1+1]; + + snprintf(count_str, sizeof(count_str), "%d", num); + sync_pipe_write_string_msg(pipe_fd, indicator, count_str); +} + /* Write a message, with a primary and secondary error message as the body, to the recipient pipe. The header is an SP_ERROR_MSG header, with the length being the length of two string submessages; the submessages diff --git a/tshark.c b/tshark.c index 16883d0dcde..94ad56695be 100644 --- a/tshark.c +++ b/tshark.c @@ -2871,7 +2871,10 @@ static void capture_input_error(capture_session *cap_session _U_, char *error_msg, char *secondary_error_msg) { cmdarg_err("%s", error_msg); - cmdarg_err_cont("%s", secondary_error_msg); + if (secondary_error_msg != NULL && *secondary_error_msg != '\0') { + /* We have both primary and secondary messages. */ + cmdarg_err_cont("%s", secondary_error_msg); + } } @@ -3159,7 +3162,7 @@ capture_input_drops(capture_session *cap_session _U_, guint32 dropped, const cha static void capture_input_closed(capture_session *cap_session _U_, gchar *msg) { - if (msg != NULL) + if (msg != NULL && *msg != '\0') fprintf(stderr, "tshark: %s\n", msg); report_counts(); diff --git a/ui/capture.c b/ui/capture.c index 894a8a1fb39..13bacc59973 100644 --- a/ui/capture.c +++ b/ui/capture.c @@ -620,7 +620,7 @@ capture_input_error(capture_session *cap_session _U_, char *error_msg, ws_assert(cap_session->state == CAPTURE_PREPARING || cap_session->state == CAPTURE_RUNNING); safe_error_msg = simple_dialog_format_message(error_msg); - if (*secondary_error_msg != '\0') { + if (secondary_error_msg != NULL && *secondary_error_msg != '\0') { /* We have both primary and secondary messages. */ safe_secondary_error_msg = simple_dialog_format_message(secondary_error_msg); simple_dialog(ESD_TYPE_ERROR, ESD_BTN_OK, "%s%s%s\n\n%s",