| Message ID | 20190626192234.11725-1-casey@schaufler-ca.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | LSM: Module stacking for AppArmor | expand |
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?
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
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.
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
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
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));
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.
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!
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.
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.
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;
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
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 > >
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?
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.
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> ---