Code Review

This pull request improves memory management and data handling in the PAM passkey responder by ensuring proper buffer allocation and null termination. It also introduces error reporting for memory allocation failures. Feedback suggests adding a safety check for the buffer length before performing a memory copy to avoid potential undefined behavior.

Review done using Claude Code / claude-opus-4-6

Functional Issues

1. Undefined behavior when buf_len is 0 and buf is NULL (src/responder/pam/pamsrv_passkey.c:823)

   read_pipe_recv can return EOK with buf == NULL and buf_len == 0 when the child process closes the pipe immediately (EOF on first read — see _read_pipe_done in src/util/child_io.c:292-295 where tevent_req_done is called without accumulating any data, leaving state->buf == NULL and state->len == 0).

   In that case, malloc(0 + 1) succeeds, but memcpy(str, NULL, 0) is undefined behavior per C11 §7.1.4 ("If an argument to a function has an invalid value... the behavior is undefined"), even though the count is zero. Add a guard:

   if (buf_len == 0 || buf == NULL) {
       tevent_req_error(req, EINVAL);
       return;
   }

2. Return value of sss_authtok_set_passkey_reply is not checked (src/responder/pam/pamsrv_passkey.c:826)

   sss_authtok_set_passkey_reply calls sss_authtok_set_string, which can fail with EINVAL (e.g. zero-length string after NUL stripping) or ENOMEM. The return value is silently discarded, and tevent_req_done(req) is called unconditionally at line 830, signaling success to the caller even when the authtok was not set. This is pre-existing, but since this patch is already improving error handling in this function (adding ENOMEM propagation on malloc failure), the same treatment should apply here:

   ret = sss_authtok_set_passkey_reply(state->pd->authtok, str, 0);
   free(str);
   if (ret != EOK) {
       tevent_req_error(req, ret);
       return;
   }

3. Missing :relnote: tag

   This commit fixes CVE-2026-6245 (an out-of-bounds read reachable via the PAM passkey flow). Security fixes are user-visible and should carry a :relnote: entry in the commit message so they appear in release notes.

Nits & Non-functional Issues

1. malloc/free instead of talloc (src/responder/pam/pamsrv_passkey.c:817,828)

   The rest of this function (and SSSD in general) uses talloc for memory management. The str buffer is the only allocation in this function using libc malloc. Using talloc_size(state, buf_len + 1) would be more consistent, would allow talloc's leak reporting to track it, and would eliminate the need for the explicit free(str) call (the buffer would be freed when state is freed). Pre-existing, but worth aligning while the code is being touched.

2. Missing FD_CLOSE for read_from_child_fd (src/responder/pam/pamsrv_passkey.c:830)

   Compare with the p11 child handler in src/responder/pam/pamsrv_p11.c:949, which calls FD_CLOSE(state->io->read_from_child_fd) after read_pipe_recv. This function does not close the read fd, leaving it to be cleaned up implicitly. Pre-existing, but inconsistent with the p11 pattern.

Confirmed Issues from Existing Review Comments

1. gemini-code-assist[bot] flagged the missing buf_len > 0 check before memcpy (src/responder/pam/pamsrv_passkey.c:823). This is valid and maps to functional issue #1 above — it is not merely a defensive coding preference but addresses real undefined behavior when buf is NULL on immediate EOF.

Hi,

thank you for the fix for this important issue. I find to usage of malloc()/free() a bit odd here as well, but since it is coming from the original code it should not stop the fix from being committed, ACK.

bye,
Sumit

LGTM! Thank you for the patch and for taking the feedback into account.

@alexey-tikhonov I see system test for fedora 44 failing, but they don't seem related to this change. Should we retrigger CI before merging?