Fix PAM response allocation to handle invalid message count#281
Fix PAM response allocation to handle invalid message count#281netliomax25-code wants to merge 1 commit intodovecot:mainfrom
Conversation
|
|
||
| resp = calloc(num_msg, sizeof(struct pam_response)); | ||
| if (num_msg < 0 || | ||
| (size_t)num_msg > SIZE_MAX / sizeof(struct pam_response)) { |
There was a problem hiding this comment.
calloc() already checks for integer overflows, and returns NULL if there would be an overflow. And Dovecot handles NULL already by failing with out-of-memory. Other large num_msgs are similarly handled as NULL -> out-of-memory. I guess there's the question of whether we should enforce some sane limit, but since PAM is a very trusted source I don't think it makes practically any difference.
There was a problem hiding this comment.
You're right that calloc() already handles overflow and that the NULL
path is properly handled here, so this doesn’t introduce a practical
issue as-is.
Looking at it again, the more relevant concern here would be avoiding
excessive allocations if num_msg is unexpectedly large. Even if PAM is
generally trusted, adding a reasonable upper bound could make this path
more robust and easier to reason about.
I can update this PR to add a limit check instead, or drop the change
if you'd prefer to keep the current behavior.
pam_userpass_conv() allocates the response array using:
The multiplication is not explicitly checked for overflow. Although
num_msg originates from PAM, it is external input and should not be
trusted blindly.
Add a SIZE_MAX guard before allocation and fail gracefully if the value
is invalid.
This is a defensive hardening change and does not affect behavior for
valid inputs.