capget() overflows buffers.

capget() overflows buffers.

[Dave Jones]

We had a user file a bug report recently that sys_capget is overflowing
a user buffer.

More details and test program are available at
https://bugzilla.redhat.com/show_bug.cgi?id=447518

The only recent change in this area was e338d263a76af78fe8f38a72131188b58fceb591
"Add 64-bit capability support to the kernel".

	Dave


-- 
http://www.codemonkey.org.uk

Re: capget() overflows buffers.

[Chris Wright]

* Dave Jones (davej@codemonkey.org.uk) wrote:
> We had a user file a bug report recently that sys_capget is overflowing
> a user buffer.
> 
> More details and test program are available at
> https://bugzilla.redhat.com/show_bug.cgi?id=447518
> 
> The only recent change in this area was e338d263a76af78fe8f38a72131188b58fceb591
> "Add 64-bit capability support to the kernel".

Yes, this thing is broken.

Trouble is, it's expecting an array of 2, and getting an array of 1.

The userspace fix is to do this (note, this does not fix the fact that
the ABI is broken, it's so opaque that it's difficult to follow).

cap_user_data_t data=malloc(sizeof(*data)*_LINUX_CAPABILITY_U32S);

Bojan, is there a capset involved as well, because that will pull in
garbage and set caps accordingly?

Re: capget() overflows buffers.

[Chris Wright]

* Chris Wright (chrisw@sous-sol.org) wrote:
> Yes, this thing is broken.

Andrew, I think this should be considered a serious problem.  The
interface ABI is stable for old programs, and fine for anything new
or old that's using libcap.  But the API has changed subtly (taking a
pointer to a blob, to a pointer to an array of blobs), and is easily
broken for programs recompiled against new headers not using libcap.

For the squid issue at least it does capget/capset, so it's likely to
write back in capset the caps it got in capget (when it doesn't hit
glibc heap overflow protection).

But bind, for example, could have garbage in the upper 32bits on a 64bit
caps system that does not HAVE_LIBCAP:

(Note: snipped it down to make it readable, removed some ifdef
HAVE_LIBCAP, etc)

linux_setcaps(cap_t caps) {
	struct __user_cap_header_struct caphead;
	struct __user_cap_data_struct cap;	<-- just one set of u32s
<snip>
	memset(&caphead, 0, sizeof(caphead));
	caphead.version = _LINUX_CAPABILITY_VERSION; <-- v2
	caphead.pid = 0;
	memset(&cap, 0, sizeof(cap));
	cap.effective = caps;
	cap.permitted = caps;
	cap.inheritable = 0;			<-- fill in just that set
<snip>
	if (syscall(SYS_capset, &caphead, &cap) < 0) {
                                          ^^^ kernel pulls 2 sets of
					  u32s, send is just junk from
					  stack



For the squid case that Bojan described:
(Note: snipped it down again)

restoreCapabilities(int keep)
{
    cap_user_header_t head = (cap_user_header_t) xcalloc(1, sizeof(cap_user_header_t));
    cap_user_data_t cap = (cap_user_data_t) xcalloc(1, sizeof(cap_user_data_t));
    head->version = _LINUX_CAPABILITY_VERSION;
    if (capget(head, cap) != 0) {
<snip>
    head->pid = 0;
    cap->inheritable = 0;
    cap->effective = (1 << CAP_NET_BIND_SERVICE);
<snip>
    if (!keep)
        cap->permitted &= cap->effective;
    if (capset(head, cap) != 0) {

I don't see a nice solution, short reverting, and adding a new set of
syscalls to support 64-bit.

thanks,
-chris

Re: capget() overflows buffers.

[Andrew G. Morgan]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Chris Wright wrote:
| * Chris Wright (chrisw@sous-sol.org) wrote:
|> Yes, this thing is broken.
|
| Andrew, I think this should be considered a serious problem.  The
| interface ABI is stable for old programs, and fine for anything new
| or old that's using libcap.  But the API has changed subtly (taking a
| pointer to a blob, to a pointer to an array of blobs), and is easily
| broken for programs recompiled against new headers not using libcap.

[There is a warning about this issue in the kernel header file.]

The kernel is not crashing, the application is...

If you take this compiled binary, that crashes on 2.6.25, and try to run
it on 2.6.24 it will fail there too - since the magic its 'forcing' is
not valid on that kernel. So the compiled 'binary' we're discussing does
not have an existence proof that it will successfully run anywhere.

In point of fact, the kernel is binary compatible with old binaries. The
problem is that _LINUX_CAPABILITY_VERSION #define now points to _2
instead of _1 by default and Squid etc., are not paying attention to the
value of the new magic cookie but expecting the previous revision of the
ABI to work.

As such, I don't agree that there is a problem with the ABI, and I don't
agree with your assertion about things being broken. I maintain there is
a problem with the application source code.

One 'solution' is to force everyone to notice at compile time by simply
removing the definition of _LINUX_CAPABILITY_VERSION and force all new
source code to be explicit about which ABI it wants to use...

Cheers

Andrew

| For the squid issue at least it does capget/capset, so it's likely to
| write back in capset the caps it got in capget (when it doesn't hit
| glibc heap overflow protection).
|
| But bind, for example, could have garbage in the upper 32bits on a 64bit
| caps system that does not HAVE_LIBCAP:
|
| (Note: snipped it down to make it readable, removed some ifdef
| HAVE_LIBCAP, etc)
|
| linux_setcaps(cap_t caps) {
| 	struct __user_cap_header_struct caphead;
| 	struct __user_cap_data_struct cap;	<-- just one set of u32s
| <snip>
| 	memset(&caphead, 0, sizeof(caphead));
| 	caphead.version = _LINUX_CAPABILITY_VERSION; <-- v2
| 	caphead.pid = 0;
| 	memset(&cap, 0, sizeof(cap));
| 	cap.effective = caps;
| 	cap.permitted = caps;
| 	cap.inheritable = 0;			<-- fill in just that set
| <snip>
| 	if (syscall(SYS_capset, &caphead, &cap) < 0) {
|                                           ^^^ kernel pulls 2 sets of
| 					  u32s, send is just junk from
| 					  stack
|
|
|
| For the squid case that Bojan described:
| (Note: snipped it down again)
|
| restoreCapabilities(int keep)
| {
|     cap_user_header_t head = (cap_user_header_t) xcalloc(1,
sizeof(cap_user_header_t));
|     cap_user_data_t cap = (cap_user_data_t) xcalloc(1,
sizeof(cap_user_data_t));
|     head->version = _LINUX_CAPABILITY_VERSION;
|     if (capget(head, cap) != 0) {
| <snip>
|     head->pid = 0;
|     cap->inheritable = 0;
|     cap->effective = (1 << CAP_NET_BIND_SERVICE);
| <snip>
|     if (!keep)
|         cap->permitted &= cap->effective;
|     if (capset(head, cap) != 0) {
|
| I don't see a nice solution, short reverting, and adding a new set of
| syscalls to support 64-bit.
|
| thanks,
| -chris
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)

iD8DBQFINfkp+bHCR3gb8jsRAll6AKCfgKejl/TtJX6KfbbEb8dQbleMXgCgp20B
fsAGaykQUensyYfL9hxlp9Q=
=w+GH
-----END PGP SIGNATURE-----

Re: capget() overflows buffers.

[Chris Wright]

* Andrew G. Morgan (morgan@kernel.org) wrote:
> Chris Wright wrote:
> | Andrew, I think this should be considered a serious problem.  The
> | interface ABI is stable for old programs, and fine for anything new
> | or old that's using libcap.  But the API has changed subtly (taking a
> | pointer to a blob, to a pointer to an array of blobs), and is easily
> | broken for programs recompiled against new headers not using libcap.
>
> [There is a warning about this issue in the kernel header file.]
>
> 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.

> If you take this compiled binary, that crashes on 2.6.25, and try to run
> it on 2.6.24 it will fail there too - since the magic its 'forcing' is
> not valid on that kernel. So the compiled 'binary' we're discussing does
> not have an existence proof that it will successfully run anywhere.

Right, that's not the issue.

> In point of fact, the kernel is binary compatible with old binaries. The
> problem is that _LINUX_CAPABILITY_VERSION #define now points to _2
> instead of _1 by default and Squid etc., are not paying attention to the
> value of the new magic cookie but expecting the previous revision of the
> ABI to work.

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).

> As such, I don't agree that there is a problem with the ABI, and I don't
> agree with your assertion about things being broken. I maintain there is
> a problem with the application source code.

History bites us...libcap wasn't always there.  As we see, people roll
their own.  You can argue that the interface is opaque, I'd agree.
Also that it's their fault for poking into the guts of the opaque
interface...yeah, I tend to even agree there.  However, it's not practical
to argue that when this app code has worked fine for 10 years.

> One 'solution' is to force everyone to notice at compile time by simply
> removing the definition of _LINUX_CAPABILITY_VERSION and force all new
> source code to be explicit about which ABI it wants to use...

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.

thanks,
-chris

Re: capget() overflows buffers.

[Andrew G. Morgan]

[-- Attachment #1: Type: text/plain, Size: 1653 bytes --]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

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?

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.

| 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.

Cheers

Andrew

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)

iD8DBQFINm2Z+bHCR3gb8jsRAmDJAJ9rm3W9wKqA9EBuUVCyccZJDy6XvACgkbqp
noq663WjGQFVe94VsjkOZYY=
=x9NL
-----END PGP SIGNATURE-----

[-- Attachment #2: remain-source-compatible-with-32-bit-raw-legacy-capa.patch --]
[-- Type: text/plain, Size: 5816 bytes --]

From ef100b0606f0e35f78be526b0840533ef188dbbe Mon Sep 17 00:00:00 2001
From: Andrew G. Morgan <morgan@kernel.org>
Date: Thu, 22 May 2008 22:57:54 -0700
Subject: [PATCH] Remain source compatible with 32-bit raw legacy capability support.

Source code out there hard-codes a notion of what the
_LINUX_CAPABILITY_VERSION #define means in terms of the semantics of
the raw capability system calls capget() and capset(). Its unfortunate,
but true.

As such, force this define to always retain its legacy value, and adopt
a new #define strategy for the kernel's internal implementation of the
preferred magic.

[User space code continues to be encouraged to use the libcap API which
protects the application from details like this.]

Signed-off-by: Andrew G. Morgan <morgan@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Chris Wright <chrisw@sous-sol.org>
Cc: Serge E. Hallyn <serue@us.ibm.com>
---
 fs/proc/array.c            |    2 +-
 include/linux/capability.h |   26 ++++++++++++++++++--------
 kernel/capability.c        |   12 ++++++------
 3 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/fs/proc/array.c b/fs/proc/array.c
index 9e3b8c3..797d775 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -288,7 +288,7 @@ static void render_cap_t(struct seq_file *m, const char *header,
 	seq_printf(m, "%s", header);
 	CAP_FOR_EACH_U32(__capi) {
 		seq_printf(m, "%08x",
-			   a->cap[(_LINUX_CAPABILITY_U32S-1) - __capi]);
+			   a->cap[(_KERNEL_CAPABILITY_U32S-1) - __capi]);
 	}
 	seq_printf(m, "\n");
 }
diff --git a/include/linux/capability.h b/include/linux/capability.h
index f4ea0dd..f88b4db 100644
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -34,9 +34,6 @@ struct task_struct;
 #define _LINUX_CAPABILITY_VERSION_2  0x20071026
 #define _LINUX_CAPABILITY_U32S_2     2
 
-#define _LINUX_CAPABILITY_VERSION    _LINUX_CAPABILITY_VERSION_2
-#define _LINUX_CAPABILITY_U32S       _LINUX_CAPABILITY_U32S_2
-
 typedef struct __user_cap_header_struct {
 	__u32 version;
 	int pid;
@@ -77,10 +74,23 @@ struct vfs_cap_data {
 	} data[VFS_CAP_U32];
 };
 
-#ifdef __KERNEL__
+#ifndef __KERNEL__
+
+/*
+ * Backwardly compatible definition for source code - trapped in a
+ * 32-bit world. If you find you need this, please consider using
+ * libcap to untrap yourself...
+ */
+#define _LINUX_CAPABILITY_VERSION  _LINUX_CAPABILITY_VERSION_1
+#define _LINUX_CAPABILITY_U32S     _LINUX_CAPABILITY_U32S_1
+
+#else
+
+#define _KERNEL_CAPABILITY_VERSION _LINUX_CAPABILITY_VERSION_2
+#define _KERNEL_CAPABILITY_U32S    _LINUX_CAPABILITY_U32S_2
 
 typedef struct kernel_cap_struct {
-	__u32 cap[_LINUX_CAPABILITY_U32S];
+	__u32 cap[_KERNEL_CAPABILITY_U32S];
 } kernel_cap_t;
 
 #define _USER_CAP_HEADER_SIZE  (sizeof(struct __user_cap_header_struct))
@@ -351,7 +361,7 @@ typedef struct kernel_cap_struct {
  */
 
 #define CAP_FOR_EACH_U32(__capi)  \
-	for (__capi = 0; __capi < _LINUX_CAPABILITY_U32S; ++__capi)
+	for (__capi = 0; __capi < _KERNEL_CAPABILITY_U32S; ++__capi)
 
 # define CAP_FS_MASK_B0     (CAP_TO_MASK(CAP_CHOWN)		\
 			    | CAP_TO_MASK(CAP_DAC_OVERRIDE)	\
@@ -361,7 +371,7 @@ typedef struct kernel_cap_struct {
 
 # define CAP_FS_MASK_B1     (CAP_TO_MASK(CAP_MAC_OVERRIDE))
 
-#if _LINUX_CAPABILITY_U32S != 2
+#if _KERNEL_CAPABILITY_U32S != 2
 # error Fix up hand-coded capability macro initializers
 #else /* HAND-CODED capability initializers */
 
@@ -372,7 +382,7 @@ typedef struct kernel_cap_struct {
 # define CAP_NFSD_SET     ((kernel_cap_t){{ CAP_FS_MASK_B0|CAP_TO_MASK(CAP_SYS_RESOURCE), \
 					CAP_FS_MASK_B1 } })
 
-#endif /* _LINUX_CAPABILITY_U32S != 2 */
+#endif /* _KERNEL_CAPABILITY_U32S != 2 */
 
 #define CAP_INIT_INH_SET    CAP_EMPTY_SET
 
diff --git a/kernel/capability.c b/kernel/capability.c
index 39e8193..8e5cc51 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -88,7 +88,7 @@ asmlinkage long sys_capget(cap_user_header_t header, cap_user_data_t dataptr)
 		tocopy = _LINUX_CAPABILITY_U32S_2;
 		break;
 	default:
-		if (put_user(_LINUX_CAPABILITY_VERSION, &header->version))
+		if (put_user(_KERNEL_CAPABILITY_VERSION, &header->version))
 			return -EFAULT;
 		return -EINVAL;
 	}
@@ -118,7 +118,7 @@ out:
 	spin_unlock(&task_capability_lock);
 
 	if (!ret) {
-		struct __user_cap_data_struct kdata[_LINUX_CAPABILITY_U32S];
+		struct __user_cap_data_struct kdata[_KERNEL_CAPABILITY_U32S];
 		unsigned i;
 
 		for (i = 0; i < tocopy; i++) {
@@ -128,7 +128,7 @@ out:
 		}
 
 		/*
-		 * Note, in the case, tocopy < _LINUX_CAPABILITY_U32S,
+		 * Note, in the case, tocopy < _KERNEL_CAPABILITY_U32S,
 		 * we silently drop the upper capabilities here. This
 		 * has the effect of making older libcap
 		 * implementations implicitly drop upper capability
@@ -240,7 +240,7 @@ static inline int cap_set_all(kernel_cap_t *effective,
  */
 asmlinkage long sys_capset(cap_user_header_t header, const cap_user_data_t data)
 {
-	struct __user_cap_data_struct kdata[_LINUX_CAPABILITY_U32S];
+	struct __user_cap_data_struct kdata[_KERNEL_CAPABILITY_U32S];
 	unsigned i, tocopy;
 	kernel_cap_t inheritable, permitted, effective;
 	__u32 version;
@@ -260,7 +260,7 @@ asmlinkage long sys_capset(cap_user_header_t header, const cap_user_data_t data)
 		tocopy = _LINUX_CAPABILITY_U32S_2;
 		break;
 	default:
-		if (put_user(_LINUX_CAPABILITY_VERSION, &header->version))
+		if (put_user(_KERNEL_CAPABILITY_VERSION, &header->version))
 			return -EFAULT;
 		return -EINVAL;
 	}
@@ -281,7 +281,7 @@ asmlinkage long sys_capset(cap_user_header_t header, const cap_user_data_t data)
 		permitted.cap[i] = kdata[i].permitted;
 		inheritable.cap[i] = kdata[i].inheritable;
 	}
-	while (i < _LINUX_CAPABILITY_U32S) {
+	while (i < _KERNEL_CAPABILITY_U32S) {
 		effective.cap[i] = 0;
 		permitted.cap[i] = 0;
 		inheritable.cap[i] = 0;
-- 
1.5.3.7

Re: capget() overflows buffers.

[Chris Wright]

* Andrew G. Morgan (morgan@kernel.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

Re: capget() overflows buffers.

[Andrew G. Morgan]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Chris Wright wrote:
| 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.

So, this syscall will either fail (because the process isn't
sufficiently capable to execute the system-call, and/or the "garbage"
has something like pP < pE, and the resulting attempt violates the pE'
<= pP' rule), or the syscall succeeds...

In the case that it is discovered to fail (on any tested architecture
like in the redhat bug tracked here:
https://bugzilla.redhat.com/show_bug.cgi?id=447518 ), clearly the code
is going to get recompiled to make it work.

| 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.

Your concern is for the situation when the garbage happens to correspond
to an apparently meaningful setting for the upper capability bits? The
problem being that this privileged application is more privileged than
intended? I agree that this is not ideal.

In practice, however, this is only a real problem if named (or a
similarly structured program) has a security related bug in it. No?

Is this your concern, or have I missed something?

Cheers

Andrew

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)

iD8DBQFIN7T0+bHCR3gb8jsRAu87AJ4+ar3LeRol8/szzwgFvDYidQkFbgCeNEqj
0CfYnVW19WRc3H/gXs2wSVY=
=pQ8d
-----END PGP SIGNATURE-----

Re: capget() overflows buffers.

[Chris Wright]

* Andrew G. Morgan (morgan@kernel.org) wrote:
> Your concern is for the situation when the garbage happens to correspond
> to an apparently meaningful setting for the upper capability bits? The
> problem being that this privileged application is more privileged than
> intended? I agree that this is not ideal.

Yep, exactly.

> In practice, however, this is only a real problem if named (or a
> similarly structured program) has a security related bug in it. No?

It's dropped privileges to help mitigate any security related bug it
may contain.  It's conceivable (albeit remote[1]) that fork/exec plus
inheritable could leak privs w/out a security related bug.

> Is this your concern, or have I missed something?

That's it.

thanks,
-chris

[1] Get lucky combo in the garbage bits and have not shed uid 0.
Much less likely.

[PATCH] security: was "Re: capget() overflows buffers."

[Andrew G. Morgan]

[-- Attachment #1: Type: text/plain, Size: 2479 bytes --]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Chris Wright wrote:
| * Andrew G. Morgan (morgan@kernel.org) wrote:
|> Your concern is for the situation when the garbage happens to correspond
|> to an apparently meaningful setting for the upper capability bits? The
|> problem being that this privileged application is more privileged than
|> intended? I agree that this is not ideal.
|
| Yep, exactly.
|
|> In practice, however, this is only a real problem if named (or a
|> similarly structured program) has a security related bug in it. No?
|
| It's dropped privileges to help mitigate any security related bug it
| may contain.  It's conceivable (albeit remote[1]) that fork/exec plus
| inheritable could leak privs w/out a security related bug.
|
|> Is this your concern, or have I missed something?
|
| That's it.

OK, so by way of summary, the kernel, per se, is *not* broken, but the
kernel include file is problematic for use by user space - ie., having
used it some recompiled programs may be subtly broken... [ Example,
https://bugzilla.redhat.com/show_bug.cgi?id=447518 ]

Basically I agree that we should err on the side of being conservative...

| thanks,
| -chris
|
| [1] Get lucky combo in the garbage bits and have not shed uid 0.
| Much less likely.

So far as I can tell, the two problems (for unprepared applications -
not using libcap etc.) are:

~ 1. what the capget() system call may be writing to data[1] may lead to
unpredictable reliability issues with the security of the running
program (when its only allocated space for data[0]).

~ 2. the garbage that the capset() system call may be setting in pI that
persists post-exec(). The security issue being (in the case that the
system has been configured with filesystem capability support) the leak
of inheritable bits that become effective through the subsequent
invocation of a filesystem-capable (fI != 0) application. The net result
being that this subsequent application gives capabilities to a user that
shouldn't wield them.

How about the attached for a combined patch? Chris, the only change over
last time is basically your suggested code change, with more comments
and a less cautious warning...

Cheers

Andrew

Ref: patch: 0c736c9f0ab16899df1803d5962287985e69a157
and libcap-2.10 supports this change.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)

iD8DBQFIO2Ea+bHCR3gb8jsRAowdAJ9kMa15tXLyv6t1EfV0pyOsbqk49QCgsjRJ
+SCiUsbN7M5nfdehXBWjzt0=
=Ri1u
-----END PGP SIGNATURE-----

[-- Attachment #2: remain-source-compatible-with-32-bit-raw-legacy-capa.patch --]
[-- Type: text/plain, Size: 9735 bytes --]

From 0c736c9f0ab16899df1803d5962287985e69a157 Mon Sep 17 00:00:00 2001
From: Andrew G. Morgan <morgan@kernel.org>
Date: Mon, 26 May 2008 18:06:27 -0700
Subject: [PATCH] Remain source compatible with 32-bit raw legacy capability support.

Source code out there hard-codes a notion of what the
_LINUX_CAPABILITY_VERSION #define means in terms of the semantics of
the raw capability system calls capget() and capset(). Its unfortunate,
but true.

Since the confusing header file has been in a released kernel, there
is software that is erroneously using 64-bit capabilities with the
semantics of 32-bit compatibilities. These recently compiled programs
may suffer corruption of their memory when sys_getcap() overwrites
more memory than they are coded to expect, and the raising of added
capabilities when using sys_capset().

As such, this patch does a number of things to clean up the situation
for all. It

  1. forces the _LINUX_CAPABILITY_VERSION define to always retain its
     legacy value.

  2. adopts a new #define strategy for the kernel's internal
     implementation of the preferred magic.

  3. depreciates v2 capability magic in favor of a new (v3) magic
     number. The functionality of v3 is entirely equivalent to v2,
     the only difference being that the v2 magic causes the kernel
     to log a "depreciated" warning so the admin can find applications
     that may be using v2 inappropriately.

[User space code continues to be encouraged to use the libcap API
which protects the application from details like this. libcap-2.10
is the first to support v3 capabilities.]

Signed-off-by: Andrew G. Morgan <morgan@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Chris Wright <chrisw@sous-sol.org>
Cc: Serge E. Hallyn <serue@us.ibm.com>
---
 fs/proc/array.c            |    2 +-
 include/linux/capability.h |   29 ++++++++---
 kernel/capability.c        |  110 +++++++++++++++++++++++++++++---------------
 3 files changed, 94 insertions(+), 47 deletions(-)

diff --git a/fs/proc/array.c b/fs/proc/array.c
index 9e3b8c3..797d775 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -288,7 +288,7 @@ static void render_cap_t(struct seq_file *m, const char *header,
 	seq_printf(m, "%s", header);
 	CAP_FOR_EACH_U32(__capi) {
 		seq_printf(m, "%08x",
-			   a->cap[(_LINUX_CAPABILITY_U32S-1) - __capi]);
+			   a->cap[(_KERNEL_CAPABILITY_U32S-1) - __capi]);
 	}
 	seq_printf(m, "\n");
 }
diff --git a/include/linux/capability.h b/include/linux/capability.h
index f4ea0dd..272e040 100644
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -31,11 +31,11 @@ struct task_struct;
 #define _LINUX_CAPABILITY_VERSION_1  0x19980330
 #define _LINUX_CAPABILITY_U32S_1     1
 
-#define _LINUX_CAPABILITY_VERSION_2  0x20071026
+#define _LINUX_CAPABILITY_VERSION_2  0x20071026  /* depreciated - use v3 */
 #define _LINUX_CAPABILITY_U32S_2     2
 
-#define _LINUX_CAPABILITY_VERSION    _LINUX_CAPABILITY_VERSION_2
-#define _LINUX_CAPABILITY_U32S       _LINUX_CAPABILITY_U32S_2
+#define _LINUX_CAPABILITY_VERSION_3  0x20080522
+#define _LINUX_CAPABILITY_U32S_3     2
 
 typedef struct __user_cap_header_struct {
 	__u32 version;
@@ -77,10 +77,23 @@ struct vfs_cap_data {
 	} data[VFS_CAP_U32];
 };
 
-#ifdef __KERNEL__
+#ifndef __KERNEL__
+
+/*
+ * Backwardly compatible definition for source code - trapped in a
+ * 32-bit world. If you find you need this, please consider using
+ * libcap to untrap yourself...
+ */
+#define _LINUX_CAPABILITY_VERSION  _LINUX_CAPABILITY_VERSION_1
+#define _LINUX_CAPABILITY_U32S     _LINUX_CAPABILITY_U32S_1
+
+#else
+
+#define _KERNEL_CAPABILITY_VERSION _LINUX_CAPABILITY_VERSION_3
+#define _KERNEL_CAPABILITY_U32S    _LINUX_CAPABILITY_U32S_3
 
 typedef struct kernel_cap_struct {
-	__u32 cap[_LINUX_CAPABILITY_U32S];
+	__u32 cap[_KERNEL_CAPABILITY_U32S];
 } kernel_cap_t;
 
 #define _USER_CAP_HEADER_SIZE  (sizeof(struct __user_cap_header_struct))
@@ -351,7 +364,7 @@ typedef struct kernel_cap_struct {
  */
 
 #define CAP_FOR_EACH_U32(__capi)  \
-	for (__capi = 0; __capi < _LINUX_CAPABILITY_U32S; ++__capi)
+	for (__capi = 0; __capi < _KERNEL_CAPABILITY_U32S; ++__capi)
 
 # define CAP_FS_MASK_B0     (CAP_TO_MASK(CAP_CHOWN)		\
 			    | CAP_TO_MASK(CAP_DAC_OVERRIDE)	\
@@ -361,7 +374,7 @@ typedef struct kernel_cap_struct {
 
 # define CAP_FS_MASK_B1     (CAP_TO_MASK(CAP_MAC_OVERRIDE))
 
-#if _LINUX_CAPABILITY_U32S != 2
+#if _KERNEL_CAPABILITY_U32S != 2
 # error Fix up hand-coded capability macro initializers
 #else /* HAND-CODED capability initializers */
 
@@ -372,7 +385,7 @@ typedef struct kernel_cap_struct {
 # define CAP_NFSD_SET     ((kernel_cap_t){{ CAP_FS_MASK_B0|CAP_TO_MASK(CAP_SYS_RESOURCE), \
 					CAP_FS_MASK_B1 } })
 
-#endif /* _LINUX_CAPABILITY_U32S != 2 */
+#endif /* _KERNEL_CAPABILITY_U32S != 2 */
 
 #define CAP_INIT_INH_SET    CAP_EMPTY_SET
 
diff --git a/kernel/capability.c b/kernel/capability.c
index 39e8193..326406c 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -53,6 +53,68 @@ static void warn_legacy_capability_use(void)
 }
 
 /*
+ * Version 2 capabilities worked fine, but the linux/capability.h file
+ * that accompanied their introduction encouraged their use without
+ * the necessary user-space source code changes. As such, we have
+ * created a version 3 with equivalent functionality to version 2, but
+ * with a header change to protect legacy source code from using
+ * version 2 when it wanted to use version 1. If your system has code
+ * that trips the following warning, it is using version 2 specific
+ * capabilities and may be doing so insecurely.
+ *
+ * The remedy is to either upgrade your version of libcap (to 2.10+,
+ * if the application is linked against it), or recompile your
+ * application with modern kernel headers and this warning will go
+ * away.
+ */
+
+static void warn_depreciated_v2(void)
+{
+	static int warned = 0;
+	if (!warned) {
+		char name[sizeof(current->comm)];
+
+		printk(KERN_INFO "warning: `%s' uses depreciated v2"
+		       " capabilities in a way that may be insecure.\n",
+		       get_task_comm(name, current));
+		warned = 1;
+	}
+}
+
+/*
+ * Version check. Return the number of u32s in each capability flag
+ * array, or a negative value on error.
+ */
+static int cap_validate_magic(cap_user_header_t header, unsigned *tocopy)
+{
+	__u32 version;
+
+	if (get_user(version, &header->version))
+		return -EFAULT;
+
+	switch (version) {
+	case _LINUX_CAPABILITY_VERSION_1:
+		warn_legacy_capability_use();
+		*tocopy = _LINUX_CAPABILITY_U32S_1;
+		break;
+	case _LINUX_CAPABILITY_VERSION_2:
+		warn_depreciated_v2();
+		/*
+		 * fall through - v3 is otherwise equivalent to v2.
+		 */
+	case _LINUX_CAPABILITY_VERSION_3:
+		*tocopy = _LINUX_CAPABILITY_U32S_3;
+		break;
+	default:
+		if (put_user(_KERNEL_CAPABILITY_VERSION, &header->version))
+			return -EFAULT;
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/*
  * For sys_getproccap() and sys_setproccap(), any of the three
  * capability set pointers may be NULL -- indicating that that set is
  * uninteresting and/or not to be changed.
@@ -71,27 +133,13 @@ asmlinkage long sys_capget(cap_user_header_t header, cap_user_data_t dataptr)
 {
 	int ret = 0;
 	pid_t pid;
-	__u32 version;
 	struct task_struct *target;
 	unsigned tocopy;
 	kernel_cap_t pE, pI, pP;
 
-	if (get_user(version, &header->version))
-		return -EFAULT;
-
-	switch (version) {
-	case _LINUX_CAPABILITY_VERSION_1:
-		warn_legacy_capability_use();
-		tocopy = _LINUX_CAPABILITY_U32S_1;
-		break;
-	case _LINUX_CAPABILITY_VERSION_2:
-		tocopy = _LINUX_CAPABILITY_U32S_2;
-		break;
-	default:
-		if (put_user(_LINUX_CAPABILITY_VERSION, &header->version))
-			return -EFAULT;
-		return -EINVAL;
-	}
+	ret = cap_validate_magic(header, &tocopy);
+	if (ret != 0)
+		return ret;
 
 	if (get_user(pid, &header->pid))
 		return -EFAULT;
@@ -118,7 +166,7 @@ out:
 	spin_unlock(&task_capability_lock);
 
 	if (!ret) {
-		struct __user_cap_data_struct kdata[_LINUX_CAPABILITY_U32S];
+		struct __user_cap_data_struct kdata[_KERNEL_CAPABILITY_U32S];
 		unsigned i;
 
 		for (i = 0; i < tocopy; i++) {
@@ -128,7 +176,7 @@ out:
 		}
 
 		/*
-		 * Note, in the case, tocopy < _LINUX_CAPABILITY_U32S,
+		 * Note, in the case, tocopy < _KERNEL_CAPABILITY_U32S,
 		 * we silently drop the upper capabilities here. This
 		 * has the effect of making older libcap
 		 * implementations implicitly drop upper capability
@@ -240,30 +288,16 @@ static inline int cap_set_all(kernel_cap_t *effective,
  */
 asmlinkage long sys_capset(cap_user_header_t header, const cap_user_data_t data)
 {
-	struct __user_cap_data_struct kdata[_LINUX_CAPABILITY_U32S];
+	struct __user_cap_data_struct kdata[_KERNEL_CAPABILITY_U32S];
 	unsigned i, tocopy;
 	kernel_cap_t inheritable, permitted, effective;
-	__u32 version;
 	struct task_struct *target;
 	int ret;
 	pid_t pid;
 
-	if (get_user(version, &header->version))
-		return -EFAULT;
-
-	switch (version) {
-	case _LINUX_CAPABILITY_VERSION_1:
-		warn_legacy_capability_use();
-		tocopy = _LINUX_CAPABILITY_U32S_1;
-		break;
-	case _LINUX_CAPABILITY_VERSION_2:
-		tocopy = _LINUX_CAPABILITY_U32S_2;
-		break;
-	default:
-		if (put_user(_LINUX_CAPABILITY_VERSION, &header->version))
-			return -EFAULT;
-		return -EINVAL;
-	}
+	ret = cap_validate_magic(header, &tocopy);
+	if (ret != 0)
+		return ret;
 
 	if (get_user(pid, &header->pid))
 		return -EFAULT;
@@ -281,7 +315,7 @@ asmlinkage long sys_capset(cap_user_header_t header, const cap_user_data_t data)
 		permitted.cap[i] = kdata[i].permitted;
 		inheritable.cap[i] = kdata[i].inheritable;
 	}
-	while (i < _LINUX_CAPABILITY_U32S) {
+	while (i < _KERNEL_CAPABILITY_U32S) {
 		effective.cap[i] = 0;
 		permitted.cap[i] = 0;
 		inheritable.cap[i] = 0;
-- 
1.5.3.7

Re: [PATCH] security: was "Re: capget() overflows buffers."

[Chris Wright]

* Andrew G. Morgan (morgan@kernel.org) wrote:
> How about the attached for a combined patch? Chris, the only change over
> last time is basically your suggested code change, with more comments
> and a less cautious warning...

Looks good, I'll queue it (I added

  Fixes issue reported in https://bugzilla.redhat.com/show_bug.cgi?id=447518.
  Thanks to Bojan Smojver for the report.

to the commit message).

thanks,
-chris

Re: [PATCH] security: was "Re: capget() overflows buffers."

[Andrew Morton]

On Mon, 26 May 2008 18:17:15 -0700 "Andrew G. Morgan" <morgan@kernel.org> wrote:

> Source code out there hard-codes a notion of what the
> _LINUX_CAPABILITY_VERSION #define means in terms of the semantics of the
> raw capability system calls capget() and capset().  Its unfortunate, but
> true.
> 
> Since the confusing header file has been in a released kernel, there is
> software that is erroneously using 64-bit capabilities with the semantics
> of 32-bit compatibilities.  These recently compiled programs may suffer
> corruption of their memory when sys_getcap() overwrites more memory than
> they are coded to expect, and the raising of added capabilities when using
> sys_capset().
> 
> As such, this patch does a number of things to clean up the situation
> for all. It
> 
>   1. forces the _LINUX_CAPABILITY_VERSION define to always retain its
>      legacy value.
> 
>   2. adopts a new #define strategy for the kernel's internal
>      implementation of the preferred magic.
> 
>   3. depreciates v2 capability magic in favor of a new (v3) magic
>      number. The functionality of v3 is entirely equivalent to v2,
>      the only difference being that the v2 magic causes the kernel
>      to log a "depreciated" warning so the admin can find applications
>      that may be using v2 inappropriately.
> 
> [User space code continues to be encouraged to use the libcap API which
> protects the application from details like this.  libcap-2.10 is the first
> to support v3 capabilities.]
> 
> ...
>
> + * Version 2 capabilities worked fine, but the linux/capability.h file
> + * that accompanied their introduction encouraged their use without
> + * the necessary user-space source code changes. As such, we have
> + * created a version 3 with equivalent functionality to version 2, but
> + * with a header change to protect legacy source code from using
> + * version 2 when it wanted to use version 1. If your system has code
> + * that trips the following warning, it is using version 2 specific
> + * capabilities and may be doing so insecurely.
> + *
> + * The remedy is to either upgrade your version of libcap (to 2.10+,
> + * if the application is linked against it), or recompile your
> + * application with modern kernel headers and this warning will go
> + * away.
> + */
> +
> +static void warn_depreciated_v2(void)

"deprecated", please.  "depreciated" is where you don't want your
investments to be.

--- a/include/linux/capability.h~capabilities-remain-source-compatible-with-32-bit-raw-legacy-capability-support-fix
+++ a/include/linux/capability.h
@@ -31,7 +31,7 @@ struct task_struct;
 #define _LINUX_CAPABILITY_VERSION_1  0x19980330
 #define _LINUX_CAPABILITY_U32S_1     1
 
-#define _LINUX_CAPABILITY_VERSION_2  0x20071026  /* depreciated - use v3 */
+#define _LINUX_CAPABILITY_VERSION_2  0x20071026  /* deprecated - use v3 */
 #define _LINUX_CAPABILITY_U32S_2     2
 
 #define _LINUX_CAPABILITY_VERSION_3  0x20080522
--- a/kernel/capability.c~capabilities-remain-source-compatible-with-32-bit-raw-legacy-capability-support-fix
+++ a/kernel/capability.c
@@ -68,13 +68,13 @@ static void warn_legacy_capability_use(v
  * away.
  */
 
-static void warn_depreciated_v2(void)
+static void warn_deprecated_v2(void)
 {
 	static int warned = 0;
 	if (!warned) {
 		char name[sizeof(current->comm)];
 
-		printk(KERN_INFO "warning: `%s' uses depreciated v2"
+		printk(KERN_INFO "warning: `%s' uses deprecated v2"
 		       " capabilities in a way that may be insecure.\n",
 		       get_task_comm(name, current));
 		warned = 1;
@@ -98,7 +98,7 @@ static int cap_validate_magic(cap_user_h
 		*tocopy = _LINUX_CAPABILITY_U32S_1;
 		break;
 	case _LINUX_CAPABILITY_VERSION_2:
-		warn_depreciated_v2();
+		warn_deprecated_v2();
 		/*
 		 * fall through - v3 is otherwise equivalent to v2.
 		 */
_

> +{
> +	static int warned = 0;
> +	if (!warned) {

We were going to have a ONCE() macro to do this but that seems to have
not happened.

> +		char name[sizeof(current->comm)];
> +
> +		printk(KERN_INFO "warning: `%s' uses depreciated v2"
> +		       " capabilities in a way that may be insecure.\n",
> +		       get_task_comm(name, current));
> +		warned = 1;
> +	}
> +}
> +
> +/*
> + * Version check. Return the number of u32s in each capability flag
> + * array, or a negative value on error.
> + */
> +static int cap_validate_magic(cap_user_header_t header, unsigned *tocopy)
> +{
> +	__u32 version;
> +
> +	if (get_user(version, &header->version))
> +		return -EFAULT;
> +
> +	switch (version) {
> +	case _LINUX_CAPABILITY_VERSION_1:
> +		warn_legacy_capability_use();
> +		*tocopy = _LINUX_CAPABILITY_U32S_1;
> +		break;
> +	case _LINUX_CAPABILITY_VERSION_2:
> +		warn_depreciated_v2();
> +		/*
> +		 * fall through - v3 is otherwise equivalent to v2.
> +		 */
> +	case _LINUX_CAPABILITY_VERSION_3:
> +		*tocopy = _LINUX_CAPABILITY_U32S_3;
> +		break;
> +	default:
> +		if (put_user(_KERNEL_CAPABILITY_VERSION, &header->version))

This is a bit fragile.  It is dependent upon the compiler's view of
sizeof(_KERNEL_CAPABILITY_VERSION).

And it'll work OK for now:

#define _LINUX_CAPABILITY_VERSION_3  0x20080522
...
#define _KERNEL_CAPABILITY_VERSION _LINUX_CAPABILITY_VERSION_3

but if, for example, someone comes up and accidentally puts an "L" at
the end of that 0x20080522 then whoops, we just broke the ABI.  I think
it would be good to do something a bit more explicit there.

Perhaps:

--- a/kernel/capability.c~capabilities-remain-source-compatible-with-32-bit-raw-legacy-capability-support-fix-fix
+++ a/kernel/capability.c
@@ -106,7 +106,7 @@ static int cap_validate_magic(cap_user_h
 		*tocopy = _LINUX_CAPABILITY_U32S_3;
 		break;
 	default:
-		if (put_user(_KERNEL_CAPABILITY_VERSION, &header->version))
+		if (put_user((u32)_KERNEL_CAPABILITY_VERSION, &header->version))
 			return -EFAULT;
 		return -EINVAL;
 	}
_

?

Re: capget() overflows buffers.

[Chris Wright]

* Andrew G. Morgan (morgan@kernel.org) wrote:
> diff --git a/include/linux/capability.h b/include/linux/capability.h
> index f4ea0dd..f88b4db 100644
> --- a/include/linux/capability.h
> +++ b/include/linux/capability.h
> @@ -34,9 +34,6 @@ struct task_struct;
>  #define _LINUX_CAPABILITY_VERSION_2  0x20071026
>  #define _LINUX_CAPABILITY_U32S_2     2
>  
> -#define _LINUX_CAPABILITY_VERSION    _LINUX_CAPABILITY_VERSION_2
> -#define _LINUX_CAPABILITY_U32S       _LINUX_CAPABILITY_U32S_2
> -
>  typedef struct __user_cap_header_struct {
>  	__u32 version;
>  	int pid;
> @@ -77,10 +74,23 @@ struct vfs_cap_data {
>  	} data[VFS_CAP_U32];
>  };
>  
> -#ifdef __KERNEL__
> +#ifndef __KERNEL__
> +
> +/*
> + * Backwardly compatible definition for source code - trapped in a
> + * 32-bit world. If you find you need this, please consider using
> + * libcap to untrap yourself...
> + */
> +#define _LINUX_CAPABILITY_VERSION  _LINUX_CAPABILITY_VERSION_1
> +#define _LINUX_CAPABILITY_U32S     _LINUX_CAPABILITY_U32S_1

I'm sure you're painfully aware, but this will need some change
to libcap as well (to let it handle 64bit caps again).

That's what I meant earlier by "And use another mechanism to                                             
signal the availability of 64bit caps."

All looks good.  I think we need to issue some warnings, because
at least Fedora 9 and openSUSE 11 are/will be 2.6.25 based.

Something like this:


--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -31,9 +31,13 @@ struct task_struct;
 #define _LINUX_CAPABILITY_VERSION_1  0x19980330
 #define _LINUX_CAPABILITY_U32S_1     1
 
+/* v2 should be considered broken */
 #define _LINUX_CAPABILITY_VERSION_2  0x20071026
 #define _LINUX_CAPABILITY_U32S_2     2
 
+#define _LINUX_CAPABILITY_VERSION_3  0x20080522
+#define _LINUX_CAPABILITY_U32S_3     2
+
 typedef struct __user_cap_header_struct {
 	__u32 version;
 	int pid;
@@ -86,8 +90,8 @@ struct vfs_cap_data {
 
 #else
 
-#define _KERNEL_CAPABILITY_VERSION _LINUX_CAPABILITY_VERSION_2
-#define _KERNEL_CAPABILITY_U32S    _LINUX_CAPABILITY_U32S_2
+#define _KERNEL_CAPABILITY_VERSION _LINUX_CAPABILITY_VERSION_3
+#define _KERNEL_CAPABILITY_U32S    _LINUX_CAPABILITY_U32S_3
 
 typedef struct kernel_cap_struct {
 	__u32 cap[_KERNEL_CAPABILITY_U32S];
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -39,6 +39,19 @@ EXPORT_SYMBOL(__cap_init_eff_set);
  *   http://www.kernel.org/pub/linux/libs/security/linux-privs/
  */
 
+static void warn_broken_capability_use(void)
+{
+	static int warned;
+	if (!warned) {
+		char name[sizeof(current->comm)];
+
+		printk(KERN_INFO "warning: `%s' uses a capabilities version"
+		       " which could be broken if it is not using libcap.\n",
+		       get_task_comm(name, current));
+		warned = 1;
+	}
+}
+
 static void warn_legacy_capability_use(void)
 {
 	static int warned;
@@ -85,8 +98,12 @@ asmlinkage long sys_capget(cap_user_head
 		tocopy = _LINUX_CAPABILITY_U32S_1;
 		break;
 	case _LINUX_CAPABILITY_VERSION_2:
+		warn_broken_capability_use();
 		tocopy = _LINUX_CAPABILITY_U32S_2;
 		break;
+	case _LINUX_CAPABILITY_VERSION_3:
+		tocopy = _LINUX_CAPABILITY_U32S_3;
+		break;
 	default:
 		if (put_user(_KERNEL_CAPABILITY_VERSION, &header->version))
 			return -EFAULT;
@@ -257,8 +274,12 @@ asmlinkage long sys_capset(cap_user_head
 		tocopy = _LINUX_CAPABILITY_U32S_1;
 		break;
 	case _LINUX_CAPABILITY_VERSION_2:
+		warn_broken_capability_use();
 		tocopy = _LINUX_CAPABILITY_U32S_2;
 		break;
+	case _LINUX_CAPABILITY_VERSION_3:
+		tocopy = _LINUX_CAPABILITY_U32S_3;
+		break;
 	default:
 		if (put_user(_KERNEL_CAPABILITY_VERSION, &header->version))
 			return -EFAULT;

Re: capget() overflows buffers.

[Andrew G. Morgan]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Chris Wright wrote:
| * Andrew G. Morgan (morgan@kernel.org) wrote:
|> diff --git a/include/linux/capability.h b/include/linux/capability.h
|> index f4ea0dd..f88b4db 100644
|> --- a/include/linux/capability.h
|> +++ b/include/linux/capability.h
|> @@ -34,9 +34,6 @@ struct task_struct;
|>  #define _LINUX_CAPABILITY_VERSION_2  0x20071026
|>  #define _LINUX_CAPABILITY_U32S_2     2
|>
|> -#define _LINUX_CAPABILITY_VERSION    _LINUX_CAPABILITY_VERSION_2
|> -#define _LINUX_CAPABILITY_U32S       _LINUX_CAPABILITY_U32S_2
|> -
|>  typedef struct __user_cap_header_struct {
|>  	__u32 version;
|>  	int pid;
|> @@ -77,10 +74,23 @@ struct vfs_cap_data {
|>  	} data[VFS_CAP_U32];
|>  };
|>
|> -#ifdef __KERNEL__
|> +#ifndef __KERNEL__
|> +
|> +/*
|> + * Backwardly compatible definition for source code - trapped in a
|> + * 32-bit world. If you find you need this, please consider using
|> + * libcap to untrap yourself...
|> + */
|> +#define _LINUX_CAPABILITY_VERSION  _LINUX_CAPABILITY_VERSION_1
|> +#define _LINUX_CAPABILITY_U32S     _LINUX_CAPABILITY_U32S_1
|
| I'm sure you're painfully aware, but this will need some change
| to libcap as well (to let it handle 64bit caps again).

Not really.

libcap2 (the one with 64-bit capability support) continues to work fine
(it ships with its own copy of a compatible linux/capability.h, and when
I update that, I'll make the corresponding change) but the effect should
be transparent to its users:

http://www.kernel.org/pub/linux/libs/security/linux-privs/kernel-2.6/

| That's what I meant earlier by "And use another mechanism to

| signal the availability of 64bit caps."

There is and has always been a method:

~   head.version = 0;
~   getcap(&head, NULL);
~   switch (head.version) {
~   case _LINUX_...:
~      ...
~      break;
~   case _,,,,:
~      etc...
~      break;
~   default:
~      abort("no idea what to do");
~   }

| All looks good.  I think we need to issue some warnings, because
| at least Fedora 9 and openSUSE 11 are/will be 2.6.25 based.

Do any of the above answers help? (FWIW I attached the patch to the
redhat bug.)

Cheers

Andrew

|
| Something like this:
|
|
| --- a/include/linux/capability.h
| +++ b/include/linux/capability.h
| @@ -31,9 +31,13 @@ struct task_struct;
|  #define _LINUX_CAPABILITY_VERSION_1  0x19980330
|  #define _LINUX_CAPABILITY_U32S_1     1
|
| +/* v2 should be considered broken */
|  #define _LINUX_CAPABILITY_VERSION_2  0x20071026
|  #define _LINUX_CAPABILITY_U32S_2     2
|
| +#define _LINUX_CAPABILITY_VERSION_3  0x20080522
| +#define _LINUX_CAPABILITY_U32S_3     2
| +
|  typedef struct __user_cap_header_struct {
|  	__u32 version;
|  	int pid;
| @@ -86,8 +90,8 @@ struct vfs_cap_data {
|
|  #else
|
| -#define _KERNEL_CAPABILITY_VERSION _LINUX_CAPABILITY_VERSION_2
| -#define _KERNEL_CAPABILITY_U32S    _LINUX_CAPABILITY_U32S_2
| +#define _KERNEL_CAPABILITY_VERSION _LINUX_CAPABILITY_VERSION_3
| +#define _KERNEL_CAPABILITY_U32S    _LINUX_CAPABILITY_U32S_3
|
|  typedef struct kernel_cap_struct {
|  	__u32 cap[_KERNEL_CAPABILITY_U32S];
| --- a/kernel/capability.c
| +++ b/kernel/capability.c
| @@ -39,6 +39,19 @@ EXPORT_SYMBOL(__cap_init_eff_set);
|   *   http://www.kernel.org/pub/linux/libs/security/linux-privs/
|   */
|
| +static void warn_broken_capability_use(void)
| +{
| +	static int warned;
| +	if (!warned) {
| +		char name[sizeof(current->comm)];
| +
| +		printk(KERN_INFO "warning: `%s' uses a capabilities version"
| +		       " which could be broken if it is not using libcap.\n",
| +		       get_task_comm(name, current));
| +		warned = 1;
| +	}
| +}
| +
|  static void warn_legacy_capability_use(void)
|  {
|  	static int warned;
| @@ -85,8 +98,12 @@ asmlinkage long sys_capget(cap_user_head
|  		tocopy = _LINUX_CAPABILITY_U32S_1;
|  		break;
|  	case _LINUX_CAPABILITY_VERSION_2:
| +		warn_broken_capability_use();
|  		tocopy = _LINUX_CAPABILITY_U32S_2;
|  		break;
| +	case _LINUX_CAPABILITY_VERSION_3:
| +		tocopy = _LINUX_CAPABILITY_U32S_3;
| +		break;
|  	default:
|  		if (put_user(_KERNEL_CAPABILITY_VERSION, &header->version))
|  			return -EFAULT;
| @@ -257,8 +274,12 @@ asmlinkage long sys_capset(cap_user_head
|  		tocopy = _LINUX_CAPABILITY_U32S_1;
|  		break;
|  	case _LINUX_CAPABILITY_VERSION_2:
| +		warn_broken_capability_use();
|  		tocopy = _LINUX_CAPABILITY_U32S_2;
|  		break;
| +	case _LINUX_CAPABILITY_VERSION_3:
| +		tocopy = _LINUX_CAPABILITY_U32S_3;
| +		break;
|  	default:
|  		if (put_user(_KERNEL_CAPABILITY_VERSION, &header->version))
|  			return -EFAULT;
|
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)

iD8DBQFIN1sQ+bHCR3gb8jsRAgezAJ0ZRuPNENeHTM46u/LC7IU2++yIowCguMwR
GJhKzg7F2p6PcNrKAv9i8Jg=
=Mrri
-----END PGP SIGNATURE-----

Re: capget() overflows buffers.

[Chris Wright]

* Andrew G. Morgan (morgan@kernel.org) wrote:
> Chris Wright wrote:
> | I'm sure you're painfully aware, but this will need some change
> | to libcap as well (to let it handle 64bit caps again).
>
> Not really.
>
> libcap2 (the one with 64-bit capability support) continues to work fine
> (it ships with its own copy of a compatible linux/capability.h, and when
> I update that, I'll make the corresponding change) but the effect should
> be transparent to its users:

Doh, I missed the leading portion of the pathname (cscope -p3 didn't
show anything more than include/sys/capability.h), thanks.

> | That's what I meant earlier by "And use another mechanism to
> | signal the availability of 64bit caps."
>
> There is and has always been a method:
>
> ~   head.version = 0;
> ~   getcap(&head, NULL);
> ~   switch (head.version) {
> ~   case _LINUX_...:
> ~      ...
> ~      break;
> ~   case _,,,,:
> ~      etc...
> ~      break;
> ~   default:
> ~      abort("no idea what to do");
> ~   }

Hmm, it would be kind of nice to have a formalized way get the size,
perhaps it would help with KaiGai's request for caps printed out.
Something that tells us either the number of u32s, or the max bit
supported?

> | All looks good.  I think we need to issue some warnings, because
> | at least Fedora 9 and openSUSE 11 are/will be 2.6.25 based.
>
> Do any of the above answers help? (FWIW I attached the patch to the
> redhat bug.)

Yes, thanks.  But I still think we need to print a warning (unfortunately
we can't distinguish libcap from non-libcap app), because apps that
aren't using libcap should really be updated (either pull new update
from vendor or recompiled by end user).

thanks,
-chris

Re: capget() overflows buffers.

[Andrew G. Morgan]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Chris Wright wrote:
| Hmm, it would be kind of nice to have a formalized way get the size,
| perhaps it would help with KaiGai's request for caps printed out.
| Something that tells us either the number of u32s, or the max bit
| supported?

Serge has already provided one  with the call,

~   sys_prctl(PR_CAPBSET_READ, x);

returns -EINVAL if (x > max-supported-capability).

(Ref: 3b7391de67da515c91f48aa371de77cb6cc5c07e)

|> | All looks good.  I think we need to issue some warnings, because
|> | at least Fedora 9 and openSUSE 11 are/will be 2.6.25 based.
|>
|> Do any of the above answers help? (FWIW I attached the patch to the
|> redhat bug.)
|
| Yes, thanks.  But I still think we need to print a warning (unfortunately
| we can't distinguish libcap from non-libcap app), because apps that
| aren't using libcap should really be updated (either pull new update
| from vendor or recompiled by end user).

Just to be clear, you are not referring to a warning that the
application is stuck in a 32-bit capability world, because we already
have one of those: warn_legacy_capability_use(). You are referring to a
warning that might indicate a problem with code like that given in your
example - in which case I'll respond to that part of the thread...

Cheers

Andrew

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)

iD8DBQFIN5xI+bHCR3gb8jsRAsz6AKDHOeOO8953r2bRJ3RXZaRdBnlGUwCdG3oX
CvWy/iQVmfVdpeRIWLa7N/w=
=R7Bq
-----END PGP SIGNATURE-----

Re: capget() overflows buffers.

[Chris Wright]

* Andrew G. Morgan (morgan@kernel.org) wrote:
> Chris Wright wrote:
> | Hmm, it would be kind of nice to have a formalized way get the size,
> | perhaps it would help with KaiGai's request for caps printed out.
> | Something that tells us either the number of u32s, or the max bit
> | supported?
>
> Serge has already provided one  with the call,
>
> ~   sys_prctl(PR_CAPBSET_READ, x);
>
> returns -EINVAL if (x > max-supported-capability).
>
> (Ref: 3b7391de67da515c91f48aa371de77cb6cc5c07e)

Yeah, that's a little roundabout..

> Just to be clear, you are not referring to a warning that the
> application is stuck in a 32-bit capability world, because we already
> have one of those: warn_legacy_capability_use(). You are referring to a
> warning that might indicate a problem with code like that given in your
> example - in which case I'll respond to that part of the thread...

Yes, like the one in the patch I sent that added a
warn_broken_capability_use().

thanks,
-chris

Re: capget() overflows buffers.

[Bojan Smojver]

On Thu, 2008-05-22 at 13:53 -0700, Chris Wright wrote:

>     cap_user_header_t head = (cap_user_header_t) xcalloc(1, sizeof(cap_user_header_t));
>     cap_user_data_t cap = (cap_user_data_t) xcalloc(1, sizeof(cap_user_data_t));

BTW, both of the above allocations have been fixed in Squid 2 & 3, as
they were incorrect. Not sure how that worked before - probably by
accident.

http://www.squid-cache.org/bzrview/squid3/trunk/changes?q=9001

-- 
Bojan

Re: capget() overflows buffers.

[Chris Wright]

* Bojan Smojver (bojan@rexursive.com) wrote:
> On Thu, 2008-05-22 at 13:53 -0700, Chris Wright wrote:
> 
> >     cap_user_header_t head = (cap_user_header_t) xcalloc(1, sizeof(cap_user_header_t));
> >     cap_user_data_t cap = (cap_user_data_t) xcalloc(1, sizeof(cap_user_data_t));
> 
> BTW, both of the above allocations have been fixed in Squid 2 & 3, as
> they were incorrect. Not sure how that worked before - probably by
> accident.

Heh, indeed ;-)

If you want to fix the problem you're having in squid you've got a few
choices:

1) switch to using the libcap interface, arguably the best sol'n since
you know longer have to directly care about the kernel interface.
only drawback is the additional library dependency.

2) force head->version to the older version, something like:

#ifdef  _LINUX_CAPABILITY_VERSION_1
  head->version = _LINUX_CAPABILITY_VERSION_1;
#else
  head->version = _LINUX_CAPABILITY_VERSION;
#endif

this has the drawback of always using the older 32bit caps, probably
fine here, since you're not using new caps.

3) try to allow for current 64bit caps

#define CAP_TO_INDEX(x)     ((x) >> 5)
#define CAP_TO_MASK(x)      (1 << ((x) & 31))
#define CAP_ADD(_data, _set, _cap)     _data[CAP_TO_INDEX(_cap)]._set |= CAP_TO_MASK(_cap)

    cap_user_data_t cap = (cap_user_data_t) xcalloc(_LINUX_CAPABILITY_U32S, sizeof(*cap));
    head->version = _LINUX_CAPABILITY_VERSION;

    CAP_ADD(cap, effective, CAP_NET_BIND_SERVICE);
    etc...

this has the drawback of not being guaranteed to work in the future on
newer kernels and needs ifdefs to work on older kernels, is complicated,
and not worth it.

Re: capget() overflows buffers.

[Bojan Smojver]

On Thu, 2008-05-22 at 19:06 -0700, Chris Wright wrote:

> 1) switch to using the libcap interface, arguably the best sol'n since
> you know longer have to directly care about the kernel interface.
> only drawback is the additional library dependency.

I'm thinking Squid folks may want to pick this up long term.

> 2) force head->version to the older version, something like:
> 
> #ifdef  _LINUX_CAPABILITY_VERSION_1
>   head->version = _LINUX_CAPABILITY_VERSION_1;
> #else
>   head->version = _LINUX_CAPABILITY_VERSION;
> #endif
> 
> this has the drawback of always using the older 32bit caps, probably
> fine here, since you're not using new caps.

Probably the best short term suggestion. I will post this in my original
bug report.

Thank you for your suggestions!

-- 
Bojan

Re: capget() overflows buffers.

[Bojan Smojver]

On Thu, 2008-05-22 at 10:58 -0700, Chris Wright wrote:

> Bojan, is there a capset involved as well, because that will pull in
> garbage and set caps accordingly?

I believe so. It's all part of Squid 3.0 code. File src/tools.cc, line
1386. I would point you to a URL in the source repository, but
squid-cache.org appears to be down right now.

-- 
Bojan