lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Wed, 25 Jul 2012 11:47:39 -0400
From:	Dave Jones <davej@...hat.com>
To:	Al Viro <viro@...IV.linux.org.uk>
Cc:	Linux Kernel <linux-kernel@...r.kernel.org>, sds@...ho.nsa.gov,
	james.l.morris@...cle.com, eparis@...isplace.org
Subject: Re: selinux_inode_setxattr oops.

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@...iv.linux.org.uk>
 > ---
 > 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ