[v4,00/23] LSM: Module stacking for AppArmor
mbox series

Message ID 20190626192234.11725-1-casey@schaufler-ca.com
Headers show
Series
  • LSM: Module stacking for AppArmor
Related show

Message

Casey Schaufler June 26, 2019, 7:22 p.m. UTC
This patchset provides the changes required for
the AppArmor security module to stack safely with any other.

Because of the changes to slot handling and the rework of
"display" I have not included the Reviewed-by tags from the
previous version.

v4: Incorporate feedback from v3
    - Mark new lsm_<blob>_alloc functions static
    - Replace the lsm and slot fields of the security_hook_list
      with a pointer to a LSM allocated lsm_id structure. The
      LSM identifies if it needs a slot explicitly. Use the
      lsm_id rather than make security_add_hooks return the
      slot value.
    - Validate slot values used in security.c
    - Reworked the "display" process attribute handling so that
      it works right and doesn't use goofy list processing.
    - fix display value check in dentry_init_security
    - Replace audit_log of secids with '?' instead of deleting
      the audit log

v3: Incorporate feedback from v2
    - Make lsmblob parameter and variable names more
      meaningful, changing "le" and "l" to "blob".
    - Improve consistency of constant naming.
    - Do more sanity checking during LSM initialization.
    - Be a bit clearer about what is temporary scaffolding.
    - Rather than clutter security_getpeersec_dgram with
      otherwise unnecessary checks remove the apparmor
      stub, which does nothing useful.

Patches 0001-0003 complete the process of moving managment
of security blobs that might be shared from the individual
modules to the infrastructure.

Patches 0004-0014 replace system use of a "secid" with
a structure "lsmblob" containing information from the
security modules to be held and reused later. At this
point lsmblob contains an array of u32 secids, one "slot"
for each of the security modules compiled into the
kernel that used secids. A "slot" is allocated when
a security module requests one.
The infrastructure is changed to use the slot number
to pass the correct secid to or from the security module
hooks.

It is important that the lsmblob be a fixed size entity
that does not have to be allocated. Several of the places
where it is used would have performance and/or locking
issues with dynamic allocation.

Patch 0015 provides a mechanism for a process to
identify which security module's hooks should be used
when displaying or converting a security context string.
A new interface /proc/.../attr/display contains the name
of the security module to show. Reading from this file
will present the name of the module, while writing to
it will set the value. Only names of active security
modules are accepted. Internally, the name is translated
to the appropriate "slot" number for the module which
is then stored in the task security blob.

Patch 0016 Starts the process of changing how a security
context is represented. Since it is possible for a
security context to have been generated by more than one
security module it is now necessary to note which module
created a security context so that the correct "release"
hook can be called. There are several places where the
module that created a security context cannot be inferred.

This is achieved by introducing a "lsmcontext" structure
which contains the context string, its length and the
"slot" number of the security module that created it.
The security_release_secctx() interface is changed,
replacing the (string,len) pointer pair with a lsmcontext
pointer.

Patches 0012-0021 convert the security interfaces from
(string,len) pointer pairs to a lsmcontext pointer.
The slot number identifying the creating module is
added by the infrastructure. Where the security context
is stored for extended periods the data type is changed.

The Netlabel code is converted to save lsmblob structures
instead of secids in Patch 0022.

Finally, with all interference on the AppArmor hooks
removed, Patch 0023 removes the exclusive bit from
AppArmor. An unnecessary stub hook was also removed.

The Ubuntu project is using an earlier version of
this patchset in their distribution to enable stacking
for containers.

Performance measurements to date have the change
within the "noise". Better benchmarks are in the
works.

https://github.com/cschaufler/lsm-stacking.git#stack-5.2-v4-apparmor

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---

Comments

Kees Cook June 26, 2019, 9:04 p.m. UTC | #1
On Wed, Jun 26, 2019 at 12:22:11PM -0700, Casey Schaufler wrote:
> This patchset provides the changes required for
> the AppArmor security module to stack safely with any other.
> 
> Because of the changes to slot handling and the rework of
> "display" I have not included the Reviewed-by tags from the
> previous version.
> 
> v4: Incorporate feedback from v3
>     - Mark new lsm_<blob>_alloc functions static
>     - Replace the lsm and slot fields of the security_hook_list
>       with a pointer to a LSM allocated lsm_id structure. The
>       LSM identifies if it needs a slot explicitly. Use the
>       lsm_id rather than make security_add_hooks return the
>       slot value.
>     - Validate slot values used in security.c
>     - Reworked the "display" process attribute handling so that
>       it works right and doesn't use goofy list processing.
>     - fix display value check in dentry_init_security
>     - Replace audit_log of secids with '?' instead of deleting
>       the audit log

I think you missed adding my and John's Reviewed-bys from v3?
John Johansen June 26, 2019, 9:11 p.m. UTC | #2
On 6/26/19 2:04 PM, Kees Cook wrote:
> On Wed, Jun 26, 2019 at 12:22:11PM -0700, Casey Schaufler wrote:
>> This patchset provides the changes required for
>> the AppArmor security module to stack safely with any other.
>>


here v

>> Because of the changes to slot handling and the rework of
>> "display" I have not included the Reviewed-by tags from the
>> previous version.


>>
>> v4: Incorporate feedback from v3
>>     - Mark new lsm_<blob>_alloc functions static
>>     - Replace the lsm and slot fields of the security_hook_list
>>       with a pointer to a LSM allocated lsm_id structure. The
>>       LSM identifies if it needs a slot explicitly. Use the
>>       lsm_id rather than make security_add_hooks return the
>>       slot value.
>>     - Validate slot values used in security.c
>>     - Reworked the "display" process attribute handling so that
>>       it works right and doesn't use goofy list processing.
>>     - fix display value check in dentry_init_security
>>     - Replace audit_log of secids with '?' instead of deleting
>>       the audit log
> 
> I think you missed adding my and John's Reviewed-bys from v3?
> 
Casey stated why above
Casey Schaufler June 26, 2019, 9:25 p.m. UTC | #3
On 6/26/2019 2:04 PM, Kees Cook wrote:
> On Wed, Jun 26, 2019 at 12:22:11PM -0700, Casey Schaufler wrote:
>> This patchset provides the changes required for
>> the AppArmor security module to stack safely with any other.
>>
>> Because of the changes to slot handling and the rework of
>> "display" I have not included the Reviewed-by tags from the
>> previous version.
>>
>> v4: Incorporate feedback from v3
>>     - Mark new lsm_<blob>_alloc functions static
>>     - Replace the lsm and slot fields of the security_hook_list
>>       with a pointer to a LSM allocated lsm_id structure. The
>>       LSM identifies if it needs a slot explicitly. Use the
>>       lsm_id rather than make security_add_hooks return the
>>       slot value.
>>     - Validate slot values used in security.c
>>     - Reworked the "display" process attribute handling so that
>>       it works right and doesn't use goofy list processing.
>>     - fix display value check in dentry_init_security
>>     - Replace audit_log of secids with '?' instead of deleting
>>       the audit log
> I think you missed adding my and John's Reviewed-bys from v3?

See the sentence just before "v4:". I thought that the changes
where sufficient to require re-review. If you don't think they
are, I will happily accept the Reviewed-bys.
Kees Cook June 26, 2019, 11:04 p.m. UTC | #4
On Wed, Jun 26, 2019 at 02:11:23PM -0700, John Johansen wrote:
> On 6/26/19 2:04 PM, Kees Cook wrote:
> > On Wed, Jun 26, 2019 at 12:22:11PM -0700, Casey Schaufler wrote:
> >> This patchset provides the changes required for
> >> the AppArmor security module to stack safely with any other.
> >>
> 
> 
> here v
> 
> >> Because of the changes to slot handling and the rework of
> >> "display" I have not included the Reviewed-by tags from the
> >> previous version.
> 
> 
> >>
> >> v4: Incorporate feedback from v3
> >>     - Mark new lsm_<blob>_alloc functions static
> >>     - Replace the lsm and slot fields of the security_hook_list
> >>       with a pointer to a LSM allocated lsm_id structure. The
> >>       LSM identifies if it needs a slot explicitly. Use the
> >>       lsm_id rather than make security_add_hooks return the
> >>       slot value.
> >>     - Validate slot values used in security.c
> >>     - Reworked the "display" process attribute handling so that
> >>       it works right and doesn't use goofy list processing.
> >>     - fix display value check in dentry_init_security
> >>     - Replace audit_log of secids with '?' instead of deleting
> >>       the audit log
> > 
> > I think you missed adding my and John's Reviewed-bys from v3?
> > 
> Casey stated why above

Oops! Thanks! I skimmed too fast and only read the "v4" log. :P
James Morris June 27, 2019, 2:41 a.m. UTC | #5
On Wed, 26 Jun 2019, Casey Schaufler wrote:

> This patchset provides the changes required for
> the AppArmor security module to stack safely with any other.

I get a kernel oops with this patchset when running the SELinux testsuite 
(binder test) with:

$ cat /sys/kernel/security/lsm 
capability,yama,loadpin,safesetid,selinux,tomoyo


[  485.357377] binder: 4224 RLIMIT_NICE not set
[  485.360727] binder: 4224 RLIMIT_NICE not set
[  485.361480] binder: 4224 RLIMIT_NICE not set
[  485.362164] BUG: unable to handle kernel paging request at 0000000000001080
[  485.362927] #PF error: [normal kernel read fault]
[  485.363143] ------------[ cut here ]------------
[  485.363581] PGD 800000044e17b067 P4D 800000044e17b067 PUD 44b796067 PMD 0 
[  485.364226] kernel BUG at drivers/android/binder_alloc.c:1139!
[  485.364865] Oops: 0000 [#1] SMP PTI
[  485.366430] CPU: 1 PID: 4224 Comm: manager Not tainted 5.1.0+ #7
[  485.367290] Hardware name: LENOVO 20HGS3KS0S/20HGS3KS0S, BIOS N1WET44W (1.23 ) 01/24/2018
[  485.367900] RIP: 0010:binder_alloc_do_buffer_copy+0x88/0x210
[  485.368515] Code: 00 65 48 8b 2c 25 00 5c 01 00 41 bd 00 10 00 00 48 89 
eb eb 3d 83 f8 08 0f 83 e3 00 00 00 a8 04 0f 
85 45 01 00 00 85 c0 74 0e <41> 0f b6 08 88 0e a8 02 0f 85 5d 01 00 00 83 
ab a8 19 00 00 01 49
[  485.369170] RSP: 0018:ffffaf3ac1f9bb88 EFLAGS: 00010202
[  485.369804] RAX: 0000000000000002 RBX: ffff8d3c84340000 RCX: 0000000000000000
[  485.370470] RDX: ffff8d3c8db74cc0 RSI: ffff8d3c8b425000 RDI: ffff8d3c89844978
[  485.371132] RBP: ffff8d3c84340000 R08: 0000000000001080 R09: 0000000000000002
[  485.371887] R10: 0000000000000000 R11: ffff8d3c89844978 R12: 0000000000000001
[  485.372656] R13: 0000000000001000 R14: ffff8d3c865d6300 R15: ffffffffa1a719c8
[  485.373340] FS:  00007fae657a8680(0000) GS:ffff8d3c91480000(0000) knlGS:0000000000000000
[  485.374017] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  485.374710] CR2: 0000000000001080 CR3: 000000044d482002 CR4: 00000000003606e0
[  485.375423] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  485.376122] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[  485.376823] Call Trace:
[  485.377514]  binder_transaction+0x371/0x2320
[  485.378231]  ? printk+0x58/0x6f
[  485.378940]  ? common_lsm_audit+0x162/0x800
[  485.379641]  ? __check_object_size+0x41/0x15d
[  485.380347]  ? binder_thread_read+0x9e4/0x1460
[  485.381065]  ? binder_update_ref_for_handle+0x83/0x1a0
[  485.381759]  binder_thread_write+0x2ae/0xfc0
[  485.382472]  ? tomoyo_path_number_perm+0x66/0x1d0
[  485.383150]  ? finish_wait+0x80/0x80
[  485.383839]  binder_ioctl+0x659/0x836
[  485.384531]  do_vfs_ioctl+0x405/0x660
[  485.385194]  ? __fput+0x157/0x230
[  485.385850]  ksys_ioctl+0x5e/0x90
[  485.386473]  __x64_sys_ioctl+0x16/0x20
[  485.387137]  do_syscall_64+0x5b/0x150
[  485.387782]  entry_SYSCALL_64_after_hwframe+0x44/0xa9


Looks to be:
(gdb) list *(binder_alloc_do_buffer_copy + 0x88)
0xffffffff817e2cb8 is in binder_alloc_do_buffer_copy 
(./include/linux/string.h:355).
350			if (q_size < size)
351				__read_overflow2();
352		}
353		if (p_size < size || q_size < size)
354			fortify_panic(__func__);
355		return __builtin_memcpy(p, q, size);
356	}
357
James Morris June 27, 2019, 2:46 a.m. UTC | #6
On Thu, 27 Jun 2019, James Morris wrote:

> On Wed, 26 Jun 2019, Casey Schaufler wrote:
> 
> > This patchset provides the changes required for
> > the AppArmor security module to stack safely with any other.
> 
> I get a kernel oops with this patchset when running the SELinux testsuite 
> (binder test) with:
> 
> $ cat /sys/kernel/security/lsm 
> capability,yama,loadpin,safesetid,selinux,tomoyo
> 
> 
> [  485.357377] binder: 4224 RLIMIT_NICE not set
> [  485.360727] binder: 4224 RLIMIT_NICE not set
> [  485.361480] binder: 4224 RLIMIT_NICE not set
> [  485.362164] BUG: unable to handle kernel paging request at 0000000000001080
> [  485.362927] #PF error: [normal kernel read fault]
> [  485.363143] ------------[ cut here ]------------
> [  485.363581] PGD 800000044e17b067 P4D 800000044e17b067 PUD 44b796067 PMD 0 
> [  485.364226] kernel BUG at drivers/android/binder_alloc.c:1139!

It's this BUG_ON:

static void binder_alloc_do_buffer_copy(struct binder_alloc *alloc,
                                        bool to_buffer,
                                        struct binder_buffer *buffer,
                                        binder_size_t buffer_offset,
                                        void *ptr,
                                        size_t bytes)
{
        /* All copies must be 32-bit aligned and 32-bit size */
        BUG_ON(!check_buffer(alloc, buffer, buffer_offset, bytes));
James Morris June 27, 2019, 3:45 a.m. UTC | #7
On Thu, 27 Jun 2019, James Morris wrote:

> On Thu, 27 Jun 2019, James Morris wrote:
> 
> > On Wed, 26 Jun 2019, Casey Schaufler wrote:
> > 
> > > This patchset provides the changes required for
> > > the AppArmor security module to stack safely with any other.
> > 
> > I get a kernel oops with this patchset when running the SELinux testsuite 
> > (binder test) with:
> > 
> > $ cat /sys/kernel/security/lsm 
> > capability,yama,loadpin,safesetid,selinux,tomoyo

Confirming there's no oops when Tomoyo is un-selected in the kernel 
config.
Kees Cook June 27, 2019, 3:51 a.m. UTC | #8
On Thu, Jun 27, 2019 at 12:46:01PM +1000, James Morris wrote:
> On Thu, 27 Jun 2019, James Morris wrote:
> 
> > On Wed, 26 Jun 2019, Casey Schaufler wrote:
> > 
> > > This patchset provides the changes required for
> > > the AppArmor security module to stack safely with any other.
> > 
> > I get a kernel oops with this patchset when running the SELinux testsuite 
> > (binder test) with:
> > 
> > $ cat /sys/kernel/security/lsm 
> > capability,yama,loadpin,safesetid,selinux,tomoyo
> > 
> > 
> > [  485.357377] binder: 4224 RLIMIT_NICE not set
> > [  485.360727] binder: 4224 RLIMIT_NICE not set
> > [  485.361480] binder: 4224 RLIMIT_NICE not set
> > [  485.362164] BUG: unable to handle kernel paging request at 0000000000001080
> > [  485.362927] #PF error: [normal kernel read fault]
> > [  485.363143] ------------[ cut here ]------------
> > [  485.363581] PGD 800000044e17b067 P4D 800000044e17b067 PUD 44b796067 PMD 0 
> > [  485.364226] kernel BUG at drivers/android/binder_alloc.c:1139!
> 
> It's this BUG_ON:
> 
> static void binder_alloc_do_buffer_copy(struct binder_alloc *alloc,
>                                         bool to_buffer,
>                                         struct binder_buffer *buffer,
>                                         binder_size_t buffer_offset,
>                                         void *ptr,
>                                         size_t bytes)
> {
>         /* All copies must be 32-bit aligned and 32-bit size */
>         BUG_ON(!check_buffer(alloc, buffer, buffer_offset, bytes));

Before:

        if (secctx) {
                size_t buf_offset = ALIGN(tr->data_size, sizeof(void *)) +
                                    ALIGN(tr->offsets_size, sizeof(void *)) +
                                    ALIGN(extra_buffers_size, sizeof(void *)) -
                                    ALIGN(secctx_sz, sizeof(u64));

                t->security_ctx = (uintptr_t)t->buffer->user_data + buf_offset;
                binder_alloc_copy_to_buffer(&target_proc->alloc,
                                            t->buffer, buf_offset,
                                            secctx, secctx_sz);
                security_release_secctx(secctx, secctx_sz);
                secctx = NULL;
        }

After:

        if (lsmctx.context) {
                size_t buf_offset = ALIGN(tr->data_size, sizeof(void *)) +
                                    ALIGN(tr->offsets_size, sizeof(void *)) +
                                    ALIGN(extra_buffers_size, sizeof(void *)) -
                                    ALIGN(lsmctx.len, sizeof(u64));

                t->security_ctx = (uintptr_t)t->buffer->user_data + buf_offset;
                binder_alloc_copy_to_buffer(&target_proc->alloc,
                                            t->buffer, buf_offset,
                                            lsmctx.context, lsmctx.len);
                security_release_secctx(&lsmctx);
        }

Which changes the "src" and "bytes" argument, assuming the size
calculation for buf_offset is the same. But, a quick shows:

-       char *secctx = NULL;
-       u32 secctx_sz = 0;
+       struct lsmcontext lsmctx;
...
-       if (secctx) {
+       if (lsmctx.context) {

lsmctx.context isn't being initialized, and it was passed by reference,
so compiler won't complain. I'll find the patch and comment. Totally
missed it in review!
James Morris June 27, 2019, 4:10 a.m. UTC | #9
On Thu, 27 Jun 2019, James Morris wrote:

> Confirming there's no oops when Tomoyo is un-selected in the kernel 
> config.

n/m, the problem is still there.
Kees Cook June 27, 2019, 5:07 p.m. UTC | #10
On Thu, Jun 27, 2019 at 02:10:18PM +1000, James Morris wrote:
> On Thu, 27 Jun 2019, James Morris wrote:
> 
> > Confirming there's no oops when Tomoyo is un-selected in the kernel 
> > config.
> 
> n/m, the problem is still there.

Were you able to test my fix for this? I wonder if what I found was just
a coincidence.
James Morris June 27, 2019, 6:10 p.m. UTC | #11
On Thu, 27 Jun 2019, Kees Cook wrote:

> On Thu, Jun 27, 2019 at 02:10:18PM +1000, James Morris wrote:
> > On Thu, 27 Jun 2019, James Morris wrote:
> > 
> > > Confirming there's no oops when Tomoyo is un-selected in the kernel 
> > > config.
> > 
> > n/m, the problem is still there.
> 
> Were you able to test my fix for this? I wonder if what I found was just
> a coincidence.

Seems to have fixed the oops I was seeing.


diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 4adf4d4a954b..e76dbeee979b 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2875,7 +2875,7 @@ static void binder_transaction(struct binder_proc *proc,
        binder_size_t last_fixup_min_off = 0;
        struct binder_context *context = proc->context;
        int t_debug_id = atomic_inc_return(&binder_last_id);
-       struct lsmcontext lsmctx;
+       struct lsmcontext lsmctx = {};
 
        e = binder_transaction_log_add(&binder_transaction_log);
        e->debug_id = t_debug_id;
John Johansen June 27, 2019, 9:52 p.m. UTC | #12
On 6/26/19 12:22 PM, Casey Schaufler wrote:
> This patchset provides the changes required for
> the AppArmor security module to stack safely with any other.
> 

I have been doing some testing of this with Casey's suggested
fix of clearing the lsmcontext in security_secid_to_secctx().

So far things are looking good. I have done smoke testing
on booting with the following combinations under an ubuntu
image. For the combinations that have apparmor I ran the
apparmor regression tests, where noted the display LSM
was set for the apparmor regression tests because they
are currently only testing the shared interface.

capability
yama
capability,yama
capability,yama,apparmor
capability,yama,selinux (no selinux policy)
capability,yama,apparmor,selinux (no selinux policy)
capability,yama,selinux,apparmor (no selinux policy) (tests that use shared interfaces fail without display LSM set, pass with it set to apparmor)
capability,yama,smack (no smack policy)
capability,yama,apparmor,smack (no smack policy)
capability,yama,smack,apparmor (no smack policy) (tests that use shared interfaces fail without display LSM set, pass with it set to apparmor)


I have more test combinations churning but figure I could report what I have so far
Casey Schaufler June 27, 2019, 10:33 p.m. UTC | #13
On 6/27/2019 2:52 PM, John Johansen wrote:
> On 6/26/19 12:22 PM, Casey Schaufler wrote:
>> This patchset provides the changes required for
>> the AppArmor security module to stack safely with any other.
>>
> I have been doing some testing of this with Casey's suggested
> fix of clearing the lsmcontext in security_secid_to_secctx().

There are still cases where the lsmcontext needs local
initialization. If security_<fillscontext> isn't called,
and code later looks for context.context == NULL you can
get bitten. I am combing for those cases and will include
initializing them in v5.

>
> So far things are looking good. I have done smoke testing
> on booting with the following combinations under an ubuntu
> image. For the combinations that have apparmor I ran the
> apparmor regression tests, where noted the display LSM
> was set for the apparmor regression tests because they
> are currently only testing the shared interface.
>
> capability
> yama
> capability,yama
> capability,yama,apparmor
> capability,yama,selinux (no selinux policy)
> capability,yama,apparmor,selinux (no selinux policy)
> capability,yama,selinux,apparmor (no selinux policy) (tests that use shared interfaces fail without display LSM set, pass with it set to apparmor)
> capability,yama,smack (no smack policy)
> capability,yama,apparmor,smack (no smack policy)
> capability,yama,smack,apparmor (no smack policy) (tests that use shared interfaces fail without display LSM set, pass with it set to apparmor)
>
>
> I have more test combinations churning but figure I could report what I have so far
>
>
James Morris June 27, 2019, 11:16 p.m. UTC | #14
On Thu, 27 Jun 2019, John Johansen wrote:

> I have more test combinations churning but figure I could report what I have so far

Do you have any way to test the nested scenario of say an AppArmor host 
with SELinux running in containers?
John Johansen June 27, 2019, 11:44 p.m. UTC | #15
On 6/27/19 4:16 PM, James Morris wrote:
> On Thu, 27 Jun 2019, John Johansen wrote:
> 
>> I have more test combinations churning but figure I could report what I have so far
> 
> Do you have any way to test the nested scenario of say an AppArmor host 
> with SELinux running in containers?
> 


No, an selinux container doesn't really work atm. The issue is to do with
namespacing. I can boot an AppArmor host with selinux enabled, but the
container loading selinux policy gets interesting, and without namespacing
the container policy affects the host.

It is of course possible to label the system such that you can sort of
make it work, but it isn't really practical.

I have played with the selinuxns branch trying to get this to work, but
I ran into some issues I couldn't resolve. However it has been five
months since I tried that so I can look at it again.


The AppArmor container on an selinux host case is easier partly because of
how policy is applied, partly because namespacing of its policy is already
supported upstream, and partly because I just know it better.

I do have plans to test the apparmor container on selinux but I haven't
gotten that far and am planning on waiting for this one until Casey kicks
out v5.