[Lxc-users] [PATCH 1/1] switch all sprintfs which can overrun to snprintfs

Serge Hallyn serge.hallyn at canonical.com
Thu Apr 26 19:31:40 UTC 2012


Otherwise code like https://code.launchpad.net/~frankban/lpsetup/lp-lxc-ip
can trivially make this code (get_init_pid() in this case) overflow.

This is on top of the patchset I sent yesterday.

Signed-off-by: Serge Hallyn <serge.hallyn at ubuntu.com>
---
 src/lxc/cgroup.c      |   67 +++++++++++++++++++++++++++++++-------
 src/lxc/commands.c    |   16 ++++++++--
 src/lxc/conf.c        |   85 +++++++++++++++++++++++++++++++++++++++----------
 src/lxc/freezer.c     |    6 +++-
 src/lxc/lxc_monitor.c |   11 +++++--
 src/lxc/network.c     |   10 ++++--
 src/lxc/state.c       |    4 ++-
 7 files changed, 164 insertions(+), 35 deletions(-)

diff --git a/src/lxc/cgroup.c b/src/lxc/cgroup.c
index 9af199d..8933c69 100644
--- a/src/lxc/cgroup.c
+++ b/src/lxc/cgroup.c
@@ -176,8 +176,10 @@ static int get_cgroup_mount(const char *subsystem, char *mnt)
 				       get_init_cgroup(subsystem, NULL,
 						       initcgroup),
 				       (flags & CGROUP_NS_CGROUP) ? "" : "/lxc");
-			if (ret < 0 || ret >= MAXPATHLEN)
+			if (ret < 0 || ret >= MAXPATHLEN) {
+				ERROR("name too long");
 				goto fail;
+			}
 			fclose(file);
 			DEBUG("using cgroup mounted at '%s'", mnt);
 			return 0;
@@ -207,12 +209,16 @@ static int cgroup_rename_nsgroup(const char *mnt, const char *name, pid_t pid)
 	int ret;
 
 	ret = snprintf(oldname, MAXPATHLEN, "%s/%d", mnt, pid);
-	if (ret >= MAXPATHLEN)
+	if (ret >= MAXPATHLEN) {
+		ERROR("pathname too long");
 		return -1;
+	}
 
 	ret = snprintf(newname, MAXPATHLEN, "%s/%s", mnt, name);
-	if (ret >= MAXPATHLEN)
+	if (ret >= MAXPATHLEN) {
+		ERROR("pathname too long");
 		return -1;
+	}
 
 	if (rename(oldname, newname)) {
 		SYSERROR("failed to rename cgroup %s->%s", oldname, newname);
@@ -250,8 +256,13 @@ int lxc_cgroup_attach(const char *path, pid_t pid)
 	FILE *f;
 	char tasks[MAXPATHLEN];
 	int ret = 0;
+	int rc;
 
-	snprintf(tasks, MAXPATHLEN, "%s/tasks", path);
+	rc = snprintf(tasks, MAXPATHLEN, "%s/tasks", path);
+	if (rc < 0 || rc >= MAXPATHLEN) {
+		ERROR("pathname too long");
+		return -1;
+	}
 
 	f = fopen(tasks, "w");
 	if (!f) {
@@ -445,6 +456,7 @@ int recursive_rmdir(char *dirname)
 
 	while (!readdir_r(dir, &dirent, &direntp)) {
 		struct stat mystat;
+		int rc;
 
 		if (!direntp)
 			break;
@@ -453,7 +465,11 @@ int recursive_rmdir(char *dirname)
 		    !strcmp(direntp->d_name, ".."))
 			continue;
 
-		snprintf(pathname, MAXPATHLEN, "%s/%s", dirname, direntp->d_name);
+		rc = snprintf(pathname, MAXPATHLEN, "%s/%s", dirname, direntp->d_name);
+		if (rc < 0 || rc >= MAXPATHLEN) {
+			ERROR("pathname too long");
+			continue;
+		}
 		ret = stat(pathname, &mystat);
 		if (ret)
 			continue;
@@ -475,10 +491,15 @@ int lxc_one_cgroup_destroy(struct mntent *mntent, const char *name)
 	char cgname[MAXPATHLEN], initcgroup[MAXPATHLEN];
 	char *cgmnt = mntent->mnt_dir;
 	int flags = get_cgroup_flags(mntent);
+	int rc;
 
-	snprintf(cgname, MAXPATHLEN, "%s%s%s/%s", cgmnt,
+	rc = snprintf(cgname, MAXPATHLEN, "%s%s%s/%s", cgmnt,
 		get_init_cgroup(NULL, mntent, initcgroup),
 		(flags & CGROUP_NS_CGROUP) ? "" : "/lxc", name);
+	if (rc < 0 || rc >= MAXPATHLEN) {
+		ERROR("path name too long");
+		return -1;
+	}
 	DEBUG("destroying %s\n", cgname);
 	if (recursive_rmdir(cgname)) {
 		SYSERROR("failed to remove cgroup '%s'", cgname);
@@ -529,11 +550,16 @@ int lxc_cgroup_path_get(char **path, const char *subsystem, const char *name)
 {
 	static char        buf[MAXPATHLEN];
 	static char        retbuf[MAXPATHLEN];
+	int rc;
 
 	/* what lxc_cgroup_set calls subsystem is actually the filename, i.e.
 	   'devices.allow'.  So for our purposee we trim it */
 	if (subsystem) {
-		snprintf(retbuf, MAXPATHLEN, "%s", subsystem);
+		rc = snprintf(retbuf, MAXPATHLEN, "%s", subsystem);
+		if (rc < 0 || rc >= MAXPATHLEN) {
+			ERROR("subsystem name too long");
+			return -1;
+		}
 		char *s = index(retbuf, '.');
 		if (s)
 			*s = '\0';
@@ -544,7 +570,11 @@ int lxc_cgroup_path_get(char **path, const char *subsystem, const char *name)
 		return -1;
 	}
 
-	snprintf(retbuf, MAXPATHLEN, "%s/%s", buf, name);
+	rc = snprintf(retbuf, MAXPATHLEN, "%s/%s", buf, name);
+	if (rc < 0 || rc >= MAXPATHLEN) {
+		ERROR("name too long");
+		return -1;
+	}
 
 	DEBUG("%s: returning %s for subsystem %s", __func__, retbuf, subsystem);
 
@@ -557,12 +587,17 @@ int lxc_cgroup_set(const char *name, const char *filename, const char *value)
 	int fd, ret;
 	char *dirpath;
 	char path[MAXPATHLEN];
+	int rc;
 
 	ret = lxc_cgroup_path_get(&dirpath, filename, name);
 	if (ret)
 		return -1;
 
-	snprintf(path, MAXPATHLEN, "%s/%s", dirpath, filename);
+	rc = snprintf(path, MAXPATHLEN, "%s/%s", dirpath, filename);
+	if (rc < 0 || rc >= MAXPATHLEN) {
+		ERROR("pathname too long");
+		return -1;
+	}
 
 	fd = open(path, O_WRONLY);
 	if (fd < 0) {
@@ -588,12 +623,17 @@ int lxc_cgroup_get(const char *name, const char *filename,
 	int fd, ret = -1;
 	char *dirpath;
 	char path[MAXPATHLEN];
+	int rc;
 
 	ret = lxc_cgroup_path_get(&dirpath, filename, name);
 	if (ret)
 		return -1;
 
-	snprintf(path, MAXPATHLEN, "%s/%s", dirpath, filename);
+	rc = snprintf(path, MAXPATHLEN, "%s/%s", dirpath, filename);
+	if (rc < 0 || rc >= MAXPATHLEN) {
+		ERROR("pathname too long");
+		return -1;
+	}
 
 	fd = open(path, O_RDONLY);
 	if (fd < 0) {
@@ -615,12 +655,17 @@ int lxc_cgroup_nrtasks(const char *name)
 	char path[MAXPATHLEN];
 	int pid, ret, count = 0;
 	FILE *file;
+	int rc;
 
 	ret = lxc_cgroup_path_get(&dpath, NULL, name);
 	if (ret)
 		return -1;
 
-	snprintf(path, MAXPATHLEN, "%s/tasks", dpath);
+	rc = snprintf(path, MAXPATHLEN, "%s/tasks", dpath);
+	if (rc < 0 || rc >= MAXPATHLEN) {
+		ERROR("pathname too long");
+		return -1;
+	}
 
 	file = fopen(path, "r");
 	if (!file) {
diff --git a/src/lxc/commands.c b/src/lxc/commands.c
index 1d488ae..cce24db 100644
--- a/src/lxc/commands.c
+++ b/src/lxc/commands.c
@@ -75,8 +75,14 @@ static int __lxc_command(const char *name, struct lxc_command *command,
 	int sock, ret = -1;
 	char path[sizeof(((struct sockaddr_un *)0)->sun_path)] = { 0 };
 	char *offset = &path[1];
+	int rc, len;
 
-	sprintf(offset, abstractname, name);
+	len = sizeof(path)-1;
+	rc = snprintf(offset, len, abstractname, name);
+	if (rc < 0 || rc >= len) {
+		ERROR("Name too long");
+		return -1;
+	}
 
 	sock = lxc_af_unix_connect(path);
 	if (sock < 0 && errno == ECONNREFUSED) {
@@ -266,8 +272,14 @@ extern int lxc_command_mainloop_add(const char *name,
 	int ret, fd;
 	char path[sizeof(((struct sockaddr_un *)0)->sun_path)] = { 0 };
 	char *offset = &path[1];
+	int rc, len;
 
-	sprintf(offset, abstractname, name);
+	len = sizeof(path)-1;
+	rc = snprintf(offset, len, abstractname, name);
+	if (rc < 0 || rc >= len) {
+		ERROR("Name too long");
+		return -1;
+	}
 
 	fd = lxc_af_unix_open(path, SOCK_STREAM, 0);
 	if (fd < 0) {
diff --git a/src/lxc/conf.c b/src/lxc/conf.c
index 87f7adc..9cc5f6b 100644
--- a/src/lxc/conf.c
+++ b/src/lxc/conf.c
@@ -241,11 +241,25 @@ static int run_script(const char *name, const char *section,
 		return -1;
 	}
 
-	ret = sprintf(buffer, "%s %s %s", script, name, section);
+	ret = snprintf(buffer, size, "%s %s %s", script, name, section);
+	if (ret < 0 || ret >= size) {
+		ERROR("Script name too long");
+		free(buffer);
+		return -1;
+	}
 
 	va_start(ap, script);
-	while ((p = va_arg(ap, char *)))
-		ret += sprintf(buffer + ret, " %s", p);
+	while ((p = va_arg(ap, char *))) {
+		int len = size-ret;
+		int rc;
+		rc = snprintf(buffer + ret, len, " %s", p);
+		if (rc < 0 || rc >= len) {
+			free(buffer);
+			ERROR("Script args too long");
+			return -1;
+		}
+		ret += rc;
+	}
 	va_end(ap);
 
 	f = popen(buffer, "r");
@@ -391,7 +405,7 @@ static int mount_rootfs_file(const char *rootfs, const char *target)
 {
 	struct dirent dirent, *direntp;
 	struct loop_info64 loinfo;
-	int ret = -1, fd = -1;
+	int ret = -1, fd = -1, rc;
 	DIR *dir;
 	char path[MAXPATHLEN];
 
@@ -415,7 +429,10 @@ static int mount_rootfs_file(const char *rootfs, const char *target)
 		if (strncmp(direntp->d_name, "loop", 4))
 			continue;
 
-		sprintf(path, "/dev/%s", direntp->d_name);
+		rc = snprintf(path, MAXPATHLEN, "/dev/%s", direntp->d_name);
+		if (rc < 0 || rc >= MAXPATHLEN)
+			continue;
+
 		fd = open(path, O_RDWR);
 		if (fd < 0)
 			continue;
@@ -581,7 +598,7 @@ static int setup_tty(const struct lxc_rootfs *rootfs,
 		}
 		if (ttydir) {
 			/* create dev/lxc/tty%d" */
-			snprintf(lxcpath, sizeof(lxcpath), "%s/dev/%s/tty%d",
+			ret = snprintf(lxcpath, sizeof(lxcpath), "%s/dev/%s/tty%d",
 				 rootfs->mount, ttydir, i + 1);
 			if (ret >= sizeof(lxcpath)) {
 				ERROR("pathname too long for ttys");
@@ -605,7 +622,11 @@ static int setup_tty(const struct lxc_rootfs *rootfs,
 				continue;
 			}
 
-			snprintf(lxcpath, sizeof(lxcpath), "%s/tty%d", ttydir, i+1);
+			ret = snprintf(lxcpath, sizeof(lxcpath), "%s/tty%d", ttydir, i+1);
+			if (ret >= sizeof(lxcpath)) {
+				ERROR("tty pathname too long");
+				return -1;
+			}
 			ret = symlink(lxcpath, path);
 			if (ret) {
 				SYSERROR("failed to create symlink for tty %d\n", i+1);
@@ -686,12 +707,17 @@ static int umount_oldrootfs(const char *oldrootfs)
 	void *cbparm[2];
 	struct lxc_list mountlist, *iterator;
 	int ok, still_mounted, last_still_mounted;
+	int rc;
 
 	/* read and parse /proc/mounts in old root fs */
 	lxc_list_init(&mountlist);
 
 	/* oldrootfs is on the top tree directory now */
-	snprintf(path, sizeof(path), "/%s", oldrootfs);
+	rc = snprintf(path, sizeof(path), "/%s", oldrootfs);
+	if (rc >= sizeof(path)) {
+		ERROR("rootfs name too long");
+		return -1;
+	}
 	cbparm[0] = &mountlist;
 
 	cbparm[1] = strdup(path);
@@ -700,7 +726,11 @@ static int umount_oldrootfs(const char *oldrootfs)
 		return -1;
 	}
 
-	snprintf(path, sizeof(path), "%s/proc/mounts", oldrootfs);
+	rc = snprintf(path, sizeof(path), "%s/proc/mounts", oldrootfs);
+	if (rc >= sizeof(path)) {
+		ERROR("container proc/mounts name too long");
+		return -1;
+	}
 
 	ok = lxc_file_for_each_line(path,
 				    setup_rootfs_pivot_root_cb, &cbparm);
@@ -754,6 +784,7 @@ static int setup_rootfs_pivot_root(const char *rootfs, const char *pivotdir)
 {
 	char path[MAXPATHLEN];
 	int remove_pivotdir = 0;
+	int rc;
 
 	/* change into new root fs */
 	if (chdir(rootfs)) {
@@ -765,7 +796,11 @@ static int setup_rootfs_pivot_root(const char *rootfs, const char *pivotdir)
 		pivotdir = "mnt";
 
 	/* compute the full path to pivotdir under rootfs */
-	snprintf(path, sizeof(path), "%s/%s", rootfs, pivotdir);
+	rc = snprintf(path, sizeof(path), "%s/%s", rootfs, pivotdir);
+	if (rc >= sizeof(path)) {
+		ERROR("pivot dir name too long");
+		return -1;
+	}
 
 	if (access(path, F_OK)) {
 
@@ -988,7 +1023,11 @@ static int setup_ttydir_console(const struct lxc_rootfs *rootfs,
 	}
 
 	/* create symlink from rootfs/dev/console to 'lxc/console' */
-	snprintf(lxcpath, sizeof(lxcpath), "%s/console", ttydir);
+	ret = snprintf(lxcpath, sizeof(lxcpath), "%s/console", ttydir);
+	if (ret >= sizeof(lxcpath)) {
+		ERROR("lxc/console path too long");
+		return -1;
+	}
 	ret = symlink(lxcpath, path);
 	if (ret) {
 		SYSERROR("failed to create symlink for console");
@@ -1182,7 +1221,7 @@ skipvarlib:
 
 skipabs:
 
-	snprintf(path, MAXPATHLEN, "%s/%s", rootfs->mount,
+	r = snprintf(path, MAXPATHLEN, "%s/%s", rootfs->mount,
 		 aux + offset);
 	if (r < 0 || r >= MAXPATHLEN) {
 		WARN("pathnme too long for '%s'", mntent->mnt_dir);
@@ -1213,7 +1252,11 @@ static int mount_entry_on_relative_rootfs(struct mntent *mntent,
 	}
 
         /* relative to root mount point */
-	snprintf(path, sizeof(path), "%s/%s", rootfs, mntent->mnt_dir);
+	ret = snprintf(path, sizeof(path), "%s/%s", rootfs, mntent->mnt_dir);
+	if (ret >= sizeof(path)) {
+		ERROR("path name too long");
+		return -1;
+	}
 
 	ret = mount_entry(mntent->mnt_fsname, path, mntent->mnt_type,
 			  mntflags, mntdata);
@@ -1679,7 +1722,11 @@ static int instanciate_veth(struct lxc_handler *handler, struct lxc_netdev *netd
 	if (netdev->priv.veth_attr.pair)
 		veth1 = netdev->priv.veth_attr.pair;
 	else {
-		snprintf(veth1buf, sizeof(veth1buf), "vethXXXXXX");
+		err = snprintf(veth1buf, sizeof(veth1buf), "vethXXXXXX");
+		if (err >= sizeof(veth1buf)) { /* can't *really* happen, but... */
+			ERROR("veth1 name too long");
+			return -1;
+		}
 		veth1 = mktemp(veth1buf);
 	}
 
@@ -1767,7 +1814,9 @@ static int instanciate_macvlan(struct lxc_handler *handler, struct lxc_netdev *n
 		return -1;
 	}
 
-	snprintf(peerbuf, sizeof(peerbuf), "mcXXXXXX");
+	err = snprintf(peerbuf, sizeof(peerbuf), "mcXXXXXX");
+	if (err >= sizeof(peerbuf))
+		return -1;
 
 	peer = mktemp(peerbuf);
 	if (!strlen(peer)) {
@@ -1814,7 +1863,11 @@ static int instanciate_vlan(struct lxc_handler *handler, struct lxc_netdev *netd
 		return -1;
 	}
 
-	snprintf(peer, sizeof(peer), "vlan%d", netdev->priv.vlan_attr.vid);
+	err = snprintf(peer, sizeof(peer), "vlan%d", netdev->priv.vlan_attr.vid);
+	if (err >= sizeof(peer)) {
+		ERROR("peer name too long");
+		return -1;
+	}
 
 	err = lxc_vlan_create(netdev->link, peer, netdev->priv.vlan_attr.vid);
 	if (err) {
diff --git a/src/lxc/freezer.c b/src/lxc/freezer.c
index 94984a3..2a6f0f5 100644
--- a/src/lxc/freezer.c
+++ b/src/lxc/freezer.c
@@ -49,7 +49,11 @@ static int freeze_unfreeze(const char *name, int freeze)
 	if (ret)
 		return -1;
 
-	snprintf(freezer, MAXPATHLEN, "%s/freezer.state", nsgroup);
+	ret = snprintf(freezer, MAXPATHLEN, "%s/freezer.state", nsgroup);
+	if (ret >= MAXPATHLEN) {
+		ERROR("freezer.state name too long");
+		return -1;
+	}
 
 	fd = open(freezer, O_RDWR);
 	if (fd < 0) {
diff --git a/src/lxc/lxc_monitor.c b/src/lxc/lxc_monitor.c
index 3802d2e..10168fb 100644
--- a/src/lxc/lxc_monitor.c
+++ b/src/lxc/lxc_monitor.c
@@ -60,6 +60,7 @@ int main(int argc, char *argv[])
 	struct lxc_msg msg;
 	regex_t preg;
 	int fd;
+	int len, rc;
 
 	if (lxc_arguments_parse(&my_args, argc, argv))
 		return -1;
@@ -68,12 +69,18 @@ int main(int argc, char *argv[])
 			 my_args.progname, my_args.quiet))
 		return -1;
 
-	regexp = malloc(strlen(my_args.name) + 3);
+	len = strlen(my_args.name) + 3;
+	regexp = malloc(len + 3);
 	if (!regexp) {
 		ERROR("failed to allocate memory");
 		return -1;
 	}
-	sprintf(regexp, "^%s$", my_args.name);
+	rc = snprintf(regexp, len, "^%s$", my_args.name);
+	if (rc < 0 || rc >= len) {
+		ERROR("Name too long");
+		free(regexp);
+		return -1;
+	}
 
 	if (regcomp(&preg, regexp, REG_NOSUB|REG_EXTENDED)) {
 		ERROR("failed to compile the regex '%s'", my_args.name);
diff --git a/src/lxc/network.c b/src/lxc/network.c
index 214460b..c8aecfb 100644
--- a/src/lxc/network.c
+++ b/src/lxc/network.c
@@ -582,12 +582,15 @@ static int proc_sys_net_write(const char *path, const char *value)
 static int ip_forward_set(const char *ifname, int family, int flag)
 {
 	char path[MAXPATHLEN];
+	int rc;
 
 	if (family != AF_INET && family != AF_INET6)
 		return -EINVAL;
 
-	snprintf(path, MAXPATHLEN, "/proc/sys/net/%s/conf/%s/forwarding",
+	rc = snprintf(path, MAXPATHLEN, "/proc/sys/net/%s/conf/%s/forwarding",
 		 family == AF_INET?"ipv4":"ipv6" , ifname);
+	if (rc >= MAXPATHLEN)
+		return -E2BIG;
 
 	return proc_sys_net_write(path, flag?"1":"0");
 }
@@ -605,13 +608,16 @@ int lxc_ip_forward_off(const char *ifname, int family)
 static int neigh_proxy_set(const char *ifname, int family, int flag)
 {
 	char path[MAXPATHLEN];
+	int ret;
 
 	if (family != AF_INET && family != AF_INET6)
 		return -EINVAL;
 
-	sprintf(path, "/proc/sys/net/%s/conf/%s/%s",
+	ret = snprintf(path, MAXPATHLEN, "/proc/sys/net/%s/conf/%s/%s",
 		family == AF_INET?"ipv4":"ipv6" , ifname,
 		family == AF_INET?"proxy_arp":"proxy_ndp");
+	if (ret < 0 || ret >= MAXPATHLEN)
+		return -E2BIG;
 
 	return proc_sys_net_write(path, flag?"1":"0");
 }
diff --git a/src/lxc/state.c b/src/lxc/state.c
index b435eba..9983656 100644
--- a/src/lxc/state.c
+++ b/src/lxc/state.c
@@ -75,7 +75,9 @@ static int freezer_state(const char *name)
 	if (err)
 		return -1;
 
-	snprintf(freezer, MAXPATHLEN, "%s/freezer.state", nsgroup);
+	err = snprintf(freezer, MAXPATHLEN, "%s/freezer.state", nsgroup);
+	if (err < 0 || err >= MAXPATHLEN)
+		return -1;
 
 	file = fopen(freezer, "r");
 	if (!file)
-- 
1.7.9.5





More information about the lxc-users mailing list