Dbus and multiple LSMs (was Preferred subj= with multiple LSMs)

Dbus and multiple LSMs (was Preferred subj= with multiple LSMs)

[Casey Schaufler]

On 7/19/2019 11:47 AM, Simon McVittie wrote:
> Thanks for considering user-space in this, and sorry if I'm hijacking
> this thread a bit (but I think some of the things I'm raising might be
> equally applicable for audit subjects).

Thank you for asking these questions. I think that if we
can address the issues around dbus we'll be in pretty good
shape in general.

> On Fri, 19 Jul 2019 at 09:29:17 -0700, Casey Schaufler wrote:
>> On 7/19/2019 5:15 AM, Simon McVittie wrote:
>>> However, I think it would be great to have multiple-"big"-LSM-aware
>>> replacements for those interfaces, which present the various LSMs as
>>> multiple parallel credentials.
>> Defining what would go into liblsm* is a task that has fallen to
>> the chicken/egg paradox. We can't really define how the user-space
>> should work without knowing how the kernel will work, and we can't
>> solidify how the kernel will work until we know what user-space
>> can use.
> I was hoping the syscall wrappers in glibc would be a viable user-space
> interface to the small amount of LSM stuff that dbus needs to use in an
> LSM-agnostic way. That's what we use in dbus at the moment (in practice
> just getsockopt, but I'd also be reading /proc/self/attr/current if there
> was a specification for how to normalize it to match SO_PEERSEC results)
> and it's no harder than the rest of the syscall-level APIs.

I don't see how to do that without making the Fedora and Ubuntu user space
environments remain functional.

> A single LSM-agnostic shared library would be the next best thing from
> my point of view.

Good, that's how it looks to me as well.

>> An option that hasn't been discussed is a display option to provide
>> the Hideous format for applications that know that's what they want.
>> Write "hideous" into /proc/self/attr/display, and from then on you
>> get selinux='a:b:c:d',apparmor='z'. This could be used widely in liblsm
>> interfaces.
> If the way to parse/split it is documented, then this would be easier
> for dbus-daemon than continually resetting attr/display. It would be
> especially good if you can document a way to find out which one of the
> many labels would have been seen by an older user-space process that never
> wrote to attr/display ("it's the first one in the list" would be fine),
> so that we can put that one in our backwards-compatible API to clients.

/sys/kernel/security/lsm provides the list of all LSMs active on the system.
It would be trivial to add /sys/kernel/security/default-display-lsm which
would contain that.

> Or, alternatively, we could pass it on directly to our clients and let
> *them* parse it (possibly by using liblsm), the same way AppArmor-aware
> D-Bus clients have to know how to use either aa_splitcon() or their
> own parsing to go from the raw SO_PEERSEC result
> "/usr/bin/firefox (enforce)" to the pair ("/usr/bin/firefox", "enforce")
> that they probably actually wanted.
>
>>> Do you mean that if process 11111 writes (for example) "apparmor" into
>>> /proc/11111/attr/display, and then reads /proc/22222/attr/current
>>> or queries the SO_PEERSEC of a socket opened by process 22222,
>>> it will specifically see 22222's AppArmor label and not 22222's SELinux
>>> label?
>> Process 11111 would see the AppArmor label when reading
>> /proc/22222/attr/current. The display value is controlled
>> by process 11111 so that it can control what data it wants
>> to see.
> OK, that's what I'd hoped.
>
>> The display is set at the task level, so should be thread safe.
> OK, good. However, thinking more about this, I have other concerns:
>
> * In library code that can be used by a thread (task) that also uses other
>   arbitrary libraries, or in an executable that uses libraries that might
>   be interested in LSMs, the only safe way to deal with attr/display would
>   be this sequence:
>
>     - write desired value to /proc/self/attr/display
>     - immediately read /proc/other/attr/current or query SO_PEERSEC
>
>   and it would not be safe to rely on writing /proc/self/attr/display
>   just once at startup, because some other library might have already
>   changed it between startup and the actual read. Paradoxically, this
>   maximizes the chance of breaking a reader that was relying on writing
>   /proc/self/attr/display once during startup.
>
> * If an async signal handler needs to know a LSM label for whatever
>   reason, it will break anything in the same thread that was relying on
>   that sequence, because it might have interrupted them between their
>   write and their read:
>
>     main execution path                  signal handler
>     -------------------                  --------------
>
>     write "apparmor" to attr/display
>     (interrupted by async signal)
>                                          write "selinux" to attr/display
>                                          read attr/current or SO_PEERSEC
>                                          do other stuff with SELinux label
>                                          return
>     (resumes)
>     read attr/current or SO_PEERSEC
>     expect an AppArmor label
>     get a SELinux label
>     sadness ensues
>
>   Of course it's probably crazy for an async signal handler to do
>   this... but people do lots of odd things in async signal handlers,
>   and open(), read(), write(), getsockopt() are all async-signal-safe
>   functions, so it's at least arguably valid.

Stephen Smalley has already pointed out some of these issues.
I see display being used in scripts:

	echo apparmor > /proc/self/attr/display
	apparmor-do-stuff --options --deamon

much more than inside new or updated programs.

>> Writing to display does not require privilege, as it affects only
>> the current process. The display is inherited on fork and reset on
>> a privileged exec.
> Another concern here: are you sure it shouldn't be reset on *any*
> exec?

Yes, because so much of the user-space ecosystem depends on programs
that rarely get updated there has to be a way to specify it externally.
I don't like the situation, but we can't ignore it.

> Lots of programs (including dbus-daemon) fork-and-exec arbitrary
> child processes that come from a different codebase not under our
> control and aren't necessarily LSM-stacking-aware. I don't really want
> to have to reset /proc/self/attr/display in our increasingly crowded
> after-fork-but-before-exec code path (which, according to POSIX, is not
> a safe place to invoke any non-async-signal-safe function, so we can't
> easily do error handling if something goes wrong there).

My hope is that new and updated programs will have to tools
they need to get it right, and that those that don't won't
fall over on a well configured system.

> Is there any possibility of having a parallel kernel API that,
> if it exists, always returns the whole stack, maybe something
> like /proc/<pid>/attr/current_stack and the SO_PEERSECLABELS that I
> suggested previously,

/proc/<pid>/attr/current_stack is easy. SO_PEERSECLABELS will be
harder to sell, but would not be hard to implement if we can get
agreement on the Hideous format.
 

> instead of repurposing /proc/<pid>/attr/current
> and SO_PEERSEC to have contents that vary according to ambient process
> state in their reader?

In addition, yes. Instead of? I don't think that we can have a
backward compatibility story that flies without it.

>  (Bonus points if they are documented/defined with
> a particular syntactic normalization this time, unlike the situation
> with /proc/<pid>/attr/current and SO_PEERSEC where in principle you
> need LSM-specific knowledge to know whether a trailing "\n" or "\0"
> is safe to discard.)

I think that's necessary.

>
>     smcv

Re: Dbus and multiple LSMs (was Preferred subj= with multiple LSMs)

[Simon McVittie]

On Fri, 19 Jul 2019 at 13:02:24 -0700, Casey Schaufler wrote:
> On 7/19/2019 11:47 AM, Simon McVittie wrote:
> > I was hoping the syscall wrappers in glibc would be a viable user-space
> > interface to the small amount of LSM stuff that dbus needs to use in an
> > LSM-agnostic way.
> 
> I don't see how to do that without making the Fedora and Ubuntu user space
> environments [not] remain functional.

What I was thinking of was a second, parallel kernel <-> user-space
interface (like the SO_PEERSECLABELS that I suggested) for future/updated
user-space components. SO_PEERSEC would continue to return some
hopefully-backwards-compatible thing, but would be deprecated, because it
cannot fully represent the reality of LSM stacking while remaining
backwards-compatible.

> I see display being used in scripts:
> 
> 	echo apparmor > /proc/self/attr/display
> 	apparmor-do-stuff --options --deamon
> 
> much more than inside new or updated programs.

Note that this implicitly relies on echo being a shell builtin, which
is common but not guaranteed (I don't think). It would work in bash or
dash, though.

If apparmor-do-stuff no longer works, and you have to wrap a shell
script around it, isn't that the same amount of user-space breakage as
if apparmor-do-stuff no longer works and you have to install a newer
version that does work? Either way, the sysadmin must take action to
change user-space components. I think the attr/display thing only reduces
the magnitude of the user-space changes required to catch up, and doesn't
eliminate the fact that those changes were needed.

> > Lots of programs (including dbus-daemon) fork-and-exec arbitrary
> > child processes that come from a different codebase not under our
> > control and aren't necessarily LSM-stacking-aware. I don't really want
> > to have to reset /proc/self/attr/display in our increasingly crowded
> > after-fork-but-before-exec code path
> 
> My hope is that new and updated programs will have to tools
> they need to get it right, and that those that don't won't
> fall over on a well configured system.

The problem I see here is that if we assume dbus-daemon is a new/updated
program that has set /proc/self/attr/display = "hideous" in order to get
the full stack of labels for its peer processes, then it will be causing
side-effects on its separately-maintained child processes - they will
no longer be able to benefit from the backwards-compatility thing where
/proc/self/attr/display (effectively) defaults to the first LSM that
has labels, because dbus-daemon overrode that (unless dbus-daemon takes
action to reverse it between fork and exec). This partially defeats the
semi-backwards-compatible handling of the existing kernel interfaces.

If dbus-daemon could read SO_PEERSECLABELS instead of SO_PEERSEC and
read /proc/<pid>/attr/current_stack instead of /proc/<pid>/attr/current,
leaving /proc/self/attr/display untouched, then this concern would go away.

Similarly, dbus-daemon can be linked to libselinux and/or libapparmor
(on Debian it's linked to both, even in the non-stackable present,
and the right one for the kernel configuration is chosen at runtime).
If one of those libraries wrote to /proc/self/attr/display, then the rest
of dbus-daemon's main thread and all child processes would implicitly be
getting the result of that - even if dbus-daemon itself had not yet been
updated for stacked LSMs (in which case it cannot be expected to reverse
their action between fork and exec, because it's an older codebase that
doesn't yet know that "big" LSMs can be stacked).

So I think libselinux and libapparmor should be enhanced to use
new kernel interfaces that get the label they want to get (either
just that label, or all the labels), instead of being enhanced to
write /proc/self/attr/display to change the meaning of old kernel
interfaces. Otherwise they can break other code in their process or
their subprocesses.

> > instead of repurposing /proc/<pid>/attr/current
> > and SO_PEERSEC to have contents that vary according to ambient process
> > state in their reader?
> 
> In addition, yes. Instead of? I don't think that we can have a
> backward compatibility story that flies without it.

Consider only SELinux and AppArmor for a moment (I know there are other
"big" LSMs like Smack, but this same reasoning applies to any pair, with
appropriate search-and-replace on their names).

Neither SELinux nor AppArmor: there are no labels, nothing changed.

AppArmor is the only "big" LSM in the stack (Ubuntu): previously,
the label was the AppArmor label; now, if attr/display is not altered,
the label is the one used by the first "big" LSM in the stack, which is
AppArmor. Nothing changed.

SELinux is the only "big" LSM in the stack (Red Hat): same as for AppArmor
being the only "big" LSM in the stack, but with s/AppArmor/SELinux/.

SELinux and AppArmor stacked: this is a situation that could not exist
before, so distro/sysadmin action must have been necessary to make it
happen. However much ambient process state is invented, I don't see any
way to make both SELinux and AppArmor user-space work without modifications:
at least one of them (the one that is second in the stack) has to use new
kernel interfaces, or alter attr/display to change the meaning of the old
kernel interfaces, or something similar, to get the second LSM's labels.
So distro/sysadmin action in user-space is also going to be necessary here
whatever happens - backward compatibility has already been broken, it's
only a question of how intrusive the user-space changes are. Is it really
so much worse if the distro/sysadmin action taken to update user-space
has to take the form of using new kernel interfaces that always do the
same thing, rather than changing the meaning of old kernel interfaces?

    smcv

Re: Dbus and multiple LSMs (was Preferred subj= with multiple LSMs)

[Casey Schaufler]

On 7/22/2019 4:36 AM, Simon McVittie wrote:
> On Fri, 19 Jul 2019 at 13:02:24 -0700, Casey Schaufler wrote:
>> On 7/19/2019 11:47 AM, Simon McVittie wrote:
>>> I was hoping the syscall wrappers in glibc would be a viable user-space
>>> interface to the small amount of LSM stuff that dbus needs to use in an
>>> LSM-agnostic way.
>> I don't see how to do that without making the Fedora and Ubuntu user space
>> environments [not] remain functional.
> What I was thinking of was a second, parallel kernel <-> user-space
> interface (like the SO_PEERSECLABELS that I suggested) for future/updated
> user-space components. SO_PEERSEC would continue to return some
> hopefully-backwards-compatible thing, but would be deprecated, because it
> cannot fully represent the reality of LSM stacking while remaining
> backwards-compatible.

I will propose SO_PEERCONTEXT and /proc/.../attr/stack/context,
both of which will use the Hideous format, in the next round. I
appreciate the suggestion and discussion.

>> I see display being used in scripts:
>>
>> 	echo apparmor > /proc/self/attr/display
>> 	apparmor-do-stuff --options --deamon
>>
>> much more than inside new or updated programs.
> Note that this implicitly relies on echo being a shell builtin, which
> is common but not guaranteed (I don't think). It would work in bash or
> dash, though.

Yes, echo being built-in can't be guaranteed. Most shells have some
way of doing the equivalent.

> If apparmor-do-stuff no longer works, and you have to wrap a shell
> script around it, isn't that the same amount of user-space breakage as
> if apparmor-do-stuff no longer works and you have to install a newer
> version that does work?

True when there is such a newer version. I'm sure you're aware
of how much system software out there hasn't been updated in this
century.

> Either way, the sysadmin must take action to
> change user-space components. I think the attr/display thing only reduces
> the magnitude of the user-space changes required to catch up, and doesn't
> eliminate the fact that those changes were needed.

Agreed. It's a tool for the times of transition.

>>> Lots of programs (including dbus-daemon) fork-and-exec arbitrary
>>> child processes that come from a different codebase not under our
>>> control and aren't necessarily LSM-stacking-aware. I don't really want
>>> to have to reset /proc/self/attr/display in our increasingly crowded
>>> after-fork-but-before-exec code path
>> My hope is that new and updated programs will have to tools
>> they need to get it right, and that those that don't won't
>> fall over on a well configured system.
> The problem I see here is that if we assume dbus-daemon is a new/updated
> program that has set /proc/self/attr/display = "hideous" in order to get
> the full stack of labels for its peer processes, then it will be causing
> side-effects on its separately-maintained child processes - they will
> no longer be able to benefit from the backwards-compatility thing where
> /proc/self/attr/display (effectively) defaults to the first LSM that
> has labels, because dbus-daemon overrode that (unless dbus-daemon takes
> action to reverse it between fork and exec). This partially defeats the
> semi-backwards-compatible handling of the existing kernel interfaces.

Point. /proc/self/attr/stack/context and SO_PEERCONTEXT comprise a better,
more reliable solution.

> If dbus-daemon could read SO_PEERSECLABELS instead of SO_PEERSEC and
> read /proc/<pid>/attr/current_stack instead of /proc/<pid>/attr/current,
> leaving /proc/self/attr/display untouched, then this concern would go away.

I agree.

> Similarly, dbus-daemon can be linked to libselinux and/or libapparmor
> (on Debian it's linked to both, even in the non-stackable present,
> and the right one for the kernel configuration is chosen at runtime).
> If one of those libraries wrote to /proc/self/attr/display, then the rest
> of dbus-daemon's main thread and all child processes would implicitly be
> getting the result of that - even if dbus-daemon itself had not yet been
> updated for stacked LSMs (in which case it cannot be expected to reverse
> their action between fork and exec, because it's an older codebase that
> doesn't yet know that "big" LSMs can be stacked).

Yes.

> So I think libselinux and libapparmor should be enhanced to use
> new kernel interfaces that get the label they want to get (either
> just that label, or all the labels), instead of being enhanced to
> write /proc/self/attr/display to change the meaning of old kernel
> interfaces. Otherwise they can break other code in their process or
> their subprocesses.

The AppArmor team is already moving away from using the /proc/self/attr
intefaces. /proc/self/attr/smack is already there, and the transition
begun. The SELinux developers seem firmly set in the position that there
is no reason they should ever change. In the long term I think we'll get
the conflict sorted out. It's hard to say what value of "long term"
we're looking at. 

>>> instead of repurposing /proc/<pid>/attr/current
>>> and SO_PEERSEC to have contents that vary according to ambient process
>>> state in their reader?
>> In addition, yes. Instead of? I don't think that we can have a
>> backward compatibility story that flies without it.
> Consider only SELinux and AppArmor for a moment (I know there are other
> "big" LSMs like Smack, but this same reasoning applies to any pair, with
> appropriate search-and-replace on their names).
>
> Neither SELinux nor AppArmor: there are no labels, nothing changed.
>
> AppArmor is the only "big" LSM in the stack (Ubuntu): previously,
> the label was the AppArmor label; now, if attr/display is not altered,
> the label is the one used by the first "big" LSM in the stack, which is
> AppArmor. Nothing changed.
>
> SELinux is the only "big" LSM in the stack (Red Hat): same as for AppArmor
> being the only "big" LSM in the stack, but with s/AppArmor/SELinux/.
>
> SELinux and AppArmor stacked: this is a situation that could not exist
> before, so distro/sysadmin action must have been necessary to make it
> happen. However much ambient process state is invented, I don't see any
> way to make both SELinux and AppArmor user-space work without modifications:
> at least one of them (the one that is second in the stack) has to use new
> kernel interfaces, or alter attr/display to change the meaning of the old
> kernel interfaces, or something similar, to get the second LSM's labels.
> So distro/sysadmin action in user-space is also going to be necessary here
> whatever happens - backward compatibility has already been broken, it's
> only a question of how intrusive the user-space changes are. Is it really
> so much worse if the distro/sysadmin action taken to update user-space
> has to take the form of using new kernel interfaces that always do the
> same thing, rather than changing the meaning of old kernel interfaces?

In addition to the big name distros/systems like RedHat, Ubuntu and
Android there are a bunch of smaller players who don't have the
expertise and/or staffing and/or upstream clout to update system
services. Some of these are targets for stacked LSMs. They will be
delighted to get updated programs, but will muddle through with the
compatibility mechanisms if they have to.

>     smcv

Thank you again for your insights on this topic. My next round
should provide what you've suggested.