Re: secmodel_register(9) API

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

I don't understand what the problems are. In any case, this does not
violate anything. The language used to describe the supposed issues
with the new API is ridiculous.

What the new API allows is interaction between secmodels that are
built by people who are not part of NetBSD and don't want their
secmodel to become part of NetBSD but do want to take advantage of
features in secmodels provided by NetBSD.

Personally I don't care if this stays or not. All I can say is that I
have not seen a single argument worthy of consideration against it and
I would strongly recommend to leave it in.

Please don't CC me any further emails about this.

Elad

On Mon, Dec 5, 2011 at 1:33 PM, Jean-Yves Migeon
<jeanyves.migeon%free.fr@localhost> wrote:
> (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