Skip to content

Commit 4436b21

Browse files
abbraabbra
committed
CVE-2026-14612: ipa-otpd/oauth2: fix memory safety and error handling bugs
Fix several pre-existing bugs in the OAuth2 device code flow: - Use-after-free: otpd_queue_item_free() was called before the retry check in oauth2_on_child_writable(). On EAGAIN, the freed saved_item would be dereferenced on the next callback invocation. - CVE-2026-14612: buffer overflow: read() into a 10240-byte buffer could return exactly 10240 bytes, causing buf[10240]='\0' to write past the end. Use sizeof(buf)-1 instead. - Memory leaks: saved_item was leaked on strndup failure and on child_ctx allocation failure. The done: label now cleans up saved_item, pipe fds, and child_ctx on error paths. - NULL dereference: ipaidpSub was dereferenced without a NULL check in check_access_token_reply(). - verto_add_child() return was not checked for NULL, leading to a NULL dereference on verto_set_private(). - set_fd_nonblocking() return values were silently ignored; a failure would leave pipes in blocking mode, hanging the daemon. - Off-by-one in memchr length calculation for second newline. - Avoid logging full oidc_child output (may contain tokens); log byte count instead. - Fix error message for reader event (was copy-pasted as "writer"). - Fix uninitialized loop variable in disabled debug block. Fixes: CVE-2026-14612 Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
1 parent 33d5f0a commit 4436b21

1 file changed

Lines changed: 65 additions & 15 deletions

File tree

‎daemons/ipa-otpd/oauth2.c‎

Lines changed: 65 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -125,8 +125,6 @@ static void oauth2_on_child_writable(verto_ctx *vctx, verto_ev *ev)
125125
idx++;
126126
io = writev(verto_get_fd(ev), iov, idx);
127127
}
128-
otpd_queue_item_free(child_ctx->saved_item);
129-
130128
if (io < 0) {
131129
switch (errno) {
132130
#if defined(EWOULDBLOCK) && (!defined(EAGAIN) || EAGAIN - EWOULDBLOCK != 0)
@@ -146,6 +144,10 @@ static void oauth2_on_child_writable(verto_ctx *vctx, verto_ev *ev)
146144
otpd_log_err(errno, "Failed to send to child");
147145
}
148146

147+
if (child_ctx->saved_item != NULL) {
148+
otpd_queue_item_free(child_ctx->saved_item);
149+
child_ctx->saved_item = NULL;
150+
}
149151
verto_del(ev);
150152
}
151153

@@ -242,6 +244,12 @@ static int check_access_token_reply(struct child_ctx *child_ctx,
242244
{
243245
int ret;
244246

247+
if (child_ctx->item->user.ipaidpSub == NULL) {
248+
otpd_log_req(child_ctx->item->req,
249+
"Missing ipaidpSub for access token verification");
250+
return EPERM;
251+
}
252+
245253
if (strlen(child_ctx->item->user.ipaidpSub) != len
246254
|| memcmp(child_ctx->item->user.ipaidpSub, buf, len) != 0) {
247255
return EPERM;
@@ -279,17 +287,21 @@ static void oauth2_on_child_readable(verto_ctx *vctx, verto_ev *ev)
279287
child_ctx->item->rsp = NULL;
280288
child_ctx->item->sent = 0;
281289

282-
io = read(verto_get_fd(ev), buf, 10240);
290+
io = read(verto_get_fd(ev), buf, sizeof(buf) - 1);
283291
if (io < 0) {
284292
otpd_log_err(errno, "Failed to read from child");
285293
goto done;
286294
}
287295

288-
if (io >= 0) {
289-
buf[io] = '\0';
290-
otpd_log_req(child_ctx->item->req, "Received: [%s]", buf);
296+
if (io == 0) {
297+
otpd_log_req(child_ctx->item->req,
298+
"oidc_child closed stdout without producing output");
299+
goto done;
291300
}
292301

302+
buf[io] = '\0';
303+
otpd_log_req(child_ctx->item->req, "Received: [%zu bytes]", (size_t)io);
304+
293305
verto_del(ev);
294306

295307
if (child_ctx->oauth2_state == OAUTH2_GET_DEVICE_CODE) {
@@ -302,7 +314,7 @@ static void oauth2_on_child_readable(verto_ctx *vctx, verto_ev *ev)
302314
if (rad_reply != NULL) {
303315
*rad_reply = '\0';
304316
rad_reply++;
305-
end = memchr(rad_reply, '\n', io - (rad_reply - 1 - buf));
317+
end = memchr(rad_reply, '\n', io - (rad_reply - buf));
306318
if (end == NULL) {
307319
otpd_log_req(child_ctx->item->req, "Missing second new-line.");
308320
goto done;
@@ -387,16 +399,20 @@ int oauth2(struct otpd_queue_item **item, enum oauth2_state oauth2_state)
387399

388400
saved_item = calloc(sizeof(struct otpd_queue_item), 1);
389401
if (saved_item == NULL) {
390-
otpd_log_req((*item)->req, "No matching saved state found");
391-
return EINVAL;
402+
otpd_log_req((*item)->req,
403+
"Failed to allocate saved state item");
404+
krb5_free_data_contents(NULL, &data_state);
405+
return ENOMEM;
392406
}
393407
saved_item->oauth2.device_code_reply = strndup(data_state.data,
394408
data_state.length);
409+
krb5_free_data_contents(NULL, &data_state);
395410
if (saved_item->oauth2.device_code_reply == NULL) {
396411
otpd_log_req((*item)->req, "Failed to copy device code reply");
397-
return EINVAL;
412+
free(saved_item);
413+
saved_item = NULL;
414+
return ENOMEM;
398415
}
399-
krb5_free_data_contents(NULL, &data_state);
400416
}
401417

402418
child_ctx = calloc(sizeof(struct child_ctx), 1);
@@ -406,6 +422,7 @@ int oauth2(struct otpd_queue_item **item, enum oauth2_state oauth2_state)
406422
}
407423
child_ctx->item = (*item);
408424
child_ctx->saved_item = saved_item;
425+
saved_item = NULL; /* ownership transferred to child_ctx */
409426
child_ctx->oauth2_state = oauth2_state;
410427

411428
otpd_log_req((*item)->req, "oauth2 start: %s",
@@ -466,7 +483,7 @@ int oauth2(struct otpd_queue_item **item, enum oauth2_state oauth2_state)
466483
}
467484

468485
#if 0
469-
for (int i; args[i]; i++) {
486+
for (int i = 0; args[i]; i++) {
470487
otpd_log_req((*item)->req, "oidc_child exec: %s", args[i]);
471488
}
472489
#endif
@@ -501,11 +518,21 @@ int oauth2(struct otpd_queue_item **item, enum oauth2_state oauth2_state)
501518
exit(EXIT_FAILURE);
502519
} else if (child_pid > 0) { /* parent */
503520
close(pipefd_to_child[0]);
504-
set_fd_nonblocking(pipefd_to_child[1]);
521+
pipefd_to_child[0] = -1;
522+
ret = set_fd_nonblocking(pipefd_to_child[1]);
523+
if (ret != 0) {
524+
otpd_log_err(ret, "Failed to set write pipe non-blocking");
525+
goto done;
526+
}
505527
child_ctx->write_to_child = pipefd_to_child[1];
506528

507529
close(pipefd_from_child[1]);
508-
set_fd_nonblocking(pipefd_from_child[0]);
530+
pipefd_from_child[1] = -1;
531+
ret = set_fd_nonblocking(pipefd_from_child[0]);
532+
if (ret != 0) {
533+
otpd_log_err(ret, "Failed to set read pipe non-blocking");
534+
goto done;
535+
}
509536
child_ctx->read_from_child = pipefd_from_child[0];
510537

511538
child_ctx->write_ev = verto_add_io(ctx.vctx, VERTO_EV_FLAG_PERSIST |
@@ -519,6 +546,8 @@ int oauth2(struct otpd_queue_item **item, enum oauth2_state oauth2_state)
519546
otpd_log_err(ret, "Unable to initialize oauth2 writer event");
520547
goto done;
521548
}
549+
/* verto owns the fd now (IO_CLOSE_FD) */
550+
pipefd_to_child[1] = -1;
522551
verto_set_private(child_ctx->write_ev, child_ctx, NULL);
523552

524553
child_ctx->read_ev = verto_add_io(ctx.vctx, VERTO_EV_FLAG_PERSIST |
@@ -529,14 +558,25 @@ int oauth2(struct otpd_queue_item **item, enum oauth2_state oauth2_state)
529558
child_ctx->read_from_child);
530559
if (child_ctx->read_ev == NULL) {
531560
ret = ENOMEM;
532-
otpd_log_err(ret, "Unable to initialize oauth2 writer event");
561+
otpd_log_err(ret, "Unable to initialize oauth2 reader event");
533562
goto done;
534563
}
564+
/* verto owns the fd now (IO_CLOSE_FD) */
565+
pipefd_from_child[0] = -1;
535566
verto_set_private(child_ctx->read_ev, child_ctx, NULL);
536567

537568
child_ctx->child_ev = verto_add_child(ctx.vctx, VERTO_EV_FLAG_NONE,
538569
oauth2_on_child_exit, child_pid);
570+
if (child_ctx->child_ev == NULL) {
571+
ret = ENOMEM;
572+
otpd_log_err(ret, "Unable to initialize oauth2 child event");
573+
verto_del(child_ctx->read_ev);
574+
verto_del(child_ctx->write_ev);
575+
goto done;
576+
}
539577
verto_set_private(child_ctx->child_ev, child_ctx, free_child_ctx);
578+
/* child_ctx is now owned by verto via free_child_ctx */
579+
child_ctx = NULL;
540580

541581
} else { /* error */
542582
ret = errno;
@@ -548,6 +588,16 @@ int oauth2(struct otpd_queue_item **item, enum oauth2_state oauth2_state)
548588
done:
549589
if (ret == 0) {
550590
*item = NULL;
591+
} else {
592+
if (saved_item != NULL) {
593+
free(saved_item->oauth2.device_code_reply);
594+
free(saved_item);
595+
}
596+
if (pipefd_to_child[0] >= 0) close(pipefd_to_child[0]);
597+
if (pipefd_to_child[1] >= 0) close(pipefd_to_child[1]);
598+
if (pipefd_from_child[0] >= 0) close(pipefd_from_child[0]);
599+
if (pipefd_from_child[1] >= 0) close(pipefd_from_child[1]);
600+
free(child_ctx);
551601
}
552602

553603
return ret;

0 commit comments

Comments
 (0)