Re: [gentoo-portage-dev] [PATCH] process.spawn: validate unshare calls (bug 673900)

[Zac Medico]

On 12/30/18 1:06 AM, Zac Medico wrote:
> On 12/30/18 12:44 AM, Michał Górny wrote:
>> On Sat, 2018-12-29 at 04:49 -0800, Zac Medico wrote:
>>> In order to prevent failed unshare calls from corrupting the state
>>> of an essential process, validate the relevant unshare call in a
>>> short-lived subprocess. An unshare call is considered valid if it
>>> successfully executes in a short-lived subprocess.
>>>
>>> Bug: https://bugs.gentoo.org/673900
>>> Signed-off-by: Zac Medico <zmedico@g.o>
>>> ---
>>>  lib/portage/process.py | 130 +++++++++++++++++++++++++++++++++--------
>>>  1 file changed, 106 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/lib/portage/process.py b/lib/portage/process.py
>>> index ce3e42a8f..4bd6a4192 100644
>>> --- a/lib/portage/process.py
>>> +++ b/lib/portage/process.py
>>> @@ -6,6 +6,7 @@
>>>  import atexit
>>>  import errno
>>>  import fcntl
>>> +import multiprocessing
>>>  import platform
>>>  import signal
>>>  import socket
>>> @@ -338,11 +339,29 @@ def spawn(mycommand, env=None, opt_name=None, fd_pipes=None, returnpid=False,
>>>  		fd_pipes[1] = pw
>>>  		fd_pipes[2] = pw
>>>
>>> -	# This caches the libc library lookup in the current
>>> -	# process, so that it's only done once rather than
>>> +	# This caches the libc library lookup and _unshare_validator results
>>> +	# in the current process, so that it's only done once rather than
>>>  	# for each child process.
>>> +	unshare_flags = 0
>>>  	if unshare_net or unshare_ipc or unshare_mount or unshare_pid:
>>> -		find_library("c")
>>> +		# from /usr/include/bits/sched.h
>>> +		CLONE_NEWNS = 0x00020000
>>> +		CLONE_NEWIPC = 0x08000000
>>> +		CLONE_NEWPID = 0x20000000
>>> +		CLONE_NEWNET = 0x40000000
>>> +
>>> +		if unshare_net:
>>> +			unshare_flags |= CLONE_NEWNET
>>> +		if unshare_ipc:
>>> +			unshare_flags |= CLONE_NEWIPC
>>> +		if unshare_mount:
>>> +			# NEWNS = mount namespace
>>> +			unshare_flags |= CLONE_NEWNS
>>> +		if unshare_pid:
>>> +			# we also need mount namespace for slave /proc
>>> +			unshare_flags |= CLONE_NEWPID | CLONE_NEWNS
>>> +
>>> +		_unshare_validator.instance.validate(unshare_flags)
>>>
>>>  	# Force instantiation of portage.data.userpriv_groups before the
>>>  	# fork, so that the result is cached in the main process.
>>> @@ -358,7 +377,7 @@ def spawn(mycommand, env=None, opt_name=None, fd_pipes=None, returnpid=False,
>>>  				_exec(binary, mycommand, opt_name, fd_pipes,
>>>  					env, gid, groups, uid, umask, cwd, pre_exec, close_fds,
>>>  					unshare_net, unshare_ipc, unshare_mount, unshare_pid,
>>> -					cgroup)
>>> +					unshare_flags, cgroup)
>>>  			except SystemExit:
>>>  				raise
>>>  			except Exception as e:
>>> @@ -430,7 +449,7 @@ def spawn(mycommand, env=None, opt_name=None, fd_pipes=None, returnpid=False,
>>>  def _exec(binary, mycommand, opt_name, fd_pipes,
>>>  	env, gid, groups, uid, umask, cwd,
>>>  	pre_exec, close_fds, unshare_net, unshare_ipc, unshare_mount, unshare_pid,
>>> -	cgroup):
>>> +	unshare_flags, cgroup):
>>>
>>>  	"""
>>>  	Execute a given binary with options
>>> @@ -466,6 +485,8 @@ def _exec(binary, mycommand, opt_name, fd_pipes,
>>>  	@type unshare_mount: Boolean
>>>  	@param unshare_pid: If True, PID ns will be unshared from the spawned process
>>>  	@type unshare_pid: Boolean
>>> +	@param unshare_flags: Flags for the unshare(2) function
>>> +	@type unshare_flags: Integer
>>>  	@param cgroup: CGroup path to bind the process to
>>>  	@type cgroup: String
>>>  	@rtype: None
>>> @@ -527,26 +548,14 @@ def _exec(binary, mycommand, opt_name, fd_pipes,
>>>  		if filename is not None:
>>>  			libc = LoadLibrary(filename)
>>>  			if libc is not None:
>>> -				# from /usr/include/bits/sched.h
>>> -				CLONE_NEWNS = 0x00020000
>>> -				CLONE_NEWIPC = 0x08000000
>>> -				CLONE_NEWPID = 0x20000000
>>> -				CLONE_NEWNET = 0x40000000
>>> -
>>> -				flags = 0
>>> -				if unshare_net:
>>> -					flags |= CLONE_NEWNET
>>> -				if unshare_ipc:
>>> -					flags |= CLONE_NEWIPC
>>> -				if unshare_mount:
>>> -					# NEWNS = mount namespace
>>> -					flags |= CLONE_NEWNS
>>> -				if unshare_pid:
>>> -					# we also need mount namespace for slave /proc
>>> -					flags |= CLONE_NEWPID | CLONE_NEWNS
>>> -
>>>  				try:
>>> -					if libc.unshare(flags) != 0:
>>> +					errno_value = _unshare_validator.instance.validate(unshare_flags)
>>> +					if errno_value != 0:
>>> +						writemsg("Unable to unshare: %s\n" % (
>>> +							errno.errorcode.get(errno_value, '?')),
>>> +							noiselevel=-1)
>>> +						raise AttributeError('unshare')
>>> +					if libc.unshare(unshare_flags) != 0:
>>>  						writemsg("Unable to unshare: %s\n" % (
>>>  							errno.errorcode.get(ctypes.get_errno(), '?')),
>>>  							noiselevel=-1)
>>> @@ -626,6 +635,79 @@ def _exec(binary, mycommand, opt_name, fd_pipes,
>>>  	# And switch to the new process.
>>>  	os.execve(binary, myargs, env)
>>>
>>> +
>>> +class _unshare_validator(object):
>>> +	"""
>>> +	In order to prevent failed unshare calls from corrupting the state
>>> +	of an essential process, validate the relevant unshare call in a
>>> +	short-lived subprocess. An unshare call is considered valid if it
>>> +	successfully executes in a short-lived subprocess.
>>> +	"""
>>> +
>>> +	instance = None
>>> +
>>> +	def __init__(self):
>>> +		self._results = {}
>>> +
>>> +	def validate(self, flags):
>>> +		"""
>>> +		Validate unshare with the given flags. Results are cached.
>>> +
>>> +		@rtype: int
>>> +		@returns: errno value, or 0 if no error occurred.
>>> +		"""
>>> +
>>> +		if flags == 0:
>>> +			return 0
>>> +
>>> +		result = self._results.get(flags)
>>> +		if result is not None:
>>> +			return result
>>> +
>>> +		filename = find_library("c")
>>> +		if filename is None:
>>> +			return errno.ENOTSUP
>>> +
>>> +		libc = LoadLibrary(filename)
>>> +		if libc is None:
>>> +			return errno.ENOTSUP
>>> +
>>> +		parent_pipe, subproc_pipe = multiprocessing.Pipe(duplex=False)
>>> +
>>> +		proc = multiprocessing.Process(
>>> +			target=self._validator_subproc,
>>> +			args=(libc.unshare, flags, subproc_pipe))
>>> +		proc.start()
>>> +		subproc_pipe.close()
>>> +
>>> +		result = parent_pipe.recv()
>>> +		parent_pipe.close()
>>> +		proc.join()
>>> +
>>> +		self._results[flags] = result
>>> +		return result
>>> +
>>> +	def _validator_subproc(self, unshare, flags, subproc_pipe):
>>> +		"""
>>> +		Perform validation, and send results to parent process.
>>> +
>>> +		@param unshare: unshare function
>>> +		@type unshare: callable
>>> +		@param flags: unshare flags
>>> +		@type flags: int
>>> +		@param subproc_pipe: connection to parent process
>>> +		@type subproc_pipe: multiprocessing.Connection
>>> +		"""
>>> +		if unshare(flags) != 0:
>>> +			subproc_pipe.send(ctypes.get_errno())
>>> +		else:
>>> +			subproc_pipe.send(0)
>>> +		subproc_pipe.close()
>>> +
>>> +
>>> +_unshare_validator.instance = _unshare_validator()
>>> +
>>> +
>>>  def _setup_pipes(fd_pipes, close_fds=True, inheritable=None):
>>>  	"""Setup pipes for a forked process.
>>>
>>
>> Can't we detect the failure in normally spawned ebuild process,
>> and restart it with unsharing disabled?  Possibly cache the result to
>> avoid having to restart everything afterwards.
>
> We'd need an IPC mechanism to communicate the failure to the parent
> process so that it could retry. However, _exec is intended to exec an
> arbitrary child process, so it's not an appropriate place for IPC.

We *could* add IPC to _exec, but that would add IPC overhead to every
single _exec call, which would not be optimal.
-- 
Thanks,
Zac