Skip to content

Commit b48fd11

Browse files
committed
Embed struct eventlog into struct log_details instead of using a pointer.
In some cases we were allocating a new struct eventlog and others just attaching a pointer which did not always persist after the function returns. In practice this was not a problem. The eventlog data in log_details was not being accessed after it went out of scope, but this is brittle and makes analysis difficult. Found by the ZeroPath AI Security Engineer <https://zeropath.com>
1 parent d83714a commit b48fd11

File tree

7 files changed

+73
-58
lines changed

7 files changed

+73
-58
lines changed

include/sudo_eventlog.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,7 @@ bool eventlog_reject(const struct eventlog *evlog, int flags, const char *reason
143143
bool eventlog_store_json(struct json_container *jsonc, const struct eventlog *evlog);
144144
bool eventlog_store_sudo(int event_type, const struct eventlog *evlog, struct sudo_lbuf *lbuf);
145145
void eventlog_free(struct eventlog *evlog);
146+
void eventlog_free_contents(struct eventlog *evlog);
146147

147148
/* eventlog_conf.c */
148149
void eventlog_set_type(int type);

lib/eventlog/eventlog_free.c

Lines changed: 49 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
/*
22
* SPDX-License-Identifier: ISC
33
*
4-
* Copyright (c) 2020 Todd C. Miller <[email protected]>
4+
* Copyright (c) 2020, 2023, 2025 Todd C. Miller <[email protected]>
55
*
66
* Permission to use, copy, modify, and distribute this software for any
77
* purpose with or without fee is hereby granted, provided that the above
@@ -34,46 +34,59 @@
3434
* Free the strings in a struct eventlog.
3535
*/
3636
void
37-
eventlog_free(struct eventlog *evlog)
37+
eventlog_free_contents(struct eventlog *evlog)
3838
{
3939
size_t i;
40+
debug_decl(eventlog_free_contents, SUDO_DEBUG_UTIL);
41+
42+
free(evlog->iolog_path);
43+
free(evlog->command);
44+
free(evlog->cwd);
45+
free(evlog->runchroot);
46+
free(evlog->runcwd);
47+
free(evlog->rungroup);
48+
free(evlog->runuser);
49+
free(evlog->peeraddr);
50+
free(evlog->signal_name);
51+
free(evlog->source);
52+
if (evlog->submitenv != NULL) {
53+
for (i = 0; evlog->submitenv[i] != NULL; i++)
54+
free(evlog->submitenv[i]);
55+
free(evlog->submitenv);
56+
}
57+
free(evlog->submithost);
58+
free(evlog->submituser);
59+
free(evlog->submitgroup);
60+
free(evlog->ttyname);
61+
if (evlog->runargv != NULL) {
62+
for (i = 0; evlog->runargv[i] != NULL; i++)
63+
free(evlog->runargv[i]);
64+
free(evlog->runargv);
65+
}
66+
if (evlog->runenv != NULL) {
67+
for (i = 0; evlog->runenv[i] != NULL; i++)
68+
free(evlog->runenv[i]);
69+
free(evlog->runenv);
70+
}
71+
if (evlog->env_add != NULL) {
72+
for (i = 0; evlog->env_add[i] != NULL; i++)
73+
free(evlog->env_add[i]);
74+
free(evlog->env_add);
75+
}
76+
77+
debug_return;
78+
}
79+
80+
/*
81+
* Free the strings in a struct eventlog and the eventlog container itself.
82+
*/
83+
void
84+
eventlog_free(struct eventlog *evlog)
85+
{
4086
debug_decl(eventlog_free, SUDO_DEBUG_UTIL);
4187

4288
if (evlog != NULL) {
43-
free(evlog->iolog_path);
44-
free(evlog->command);
45-
free(evlog->cwd);
46-
free(evlog->runchroot);
47-
free(evlog->runcwd);
48-
free(evlog->rungroup);
49-
free(evlog->runuser);
50-
free(evlog->peeraddr);
51-
free(evlog->signal_name);
52-
free(evlog->source);
53-
if (evlog->submitenv != NULL) {
54-
for (i = 0; evlog->submitenv[i] != NULL; i++)
55-
free(evlog->submitenv[i]);
56-
free(evlog->submitenv);
57-
}
58-
free(evlog->submithost);
59-
free(evlog->submituser);
60-
free(evlog->submitgroup);
61-
free(evlog->ttyname);
62-
if (evlog->runargv != NULL) {
63-
for (i = 0; evlog->runargv[i] != NULL; i++)
64-
free(evlog->runargv[i]);
65-
free(evlog->runargv);
66-
}
67-
if (evlog->runenv != NULL) {
68-
for (i = 0; evlog->runenv[i] != NULL; i++)
69-
free(evlog->runenv[i]);
70-
free(evlog->runenv);
71-
}
72-
if (evlog->env_add != NULL) {
73-
for (i = 0; evlog->env_add[i] != NULL; i++)
74-
free(evlog->env_add[i]);
75-
free(evlog->env_add);
76-
}
89+
eventlog_free_contents(evlog);
7790
free(evlog);
7891
}
7992

plugins/sudoers/audit.c

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,10 @@ audit_to_eventlog(const struct sudoers_context *ctx, struct eventlog *evlog,
241241
}
242242

243243
#ifdef SUDOERS_LOG_CLIENT
244+
/*
245+
* Persistent audit details and associated event log data used
246+
* when opening a new connection to the log server.
247+
*/
244248
static struct log_details audit_details;
245249

246250
static void

plugins/sudoers/iolog.c

Lines changed: 14 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -190,16 +190,17 @@ free_iolog_details(void)
190190
{
191191
debug_decl(free_iolog_details, SUDOERS_DEBUG_PLUGIN);
192192

193-
if (iolog_details.evlog != NULL) {
194-
/* We only make a shallow copy of argv and envp. */
195-
free(iolog_details.evlog->runargv);
196-
iolog_details.evlog->runargv = NULL;
197-
free(iolog_details.evlog->runenv);
198-
iolog_details.evlog->runenv = NULL;
199-
free(iolog_details.evlog->submitenv);
200-
iolog_details.evlog->submitenv = NULL;
201-
eventlog_free(iolog_details.evlog);
202-
}
193+
/*
194+
* We only make a shallow copy of argv and envp.
195+
*/
196+
free(iolog_details.evlog.runargv);
197+
iolog_details.evlog.runargv = NULL;
198+
free(iolog_details.evlog.runenv);
199+
iolog_details.evlog.runenv = NULL;
200+
free(iolog_details.evlog.submitenv);
201+
iolog_details.evlog.submitenv = NULL;
202+
eventlog_free_contents(&iolog_details.evlog);
203+
203204
str_list_free(iolog_details.log_servers);
204205
#if defined(HAVE_OPENSSL)
205206
free(iolog_details.ca_bundle);
@@ -290,7 +291,7 @@ iolog_deserialize_info(struct log_details *details, char * const user_info[],
290291
char * const command_info[], char * const argv[], char * const user_env[])
291292
{
292293
const struct sudoers_context *ctx = sudoers_get_context();
293-
struct eventlog *evlog;
294+
struct eventlog *evlog = &details->evlog;
294295
const char *runas_uid_str = "0", *runas_euid_str = NULL;
295296
const char *runas_gid_str = "0", *runas_egid_str = NULL;
296297
const char *errstr;
@@ -301,10 +302,6 @@ iolog_deserialize_info(struct log_details *details, char * const user_info[],
301302
id_t id;
302303
debug_decl(iolog_deserialize_info, SUDOERS_DEBUG_UTIL);
303304

304-
if ((evlog = calloc(1, sizeof(*evlog))) == NULL)
305-
goto oom;
306-
details->evlog = evlog;
307-
308305
evlog->lines = 24;
309306
evlog->columns = 80;
310307
evlog->runuid = ROOT_UID;
@@ -683,7 +680,7 @@ static int
683680
sudoers_io_open_local(struct timespec *start_time)
684681
{
685682
const struct sudoers_context *ctx = sudoers_get_context();
686-
struct eventlog *evlog = iolog_details.evlog;
683+
struct eventlog *evlog = &iolog_details.evlog;
687684
int i;
688685
debug_decl(sudoers_io_open_local, SUDOERS_DEBUG_PLUGIN);
689686

@@ -727,7 +724,7 @@ sudoers_io_open_local(struct timespec *start_time)
727724
}
728725

729726
/* Write log file with user and command details. */
730-
if (!iolog_write_info_file(iolog_dir_fd, iolog_details.evlog)) {
727+
if (!iolog_write_info_file(iolog_dir_fd, &iolog_details.evlog)) {
731728
log_warningx(ctx, SLOG_SEND_MAIL,
732729
N_("unable to write to I/O log file: %s"), strerror(errno));
733730
warned = true;

plugins/sudoers/log_client.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1164,7 +1164,7 @@ fmt_initial_message(struct client_closure *closure)
11641164
switch (closure->state) {
11651165
case SEND_ACCEPT:
11661166
/* Format and schedule AcceptMessage. */
1167-
if ((ret = fmt_accept_message(closure, closure->log_details->evlog))) {
1167+
if ((ret = fmt_accept_message(closure, &closure->log_details->evlog))) {
11681168
/*
11691169
* Move read/write events back to main sudo event loop.
11701170
* Server messages may occur at any time, so no timeout.
@@ -1180,12 +1180,12 @@ fmt_initial_message(struct client_closure *closure)
11801180
break;
11811181
case SEND_REJECT:
11821182
/* Format and schedule RejectMessage. */
1183-
ret = fmt_reject_message(closure, closure->log_details->evlog,
1183+
ret = fmt_reject_message(closure, &closure->log_details->evlog,
11841184
closure->reason);
11851185
break;
11861186
case SEND_ALERT:
11871187
/* Format and schedule AlertMessage. */
1188-
ret = fmt_alert_message(closure, closure->log_details->evlog,
1188+
ret = fmt_alert_message(closure, &closure->log_details->evlog,
11891189
closure->reason);
11901190
break;
11911191
default:

plugins/sudoers/logging.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ init_log_details(struct log_details *details, struct eventlog *evlog)
106106
debug_return_bool(false);
107107
}
108108

109-
details->evlog = evlog;
109+
details->evlog = *evlog;
110110
details->ignore_log_errors = def_ignore_logfile_errors;
111111
details->log_servers = log_servers;
112112
details->server_timeout.tv_sec = def_log_server_timeout;

plugins/sudoers/logging.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424

2525
struct sudoers_str_list;
2626
struct log_details {
27-
struct eventlog *evlog;
27+
struct eventlog evlog;
2828
struct sudoers_str_list *log_servers;
2929
struct timespec server_timeout;
3030
# if defined(HAVE_OPENSSL)

0 commit comments

Comments
 (0)