LSM changes for 2.5.38

/* Set EXTENT bits starting at BASE in BITMAP to value TURN_ON. */ static void set_bitmap(unsigned long *bitmap, short base, short extent, int new_value)

+ * @module_create:
+ * Check the permission before allocating space for a module.
+ * @name contains the module name.
+ * @size contains the module size.
+ * Return 0 if permission is granted.
+ * @module_initialize:
+ * Check permission before initializing a module.
+ * @mod contains a pointer to the module being initialized.
+ * Return 0 if permission is granted.

+ * @sethostname:
+ * Check permission before the hostname is set to @hostname.
+ * @hostname contains the new hostname
+ * Return 0 if permission is granted.
+ * @setdomainname:
+ * Check permission before the domainname is set to @domainname.
+ * @domainname contains the new domainname
+ * Return 0 if permission is granted.

+ * @ioperm:
+ * Check permission before setting port input/output permissions for the
+ * process for @num bytes starting from the port address @from to the
+ * value @turn_on.
+ * @from contains the starting port address.
+ * @num contains the number of bytes starting from @from.

+ * @turn_on contains the permissions value.
+ * Return 0 if permission is granted.
+ * @iopl:
+ * Check permission before changing the I/O privilege level of the current
+ * process from @old to @level.
+ * @old contains the old level.
+ * @level contains the new level.
+ * Return 0 if permission is granted.

+ * @sysctl:
+ * Check permission before accessing the @table sysctl variable in the
+ * manner specified by @op.
+ * @table contains the ctl_table structure for the sysctl variable.
+ * @op contains the operation (001 = search, 002 = write, 004 = read).
+ * Return 0 if permission is granted.

+ * @swapon:
+ * Check permission before enabling swapping to the file or block device
+ * identified by @swap.
+ * @swap contains the swap_info_struct structure for the swap file and device.
+ * Return 0 if permission is granted.
+ * @swapoff:
+ * Check permission before disabling swapping to the file or block device
+ * identified by @swap.
+ * @swap contains the swap_info_struct structure for the swap file and device.
+ * Return 0 if permission is granted.

diff -Nru a/kernel/sys.c b/kernel/sys.c

kernel/sys.c +1 -0

@@ -349,11 +349,17 @@
 asmlinkage long sys_reboot(int magic1, int magic2, unsigned int cmd, void * arg)
 {
 	char buffer[256];
+	int retval;

Quoted 11 lines

/* Set EXTENT bits starting at BASE in BITMAP to value TURN_ON. */ static void set_bitmap(unsigned long *bitmap, short base, short extent, int new_value)

(quoted diff) +3 -1

@@ -62,7 +63,12 @@
 		return -EINVAL;
 	if (turn_on && !capable(CAP_SYS_RAWIO))
 		return -EPERM;
-
+ 
+ 	ret = security_ops->ioperm(from, num, turn_on);
+ 	if (ret) {

Review line 71

Quoted 3 lines

(quoted diff) +3 -0

+ 		return ret;
+ 	}
+ 

Sorry, but this is bullshit (like most of the lsm changes). Either you leave the capable in and say it's enough or you add your random hook and remove that one. Just adding more and more hooks without thinking gets us exactly nowhere except to an unmaintainable codebase.

Also is there a _real_ need to pass in all the arguments?

Review

Quoted 7 lines

(quoted diff) +5 -0

 			return -EPERM;
 	}
+	retval = security_ops->iopl(old, level);
+	if (retval) {
+		return retval;
+	}
+

again (and another few times)

Quoted 9 lines

+ * @module_create:
+ * Check the permission before allocating space for a module.
+ * @name contains the module name.
+ * @size contains the module size.
+ * Return 0 if permission is granted.
+ * @module_initialize:
+ * Check permission before initializing a module.
+ * @mod contains a pointer to the module being initialized.
+ * Return 0 if permission is granted.

Umm, you can't tell me you deny someone to initialize a module he has just created?

Sorry, but this is bullshit (like most of the lsm changes). Either you leave the capable in and say it's enough or you add your random hook and remove that one. Just adding more and more hooks without thinking gets us exactly nowhere except to an unmaintainable codebase.

Also is there a _real_ need to pass in all the arguments?

Umm, you can't tell me you deny someone to initialize a module he has just created?

You don't think this should maybe be just one hook?

Aha. So every LS module knows about every single sysctl in the kernel. Common, this is silly guys (and girls if there any)!

Quoted 2 lines

Also is there a _real_ need to pass in all the arguments?

Define _real_. It is true that none of the existing open source security modules presently use this particular hook. SELinux doesn't presently use it, but it seems reasonable to support finer-grained control over ioperm() than the all-or-nothing CAP_SYS_RAWIO. Is the criteria that every hook and every parameter to every hook must be used by an existing open source security module? If so, then yes, this hook can be dropped.

Quoted 3 lines

leave the capable in and say it's enough or you add your random hook and remove that one. Just adding more and more hooks without thinking gets us exactly nowhere except to an unmaintainable codebase.

capable is needed to be checked, as we are not modifying the existing permission logic.

Quoted 4 lines · Christoph Hellwig

Sorry, but this is bullshit (like most of the lsm changes). Either you leave the capable in and say it's enough or you add your random hook and remove that one. Just adding more and more hooks without thinking gets us exactly nowhere except to an unmaintainable codebase.

The LSM hooks are primarily restrictive, i.e. a security module can deny access that would normally be granted by the existing Linux access checks.

Quoted 1 line

Also is there a _real_ need to pass in all the arguments?

Define _real_.

It is true that none of the existing open source security modules presently use this particular hook. SELinux doesn't presently use it, but it seems reasonable to support finer-grained control over ioperm() than the all-or-nothing CAP_SYS_RAWIO. Is the criteria that every hook and every parameter to every hook must be used by an existing open source security module? If so, then yes, this hook can be dropped.

In sys_create_module, you only know the name and size of the module and who is performing the operation. In sys_init_module, you actually have information about the module available. Hence, you can make a finer-grained decision in the module_initialize hook, and possibly deny even after a successful module_create. As above, SELinux doesn't use these hooks presently.

Quoted 2 lines

capable is needed to be checked, as we are not modifying the existing permission logic.

I odn't think it makes sense to have two security checks that both end up in the LSM code after each other..

For cases like the module_* hooks, and the other examples you pointed out, I agree.

For other cases, capable() is just not fine grained enough to actually know what is going on (like CAP_SYS_ADMIN). In those cases you need an extra hook to determine where in the kernel you are.

Quoted 3 lines · Greg KH

For cases like the module_* hooks, and the other examples you pointed out, I agree.

Quoted 3 lines

For other cases, capable() is just not fine grained enough to actually know what is going on (like CAP_SYS_ADMIN). In those cases you need an extra hook to determine where in the kernel you are.

Either we make capable fine grained enough (64 or 128bit capability vectors, I have some old code for that around and I know SGI used that more than a year ago) or we replace the capable in those cases with hooks entirely.

And WTF is the use a security policy that checks module arguments? Do you want to disallow options that are quotes from books on the index or not political correct enough for a US state agency?

Quoted 3 lines · Christoph Hellwig

And WTF is the use a security policy that checks module arguments? Do you want to disallow options that are quotes from books on the index or not political correct enough for a US state agency?

How about a security policy that says:

1) Thou mayest do an 'modprobe wvlan_cs'

2) Thou mayest not do 'modprobe wvlan_cs eth=0'.

'eth=0' causes it to create the interface as 'wvlan0' 'wvlan1' etc rather than 'eth0', 'eth1', etc. This makes a difference if you have iptables rules that say '-i eth+' or '-i wvlan+' that implement different rulesets for wireless and hard-wired connections.

So I have to download the driver source and change the parameter to i_m_to_fscking_intelligent_for_the_nsa_eth=0?

By the same token, at that point you can download the kernel source and build it without LSM. What I showed was a way to bypass the iptables rules set up *WITHOUT REPLACING A MODULE* (which might be detected by tripwire, or totally refused because the LSM rejects any writes in /lib/modules).

In 2.5 LSM hooks are part of the Linux access checks, and any reason to make your patch less intrusive inb 2.4 doesn't apply anymore. Having two checks for the same thing is indeed very bad in will cause harm and maintaince burden in the long term. Don't do it.

Show me a useful example that needs this argument.

And WTF is the use a security policy that checks module arguments? Do you want to disallow options that are quotes from books on the index or not political correct enough for a US state agency?

Review

Quoted 10 lines

(quoted diff) +6 -1

 	if (turn_on && !capable(CAP_SYS_RAWIO))
 		return -EPERM;
-
+ 
+ 	ret = security_ops->ioperm(from, num, turn_on);
+ 	if (ret) {
+ 		return ret;
+ 	}
+ 

Sorry, but this is bullshit (like most of the lsm changes). Either you leave the capable in and say it's enough or you add your random hook and remove that one.

Just adding more and more hooks without thinking gets us exactly nowhere except to an unmaintainable codebase.

Also is there a _real_ need to pass in all the arguments?

Quoted 9 lines

+ * @module_create:
+ * Check the permission before allocating space for a module.
+ * @name contains the module name.
+ * @size contains the module size.
+ * Return 0 if permission is granted.
+ * @module_initialize:
+ * Check permission before initializing a module.
+ * @mod contains a pointer to the module being initialized.
+ * Return 0 if permission is granted.

Umm, you can't tell me you deny someone to initialize a module he has just created?

Quoted 8 lines

+ * @sethostname:
+ * Check permission before the hostname is set to @hostname.
+ * @hostname contains the new hostname
+ * Return 0 if permission is granted.
+ * @setdomainname:
+ * Check permission before the domainname is set to @domainname.
+ * @domainname contains the new domainname
+ * Return 0 if permission is granted.

You don't think this should maybe be just one hook?

You might be allowed to swapon but not swapoff?

Review

Quoted 4 lines

(quoted diff) +4 -0

+static int cap_swapoff (struct swap_info_struct *swap)
+{
+	return 0;
+}

Live would be a lot simple if an unimplemented op would behave as returning zero..

insmod doesn't require modules to be in /lib/modules.

Given that we really want to fine-grained control who's netdevice can get what names we'd` better place a hook in dev_alloc_name.

And that's my whole point: LSM adds random hooks all over the place without even thinking what they intend to protect.

Quoted 1 line · Christoph Hellwig

insmod doesn't require modules to be in /lib/modules.

This would probably be closed by this code in sys_create_module():

        /* check that we have permission to do this */
        error = security_ops->module_ops->create_module(name, size);
        if (error)
                goto err1;

Quoted 1 line · Christoph Hellwig

insmod doesn't require modules to be in /lib/modules.

This would probably be closed by this code in sys_create_module():

        /* check that we have permission to do this */
        error = security_ops->module_ops->create_module(name, size);
        if (error)
                goto err1;

Similarly, there are other hooks that will stop renaming of the interface (I have to admit I haven't had enough caffeine to verify whether doing a /bin/mv to rename an interface will change the name that's presented to the iptables rulesets), or did you have other "rename" methods in mind?

However, you're missing the point - I was using that as *AN EXAMPLE* of "you might want to check what parameters are being passed". Are you prepared to make the same sort of analysis for *EVERY* parameter of *every* module, and every combination thereof (including interacting parameters of different modules)?

In order to assert that the hook to check parameters in module loading is useless, you'd have to verify that there exist *NO* modules that can have their security status changed by changing the parameters.

And even more importantly,
you'd have to make this a permanent restriction on module parameters, which puts the onus on "getting it right" on the module author, rather than on the LSM author who's in a better position to know what is/isn't acceptable for his module.

And quite a bit of thought *did* go into it - a *LOT* of those hooks got added along the way precisely *because* the writers of a module found that they were trying to enforce a policy, and found it could be backdoored by means like module parameters.

From my reading of the LSM lists a hgook usually got added because author A

So you want to move the 'if (turn_on && !capable(CAP_SYS_RAWIO)) return -EPERM;' from the base kernel into the security module's ioperm() hook function, in addition to whatever additional logic the module may implement? [Assuming for the moment that we kept the ioperm() hook, even though that isn't likely given its current lack of use and architecture-specific nature].

If so, what about the rest of the kernel access checking logic? Do you want all of the permission() logic pushed into the security module's inode_permission() hook function?

Do you want the bad_signal() logic
pushed into the security module's task_kill() hook function?

That kind of
change was considered and discussed on linux-security-module long ago, but it will yield a very invasive patch for very little gain. It also requires cleanly separating all access checking logic from functional logic (it is sometimes fairly intertwined) and determining exactly which is which

(e.g. is enforcing a read-only mount a security behavior or a functional behavior?).

Quoted 2 lines

Show me a useful example that needs this argument.

Do you want every process that can use ioperm() to be able to access the full range of ports accessible by that call? If not, then you need something finer-grained than CAP_SYS_RAWIO. But as I said, we don't presently use this hook, and it is architecture-dependent.

Quoted 4 lines

And WTF is the use a security policy that checks module arguments? Do you want to disallow options that are quotes from books on the index or not political correct enough for a US state agency?

The LSM module_initialize hook is called with a pointer to the kernel's copy of the relocated module image with the struct module header. Hence, the security module is free to perform whatever validation it wants on that image prior to the execution of the init function. But if the criteria is that there must be a specific existing security module that uses the hook, then this one will go away too.

For gods sake, what interface do you want to /bin/mv? network device don't have associated special files in Linux (or BSD). I'm talking about SIOCSIFNAME.

How does the lsm author know what the option x=y means for my module? In my

From my reading of the LSM lists a hgook usually got added because author A of the unaudited module B though that it might help him in imlpementing what he and only he thinks his module should do if it worked properly.

Quoted 2 lines · Christoph Hellwig

For gods sake, what interface do you want to /bin/mv? network device don't have associated special files in Linux (or BSD). I'm talking about SIOCSIFNAME.

Well, I figured that you must have meant something else, since ioctl() is hooked, so an attacker couldn't use THAT method if the security module didn't want him to.

Of course, that means we probably should pull that ioctl() hook too, since obviously there's ways to bypass it.

So we shouldn't deploy a check on module parameters to prevent renaming an interface, and we shouldn't bother having OTHER checks to stop it because they could always load a module and bypass it

- if you feel THAT
strongly that the whole concept of a security framework is that pointless, you're free not to use it. ;)

Quoted 1 line

How does the lsm author know what the option x=y means for my module? In my

Umm.. the same way that *I* noticed that 'eth=0' has a security implication.

It seems to me that you're arguing both sides here - first you say that a full code audit is needed so you know 'WTF is going on', and then you're saying that it's impossible to know.

OK - so to summarize: You're saying that somebody conceives of a reasonable security model, finds a set of 10 or 15 hooks that implement 95% of what he needs, but there's a code path that a hook doesn't catch that lets a user subvert the model - and then it's a *BAD THING* that the kernel be modified to *actually solve a problem*?

Somehow, this goes against the whole spirit of the Linux kernel - I wonder what miniscule percent of the current kernel wasn't done to solve a problem...

Quoted 3 lines

It seems to me that you're arguing both sides here - first you say that a full code audit is needed so you know 'WTF is going on', and then you're saying that it's impossible to know.

The person who performs the audit can know it. But how often will that be the author of the LSM module?

We've said on this list a few times that it is important for security module authors to understand the implications of their decisions. Deciding to not mediate module parameters is a valid decision. Deciding to mediate module parameters is a valid decision. One requires very little thought and sidesteps the matter entirely. The other requires quite a bit of thought and is difficult to get right -- but that is not a problem for LSM, per se; it is for the authors of security modules.