update: secure include

[CAE Samba Admin]

Hello,

I've added quite a bit more security to "secure include" as per
suggestions of a few people here (thanks).

This still isn't dynamic, sorry...  But it does function as a nice
absolute security (ie, root and only root).

I've implemented verification of inodes, and I now perform the uid and
write permission checks across the whole path up to the include file.

I've included a diff against 2.0.4b below.

I am still interested in seeing something like this integrated into the
source trees, I'm also interested in cleaning this up for my own purposes.
Any suggestions anyone could offer are welcome.

I have a couple concerns as of now:

	~ readlink() is called, but i don't think this function is POSIX.
		does anyone know of a POSIX way to do this?

	~ root is assumed to be uid==0.
		is this true for all operating systems?

	~ '/' is used in hacking apart the path
		is there a way to determine the directory seperator
		character for the OS?


Again, any help making this conform to samba source standards will be
appreciated.

Thanks,

Kevin Currie



-------------- next part --------------
diff -uNr samba-2.0.4b/source/param/loadparm.c samba-2.0.4b-diff/source/param/loadparm.c
--- samba-2.0.4b/source/param/loadparm.c	Mon May 17 19:37:24 1999
+++ samba-2.0.4b-diff/source/param/loadparm.c	Fri Jun 11 15:50:50 1999
@@ -261,6 +261,7 @@
   char *szAdminUsers;
   char *szCopy;
   char *szInclude;
+  char *szSecureInclude;
   char *szPreExec;
   char *szPostExec;
   char *szRootPreExec;
@@ -356,6 +357,7 @@
   NULL,    /* szAdminUsers */
   NULL,    /* szCopy */
   NULL,    /* szInclude */
+  NULL,    /* szSecureInclude */
   NULL,    /* szPreExec */
   NULL,    /* szPostExec */
   NULL,    /* szRootPreExec */
@@ -452,6 +454,7 @@
 /* prototypes for the special type handlers */
 static BOOL handle_valid_chars(char *pszParmValue, char **ptr);
 static BOOL handle_include(char *pszParmValue, char **ptr);
+static BOOL handle_secure_include(char *pszParmValue, char **ptr);
 static BOOL handle_copy(char *pszParmValue, char **ptr);
 static BOOL handle_character_set(char *pszParmValue,char **ptr);
 static BOOL handle_coding_system(char *pszParmValue,char **ptr);
@@ -784,6 +787,7 @@
   {"-valid",           P_BOOL,    P_LOCAL,  &sDefault.valid,            NULL,   NULL,  FLAG_HIDE},
   {"copy",             P_STRING,  P_LOCAL,  &sDefault.szCopy,           handle_copy, NULL,  FLAG_HIDE},
   {"include",          P_STRING,  P_LOCAL,  &sDefault.szInclude,        handle_include, NULL,  FLAG_HIDE},
+  {"secure include",   P_STRING,  P_LOCAL,  &sDefault.szSecureInclude,  handle_secure_include, NULL, FLAG_HIDE},
   {"exec",             P_STRING,  P_LOCAL,  &sDefault.szPreExec,        NULL,   NULL,  FLAG_SHARE|FLAG_PRINT},
   {"preexec",          P_STRING,  P_LOCAL,  &sDefault.szPreExec,        NULL,   NULL,  0},
   {"postexec",         P_STRING,  P_LOCAL,  &sDefault.szPostExec,       NULL,   NULL,  FLAG_SHARE|FLAG_PRINT},
@@ -1889,6 +1893,131 @@
 }
 
 
+/***************************************************************************
+handle the secure include operation
+***************************************************************************/
+static BOOL handle_secure_include(char *pszParmValue,char **ptr)
+{ 
+  SMB_STRUCT_STAT init_stat, link_stat, test_stat;
+  int filedes;
+
+  pstring fname, path, front;
+  pstrcpy(fname,pszParmValue);
+
+  add_to_file_list(fname);
+
+  standard_sub_basic(fname);
+
+  string_set(ptr,fname);
+   
+  if (!(file_exist(fname,NULL))) {
+      DEBUG(2,("Can't find include file %s\n",fname));
+      return(False);
+  }
+
+  // preserve fname since split_to_last_component is destructive
+
+  pstrcpy(front, fname);
+
+  // check security of full path of secure include file
+
+  do {
+
+    // Get file stat info on filename, IOW: get the inode ASAP
+
+    if (sys_stat(front, &init_stat)) {
+	DEBUG(2,("ERROR: sys_stat failed on secure include file/path %s\n",front));
+	return(False);
+    } 
+
+    // Get link stat info on filename
+        
+    if (sys_lstat(front, &init_stat)) {
+	DEBUG(2,("ERROR: sys_lstat failed on secure include file/path %s\n",front));
+	return(False);
+    } 
+
+    // If filename is a symlink, get the name of the file it points to
+
+    if (S_ISLNK(link_stat.st_mode)) {
+        if (readlink(front, front, sizeof(pstring)) < 0) {
+            DEBUG(2,("ERROR: readlink failed on secure include file/path %s\n",front));
+            return(False);
+        }
+    }
+
+    // Make user the link isn't broken
+    // We don't use file_exist() here because we also process directories
+
+    if (sys_stat(front, &link_stat)) {
+       DEBUG(2,("ERROR: secure include file/path %s is a broken link\n",front));
+       return(False);
+    }
+
+    // Open the file while doing security checks so that softlinks
+    // cannot be redirected on a bogged down system before the
+    // checks can occur
+
+    filedes = sys_open(front, O_RDONLY, S_IRWXU);
+    if (filedes < 0) {
+        DEBUG(2,("ERROR: sys_open failed on secure include file/path %s\n",front));
+        return(False);
+    }
+
+    // Get stat info on the open file
+
+    if (sys_fstat(filedes, &test_stat)) {
+	DEBUG(2,("ERROR: sys_fstat failed on secure include file/path %s\n",front));
+	return(False);
+    } 
+
+    // Now that we have the info, close the file
+
+    if (close(filedes)) {
+       DEBUG(2,("ERROR: close failed on secure include file/path %s\n",front));
+       return(False);
+    }
+
+    // Ensure we're at the same inode we started with
+
+    if (init_stat.st_ino != test_stat.st_ino) {
+        DEBUG(2,("ERROR: secure include file/path %s inode changed during security checks\n",front));
+        return(False);
+    }
+
+    // Ensure the file's uid == root
+
+    if (test_stat.st_uid) {
+        DEBUG(2,("ERROR: secure include file/path %s uid not root\n",front));
+        return(False);
+    }
+	
+    // Make sure the file isn't group writable
+
+    if (test_stat.st_mode & S_IWGRP) {
+       DEBUG(2,("ERROR: secure include file/path %s has group write bit set\n",front)); 
+       return(False);
+    }
+
+    // Make sure the file isn't world writable
+
+    if (test_stat.st_mode & S_IWOTH) {
+       DEBUG(2,("ERROR: secure include file/path %s has world write bit set\n",front)); 
+       return(False);
+    }
+
+    pstrcpy(path, front);
+    split_at_last_component(path, front, '/', NULL);
+  
+  } while (*front != '\0');
+
+  // Okay, we made it.  Go ahead and process the file
+
+  return(pm_process(fname, do_section, do_parameter));     
+
+}
+
+ 
 /***************************************************************************
 handle the interpretation of the copy parameter
 ***************************************************************************/