openssh bugs - May 2009 - [Bug 1601] New: Memory leak caused by forwarded GSSAPI credential store

https://bugzilla.mindrot.org/show_bug.cgi?id=1601

           Summary: Memory leak caused by forwarded GSSAPI credential
                    store
           Product: Portable OpenSSH
           Version: 5.2p1
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: normal
          Priority: P2
         Component: sshd
        AssignedTo: unassigned-bugs at mindrot.org
        ReportedBy: miguel.sanders at arcelormittal.com
                CC: miguel.sanders at arcelormittal.com


Created an attachment (id=1641)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=1641)
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.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1601


Simon Wilkinson <simon at sxw.org.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |simon at sxw.org.uk




--- Comment #1 from Simon Wilkinson <simon at sxw.org.uk>  2009-05-23
21:52:04 ---
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.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1601





--- Comment #2 from Miguel Sanders <miguel.sanders at arcelormittal.com> 
2009-05-23 23:01:01 ---
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.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1601





--- Comment #3 from Miguel Sanders <miguel.sanders at arcelormittal.com> 
2009-05-23 23:01:58 ---
Created an attachment (id=1642)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=1642)
F

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1601


Miguel Sanders <miguel.sanders at arcelormittal.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1641|0                           |1
        is obsolete|                            |
   Attachment #1642|0                           |1
        is obsolete|                            |




--- Comment #4 from Miguel Sanders <miguel.sanders at arcelormittal.com> 
2009-05-23 23:03:39 ---
Created an attachment (id=1643)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=1643)
Fix for memory leak (2)

Corrected a few mistakes pointed out by Simon

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1601


Simon Wilkinson <simon at sxw.org.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1642|application/octet-stream    |text/plain
          mime type|                            |
   Attachment #1642|0                           |1
           is patch|                            |
   Attachment #1642|1                           |0
        is obsolete|                            |




-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1601


Miguel Sanders <miguel.sanders at arcelormittal.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1642|0                           |1
        is obsolete|                            |
   Attachment #1643|0                           |1
        is obsolete|                            |




--- Comment #5 from Miguel Sanders <miguel.sanders at arcelormittal.com> 
2009-05-23 23:40:45 ---
Created an attachment (id=1644)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=1644)
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.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1601

--- Comment #6 from Miguel Sanders <miguel.sanders at arcelormittal.com>
2010-02-06 22:17:09 EST ---
Hi Simon

Have you already had a chance to have a look at this?

Thanks!

Miguel

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.