Current-Users archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: secmodel_register(9) API

(I am CC'ing Elad, as we both worked on it)

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. 


So, when one wants to create sysctls that can only be changed when
securelevel is below a certain level, you end up adding more hooks to 
secmodel_securelevel(9)...
That is intentional. Every time a new control is added all the secmodels 
need to be examined if they need updating.


which is problematic, because the sysctl does
not necessarily belong to this secmodel (consider curtain and suser, as 
an exemple).
Uh, sysctls do not "belong to a secmodel". Whatever that is supposed to 
mean.
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.
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. 


error = secmodel_eval("org.netbsd.secmodel.suser", "is-root",
    cred, &isroot, NULL);



This one asks the suser module if the user cred is assumed to be
root when evaluated by suser module. If the module is present, it
will respond. If absent, the call will return an error.
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). 


This is adding
back all the interdependencies and knowledge about internals that kauth 
is supposed to abstract and hide.


That's precisely what we are trying to avoid.
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.
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.
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). 


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. 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?"). 


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.
For the "is-root" case, the evaluation will return an error if the secmodel_suser(9) is not loaded. The curtain extension will then make the decision concerning this, and fallback to its default case.
IAW not even does this not scale from 1 to 2. It doesn't even scale from 
A to B.


Args and command are arbitrarily defined; it's up to the secmodel(9)
to explain what it expects.
This is even worse. The type-unsafety of kauth_authorize_action is bad enough. There is no reason to make it worse by introducing a variadic 
function in a security context.

IMNHO this idea alone is reason enough to back this out and have it
redesigned.
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. 


Typical example is securelevel testing: when someone wants to know
whether securelevel is raised above a certain level or not, the
caller has to request this property to the secmodel_securelevel(9)
module. Given that securelevel module may be absent from system's
context (thus making access to the global "securelevel" variable
impossible or unsafe), this API can cope with this absence and
return an error.
This isn't a typical example. There's only one entity in the kernel that 
wants to know whether securelevel is raised: secmodel_securelevel.
The typical example is that the kernel wants to ask the secmodels: "are 
these credentials authorized to perform the action detailled in the
remaining argument?".
And if the securelevel secmodel is loaded that sometimes says "yay" or 
"nay" for the cases that it is interested in.

In other words, you are asking the wrong questions and thinking about
this in an incorrect way. Therefore you end up at incorrect solutions. 

Right, dependency inversion. Let's do it that way then.
How are you suppose to allow/deny the modification of "curtain" based on securelevel? - 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) - you end up implementing _all_ sysctl management inside securelevel(9). So now, instead of exposing a fraction of the securelevel internals to other secmodels, you expose other secmodels internals to securelevel(9).
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. 


On Tue, Nov 29, 2011 at 01:58:20PM +0100, Jean-Yves Migeon wrote:
On Tue, 29 Nov 2011 11:13:01 +0000 (UTC), yamt%mwd.biglobe.ne.jp@localhost wrote: 
Consider user_set_cpu_affinity: if the sysctl cannot be set any more
when securelevel is above or below a threshold, checking for the
securelevel variable means that this sysctl has a strong dependency
on securelevel (or else, it won't be able to get the variable). So
if you want to still provide this sysctl but without having
securelevel loaded, you are screwed: it's part of this module.
There is no need for the code that manages user_set_cpu_affinity to have a dependency on the securelevel variable. Or even to know about it in the 
first place.

All that is need is a call to kauth_authorize_action asking if it is
allowed to modify the variable bound to the sysctl name.
This is precisly the reason that the indirection level that kauth provides 
has been introduced.

You are starting from false premises.
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.
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? 


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. 


For convenience, curtain may allow specific credentials to gather
information for all objects, and not just the ones a user owns.


Distinguishing credentials and giving some of them higher privileges
is entirely internal to a secmodel.  That is why curtain should do
its own user tracking through kauth_cred's specificdata.


When
suser is loaded, thes credentials are those corresponding to root.
That, however, is at suser's discretion to decide. I.e. curtain should 
not know about the internals of suser or what kind of credentials it
considers privileged for which operations.
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. 

[snip]
What you have is a requirement that changing sysctl variable that control that behaviour (curtain and user_set_cpu_affinity) requires privilege.
How that privilege is granted and checked depends on the secmodels that 
are currently active.
E.g. if only suser is available conventionally only root would be able to 
change these sysctl variables.

If securelevel is available that secmodel may restrict the operation
based on the setting of the securelevel variable.
If some other secmodel is loaded then that has a say on that matter too. 
Maybe it counts the demons in the users nose.

And if only that other secmodel is loaded then being able to change
those sysctl variable may only depend on the number of demons in your
nose.

It is intentional that there is no strong coupling between secmodels.
and kauth_authorize is a much better interface for checking for privilege 
than the proposed secmodel_eval.
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. 

--
Jean-Yves Migeon
jeanyves.migeon%free.fr@localhost
Home | Main Index | Thread Index | Old Index