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

NFS & similar filesystem types which require e.g. RPC are the only
"corner-case" I am aware of. A native mount(2) would require

| chdir(new_chroot);
| chrootChdir("/dev/pts");
| mount("none", ".", "devpts", "gid=5,mode=620");

or

| chroot(new_chroot);
| mount("none", "/dev/pts", "devpts", "gid=5,mode=620");

while mount(8) requires significantly more written and executed code
(think of cmdline escaping, loading the 9 libraries /bin/mount is linked
against, ...).


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

The comment above was about 'rm -rf'. After a quick look into its code,
I really can not tell whether it is implemented race-free. E.g. whether

   rm -rf chroot                       attacker
|  chdir("chroot");
|  lstat("bin");  --> is-dir
|                                      rmdir("bin");
|                                      symlink("/etc", "bin");
|  chdir("bin");
|  remove-all-what-I-can-find

will be prevented.


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

do it in a safe manner...

| chdir(rootdir);
| chrootChdir("/dev/pts");
| mount("none", ".", "devpts", "gid=5,mode=620");

with

| chrootChdir(char const *dir)
| {
|   int root_fd = open("/", O_RDONLY);
|   chroot(".");
|   chdir(dir);
|   int tgt_fd  = open(".", O_RDONLY);
|   fchdir(root_fd);
|   chroot(".");
|   fchdir(tgt_fd);
|   close(tgt_fd);
|   close(root_fd);
| }

[obviously, error-checking must be added at every line]


>>> +        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)?

As shown above, mount(2) results in significantly less written and
executed code than mount(8). This is resulting both in faster and more
secure code. And I really do not know how people came to the idea to
use system(3) at this place. Even prototyping languages like perl or
python know execve(2) equivalences.


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

Sorry; my comment was not about the mkdir stuff with the (IMO unneeded)
test-if-exists. It just stepped in more or less inadvertently.


> Use the install utility?

no; only the underlined part. When 'install' was compiled with (or
without ... I can not remember the exact conditions) AFS support, the
'make install' will fail as non-root user.


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

We are in userspace where we have 8MB stack or so by default... And I
do not think that somebody wants to implement mock-helper as a kernel
module with only 4KB 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?

We are speaking about SUID programs. Every additional and unneeded line
of code increases complexity and complicates an audit. Your original
code showed that the unneeded memory-allocating is error-prone too.


> It's done once then and then we exec python.

Why write complicated code, when same thing can be done shorter, faster
and less error-prone?




Enrico
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 480 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/fedora-buildsys-list/attachments/20060623/b5552171/attachment.sig>