-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Enrico Scholz wrote:
williams@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