Re: secmodel_register(9) API

[Christoph Badura <bad%bsd.de@localhost>]

On Mon, Dec 05, 2011 at 07:33:35PM +0100, Jean-Yves Migeon wrote:
> On Mon, 5 Dec 2011 16:22:33 +0100, Christoph Badura wrote:
> >That is by design.  The idea behind splitting the decision process
> >into
> >separate secmodels is to decouple the models and the decision making.
> I can't see how -- secmodels are responsible for the decision
> making, so we cannot decouple these easily.

secmodel_suser doesn't know about securelevel.  secmodel_securelevel
doesn't know about root.  Complete decoupling between models.

> Okay, let's put that differently: enabling/disabling a security
> feature like curtain has nothing to do with secmodel_suser(9)
> itself. Curtain is a feature that says: "only owners of an object
> can query information about it." So securelevel, suser, or bsd44...
> do not intervene in this process, _except_ when you are root
> (pragmatism always add exceptions). That's it.

That ("_except_ when you are root") works only by strongly coupling
curtain with secmodel_suser.  The coupling is actually so strong that you
cannot bring yourself to consider that asking "is-root" is not the only
possible approach.

OK.  Assuming that is the only possible way for the moment...
What you really want to do amounts to calling suser_isroot.  You could just
as well make that call directly and let the linker resolve the symbol.
That way you would satisfy the requirement that secmodel_suser is loaded
in a simple way. Instead you create a baroque interface that doesn't scale
and ends up doing the same thing just more complex and costly.

So far in my books that is more bloat for no discernable gain.
Maybe there is some additional gain that I have missed in your descriptions.
If so, please provide concrete examples.

I think calling the introduction of a variadic function as interface in
the kernel certainly warrants describing it as baroque.

> But the "am I root?" evaluation is more an exception than the rule
> (a weak dependency). Turning curtain into a full fledged suser
> extension because there is one slight divergence is problematic.

How is it "more an exception than the rule"?  AFAICT if you need to make
an exception for root on all decisions made by curtain then "am I root"
needs to be evaluated for every call for a decision by curtain.

I'm not clear what you mean by "there is one slight divergence" and how it
is problematic.  Can you explain that with a concrete example?

> >This so-called "API" is a complete perversion of the kauth system.
> >It
> >pulls the implementation details that the other secmodel is supposed
> >to hide and abstract away out into unrelated code again.

> That's weird: what you describe here is the situation before this
> patch: curtain and usermount were "planted" inside a secmodel they
> do not belong to (secmodel_suser is about making decisions about
> root's rights, not ordinary users).

I don't understand how what I describe would require "planting" (whatever
that means. please explain.) curtain and usermount into a secmodel
they do not belong to.

It is true that currently curtain and usermount are so strongly tied into
the internals of secmodel_suser (that fact that need to know if something
is root) that even with your proposed "API" you cannot avoid stating
that dependency explicitly and blatantly.

There is a simple litmus test that should be performed on any design for an
API or an API enhancement:

    Does this new way of doing things let me do something that wasn't
    possible before?

I cannot see how the secmodel_eval API does that as it is just an extremly
contorted way of doing something that the linker already does.  I don't
see any other value that we get buy paying with this bloat.

AFAICT secmodel_eval fails that test.  But maybe you can give a concrete
and practical example to the contrary.

It isn't difficult to imagine that curtain and usermount be implemented
as separate, full-fledged secmodels with no knowledge about the internals
of other secmodels.  Of course, we would need an API enhancement to ask
through the kauth system if the credential at hand is considered
privileged.  Using that API would mean asking all active secmodels whether
they consider the credential as privileged.

Now that would be an interesting addition.  And it would let you do things
that weren't possible before.

> Knowledge about internals were the de facto standard before: if you
> wanted to have an extension which implements a policy ("users can
> only see state of their objects") with an explicit dependency on
> implementation internals ("is that user privileged?"), you had to
> put the extension inside the secmodel(9) that implements the
> privileged evaluation.

But there is an important difference between asking a generic question
like "is this credential privileged" and asking a specific question about
an implementation detail like "secmodel_suser, is this credential root" or
"secmodel_securelevel, is securelevel above one".

The former can be successfully abstracted into an API while trying to do
the same with the latter just ends up with a painful emulation of the
linker resolving symbols which is already available.

The former can be desirable.  But the latter must be avoided.


> >The problem with this is that it doesn't scale.  You are just
> >arbitrarily
> >moving the code around that needs to be modified when new actions
> >need to
> >be authorized.

> No; we are allowing secmodel(9) to expose evaluation logic ("a
> question") to other secmodels, so they can query for a result that
> depends on their internals.

Uh. I have given a concrete example on how it doesn't scale and how it
has to be modified for other secmodels (E.g. for agc's RBAC secmodel).

You cannot wipe that under the table by just saying "no".

> The ultimate decision remains in the hand of the one making the
> query. Asking a question in the form of "do you allow that action to
> happen?" is a plain miss-use of this API; that should indeed be done
> by kauth(9).

How does asking other secmodels for feedback and making a decision on the
feedback received take the ultimate decision out of the hand of the one
making the query?  Please explain?

(Nevermind that the ultimate decision is made by kauth_authorize_action,
but whatever.)

> >It doesn't scale in the case when a new secmodel is introduced that
> >needs to have a say on this action.

> The evaluation API is not designed to accept hooks, nor shall it be.

I didn't say that the evaluation API should accept hooks.

OTOH the evaluation API is a classic hook implementation in itself.

That makes it kind of difficult for me to see what you have against hooks.

> It's meant for a secmodel(9) to query "safely" (as is: no
> symbol/run-time dependency on code being loaded) a secmodel for
> internal state ("are you in this state?") or internal logic ("given
> these credentials, do you consider them privileged or not?").

What would happen in the "unsafe" case when a symbol/run-time dependency
were used?  The secmodel simply wouldn't load.  Hence it wouldn't make any
decisions.  That's exactly what you describe the "API" to be delivering.

So you replace a one-time check with a dynamic check at runtime for no
value gained but bloat and a performance penality suffered.  I.e. a net
loss!

There's even more damage: before your commit one could be sure that
if a secmodel is present in the kernel that its run-time dependencies 
are fulfilled and it is functional.  With the introduction of the new
API that is no longer possible.  More loss.

> >And it doesn't scale when bsd44
> >or suser is replace with a secmodel that defines "privilege" other
> >than
> >"is-root".

> It should not. If there's another secmodel that appears later that
> redefines "privilege", that secmodel will have to expose evaluation
> callback and let the curtain extension handle it.

Why should it not scale?  Please explain, giving concrete and practical
examples, why it is wrong to have a generic interface that does scale?

> I am not quite sure that the type-usnafety of
> kauth_authorize_action(9) is really kauth's fault, but more a
> shortcoming of the langage used. Serious type-safety is not
> something to expect from C.

The language allows for an API that is much more type-safe.  Admittedly
at the price of introducing more symbols and increasing the "surface"
of the API.  But that just shows that going the un-typesafe route was
a deliberate design decision.

> How are you suppose to allow/deny the modification of "curtain"
> based on securelevel?

Still the wrong question.  The right question is: How are you supposed to
allow/deney the modification of "curtain" based on kauth authorization.

> - you start passing arguments to describe the curtain sysctl(7) so
> that secmodel_securelevel(9) knows that we are trying to modify
> "curtain".
> - you add evaluation logic inside securelevel(9) so it can
> allow/deny this action (you can always enable curtain, but never
> disable it when securelevel > 0)

right.

> - you end up implementing _all_ sysctl management inside
> securelevel(9).

This doesn't follow at all.  If it would there would practical evidence
for that in the code already.  Show me the code.

> So now, instead of exposing a fraction of the
> securelevel internals to other secmodels, you expose other secmodels
> internals to securelevel(9).

I guess we can dismiss this as a classic non-sequitur.

> You cannot really end up with a clear separation of concerns here;
> either securelevel is in charge and knows about curtain. Or curtain
> is in charge and knows about securelevel.

You are mixing up two separate concerns.  Securelevel does not need to
know anything about the semantics of "curtain".  All it needs to know
is that needs to know that changing the value of a certain sysctl variable
needs to be authorized depending on the securelevel.  To me that looks
perfectly like a clear separation of concerns.

> As said above for curtain: any of the two _will_ end up knowing
> internals of the other. Remember: user_set_cpu_affinity cannot be
> enabled when securelevel is 1+, but can still be disabled (if it was
> enabled beforehand). If you implement this logic inside
> securelevel(9), you are implementing user_set_cpu_affinity control
> inside securelevel. securelevel ends up knowing more about
> user_set_cpu_affinity than it ought to.

I disagree for the following reason: A secmodel implements a policy.
The policy in this example is not about what user_set_cpu_affinity's
semantics are but who can change the related sysctl variable and how.

Wether secmodel_securelevel is loaded has no bearing on the semantics
of user_set_cpu_affinity.  It only determines under what conditions
user_set_cpu_affinity can be changed and how.

If secmodel_securelevel is loaded and securelevel is >= 1 than
the current policy is that user_set_cpu_affinity can only be disabled.

But if secmodel_securelevel is unloaded and
secmodel_jym_can_change_user_set_cpu_affinity_willy_nilly is loaded
instead, then jym can, presumably, change user_set_cpu_affinity willy
nilly.  But still changing the value of user_set_cpu_affinity does not
change how user_set_cpu_affinity behaves.

Now, by making the changing of user_set_cpu_affinity depend on a classic
kauth_authorize_action check for it, the above functionality becomes
possible at all.

There is no reason to hard-code the policy about what secmodels
can change user_set_cpu_affinity into some secmodel (or secmodel
extension).

> Given that sysctl IDs are created dynamically, what solution is
> available to securelevel(9) to know that the authorization call is
> about user_set_cpu_affinity, and not another node?

That's extremely simple.  You don't use the sysctl IDs.  You allocate a
new sub-action, say for kauth_system_req, and pass it a unique identifer
for the sysctl variable.  This isn't different from all the other kauth
requests.

It isn't any worse than the current situation because all kauth requests
are enumerated in sys/kauth.h and aren't dynamically changeable at all.

(And hey, binary compatibility can't be an issue, what with the sorting
of the request enums alphabetically at random times, that causes the
crucial request numbers to change arbitrarily.)

> >If curtain doesn't want to know about the internals of kauth_cred it
> >should do it's own user tracking and attach that data to kauth_cred's
> >specificdata.  That's what that interface is for.  And it worked just
> >fine for gaols.
> And curtain ends up asking suser "is user privileged?" to set the
> specificdata key. We are moving the problem around, not solving it.

Uhm, no.  We are not moving the problem around.  Attaching specificdata
*did* solve the problem just fine in the gaols code.

I concede that in the gaols case keeping track of privilege may have
been easier.  But that is a SMOP.

Perhaps I am missing something, though, and you can explain with concrete
and practical examples how it is not solving the problem.

> >If curtain wants to be truly independent of other secmodel's
> >internal then
> >it needs to track itself which credentials it considers privileged.

> It can't; in the case of suser(), it has to ask to secmodel_suser(9)
> whether this user is deemed privileged or not. Otherwise, curtain
> will deny super-user to gather information about objects it does not
> own.

Above I have described a credible design for how curtain, in fact, can
be independent from other secmodel's internals.  How do you explain the
discrepancy?

> Please stop with that one; me and Elad agreed right from the start
> (I can send the private mails if you want them) that
> secmodel_eval(9) _is_ _not_ meant as an alternative for
> authorization call, and should never be used as such. It allows a
> secmodel to expose internal code evaluation logic to the outside
> world at his own discretion.

I have difficulty following you.  What possible weight could you and Elad
agreeing on something beforehand have on the outcome of a technical
discussion?  Please explain.

--chris