Skip to content

Commit 54bdc7c

Browse files
millertmillert
committed
Add missing NULL checks for messages inside other protobuf messages.
When using nesting messages in protocol buffers, we need to verify that the nested message (or struct) is non-NULL before using it. Also correct the checks for strings, which are initialized to the empty string and not NULL. Thanks to Joshua Rogers for finding these.
1 parent 40fb619 commit 54bdc7c

File tree

6 files changed

+78
-24
lines changed

6 files changed

+78
-24
lines changed

‎logsrvd/iolog_writer.c‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,11 @@ evlog_new(const TimeSpec *submit_time, InfoMessage * const *info_msgs,
178178
/* Pull out values by key from info array. */
179179
for (idx = 0; idx < infolen; idx++) {
180180
const InfoMessage *info = info_msgs[idx];
181-
const char *key = info->key;
181+
const char *key;
182+
183+
if (info == NULL)
184+
continue;
185+
key = info->key;
182186
switch (key[0]) {
183187
case 'c':
184188
if (strcmp(key, "columns") == 0) {

‎logsrvd/logsrvd.c‎

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -458,7 +458,7 @@ handle_accept(const AcceptMessage *msg, const uint8_t *buf, size_t len,
458458
}
459459

460460
/* Check that message is valid. */
461-
if (msg->submit_time == NULL || msg->n_info_msgs == 0) {
461+
if (msg == NULL || msg->submit_time == NULL || msg->n_info_msgs == 0) {
462462
sudo_warnx(U_("%s: %s"), source, U_("invalid AcceptMessage"));
463463
closure->errstr = _("invalid AcceptMessage");
464464
debug_return_bool(false);
@@ -495,7 +495,8 @@ handle_reject(const RejectMessage *msg, const uint8_t *buf, size_t len,
495495
}
496496

497497
/* Check that message is valid. */
498-
if (msg->submit_time == NULL || msg->n_info_msgs == 0) {
498+
if (msg == NULL || msg->submit_time == NULL || msg->reason[0] == '\0' ||
499+
msg->n_info_msgs == 0) {
499500
sudo_warnx(U_("%s: %s"), source, U_("invalid RejectMessage"));
500501
closure->errstr = _("invalid RejectMessage");
501502
debug_return_bool(false);
@@ -527,7 +528,7 @@ handle_exit(const ExitMessage *msg, const uint8_t *buf, size_t len,
527528
}
528529

529530
/* Check that message is valid. */
530-
if (msg->run_time == NULL) {
531+
if (msg == NULL || msg->run_time == NULL) {
531532
sudo_warnx(U_("%s: %s"), source, U_("invalid ExitMessage"));
532533
closure->errstr = _("invalid ExitMessage");
533534
debug_return_bool(false);
@@ -581,7 +582,7 @@ handle_restart(const RestartMessage *msg, const uint8_t *buf, size_t len,
581582
}
582583

583584
/* Check that message is valid. */
584-
if (msg->log_id == NULL || msg->resume_point == NULL) {
585+
if (msg == NULL || msg->log_id[0] == '\0' || msg->resume_point == NULL) {
585586
sudo_warnx(U_("%s: %s"), source, U_("invalid RestartMessage"));
586587
closure->errstr = _("invalid RestartMessage");
587588
debug_return_bool(false);
@@ -616,7 +617,7 @@ handle_alert(const AlertMessage *msg, const uint8_t *buf, size_t len,
616617
debug_decl(handle_alert, SUDO_DEBUG_UTIL);
617618

618619
/* Check that message is valid. */
619-
if (msg->alert_time == NULL || msg->reason == NULL) {
620+
if (msg == NULL || msg->alert_time == NULL || msg->reason[0] == '\0') {
620621
sudo_warnx(U_("%s: %s"), source, U_("invalid AlertMessage"));
621622
closure->errstr = _("invalid AlertMessage");
622623
debug_return_bool(false);
@@ -665,7 +666,7 @@ handle_iobuf(int iofd, const IoBuffer *iobuf, const uint8_t *buf, size_t len,
665666
}
666667

667668
/* Check that message is valid. */
668-
if (iobuf->delay == NULL) {
669+
if (iobuf == NULL || iobuf->delay == NULL || iobuf->data.len == 0) {
669670
sudo_warnx(U_("%s: %s"), source, U_("invalid IoBuffer"));
670671
closure->errstr = _("invalid IoBuffer");
671672
debug_return_bool(false);
@@ -701,7 +702,7 @@ handle_winsize(const ChangeWindowSize *msg, const uint8_t *buf, size_t len,
701702
}
702703

703704
/* Check that message is valid. */
704-
if (msg->delay == NULL) {
705+
if (msg == NULL || msg->delay == NULL) {
705706
sudo_warnx(U_("%s: %s"), source, U_("invalid ChangeWindowSize"));
706707
closure->errstr = _("invalid ChangeWindowSize");
707708
debug_return_bool(false);
@@ -737,7 +738,7 @@ handle_suspend(const CommandSuspend *msg, const uint8_t *buf, size_t len,
737738
}
738739

739740
/* Check that message is valid. */
740-
if (msg->delay == NULL || msg->signal == NULL) {
741+
if (msg == NULL || msg->delay == NULL || msg->signal[0] == '\0') {
741742
sudo_warnx(U_("%s: %s"), source, U_("invalid CommandSuspend"));
742743
closure->errstr = _("invalid CommandSuspend");
743744
debug_return_bool(false);
@@ -767,10 +768,16 @@ handle_client_hello(const ClientHello *msg, const uint8_t *buf, size_t len,
767768
debug_return_bool(false);
768769
}
769770

771+
/* Check that message is valid. */
772+
if (msg == NULL || msg->client_id[0] == '\0') {
773+
sudo_warnx(U_("%s: %s"), source, U_("invalid ClientHello"));
774+
closure->errstr = _("invalid ClientHello");
775+
debug_return_bool(false);
776+
}
770777
sudo_debug_printf(SUDO_DEBUG_INFO, "%s: received ClientHello",
771778
__func__);
772779
sudo_debug_printf(SUDO_DEBUG_INFO, "%s: client ID %s",
773-
__func__, msg->client_id ? msg->client_id : "unknown");
780+
__func__, msg->client_id);
774781

775782
debug_return_bool(true);
776783
}

‎logsrvd/logsrvd_local.c‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -405,12 +405,13 @@ store_exit_local(const ExitMessage *msg, const uint8_t *buf, size_t len,
405405
int flags = 0;
406406
debug_decl(store_exit_local, SUDO_DEBUG_UTIL);
407407

408+
/* TODO: store the "error" string if present. */
408409
if (msg->run_time != NULL) {
409410
evlog->run_time.tv_sec = (time_t)msg->run_time->tv_sec;
410411
evlog->run_time.tv_nsec = (long)msg->run_time->tv_nsec;
411412
}
412413
evlog->exit_value = msg->exit_value;
413-
if (msg->signal != NULL && msg->signal[0] != '\0') {
414+
if (msg->signal[0] != '\0') {
414415
sudo_debug_printf(SUDO_DEBUG_INFO|SUDO_DEBUG_LINENO,
415416
"command was killed by SIG%s%s", msg->signal,
416417
msg->dumped_core ? " (core dumped)" : "");

‎logsrvd/logsrvd_relay.c‎

Lines changed: 31 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -516,7 +516,7 @@ handle_server_hello(const ServerHello *msg, struct connection_closure *closure)
516516
}
517517

518518
/* Check that ServerHello is valid. */
519-
if (msg->server_id == NULL || msg->server_id[0] == '\0') {
519+
if (msg == NULL || msg->server_id == NULL || msg->server_id[0] == '\0') {
520520
sudo_warnx(U_("%s: invalid ServerHello, missing server_id"),
521521
relay_closure->relay_name.ipaddr);
522522
closure->errstr = _("invalid ServerHello");
@@ -533,7 +533,7 @@ handle_server_hello(const ServerHello *msg, struct connection_closure *closure)
533533
}
534534

535535
/*
536-
* Respond to a CommitPoint message from the relay.
536+
* Respond to a commit_point ServerMessage from the relay.
537537
* Returns true on success, false on error.
538538
*/
539539
static bool
@@ -549,13 +549,21 @@ handle_commit_point(const TimeSpec *commit_point,
549549
debug_return_bool(false);
550550
}
551551

552+
/* Check that ServerMessage's commit_point is valid. */
553+
if (commit_point == NULL) {
554+
sudo_warnx(U_("%s: invalid ServerMessage, missing commit_point"),
555+
closure->relay_closure->relay_name.ipaddr);
556+
closure->errstr = _("invalid ServerMessage");
557+
debug_return_bool(false);
558+
}
559+
552560
/* Pass commit point from relay to client. */
553561
debug_return_bool(schedule_commit_point(commit_point, closure));
554562
}
555563

556564
/*
557-
* Respond to a LogId message from the relay.
558-
* Always returns true.
565+
* Respond to a log_id ServerMessage from the relay.
566+
* Returns true on success, false on error.
559567
*/
560568
static bool
561569
handle_log_id(const char *id, struct connection_closure *closure)
@@ -574,6 +582,13 @@ handle_log_id(const char *id, struct connection_closure *closure)
574582
if (closure->write_ev == NULL)
575583
debug_return_bool(true);
576584

585+
if (id[0] == '\0') {
586+
sudo_warnx(U_("%s: invalid ServerMessage, missing log_id string"),
587+
closure->relay_closure->relay_name.ipaddr);
588+
closure->errstr = _("invalid ServerMessage");
589+
debug_return_bool(false);
590+
}
591+
577592
/* Generate a new log ID that includes the relay host. */
578593
len = asprintf(&new_id, "%s/%s", id,
579594
closure->relay_closure->relay_name.name);
@@ -593,8 +608,8 @@ handle_log_id(const char *id, struct connection_closure *closure)
593608
}
594609

595610
/*
596-
* Respond to a ServerError message from the relay.
597-
* Always returns false.
611+
* Respond to an error ServerMessage from the relay.
612+
* Returns true on success, false on error.
598613
*/
599614
static bool
600615
handle_server_error(const char *errmsg, struct connection_closure *closure)
@@ -611,15 +626,19 @@ handle_server_error(const char *errmsg, struct connection_closure *closure)
611626
sudo_ev_del(closure->evbase, closure->relay_closure->read_ev);
612627
sudo_ev_del(closure->evbase, closure->relay_closure->write_ev);
613628

629+
/* Missing error string. */
630+
if (errmsg[0] == '\0')
631+
errmsg = "unknown error";
632+
614633
if (!schedule_error_message(errmsg, closure))
615634
debug_return_bool(false);
616635

617636
debug_return_bool(true);
618637
}
619638

620639
/*
621-
* Respond to a ServerAbort message from the server.
622-
* Always returns false.
640+
* Respond to an abort ServerMessage from the relay.
641+
* Returns true on success, false on error.
623642
*/
624643
static bool
625644
handle_server_abort(const char *errmsg, struct connection_closure *closure)
@@ -632,6 +651,10 @@ handle_server_abort(const char *errmsg, struct connection_closure *closure)
632651
relay_closure->relay_name.name, relay_closure->relay_name.ipaddr,
633652
errmsg);
634653

654+
/* Missing error string. */
655+
if (errmsg[0] == '\0')
656+
errmsg = "unknown error";
657+
635658
if (!schedule_error_message(errmsg, closure))
636659
debug_return_bool(false);
637660

‎logsrvd/sendlog.c‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1132,7 +1132,7 @@ handle_server_hello(const ServerHello *msg, struct client_closure *closure)
11321132
}
11331133

11341134
/* Check that ServerHello is valid. */
1135-
if (msg->server_id == NULL || msg->server_id[0] == '\0') {
1135+
if (msg == NULL || msg->server_id == NULL || msg->server_id[0] == '\0') {
11361136
sudo_warnx("%s", U_("invalid ServerHello"));
11371137
debug_return_bool(false);
11381138
}
@@ -1151,7 +1151,7 @@ handle_server_hello(const ServerHello *msg, struct client_closure *closure)
11511151
}
11521152

11531153
/*
1154-
* Respond to a CommitPoint message from the server.
1154+
* Respond to a commit_point ServerMessage from the server.
11551155
* Returns true on success, false on error.
11561156
*/
11571157
static bool
@@ -1166,6 +1166,13 @@ handle_commit_point(const TimeSpec *commit_point,
11661166
debug_return_bool(false);
11671167
}
11681168

1169+
/* Check that ServerMessage's commit_point is valid. */
1170+
if (commit_point == NULL) {
1171+
sudo_warnx(U_("%s: invalid ServerMessage, missing commit_point"),
1172+
server_info.name);
1173+
debug_return_bool(false);
1174+
}
1175+
11691176
sudo_debug_printf(SUDO_DEBUG_INFO, "%s: commit point: [%lld, %d]",
11701177
__func__, (long long)commit_point->tv_sec, commit_point->tv_nsec);
11711178
closure->committed.tv_sec = (time_t)commit_point->tv_sec;
@@ -1175,7 +1182,7 @@ handle_commit_point(const TimeSpec *commit_point,
11751182
}
11761183

11771184
/*
1178-
* Respond to a LogId message from the server.
1185+
* Respond to a log_id ServerMessage from the relay.
11791186
* Always returns true.
11801187
*/
11811188
static bool

‎plugins/sudoers/log_client.c‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1502,7 +1502,7 @@ handle_server_hello(const ServerHello *msg, struct client_closure *closure)
15021502
}
15031503

15041504
/* Check that ServerHello is valid. */
1505-
if (msg->server_id == NULL || msg->server_id[0] == '\0') {
1505+
if (msg == NULL || msg->server_id == NULL || msg->server_id[0] == '\0') {
15061506
sudo_warnx("%s", U_("invalid ServerHello"));
15071507
debug_return_bool(false);
15081508
}
@@ -1526,7 +1526,7 @@ handle_server_hello(const ServerHello *msg, struct client_closure *closure)
15261526
}
15271527

15281528
/*
1529-
* Respond to a CommitPoint message from the server.
1529+
* Respond to a commit_point ServerMessage from the server.
15301530
* Returns true on success, false on error.
15311531
*/
15321532
static bool
@@ -1541,6 +1541,12 @@ handle_commit_point(const TimeSpec *commit_point,
15411541
debug_return_bool(false);
15421542
}
15431543

1544+
/* Check that ServerMessage's commit_point is valid. */
1545+
if (commit_point == NULL) {
1546+
sudo_warnx(U_("invalid ServerMessage"));
1547+
debug_return_bool(false);
1548+
}
1549+
15441550
closure->committed.tv_sec = (time_t)commit_point->tv_sec;
15451551
closure->committed.tv_nsec = (long)commit_point->tv_nsec;
15461552
sudo_debug_printf(SUDO_DEBUG_INFO,
@@ -1570,6 +1576,12 @@ handle_log_id(const char *id, struct client_closure *closure)
15701576
debug_decl(handle_log_id, SUDOERS_DEBUG_UTIL);
15711577

15721578
sudo_debug_printf(SUDO_DEBUG_INFO, "%s: remote log ID: %s", __func__, id);
1579+
1580+
if (id[0] == '\0') {
1581+
sudo_warnx(U_("invalid ServerMessage"));
1582+
debug_return_bool(false);
1583+
}
1584+
15731585
if (closure->iolog_id == NULL) {
15741586
if ((closure->iolog_id = strdup(id)) == NULL)
15751587
sudo_fatal(U_("%s: %s"), __func__, U_("unable to allocate memory"));

0 commit comments

Comments
 (0)