New version of mock working (I think)

[Clark Williams]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Enrico Scholz wrote:
> 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.
>
>

Well, as an old-timey UNIX programmer, I was always taught (by even
older UNIX hackers) that you shouldn't use mount(2), that mount(8) is
preferred because all the corner-case logic for dealing with mount
points is encapsulated there; just use it.  Perhaps that isn't the
case anymore and I'm just falling back on old habits. In any case it's
much easier from a code maintenance perspective to just delegate the
responsibility of mounting filesystems to mount(8).

What race condition(s) would we avoid by using mount(2)?

>> 
>>      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).
>
>

With the change in managing uid/gids, I wonder if we even need the
mock group anymore? Currently we cycle between the users's real uid
and root (when a root action is required). I'm not sure we need to
setgid to mock any more.

>>          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 /".

Well, we have to prepend a path, since these mounts are occurring
before we chroot.  What would you suggest as an alternative?

>
>
>>          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.
>

I disagree that it's easier to mount with mount(2). Are you suggesting
that it's less code than the above two lines (format and os.system)?

>
>>          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.
>
>

No reason other than good programming practice perhaps.

Thanks for catching the -n.

>> +    [ -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.
>
>

Please do not do *what* things? Create directories? Use the install
utility?

Please elaborate your concern.

>>     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.
>

Well, I'd say that this is a holdover from kernel programming where
you don't put things other than trivial variables on the stack.
Probably not an issue in user-space, but I'm not going to change it
for a one-time exec. Who cares if it's a couple of micro-seconds
faster to assign to the stack as opposed to allocating a page off the
heap and duping a few strings? It's done once then and then we exec
python.

>
>>     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;').
>

Ah, I should have checked the strdup return.  Busted...



Clark
 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org

iD8DBQFEnAvsHyuj/+TTEp0RAtQ9AKDPOVMS525eT3bKrTYS36fnJADyvACff2xR
k76XCHVvjO4Ass30Sdgrj6s=
=mUTh
-----END PGP SIGNATURE-----