Merge branch 'bug17852_revised'

changes/bug17852

@@ -0,0 +1,10 @@
+  o Minor features (code hardening):
+    - Use tor_snprintf() and tor_vsnprintf() even in external and
+      low-level code, to harden against accidental failures to NUL-
+      terminate. Part of ticket 17852. Patch from 'jsturgix'. Found
+      with Flawfinder.
+
+  o Minor bugfixes (private directory):
+    - Prevent a race condition when creating private directories.
+      Fixes part of bug 17852; bugfix on 0.2pre13. Part of ticket
+      17852. Patch from 'jsturgix'. Found with Flawfinder.

src/common/compat.c

@@ -576,14 +576,17 @@ tor_vasprintf(char **strp, const char *fmt, va_list args)
   int len, r;
   va_list tmp_args;
   va_copy(tmp_args, args);
-  len = vsnprintf(buf, sizeof(buf), fmt, tmp_args);
+  /* vsnprintf() was properly checked but tor_vsnprintf() available so
+   * why not use it? */
+  len = tor_vsnprintf(buf, sizeof(buf), fmt, tmp_args);
   va_end(tmp_args);
   if (len < (int)sizeof(buf)) {
     *strp = tor_strdup(buf);
     return len;
   }
   strp_tmp = tor_malloc(len+1);
-  r = vsnprintf(strp_tmp, len+1, fmt, args);
+  /* use of tor_vsnprintf() will ensure string is null terminated */
+  r = tor_vsnprintf(strp_tmp, len+1, fmt, args);
   if (r != len) {
     tor_free(strp_tmp);
     *strp = NULL;

src/common/util.c

@@ -2056,9 +2056,10 @@ int
 check_private_dir(const char *dirname, cpd_check_t check,
                   const char *effective_user)
 {
+  int fd;
   int r;
   struct stat st;
-  char *f;
+  //char *f;
 #ifndef _WIN32
   unsigned unwanted_bits = 0;
   const struct passwd *pw = NULL;
@@ -2068,18 +2069,34 @@ check_private_dir(const char *dirname, cpd_check_t check,
   (void)effective_user;
 #endif
 
+  /*
+   * Goal is to harden the implementation by removing any
+   * potential for race between stat() and chmod().
+   * chmod() accepts filename as argument. If an attacker can move
+   * the file between stat() and chmod(), a potential race exists.
+   *
+   * Several suggestions taken from:
+   * https://developer.apple.com/library/mac/documentation/Security/Conceptual/SecureCodingGuide/Articles/RaceConditions.html
+   */
   tor_assert(dirname);
-  f = tor_strdup(dirname);
-  clean_name_for_stat(f);
-  log_debug(LD_FS, "stat()ing %s", f);
-  r = stat(sandbox_intern_string(f), &st);
-  tor_free(f);
-  if (r) {
+
+  /* Open directory. 
+   * O_NOFOLLOW to ensure that it does not follow symbolic links */
+  fd = open(sandbox_intern_string(dirname), O_NOFOLLOW);
+
+  /* Was there an error? Maybe the directory does not exist? */
+  if (fd == -1) {
+
     if (errno != ENOENT) {
+      /* Other directory error */
       log_warn(LD_FS, "Directory %s cannot be read: %s", dirname,
                strerror(errno));
       return -1;
     }
+
+    /* Received ENOENT: Directory does not exist */
+
+    /* Should we create the directory? */
     if (check & CPD_CREATE) {
       log_info(LD_GENERAL, "Creating directory %s", dirname);
 #if defined (_WIN32)
@@ -2091,23 +2108,51 @@ check_private_dir(const char *dirname, cpd_check_t check,
         r = mkdir(dirname, 0700);
       }
 #endif
+
+      /* check for mkdir() error */
       if (r) {
         log_warn(LD_FS, "Error creating directory %s: %s", dirname,
             strerror(errno));
         return -1;
       }
-    } else if (!(check & CPD_CHECK)) {
+
+      /* we just created the directory. try to open it again.
+       * permissions on the directory will be checked again below.*/
+      fd = open(sandbox_intern_string(dirname), O_NOFOLLOW);
+
+      if ( fd == -1 ) return -1;
+
+    } else if (!(check & CPD_CHECK)) { 
       log_warn(LD_FS, "Directory %s does not exist.", dirname);
       return -1;
     }
+
     /* XXXX In the case where check==CPD_CHECK, we should look at the
      * parent directory a little harder. */
     return 0;
   }
+
+  tor_assert(fd);
+
+  //f = tor_strdup(dirname);
+  //clean_name_for_stat(f);
+  log_debug(LD_FS, "stat()ing %s", dirname);
+  //r = stat(sandbox_intern_string(f), &st);
+  r = fstat(fd, &st);
+  if (r == -1) {
+      log_warn(LD_FS, "fstat() on directory %s failed.", dirname);
+      close(fd);
+      return -1;
+  }
+  //tor_free(f);
+
+  /* check that dirname is a directory */
   if (!(st.st_mode & S_IFDIR)) {
     log_warn(LD_FS, "%s is not a directory", dirname);
+    close(fd);
     return -1;
   }
+
 #ifndef _WIN32
   if (effective_user) {
     /* Look up the user and group information.
@@ -2124,7 +2169,6 @@ check_private_dir(const char *dirname, cpd_check_t check,
     running_uid = getuid();
     running_gid = getgid();
   }
-
   if (st.st_uid != running_uid) {
     const struct passwd *pw = NULL;
     char *process_ownername = NULL;
@@ -2140,6 +2184,7 @@ check_private_dir(const char *dirname, cpd_check_t check,
                          pw ? pw->pw_name : "<unknown>", (int)st.st_uid);
 
     tor_free(process_ownername);
+    close(fd);
     return -1;
   }
   if ( (check & (CPD_GROUP_OK|CPD_GROUP_READ))
@@ -2156,6 +2201,7 @@ check_private_dir(const char *dirname, cpd_check_t check,
              gr ?  gr->gr_name : "<unknown>", (int)st.st_gid);
 
     tor_free(process_groupname);
+    close(fd);
     return -1;
   }
   if (check & (CPD_GROUP_OK|CPD_GROUP_READ)) {
@@ -2168,6 +2214,7 @@ check_private_dir(const char *dirname, cpd_check_t check,
     if (check & CPD_CHECK_MODE_ONLY) {
       log_warn(LD_FS, "Permissions on directory %s are too permissive.",
                dirname);
+      close(fd);
       return -1;
     }
     log_warn(LD_FS, "Fixing permissions on directory %s", dirname);
@@ -2177,15 +2224,18 @@ check_private_dir(const char *dirname, cpd_check_t check,
       new_mode |= 0050; /* Group should have rx */
     }
     new_mode &= ~unwanted_bits; /* Clear the bits that we didn't want set...*/
-    if (chmod(dirname, new_mode)) {
+    if (fchmod(fd, new_mode)) {
       log_warn(LD_FS, "Could not chmod directory %s: %s", dirname,
                strerror(errno));
+      close(fd);
       return -1;
     } else {
+      close(fd);
       return 0;
     }
   }
 #endif
+  close(fd);
   return 0;
 }
 
@@ -2900,6 +2950,9 @@ expand_filename(const char *filename)
 {
   tor_assert(filename);
 #ifdef _WIN32
+  /* Might consider using GetFullPathName() as described here:
+   * http://etutorials.org/Programming/secure+programming/Chapter+3.+Input+Validation/3.7+Validating+Filenames+and+Paths/
+   */
   return tor_strdup(filename);
 #else
   if (*filename == '~') {

src/ext/eventdns.c

@@ -388,7 +388,7 @@ debug_ntoa(u32 address)
 {
 	static char buf[32];
 	u32 a = ntohl(address);
-	snprintf(buf, sizeof(buf), "%d.%d.%d.%d",
+	tor_snprintf(buf, sizeof(buf), "%d.%d.%d.%d",
 			(int)(u8)((a>>24)&0xff),
 			(int)(u8)((a>>16)&0xff),
 			(int)(u8)((a>>8 )&0xff),
@@ -436,12 +436,7 @@ evdns_log(int warn, const char *fmt, ...)
 	if (!evdns_log_fn)
 		return;
 	va_start(args,fmt);
-#ifdef _WIN32
-	_vsnprintf(buf, sizeof(buf), fmt, args);
-#else
-	vsnprintf(buf, sizeof(buf), fmt, args);
-#endif
-	buf[sizeof(buf)-1] = '\0';
+	tor_vsnprintf(buf, sizeof(buf), fmt, args);
 	evdns_log_fn(warn, buf);
 	va_end(args);
 }
@@ -762,7 +757,7 @@ reply_handle(struct evdns_request *const req, u16 flags, u32 ttl, struct reply *
 			/* we regard these errors as marking a bad nameserver */
 			if (req->reissue_count < global_max_reissues) {
 				char msg[64];
-				snprintf(msg, sizeof(msg), "Bad response %d (%s)",
+				tor_snprintf(msg, sizeof(msg), "Bad response %d (%s)",
 						 error, evdns_err_to_string(error));
 				nameserver_failed(req->ns, msg);
 				if (!request_reissue(req)) return;
@@ -1705,7 +1700,7 @@ evdns_server_request_add_ptr_reply(struct evdns_server_request *req, struct in_a
 	assert(!(in && inaddr_name));
 	if (in) {
 		a = ntohl(in->s_addr);
-		snprintf(buf, sizeof(buf), "%d.%d.%d.%d.in-addr.arpa",
+		tor_snprintf(buf, sizeof(buf), "%d.%d.%d.%d.in-addr.arpa",
 				(int)(u8)((a	)&0xff),
 				(int)(u8)((a>>8 )&0xff),
 				(int)(u8)((a>>16)&0xff),
@@ -2638,7 +2633,7 @@ int evdns_resolve_reverse(const struct in_addr *in, int flags, evdns_callback_ty
 	u32 a;
 	assert(in);
 	a = ntohl(in->s_addr);
-	snprintf(buf, sizeof(buf), "%d.%d.%d.%d.in-addr.arpa",
+	tor_snprintf(buf, sizeof(buf), "%d.%d.%d.%d.in-addr.arpa",
 			(int)(u8)((a	)&0xff),
 			(int)(u8)((a>>8 )&0xff),
 			(int)(u8)((a>>16)&0xff),