Tried making the perm permissions faster by perparing queries

Previously, process name was grabbed from `/proc/pid/cmdline`. This was
revealed to be faulty, since the path to the executable might be
relative, and thus would change the result depending on how the program
was called. Also, it made executable renaming a viable bypass of the
entire access control.

I still don't fully undestand how I managed to not think of this before
:)

.gitignore

@@ -4,3 +4,5 @@ build/*
 test/protected/*
 test/.pt.db
 compile_commands.json
+test/perf*
+test/callgraph*

Makefile

@@ -21,14 +21,14 @@ endif
 
 # set up cflags and libs
 
-CFLAGS := -D_FILE_OFFSET_BITS=64
+CFLAGS := -D_FILE_OFFSET_BITS=64 -g
 LDFLAGS :=
 
 CFLAGS += $(shell pkg-config --cflags $(PACKAGE_NAMES))
 LDFLAGS += $(shell pkg-config --libs $(PACKAGE_NAMES))
 
 ifeq ($(DEBUG),1)
-	CFLAGS += -O0 -pedantic -g -Wall -Wextra -Wcast-align \
+	CFLAGS += -O0 -pedantic -Wall -Wextra -Wcast-align \
 						-Wcast-qual -Wdisabled-optimization -Wformat=2 \
 						-Winit-self -Wlogical-op -Wmissing-declarations \
 						-Wmissing-include-dirs -Wredundant-decls -Wshadow \

src/fuse_operations.c

@@ -11,6 +11,7 @@
   See the file LICENSE.
 */
 
+#include <stddef.h>
 #define FUSE_USE_VERSION 31
 
 #define _GNU_SOURCE
@@ -39,23 +40,58 @@
 #include "sourcefs.h"
 #include "ui-socket.h"
 
-// TODO: move this to other file
 const char *get_process_name_by_pid(const int pid) {
-  char *name = (char *)calloc(1024, sizeof(char));
-  if (name) {
-    sprintf(name, "/proc/%d/cmdline", pid);
-    FILE *f = fopen(name, "r");
-    if (f) {
-      size_t size;
-      size = fread(name, sizeof(char), 1024, f);
-      if (size > 0) {
-        if ('\n' == name[size - 1])
-          name[size - 1] = '\0';
-      }
-      fclose(f);
+  char path[1024];
+  sprintf(path, "/proc/%d/exe", pid);
+
+  char *name = realpath(path, NULL);
+  if (name == NULL) {
+    fprintf(stderr, "Could not get process name by pid %d", pid);
+    perror("");
+  }
+
+  /*
+  size_t namelen = 32;
+  ssize_t readret = 0;
+  char *name = NULL;
+  while (namelen >= (size_t)readret && readret > 0) {
+    namelen *= 2;
+    name = calloc(namelen, sizeof(char));
+    if (name == NULL) {
+      free(path);
+      fprintf(stderr, "Could not get get process name by pid %d", pid);
+      perror("");
+      return NULL;
+    }
+    readret = readlink(path, name, namelen);
+    if (readret < 0) {
+      free(name);
+      free(path);
+      fprintf(stderr, "Couldn't get process name by pid %d", pid);
+      perror("");
+      return NULL;
+    }
+    if (namelen >= (size_t)readret) {
+      free(name);
     }
   }
+  */
+
   return name;
+
+  /*
+  FILE *file = fopen(path, "r");
+  if (file) {
+    size_t size = 0;
+    size = fread(path, sizeof(char), 1024, file);
+    if (size > 0) {
+      if ('\n' == path[size - 1]) {
+        path[size - 1] = '\0';
+      }
+    }
+    fclose(file);
+  }
+  */
 }
 
 // TODO: move this somewhere else
@@ -70,8 +106,8 @@ static void *xmp_init(struct fuse_conn_info *conn, struct fuse_config *cfg) {
      To make parallel_direct_writes valid, need either set cfg->direct_io
      in current function (recommended in high level API) or set fi->direct_io
      in xmp_create() or xmp_open(). */
-  // cfg->direct_io = 1;
-  // cfg->parallel_direct_writes = 1;
+  cfg->direct_io = 1;
+  cfg->parallel_direct_writes = 1;
 
   /* Pick up changes from lower filesystem right away. This is
      also necessary for better hardlink support. When the kernel
@@ -83,18 +119,19 @@ static void *xmp_init(struct fuse_conn_info *conn, struct fuse_config *cfg) {
   cfg->entry_timeout = 0;
   cfg->attr_timeout = 0;
   cfg->negative_timeout = 0;
+  fprintf(stderr, "%d\n", getpid());
 
   return NULL;
 }
 
 static int xmp_getattr(const char *path, struct stat *stbuf,
-                       struct fuse_file_info *fi) {
+                       struct fuse_file_info *file_info) {
   int res;
 
   (void)path;
 
-  if (fi)
-    res = fstat(fi->fh, stbuf);
+  if (file_info)
+    res = fstat(file_info->fh, stbuf);
   else
     res = source_stat(path, stbuf);
   if (res == -1) {
@@ -106,38 +143,39 @@ static int xmp_getattr(const char *path, struct stat *stbuf,
 }
 
 static int xmp_access(const char *path, int mask) {
-  int res;
+  int res = -1;
 
   // if mask is F_OK, then we don't need to check the permissions
   // (is that possible?)
 
   if (mask != F_OK) {
-    struct process_info pi;
-    struct fuse_context *fc = fuse_get_context();
+    struct process_info proc_info;
+    struct fuse_context *context = fuse_get_context();
 
-    pi.PID = fc->pid;
-    pi.name = get_process_name_by_pid(pi.PID);
+    proc_info.PID = context->pid;
+    proc_info.name = get_process_name_by_pid(proc_info.PID);
 
     // fprintf(stderr, "%s, %d\n", path, ask_access(path, pi));
 
-    if (!interactive_access(real_filename(path), pi, 0)) {
-      free(pi.name);
+    if (!interactive_access(real_filename(path), proc_info, 0)) {
+      free((void *)proc_info.name);
       return -EACCES;
     }
 
-    free(pi.name);
+    free((void *)proc_info.name);
   }
 
   res = source_access(path, mask);
 
-  if (res == -1)
+  if (res == -1) {
     return -errno;
+  }
 
   return 0;
 }
 
 static int xmp_readlink(const char *path, char *buf, size_t size) {
-  int res;
+  int res = -1;
 
   res = readlink(path, buf, size - 1);
   if (res == -1)
@@ -264,17 +302,18 @@ static int xmp_mknod(const char *path, mode_t mode, dev_t rdev) {
 */
 
 static int xmp_mkdir(const char *path, mode_t mode) {
-  int res;
+  int res = -1;
 
   res = source_mkdir(path, mode);
-  if (res == -1)
+  if (res == -1) {
     return -errno;
+  }
 
   return 0;
 }
 
 static int xmp_unlink(const char *path) {
-  int res;
+  int res = -1;
   struct process_info pi;
   struct fuse_context *fc = fuse_get_context();
 

src/main.c

@@ -10,16 +10,15 @@
   See the file LICENSE.
 */
 
-#include <sys/types.h>
-#include <unistd.h>
 #define FUSE_USE_VERSION 31
 
 #define _GNU_SOURCE
 
 #include <fuse3/fuse.h>
-
 #include <stdio.h>
 #include <stdlib.h>
+#include <sys/types.h>
+#include <unistd.h>
 
 #include "fuse_operations.h"
 #include "sourcefs.h"
@@ -28,15 +27,25 @@
 const char *mountpoint = NULL;
 
 int main(int argc, char *argv[]) {
+  if (argc < 3) {
+    fprintf(stderr, "Usage: icfs <FUSE arguments> [target directory] [path to "
+                    "the permanent permissions database\n");
+    return EXIT_FAILURE;
+  }
+
+  // if umask != 0, the filesystem will create files with more restrictive
+  // permissions than it's caller reqested
   umask(0);
 
-  int ret = init_ui_socket();
+  // ui socket should always be initialized before anything else, since it
+  // handles the setuid bits!
+  int ret = init_ui_socket(argv[argc - 1]);
   if (ret != 0) {
     fprintf(stderr, "Could not initalize ui-socket.\n");
     exit(EXIT_FAILURE);
   }
 
-  mountpoint = realpath(argv[argc - 1], NULL);
+  mountpoint = realpath(argv[argc - 2], NULL);
 
   ret = source_init(mountpoint);
   if (ret != 0) {
@@ -44,9 +53,9 @@ int main(int argc, char *argv[]) {
     exit(EXIT_FAILURE);
   }
 
-  ret = fuse_main(argc, argv, get_fuse_operations(), NULL);
+  ret = fuse_main(argc - 1, argv, get_fuse_operations(), NULL);
 
-  free(mountpoint);
+  free((void *)mountpoint);
   destroy_ui_socket();
   return ret;
 }

src/perm_permissions_table.c

@@ -27,6 +27,7 @@ const int column_count = 2;
 const char *const schema[] = {"executable", "filename"};
 const char *const types[] = {"TEXT", "TEXT"};
 uid_t ruid, euid, current_pid;
+sqlite3_stmt *perm_check_statement = NULL;
 pthread_mutex_t uid_switch = PTHREAD_MUTEX_INITIALIZER;
 
 void set_db_fsuid() {
@@ -141,6 +142,45 @@ int ensure_database_schema() {
   return 0;
 }
 
+int prepare_sql_queries() {
+  const char *query_template =
+      "SELECT * FROM %s WHERE executable = ? AND filename = ?;";
+  char *query_string = NULL;
+  int query_len = snprintf(NULL, 0, query_template, table_name) + 1;
+
+  if (query_len < 0) {
+    fprintf(stderr, "Failed to prepare statement");
+    perror("");
+    return 1;
+  }
+
+  query_string = malloc(query_len);
+  if (query_string == NULL) {
+    fprintf(stderr, "Failed to allocate memory for the query");
+    perror("");
+    return 1;
+  }
+
+  int ret = snprintf(query_string, query_len, query_template, table_name);
+  if (ret < 0) {
+    fprintf(stderr, "Failed to prepare statement");
+    perror("");
+    free(query_string);
+    return 1;
+  }
+
+  if (sqlite3_prepare_v2(perm_database, query_string, -1, &perm_check_statement,
+                         NULL) != SQLITE_OK) {
+    fprintf(stderr, "Failed to prepare statement: %s\n",
+            sqlite3_errmsg(perm_database));
+    free(query_string);
+    return 1;
+  }
+  free(query_string);
+  return 0;
+}
+
+void free_sql_queries(void) { sqlite3_finalize(perm_check_statement); }
 /**
  * Initializes the permanent permissions table.
  *
@@ -169,17 +209,24 @@ int init_perm_permissions_table(const char *db_filename) {
 
   int status = seteuid(ruid);
   if (status < 0) {
-    fprintf(stderr, "Couldn't set euid to ruid during database setup.\n");
+    fprintf(stderr, "Couldn't set euid to ruid.\n");
     exit(status);
   }
 
+  if (prepare_sql_queries()) {
+    fprintf(stderr, "Couldn't prepare sql queries.\n");
+    exit(status);
+  }
   return 0;
 }
 
 /**
  * Destroys the permanent permissions table.
  */
-void destroy_perm_permissions_table() { sqlite3_close(perm_database); }
+void destroy_perm_permissions_table(void) {
+  free_sql_queries();
+  sqlite3_close(perm_database);
+}
 
 /**
  * Checks if the process has a permanent access to the file.

src/ui-socket.c

@@ -13,7 +13,6 @@
 #include "perm_permissions_table.h"
 #include "temp_permissions_table.h"
 #include "ui-socket.h"
-#include <errno.h>
 #include <pthread.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -22,17 +21,17 @@
 #include <sys/un.h>
 #include <unistd.h>
 
-int init_ui_socket() {
-  char line[256];
-  FILE *fp;
+#define ZENITY_TEMP_ALLOW_MESSAGE "Allow this time\n"
+
+int init_ui_socket(const char *perm_permissions_db_filename) {
+  FILE *fp = NULL;
 
   if (init_temp_permissions_table()) {
     fprintf(stderr, "Could not initialize temporary permissions table.\n");
     return 1;
   }
 
-  if (init_perm_permissions_table(
-          "/home/fedir/Developement/uni/ICFS/test/.pt.db")) {
+  if (init_perm_permissions_table(perm_permissions_db_filename)) {
     fprintf(stderr, "Could not initialize permanent permissions table.\n");
     return 1;
   }
@@ -44,13 +43,11 @@ int init_ui_socket() {
     return 1;
   }
 
-  while (fgets(line, sizeof(line), fp))
-    printf("%s", line);
   pclose(fp);
   return 0;
 }
 
-void destroy_ui_socket() {
+void destroy_ui_socket(void) {
   destroy_temp_permissions_table();
   destroy_perm_permissions_table();
 }
@@ -64,16 +61,16 @@ void destroy_ui_socket() {
  * @return: 0 if access is denied, 1 if access is allowed, 2 if access is
  * allowed for the runtime of the process
  */
-int ask_access(const char *filename, struct process_info pi) {
-  FILE *fp;
+int ask_access(const char *filename, struct process_info proc_info) {
+  FILE *fp = NULL;
   size_t command_len =
-      139 + sizeof(pid_t) * 8 + strlen(pi.name) + strlen(filename);
+      139 + sizeof(pid_t) * 8 + strlen(proc_info.name) + strlen(filename);
   char *command = (char *)malloc(command_len);
   snprintf(command, command_len,
            "zenity --question --extra-button \"Allow this time\" --title "
            "\"Allow Access?\" --text \"Allow process "
            "<tt>%s</tt> with PID <tt>%d</tt> to access <tt>%s</tt>\"",
-           pi.name, pi.PID, filename);
+           proc_info.name, proc_info.PID, filename);
 
   // Zenity Question Message Popup
   fp = popen(command, "r");
@@ -87,10 +84,10 @@ int ask_access(const char *filename, struct process_info pi) {
   // if the user clicks the "Allow this time" button, `zenity` will only
   // write it to `stdout`, but the exit code will still be `1`. So, we need
   // to manually check the output.
-  char buffer[1024];
+  char buffer[sizeof(ZENITY_TEMP_ALLOW_MESSAGE) + 1];
   while (fgets(buffer, sizeof(buffer), fp)) {
     printf("%s", buffer);
-    if (strcmp(buffer, "Allow this time\n") == 0) {
+    if (strcmp(buffer, ZENITY_TEMP_ALLOW_MESSAGE) == 0) {
       pclose(fp);
       return 2;
     }
@@ -117,9 +114,11 @@ int ask_access(const char *filename, struct process_info pi) {
  * @param opts: options (GRANT_TEMP, GRANT_PERM)
  * @return: 0 if access is denied, 1 if access is allowed
  */
-int interactive_access(const char *filename, struct process_info pi, int opts) {
+int interactive_access(const char *filename, struct process_info proc_info,
+                       int opts) {
 
-  if (check_temp_access(filename, pi) || check_perm_access(filename, pi)) {
+  if (check_temp_access(filename, proc_info) ||
+      check_perm_access(filename, proc_info)) {
     // access was already granted before
     return 1;
   }
@@ -128,22 +127,24 @@ int interactive_access(const char *filename, struct process_info pi, int opts) {
   // permissions are granted
 
   if (opts & GRANT_PERM) {
-    give_perm_access(filename, pi);
+    give_perm_access(filename, proc_info);
     return 1;
   }
   if (opts & GRANT_TEMP) {
-    give_temp_access(filename, pi);
+    give_temp_access(filename, proc_info);
     return 1;
   }
 
-  int user_response = ask_access(filename, pi);
+  int user_response = ask_access(filename, proc_info);
   if (user_response == 1) {
     // user said "yes"
-    give_perm_access(filename, pi);
+    give_perm_access(filename, proc_info);
     return 1;
-  } else if (user_response == 2) {
+  }
+
+  if (user_response == 2) {
     // user said "yes, but only this time"
-    give_temp_access(filename, pi);
+    give_temp_access(filename, proc_info);
     return 1;
   }
 

src/ui-socket.h

@@ -21,7 +21,7 @@
  *
  * @return: 0 on success, -1 on faliure.
  */
-int init_ui_socket(void);
+int init_ui_socket(const char *perm_permissions_db_filename);
 
 /**
  * Close the GUI communication.

test/mock/zenity

@@ -12,10 +12,17 @@ else
     if [[ $FAKE_ZENITY_RESPONSE == "yes_tmp" ]]; then
       printf "Allow this time\n"
       exit 1
+    elif [[ $FAKE_ZENITY_RESPONSE == "yes_tmp_alt" ]]; then
+      printf "Allow this time\n"
+      echo "yes_alt" >~/.fake_zenity_response
+      exit 1
     elif [[ $FAKE_ZENITY_RESPONSE == "no" ]]; then
       exit 1
     elif [[ $FAKE_ZENITY_RESPONSE == "yes" ]]; then
       exit 0
+    elif [[ $FAKE_ZENITY_RESPONSE == "yes_alt" ]]; then
+      echo "yes_tmp_alt" >~/.fake_zenity_response
+      exit 0
     fi
   fi
 fi

test/test.bash

@@ -23,12 +23,20 @@ if [[ $1 == "--setuid" ]]; then
   sudo chown icfs: ../build/icfs && sudo chmod 4777 ../build/icfs
   chmod g+w . # needed for icfs to be able to create the database
   echo "Valgrind will not be used due to setuid compatibility issues."
-  ../build/icfs -o default_permissions ./protected &
+  ../build/icfs -o default_permissions ./protected ./.pt.db &
   sleep 1
+elif [[ $1 == "--perf" ]]; then
+  echo "Profiling with perf..."
+  ../build/icfs -o default_permissions ./protected ./.pt.db &
+  echo "Profiling will require root privilieges."
+  sleep 3
+  echo "Attaching to $(pgrep icfs)"
+  sudo perf record -g -e cycles:u --call-graph dwarf -p $(pgrep icfs) &
+  sleep 10
 else
   echo "Database protection will not be tested due to the lack of setuid capabilites."
   echo "To test it, run this script with '--setuid'."
-  valgrind -s ../build/icfs -o default_permissions ./protected &
+  valgrind --leak-check=full -s ../build/icfs -o default_permissions ./protected ./.pt.db &
   sleep 5
 fi
 
@@ -126,9 +134,29 @@ else
   echo "[ICFS-TEST]: OK"
 fi
 
+if [[ $1 == "--perf" ]]; then
+  zenity --set-fake-response yes_tmp
+  rm -rf ./protected/*
+  zenity --set-fake-response yes_alt
+  bonnie++ -p 4
+  bonnie++ -d ./protected -c 4 -r 256 -y s >/dev/null &
+  bonnie++ -d ./protected -c 4 -r 256 -y s >/dev/null &
+  bonnie++ -d ./protected -c 4 -r 256 -y s >/dev/null &
+  bonnie++ -d ./protected -c 4 -r 256 -y s >/dev/null
+  bonnie++ -p -1
+fi
+
 # unmount
 
 sleep 0.5
 #lsof +f -- $(realpath ./protected)
 umount $(realpath ./protected)
 sleep 0.5
+
+if [[ $1 == "--perf" ]]; then
+  mv ./callgraph.png ./callgraph_old.png
+  real_user=$USER
+  sudo chown "$real_user" ./perf.data
+  perf script --dsos=icfs | gprof2dot -f perf | dot -Tpng -o callgraph.png
+  echo "Profile graph was written to \"callgraph.png\""
+fi