Re: [RFC][PATCH] Privilege dropping security module

[Andy Spencer <andy753421@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5599 bytes --]

> I think I saw this mentioned elsewhere, but you can't put a
> 4k buffer on the stack.
> ...
> "//" comments are not used in the kernel
> ...
> Why use a value other than PATH_MAX? If it's arbitrary,
> go with the "standard" arbitrary.
> ... 
> Stick with your namespace.

Fixed as suggested, thanks.


> The term "privilege" is generally applied to a broader scope
> than discretionary access controls. You might want to consider
> using a name that is more precisely descriptive of the feature.
> It probably isn't that important, but I for one would be happier.
> 
> You're not dropping privilege, that would imply restricting
> root and/or capability access. You're masking file permissions.

I have no objections to changing the name if you have a better
suggestion. However, I would like to avoid the term "discretionary
access control" since DACs deal with passing permissions between
subjects, while in dpriv, the only subject involved is the current
subject (well, and its children). Also, as pointed out in another
thread, dpriv will eventually need to support more than just file
permissions if it wants to be secure.


> > +#define pr_fmt(fmt) "%s: " fmt, __func__
> 
> You have defined this in multiple places, which isn't good,
> and it's a bit of code that obfuscates what's going on. It
> may seem like a good idea, but you'll be better off if you
> go ahead and put the code in where you're using it.

Defining pr_fmt seems to be fairly common in the rest of the kernel, a
quick grep shows 98 occurrences. If these are all deprecated, I don't
mind changing it, but it seem worth the bit of obfuscation to me.


> > +u16 flags_to_mode(unsigned int flags)
> > +int str_to_perm(const char *str)
> > +void perm_to_str(u16 perm, char *str)
> 
> Definitely namespace. Could this be static?

I added the namespace, but they're used in a couple other files so they
can't be static. There are a couple other function in policy.[ch] that
could be static but I'm leaving them in the header for now in case they
end up being needed later.


> > +#define dpriv_cur_task   ((struct dpriv_task *)current_security())
> > +#define dpriv_cur_stage  ((struct dpriv_policy *)&dpriv_cur_task->stage)
> > +#define dpriv_cur_policy ((struct dpriv_policy *)&dpriv_cur_task->policy)
> 
> You may get other feedback, but I think that using macros
> to hide indirections make code harder to understand.

I think I'd like to keep these as well, although it might be good to
change them to either `dpriv_current_*()' or `current_dpriv_*()' to
follow the conventions used in <linux/cred.h>.


> > +/* Mode conversion functions */
> > +#define deny(perm, request) \
> > +	unlikely(perm >= 0 && ~perm & request)
> 
> Keep to your namespace and use static inline functions.

I changed these to static inline functions, but the one thing I don't
like about doing so is that I don't think the compiler can't optimize
the `unlikely()' macro as well. For the time being, I moved the unlikely
macros to where deny (now dpriv_denied) gets called.


> Function macros are discouraged. If you really want code duplication
> use static inline functions. And stick with your namespace, use
> dpriv_strfwd() instead of strfwd().
> ...
> String parsing in the kernel is considered harmful.
> Simplify this.

I reworked much of the string parsing. I think /some/ parsing is going
to be necessary no matter what, but hopefully the new way of doing it is
easier to understand and less likely to contains bugs. As a result those
macros are no longer needed at all. (Note that this depends on a
separate patch to sscanf). I've included the new dpriv_stage_write()
below.

(I can include another full diff against either the previous patch or
the mainline kernel if anyone would like it.)

 
static ssize_t dpriv_stage_write(struct file *filp, const char *ubuffer,
		size_t length, loff_t *off)
{
	struct file *file;
	int err, rval, perm;
	char *kbuffer, *perm_str, *path_str;
	int perm_start, perm_end, path_start;

	if (!(kbuffer = kzalloc(length+1, GFP_KERNEL)))
		return -ENOMEM;

	if (copy_from_user(kbuffer, ubuffer, length))
		goto fail_fault;

	/* Parse input */
	path_start = -1;
	sscanf(kbuffer, " %n%*s%n %n", &perm_start, &perm_end, &path_start);
	if (path_start == -1)
		goto fail_inval;
	perm_str = kbuffer+perm_start;
	kbuffer[perm_end] = '\0';
	path_str = kbuffer+path_start;

	/* Check and convert perm */
	if (perm_str[0] == '\0')
		goto fail_inval;
	if ((perm = dpriv_str_to_perm(perm_str)) < 0)
		goto fail_inval;

	/* Check and open path */
	if (path_str[0] == '\0')
		goto fail_inval;
	if (IS_ERR(file = filp_open(path_str, 0, 0))) {
		/* file not found, try trimming trailing spaces */
		strstrip(path_str);
		if (path_str[0] == '\0')
			goto fail_inval;
		if (IS_ERR(file = filp_open(path_str, 0, 0)))
			goto fail_noent;
	}

	path_str = kstrdup(path_str, GFP_KERNEL);
	if (!path_str)
		goto fail_nomem;

	if ((err = dpriv_policy_set_perm(dpriv_cur_stage,
			file->f_dentry->d_inode, path_str, perm))) {
		kfree(path_str);
		rval = err;
		goto out;
	}

	pr_debug("dpriv_task=%p pid=%d perm=%o[%s] path=%p[%s]\n",
		dpriv_cur_task, current->pid, perm, perm_str, file, path_str);

	rval = length;
	goto out; /* Success */

fail_inval: rval = -EINVAL; goto out;
fail_nomem: rval = -ENOMEM; goto out;
fail_fault: rval = -EFAULT; goto out;
fail_noent: rval = -ENOENT; goto out;
out:
	kfree(kbuffer);
	/* if (rval < 0) abort task ? */
	return rval;
}

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]