2009-04-30 Dave Brolley <brolley@redhat.com>

        * modsign.cxx (unistd.h,sts/stat.h,systypes.h,pwd.h): #include them.
        (check_cert_file_permissions, check_db_file_permissions)
        (check_cert_db_permissions): New functions.
        (check_cert_db_path): Don't check for keyFiles.
        * main.cxx (usage): Remove --signing-cert option.
        (main): Likewise.
        * cache.cxx (cassert): #include it.
        (add_to_cache): Assume the module is signed.
        * buildrun.cxx (compile_pass): Always sign the module.
        * stap-server.8.in: Update documentation.

5 files changed, 275 insertions, 94 deletions

diff --git a/buildrun.cxx b/buildrun.cxx
index 30a602b3..31f7ec00 100644
--- a/buildrun.cxx
+++ b/buildrun.cxx
@@ -225,8 +225,8 @@ compile_pass (systemtap_session& s)
   // If a certificate database was specified, then try to sign the module.
   // Failure to do so is not a fatal error. If the signature is actually needed,
   // staprun will complain at that time.
-  if (!s.cert_db_path.empty())
-    sign_module (s);
+  assert (! s.cert_db_path.empty());
+  sign_module (s);
 #endif
 
   return rc;
diff --git a/cache.cxx b/cache.cxx
index 766600a8..bfe2b527 100644
--- a/cache.cxx
+++ b/cache.cxx
@@ -14,6 +14,7 @@
 #include <string>
 #include <fstream>
 #include <cstring>
+#include <cassert>
 
 extern "C" {
 #include <sys/types.h>
@@ -73,33 +74,21 @@ add_to_cache(systemtap_session& s)
   string module_signature_dest_path = s.hash_path;
   module_signature_dest_path += ".sgn";
 
-  if (!s.cert_db_path.empty())
-    {
-      // Copy the module signature, if it was signed.
-      string module_signature_src_path = module_src_path;
-      module_signature_src_path += ".sgn";
+  // Copy the module signature.
+  assert (! s.cert_db_path.empty());
+  string module_signature_src_path = module_src_path;
+  module_signature_src_path += ".sgn";
 
-      if (s.verbose > 1)
-	clog << "Copying " << module_signature_src_path << " to " << module_signature_dest_path << endl;
-      if (copy_file(module_signature_src_path.c_str(), module_signature_dest_path.c_str()) != 0)
-	{
-	  cerr << "Copy failed (\"" << module_signature_src_path << "\" to \""
-	       << module_signature_dest_path << "\"): " << strerror(errno) << endl;
-	  // NB: this is not so severe as to prevent reuse of the .ko
-	  // already copied.
-	  //
-	  // s.use_cache = false;
-	}
-    }
-  else
+  if (s.verbose > 1)
+    clog << "Copying " << module_signature_src_path << " to " << module_signature_dest_path << endl;
+  if (copy_file(module_signature_src_path.c_str(), module_signature_dest_path.c_str()) != 0)
     {
-      // If this module was not signed, then delete any existing signature from the cache.
-      // This is not a fatal error. Even if the existing signature happens to match a
-      // new module later, it still means that the module is identical to one generated by a
-      // trusted server.
-      if (remove_file_or_dir (module_signature_dest_path.c_str()) != 0)
-	cerr << "Failed to remove \"" << module_signature_dest_path << "\" from the cache: "
-	     << strerror(errno) << endl;
+      cerr << "Copy failed (\"" << module_signature_src_path << "\" to \""
+	   << module_signature_dest_path << "\"): " << strerror(errno) << endl;
+      // NB: this is not so severe as to prevent reuse of the .ko
+      // already copied.
+      //
+      // s.use_cache = false;
     }
 #endif /* HAVE_NSS */
 
diff --git a/main.cxx b/main.cxx
index aecb2341..84bbce73 100644
--- a/main.cxx
+++ b/main.cxx
@@ -139,8 +139,6 @@ usage (systemtap_session& s, int exitcode)
 #endif
   // Formerly present --ignore-{vmlinux,dwarf} options are for testsuite use
   // only, and don't belong in the eyesight of a plain user.
-    << "   --signing-cert=DIRECTORY" << endl
-    << "              specify an alternate certificate database for module signing" << endl
     << "   --skip-badvars" << endl
     << "              overlook context of bad $ variables" << endl
     << endl
@@ -408,7 +406,7 @@ main (int argc, char * const argv [])
   s.skip_badvars = false;
   s.unprivileged = false;
 
-  // Default location for our signing certificate.
+  // Location of our signing certificate.
   // If we're root, use the database in SYSCONFDIR, otherwise
   // use the one in our $HOME directory.  */
   if (getuid() == 0)
@@ -480,8 +478,7 @@ main (int argc, char * const argv [])
 #define LONG_OPT_IGNORE_DWARF 4
 #define LONG_OPT_VERBOSE_PASS 5
 #define LONG_OPT_SKIP_BADVARS 6
-#define LONG_OPT_SIGNING_CERT 7
-#define LONG_OPT_UNPRIVILEGED 8
+#define LONG_OPT_UNPRIVILEGED 7
       // NB: also see find_hash(), usage(), switch stmt below, stap.1 man page
       static struct option long_options[] = {
         { "kelf", 0, &long_opt, LONG_OPT_KELF },
@@ -490,7 +487,6 @@ main (int argc, char * const argv [])
         { "ignore-dwarf", 0, &long_opt, LONG_OPT_IGNORE_DWARF },
 	{ "skip-badvars", 0, &long_opt, LONG_OPT_SKIP_BADVARS },
         { "vp", 1, &long_opt, LONG_OPT_VERBOSE_PASS },
-        { "signing-cert", 2, &long_opt, LONG_OPT_SIGNING_CERT },
         { "unprivileged", 0, &long_opt, LONG_OPT_UNPRIVILEGED },
         { NULL, 0, NULL, 0 }
       };
@@ -736,30 +732,6 @@ main (int argc, char * const argv [])
 	    case LONG_OPT_SKIP_BADVARS:
 	      s.skip_badvars = true;
 	      break;
-            case LONG_OPT_SIGNING_CERT:
-#if HAVE_NSS
-              if (optarg)
-		{
-		  string arg = optarg;
-		  string::size_type len = arg.length();
-
-		  // Make sure the name is not empty (i.e. --signing-cert= )
-		  if (len == 0)
-		    {
-		      cerr << "Certificate database directory name for --signing-cert can not be empty." << endl;
-		      usage (s, 1);
-		    }
-
-		  s.cert_db_path = arg;
-
-		  // Chop off any trailing '/'.
-		  if (len > 1 && s.cert_db_path.substr(len - 1, 1) == "/")
-		    s.cert_db_path.erase(len - 1);
-		}
-#else
-	      cerr << "WARNING: Module signing is disabled. The required nss libraries are not available." << endl;
-#endif
-	      break;
 	    case LONG_OPT_UNPRIVILEGED:
 	      s.unprivileged = true;
 	      s.guru_mode = false;
@@ -1182,19 +1154,17 @@ main (int argc, char * const argv [])
 		 << module_dest_path << "\"): " << strerror(errno) << endl;
 
 #if HAVE_NSS
-	  // Save the signature as well, if the module was signed.
-	  if (!s.cert_db_path.empty())
-	    {
-	      module_src_path += ".sgn";
-	      module_dest_path += ".sgn";
-
-	      if (s.verbose > 1)
-		clog << "Copying " << module_src_path << " to "
-		     << module_dest_path << endl;
-	      if (copy_file(module_src_path.c_str(), module_dest_path.c_str()) != 0)
-		cerr << "Copy failed (\"" << module_src_path << "\" to \""
-		     << module_dest_path << "\"): " << strerror(errno) << endl;
-	    }
+	  // Save the signature as well.
+	  assert (! s.cert_db_path.empty());
+	  module_src_path += ".sgn";
+	  module_dest_path += ".sgn";
+
+	  if (s.verbose > 1)
+	    clog << "Copying " << module_src_path << " to "
+		 << module_dest_path << endl;
+	  if (copy_file(module_src_path.c_str(), module_dest_path.c_str()) != 0)
+	    cerr << "Copy failed (\"" << module_src_path << "\" to \""
+		 << module_dest_path << "\"): " << strerror(errno) << endl;
 #endif
 	}
     }
diff --git a/modsign.cxx b/modsign.cxx
index 34537921..1cb6ddf9 100644
--- a/modsign.cxx
+++ b/modsign.cxx
@@ -34,10 +34,247 @@ extern "C" {
 
 #include <stdio.h>
 #include <stdlib.h>
+
+#include <unistd.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <pwd.h>
 }
 
 using namespace std;
 
+/* Function: int check_cert_db_permissions (const string &cert_db_path);
+ * 
+ * Check that the given certificate directory and its contents have
+ * the correct permissions.
+ *
+ * Returns 0 if there is an error, 1 otherwise.
+ */
+static int
+check_cert_file_permissions (
+  const string &cert_file,
+  uid_t euid,
+  const struct passwd *pw
+) {
+  struct stat info;
+  int rc;
+
+  rc = stat (cert_file.c_str (), & info);
+  if (rc)
+    {
+      cerr << "Could not obtain information on certificate database " << cert_file << "." << endl;
+      perror ("");
+      return 0;
+    }
+
+  rc = 1; // ok
+
+  // We must be the owner of the file.
+  if (info.st_uid != euid)
+    {
+      cerr << "Certificate file " << cert_file << " must be owned by "
+	   << pw->pw_name << endl;
+      rc = 0;
+    }
+
+  // Check the access permissions of the file
+  if ((info.st_mode & S_IRUSR) == 0)
+    cerr << "Certificate file " << cert_file << " should be readable by the owner" << "."  << endl;
+  if ((info.st_mode & S_IWUSR) == 0)
+    cerr << "Certificate file " << cert_file << " should be writeable by the owner" << "."  << endl;
+  if ((info.st_mode & S_IXUSR) != 0)
+    {
+      cerr << "Certificate file " << cert_file << " must not be executable by the owner" << "."  << endl;
+      rc = 0;
+    }
+  if ((info.st_mode & S_IRGRP) == 0)
+    cerr << "Certificate file " << cert_file << " should be readable by the group" << "."  << endl;
+  if ((info.st_mode & S_IWGRP) != 0)
+    {
+      cerr << "Certificate file " << cert_file << " must not be writable by the group" << "."  << endl;
+      rc = 0;
+    }
+  if ((info.st_mode & S_IXGRP) != 0)
+    {
+      cerr << "Certificate file " << cert_file << " must not be executable by the group" << "."  << endl;
+      rc = 0;
+    }
+  if ((info.st_mode & S_IROTH) == 0)
+    cerr << "Certificate file " << cert_file << " should be readable by others" << "."  << endl;
+  if ((info.st_mode & S_IWOTH) != 0)
+    {
+      cerr << "Certificate file " << cert_file << " must not be writable by others" << "."  << endl;
+      rc = 0;
+    }
+  if ((info.st_mode & S_IXOTH) != 0)
+    {
+      cerr << "Certificate file " << cert_file << " must not be executable by others" << "."  << endl;
+      rc = 0;
+    }
+
+  return rc;
+}
+
+/* Function: int check_cert_db_permissions (const string &cert_db_path);
+ * 
+ * Check that the given certificate directory and its contents have
+ * the correct permissions.
+ *
+ * Returns 0 if there is an error, 1 otherwise.
+ */
+static int
+check_db_file_permissions (
+  const string &cert_db_file,
+  uid_t euid,
+  const struct passwd *pw
+) {
+  struct stat info;
+  int rc;
+
+  rc = stat (cert_db_file.c_str (), & info);
+  if (rc)
+    {
+      cerr << "Could not obtain information on certificate database file " << cert_db_file << "." << endl;
+      perror ("");
+      return 0;
+    }
+
+  rc = 1; // ok
+
+  // We must be the owner of the file.
+  if (info.st_uid != euid)
+    {
+      cerr << "Certificate database file " << cert_db_file << " must be owned by "
+	   << pw->pw_name << endl;
+      rc = 0;
+    }
+
+  // Check the access permissions of the file
+  if ((info.st_mode & S_IRUSR) == 0)
+    cerr << "Certificate database file " << cert_db_file << " should be readable by the owner" << "."  << endl;
+  if ((info.st_mode & S_IWUSR) == 0)
+    cerr << "Certificate database file " << cert_db_file << " should be writeable by the owner" << "."  << endl;
+  if ((info.st_mode & S_IXUSR) != 0)
+    {
+      cerr << "Certificate database file " << cert_db_file << " must not be executable by the owner" << "."  << endl;
+      rc = 0;
+    }
+  if ((info.st_mode & S_IRGRP) != 0)
+    {
+      cerr << "Certificate database file " << cert_db_file << " must not be readable by the group" << "."  << endl;
+      rc = 0;
+    }
+  if ((info.st_mode & S_IWGRP) != 0)
+    {
+      cerr << "Certificate database file " << cert_db_file << " must not be writable by the group" << "."  << endl;
+      rc = 0;
+    }
+  if ((info.st_mode & S_IXGRP) != 0)
+    {
+      cerr << "Certificate database file " << cert_db_file << " must not be executable by the group" << "."  << endl;
+      rc = 0;
+    }
+  if ((info.st_mode & S_IROTH) != 0)
+    {
+      cerr << "Certificate database file " << cert_db_file << " must not be readable by others" << "."  << endl;
+      rc = 0;
+    }
+  if ((info.st_mode & S_IWOTH) != 0)
+    {
+      cerr << "Certificate database file " << cert_db_file << " must not be writable by others" << "."  << endl;
+      rc = 0;
+    }
+  if ((info.st_mode & S_IXOTH) != 0)
+    {
+      cerr << "Certificate database file " << cert_db_file << " must not be executable by others" << "."  << endl;
+      rc = 0;
+    }
+
+  return rc;
+}
+
+/* Function: int check_cert_db_permissions (const string &cert_db_path);
+ * 
+ * Check that the given certificate directory and its contents have
+ * the correct permissions.
+ *
+ * Returns 0 if there is an error, 1 otherwise.
+ */
+static int
+check_cert_db_permissions (const string &cert_db_path) {
+  struct stat info;
+  const struct passwd *pw;
+  uid_t euid;
+  int rc;
+
+  rc = stat (cert_db_path.c_str (), & info);
+  if (rc)
+    {
+      cerr << "Could not obtain information on certificate database directory " << cert_db_path << "." << endl;
+      perror ("");
+      return 0;
+    }
+
+  rc = 1; // ok
+
+  // We must be the owner of the database.
+  euid = geteuid ();
+  if (info.st_uid != euid)
+    {
+      pw = getpwuid (euid);
+      if (pw)
+	{
+	  cerr << "Certificate database " << cert_db_path << " must be owned by "
+	       << pw->pw_name << endl;
+	}
+      else
+	{
+	  cerr << "Unable to obtain current user information which checking certificate database "
+	       << cert_db_path << endl;
+	  perror ("");
+	}
+      rc = 0;
+    }
+
+  // Check the database directory access permissions
+  if ((info.st_mode & S_IRUSR) == 0)
+    cerr << "Certificate database " << cert_db_path << " should be readable by the owner" << "."  << endl;
+  if ((info.st_mode & S_IWUSR) == 0)
+    cerr << "Certificate database " << cert_db_path << " should be writeable by the owner" << "."  << endl;
+  if ((info.st_mode & S_IXUSR) == 0)
+    cerr << "Certificate database " << cert_db_path << " should be searchable by the owner" << "."  << endl;
+  if ((info.st_mode & S_IRGRP) == 0)
+    cerr << "Certificate database " << cert_db_path << " should be readable by the group" << "."  << endl;
+  if ((info.st_mode & S_IWGRP) != 0)
+    {
+      cerr << "Certificate database " << cert_db_path << " must not be writable by the group" << "."  << endl;
+      rc = 0;
+    }
+  if ((info.st_mode & S_IXGRP) == 0)
+    cerr << "Certificate database " << cert_db_path << " should be searchable by the group" << "."  << endl;
+  if ((info.st_mode & S_IROTH) == 0)
+    cerr << "Certificate database " << cert_db_path << " should be readable by others" << "."  << endl;
+  if ((info.st_mode & S_IWOTH) != 0)
+    {
+      cerr << "Certificate database " << cert_db_path << " must not be writable by others" << "."  << endl;
+      rc = 0;
+    }
+  if ((info.st_mode & S_IXOTH) == 0)
+    cerr << "Certificate database " << cert_db_path << " should be searchable by others" << "."  << endl;
+
+  // Now check the permissions of the critical files.
+  rc &= check_db_file_permissions (cert_db_path + "/cert8.db", euid, pw);
+  rc &= check_db_file_permissions (cert_db_path + "/key3.db", euid, pw);
+  rc &= check_db_file_permissions (cert_db_path + "/secmod.db", euid, pw);
+  rc &= check_db_file_permissions (cert_db_path + "/pw", euid, pw);
+  rc &= check_cert_file_permissions (cert_db_path + "/stap.cert", euid, pw);
+
+  if (rc == 0)
+    cerr << "Unable to use certificate database " << cert_db_path << " due to errors" << "."  << endl;
+
+  return rc;
+}
+
 /* Function: int init_cert_db_path (const string &cert_db_path);
  * 
  * Initialize a certificate database at the given path.
@@ -55,10 +292,6 @@ init_cert_db_path (const string &cert_db_path) {
  */
 static int
 check_cert_db_path (const string &cert_db_path) {
-  static const char* keyFiles[] = {
-    "cert8.db", "key3.db", "pw", "secmod.db", "stap.cert", NULL
-  };
-
   // Does the path exist?
   PRFileInfo fileInfo;
   PRStatus prStatus = PR_GetFileInfo (cert_db_path.c_str(), &fileInfo);
@@ -78,15 +311,7 @@ check_cert_db_path (const string &cert_db_path) {
 	PR_Delete (fname.c_str ());
     }
 
-  // Does it contain the key files?
-  for (int i = 0; keyFiles[i]; ++i) {
-    fname = cert_db_path + "/" + keyFiles[i];
-    prStatus = PR_GetFileInfo (fname.c_str (), &fileInfo);
-    if (prStatus != PR_SUCCESS || fileInfo.type != PR_FILE_FILE || fileInfo.size < 0)
-      return init_cert_db_path (cert_db_path);
-  }
-
-  return 1; // ok
+  return check_cert_db_permissions (cert_db_path);
 }
 
 /* Function: char * password_callback()
diff --git a/stap-server.8.in b/stap-server.8.in
index 36bbb23b..2ec00c24 100644
--- a/stap-server.8.in
+++ b/stap-server.8.in
@@ -168,7 +168,7 @@ program requires a process id argument which identifies the server to be stopped
 
 .PP
 The
-.I stap\-authorize\-cert
+.I stap\-authorize\-server\-cert
 program accepts two arguments:
 
 .TP
@@ -222,7 +222,7 @@ For root users (EUID=0), it will be created in
 .I $sysconfdir/systemtap/ssl/server\fP.
 
 .IP \(bu 4
-At this time the
+At this time, the
 server will also create a local client\-side certificate database and add the
 server\[aq]s certificate to it. For non\-root users,
 this database will be created in
@@ -255,7 +255,7 @@ A user may add the certificate of a new trusted server to his own local
 client\-side certificate database using
 \[aq]\fBstap\-authorize\-server\-cert \fICERTFILE\fR\[aq]
 (see above), where \fICERTFILE\fP is the server\[aq]s certificate file
-(\fIstap\-server.cert\fP) from the servers certificate database directory and
+(\fIstap.cert\fP) from the server\[aq]s certificate database directory and
 \fIDIRNAME\fP is the
 directory containing the user\[aq]s client\-side certificate database.
 
@@ -316,11 +316,11 @@ simple example
 .PP
 To permanently trust a given server for your own use
 .PP
-.B \& $ stap\-authorize\-cert \fICERTFILE\fP
+.B \& $ stap\-authorize\-server\-cert \fICERTFILE\fP
 .PP
 As root, to permanently trust a given server for all users on your host
 .PP
-.B \& $ stap\-authorize\-cert \fICERTFILE\fP
+.B \& $ stap\-authorize\-server\-cert \fICERTFILE\fP
 .PP
 If a process id was echoed by
 .I stap\-start\-server
@@ -341,11 +341,9 @@ manual page for additional information on safety and security.
 .PP
 The systemtap server and its related utilities use the Secure Socket Layer
 (SSL) as implemented by Network Security Services (NSS)
-for network security and the NSS tools
+for network security. The NSS tool
 .I certutil
-and
-.I signtool
-for the generation of certificates and for signing respectively. The related
+is used for the generation of certificates. The related
 certificate databases must be protected in order to maintain the security of
 the system.
 Use of the utilities provided will help to ensure that the proper protection
@@ -359,8 +357,7 @@ access permissions before making use of any certificate database.
 .IR stapfuncs (3stap),
 .IR stapex (3stap),
 .IR NSS ,
-.IR certutil ,
-.IR signtool
+.IR certutil
 
 .SH BUGS
 Use the Bugzilla link off of the project web page or our mailing list.