From: David Smith <dsmith@redhat.com>
Date: Fri, 3 Dec 2010 18:52:41 +0000 (-0600)
Subject: Audit possible string buffer overruns.
X-Git-Tag: release-1.4~54
X-Git-Url: https://sourceware.org/git/?a=commitdiff_plain;h=459d6c80eeee6ceca58455fdf88d8e5a6b3479b1;p=systemtap.git

Audit possible string buffer overruns.

* runtime/transport/procfs.c (_stp_register_ctl_channel_fs): Change
  sprintf() call to snprintf() call.
  (_stp_unregister_ctl_channel_fs): Ditto.
* runtime/transport/ring_buffer.c (_stp_transport_data_fs_init): Ditto.
* runtime/unwind.c (_stp_eh_enc_name): Ditto.
* runtime/io.c: Make sure STP_LOG_BUF_LEN isn't too short.
* runtime/staprun/staprun_funcs.c: Add comments about strcpy/strcat/sprintf
  appropriateness.
  (check_signature): Fix string length calculation.
* runtime/staprun/common.c: Add comments about strcpy/strcat/sprintf
  appropriateness.
* runtime/staprun/modverify.c: Ditto.
* runtime/staprun/relay.c: Ditto.
* runtime/staprun/relay_old.c: Ditto.
* runtime/staprun/staprun_funcs.c: Ditto.
---

diff --git a/runtime/io.c b/runtime/io.c
index 0136aae56..b133620e2 100644
--- a/runtime/io.c
+++ b/runtime/io.c
@@ -20,6 +20,10 @@
 
 #define WARN_STRING "WARNING: "
 #define ERR_STRING "ERROR: "
+#if (STP_LOG_BUF_LEN < 10) /* sizeof(WARN_STRING) */
+#error "STP_LOG_BUF_LEN is too short"
+#endif
+
 enum code { INFO=0, WARN, ERROR, DBUG };
 
 static void _stp_vlog (enum code type, const char *func, int line, const char *fmt, va_list args)
@@ -34,9 +38,13 @@ static void _stp_vlog (enum code type, const char *func, int line, const char *f
 	if (type == DBUG) {
 		start = _stp_snprintf(buf, STP_LOG_BUF_LEN, "%s:%d: ", func, line);
 	} else if (type == WARN) {
+		/* This strcpy() is OK, since we know STP_LOG_BUF_LEN
+		 * is > sizeof(WARN_STRING). */
 		strcpy (buf, WARN_STRING);
 		start = sizeof(WARN_STRING) - 1;
 	} else if (type == ERROR) {
+		/* This strcpy() is OK, since we know STP_LOG_BUF_LEN
+		 * is > sizeof(ERR_STRING) (which is < sizeof(WARN_STRING). */
 		strcpy (buf, ERR_STRING);
 		start = sizeof(ERR_STRING) - 1;
 	}
diff --git a/runtime/staprun/common.c b/runtime/staprun/common.c
index f5fbf2286..4fbc9e5c4 100644
--- a/runtime/staprun/common.c
+++ b/runtime/staprun/common.c
@@ -52,6 +52,8 @@ static char *get_abspath(char *path)
 	if (len + 2 + strlen(path) >= PATH_MAX)
 		return NULL;
 	path_buf[len] = '/';
+	/* Note that this strcpy() call is OK, since we checked
+	 * the length earlier to make sure the string would fit. */
 	strcpy(&path_buf[len + 1], path);
 	return path_buf;
 }
@@ -81,6 +83,9 @@ int make_outfile_name(char *buf, int max, int fnum, int cpu, time_t t, int bulk)
 	}
 	/* special case: for testing we sometimes want to write to /dev/null */
 	if (strcmp(outfile_name, "/dev/null") == 0) {
+		/* This strcpy() call is OK since we know that the
+		 * buffer is at least PATH_MAX bytes long at this
+		 * point. */
 		strcpy(buf, "/dev/null");
 	} else {
 		if (bulk) {
diff --git a/runtime/staprun/modverify.c b/runtime/staprun/modverify.c
index 3fa3047b7..402371f34 100644
--- a/runtime/staprun/modverify.c
+++ b/runtime/staprun/modverify.c
@@ -188,6 +188,8 @@ check_cert_db_permissions (const char *cert_db_path) {
       return 0;
     }
 
+  /* These uses of sprintf() are OK, since we just allocated the
+   * string to be the correct length. */
   sprintf (fileName, "%s/cert8.db", cert_db_path);
   rc &= check_db_file_permissions (fileName);
   sprintf (fileName, "%s/key3.db", cert_db_path);
diff --git a/runtime/staprun/relay.c b/runtime/staprun/relay.c
index 28fba651d..ec4ba28a8 100644
--- a/runtime/staprun/relay.c
+++ b/runtime/staprun/relay.c
@@ -295,6 +295,9 @@ int init_relayfs(void)
 			if (outfile_name) {
 				/* special case: for testing we sometimes want to write to /dev/null */
 				if (strcmp(outfile_name, "/dev/null") == 0) {
+					/* This strcpy() is OK, since
+					 * we know buf is PATH_MAX
+					 * bytes long. */
 					strcpy(buf, "/dev/null");
 				} else {
 					len = stap_strfloctime(buf, PATH_MAX,
diff --git a/runtime/staprun/relay_old.c b/runtime/staprun/relay_old.c
index 8dfcc16b4..85cff6dfe 100644
--- a/runtime/staprun/relay_old.c
+++ b/runtime/staprun/relay_old.c
@@ -163,6 +163,8 @@ static int open_relayfs_files(int cpu, const char *relay_filebase, const char *p
 		/* special case: for testing we sometimes want to
 		 * write to /dev/null */
 		if (strcmp(outfile_name, "/dev/null") == 0) {
+			/* This strcpy() is OK, since we know tmp is
+			 * PATH_MAX bytes long. */
 			strcpy(tmp, "/dev/null");
 		} else {
 			int len;
diff --git a/runtime/staprun/staprun_funcs.c b/runtime/staprun/staprun_funcs.c
index 9771986b8..df464d0a1 100644
--- a/runtime/staprun/staprun_funcs.c
+++ b/runtime/staprun/staprun_funcs.c
@@ -73,6 +73,8 @@ int insert_module(
 			_perr("[re]allocating memory failed");
 			return -1;
 		}
+		/* Note that these strcat() calls are OK, since we just
+		 * allocated space for the resulting string. */
 		strcat(opts, " ");
 		strcat(opts, options[i]);
 	}
@@ -247,10 +249,12 @@ check_signature(const char *path, const void *module_data, off_t module_size)
 
   /* Add the .sgn suffix to the canonicalized module path to get the signature
      file path.  */
-  if (strlen (path) >= PATH_MAX - 4) {
+  if (strlen (path) >= PATH_MAX - 5) {
     err("Path \"%s.sgn\" is too long.", path);
     return -1;
   }
+  /* This use of sprintf() is OK, since we just checked the final
+   * string's length. */
   sprintf (signature_realpath, "%s.sgn", path);
 
   rc = verify_module (signature_realpath, path, module_data, module_size);
@@ -301,6 +305,8 @@ check_stap_module_path(const char *module_path, int module_fd)
 	 * /lib/modules/`uname -r`/systemtapmod.ko, put a '/' on the
 	 * end of staplib_dir_realpath. */
 	if (strlen(staplib_dir_realpath) < (PATH_MAX - 1))
+		/* Note that this strcat() is OK, since we just
+		 * checked the length of the resulting string.  */
 		strcat(staplib_dir_realpath, "/");
 	else {
 		err("ERROR: Path \"%s\" is too long.", staplib_dir_realpath);
diff --git a/runtime/transport/procfs.c b/runtime/transport/procfs.c
index 9e05cc147..491017318 100644
--- a/runtime/transport/procfs.c
+++ b/runtime/transport/procfs.c
@@ -110,7 +110,7 @@ static int _stp_register_ctl_channel_fs(void)
 #ifdef STP_BULKMODE
 	/* now for each cpu "n", create /proc/systemtap/module_name/n  */
 	stp_for_each_cpu(i) {
-		sprintf(buf, "%d", i);
+		snprintf(buf, sizeof(buf), "%d", i);
 		de = create_proc_entry(buf, 0600, _stp_proc_root);
 		if (de == NULL)
 			goto err1;
@@ -144,7 +144,7 @@ err1:
 	stp_for_each_cpu(j) {
 		if (j == i)
 			break;
-		sprintf(buf, "%d", j);
+		snprintf(buf, sizeof(buf), "%d", j);
 		remove_proc_entry(buf, _stp_proc_root);
 
 	}
@@ -168,7 +168,7 @@ static void _stp_unregister_ctl_channel_fs(void)
 		_stp_kfree(de->data);
 
 	stp_for_each_cpu(i) {
-		sprintf(buf, "%d", i);
+		snprintf(buf, sizeof(buf), "%d", i);
 		remove_proc_entry(buf, _stp_proc_root);
 	}
 	remove_proc_entry("bufsize", _stp_proc_root);
diff --git a/runtime/transport/ring_buffer.c b/runtime/transport/ring_buffer.c
index 232f09993..f0d195cb7 100644
--- a/runtime/transport/ring_buffer.c
+++ b/runtime/transport/ring_buffer.c
@@ -696,7 +696,7 @@ static int _stp_transport_data_fs_init(void)
 			_stp_transport_data_fs_close();
 			return -EINVAL;
 		}
-		sprintf(cpu_file, "trace%d", cpu);
+		snprintf(cpu_file, sizeof(cpu_file), "trace%d", cpu);
 		__stp_entry[cpu] = debugfs_create_file(cpu_file, 0600,
 						       _stp_get_module_dir(),
 						       (void *)(long)cpu,
diff --git a/runtime/unwind.c b/runtime/unwind.c
index da39223a8..3e569657c 100644
--- a/runtime/unwind.c
+++ b/runtime/unwind.c
@@ -532,7 +532,7 @@ static char *_stp_eh_enc_name(signed type)
 	hi = (type & DW_EH_PE_ADJUST) >> 4;
 	low = type & DW_EH_PE_FORM;
 	if (hi > 5 || low > 4 || (low == 0 && (type & DW_EH_PE_signed))) {
-		sprintf(buf, "ERROR:encoding=0x%x", type);
+	    snprintf(buf, sizeof(buf), "ERROR:encoding=0x%x", type);
 		return buf;
 	}