Re: selinux_inode_setxattr oops.

From: Dave Jones
Date: Wed Jul 25 2012 - 11:47:57 EST

On Sat, Jun 09, 2012 at 08:15:16AM +0100, Al Viro wrote:
> On Mon, Jun 04, 2012 at 05:57:29PM -0400, Dave Jones wrote:
> > More syscall fuzzing fallout..
> >
> > BUG: unable to handle kernel paging request at ffffffffffffffff
> > IP: [<ffffffff812cf3f6>] selinux_inode_setxattr+0x196/0x200
> > PGD 1c0d067 PUD 1c0e067 PMD 0
> > Oops: 0000 [#1] SMP
> > CPU 0
> > Modules linked in: ipt_ULOG can_raw binfmt_misc bnep cmtp kernelcapi dccp_ipv4 dccp hidp af_802154 phonet bluetooth rfkill can pppoe pppox ppp_generic slhc irda crc_ccitt rds af_key rose ax25 atm appletalk ipx p8022 psnap llc p8023 nfs fscache auth_rpcgss nfs_acl lockd ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 xt_state nf_conntrack ip6table_filter ip6_tables btrfs dm_mirror dm_region_hash dm_log zlib_deflate libcrc32c coretemp kvm_intel kvm raid0 ppdev snd_hda_codec_idt dcdbas snd_hda_intel snd_hda_codec snd_hwdep snd_seq snd_seq_device snd_pcm snd_timer snd microcode soundcore pcspkr snd_page_alloc serio_raw i2c_i801 lpc_ich tg3 mfd_core i5000_edac edac_core i5k_amb parport_pc parport shpchp sunrpc firewire_ohci firewire_core crc_itu_t floppy nouveau ttm drm_kms_helper drm i2c_algo_bit i2c_core mxm_wmi video wmi [last unloaded: scsi_wait_scan]
> >
> > Pid: 12482, comm: trinity-child0 Not tainted 3.5.0-rc1+ #61 Dell Inc. Precision WorkStation 490 /0DT031
> > RIP: 0010:[<ffffffff812cf3f6>] [<ffffffff812cf3f6>] selinux_inode_setxattr+0x196/0x200
> > RSP: 0018:ffff8801f8103cd8 EFLAGS: 00010246
> > RAX: 0000000000000000 RBX: ffff8801f3c68860 RCX: 0000000000000021
> > RDX: 0000000000000000 RSI: 0000000000000020 RDI: 0000000000000000
> > RBP: ffff8801f8103d58 R08: 0000000000000000 R09: 0000000000000001
> > R10: 0000000000000000 R11: 0000000000000000 R12: ffff8801f519a480
> > R13: ffff88022333d550 R14: ffff8801f7e51720 R15: ffff8801f7e9d0b0
> > FS: 00007f1bc4538700(0000) GS:ffff880226600000(0000) knlGS:0000000000000000
> > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: ffffffffffffffff CR3: 00000001d39ee000 CR4: 00000000000007f0
> > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> > Process trinity-child0 (pid: 12482, threadinfo ffff8801f8102000, task ffff8801f519a480)
> > Stack:
> > ffff8801ffffffea 0000000000000000 0000000000000000 0000071f81021de9
> > ffff8801f8103d28 ffff8801f7e9d10a ffff8801f7e51720 0000000000000000
> > 2222222222222222 0000000022222222 2222222222222222 ffff8801f7e51720
> > Call Trace:
> > [<ffffffff812c8910>] security_inode_setxattr+0x20/0x30
> > [<ffffffff811e96a1>] vfs_setxattr+0x91/0xd0
> > [<ffffffff811e97d3>] setxattr+0xf3/0x1a0
> > [<ffffffff810cdba5>] ? lock_release_holdtime.part.9+0x15/0x1a0
> > [<ffffffff810938b1>] ? lock_hrtimer_base+0x31/0x60
> > [<ffffffff8106c9fe>] ? do_setitimer+0x18e/0x360
> > [<ffffffff816b6710>] ? _raw_spin_unlock_irq+0x30/0x50
> > [<ffffffff810d397d>] ? trace_hardirqs_on_caller+0x10d/0x1a0
> > [<ffffffff810d3a1d>] ? trace_hardirqs_on+0xd/0x10
> > [<ffffffff816b6710>] ? _raw_spin_unlock_irq+0x30/0x50
> > [<ffffffff811c47f9>] ? fget_light+0x3f9/0x4f0
> > [<ffffffff816bfc15>] ? sysret_check+0x22/0x5d
> > [<ffffffff811e9c3b>] sys_fsetxattr+0xbb/0xf0
> > [<ffffffff816bfbe9>] system_call_fastpath+0x16/0x1b
> > Code: 0f 1f 44 00 00 bf 21 00 00 00 89 45 80 e8 63 2f da ff 84 c0 75 5f 48 8b 55 90 48 8b 45 88 be 20 00 00 00 49 8b bc 24 d0 05 00 00 <80> 7c 02 ff 01 49 89 c5 ba 79 05 00 00 49 83 dd 00 e8 b4 77 e2
>
> How quaint... That looks *almost* like the only place in
> security_inode_setxattr() where I'd expect an access at that address, but...
> it's comparing with the wrong value. 80 7c 02 ff 01 is
> cmpb $0x1,-0x1(%rdx,%rax,1)
> and if not for that last 01 I would've definitely pointed to comparison in
>
> /* We strip a nul only if it is at the end, otherwise the
> * context contains a nul and we should audit that */
> str = value;
> if (str[size - 1] == '\0')
> audit_size = size - 1;
> else
> audit_size = size;
>
> but we are comparing with '\1', not '\0'... Very odd. Could you post
> disassembled security_inode_setxattr() from that kernel? In any case,
> that looks like a bug capable of producing such dereferences, if you
> can get there with size == 0. Look: security_context_to_sid() ends up
> doing
> /* Copy the string so that we can modify the copy as we parse it. */
> scontext2 = kmalloc(scontext_len + 1, gfp_flags);
> if (!scontext2)
> return -ENOMEM;
> memcpy(scontext2, scontext, scontext_len);
> scontext2[scontext_len] = 0;
> and doesn't dereference scontext ever after. If the things below that point
> end up returning -EINVAL, you'll end up with just that kind of oops.
>
> The question is, can we get there with value == NULL and size == 0? That
> would've meant vfs_setxattr(dentry, name, NULL, 0, flags)... and AFAICS
> sys_setxattr() does exactly that if it gets zero as "size" argument.
> So this is legitimate. I wonder why it doesn't trigger all the time,
> then...
>
> OK, what we have so far is e.g.
> setxattr(path, name, whatever, 0, XATTR_REPLACE)
> with name being good enough to get through xattr_permission().
> Then we reach security_inode_setxattr() with the desired value and size.
> Aha. name should begin with "security.selinux", or we won't get that
> far in selinux_inode_setxattr(). Suppose we got there and have enough
> permissions to relabel that sucker. We call security_context_to_sid()
> with value == NULL, size == 0. OK, we want ss_initialized to be non-zero.
> I.e. after everything had been set up and running. No problem...
>
> We do 1-byte kmalloc(), zero-length memcpy() (which doesn't oops, even
> thought the source is NULL) and put a NUL there. I.e. form an empty
> string. string_to_context_struct() is called and looks for the first
> ':' in there. Not found, -EINVAL we get. OK, security_context_to_sid_core()
> has rc == -EINVAL, force == 0, so it silently returns -EINVAL.
> All it takes now is not having CAP_MAC_ADMIN and we are fucked.
>
> All right, it might be a different bug (modulo strange code quoted in the
> report), but it's real. Easily fixed, AFAICS:
>
> Deal with size == 0, value == NULL case in selinux_inode_setxattr()
>
> Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> ---
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 372ec65..65df65f 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2792,11 +2792,16 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name,
>
> /* We strip a nul only if it is at the end, otherwise the
> * context contains a nul and we should audit that */
> - str = value;
> - if (str[size - 1] == '\0')
> - audit_size = size - 1;
> - else
> - audit_size = size;
> + if (value) {
> + str = value;
> + if (str[size - 1] == '\0')
> + audit_size = size - 1;
> + else
> + audit_size = size;
> + } else {
> + str = "";
> + audit_size = 0;
> + }
> ab = audit_log_start(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR);
> audit_log_format(ab, "op=setxattr invalid_context=");
> audit_log_n_untrustedstring(ab, value, audit_size);

Did this get queued up anywhere ?
I just stumbled across this still sitting in my tree. I've not seen the spew
from fuzzing since adding it, so I guess I can add my Tested-by: there.

Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/