On 03/18/2017 12:53 AM, Tetsuo Handa wrote:

Casey Schaufler wrote:

quoted

On 3/17/2017 5:52 AM, Tetsuo Handa wrote:

quoted

Casey Schaufler wrote:

quoted

Subject: [PATCH RFC 1/9] LSM: Add /sys/kernel/security/lsm

This has been accepted into security next - provided for completeness

Before going to "[PATCH RFC 5/9] LSM: General but not extreme module
stacking", what do you think about below change?

----------------------------------------

quoted

From 8fe80e4b6a479a81d720571ad3b5979f6fd1e6ae Mon Sep 17 00:00:00 2001

From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Fri, 17 Mar 2017 21:34:51 +0900
Subject: [PATCH] LSM: Pass module name via "union security_list_options".

Commit d69dece5f5b6bc7a ("LSM: Add /sys/kernel/security/lsm") added
"char *lsm" field to "struct security_hook_list". But it is currently
never used after set at initialization time. While there is a plan that
this field will be used by "LSM: General but not extreme module stacking"
patch, there is no point with setting the same value to each element of
"struct security_hook_list" array. We can keep "struct security_hook_list"
4 * sizeof(void *) bytes if we keep this field in "union
security_list_options".

The "char *" argument in security_add_hooks() was removed because we can
fetch it from security_list_options"->module_name field. This implies
that setting security_list_options"->module_name field is mandatory.

On further reflection, and spending a little time back
in the code, it turns out that this won't work at all.
The lsm field needs to be in the "struct security_hook_list"
so that hook processing that uses the module name such
as security_getprocattr has it. It wouldn't hurt to have
the module name in "union security_list_options", but
a list of module names doesn't help anything, either.

Oops. I didn't expect you to use a list of module names for
getprocattr()/setprocattr(). I assumed you pass module name to
individual module and let individual module return a value or -ENOENT.

But looking at PATCH 5/9, do we really want to use "context" format?
TOMOYO uses $name=$value encoding which can include arbitrary characters
and thus allows simple tokenizing using a whitespace like

If its arbitrary characters does that not include spaces? In which case
simple tokenizing based on whitespacing would not work.

Regardless as much as I would like to go back and change it, the apparmor
context stuff really does use arbitrary characters including ws, quotes
and if we are talking writes \000 is used as well.

We can update libapparmor to hit a different interface for writing,
and work with lxc and a few other projects that are using the interface
directly, so that stop hitting the proc interface.

But we would very much like the read side to work correctly. The apparmor
context sadly currently always includes ws. So simple ws tokenizing won't
work, but I am open to changing that. Individual files would work better
for apparmor, but that has its own issues.

So I am some what ambivalent about the interface chosen. As long as
we can standardize on something, I'll make the effort on the apparmor
side to make sure it works. It may mean apparmor has to break some
things but its not like apparmor doesn't already have work arounds in
place that can be used to remove the arbitrary character issue, we just
have to force people to add them to their existing policy.

  lsm1=v1 lsm2=v2 lsm3=v3

but you are trying to introduce

  lsm1='v1'lsm2='v2'lsm3='v3'

format at the cost of giving up ability to include arbitrary characters
(e.g. '\n'), aren't you?

If userspace has to concatenate strings like

  lsm1='v1'lsm2='v2'lsm3='v3'

and write to single file at one time like

  echo lsm1='v1'lsm2='v2'lsm3='v3' > /proc/$pid/attr/context

, userspace can simply do

  echo $v1 > /proc/$pid/attr/$lsm1/$name
  echo $v2 > /proc/$pid/attr/$lsm2/$name
  echo $v3 > /proc/$pid/attr/$lsm3/$name

which will not need to exclude characters like '\n'.

sure but then userspace also has to iterate those files, not such an
issue for writes, but for reads the the application is going to have
to try to organize the data into some form.

If there were hundreds of modules, the overhead might matter.
But given that there will be only few modules which use these
interfaces, I think userspace can tolerate such multiple writes.

I'm not worried about the writes, those will largely be coming from
the tools for a given project. It is the generic tools doing reads (ps,
top, ...) that should have a standard format to work with, and
should not have to worry about the details of each lsm. This is what
makes me lean in the direction of the single file.

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html