Fixed bad security loophole in write path

diff --git a/Makefile b/Makefile
index a328711..8dfd78e 100644 (file)
--- a/Makefile
+++ b/Makefile
@@ -1,7 +1,7 @@
 obj-m += procprotect.o
 
 all:
 obj-m += procprotect.o
 
 all:

-       make -C /lib/modules/$(shell uname -r)/build M=$(PWD) modules
+       make -C /lib/modules/3.4.9-2.fc16.x86_64/build M=$(PWD) modules

 
 clean:
        make -C /lib/modules/$(shell uname -r)/build M=$(PWD) clean
 
 clean:
        make -C /lib/modules/$(shell uname -r)/build M=$(PWD) clean

diff --git a/procprotect.c b/procprotect.c
index d49a41b..c1c0162 100644 (file)
--- a/procprotect.c
+++ b/procprotect.c
@@ -45,12 +45,13 @@ MODULE_LICENSE("GPL");
 MODULE_VERSION(VERSION_STR);
 
 struct procprotect_ctx {
 MODULE_VERSION(VERSION_STR);
 
 struct procprotect_ctx {

-       struct inode **inode;
+    struct inode **inode;
+    int flags;

 };
 
 struct acl_entry {
 };
 
 struct acl_entry {

-       unsigned int ino;
-       struct hlist_node hlist;
+    unsigned int ino;
+    struct hlist_node hlist;

 };
 
 #define HASH_SIZE (1<<16)
 };
 
 #define HASH_SIZE (1<<16)


@@ -60,166 +61,205 @@ struct hlist_head procprotect_hash[HASH_SIZE];
 struct proc_dir_entry *proc_entry;
 
 /*
 struct proc_dir_entry *proc_entry;
 
 /*

-  Entry point of intercepted call. We need to do two things here:
-  - Decide if we need the heavier return hook to be called
-  - Save the first argument, which is in a register, for consideration in the return hook
-*/
+   Entry point of intercepted call. We need to do two things here:
+   - Decide if we need the heavier return hook to be called
+   - Save the first argument, which is in a register, for consideration in the return hook
+   */

 static int do_lookup_entry(struct kretprobe_instance *ri, struct pt_regs *regs) {
 static int do_lookup_entry(struct kretprobe_instance *ri, struct pt_regs *regs) {

-       int ret = -1;
-       struct procprotect_ctx *ctx;
-       struct nameidata *nd = (struct nameidata *) regs->di;
-       struct qstr *q = (struct qstr *) regs->si;
-       struct dentry *parent = nd->path.dentry;
-       struct inode *pinode = parent->d_inode;
-       
-       if (pinode->i_sb->s_magic == PROC_SUPER_MAGIC) {        
-               ctx = (struct procprotect_ctx *) ri->data;
-               ctx->inode = (struct inode **) regs->cx;
-               ret = 0;
-       }
-
-       return ret;
+    int ret = -1;
+    struct procprotect_ctx *ctx;
+    struct nameidata *nd = (struct nameidata *) regs->di;
+    struct qstr *q = (struct qstr *) regs->si;
+    struct dentry *parent = nd->path.dentry;
+    struct inode *pinode = parent->d_inode;
+
+    if (pinode->i_sb->s_magic == PROC_SUPER_MAGIC
+            && current->nsproxy->mnt_ns!=init_task.nsproxy->mnt_ns) {  
+        ctx = (struct procprotect_ctx *) ri->data;
+        ctx->inode = regs->cx;
+        ctx->flags = nd->flags;
+        ret = 0;
+    }
+
+    return ret;

 }
 
 static int run_acl(unsigned long ino) {
 }
 
 static int run_acl(unsigned long ino) {

-       struct hlist_node *n;
-       struct acl_entry *entry;
-       hlist_for_each_entry_rcu(entry, 
-               n, &procprotect_hash[ino & (HASH_SIZE-1)],
-               hlist) {
-               if (entry->ino==ino) {
-                       return 0;
-               }
-       }
-       return 1;
+    struct hlist_node *n;
+    struct acl_entry *entry;
+    hlist_for_each_entry_rcu(entry, 
+            n, &procprotect_hash[ino & (HASH_SIZE-1)],
+            hlist) {
+        if (entry->ino==ino) {
+            return 0;
+        }
+    }
+    return 1;

 }
 
 /* The entry hook ensures that the return hook is only called for
 }
 
 /* The entry hook ensures that the return hook is only called for

-accesses to /proc */
+   accesses to /proc */

 
 static int do_lookup_ret(struct kretprobe_instance *ri, struct pt_regs *regs)
 {
 
 static int do_lookup_ret(struct kretprobe_instance *ri, struct pt_regs *regs)
 {

-       struct procprotect_ctx *ctx = (struct procprotect_ctx *) ri->data;
-       int ret = regs->ax;
-       
-       if (ret==0) {
-               /* The kernel is going to honor the request. Here's where we step in */
-               struct inode *inode = *(ctx->inode);
-               //printk(KERN_CRIT "Checking inode %x number %u",inode,inode->i_ino);
-               if (!run_acl(inode->i_ino)) {
-                       if (current->nsproxy->mnt_ns!=init_task.nsproxy->mnt_ns)
-                               regs->ax = -EPERM;
-               }
-       }
-       
-       return 0;
+    struct procprotect_ctx *ctx = (struct procprotect_ctx *) ri->data;
+    int ret = regs->ax;
+
+    if (ret==0) {
+        /* The kernel is going to honor the request. Here's where we step in */
+        struct inode *inode = *(ctx->inode);
+        if (!run_acl(inode->i_ino)) {
+            if (current->nsproxy->mnt_ns!=init_task.nsproxy->mnt_ns) {
+                regs->ax = -EPERM;
+            }
+        }
+    }
+
+    return 0;

 }
 
 }
 

+struct open_flags {
+  int open_flag;
+  umode_t mode;
+  int acc_mode;
+  int intent;
+};
+
+static struct file *do_last_probe(struct nameidata *nd, struct path *path,
+                         struct open_flags *op, const char *pathname) {
+    struct dentry *parent = nd->path.dentry;
+    struct inode *pinode = parent->d_inode;
+
+    if (pinode->i_sb->s_magic == PROC_SUPER_MAGIC && current->nsproxy->mnt_ns!=init_task.nsproxy->mnt_ns) {
+        op->open_flag &= ~O_CREAT;
+    }
+    jprobe_return();
+}
+
+static struct jprobe dolast_probe = {
+       .entry = (kprobe_opcode_t *) do_last_probe
+};
+

 static struct kretprobe proc_probe = {
 static struct kretprobe proc_probe = {

-       .entry_handler = (kprobe_opcode_t *) do_lookup_entry,
-        .handler = (kprobe_opcode_t *) do_lookup_ret,
-       .maxactive = 20,
-       .data_size = sizeof(struct procprotect_ctx)
+    .entry_handler = (kprobe_opcode_t *) do_lookup_entry,
+    .handler = (kprobe_opcode_t *) do_lookup_ret,
+    .maxactive = 20,
+    .data_size = sizeof(struct procprotect_ctx)

 };
 
 static void add_entry(char *pathname) {
 };
 
 static void add_entry(char *pathname) {

-       struct path path;
-       if (kern_path(pathname, 0, &path)) {
-               printk(KERN_CRIT "Path lookup failed for %s",pathname);
-       }       
-       else {
-               unsigned int ino = path.dentry->d_inode->i_ino;
-               struct acl_entry *entry;
-               entry = kmalloc(GFP_KERNEL, sizeof(struct acl_entry));
-               entry->ino = ino;
-               
-               if (!entry) {
-                       printk(KERN_CRIT "Could not allocate memory for %s",pathname);
-               }
-               else {
-                       if (run_acl(ino)) {
-                               hlist_add_head_rcu(&entry->hlist,&procprotect_hash[ino&(HASH_SIZE-1)]);
-                               printk(KERN_CRIT "Added inode %u",ino);
-                       }
-                       else {
-                               printk(KERN_CRIT "Did not add inode %u, already in list", ino);
-                       }
-               }
-       }
+    struct path path;
+    if (kern_path(pathname, 0, &path)) {
+        printk(KERN_CRIT "Path lookup failed for %s",pathname);
+    }  
+    else {
+        unsigned int ino = path.dentry->d_inode->i_ino;
+        struct acl_entry *entry;
+        entry = kmalloc(GFP_KERNEL, sizeof(struct acl_entry));
+        entry->ino = ino;
+
+        if (!entry) {
+            printk(KERN_CRIT "Could not allocate memory for %s",pathname);
+        }
+        else {
+            if (run_acl(ino)) {
+                hlist_add_head_rcu(&entry->hlist,&procprotect_hash[ino&(HASH_SIZE-1)]);
+                printk(KERN_CRIT "Added inode %u",ino);
+            }
+            else {
+                printk(KERN_CRIT "Did not add inode %u, already in list", ino);
+            }
+        }
+    }

 }
 
 
 static void __exit procprotect_exit(void)
 {
 }
 
 
 static void __exit procprotect_exit(void)
 {

-       unregister_kretprobe(&proc_probe);
-       struct hlist_node *n;
-       struct acl_entry *entry;
-       int i;
-
-       for (i=0;i<HASH_SIZE;i++) {
-               hlist_for_each_entry_rcu(entry, 
-                       n, &procprotect_hash[i],
-                       hlist) {
-                               kfree(entry);
-               }
-       }
-       
-       remove_proc_entry("procprotect",NULL);
-       printk("Procprotect: Stopped procprotect.\n");
+    unregister_kretprobe(&proc_probe);
+       unregister_jprobe(&dolast_probe);
+    struct hlist_node *n;
+    struct acl_entry *entry;
+    int i;
+
+    for (i=0;i<HASH_SIZE;i++) {
+        hlist_for_each_entry_rcu(entry, 
+                n, &procprotect_hash[i],
+                hlist) {
+            kfree(entry);
+        }
+    }
+
+    remove_proc_entry("procprotect",NULL);
+    printk("Procprotect: Stopped procprotect.\n");

 }
 
 
 
 int procfile_write(struct file *file, const char *buffer, unsigned long count, void *data) {           
 }
 
 
 
 int procfile_write(struct file *file, const char *buffer, unsigned long count, void *data) {           

-       char pathname[PATH_MAX];
-
-       if (current->nsproxy->mnt_ns!=init_task.nsproxy->mnt_ns)
-               return -EPERM;
-
-       if (copy_from_user(pathname, buffer, count)) {
-               return -EFAULT;
-       }
-       if (count && (pathname[count-1]==10 || pathname[count-1]==13)) {
-               pathname[count-1]='\0';
-       }
-       else
-               pathname[count]='\0';
-       add_entry(pathname);    
-       printk(KERN_CRIT "Length of buffer=%d",strlen(pathname));
-       return count;
+    char pathname[PATH_MAX];
+
+    if (current->nsproxy->mnt_ns!=init_task.nsproxy->mnt_ns)
+        return -EPERM;
+
+    if (copy_from_user(pathname, buffer, count)) {
+        return -EFAULT;
+    }
+    if (count && (pathname[count-1]==10 || pathname[count-1]==13)) {
+        pathname[count-1]='\0';
+    }
+    else
+        pathname[count]='\0';
+    add_entry(pathname);       
+    printk(KERN_CRIT "Length of buffer=%d",strlen(pathname));
+    return count;

 }
 
 static int __init procprotect_init(void)
 {
 }
 
 static int __init procprotect_init(void)
 {

-       printk("Procprotect: starting procprotect version %s with ACLs at path %s.\n",
-              VERSION_STR, aclpath);
-       int ret;
-       int i;
-
-       for(i=0;i<HASH_SIZE;i++) {
-               INIT_HLIST_HEAD(&procprotect_hash[i]);
-       }
-
-         aclqpath.name = aclpath;
-         aclqpath.len = strnlen(aclpath, PATH_MAX);
-
-          proc_probe.kp.addr = 
-                  (kprobe_opcode_t *) kallsyms_lookup_name("do_lookup");
-          if (!proc_probe.kp.addr) {
-                  printk("Couldn't find %s to plant kretprobe\n", "do_execve");
-                  return -1;
-          }
-  
-          if ((ret = register_kretprobe(&proc_probe)) <0) {
-                  printk("register_kretprobe failed, returned %d\n", ret);
+    printk("Procprotect: starting procprotect version %s with ACLs at path %s.\n",
+            VERSION_STR, aclpath);
+    int ret;
+    int i;
+
+    for(i=0;i<HASH_SIZE;i++) {
+        INIT_HLIST_HEAD(&procprotect_hash[i]);
+    }
+
+    aclqpath.name = aclpath;
+    aclqpath.len = strnlen(aclpath, PATH_MAX);
+
+    proc_probe.kp.addr = 
+        (kprobe_opcode_t *) kallsyms_lookup_name("do_lookup");
+    if (!proc_probe.kp.addr) {
+        printk("Couldn't find %s to plant kretprobe\n", "do_execve");
+        return -1;
+    }
+
+    dolast_probe.kp.addr = 
+        (kprobe_opcode_t *) kallsyms_lookup_name("do_last");
+
+    if (!dolast_probe.kp.addr) {
+        printk("Couldn't find %s to plant kretprobe\n", "do_last");
+        return -1;
+    }
+
+    if ((ret = register_jprobe(&dolast_probe)) <0) {
+                  printk("register_jprobe failed, returned %u\n", ret);

                   return -1;
                   return -1;

-          }
-          printk("Planted kretprobe at %p, handler addr %p\n",
-                 proc_probe.kp.addr, proc_probe.handler);
+    }
+
+    if ((ret = register_kretprobe(&proc_probe)) <0) {
+        printk("register_kretprobe failed, returned %d\n", ret);
+        return -1;
+    }
+    printk("Planted kretprobe at %p, handler addr %p\n",
+            proc_probe.kp.addr, proc_probe.handler);
+
+    proc_entry = create_proc_entry("procprotect", 0644, NULL);
+    proc_entry->write_proc = procfile_write;

 
 

-       proc_entry = create_proc_entry("procprotect", 0644, NULL);
-       proc_entry->write_proc = procfile_write;
-        return ret;
+    return ret;

 }
 
 
 }