Created attachment 1641 [details] Fix for memory leak While debugging a GSSAPI memory allocation problem not related to OpenSSH, I found a memory leak in OpenSSH when storing forwarded GSSAPI credentials resulting in a growing process segment for each connection that uses GSSAPI credentials forwarding. What happens is the following: In the privileged parent, we are calling ssh_gssapi_storecreds() which itself calls ssh_gssapi_krb5_storecreds(). ssh_gssapi_krb5_storecreds() makes some memory allocations in order to save the credentials store for the gssapi client. +167 client->store.filename = xstrdup(krb5_cc_get_name(krb_context, ccache)); +168 client->store.envvar = "KRB5CCNAME"; +169 len = strlen(client->store.filename) + 6; +170 client->store.envval = xmalloc(len); +171 snprintf(client->store.envval, len, "FILE:%s", client->store.filename); Those memory allocations are never freed. Moreover, since those memory allocations are done in the privileged parent (which is a finite-state machine and never returns) before forking the unprivileged child, the memory leak gets doubled for each connection that uses GSSAPI credential forwarding. A solution would be the following: 1) Migrate the ssh_gssapi_storecreds() call to the unprivileged child 2) Create a ssh_gssapi_free_store() call in gss-serv.c which frees the memory allocations. At first I was thinking of integrating this in the ssh_gssapi_cleanup_creds() call but freeing the memory is mandatory while the cleanup of credentials is the user's choice. 3) Integrate ssh_gssapi_free_store() call in the do_cleanup() call, which is located in session.c. I added a patch which solved this issue.
As noted on the mailing this, this fix is wrong ... GSSAPI credentials need to be stored before the PAM stack is invoked (this also means that the credentials need to be stored in the process which invokes pam_setcred, and not in the unprivileged child). Also, credentials need to be stored whether the user is running privsep or not - this change moves credential storage to a privsep only code path. An alternative fix, that doesn't move the location of the storecreds() call, is going to be required. One option would be to dispose of these structures in the parent as soon as the child is forked (if we're running privsep), so removing the leak in the parent, and tidying up the leak in the child in the manner in the attachment.
Sorry for the mistake. I made some modifications which now dispose the store after the fork operation in the parent. I also added a modification so that the memory is freed even if we're not running privsep.
Created attachment 1642 [details] F
Created attachment 1643 [details] Fix for memory leak (2) Corrected a few mistakes pointed out by Simon
Created attachment 1644 [details] Fix for memory leak (3) ssh_gssapi_free_store() would get called twice in case of privsep. Once from do_cleanup() and once from main(). This fixes this.
Hi Simon Have you already had a chance to have a look at this? Thanks! Miguel