diff mbox series

[ovs-dev,3/6] lldp: fix a buffer overflow when handling management address TLV

Message ID 20201026205745.56604-4-fdangelo@redhat.com
State Changes Requested
Headers show
Series Incorporate fixes from lldpd upstream | expand

Commit Message

Fabrizio D'Angelo Oct. 26, 2020, 8:57 p.m. UTC 
From: Vincent Bernat <vincent@bernat.im>

Upstream commit:
	commit a8d8006c06d9ac16ebcf33295cbd625c0847ca9b
	Author: Vincent Bernat <vincent@bernat.im>
	Date: Sun, 4 Oct 2015 01:50:38 +0200

	lldp: fix a buffer overflow when handling management address TLV

	When a remote device was advertising a too large management address
	while still respecting TLV boundaries, lldpd would crash due to a buffer
	overflow. However, the buffer being a static one, this buffer overflow
	is not exploitable if hardening was not disabled. This bug exists since
	version 0.5.6.

Co-authored-by: Fabrizio D'Angelo <fdangelo@redhat.com>
Signed-off-by: Fabrizio D'Angelo <fdangelo@redhat.com>
---
 lib/lldp/lldp.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

0-day Robot Oct. 26, 2020, 10:05 p.m. UTC | #1 
Bleep bloop.  Greetings Fabrizio D'Angelo, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
ERROR: Author Vincent Bernat <vincent@bernat.im> needs to sign off.
Lines checked: 52, Warnings: 0, Errors: 1


build:
                 VLOG_WARN(name "too large management address on %s",
                 ^
lib/lldp/lldp.c:534:27: note: each undeclared identifier is reported only once for each function it appears in
                 VLOG_WARN(name "too large management address on %s",
                           ^
./include/openvswitch/vlog.h:280:41: note: in definition of macro 'VLOG'
             vlog(&this_module, level__, __VA_ARGS__);   \
                                         ^
lib/lldp/lldp.c:534:17: note: in expansion of macro 'VLOG_WARN'
                 VLOG_WARN(name "too large management address on %s",
                 ^
lib/lldp/lldp.c:534:32: error: expected ')' before string constant
                 VLOG_WARN(name "too large management address on %s",
                                ^
./include/openvswitch/vlog.h:280:41: note: in definition of macro 'VLOG'
             vlog(&this_module, level__, __VA_ARGS__);   \
                                         ^
lib/lldp/lldp.c:534:17: note: in expansion of macro 'VLOG_WARN'
                 VLOG_WARN(name "too large management address on %s",
                 ^
make[2]: *** [lib/lldp/lldp.lo] Error 1
make[2]: Leaving directory `/var/lib/jenkins/jobs/0day_robot_upstream_build_from_pw/workspace'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/var/lib/jenkins/jobs/0day_robot_upstream_build_from_pw/workspace'
make: *** [all] Error 2


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Aaron Conole Oct. 27, 2020, 12:54 p.m. UTC | #2 
Hi Fabrizio,

Thanks for the work on this.

Fabrizio D'Angelo <fdangelo@redhat.com> writes:

> From: Vincent Bernat <vincent@bernat.im>
>
> Upstream commit:
> 	commit a8d8006c06d9ac16ebcf33295cbd625c0847ca9b
> 	Author: Vincent Bernat <vincent@bernat.im>
> 	Date: Sun, 4 Oct 2015 01:50:38 +0200
>
> 	lldp: fix a buffer overflow when handling management address TLV
>
> 	When a remote device was advertising a too large management address
> 	while still respecting TLV boundaries, lldpd would crash due to a buffer
> 	overflow. However, the buffer being a static one, this buffer overflow
> 	is not exploitable if hardening was not disabled. This bug exists since
> 	version 0.5.6.
>
> Co-authored-by: Fabrizio D'Angelo <fdangelo@redhat.com>
> Signed-off-by: Fabrizio D'Angelo <fdangelo@redhat.com>
> ---
>  lib/lldp/lldp.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/lib/lldp/lldp.c b/lib/lldp/lldp.c
> index 593c5e1c34..aaa8ec842a 100644
> --- a/lib/lldp/lldp.c
> +++ b/lib/lldp/lldp.c
> @@ -530,6 +530,11 @@ lldp_decode(struct lldpd *cfg OVS_UNUSED, char *frame, int s,
>          case LLDP_TLV_MGMT_ADDR:
>              CHECK_TLV_SIZE(1, "Management address");
>              addr_str_length = PEEK_UINT8;
> +            if (addr_str_length > sizeof(addr_str_buffer)) {
> +                VLOG_WARN(name "too large management address on %s",
> +                      hardware->h_ifname);

Two things about this change.

1. remove 'name', which seems to have been leftover from converting.
   This causes a build error.

2. Align the hardware->h_ifname argument so that it will look like:

+                VLOG_WARN("too large management address on %s",
+                          hardware->h_ifname);


> +                goto malformed;
> +            }
>              CHECK_TLV_SIZE(1 + addr_str_length, "Management address");
>              PEEK_BYTES(addr_str_buffer, addr_str_length);
>              addr_length = addr_str_length - 1;
> @@ -554,7 +559,7 @@ lldp_decode(struct lldpd *cfg OVS_UNUSED, char *frame, int s,
>              break;
>  
>          case LLDP_TLV_ORG:
> -            CHECK_TLV_SIZE(4, "Organisational");
> +            CHECK_TLV_SIZE(1 + (int)sizeof(orgid), "Organisational");
>              PEEK_BYTES(orgid, sizeof orgid);
>              tlv_subtype = PEEK_UINT8;
>              if (memcmp(dot1, orgid, sizeof orgid) == 0) {
diff mbox series

Patch

diff --git a/lib/lldp/lldp.c b/lib/lldp/lldp.c
index 593c5e1c34..aaa8ec842a 100644
--- a/lib/lldp/lldp.c
+++ b/lib/lldp/lldp.c
@@ -530,6 +530,11 @@  lldp_decode(struct lldpd *cfg OVS_UNUSED, char *frame, int s,
         case LLDP_TLV_MGMT_ADDR:
             CHECK_TLV_SIZE(1, "Management address");
             addr_str_length = PEEK_UINT8;
+            if (addr_str_length > sizeof(addr_str_buffer)) {
+                VLOG_WARN(name "too large management address on %s",
+                      hardware->h_ifname);
+                goto malformed;
+            }
             CHECK_TLV_SIZE(1 + addr_str_length, "Management address");
             PEEK_BYTES(addr_str_buffer, addr_str_length);
             addr_length = addr_str_length - 1;
@@ -554,7 +559,7 @@  lldp_decode(struct lldpd *cfg OVS_UNUSED, char *frame, int s,
             break;
 
         case LLDP_TLV_ORG:
-            CHECK_TLV_SIZE(4, "Organisational");
+            CHECK_TLV_SIZE(1 + (int)sizeof(orgid), "Organisational");
             PEEK_BYTES(orgid, sizeof orgid);
             tlv_subtype = PEEK_UINT8;
             if (memcmp(dot1, orgid, sizeof orgid) == 0) {