Re: [PATCH] kernel: make /proc/kallsyms mode 400 to reduce ease of attacking

[Ingo Molnar <mingo@elte.hu>]

* Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > >  	/* Some debugging symbols have no name.  Ignore them. */
> > > -	if (!iter->name[0])
> > > +	if (!iter->name[0] || !capable(CAP_SYS_ADMIN))
> > >  		return 0;
> 
> This is hardcoding file permission policy into the kernel in a way the
> user cannot change - its bogus in the extreme. Use file permissions that
> way saner people can chmod them as they like. Indeed quite a few people
> *already* chmod chunks of /proc.

Peter already pointed that out and i agree.

The main goal here was to establish that a regression-free patch can be implemented 
by giving user-space a *empty /proc/kallsyms file* - that we older systems do not 
crash on bootup.

> It also means that things like SELinux and Tomoyo can be used to manage security 
> on it in clever ways - something that using a capability completely buggers up.

Frankly, our security interfaces are a mess - i did not even try to figure out the 
'right' way to do it. Modularization of security subsystem made it all distinctly 
worse.

Why dont we have coherent, easy to use (and hard to mess up) security interfaces to 
begin with? The moment a kernel developer has to think of:

        retval = -EPERM;
        if (capable(CAP_SETUID)) {
                new->suid = new->uid = uid;
                if (uid != old->uid) {
                        retval = set_user(new);
                        if (retval < 0)
                                goto error;
                }
        } else if (uid != old->uid && uid != new->suid) {
                goto error;
        }

        new->fsuid = new->euid = uid;

        retval = security_task_fix_setuid(new, old, LSM_SETID_ID);
        if (retval < 0)
                goto error;


As the 'secure' implementation of a piece of kernel logic we have lost the 
'security' battle ...

The current security callbacks are absolutely nonsensical random crap slapped all 
around the kernel. It increases our security complexity and has thus the opposite 
effect - it makes us _less_ secure.

Did no-one think of merging the capabilities checks and the security subsystem 
callbacks in some easy-to-use manner, which makes the default security policy 
apparent at first sight?

This code should be written in a simpler form, something like:

        retval = -EPERM;
	if (!security_allow_task_fix_setuid(new, old)) {
                new->suid = new->uid = uid;
                if (uid != old->uid) {
                        retval = set_user(new);
                        if (retval < 0)
                                goto error;
                }
        } else if (uid != old->uid && uid != new->suid) {
                goto error;
        }

        new->fsuid = new->euid = uid;

Where the default security_allow_task_fix_setuid() is basically a CAP_SETUID check - 
and we know this from the 'security_allow_task_fix_setuid' name already.

This way all those stupid, passive security callbacks become _active participants of 
the code_, and the code becomes more compact and easier to understand - and it 
becomes harder to mess up both compatibility details and permission details.

[ And yes, i realize that this isnt a 100% replacement of the existing callback,
  because some of the default logic cannot be turned off - but heck, that's a 
  feature not a bug! We dont want to allow security modules to make things _less_ 
  secure, or break legacies, right? So they should be shaped as _additional_
  restrictions on the coarse default semantics.

  And dont get me started about the idiocy of LSM_SETID_ID. Why isnt that detail put 
  into the callback name? What's wrong with security_task_fix_setuid_id(new, old)? ]

Whoever allowed security modules to be added in their current form needs some 
talking to.

Thanks,

	Ingo