tech-kern archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: Vnode scope implementation
David Holland wrote:
On Sat, Jul 04, 2009 at 09:08:18PM +0300, Elad Efrat wrote:
> I've got a feeling that you haven't read the man-page or Apple's
> TN2127, but anyway--
Is it required to read random Apple docs to understand what you're
doing?
There are several ways you can go about understanding what we're doing.
First, there are NetBSD mailing list discussions that go back to 2005
regarding the kauth(9) implementation in NetBSD. I suppose you don't
expect me, or anyone else for that matter, to repeat things we've said
several times in the past? :)
You could also read the man-pages, which are very detailed, also
referring you to the "random" Apple document on which this whole
subsystem is based.
Another thing you could do to get a bigger picture of where all this
is going is read my paper from 2006, that elaborates not only on
kauth(9), but other security features as well.
Finally, you could take a look at the code and see that we already have
several scopes, and what they're used to is very simple: where we used
to say "is this root?", we now pass the decision onto kauth(9), to allow
plugging in additional authorization mechanisms. There are several OSs
out there that implement similar frameworks -- Linux and FreeBSD come to
mind.
> > I'd like to see some examples of what this is expected to look like.
>
> There are numerous examples of how this will look like: anything that
> was already converted to kauth(9), pretty much, as well as the ACL
> examples I've posted not too long ago.
There are no *vnode* examples.
Okay. The first transition I'm planning will be in access() routines.
Let's take ufs_access(). Below is the original function:
int
ufs_access(void *v)
{
struct vop_access_args /* {
struct vnode *a_vp;
int a_mode;
kauth_cred_t a_cred;
} */ *ap = v;
struct vnode *vp;
struct inode *ip;
mode_t mode;
int error;
vp = ap->a_vp;
ip = VTOI(vp);
mode = ap->a_mode;
error = ufs_check_possible(vp, ip, mode);
if (error)
return error;
error = ufs_check_permitted(vp, ip, mode, ap->a_cred);
return error;
}
What I would like to do as a first step -- which I think I've mentioned
several times -- is very simple. I would like to add an additional
function call before the last return statement, so that the function
looks somewhat like this:
int
ufs_access(void *v)
{
struct vop_access_args /* {
struct vnode *a_vp;
int a_mode;
kauth_cred_t a_cred;
} */ *ap = v;
struct vnode *vp;
struct inode *ip;
mode_t mode;
int error;
vp = ap->a_vp;
ip = VTOI(vp);
mode = ap->a_mode;
error = ufs_check_possible(vp, ip, mode);
if (error)
return error;
error = ufs_check_permitted(vp, ip, mode, ap->a_cred);
error = kauth_authorize_vnode(ap->a_cred,
kauth_mode_to_action(mode), vp, NULL, error);
return error;
}
Similar to any other place in which we use kauth(9), here too we
are passing an action and a context to the listeners to make a decision.
What will happen is very simple: the file-system will check access
based on permissions only, without taking for granted one exception or
another (i.e., root). Then, it will pass that decision to kauth(9). The
latter will, in the default, simple case, simply check if the user is
root or not -- and either flip the decision or leave it as-is. Under
different circumstances, where, say, an ACL secmodel is loaded,
additional rules may apply (another example is the securelevel secmodel,
that doesn't care about root and will prevent you from doing things
even then).
I am currently running with and testing a diff that switches tmpfs to
use the vnode scope locally. I've attached it (note that it's *not* a
proposal and it's *not* up for discussion), so you could get a better
idea of the changes. You will notice, however, that the set of actions
is slightly different than that of Apple's. You might like some, you
might not, but it doesn't matter because none of it is up for discussion
yet.
What's up for discussion is the addition of a back-end to authorize
*any* actions on a *vnode* scope.
> > however, when you go to implement you'll find you've left
> > a few things off here.
>
> More than a few. Again, if you will look at TN2127, you'll see that it
> contains both the definitions you see above as well as several others.
> I would like to implement them incrementally and not add actions
> before they're used. This is why my mail says "some actions". This is
> how kauth(9) was implemented so far.
So you're just taking the set of actions from Apple lock, stock, and
barrel? Or are you planning something else that we won't get to see
until you've committed it?
A quick look at the Apple document, followed by an even quicker look at
any of the aforementioned references (or just sys/kauth.h) will answer
that question for you: NetBSD is aware of the Apple implementation, but
extends it far beyond what Apple is using it for. So far, we've borrowed
some of the actions, while we added a lot more scopes and plenty other
actions in addition. It's reasonable to believe that we'll be doing
exactly the same here -- we'll probably have a little bit different set
of actions than them, and our usage of the scope in general differs (as
we will be authorizing from the file-systems, rather than normalize
permissions/ACLs and doing it in the VFS -- which, to remind you, was
something that you requested).
Either way, I'd like to see the complete set of actions you propose so
it can be checked for completeness and orthogonality.
Unfortunately that will not happen. The nature of development, not in
just the recent months, but throughout the whole time since kauth(9) was
introduced, was doing things incrementally. This whole work, which I
call "security model abstraction" (i.e., replacing KAUTH_GENERIC_ISSUSER
with something more meaningful) is slowly taking place whenever I get
the time to move on with it. Just as we're still adding scopes, actions
and requests these days, long after the kauth(9) integration, we will be
shaping our vnode scope slowly as we move along.
Luckily, that should not affect the subject of this thread, since we
*are* interested in a vnode scope, and so we *should* have a way to
authorize *some set of actions* in it. Testing diffs that adapt a
file-system to use kauth(9) before we have the back-end in place is
somewhat more difficult than first introducing the back-end and *then*
adapting file-systems. Also, note, that the decision logic stays
*inside* the file-systems, and pretty much all the kauth(9) around it
is taking care of just one thing: comparing the euid to zero.
If you are suggesting that I will be checking in kauth(9) functionality
without peer review or approval, I suggest you go through some of the
relevant commit logs.
David, I've got a feeling that you're very emotional about the vnode
scope because you have plans for the NetBSD's VFS/file-systems (I am
guessing because over the past months you've only made comments on
file-system related changes, rather than kauth(9) developments in
general). The changes proposed in this thread do not modify anything in
that area: they affect kauth(9) internals only. Based on the above
logic, I see no reason not to check them in, so we can move on to the
important changes, that I believe you are more worried about.
Let's not waste our time. :)
Thanks,
-e.
Index: tmpfs_subr.c
===================================================================
RCS file: /usr/cvs/src/sys/fs/tmpfs/tmpfs_subr.c,v
retrieving revision 1.53
diff -u -p -r1.53 tmpfs_subr.c
--- tmpfs_subr.c 7 May 2009 19:30:30 -0000 1.53
+++ tmpfs_subr.c 4 Jul 2009 21:25:23 -0000
@@ -964,6 +964,8 @@ tmpfs_chflags(struct vnode *vp, int flag
{
int error;
struct tmpfs_node *node;
+ kauth_action_t action = KAUTH_VNODE_CHANGE_FLAGS;
+ int fs_decision = 0;
KASSERT(VOP_ISLOCKED(vp));
@@ -973,30 +975,44 @@ tmpfs_chflags(struct vnode *vp, int flag
if (vp->v_mount->mnt_flag & MNT_RDONLY)
return EROFS;
- /* XXX: The following comes from UFS code, and can be found in
- * several other file systems. Shouldn't this be centralized
- * somewhere? */
- if (kauth_cred_geteuid(cred) != node->tn_uid &&
- (error = kauth_authorize_generic(cred, KAUTH_GENERIC_ISSUSER,
- NULL)))
+ if (kauth_cred_geteuid(cred) != node->tn_uid)
+ fs_decision = EACCES;
+
+ /*
+ * If the new flags have non-user flags that are different than
+ * those on the node, we need special permission to change them.
+ */
+ if ((flags & SF_SETTABLE) != (node->tn_flags & SF_SETTABLE)) {
+ action |= KAUTH_VNODE_CHANGE_SYSFLAGS;
+ if (!fs_decision)
+ fs_decision = EPERM;
+ }
+
+ /*
+ * Indicate that this node's flags have system attributes in them if
+ * that's the case.
+ */
+ if (node->tn_flags & (SF_IMMUTABLE | SF_APPEND)) {
+ action |= KAUTH_VNODE_HAS_SYSFLAGS;
+ }
+
+ error = kauth_authorize_vnode(cred, action, vp, NULL, fs_decision);
+ if (error)
return error;
- if (kauth_authorize_generic(cred, KAUTH_GENERIC_ISSUSER, NULL) == 0) {
- /* The super-user is only allowed to change flags if the file
- * wasn't protected before and the securelevel is zero. */
- if ((node->tn_flags & (SF_IMMUTABLE | SF_APPEND)) &&
- kauth_authorize_system(l->l_cred, KAUTH_SYSTEM_CHSYSFLAGS,
- 0, NULL, NULL, NULL))
- return EPERM;
+
+ /*
+ * Set the flags. If we're not setting non-user flags, be careful not
+ * to overwrite them.
+ *
+ * XXX: Can't we always assign here? if the system flags are different,
+ * the code above should catch attempts to change them without
+ * proper permissions, and if we're here it means it's okay to
+ * change them...
+ */
+ if (action & KAUTH_VNODE_CHANGE_SYSFLAGS) {
node->tn_flags = flags;
} else {
- /* Regular users can change flags provided they only want to
- * change user-specific ones, not those reserved for the
- * super-user. */
- if ((node->tn_flags & (SF_IMMUTABLE | SF_APPEND)) ||
- (flags & UF_SETTABLE) != flags)
- return EPERM;
- if ((node->tn_flags & SF_SETTABLE) != (flags & SF_SETTABLE))
- return EPERM;
+ /* Clear all user-settable flags and re-set them. */
node->tn_flags &= SF_SETTABLE;
node->tn_flags |= (flags & UF_SETTABLE);
}
@@ -1036,6 +1052,9 @@ tmpfs_chmod(struct vnode *vp, mode_t mod
error = genfs_can_chmod(vp, cred, node->tn_uid, node->tn_gid,
mode);
+
+ error = kauth_authorize_vnode(cred, KAUTH_VNODE_CHANGE_MODE, vp, NULL,
+ error);
if (error)
return (error);
@@ -1087,6 +1106,9 @@ tmpfs_chown(struct vnode *vp, uid_t uid,
error = genfs_can_chown(vp, cred, node->tn_uid, node->tn_gid, uid,
gid);
+
+ error = kauth_authorize_vnode(cred, KAUTH_VNODE_CHANGE_OWNERSHIP, vp,
+ NULL, error);
if (error)
return (error);
@@ -1186,6 +1208,9 @@ tmpfs_chtimes(struct vnode *vp, const st
return EPERM;
error = genfs_can_chtimes(vp, vaflags, node->tn_uid, cred);
+
+ error = kauth_authorize_vnode(cred, KAUTH_VNODE_CHANGE_TIMES, vp, NULL,
+ error);
if (error)
return (error);
Index: tmpfs_vnops.c
===================================================================
RCS file: /usr/cvs/src/sys/fs/tmpfs/tmpfs_vnops.c,v
retrieving revision 1.61
diff -u -p -r1.61 tmpfs_vnops.c
--- tmpfs_vnops.c 3 Jul 2009 21:17:41 -0000 1.61
+++ tmpfs_vnops.c 4 Jul 2009 19:51:07 -0000
@@ -209,13 +209,33 @@ tmpfs_lookup(void *v)
if ((cnp->cn_flags & ISLASTCN) &&
(cnp->cn_nameiop == DELETE ||
cnp->cn_nameiop == RENAME)) {
- if ((dnode->tn_mode & S_ISTXT) != 0 &&
- kauth_authorize_generic(cnp->cn_cred,
- KAUTH_GENERIC_ISSUSER, NULL) != 0 &&
+ kauth_action_t action = 0;
+
+ /* Indicate if the parent directory is sticky.
*/
+ if ((dnode->tn_mode & S_ISTXT) != 0) {
+ action |= KAUTH_VNODE_PARENT_STICKY;
+ }
+
+ /* This is the file-system's decision. */
+ if ((action & KAUTH_VNODE_PARENT_STICKY) != 0 &&
kauth_cred_geteuid(cnp->cn_cred) !=
dnode->tn_uid &&
- kauth_cred_geteuid(cnp->cn_cred) !=
tnode->tn_uid)
- return EPERM;
- error = VOP_ACCESS(dvp, VWRITE, cnp->cn_cred);
+ kauth_cred_geteuid(cnp->cn_cred) !=
tnode->tn_uid) {
+ error = EPERM;
+ } else
+ error = 0;
+
+ /* Only bother if we're not already failing it.
*/
+ if (!error) {
+ error = VOP_ACCESS(dvp, VWRITE,
cnp->cn_cred);
+ }
+
+ if (cnp->cn_nameiop == DELETE)
+ action |= KAUTH_VNODE_DELETE;
+ else /* if (cnp->cn_nameiop == RENAME) */
+ action |= KAUTH_VNODE_RENAME;
+
+ error = kauth_authorize_vnode(cnp->cn_cred,
+ action, *vpp, dvp, error);
if (error != 0)
goto out;
cnp->cn_flags |= SAVENAME;
@@ -406,6 +426,9 @@ tmpfs_access(void *v)
error = tmpfs_check_permitted(vp, node, mode, cred);
+ error = kauth_authorize_vnode(cred, kauth_mode_to_action(mode), vp,
+ NULL, error);
+
out:
KASSERT(VOP_ISLOCKED(vp));
Home |
Main Index |
Thread Index |
Old Index