Re: [RFC PATCH] per-process securebits

[Serge E. Hallyn]

Quoting Andrew G. Morgan ([EMAIL PROTECTED]):
> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA1
>
>
>
> [EMAIL PROTECTED] wrote:
> | Quoting Andrew G. Morgan ([EMAIL PROTECTED]):
> | Here is my latest per-process secure-bits patch.
> |
> |> Hey Andrew,
> |
> |> looks really good.  Two comments inline.
>
> Thanks for the review!
>
> - -   unsigned keep_capabilities:1;
> + unsigned securebits;
>
> | should this maybe be an unsigned short?
>
> I'm not sure what this would buy us. Given the placement in the data
> structure its going to end up aligned by the surrounding data types and
> will occupy at least a sizeof(unsigned) amount of space even if we
> declare it as a short. Could you clarify why you think a short would be
> better?

Is that true on all architectures?  I figured there must be some
architecture where it isn't, but if not then never mind.

>
> + case PR_GET_KEEPCAPS:
> + if (issecure(SECURE_KEEP_CAPS))
> + error = 1;
> + break;
> + case PR_SET_KEEPCAPS:
> + if ((arg2 > 1) && !issecure(SECURE_KEEP_CAPS_LOCKED))
>
> | I don't understand what you're doing here.  If I had to guess, I'd say
> | you're only enforcing a check on a valid arg2 (which is 0 or 1) only if
> | SECURE_KEEP_CAPS_LOCKED is not set since otherwise the value you can't
> | be changed?  But you don't enforce that so it can in fact be changed,
> | and even if you did this seems to give the user poor feedback in more 
> than
> | one case, so it seems nicer to do
>
> I think the long and the short of it was that I clearly got distracted
> when writing that code! It didn't help that all of my testing was with
> the PR_*_SECUREBITS prctl()... What I had meant to write was this:
>
>   case PR_SET_KEEPCAPS:
>   /* Note, we rely on arg2 being unsigned here: */
>   if ((arg2 > 1) || issecure(SECURE_KEEP_CAPS_LOCKED))
>   error = -EINVAL;
>   else
>   current->securebits
>   = (current->securebits
>  & ~issecure_mask(SECURE_KEEP_CAPS))
>   | (arg2 << SECURE_KEEP_CAPS);
>   break;
>
> | if (arg2 > 1)
> | return -EINVAL;
> | if (issecure(SECURE_KEEP_CAPS_LOCKED))
> | return -EPERM;
>
> + error = -EINVAL;
> + else
> + current->securebits
> + = (current->securebits
> +& ~issecure_mask(SECURE_KEEP_CAPS))
> + | (arg2 * issecure_mask(SECURE_KEEP_CAPS));
>
> | Seems overly baroque, and since you're not checking arg2>1 if
> | SECURE_KEEP_CAPS_LOCKED was set, this could actually set a wrong bit
> | with the multiplication.  After the above checks a simple
>
> | if (arg2)
> | current->securebits |= issecure_mask(SECURE_KEEP_CAPS);
> | else
> | current->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);
>
> | would be much easier to read...
>
> I agree. Both that -EPERM is a better error for the locked case, and the
> subsequent if / else. So, I'll use that.

thanks,
-serge
-
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] per-process securebits

[Andrew G. Morgan]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1



[EMAIL PROTECTED] wrote:
| Quoting Andrew G. Morgan ([EMAIL PROTECTED]):
| Here is my latest per-process secure-bits patch.
|
|> Hey Andrew,
|
|> looks really good.  Two comments inline.

Thanks for the review!

- - unsigned keep_capabilities:1;
+   unsigned securebits;

| should this maybe be an unsigned short?

I'm not sure what this would buy us. Given the placement in the data
structure its going to end up aligned by the surrounding data types and
will occupy at least a sizeof(unsigned) amount of space even if we
declare it as a short. Could you clarify why you think a short would be
better?

+   case PR_GET_KEEPCAPS:
+   if (issecure(SECURE_KEEP_CAPS))
+   error = 1;
+   break;
+   case PR_SET_KEEPCAPS:
+   if ((arg2 > 1) && !issecure(SECURE_KEEP_CAPS_LOCKED))

| I don't understand what you're doing here.  If I had to guess, I'd say
| you're only enforcing a check on a valid arg2 (which is 0 or 1) only if
| SECURE_KEEP_CAPS_LOCKED is not set since otherwise the value you can't
| be changed?  But you don't enforce that so it can in fact be changed,
| and even if you did this seems to give the user poor feedback in more than
| one case, so it seems nicer to do

I think the long and the short of it was that I clearly got distracted
when writing that code! It didn't help that all of my testing was with
the PR_*_SECUREBITS prctl()... What I had meant to write was this:

case PR_SET_KEEPCAPS:
/* Note, we rely on arg2 being unsigned here: */
if ((arg2 > 1) || issecure(SECURE_KEEP_CAPS_LOCKED))
error = -EINVAL;
else
current->securebits
= (current->securebits
   & ~issecure_mask(SECURE_KEEP_CAPS))
| (arg2 << SECURE_KEEP_CAPS);
break;

|   if (arg2 > 1)
|   return -EINVAL;
|   if (issecure(SECURE_KEEP_CAPS_LOCKED))
|   return -EPERM;

+   error = -EINVAL;
+   else
+   current->securebits
+   = (current->securebits
+  & ~issecure_mask(SECURE_KEEP_CAPS))
+   | (arg2 * issecure_mask(SECURE_KEEP_CAPS));

| Seems overly baroque, and since you're not checking arg2>1 if
| SECURE_KEEP_CAPS_LOCKED was set, this could actually set a wrong bit
| with the multiplication.  After the above checks a simple

|   if (arg2)
|   current->securebits |= issecure_mask(SECURE_KEEP_CAPS);
|   else
|   current->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);

| would be much easier to read...

I agree. Both that -EPERM is a better error for the locked case, and the
subsequent if / else. So, I'll use that.

Thanks

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

iD8DBQFHoBN0+bHCR3gb8jsRAqRkAJ99ABdBV6iP61MR/OEqmyj+aKOhrACcDuT2
Qm89xx7i9/wOEPLoOASFZHs=
=Zao3
-END PGP SIGNATURE-
-
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] per-process securebits

[serge]

Quoting Andrew G. Morgan ([EMAIL PROTECTED]):
> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA1
>
> Here is my latest per-process secure-bits patch.

Hey Andrew,

looks really good.  Two comments inline.

> Cheers
>
> Andrew
> -BEGIN PGP SIGNATURE-
> Version: GnuPG v1.2.6 (GNU/Linux)
>
> iD8DBQFHmg44+bHCR3gb8jsRAqPoAJ9IrlrQLKNcw8c4T0pgCmn/Lcng7wCfYjVI
> Tu1ufhQCjaMjuUizjJuMvrM=
> =NiGN
> -END PGP SIGNATURE-

> From 16fe33a1f6ab9957c83d4e74b67a25f920f2e7ba Mon Sep 17 00:00:00 2001
> From: Andrew G. Morgan <[EMAIL PROTECTED]>
> Date: Wed, 23 Jan 2008 23:45:21 -0800
> Subject: [PATCH] Implement per-process, prctl-based, securebits
> 
> With filesystem capabilities it is now possible to do away with
> (set)uid-0 based privilege and use capabilities instead.
> 
> Historically, this was first attempted with a kernel-global set of
> securebits. That implementation, however, proved problematic, and has
> slowly whithered in the kernel. Prior to this patch, there remained no
> interface for manipulating the securebits - and thus no interface for
> suppressing root as all-capable.
> 
> This patch reimplements securebits, with bit locking, as a per-process
> value. (To avoid increasing the per-task footprint of this change,
> I've merged the implementation of the per-process keep_capabilities
> bit with the per-process securebits value.)
> 
> A process can now drop all legacy privilege (through uid=0), for
> itself and all of its fork()'d/exec()'d children with:
> 
>   prctl(PR_SET_SECUREBITS, 0x2f);
> 
> Signed-off-by: Andrew G. Morgan <[EMAIL PROTECTED]>
> 
> PS. Applying the following patch to progs/capsh.c from libcap-2.05
> adds support for this new prctl interface to capsh.c:
> 
> http://www.kernel.org/pub/linux/libs/security/linux-privs/libcap2/support-for-prctl-based-securebits.patch
> ---
>  include/linux/capability.h |3 +-
>  include/linux/init_task.h  |3 +-
>  include/linux/prctl.h  |9 +++-
>  include/linux/sched.h  |3 +-
>  include/linux/securebits.h |   25 ---
>  include/linux/security.h   |   14 ---
>  kernel/sys.c   |   25 +--
>  security/capability.c  |1 +
>  security/commoncap.c   |  101 
> 
>  security/dummy.c   |2 +-
>  security/security.c|4 +-
>  security/selinux/hooks.c   |5 +-
>  12 files changed, 137 insertions(+), 58 deletions(-)
> 
> diff --git a/include/linux/capability.h b/include/linux/capability.h
> index 7d50ff6..eaab759 100644
> --- a/include/linux/capability.h
> +++ b/include/linux/capability.h
> @@ -155,6 +155,7 @@ typedef struct kernel_cap_struct {
>   *   Add any capability from current's capability bounding set
>   *   to the current process' inheritable set
>   *   Allow taking bits out of capability bounding set
> + *   Allow modification of the securebits for a process
>   */
>  
>  #define CAP_SETPCAP  8
> @@ -490,8 +491,6 @@ extern const kernel_cap_t __cap_init_eff_set;
>  int capable(int cap);
>  int __capable(struct task_struct *t, int cap);
>  
> -extern long cap_prctl_drop(unsigned long cap);
> -
>  #endif /* __KERNEL__ */
>  
>  #endif /* !_LINUX_CAPABILITY_H */
> diff --git a/include/linux/init_task.h b/include/linux/init_task.h
> index b0fa0f2..81f5582 100644
> --- a/include/linux/init_task.h
> +++ b/include/linux/init_task.h
> @@ -9,6 +9,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #define INIT_FDTABLE \
> @@ -170,7 +171,7 @@ extern struct group_info init_groups;
>   .cap_inheritable = CAP_INIT_INH_SET,\
>   .cap_permitted  = CAP_FULL_SET, \
>   .cap_bset   = CAP_INIT_BSET,\
> - .keep_capabilities = 0, \
> + .securebits = SECUREBITS_DEFAULT,   \
>   .user   = INIT_USER,\
>   .comm   = "swapper",\
>   .thread = INIT_THREAD,  \
> diff --git a/include/linux/prctl.h b/include/linux/prctl.h
> index 3800639..b6c15cc 100644
> --- a/include/linux/prctl.h
> +++ b/include/linux/prctl.h
> @@ -16,7 +16,8 @@
>  # define PR_UNALIGN_NOPRINT  1   /* silently fix up unaligned user 
> accesses */
>  # define PR_UNALIGN_SIGBUS   2   /* generate SIGBUS on unaligned user 
> access */
>  
> -/* Get/set whether or not to drop capabilities on setuid() away from uid 0 */
> +/* Get/set whether or not to drop capabilities on setuid() away from
> + * uid 0 (as per security/commoncap.c) */
>  #define PR_GET_KEEPCAPS   7
>  #define PR_SET_KEEPCAPS   8
>  
> @@ -63,8 +64,12 @@
>  #define PR_GET_SECCOMP   21
>  #define PR_SET_SECCOMP   22
>  
> -/* Get/set the capability bounding set */
> +/* Get/set the capability bounding set (as per security/commoncap.c) */
>  

Re: [RFC PATCH] per-process securebits

[Serge E. Hallyn]

Quoting Andrew G. Morgan ([EMAIL PROTECTED]):
> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA1
>
> Here is my latest per-process secure-bits patch.

Thanks Andrew, I'll check this out tonight or this weekend.

-serge

>
> Cheers
>
> Andrew
> -BEGIN PGP SIGNATURE-
> Version: GnuPG v1.2.6 (GNU/Linux)
>
> iD8DBQFHmg44+bHCR3gb8jsRAqPoAJ9IrlrQLKNcw8c4T0pgCmn/Lcng7wCfYjVI
> Tu1ufhQCjaMjuUizjJuMvrM=
> =NiGN
> -END PGP SIGNATURE-

> From 16fe33a1f6ab9957c83d4e74b67a25f920f2e7ba Mon Sep 17 00:00:00 2001
> From: Andrew G. Morgan <[EMAIL PROTECTED]>
> Date: Wed, 23 Jan 2008 23:45:21 -0800
> Subject: [PATCH] Implement per-process, prctl-based, securebits
> 
> With filesystem capabilities it is now possible to do away with
> (set)uid-0 based privilege and use capabilities instead.
> 
> Historically, this was first attempted with a kernel-global set of
> securebits. That implementation, however, proved problematic, and has
> slowly whithered in the kernel. Prior to this patch, there remained no
> interface for manipulating the securebits - and thus no interface for
> suppressing root as all-capable.
> 
> This patch reimplements securebits, with bit locking, as a per-process
> value. (To avoid increasing the per-task footprint of this change,
> I've merged the implementation of the per-process keep_capabilities
> bit with the per-process securebits value.)
> 
> A process can now drop all legacy privilege (through uid=0), for
> itself and all of its fork()'d/exec()'d children with:
> 
>   prctl(PR_SET_SECUREBITS, 0x2f);
> 
> Signed-off-by: Andrew G. Morgan <[EMAIL PROTECTED]>
> 
> PS. Applying the following patch to progs/capsh.c from libcap-2.05
> adds support for this new prctl interface to capsh.c:
> 
> http://www.kernel.org/pub/linux/libs/security/linux-privs/libcap2/support-for-prctl-based-securebits.patch
> ---
>  include/linux/capability.h |3 +-
>  include/linux/init_task.h  |3 +-
>  include/linux/prctl.h  |9 +++-
>  include/linux/sched.h  |3 +-
>  include/linux/securebits.h |   25 ---
>  include/linux/security.h   |   14 ---
>  kernel/sys.c   |   25 +--
>  security/capability.c  |1 +
>  security/commoncap.c   |  101 
> 
>  security/dummy.c   |2 +-
>  security/security.c|4 +-
>  security/selinux/hooks.c   |5 +-
>  12 files changed, 137 insertions(+), 58 deletions(-)
> 
> diff --git a/include/linux/capability.h b/include/linux/capability.h
> index 7d50ff6..eaab759 100644
> --- a/include/linux/capability.h
> +++ b/include/linux/capability.h
> @@ -155,6 +155,7 @@ typedef struct kernel_cap_struct {
>   *   Add any capability from current's capability bounding set
>   *   to the current process' inheritable set
>   *   Allow taking bits out of capability bounding set
> + *   Allow modification of the securebits for a process
>   */
>  
>  #define CAP_SETPCAP  8
> @@ -490,8 +491,6 @@ extern const kernel_cap_t __cap_init_eff_set;
>  int capable(int cap);
>  int __capable(struct task_struct *t, int cap);
>  
> -extern long cap_prctl_drop(unsigned long cap);
> -
>  #endif /* __KERNEL__ */
>  
>  #endif /* !_LINUX_CAPABILITY_H */
> diff --git a/include/linux/init_task.h b/include/linux/init_task.h
> index b0fa0f2..81f5582 100644
> --- a/include/linux/init_task.h
> +++ b/include/linux/init_task.h
> @@ -9,6 +9,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #define INIT_FDTABLE \
> @@ -170,7 +171,7 @@ extern struct group_info init_groups;
>   .cap_inheritable = CAP_INIT_INH_SET,\
>   .cap_permitted  = CAP_FULL_SET, \
>   .cap_bset   = CAP_INIT_BSET,\
> - .keep_capabilities = 0, \
> + .securebits = SECUREBITS_DEFAULT,   \
>   .user   = INIT_USER,\
>   .comm   = "swapper",\
>   .thread = INIT_THREAD,  \
> diff --git a/include/linux/prctl.h b/include/linux/prctl.h
> index 3800639..b6c15cc 100644
> --- a/include/linux/prctl.h
> +++ b/include/linux/prctl.h
> @@ -16,7 +16,8 @@
>  # define PR_UNALIGN_NOPRINT  1   /* silently fix up unaligned user 
> accesses */
>  # define PR_UNALIGN_SIGBUS   2   /* generate SIGBUS on unaligned user 
> access */
>  
> -/* Get/set whether or not to drop capabilities on setuid() away from uid 0 */
> +/* Get/set whether or not to drop capabilities on setuid() away from
> + * uid 0 (as per security/commoncap.c) */
>  #define PR_GET_KEEPCAPS   7
>  #define PR_SET_KEEPCAPS   8
>  
> @@ -63,8 +64,12 @@
>  #define PR_GET_SECCOMP   21
>  #define PR_SET_SECCOMP   22
>  
> -/* Get/set the capability bounding set */
> +/* Get/set the capability bounding set (as per 

[RFC PATCH] per-process securebits

[Andrew G. Morgan]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Here is my latest per-process secure-bits patch.

Cheers

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

iD8DBQFHmg44+bHCR3gb8jsRAqPoAJ9IrlrQLKNcw8c4T0pgCmn/Lcng7wCfYjVI
Tu1ufhQCjaMjuUizjJuMvrM=
=NiGN
-END PGP SIGNATURE-
From 16fe33a1f6ab9957c83d4e74b67a25f920f2e7ba Mon Sep 17 00:00:00 2001
From: Andrew G. Morgan <[EMAIL PROTECTED]>
Date: Wed, 23 Jan 2008 23:45:21 -0800
Subject: [PATCH] Implement per-process, prctl-based, securebits

With filesystem capabilities it is now possible to do away with
(set)uid-0 based privilege and use capabilities instead.

Historically, this was first attempted with a kernel-global set of
securebits. That implementation, however, proved problematic, and has
slowly whithered in the kernel. Prior to this patch, there remained no
interface for manipulating the securebits - and thus no interface for
suppressing root as all-capable.

This patch reimplements securebits, with bit locking, as a per-process
value. (To avoid increasing the per-task footprint of this change,
I've merged the implementation of the per-process keep_capabilities
bit with the per-process securebits value.)

A process can now drop all legacy privilege (through uid=0), for
itself and all of its fork()'d/exec()'d children with:

  prctl(PR_SET_SECUREBITS, 0x2f);

Signed-off-by: Andrew G. Morgan <[EMAIL PROTECTED]>

PS. Applying the following patch to progs/capsh.c from libcap-2.05
adds support for this new prctl interface to capsh.c:

http://www.kernel.org/pub/linux/libs/security/linux-privs/libcap2/support-for-prctl-based-securebits.patch
---
 include/linux/capability.h |3 +-
 include/linux/init_task.h  |3 +-
 include/linux/prctl.h  |9 +++-
 include/linux/sched.h  |3 +-
 include/linux/securebits.h |   25 ---
 include/linux/security.h   |   14 ---
 kernel/sys.c   |   25 +--
 security/capability.c  |1 +
 security/commoncap.c   |  101 
 security/dummy.c   |2 +-
 security/security.c|4 +-
 security/selinux/hooks.c   |5 +-
 12 files changed, 137 insertions(+), 58 deletions(-)

diff --git a/include/linux/capability.h b/include/linux/capability.h
index 7d50ff6..eaab759 100644
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -155,6 +155,7 @@ typedef struct kernel_cap_struct {
  *   Add any capability from current's capability bounding set
  *   to the current process' inheritable set
  *   Allow taking bits out of capability bounding set
+ *   Allow modification of the securebits for a process
  */
 
 #define CAP_SETPCAP  8
@@ -490,8 +491,6 @@ extern const kernel_cap_t __cap_init_eff_set;
 int capable(int cap);
 int __capable(struct task_struct *t, int cap);
 
-extern long cap_prctl_drop(unsigned long cap);
-
 #endif /* __KERNEL__ */
 
 #endif /* !_LINUX_CAPABILITY_H */
diff --git a/include/linux/init_task.h b/include/linux/init_task.h
index b0fa0f2..81f5582 100644
--- a/include/linux/init_task.h
+++ b/include/linux/init_task.h
@@ -9,6 +9,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #define INIT_FDTABLE \
@@ -170,7 +171,7 @@ extern struct group_info init_groups;
.cap_inheritable = CAP_INIT_INH_SET,\
.cap_permitted  = CAP_FULL_SET, \
.cap_bset   = CAP_INIT_BSET,\
-   .keep_capabilities = 0, \
+   .securebits = SECUREBITS_DEFAULT,   \
.user   = INIT_USER,\
.comm   = "swapper",\
.thread = INIT_THREAD,  \
diff --git a/include/linux/prctl.h b/include/linux/prctl.h
index 3800639..b6c15cc 100644
--- a/include/linux/prctl.h
+++ b/include/linux/prctl.h
@@ -16,7 +16,8 @@
 # define PR_UNALIGN_NOPRINT1   /* silently fix up unaligned user 
accesses */
 # define PR_UNALIGN_SIGBUS 2   /* generate SIGBUS on unaligned user 
access */
 
-/* Get/set whether or not to drop capabilities on setuid() away from uid 0 */
+/* Get/set whether or not to drop capabilities on setuid() away from
+ * uid 0 (as per security/commoncap.c) */
 #define PR_GET_KEEPCAPS   7
 #define PR_SET_KEEPCAPS   8
 
@@ -63,8 +64,12 @@
 #define PR_GET_SECCOMP 21
 #define PR_SET_SECCOMP 22
 
-/* Get/set the capability bounding set */
+/* Get/set the capability bounding set (as per security/commoncap.c) */
 #define PR_CAPBSET_READ 23
 #define PR_CAPBSET_DROP 24
 
+/* Get/set securebits (as per security/commoncap.c) */
+#define PR_GET_SECUREBITS 25
+#define PR_SET_SECUREBITS 26
+
 #endif /* _LINUX_PRCTL_H */
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 198659b..063f575 100644
--- a/include/linux/sched.h
+++