Re: ideas on interface (was Be careful please)

dawat_private

Chris Wright  wrote:
>See attached "header" file for a rough sketch of an interface that
>we'd like to propose/discuss.

Ok, you asked for it! :-)  Here are some detailed comments.
But first: Let me thank you for putting up a concrete proposal.

A general note: These hooks seem narrowly tailored towards
allowing one to implement the existing capabilities policy.
This is a good starting point, but I would be very disappointed
if this is also the ending point.

Also, can you tell me about the semantics of these hooks?  If
I see `int (* foo)(void);', what does the return value mean?
Is the hook called *instead* of some basic operation?  Is it
called before, and a -EPERM ensures the request will be denied?

>	int  (* mount)			(void);		// part of CAP_SYS_ADMIN
>	int  (* ioperm)			(void);		// part of CAP_RAWIO
>	int  (* iopl)			(void);		// part of CAP_RAWIO
>	int  (* ptrace)			(void);		// CAP_PTRACE
>	int  (* setcapablity)	(void);		// CAP_SETPCAP
>	int (* setuid)			(void);		// CAP_SETUID
>	int (* setgid)			(void);		// CAP_SETGID
>	int (* setpriority)		(void);		// CAP_NICE
>	int (* kill)			(void);		// CAP_KILL

This is massively limiting.  All you can do is allow or deny all
mount(2) operations, for instance.  How about some arguments?
I can't overstate the importance of this limitation.  Really.

>	int  (* sethostname)		(void);
>	int  (* setdomainname)		(void);

I suggest allowing the module to make its decision based on the
value of the new hostname.

>	int  (* reboot)			(void);		// CAP_SYS_BOOT

Does this correspond to the syscall reboot(2), or to the action
of rebooting the system?  If the former, note that reboot(2) is
used for things other than rebooting the system, and in this case
you should pass its argument to the module.  But I suggest making
the semantics be the latter (and change the name of the hook slightly
to make the semantics more obvious?).

>	/* cmw: this should allow you to know if two permissions
>	 * stuctures are the same, subset, or superset
>	 * returns 0 when same, 1 when perm1 is superset of perm2 and
>	 * -1 when perm1 is subset of perm2...hmmm what if they have no
>	 * relation?  need better type than void* ;-/
>	 * at least subset is needed for capabilities
>	 */
>	int (*perm_comp)		(void * perm1, void * perm2)
>	void (*init_perm)		(void * perm);
>	int (*change_perm)		(void * perm);

What are you envisioning here?  I recommend treading carefully
here.  I've yet to run into anything in the kernel that would
need this functionality, and I've seen implementation experience
suggesting that this can cause lots of trouble when people start
implementing policies that don't fit the mental model of the
people using perm_comp(), etc.  Did you have some particular
problem in mind that this is trying to solve?

Is this a burden imposed on module writers?  How are they going to
be able to implement this?  I mean, all you've got is function pointers,
and identifying whether two different procedures compute the same thing
is notoriously hard...

>struct socket_security_ops {
>};

Did you mean for this to be empty?  What about all of the socket ops:
bind, connect, and so forth?

>extern int register_security	(struct security_operations *ops);
>extern int unregister_security	(struct security_operations *ops);

Don't you mean the following?

extern int register_security(task_struct *p, struct security_operations *ops);
extern int unregister_security(task_struct *p, struct security_operations *ops);

>/* global variables */
>extern struct security_operations *security_ops;

What's going on here?  What's this global variable doing?

I have many more comments, but I'll avoid overloading you in this
email.

_______________________________________________
linux-security-module mailing list
linux-security-moduleat_private
http://mail.wirex.com/mailman/listinfo/linux-security-module