Skip to content

Only free allocated memory on UnixWare#284

Closed
stoeckmann wants to merge 1 commit into
openssh:masterfrom
stoeckmann:libaif
Closed

Only free allocated memory on UnixWare#284
stoeckmann wants to merge 1 commit into
openssh:masterfrom
stoeckmann:libaif

Conversation

@stoeckmann

Copy link
Copy Markdown

If libaif is compiled in together with shadow support, then a
misconfigured system could trigger invalid free calls:

The "spw" variable can be NULL even though a password entry
exists. This can be reproduced on systems with shadow support if
/etc/shadow lacks a user entry which exists in /etc/passwd: Thus
it requires a misconfigured system to trigger this bug.

If "spw" is NULL then get_iaf_password is never called, which
means that "passwd" still points to memory within the pw struct.

Shoutout to @c3h2_ctf

If libaif is compiled in together with shadow support, then a
misconfigured system could trigger invalid free calls:

The "spw" variable can be NULL even though a password entry
exists. This can be reproduced on systems with shadow support if
/etc/shadow lacks a user entry which exists in /etc/passwd: Thus
it requires a misconfigured system to trigger this bug.

If "spw" is NULL then get_iaf_password is never called, which
means that "passwd" still points to memory within the pw struct.
Comment thread auth.c
#ifdef USE_LIBIAF
free((void *) passwd);
if (spw != NULL)
free((void *) passwd);

@daztucker daztucker Oct 22, 2021

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.

I agree with your analysis and that this fixes it, but the whole section of code is not very clear and this makes it less so. Instead, I'd suggest keeping another copy of the pointer and using that which would make it clearer what's being freed and why, ie

char *iaf_passwd = NULL;
[...]          
iaf_passwd = passwd = get_iaf_password(pw);
[...]
free((void *) iaf_passwd(iaf_passwd);  /* maybe zero it too */

but I think all of the locked account check could be factored out into its own function in platform.c, which would clean up auth.c quite a bit.

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.

It'd look like this (untested): daztucker@b964748

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.

Looks definitely much better and removes this nested portability handling out of auth.c. Definitely agreeing on that proposal.

@daztucker

Copy link
Copy Markdown
Contributor

I've committed the proposed patch as 2923d02. Thanks for the report.

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