[autobackport: sssd-2-9] pam: fix out-of-bounds read in pam_passkey_child_read_data#8623
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the PAM passkey responder by adding validation for input buffers and implementing proper error handling for memory allocation and function returns. The changes replace snprintf with memcpy to handle data more safely. Review feedback indicates that the manual allocation and copying of the buffer are unnecessary, as the target function sss_authtok_set_passkey_reply can accept the buffer and length directly to perform its own internal copy.
| str = malloc(buf_len + 1); | ||
| if (str == NULL) { | ||
| tevent_req_error(req, ENOMEM); | ||
| return; | ||
| } | ||
|
|
||
| sss_authtok_set_passkey_reply(state->pd->authtok, str, 0); | ||
| memcpy(str, buf, buf_len); | ||
| str[buf_len] = '\0'; | ||
|
|
||
| ret = sss_authtok_set_passkey_reply(state->pd->authtok, str, 0); | ||
| free(str); | ||
| if (ret != EOK) { | ||
| tevent_req_error(req, ret); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The manual allocation, copying, and NUL-termination of the buffer are unnecessary. The sss_authtok_set_passkey_reply function already creates its own internal NUL-terminated copy. By passing the length buf_len directly, the function safely reads only the available data and handles any trailing NUL characters correctly. This simplification avoids manual memory management and an extra copy of the data.
ret = sss_authtok_set_passkey_reply(state->pd->authtok, (const char *)buf, (size_t)buf_len);
if (ret != EOK) {
tevent_req_error(req, ret);
return;
}
There was a problem hiding this comment.
Hmm...
@xuraoqing, @sumit-bose, @ikerexxe, looks like a good idea, imo...
There was a problem hiding this comment.
Hmm... @xuraoqing, @sumit-bose, @ikerexxe, looks like a good idea, imo...
That is indeed the case. However, according to the description of the function in src/util/authtok.h, the str parameter needs to be passed as a string, so it is generally considered to be a NULL-terminated string.
There was a problem hiding this comment.
Hi,
sss_authtok_set_string() calls sss_authtok_set_string() which checks id there is a \0 at the end or not and adds one if needed. So I think it is save to use sss_authtok_set_string().
bye,
Sumit
There was a problem hiding this comment.
according to the description of the function in src/util/authtok.h, the str parameter needs to be passed as a string
I think, len parameter is telling enough.
|
C9S build fails due to missing "gdm-choice-list-pam-extension.h", this is an issue in GDM devel package content: |
sumit-bose
left a comment
sumit-bose
left a comment
There was a problem hiding this comment.
Hi,
backport plus cherry-pick match what is in main branch, ACK.
bye,
Sumit
The pam_passkey_child_read_data() function failed to properly handle raw bytes received from a pipe. The data was treated as a NUL-terminated C string without explicit termination, resulting in an out-of-bounds read when processed by snprintf() with %s format. Fix by using memcpy instead of snprintf and explicitly NUL-terminating the buffer. Add checks for buf_len == 0 or buf == NULL to avoid undefined behavior. Check the return value of sss_authtok_set_passkey_reply and propagate errors properly. Fixes: CVE-2026-6245 :relnote: Security fix for CVE-2026-6245: out-of-bounds read in PAM passkey responder Reviewed-by: Alexey Tikhonov <[email protected]> Reviewed-by: Iker Pedrosa <[email protected]> Reviewed-by: Sumit Bose <[email protected]> (cherry picked from commit 550b08c)
`sss_authtok_set_passkey_reply()` -> `sss_authtok_set_string()` handles non NULL-terminated buffer correctly. Reviewed-by: Iker Pedrosa <[email protected]> Reviewed-by: Sumit Bose <[email protected]> (cherry picked from commit 3b0b16e)
|
The pull request was accepted by @alexey-tikhonov with the following PR CI status: 🟢 CodeQL (success) There are unsuccessful or unfinished checks. Make sure that the failures are not related to this pull request before merging. |
8a2556f to
50436ac
Compare
5 participants
This is an automatic backport of PR#8622 pam: fix out-of-bounds read in pam_passkey_child_read_data to branch sssd-2-9, created by @xuraoqing.
Please make sure this backport is correct.
Note
The commits were cherry-picked without conflicts.
You can push changes to this pull request
Original commits
550b08c - pam: fix out-of-bounds read in pam_passkey_child_read_data
Backported commits
Original Pull Request Body
The pam_passkey_child_read_data() function failed to properly handle raw bytes received from a pipe. The data was treated as a NUL-terminated C string without explicit termination, resulting in an out-of-bounds read when processed by snprintf() with %s format.
Fix by using memcpy instead of snprintf and explicitly NUL-terminating the buffer. Also fix the error handling to return ENOMEM on allocation failure.
Fixes: CVE-2026-6245