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

[John Crispin]

On 02/02/18 08:29, cshored at thecshore.com wrote:
> 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>

Hi cshore,

NAK on this one. truncating the name to make '.$vid' fit at the end is 
not an option. we can run into lots of weird issues doing so. please 
make the code error out gracefully and be verbose about it when 
strlen(dev->name + '.' * $vid) > 15.

     John
> ---
>   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);
>   }
>   
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel