new parameter: "secure include"

[CAE Samba Admin]

Hello,

I know that samba-ntdom probably isn't the best place for this message;
however, I am not subscribed to samba-technical (nor do I want to be at
the moment).  I'm CC'ing this message over to samba-technical, so anyone
reading over there, please mail me directly as well as mailing the list.

Okay with that formaility out of the way, let me describe what I've done
and why I did it.

I'm running in a samba domain (PDC, yes I know, please don't tell me
not to run the 2.0.x branch) where we are trying to get a setup where
multiple departments will all use the PDC for authentication, but would
like some control over the configuration.  

Using a %m substitution with an "include" in the smb.conf file would be a
nice way to do this.  We could just have directories coresponding to the
machine accounts and then have an additional conf file in there with
departmental override options.

Unfortunately, this is a huge security risk.  Just think about the
possibilities of someone stuck a root preexec in their conf file.

Here's my solution.  All department config files must be "root approved".
IOW, only root can actually change the files.  Departments submit changes
they want made to the sysadmin for review.  Now, instead of a config file
in the machine directories, we symlink to something like
"/usr/local/samba/lib/smb_global-dept.conf" and through smb.conf like
below:

[global]

   secure include = /home/machines/%m/globals.conf


Of course /home/machines/%m/globals.conf is a symlink to
/usr/local/samba/lib/smb_global-dept.conf.  The actual config file is
owned by root and only writable by root.

I basically copied the "handle_include" function in param/loadparm.c and
made a "handle_secure_include" function which refuses to include the file
of any of the following three conditions (in this order) aren't met:

	1. 	root must own the file
	2.	the file must not be group writable
	3.	the file must not be world writable

I've attached a diff against samba-2.0.4b below.  I would really like to
see this incorporated into future releases.

I have some concerns about my coding ability though.  Can someone check
and make sure there are no gaping holes in my code or any loss of
portability?  I am especially concerned about systems where root uid != 0
(do any exist?).  What would be a better way of checking than I am doing
now?

Thanks,

Kevin Currie
CAE Center
Western Michigan University
-------------- next part --------------
diff -uNr samba-2.0.4b/source/param/loadparm.c samba-2.0.4b-kgc/source/param/loadparm.c
--- samba-2.0.4b/source/param/loadparm.c	Mon May 17 19:37:24 1999
+++ samba-2.0.4b-kgc/source/param/loadparm.c	Thu Jun 10 13:51:52 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,61 @@
 }
 
 
+/***************************************************************************
+handle the secure include operation
+***************************************************************************/
+static BOOL handle_secure_include(char *pszParmValue,char **ptr)
+{ 
+  SMB_STRUCT_STAT istat;
+
+  pstring fname;
+  pstrcpy(fname,pszParmValue);
+
+  add_to_file_list(fname);
+
+  standard_sub_basic(fname);
+
+  string_set(ptr,fname);
+   
+  if (file_exist(fname,NULL)) {
+    
+    if (sys_stat(fname, &istat)) {
+	DEBUG(3,("ERROR: sys_stat failed on include file %s\n",fname));
+	return(False);
+    } 
+    else {
+        
+	// Ensure the file's uid == root
+
+	if (istat.st_uid) {
+           DEBUG(2,("ERROR: secure include file %s uid not root\n",fname));
+           return(False);
+	}
+	
+	// Make sure the file isn't group writable
+
+	if (istat.st_mode & S_IWGRP) {
+           DEBUG(2,("ERROR: secure include file %s has group write bit set\n",fname)); 
+           return(False);
+        }
+
+	// Make sure the file isn't world writable
+
+	if (istat.st_mode & S_IWOTH) {
+           DEBUG(2,("ERROR: secure include file %s has world write bit set\n",fname)); 
+           return(False);
+        }
+
+        return(pm_process(fname, do_section, do_parameter));     
+    }
+  }
+
+  DEBUG(2,("Can't find include file %s\n",fname));
+
+  return(False);
+}
+
+ 
 /***************************************************************************
 handle the interpretation of the copy parameter
 ***************************************************************************/