[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