[PATCH] kernel: auditfilter: Fix a possible null-pointer dereference in audit_watch_path()

[Jia-Ju Bai]

In audit_find_rule(), there is an if statement on line 894 to check
whether entry->rule.watch is NULL:
else if (entry->rule.watch)

If entry->rule.watch is NULL, audit_compare_rule on 910 is called:
audit_compare_rule(&entry->rule, &e->rule))

In audit_compare_rule(), a->watch is used on line 720:
if (strcmp(audit_watch_path(a->watch), ...)

In this case, a->watch is NULL, and audit_watch_path() will use:
watch->path

Thus, a possible null-pointer dereference may occur in this case.

To fix this possible bug, an if statement is added in
audit_compare_rule() to check a->watch before using a->watch.

This bug is found by a static analysis tool STCheck written by us.

Signed-off-by: Jia-Ju Bai 
---
 kernel/auditfilter.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index b0126e9c0743..b0ad17b14609 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -717,6 +717,8 @@ static int audit_compare_rule(struct audit_krule *a, struct 
audit_krule *b)
return 1;
break;
case AUDIT_WATCH:
+   if (!a->watch)
+   break;
if (strcmp(audit_watch_path(a->watch),
   audit_watch_path(b->watch)))
return 1;
-- 
2.17.0

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//attr/current_stack instead of /proc//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//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 am

Re: Preferred subj= with multiple LSMs

[Simon McVittie]

On Mon, 22 Jul 2019 at 18:30:35 -0400, Paul Moore wrote:
> On Mon, Jul 22, 2019 at 6:01 PM Casey Schaufler  
> wrote:
> > I suggest that if supporting dbus well is assisted by
> > making reasonable restrictions on what constitutes a valid LSM
> > "context" that we have a good reason.
> 
> I continue to believe that restrictions on the label format are a bad
> idea

Does this include the restriction "the label does not include \0",
which is an assumption that dbus is already relying on since I checked
it in the thread around
?
Or is that restriction so fundamental that it's considered OK?

(Other user-space tools like ls -Z and ps -Z also rely on that assumption
by printing security contexts with %s, as far as I know.)

dbus does not require a way to multiplex multiple LSMs' labels in a
printable text string, so from my point of view, multiplexed labels do
not necessarily have to be in what Casey calls the "Hideous" format,
or in any text format at all: anything with documented rules for parsing
(including the unescaping that readers are expected to apply, if there
is any) would be fine. Based on the assumption of no "\0", I previously
suggested a "\0"-delimited encoding similar to /proc/self/cmdline, which
would not need any escaping/unescaping:

"apparmor\0"  "\0"
"selinux\0"  "\0"
...
"\0" (or this could be omitted since it's redundant with the length)

which would be fine (indeed, actually easier than the "Hideous" format)
from dbus' point of view.

dbus does not strictly need reading security labels for sockets or
processes to be atomic, either: it would be OK if we can get the complete
list of LSM labels *somehow*, possibly in O(number of LSMs) rather than
O(1) syscalls (although I'd prefer O(1)).

However, the getsockopt() interface only lets the kernel return one thing
per socket option, and I assume the networking maintainers probably don't
want to have to add SO_PEERAPPARMOR, SO_PEERSELINUX... for each LSM -
so this part at least would probably be easier as a single blob in some
text or binary format.

smcv

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: Preferred subj= with multiple LSMs

[Casey Schaufler]

On 7/23/2019 7:06 AM, Simon McVittie wrote:
> On Mon, 22 Jul 2019 at 18:30:35 -0400, Paul Moore wrote:
>> On Mon, Jul 22, 2019 at 6:01 PM Casey Schaufler  
>> wrote:
>>> I suggest that if supporting dbus well is assisted by
>>> making reasonable restrictions on what constitutes a valid LSM
>>> "context" that we have a good reason.
>> I continue to believe that restrictions on the label format are a bad
>> idea
> Does this include the restriction "the label does not include \0",
> which is an assumption that dbus is already relying on since I checked
> it in the thread around
> ?
> Or is that restriction so fundamental that it's considered OK?
>
> (Other user-space tools like ls -Z and ps -Z also rely on that assumption
> by printing security contexts with %s, as far as I know.)

The "-Z" options for ls and ps are unfortunately hard coded for SELinux.
For applications to be general in the presence of LSMs you are correct
that there need to be some assumptions.

> dbus does not require a way to multiplex multiple LSMs' labels in a
> printable text string, so from my point of view, multiplexed labels do
> not necessarily have to be in what Casey calls the "Hideous" format,
> or in any text format at all: anything with documented rules for parsing
> (including the unescaping that readers are expected to apply, if there
> is any) would be fine. Based on the assumption of no "\0", I previously
> suggested a "\0"-delimited encoding similar to /proc/self/cmdline, which
> would not need any escaping/unescaping:
>
> "apparmor\0"  "\0"
> "selinux\0"  "\0"
> ...
> "\0" (or this could be omitted since it's redundant with the length)
>
> which would be fine (indeed, actually easier than the "Hideous" format)
> from dbus' point of view.

I am an advocate of a single string due to the preponderance of
scripting language programing in today's world. It would be easy to provide
a library function to transform the "Hideous" format into the "cmdline"
format or, I'll admit, the other way round. I'm not so set in my opinion
that if it came down to "cmdline" or nothing I wouldn't cave in.

> dbus does not strictly need reading security labels for sockets or
> processes to be atomic, either: it would be OK if we can get the complete
> list of LSM labels *somehow*, possibly in O(number of LSMs) rather than
> O(1) syscalls (although I'd prefer O(1)).

Stephen Smalley did an excellent job of outlining the dangers of
using the proposed "display" mechanism with multiple calls to
get the complete attribute set. Adding a new interface that gets
them all at once addresses that set of problems.

> However, the getsockopt() interface only lets the kernel return one thing
> per socket option, and I assume the networking maintainers probably don't
> want to have to add SO_PEERAPPARMOR, SO_PEERSELINUX... for each LSM -

Or a getsockopt() option that takes an LSM name and returns the value
for that module. You could do any of those, but you still end up with O(n)
and a need to know in advance what security modules to look for.

> so this part at least would probably be easier as a single blob in some
> text or binary format.

For the long term I agree. I still have to deal with legacy services
and applications that won't be updated in the foreseeable future, which
is why the old interfaces can't be updated. New interfaces are required.
I'm open to discussion on details, including format.

> smcv


--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: Preferred subj= with multiple LSMs

[James Morris]

On Tue, 23 Jul 2019, Simon McVittie wrote:

> On Mon, 22 Jul 2019 at 18:30:35 -0400, Paul Moore wrote:
> > On Mon, Jul 22, 2019 at 6:01 PM Casey Schaufler  
> > wrote:
> > > I suggest that if supporting dbus well is assisted by
> > > making reasonable restrictions on what constitutes a valid LSM
> > > "context" that we have a good reason.
> > 
> > I continue to believe that restrictions on the label format are a bad
> > idea
> 
> Does this include the restriction "the label does not include \0",
> which is an assumption that dbus is already relying on since I checked
> it in the thread around
> ?
> Or is that restriction so fundamental that it's considered OK?

Security labels are strings, so this is implied.


-- 
James Morris


--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit