2009-08-06 Dave Brolley <brolley@redhat.com>

        * modverify.c (staprun.h): #include it.
        (verify_it): Now accepts module data and signature data as arguments.
        Don't open and read the signature here. Don't read the module here.
        (verify_module): Now accepts module data as argument. Read the signature

        once here.
        * modverify.h (verify_module): Now accepts module data as argument.
        * staprun.c (main): Don't call check_permissions here.
        * staprun.h (check_permissions): Prototype removed.
        * staprun_funcs.c (check_permissions): Now static. Accepts module data
        as argument. Pass module data to check_signature.
        (insert_module): Canonicalize the module path early here. Call
        check_permissions here, passing it the mapped module data.
        (check_signature): Now accepts module data as argument. Pass the module
        data to verify_module.
        (check_path): Use the already-canonicalized module path.

5 files changed, 118 insertions, 143 deletions

diff --git a/runtime/staprun/modverify.c b/runtime/staprun/modverify.c
index f4b15ac3..15447279 100644
--- a/runtime/staprun/modverify.c
+++ b/runtime/staprun/modverify.c
@@ -29,6 +29,7 @@
 #include <certt.h>
 
 #include "nsscommon.h"
+#include "staprun.h"
 #include "modverify.h"
 
 #include <sys/stat.h>
@@ -130,7 +131,11 @@ check_cert_db_permissions (const char *cert_db_path) {
       return 0;
     }
 
-  rc = 1; /* ok */
+  if (! S_ISDIR (info.st_mode))
+    {
+      fprintf (stderr, "Certificate database %s is not a directory.\n", cert_db_path);
+      return 0;
+    }
 
   /* The owner of the database must be root.  */
   if (info.st_uid != 0)
@@ -139,6 +144,8 @@ check_cert_db_permissions (const char *cert_db_path) {
       rc = 0;
     }
 
+  rc = 1; /* ok */
+
   /* Check the database directory access permissions  */
   if ((info.st_mode & S_IRUSR) == 0)
     fprintf (stderr, "Certificate database %s should be readable by the owner.\n", cert_db_path);
@@ -189,16 +196,75 @@ check_cert_db_permissions (const char *cert_db_path) {
 }
 
 static int
-verify_it (const char *inputName, const char *signatureName, SECKEYPublicKey *pubKey)
+verify_it (const char *signatureName, const SECItem *signature,
+	   const void *module_data, off_t module_size,
+	   const SECKEYPublicKey *pubKey)
 {
-  unsigned char buffer[4096];
-  PRFileInfo info;
+  VFYContext *vfy;
+  SECStatus secStatus;
+
+  /* Create a verification context.  */
+  vfy = VFY_CreateContextDirect (pubKey, signature, SEC_OID_PKCS1_RSA_ENCRYPTION,
+				 SEC_OID_UNKNOWN, NULL, NULL);
+  if (! vfy)
+    {
+      /* The key does not match the signature. This is not an error. It just
+	 means we are currently trying the wrong certificate/key. i.e. the
+	 module remains untrusted for now.  */
+      return MODULE_UNTRUSTED;
+    }
+
+  /* Begin the verification process.  */
+  secStatus = VFY_Begin(vfy);
+  if (secStatus != SECSuccess)
+    {
+      fprintf (stderr, "Unable to initialize verification context while verifying %s using the signature in %s.\n",
+	       modpath, signatureName);
+      nssError ();
+      return MODULE_CHECK_ERROR;
+    }
+
+  /* Add the data to be verified.  */
+  secStatus = VFY_Update (vfy, module_data, module_size);
+  if (secStatus != SECSuccess)
+    {
+      fprintf (stderr, "Error while verifying %s using the signature in %s.\n",
+	       modpath, signatureName);
+      nssError ();
+      return MODULE_CHECK_ERROR;
+    }
+
+  /* Complete the verification.  */
+  secStatus = VFY_End (vfy);
+  if (secStatus != SECSuccess) {
+    fprintf (stderr, "Unable to verify the signed module %s. It may have been altered since it was created.\n",
+	     modpath);
+    nssError ();
+    return MODULE_ALTERED;
+  }
+
+  return MODULE_OK;
+}
+
+int verify_module (const char *signatureName, const void *module_data,
+		   off_t module_size)
+{
+  const char *dbdir  = SYSCONFDIR "/systemtap/staprun";
+  SECKEYPublicKey *pubKey;
+  SECStatus secStatus;
+  CERTCertList *certList;
+  CERTCertListNode *certListNode;
+  CERTCertificate *cert;
   PRStatus prStatus;
+  PRFileInfo info;
   PRInt32  numBytes;
   PRFileDesc *local_file_fd;
-  VFYContext *vfy;
   SECItem signature;
-  SECStatus secStatus;
+  int rc = 0;
+
+  /* Verify the permissions of the certificate database and its files.  */
+  if (! check_cert_db_permissions (dbdir))
+    return MODULE_UNTRUSTED;
 
   /* Get the size of the signature file.  */
   prStatus = PR_GetFileInfo (signatureName, &info);
@@ -246,94 +312,6 @@ verify_it (const char *inputName, const char *signatureName, SECKEYPublicKey *pu
   /* Done with the signature file.  */
   PR_Close (local_file_fd);
 
-  /* Create a verification context.  */
-  vfy = VFY_CreateContextDirect (pubKey, & signature, SEC_OID_PKCS1_RSA_ENCRYPTION,
-				 SEC_OID_UNKNOWN, NULL, NULL);
-  if (! vfy)
-    {
-      /* The key does not match the signature. This is not an error. It just means
-	 we are currently trying the wrong certificate/key. i.e. the module
-	 remains untrusted for now.  */
-      return MODULE_UNTRUSTED;
-    }
-
-  /* Begin the verification process.  */
-  secStatus = VFY_Begin(vfy);
-  if (secStatus != SECSuccess)
-    {
-      fprintf (stderr, "Unable to initialize verification context while verifying %s using the signature in %s.\n",
-	       inputName, signatureName);
-      nssError ();
-      return MODULE_CHECK_ERROR;
-    }
-
-  /* Now read the data and add it to the signature.  */
-  local_file_fd = PR_Open (inputName, PR_RDONLY, 0);
-  if (local_file_fd == NULL)
-    {
-      fprintf (stderr, "Could not open module file %s.\n", inputName);
-      nssError ();
-      return MODULE_CHECK_ERROR;
-    }
-
-  for (;;)
-    {
-      numBytes = PR_Read (local_file_fd, buffer, sizeof (buffer));
-      if (numBytes == 0)
-	break;	/* EOF */
-
-      if (numBytes < 0)
-	{
-	  fprintf (stderr, "Error reading module file %s.\n", inputName);
-	  nssError ();
-	  return MODULE_CHECK_ERROR;
-	}
-
-      /* Add the data to the signature.  */
-      secStatus = VFY_Update (vfy, buffer, numBytes);
-      if (secStatus != SECSuccess)
-	{
-	  fprintf (stderr, "Error while verifying module file %s.\n", inputName);
-	  nssError ();
-	  return MODULE_CHECK_ERROR;
-	}
-    }
-
-  PR_Close(local_file_fd);
-
-  /* Complete the verification.  */
-  secStatus = VFY_End (vfy);
-  if (secStatus != SECSuccess) {
-    fprintf (stderr, "Unable to verify signed module %s. It may have been altered since it was created.\n", inputName);
-    nssError ();
-    return MODULE_ALTERED;
-  }
-
-  return MODULE_OK;
-}
-
-int verify_module (const char *module_name, const char *signature_name)
-{
-  const char *dbdir  = SYSCONFDIR "/systemtap/staprun";
-  SECKEYPublicKey *pubKey;
-  SECStatus secStatus;
-  CERTCertList *certList;
-  CERTCertListNode *certListNode;
-  CERTCertificate *cert;
-  PRStatus prStatus;
-  PRFileInfo info;
-  int rc = 0;
-
-  /* Look for the certificate database. If it's not there, it's not an error, it
-     just means that the module can't be verified.  */
-  prStatus = PR_GetFileInfo (dbdir, &info);
-  if (prStatus != PR_SUCCESS || info.type != PR_FILE_DIRECTORY)
-    return MODULE_UNTRUSTED;
-
-  /* Verify the permissions of the certificate database and its files.  */
-  if (! check_cert_db_permissions (dbdir))
-    return MODULE_UNTRUSTED;
-
   /* Call the NSPR initialization routines. */
   PR_Init (PR_SYSTEM_THREAD, PR_PRIORITY_NORMAL, 1);
 
@@ -373,7 +351,7 @@ int verify_module (const char *module_name, const char *signature_name)
 	}
 
       /* Verify the file. */
-      rc = verify_it (module_name, signature_name, pubKey);
+      rc = verify_it (signatureName, & signature, module_data, module_size, pubKey);
       if (rc == MODULE_OK || rc == MODULE_ALTERED || rc == MODULE_CHECK_ERROR)
 	break; /* resolved or error */
     }
diff --git a/runtime/staprun/modverify.h b/runtime/staprun/modverify.h
index 49b90bfe..730a5e86 100644
--- a/runtime/staprun/modverify.h
+++ b/runtime/staprun/modverify.h
@@ -1,4 +1,4 @@
-int verify_module (const char *module_name, const char *signature_name);
+int verify_module (const char *signature_name, const void *module_data, off_t module_size);
 
 /* return codes for verify_module.  */
 #define MODULE_OK           1
diff --git a/runtime/staprun/staprun.c b/runtime/staprun/staprun.c
index 554eecc8..c64bf5b3 100644
--- a/runtime/staprun/staprun.c
+++ b/runtime/staprun/staprun.c
@@ -303,9 +303,6 @@ int main(int argc, char **argv)
 		exit(1);
 	}
 
-	if (check_permissions() != 1)
-		usage(argv[0]);
-
 	if (init_staprun())
 		exit(1);
 
diff --git a/runtime/staprun/staprun.h b/runtime/staprun/staprun.h
index 0a1ca885..1dcfabbe 100644
--- a/runtime/staprun/staprun.h
+++ b/runtime/staprun/staprun.h
@@ -134,7 +134,6 @@ const char *moderror(int err);
 int insert_module(const char *path, const char *special_options,
 	char **options);
 int mountfs(void);
-int check_permissions(void);
 void start_symbol_thread(void);
 void stop_symbol_thread(void);
 
diff --git a/runtime/staprun/staprun_funcs.c b/runtime/staprun/staprun_funcs.c
index 1bbb59f5..4e525b00 100644
--- a/runtime/staprun/staprun_funcs.c
+++ b/runtime/staprun/staprun_funcs.c
@@ -23,6 +23,7 @@
 #include <assert.h>
 
 extern long init_module(void *, unsigned long, const char *);
+static int check_permissions(const void *, off_t);
 
 /* Module errors get translated. */
 const char *moderror(int err)
@@ -48,6 +49,7 @@ int insert_module(const char *path, const char *special_options, char **options)
 	void *file;
 	char *opts;
 	int fd, saved_errno;
+	char module_realpath[PATH_MAX];
 	struct stat sbuf;
 
 	dbug(2, "inserting module\n");
@@ -71,8 +73,24 @@ int insert_module(const char *path, const char *special_options, char **options)
 	}
 	dbug(2, "module options: %s\n", opts);
 
-	/* Open the module file. */
-	fd = open(path, O_RDONLY);
+	/* Use realpath() to canonicalize the module path. */
+	if (realpath(modpath, module_realpath) == NULL) {
+		perr("Unable to canonicalize path \"%s\"", modpath);
+		return -1;
+	}
+
+        /* Overwrite the modpath with the canonicalized one, to defeat
+           a possible race between path and signature checking below and,
+	   somewhat later, module loading. */
+        modpath = strdup (module_realpath);
+        if (modpath == NULL) {
+		_perr("allocating memory failed");
+                exit (1);
+        }
+
+	/* Open the module file. Work with the open file descriptor from this
+	   point on to avoid TOCTOU problems. */
+	fd = open(modpath, O_RDONLY);
 	if (fd < 0) {
 		perr("Error opening '%s'", path);
 		return -1;
@@ -94,6 +112,11 @@ int insert_module(const char *path, const char *special_options, char **options)
 		return -1;
 	}
 
+	/* Check whether this module can be loaded by the current user.  */
+	ret = check_permissions (file, sbuf.st_size);
+	if (ret != 1)
+		return -1;
+
 	STAP_PROBE1(staprun, insert__module, path);
 	/* Actually insert the module */
 	ret = init_module(file, sbuf.st_size, opts);
@@ -216,28 +239,22 @@ int mountfs(void)
  * Returns: -1 on errors, 0 on failure, 1 on success.
  */
 static int
-check_signature(void)
+check_signature(const void *module_data, off_t module_size)
 {
-  char module_realpath[PATH_MAX];
   char signature_realpath[PATH_MAX];
   int rc;
 
   dbug(2, "checking signature for %s\n", modpath);
 
-  /* Use realpath() to canonicalize the module path. */
-  if (realpath(modpath, module_realpath) == NULL) {
-    perr("Unable to canonicalize module path \"%s\"", modpath);
-    return MODULE_CHECK_ERROR;
-  }
-
-  /* Now add the .sgn suffix to get the signature file name.  */
-  if (strlen (module_realpath) > PATH_MAX - 4) {
-    err("Path \"%s\" is too long.", modpath);
-    return MODULE_CHECK_ERROR;
+  /* Add the .sgn suffix to the canonicalized module path to get the signature
+     file path.  */
+  if (strlen (modpath) >= PATH_MAX - 4) {
+    err("Path \"%s.sgn\" is too long.", modpath);
+    return -1;
   }
-  sprintf (signature_realpath, "%s.sgn", module_realpath);
+  sprintf (signature_realpath, "%s.sgn", modpath);
 
-  rc = verify_module (module_realpath, signature_realpath);
+  rc = verify_module (signature_realpath, module_data, module_size);
 
   dbug(2, "verify_module returns %d\n", rc);
 
@@ -255,11 +272,10 @@ check_signature(void)
 static int
 check_path(void)
 {
-	struct utsname utsbuf;
-	struct stat sb;
 	char staplib_dir_path[PATH_MAX];
 	char staplib_dir_realpath[PATH_MAX];
-	char module_realpath[PATH_MAX];
+	struct utsname utsbuf;
+	struct stat sb;
 
 	/* First, we need to figure out what the kernel
 	 * version is and build the '/lib/modules/KVER/systemtap' path. */
@@ -308,21 +324,6 @@ check_path(void)
 		return -1;
 	}
 
-	/* Use realpath() to canonicalize the module path. */
-	if (realpath(modpath, module_realpath) == NULL) {
-		perr("Unable to canonicalize path \"%s\"", modpath);
-		return -1;
-	}
-
-        /* Overwrite the modpath with the canonicalized one, to defeat
-           a possible race between path checking below and somewhat later
-           module loading. */
-        modpath = strdup (module_realpath);
-        if (modpath == NULL) {
-		_perr("allocating memory failed");
-                exit (1);
-        }
-
 	/* To make sure the user can't specify something like
 	 * /lib/modules/`uname -r`/systemtapmod.ko, put a '/' on the
 	 * end of staplib_dir_realpath. */
@@ -334,8 +335,8 @@ check_path(void)
 	}
 
 	/* Now we've got two canonicalized paths.  Make sure
-	 * module_realpath starts with staplib_dir_realpath. */
-	if (strncmp(staplib_dir_realpath, module_realpath,
+	 * modpath starts with staplib_dir_realpath. */
+	if (strncmp(staplib_dir_realpath, modpath,
 		    strlen(staplib_dir_realpath)) != 0) {
 		err("ERROR: Members of the \"stapusr\" group can only use modules within\n"
 		    "  the \"%s\" directory.\n"
@@ -347,7 +348,7 @@ check_path(void)
 }
 
 /*
- * Check the user's group membership.  Is he allowed to run staprun (or is
+ * Check the user's group membership.
  *
  * o members of stapdev can do anything
  * o members of stapusr can load modules from /lib/modules/KVER/systemtap
@@ -433,8 +434,8 @@ check_groups (void)
 }
 
 /*
- * Check the user's permissions.  Is he allowed to run staprun (or is
- * he limited to "blessed" modules)?
+ * Check the user's permissions.  Is he allowed to run staprun, or is
+ * he limited to "blessed" modules?
  *
  * There are several levels of possible permission:
  *
@@ -447,15 +448,15 @@ check_groups (void)
  *
  * Returns: -1 on errors, 0 on failure, 1 on success.
  */
-int check_permissions(void)
+int check_permissions(const void *module_data, off_t module_size)
 {
 	int check_groups_rc;
 	int check_signature_rc = 0;
-#if HAVE_NSS
 
+#if HAVE_NSS
 	/* Attempt to verify the module against its signature. Return failure
 	   if the module has been tampered with (altered).  */
-	check_signature_rc = check_signature ();
+	check_signature_rc = check_signature (module_data, module_size);
 	if (check_signature_rc == MODULE_ALTERED)
 		return 0;
 #endif
@@ -483,7 +484,7 @@ int check_permissions(void)
 		return 1;
 
 	/* The user is an ordinary user. If the module has been signed with
-	 * a "blessed" certificate and private key, then we will load it for
+	 * an authorized certificate and private key, then we will load it for
 	 * anyone.  */
 #if HAVE_NSS
 	if (check_signature_rc == MODULE_OK)