openssh bugs - Jun 2021 - [Bug 3321] New: Abnormal packet reading when SSH and tcmalloc are used together

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

            Bug ID: 3321
           Summary: Abnormal packet reading when SSH and tcmalloc are used
                    together
           Product: Portable OpenSSH
           Version: 8.6p1
          Hardware: amd64
                OS: Linux
            Status: NEW
          Severity: major
          Priority: P5
         Component: ssh
          Assignee: unassigned-bugs at mindrot.org
          Reporter: kircherlike at outlook.com

tcmalloc is a fast C/C++ memory allocator designed around a fast path
that avoids synchronizing with other threads for most allocations in
the gperftools.

The tcmalloc of gperftools can be found in
https://github.com/gperftools/gperftools. Using tcmalloc replaces the
malloc standard library function of glibc.

In an x86 system, when the memory of tcmalloc is insufficient, the heap
extension obtains the current call stack through libunwind. libunwind
creates a pipe to check whether the address is valid.

In the main function of ssh.c, the pipeline created by saved_av in
xcalloc by using the tcmalloc process is released abnormally by the
closefrom function.

...
#ifndef HAVE_SETPROCTITLE
        /* Prepare for later setproctitle emulation */
        /* Save argv so it isn't clobbered by setproctitle() emulation
*/
        saved_av = xcalloc(ac + 1, sizeof(*saved_av));
        for (i = 0; i < ac; i++)
                saved_av[i] = xstrdup(av[i]);
        saved_av[i] = NULL;
        compat_init_setproctitle(ac, av);
        av = saved_av;
#endif

        seed_rng();

        /*
         * Discard other fds that are hanging around. These can cause
problem
         * with backgrounded ssh processes started by ControlPersist.
         */
        closefrom(STDERR_FILENO + 1);
...

When tcmalloc attempts to read the pipe, it incorrectly reads the
contents of connection_in, resulting in an error in the MAC
verification of the packet.

Therefore, swapping the order of xcalloc and closefrom statements is
probably the best way to be compatible for tcmalloc.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

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

Darren Tucker <dtucker at dtucker.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |3302
                 CC|                            |dtucker at dtucker.net


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=3302
[Bug 3302] Tracking bug for openssh-8.7
-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

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

Darren Tucker <dtucker at dtucker.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #1 from Darren Tucker <dtucker at dtucker.net> ---
The change has gone in
(https://github.com/openssh/openssh-portable/commit/c9f7bba2e6f70b7ac1f5ea190d890cb5162ce127)
and will be in the next major release.  Thanks for the report.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

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

kircher <kircherlike at outlook.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|FIXED                       |---
             Status|RESOLVED                    |REOPENED

--- Comment #2 from kircher <kircherlike at outlook.com> ---
Thank you very much for helping me incorporate the code.

As far as I know, tcmalloc may apply for fds before the main function
in some unknown scenarios. In this case, the problem can be solved only
by deleting closefrom and carefully checking the status of these fds.

Because deleting all handles greater than or equal to 3 is a strong
constraint, do you have a plan to delete closefrom?

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |djm at mindrot.org

--- Comment #3 from Damien Miller <djm at mindrot.org> ---
There's no way we're going to remove closefrom() - it's essential
for
preventing leakage of open fds from parent to child processes,
including fds that might have been inherited from the process that
started sshd.

Maybe fix tcmalloc not to do that?

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

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

Darren Tucker <dtucker at dtucker.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|REOPENED                    |RESOLVED

--- Comment #4 from Darren Tucker <dtucker at dtucker.net> ---
I did, however, add a tcmalloc test configuration:
https://github.com/openssh/openssh-portable/commit/d4b38144c02f3faa5271e5fb35df93507e06f1b4
https://github.com/openssh/openssh-portable/actions/runs/1332985155
so it should catch the common case of future changes causing it to stop
working.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #5 from Damien Miller <djm at mindrot.org> ---
closing bugs resolved before openssh-8.9

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.