New version of mock working (I think)

[Enrico Scholz]

williams at redhat.com (Clark Williams) writes:

>          if os.path.exists(self.basedir):
>              cmd = '%s -rf %s' % (self.config['rm'], self.basedir)

wouldn't it be better to do such things with native syscalls instead of
invoking a command? You will never get this implemented race-free with
shell commands.


>  
>      def pack(self):
>          self.state('create cache')
>          self._ensure_dir(os.path.join(self.config['basedir'], self.config['cache_topdir']))

This seems to open an attack vector; attacker could create the
cache_topdir (basedir is writable by mock group) and create e.g. a
cache.tar -> /etc/nologin symlink.

Perhaps it suffices to remove group write-permissions from basedir. The
cleaner way would be to open the cache_file in a safe way and give only
the fd to tar. (ditto for unpack).


>          self.debug("mounting proc in %s" % procdir)
> -        command = '%s -t proc proc %s/proc' % (self.config['mount'], 
> +        command = '%s -n -t proc proc %s/proc' % (self.config['mount'], 
>                                                 self.rootdir)

See below...

>          track.flush()
>          
>          if retval != 0:
> @@ -427,9 +459,9 @@
>          devptsdir = os.path.join(self.rootdir, 'dev/pts')

When you are about to increase security, you should fix such beginner
errors too. 'chroot' does not mean "prepend a path", but "change /".


>          self._ensure_dir(devptsdir)
>          self.debug("mounting devpts in %s" % devptsdir)
> -        command = '%s -t devpts devpts %s' % (self.config['mount'], devptsdir)
> +        command = '%s -n -t devpts devpts %s' % (self.config['mount'], devptsdir)

There is no reason to do this kind of mounting with the mount(8) shell
command. mount(2) is a much better (and easier) way.


>          item = '%s/%s' % (self.rootdir, path)
>          command = '%s %s' % (self.config['umount'], item)

There is no reason for explicit unmounting. At least MNT_DETACH should
be set at least. Your (deprecated) shell command should get a '-n' too.


> +	[ -d $(DESTDIR)/$(BINDIR) ] || \
> +		$(MKDIR) -p $(DESTDIR)/$(BINDIR)
> +	[ -d $(DESTDIR)/$(LIBDIR) ] || \
> +		$(MKDIR) -p $(DESTDIR)/$(LIBDIR)
> +	$(INSTALL) -o root -g $(MOCKGROUP) -m 4750 $(EXECUTABLE) $(DESTDIR)/$(BINDIR)
                   ~~~~~~~~~~~~~~~~~~~~~~~

please do not do such things. rpm packagers will hate you for that.


> 	memset(env, '\0', sizeof(char *) * (3 + ALLOWED_ENV_SIZE));
> 	env[0] = strdup(SAFE_PATH);

plain assignment without strdup(3) suffices here. You could write

| char const *     env[ALLOWED_ENV_SIZE] = {
|   [0] = SAFE_PATH,
|   [1] = SAFE_HOME,
|   [2] = NSFLAG
| };

here.

> 	// set up a new argv/argc
> 	//     new argv[0] will be "/usr/bin/python"
> 	//     new argv[1] will be "/usr/bin/mock.py"
> 	//     remainder of new argv will be old argv[1:n]
> 	//     allocate one extra for null at end
> 	newargc = argc + 1;
> 	newargvsz = sizeof(char *) * (newargc + 1);
> 	newargv = malloc(newargvsz);

why are you doing dynamic memory allocations here? Operating on stack is
much cheaper.


> 	if (newargv == NULL)
> 		error("malloc of argument vector (%d bytes) failed!\n", newargvsz);
> 	memset(newargv, '\0', newargvsz);
> 	newargv[0] = strdup(PYTHON_PATH);

ditto; you are missing an error check here btw... (which would not be
needed by a simple 'newargv[0] = PYTHON_PATH;').



Enrico