Bug 3321 - Abnormal packet reading when SSH and tcmalloc are used together
Summary: Abnormal packet reading when SSH and tcmalloc are used together
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: ssh (show other bugs)
Version: 8.6p1
Hardware: amd64 Linux
: P5 major
Assignee: Assigned to nobody
URL:
Keywords:
Depends on:
Blocks: V_8_7
  Show dependency treegraph
 
Reported: 2021-06-18 18:08 AEST by kircher
Modified: 2022-02-25 13:58 AEDT (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description kircher 2021-06-18 18:08:57 AEST
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.
Comment 1 Darren Tucker 2021-06-25 15:24:26 AEST
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.
Comment 2 kircher 2021-07-01 19:48:20 AEST
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?
Comment 3 Damien Miller 2021-10-13 10:15:29 AEDT
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?
Comment 4 Darren Tucker 2021-10-13 12:06:06 AEDT
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.
Comment 5 Damien Miller 2022-02-25 13:58:00 AEDT
closing bugs resolved before openssh-8.9