linux-kernel - Re: capget() overflows buffers.

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:	Fri, 23 May 2008 08:57:18 -0700
From:	Chris Wright <chrisw@...s-sol.org>
To:	"Andrew G. Morgan" <morgan@...nel.org>
Cc:	Chris Wright <chrisw@...s-sol.org>,
	Dave Jones <davej@...emonkey.org.uk>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	bojan@...ursive.com, "Serge E. Hallyn" <serue@...ibm.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linux Security Modules List 
	<linux-security-module@...r.kernel.org>
Subject: Re: capget() overflows buffers.

* Andrew G. Morgan (morgan@...nel.org) wrote:
> Chris Wright wrote:
> |> The kernel is not crashing, the application is...
> |
> | It's not about crashing.  It's about app security.  Currently, there is
> | nothing guaranteeing named has actually dropped privileges.  That's why
> | I consider this serious.
>
> I have to say the details of this are not clear to me. Can you sketch an
> example?

Sure.  named is doing this:

caps is essentially
CAP_NET_BIND_SERVICE|CAP_SYS_CHROOT|CAP_SETGID|CAP_DAC_READ_SEARCH|CAP_SYS_RESOURCE

linux_setcaps(cap_t caps) {
	struct __user_cap_header_struct caphead;
	struct __user_cap_data_struct cap;	<-- 12 bytes on stack
	memset(&caphead, 0, sizeof(caphead));
	caphead.version = _LINUX_CAPABILITY_VERSION;  <-- v2 now
	caphead.pid = 0;
	memset(&cap, 0, sizeof(cap));		<-- start initializing 12 bytes
	cap.effective = caps;
	cap.permitted = caps;
	cap.inheritable = 0;			<-- finish initializing 12 bytes
	if (syscall(SYS_capset, &caphead, &cap) < 0) <-- kernel copies in 24 bytes

So it's blindly setting v2.  The kernel is sucking in 24 bytes.  The
second set of u32s (the extra 12 bytes) is just garbage from the stack.
This could include setting CAP_MAC_OVERRIDE or CAP_MAC_ADMIN, percisely
the kinds of things you _wouldn't_ want set when the goal of the above
code is to drop privs.

> Otherwise, I find myself generally persuaded..
>
> | Yes, as they should.  I don't think we should ever expect an existing
> | userspace program change just by recompiling against a new kernel header
> | when using an already existing mechanism.  Their app has been working
> | fine since 2.2.
> |
> |   int fd = open("foo", O_FLAGS, mode);
> |
> | compile once...binary compatible going forward (as is cap{s,g}et).
> | update kernel, recompile...source API comaptible...still working
> | (this is broken in cap{s,g}et).
>
> [I guess that this was what libcap was all about, and why there are so
> many comments about using it littered through the kernel... Oh well.]
>
> | History bites us...libcap wasn't always there.  As we see, people roll
> [...]
>
> Not to pick holes in your argument, but libcap *has* always been there.
> It was co-developed with the original kernel patches.

Yeah, that was confusing.  I didn't mean it hadn't been developed.
But all too often it wasn't picked up by distros.  And this is why apps
went the 'roll your own' direction.

> | No, the solution there would be to keep _LINUX_CAPABILITY_VERSION as
> | v1 (otherwise you just broke apps again).  And use another mechanism to
> | signal the availability of 64bit caps.
>
> Point taken. Patch attached.

Thanks, I'll comment on that in a different mail.
-chris
--
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/