Re: [RFC PATCH] per-process securebits
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 arg21 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: [PATCH 24/27] NFS: Use local caching [try #2]
Hi David-
On Jan 29, 2008, at 10:25 PM, David Howells wrote:
Chuck Lever [EMAIL PROTECTED] wrote:
This patch really ought to be broken into more manageable atomic
changes to make it easier to review, and to provide more fine-grained
explanation and rationalization for each specific change via
individual patch descriptions.
Hmmm I broke the patch up as Trond stipulated - at least, I
thought I
had.
In many ways this request doesn't make sense. You can't do NFS
caching
without all the appropriate bits, so logically they should be one
patch.
Breaking it up won't help git-bisect since the option to enable all
this is
the last (or nearly last) patch.
In addition to adding a new feature, you are changing existing code.
If any one of the changes you made breaks existing behavior, having
them all in small atomic patches makes it practical to bisect and
find the problem.
In addition it makes it worlds easier to review by people who are not
so familiar with your fscache implementation. And smaller patches
means the ratio of patch descriptions to code changes can be much
higher.
It does make sense to introduce the files under fs/fsc in a single
patch. But when you are changing code that is already being used,
more care needs to be taken.
This should no longer be necessary. The latest mount.nfs subcommand
from nfs-utils supports text-based mounts when running on kernels
2.6.23 and later.
Okay. I'll update my patches to reflect this. Note, however, I've
got
someone reporting a bug that seems to show otherwise. I'll have to
investigate this more next week.
The very latest version (post 1.1.1) is required today for text-based
NFS mounts. (That is, the bleeding edge version you get by cloning
the nfs-utils git repo).
And it only works on kernels later than 2.6.22 -- if that particular
user is testing fscache on 2.6.22 or older, then only the legacy
binary NFS mount system call API is supported.
Add comments like this in a separate clean up patch.
+/*
+ * Notification that a PTE pointing to an NFS page is about to be made
+ * writable, implying that someone is about to modify the page
through a
+ * shared-writable mapping
+ */
What does that have to do with local disk caching?
+struct nfs_fh_auxdata {
+ struct timespec i_mtime;
+ struct timespec i_ctime;
+ loff_t i_size;
+};
It might be useful to explain here why you need to supplement the
mtime, ctime, and size fields that already exist in an NFS inode.
Supplement? I don't understand.
Why is it necessary to add additional mtime, ctime and size fields
for NFS inodes? Similar metadata is already stored in nfsi.
All I'm asking for is some documentation of what these fields do that
the existing time stamps and size fields in nfsi don't. Explain why
the NFS fsc implementation needs this data structure.
+ key-port = clp-cl_addr.sin_port;
Not sure why you are using the server's port here. In almost every
case the server side port number will be 2049, so it really doesn't
add any uniquification.
The reason lies is in almost every case. It's possible to
configure it
such that a server is running two separate NFS servers on different
ports.
We should explore whether it is typical or even possible that such a
configuration exports the same file handles on different ports, and
whether that really matters to the client.
I strongly recommend you use the existing IPv6 address conversion
macros for this instead of open-coding yet another way of mapping an
IPv4 address to an IPv6 address.
However, since AF_INET6 support is being introduced in the NFS client
in 2.6.24, I recommend you take a look at these source files after
Trond has pushed his NFS_ALL for 2.6.24.
I'll look at them.
I always do this: I meant 2.6.25, not 2.6.24.
By the time you return, basic IPv6 support for NFSv4 should be in
2.6.25-rc1's NFS client (not server). Not that it is bug-free, but
an implementation is now there.
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
-
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
[PATCH] per-process securebits
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
Here is the patch adding per-process secure-bits. This patch was
generated over 2.6.24-rc8-mm1 + my privilege escalation bugfix.
Cheers
Andrew
Ref: 6a63d67f37e50dd2031b3a050ebac1e64eae916e
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.2.6 (GNU/Linux)
iD8DBQFHoXKG+bHCR3gb8jsRAon4AJ9bGGOjHhzxpgiGdShkcjEYr1+vUwCeJPYh
YqNC8gHO/Kx4ST61G6ZwTXA=
=2fdu
-END PGP SIGNATURE-
From 6a63d67f37e50dd2031b3a050ebac1e64eae916e 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 | 105 +++
security/dummy.c |2 +-
security/security.c|4 +-
security/selinux/hooks.c |5 +-
12 files changed, 140 insertions(+), 59 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 linux/ipc.h
#include linux/pid_namespace.h
#include linux/user_namespace.h
+#include linux/securebits.h
#include net/net_namespace.h
#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
Re: [PATCH] per-process securebits
On Wed, 30 Jan 2008 23:02:30 -0800 Andrew G. Morgan [EMAIL PROTECTED] wrote: 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); This is the sort of patch which strikes terror into many hearts. Please, it cannot be hidden over on the lsm list. I'll assume that this version is an rfc/rfr for now and will cheerily delete it. For the next version, please do circulate it more widely. It will need careful explanation and review. I think it would be useful for this patch's changelog to give us a little recap of what went wrong with capabilities, if that's possible (and if it's relevant). What was the bug which caused us to cripple capability inheritance (some sendmail thing?) and why was that bug considered unfixable at the time and by what means does this new code avoid the same old bug? A bit more changelog-for-dummies would be nice, too. This particular dummy doesn't understand why/how fs-caps made it possible for us to start using capabilities properly. And last, but by no means least: s/whither/wither/ ;) Thanks. - 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
