Re: [PATCH][RFC] /proc umask and gid [was: Make /proc/pid chmod'able]

2005-03-18 Thread Rene Scharfe
On Tue, Mar 15, 2005 at 11:41:28PM -0500, Albert Cahalan wrote:
 Better interface:
 
 /sbin/sysctl -w proc.maps=0440
 /sbin/sysctl -w proc.cmdline=0444
 /sbin/sysctl -w proc.status=0444

Indeed it is, but it's much harder to implement.

Patch is against 2.6.11-mm3 but works with -mm4, too.  I only made the
permissions configurable for some of the files, more can be added easily
if and when needed.

It's uglier than it needs to be, but before cleaning it up I'll go and
get some sleep.

Comments and patches much appreciated! :)

Rene



diff -pur linux-2.6.11-mm3/fs/proc/base.c l5/fs/proc/base.c
--- linux-2.6.11-mm3/fs/proc/base.c 2005-03-12 19:23:36.0 +0100
+++ l5/fs/proc/base.c   2005-03-19 01:08:40.0 +0100
@@ -35,6 +35,7 @@
 #include linux/seccomp.h
 #include linux/cpuset.h
 #include linux/audit.h
+#include linux/sysctl.h
 #include internal.h
 
 /*
@@ -1419,6 +1420,93 @@ static struct file_operations proc_tgid_
 static struct inode_operations proc_tgid_attr_inode_operations;
 #endif
 
+static struct pid_entry *proc_base_entry_by_name(struct pid_entry *ents,
+ struct qstr *name)
+{
+   struct pid_entry *p;
+
+   for (p = ents; p-name; p++) {
+   if (p-len != name-len)
+   continue;
+   if (!memcmp(name-name, p-name, p-len))
+   return p;
+   }
+   return NULL;
+}
+
+#ifdef CONFIG_SYSCTL
+struct proc_sysctl_info {
+   int type;
+   mode_t mode;
+};
+
+/* order must match the CTL_PROC table in sysctl.h */
+struct proc_sysctl_info proc_base_permissions[] = {
+   {PROC_TGID_CMDLINE, S_IRUGO},
+   {PROC_TGID_MAPS,S_IRUGO},
+   {PROC_TGID_STAT,S_IRUGO},
+   {PROC_TGID_STATM,   S_IRUGO},
+   {PROC_TGID_STATUS,  S_IRUGO},
+   {0, 0}
+};
+
+static inline int is_system_task(struct task_struct *task)
+{
+   int rc = 0;
+
+   if (task-pid == 1)
+   rc = 1;
+   // TODO: kernel threads?
+   return rc;
+}
+
+static void proc_update_mode(mode_t *mode, int type, struct task_struct *task)
+{
+   struct proc_sysctl_info *s;
+
+   if (is_system_task(task))
+   return;
+   for (s = proc_base_permissions; s-type; s++) {
+   if (s-type == type) {
+   *mode = (*mode  ~S_IALLUGO) | s-mode;
+   break;
+   }
+   }
+}
+
+static int proc_base_permission(struct inode *inode, int mask,
+struct nameidata *nd)
+{
+   struct task_struct *task = proc_task(inode);
+   struct pid_entry *p;
+
+   p = proc_base_entry_by_name(tgid_base_stuff, nd-dentry-d_name);
+   if (p)
+   proc_update_mode(inode-i_mode, p-type, task);
+   return generic_permission(inode, mask, NULL);
+}
+
+static int proc_base_getattr(struct vfsmount *mnt, struct dentry *dentry,
+ struct kstat *stat)
+{
+   struct inode *inode = dentry-d_inode;
+   struct task_struct *task = proc_task(inode);
+   struct pid_entry *p;
+
+   generic_fillattr(inode, stat);
+   p = proc_base_entry_by_name(tgid_base_stuff, dentry-d_name);
+   if (p)
+   proc_update_mode(inode-i_mode, p-type, task);
+   return 0;
+}
+
+static struct inode_operations proc_base_inode_operations = {
+   .getattr= proc_base_getattr,
+   .permission = proc_base_permission,
+};
+
+#endif /* CONFIG_SYSCTL */
+
 /* SMP-safe */
 static struct dentry *proc_pident_lookup(struct inode *dir, 
 struct dentry *dentry,
@@ -1436,13 +1524,8 @@ static struct dentry *proc_pident_lookup
if (!pid_alive(task))
goto out;
 
-   for (p = ents; p-name; p++) {
-   if (p-len != dentry-d_name.len)
-   continue;
-   if (!memcmp(dentry-d_name.name, p-name, p-len))
-   break;
-   }
-   if (!p-name)
+   p = proc_base_entry_by_name(ents, dentry-d_name);
+   if (p == NULL)
goto out;
 
error = -EINVAL;
@@ -1452,6 +1535,10 @@ static struct dentry *proc_pident_lookup
 
ei = PROC_I(inode);
inode-i_mode = p-mode;
+#ifdef CONFIG_SYSCTL
+   proc_update_mode(inode-i_mode, p-type, task);
+   inode-i_op = proc_base_inode_operations;
+#endif
/*
 * Yes, it does not scale. And it should not. Don't add
 * new entries into /proc/tgid/ without very good reasons.
diff -pur linux-2.6.11-mm3/include/linux/sysctl.h l5/include/linux/sysctl.h
--- linux-2.6.11-mm3/include/linux/sysctl.h 2005-03-12 19:23:37.0 
+0100
+++ l5/include/linux/sysctl.h   2005-03-18 23:40:59.0 +0100
@@ -654,6 +654,13 @@ enum {
 };
 
 /* CTL_PROC names: */
+enum {
+   PROC_CMDLINE= 1,
+   PROC_MAPS   = 2,
+   PROC_STAT   = 3,
+   PROC_STATM  = 4,
+   

Re: [PATCH][RFC] /proc umask and gid [was: Make /proc/pid chmod'able]

2005-03-15 Thread Rene Scharfe
So, I gather from the feedback I've got that chmod'able /proc/pid
would be a bit over the top. 8-)  While providing the easiest and most
intuitive user interface for changing the permissions on those
directories, it is overkill.  Paul is right when he says that such a
feature should be turned on or off for all sessions at once, and that's
it.

My patch had at least one other problem: the contents of eac
/proc/pid directory became chmod'able, too, which was not intended.

Instead of fixing it up I took two steps back, dusted off the umask
kernel parameter patch and added the special gid feature I mentioned.

Without the new kernel parameters behaviour is unchanged.  Add
proc.umask=077 and all /proc/pid will get a permission mode of 500.
This breaks pstree (no output), as Bodo already noted, because this
program needs access to /proc/1.  It also breaks w -- it shows the
correct number of users but it lists X even for sessions owned
by the user running it.

Use proc.umask=007 and proc.gid=50 instead and all /proc/pid dirs
will have a mode of 550 and their group attribute will be set to 50
(that's staff on my Debian system).  Pstree will work for all members
of that special group (just like top, ps and w -- which also show
everything in that case).  Normal users will still have a restricted
view.

Albert, would you take fixes for w even though you despise the feature
that makes them necessary?

Is this less scary?  Still useful?

Thanks,
Rene



diff -urp linux-2.6.11-mm3/Documentation/kernel-parameters.txt 
l5/Documentation/kernel-parameters.txt
--- linux-2.6.11-mm3/Documentation/kernel-parameters.txt2005-03-12 
19:23:30.0 +0100
+++ l5/Documentation/kernel-parameters.txt  2005-03-16 01:14:05.0 
+0100
@@ -1095,16 +1095,22 @@ running once the system is up.
[ISAPNP] Exclude memory regions for the 
autoconfiguration
Ranges are in pairs (memory base and size).
 
+   processor.max_cstate=   [HW, ACPI]
+   Limit processor to maximum C-state
+   max_cstate=9 overrides any DMI blacklist limit.
+
+   proc.gid=   [KNL] If non-zero all /proc/pid directories will
+   have their group attribute set to that value.
+
+   proc.umask= [KNL] Restrict permissions of process specific
+   entries in /proc (i.e. the numerical directories).
+
profile=[KNL] Enable kernel profiling via /proc/profile
{ schedule | number }
(param: schedule - profile schedule points}
(param: profile step/bucket size as a power of 2 for
statistical time based profiling)
 
-   processor.max_cstate=   [HW, ACPI]
-   Limit processor to maximum C-state
-   max_cstate=9 overrides any DMI blacklist limit.
-
prompt_ramdisk= [RAM] List of RAM disks to prompt for floppy disk
before loading.
See Documentation/ramdisk.txt.
diff -urp linux-2.6.11-mm3/fs/proc/base.c l5/fs/proc/base.c
--- linux-2.6.11-mm3/fs/proc/base.c 2005-03-12 19:23:36.0 +0100
+++ l5/fs/proc/base.c   2005-03-16 01:54:52.0 +0100
@@ -35,8 +35,18 @@
 #include linux/seccomp.h
 #include linux/cpuset.h
 #include linux/audit.h
+#include linux/module.h
+#include linux/moduleparam.h
 #include internal.h
 
+static umode_t proc_umask;
+module_param_named(umask, proc_umask, ushort, 0);
+MODULE_PARM_DESC(umask, umask for all /proc/pid entries);
+
+static gid_t proc_gid;
+module_param_named(gid, proc_gid, uint, 0);
+MODULE_PARM_DESC(gid, group attribute of all /proc/pid entries);
+
 /*
  * For hysterical raisins we keep the same inumbers as in the old procfs.
  * Feel free to change the macro below - just keep the range distinct from
@@ -1149,6 +1159,8 @@ static struct inode *proc_pid_make_inode
inode-i_uid = task-euid;
inode-i_gid = task-egid;
}
+   if ((ino == PROC_TGID_INO || ino == PROC_TID_INO)  proc_gid)
+   inode-i_gid = proc_gid;
security_task_to_inode(task, inode);
 
 out:
@@ -1182,6 +1194,8 @@ static int pid_revalidate(struct dentry 
inode-i_uid = 0;
inode-i_gid = 0;
}
+   if ((proc_type(inode) == PROC_TGID_INO || proc_type(inode) == 
PROC_TID_INO)  proc_gid)
+   inode-i_gid = proc_gid;
security_task_to_inode(task, inode);
return 1;
}
@@ -1797,7 +1811,7 @@ struct dentry *proc_pid_lookup(struct in
put_task_struct(task);
goto out;
}
-   inode-i_mode = S_IFDIR|S_IRUGO|S_IXUGO;
+   inode-i_mode = S_IFDIR | ((S_IRUGO|S_IXUGO)  ~proc_umask);
inode-i_op = proc_tgid_base_inode_operations;
inode-i_fop = proc_tgid_base_operations;

Re: [PATCH][RFC] /proc umask and gid [was: Make /proc/pid chmod'able]

2005-03-15 Thread Albert Cahalan
On Wed, 2005-03-16 at 03:39 +0100, Rene Scharfe wrote:
 So, I gather from the feedback I've got that chmod'able /proc/pid
 would be a bit over the top. 8-)  While providing the easiest and most
 intuitive user interface for changing the permissions on those
 directories, it is overkill.  Paul is right when he says that such a
 feature should be turned on or off for all sessions at once, and that's
 it.
 
 My patch had at least one other problem: the contents of eac
 /proc/pid directory became chmod'able, too, which was not intended.
 
 Instead of fixing it up I took two steps back, dusted off the umask
 kernel parameter patch and added the special gid feature I mentioned.
 
 Without the new kernel parameters behaviour is unchanged.  Add
 proc.umask=077 and all /proc/pid will get a permission mode of 500.
 This breaks pstree (no output), as Bodo already noted, because this
 program needs access to /proc/1.  It also breaks w -- it shows the
 correct number of users but it lists X even for sessions owned
 by the user running it.
 
 Use proc.umask=007 and proc.gid=50 instead and all /proc/pid dirs
 will have a mode of 550 and their group attribute will be set to 50
 (that's staff on my Debian system).  Pstree will work for all members
 of that special group (just like top, ps and w -- which also show
 everything in that case).  Normal users will still have a restricted
 view.
 
 Albert, would you take fixes for w even though you despise the feature
 that makes them necessary?

I will take patches if they are not too messy and they do not
cause tools to report garbage output. For example, I do not
wish to have tools reporting -1, 0, or uninitialized data in
place of correct data.

Distinct controls for the various files could be useful.
I might want to make /proc/*/cmdline be public, or make
/proc/*/maps be private. This is particularly helpful if
a low-security file is added for bare-bones ps operation.

You might make a special exception for built-in kernel tasks
and init.


-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH][RFC] /proc umask and gid [was: Make /proc/pid chmod'able]

2005-03-15 Thread Albert Cahalan
Better interface:

/sbin/sysctl -w proc.maps=0440
/sbin/sysctl -w proc.cmdline=0444
/sbin/sysctl -w proc.status=0444

The /etc/sysctl.conf file can be used to set these
at boot time.


-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/