Re: Vnode scope implementation

[Elad Efrat <elad%NetBSD.org@localhost>]

On Fri, Jul 10, 2009 at 11:33 AM, David 
Holland<dholland-tech%netbsd.org@localhost> wrote:
> On Mon, Jul 06, 2009 at 05:02:39AM +0300, Elad Efrat wrote:
>  > > I realize that it's a considerable amount of work up front to wade
>  > > through all the places that will need to be adjusted to come up with a
>  > > complete list of all actions that will be required.
>  > >
>  > > However, I think it's an important exercise at this stage, just as
>  > > taking a survey of all the users of namei was an important
>  > > prerequisite for the namei changes I've been planning.
>  >
>  > I've already waded through a lot of code, file-systems and otherwise,
>  > so I'm not worried about that. ;)
>
> Sure, so then you should already have the list of actions you need,
> right? :-)

Of course I do. I have raw lists of various modifications needed to
finish the secmodel abstractions that are based on "grep -n"s plus
information on what action/request to use, whether the code in
question needs some sort of refactoring, etc. Since none of that is
the subject of this thread, though, there's no point in posting it. :)

>  > All I'm saying is that the list of
>  > available actions is not directly related to the scope back-end
>  > itself, which (at least as far as bsd44/suser goes) will not even
>  > examine the actions -- in other words, I'd like to post diffs like the
>  > one I've attached to my email so people can test rather than also
>  > attach a vnode scope implementation each time!
>
> That's perfectly true, but what you're proposing to commit include
> actions, and I don't think it's a good idea to commit a partial set of
> actions in the expectation of sorting it out later. This is partly
> because it's a fairly delicate design issue and partly from routine
> maintenance/entropy concerns.

FWIW, the diff included basically three actions that I think we will
be adding anyway, as they represent "read", "write", and "execute".

> For current purposes it would be enough to have one action "ANY",
> right? Maybe going with that as an intermediate step would be a good
> compromise.

That's not a good compromise. What we'll do instead is not add any
actions at all, since at least for now the scope isn't even being
used. I'd rather not add any actions than adding actions that I know
100% will be removed later.

>  > > > +   /* Indicate if the parent directory is sticky. */
>  > > > +   if ((dnode->tn_mode & S_ISTXT) != 0) {
>  > > > +           action |= KAUTH_VNODE_PARENT_STICKY;
>  > > > +   }
>  > >
>  > > ...and this is the kind of thing that concerns me. The parent
>  > > directory being sticky is not an action. The parent directory being
>  > > sticky is part of the context in which an action is taken.
>  >
>  > Like I said, do not take it as a proposed implementation, it's just
>  > something I'm playing with to (a) have a sample file-system working
>  > entirely on kauth(9) and (b) see what actions we need to implement to
>  > cover all functionality (as you suggest above).
>
> Sure. Except that this is an issue in the interface design.
>
> If you look closely at what I posted upthread you'll note that when I
> did this for VINO (abstracting out permission checks) I set up three
> different scopes, or at least I think they're analogous to your
> scopes, one for directories, one for files, and one for a pair
> consisting of a file and its containing directory. To check for unlink
> permission you use the pair context, and any consideration of
> stickiness can be handled abstractly. (From what I can tell, and from
> comments in the source, the MAY_REMSTICKY action that's in the
> file-only context is supposed to be internal use only, and ideally
> wouldn't have been exposed outside the security code.)

The diff I attached does use the "pair context", as suggested by
Apple's document (see KAUTH_VNODE_DELETE). In the tmpfs diff, the
relevant line is:

    error = kauth_authorize_vnode(cnp->cn_cred,
        action, *vpp, dvp, error);

I'm not going to grep through VINO's sources to compare MAY_REMSTICKY
and KAUTH_VNODE_PARENT_STICKY, but if I had to guess I'd say they're
not too different from each other.

>  > But, since you did bring it up, I will explain why I did that.
>  >
>  > There are two issues with the traditional implementation, or better
>  > said, one issue that manifests twice: internals of two different
>  > subsystems are taken for granted. Think about the sticky bit
>  > directories and root, or the immutable flag and securelevel: a mode
>  > bit and a flag -- file-system internals -- are examined with regards
>  > to the "admin" user (uid 0) and the securelevel.
>
> Right. I seem to recall saying something at some past point about
> needing to abstract either credentials or permissions but not both.
> That is, you can either pass a specific credential (euid 0,
> securelevel) to something abstracted that knows how to apply it to
> fs-specific permissions, or you can give a specific permission
> (sticky, immutable) to something abstract that knows how to apply
> model-specific credentials to it. Doing neither gives you next to no
> flexibility and doing both is messy and difficult.
>
> From what I've seen so far (and from rewriting this section of this
> mail repeatedly trying to clarify it) my understanding is that kauth
> is not supposed to abstract either credentials or permissions;
> instead, it allows patching the decision that arises from applying a
> specific representation of credentials to a specific representation of
> permissions.
>
> This alone provides next to no flexibility; flexibility is added by
> allowing listeners to maintain additional information to use as the
> basis of decisions. This additional information might (AIUI) function
> as either credentials or permissions, or both.

What prevents listeners from maintaining additional information to use
as the basis for decisions?

> This is in turn problematic as soon as there are fs-specific
> permissions, because applying arbitrary additional credential
> information to existing permissions requires that the permissions be
> specific and not abstracted. Therefore, every fs-specific permission
> that might appear has to be explicitly built into and visible in kauth
> so an arbitrary listener can interact with it correctly.
>
> I don't really like that model, although I'm not sure what else to
> suggest at the moment.
>
> I've been thinking in terms of abstract representations of credentials
> and permissions (because encapsulating each is the best way to go
> about centralizing security decisions) and I think this is one of the
> reasons we have been talking past each other.
>
> Anyway, if you're going to be potentially working in arbitrary
> additional credentials *and* permissions you definitely need to handle
> stickiness by providing both the containing directory and the
> contained file and letting the listener check for the sticky bit.

I.e., issuing a VOP_GETATTR() in the listener. FWIW, I have removed
the KAUTH_VNODE_PARENT_STICKY flag from my local tree, and listeners
are now required to query for it themselves. :)

Thanks,

-e.