diff mbox series

[OpenWrt-Devel,netifd] vlan: Array out of bounds in snprintf for vlans

Message ID 20180130181649.17813-1-cshored@thecshore.com
State Superseded, archived
Headers show
Series [OpenWrt-Devel,netifd] vlan: Array out of bounds in snprintf for vlans | expand

Commit Message

Daniel Dickinson Jan. 30, 2018, 6:16 p.m. UTC 
From: "Daniel F. Dickinson" <cshored@thecshore.com>

Detected during a side project.  Not a brilliant fix, but it
gets the job done for now.  *very* lightly tested, more
for your information than anything else.

Array out-of-bounds 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.  We fix this
by using fields widths (and make sure we don't truncate
more of the orogin device name than we must).

Signed-off-by: Daniel F. Dickinson <cshored@thecshore.com>
---
 vlan.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Daniel Dickinson Jan. 30, 2018, 6:24 p.m. UTC | #1 
On 30/01/18 01:16 PM, cshored@thecshore.com wrote:
> From: "Daniel F. Dickinson" <cshored@thecshore.com>
> 
> Detected during a side project.  Not a brilliant fix, but it
> gets the job done for now.  *very* lightly tested, more
> for your information than anything else.

Dammit, made an off-by-one in the fix....will update and resend...
Paul Oranje Jan. 31, 2018, 11:20 a.m. UTC | #2 
Why use a hard coded value 4 in "snprintf(devnum, 4, "%d", vldev->id);" ?
Paul

> Op 30 jan. 2018, om 19:16 heeft cshored@thecshore.com het volgende geschreven:
> 
> From: "Daniel F. Dickinson" <cshored@thecshore.com>
> 
> Detected during a side project.  Not a brilliant fix, but it
> gets the job done for now.  *very* lightly tested, more
> for your information than anything else.
> 
> Array out-of-bounds 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.  We fix this
> by using fields widths (and make sure we don't truncate
> more of the orogin device name than we must).
> 
> Signed-off-by: Daniel F. Dickinson <cshored@thecshore.com>
> ---
> vlan.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/vlan.c b/vlan.c
> index 067f624..44852f4 100644
> --- a/vlan.c
> +++ b/vlan.c
> @@ -63,10 +63,17 @@ static int vlan_set_device_state(struct device *dev, bool up)
> 
> static void vlan_dev_set_name(struct vlan_device *vldev, struct device *dev)
> {
> -	char name[IFNAMSIZ];
> +	char name[IFNAMSIZ + 1];
> +	char devnum[5];
> +	int i, j = 0;
> 
> 	vldev->dev.hidden = dev->hidden;
> -	snprintf(name, IFNAMSIZ, "%s.%d", dev->ifname, vldev->id);
> +	snprintf(devnum, 4, "%d", vldev->id);
> +	i = strnlen(devnum, 4);
> +	j = IFNAMSIZ - i;
> +	strncpy(name, dev->ifname, j);
> +	strncat(name, ".", 1);
> +	strncat(name, devnum, i);
> 	device_set_ifname(&vldev->dev, name);
> }
> 
> -- 
> 2.11.0
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
Daniel Dickinson Feb. 1, 2018, 7:29 a.m. UTC | #3 
On 31/01/18 06:20 AM, Paul Oranje wrote:
> Why use a hard coded value 4 in "snprintf(devnum, 4, "%d", vldev->id);" ?
> Paul

Max value for a VLAN id is 4095 = 4 digits, although probably better 
would be to accept full length for int and truncate in the next line.

That and this was a quick hack to get around a compile error with GCC 
2.7 (because of the buffer overrun), and not a fully formed patch; as 
stated, in the message this was more informational.

I suppose I should find some time and energy and submit a fully formed 
patch, unless someone gets there first.

Regards,

Daniel

> 
>> Op 30 jan. 2018, om 19:16 heeft cshored@thecshore.com het volgende geschreven:
>>
>> From: "Daniel F. Dickinson" <cshored@thecshore.com>
>>
>> Detected during a side project.  Not a brilliant fix, but it
>> gets the job done for now.  *very* lightly tested, more
>> for your information than anything else.
>>
>> Array out-of-bounds 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.  We fix this
>> by using fields widths (and make sure we don't truncate
>> more of the orogin device name than we must).
>>
>> Signed-off-by: Daniel F. Dickinson <cshored@thecshore.com>
>> ---
>> vlan.c | 11 +++++++++--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/vlan.c b/vlan.c
>> index 067f624..44852f4 100644
>> --- a/vlan.c
>> +++ b/vlan.c
>> @@ -63,10 +63,17 @@ static int vlan_set_device_state(struct device *dev, bool up)
>>
>> static void vlan_dev_set_name(struct vlan_device *vldev, struct device *dev)
>> {
>> -	char name[IFNAMSIZ];
>> +	char name[IFNAMSIZ + 1];
>> +	char devnum[5];
>> +	int i, j = 0;
>>
>> 	vldev->dev.hidden = dev->hidden;
>> -	snprintf(name, IFNAMSIZ, "%s.%d", dev->ifname, vldev->id);
>> +	snprintf(devnum, 4, "%d", vldev->id);
>> +	i = strnlen(devnum, 4);
>> +	j = IFNAMSIZ - i;
>> +	strncpy(name, dev->ifname, j);
>> +	strncat(name, ".", 1);
>> +	strncat(name, devnum, i);
>> 	device_set_ifname(&vldev->dev, name);
>> }
>>
>> -- 
>> 2.11.0
>> _______________________________________________
>> openwrt-devel mailing list
>> openwrt-devel@lists.openwrt.org
>> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
>
Daniel Dickinson Feb. 1, 2018, 11:54 a.m. UTC | #4 
On 01/02/18 02:29 AM, Daniel F. Dickinson wrote:
> On 31/01/18 06:20 AM, Paul Oranje wrote:
>> Why use a hard coded value 4 in "snprintf(devnum, 4, "%d", vldev->id);" ?
>> Paul
> 

Oh I see this also the uglier first throw-together; there is a v2 that 
isn't as bad (and is actually right; this version actually is wrong but 
I tested the wrong thing (short end not long end), and didn't notice the 
issue until I came back and reviewed for titch longer.

Regards,

Daniel
diff mbox series

Patch

diff --git a/vlan.c b/vlan.c
index 067f624..44852f4 100644
--- a/vlan.c
+++ b/vlan.c
@@ -63,10 +63,17 @@  static int vlan_set_device_state(struct device *dev, bool up)
 
 static void vlan_dev_set_name(struct vlan_device *vldev, struct device *dev)
 {
-	char name[IFNAMSIZ];
+	char name[IFNAMSIZ + 1];
+	char devnum[5];
+	int i, j = 0;
 
 	vldev->dev.hidden = dev->hidden;
-	snprintf(name, IFNAMSIZ, "%s.%d", dev->ifname, vldev->id);
+	snprintf(devnum, 4, "%d", vldev->id);
+	i = strnlen(devnum, 4);
+	j = IFNAMSIZ - i;
+	strncpy(name, dev->ifname, j);
+	strncat(name, ".", 1);
+	strncat(name, devnum, i);
 	device_set_ifname(&vldev->dev, name);
 }