Skip to content

Fix PAM response allocation to handle invalid message count#281

Open
netliomax25-code wants to merge 1 commit intodovecot:mainfrom
netliomax25-code:pam-calloc-overflow-guard
Open

Fix PAM response allocation to handle invalid message count#281
netliomax25-code wants to merge 1 commit intodovecot:mainfrom
netliomax25-code:pam-calloc-overflow-guard

Conversation

@netliomax25-code
Copy link
Copy Markdown

pam_userpass_conv() allocates the response array using:

resp = calloc(num_msg, sizeof(struct pam_response));

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.

Comment thread src/auth/passdb-pam.c

resp = calloc(num_msg, sizeof(struct pam_response));
if (num_msg < 0 ||
(size_t)num_msg > SIZE_MAX / sizeof(struct pam_response)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants