[OpenWrt-Devel] [PATCH v3] [netifd] vlan: Buffer overlow in snprintf for vlans

[cshored at thecshore.com]

From: "Daniel F. Dickinson" <cshored at thecshore.com>

Buffer overflow condition can occur because vlan
device name is constructed from device name (size IFNAMSIZ)
plus the ASCII decimal representation of the vlan id plus
a dot, but the target can only be IFNAMSIZ.

Note that the generic device name code must also be updated
or vlan id's may be truncated for device like vlan on
top of bridges.

Credit to "Andrey Melnikov" <temnota.am at gmail.com> for
a better solution than the original patch I sent for the
vlan.c part.

Signed-off-by: Daniel F. Dickinson <cshored at thecshore.com>
---
 device.c | 11 ++++++++++-
 vlan.c   |  3 ++-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/device.c b/device.c
index a851037..9f8ffa2 100644
--- a/device.c
+++ b/device.c
@@ -660,6 +660,7 @@ void device_set_ifindex(struct device *dev, int ifindex)
 int device_set_ifname(struct device *dev, const char *name)
 {
 	int ret = 0;
+	char *vlandot = 0;
 
 	if (!strcmp(dev->ifname, name))
 		return 0;
@@ -667,7 +668,15 @@ int device_set_ifname(struct device *dev, const char *name)
 	if (dev->avl.key)
 		avl_delete(&devices, &dev->avl);
 
-	strncpy(dev->ifname, name, IFNAMSIZ);
+	if ((vlandot = strchr(name, '.'))) {
+	  /* Copy the part of name that will leave space for a vlan id
+           * the dot, and terminating null. NB. max vlan id is 4095
+           */
+	  strncpy(dev->ifname, name, IFNAMSIZ - strnlen(vlandot, 5) - 1);
+	  strncat(dev->ifname, vlandot, 5);
+	} else {
+	  strncpy(dev->ifname, name, IFNAMSIZ);
+	}
 
 	if (dev->avl.key)
 		ret = avl_insert(&devices, &dev->avl);
diff --git a/vlan.c b/vlan.c
index 067f624..8bacc8f 100644
--- a/vlan.c
+++ b/vlan.c
@@ -66,7 +66,8 @@ static void vlan_dev_set_name(struct vlan_device *vldev, struct device *dev)
 	char name[IFNAMSIZ];
 
 	vldev->dev.hidden = dev->hidden;
-	snprintf(name, IFNAMSIZ, "%s.%d", dev->ifname, vldev->id);
+	/* Truncate dev->ifname so that adding dot + VLAN id + '\0' doesn't exceed IFNAMSIZ */
+	snprintf(name, sizeof(name), "%.*s.%d", IFNAMSIZ - 6, dev->ifname, vldev->id & 0xfff);
 	device_set_ifname(&vldev->dev, name);
 }
 
-- 
2.11.0
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel