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

[Zac Medico]

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.
-- 
Thanks,
Zac