Re: capget() overflows buffers.

From: Andrew G. Morgan
Date: Fri May 23 2008 - 03:09:28 EST

Next message: Thomas Gleixner: "Re: [PATCH 4/5] x86: move mp_bus_not_pci from mpparse.c"
Previous message: Miao Xie: "[PATCH] cpusets: fix and update Documentation"
In reply to: Chris Wright: "Re: capget() overflows buffers."
Next in thread: Chris Wright: "Re: capget() overflows buffers."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

-----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-----
From ef100b0606f0e35f78be526b0840533ef188dbbe Mon Sep 17 00:00:00 2001
From: Andrew G. Morgan <morgan@xxxxxxxxxx>
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@xxxxxxxxxx>
Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
Cc: Chris Wright <chrisw@xxxxxxxxxxxx>
Cc: Serge E. Hallyn <serue@xxxxxxxxxx>
---
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

Next message: Thomas Gleixner: "Re: [PATCH 4/5] x86: move mp_bus_not_pci from mpparse.c"
Previous message: Miao Xie: "[PATCH] cpusets: fix and update Documentation"
In reply to: Chris Wright: "Re: capget() overflows buffers."
Next in thread: Chris Wright: "Re: capget() overflows buffers."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]