Re: secmodel_register(9) API

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

On Tue, Nov 29, 2011 at 02:51:38AM +0100, Jean-Yves Migeon wrote:
> A few weeks back I tried adding a sysctl knob to allow users to control
> their CPU affinity. However, while implementing checks to restrict usage
> of this sysctl, I hit a limitation regarding secmodel(9): the current
> design cannot allow a secmodel to query the state/properties of other
> secmodels.

That is by design.  The idea behind splitting the decision process into
separate secmodels is to decouple the models and the decision making.

> 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.

> 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.  This is adding
back all the interdependencies and knowledge about internals that kauth
is supposed to abstract and hide.

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.

It doesn't scale in the case when a new secmodel is introduced that
needs to have a say on this action.  And it doesn't scale when bsd44
or suser is replace with a secmodel that defines "privilege" other than
"is-root".

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.

> 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.

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.

On Mon, Dec 05, 2011 at 11:01:50AM +0100, Jean-Yves Migeon wrote:
> kauth(9) hides all the credentials behind "opaque" types, like
> kauth_cred_t. What is found behind is implementation-defined.
> Nowadays, uid_t/euid_t model is used.

One can also take a different view.  Putting the uid/euid/group data
directly into the struct kauth_cred can be seen as a performance
optimisation because the bsd44 model is standard and called so frequently.

Just as well the uid/euid/gid stuff could have been put into secmodel_bsd44
private specificdata attached to kauth_cred.

> secmodel_suser() expresses the policy bound to the super-user (ie.
> all operations that root - everything with uid 0 - is allowed to
> perform). That is: _just_ super-user. Nothing more.
> 
> === curtain ===
> 
> curtain is a security measure that restricts the information
> accessible to any given user to the objects it currently "owns",
> ownership being known by the credentials attached to this object.
> Note that the "credentials" is still an opaque "type" here, and
> should not assume whether it represents an uid, label, or role.
> 
> However, secmodel_suser(9) _does_ make that assumption (credentials
> being UIDs), which contradicts the original intent of curtain.

Eh?  What does the internal implementation details of secmodel_suser
have to do with curtain?

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.

> 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.

If it isn't independ then there is no reason to invent an "API" on the
kauth level to mess with the other secmodels' internals.

> But in the event that suser is replaced by another secmodel,
> modifying secmodel_extensions to cope with this new shiny secmodel
> is pretty trivial. While with the "old" design, you would have to
> reimplement curtain in this secmodel first, by copy/pasting it from
> suser.

> === user_set_cpu_affinity ===

> All these sysctl have requirements on the alteration of their value
> when securelevel is above 0. Basically, you can remove rights to
> users, but cannot grant them more any more (unless you did so before
> raising level). That implies that securelevel is present, but alas,
> anyone is free not to use/load securelevel module. Hence, we have to
> provide a way to cope with this: here comes secmodel_eval(9).

This is not the right approach to think about this.

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.

--chris