diff mbox series

[ovs-dev] lldp: fix bugs when parsing malformed AutoAttach

Message ID 20221219192712.210350-1-aconole@redhat.com
State Superseded
Headers show
Series [ovs-dev] lldp: fix bugs when parsing malformed AutoAttach | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Aaron Conole Dec. 19, 2022, 7:27 p.m. UTC 
From: Qian Chen <cq674350529@163.com>

The OVS LLDP implementation includes support for AutoAttach standard, which
the 'upstream' lldpd project does not include.  As part of adding this
support, the message parsing for these TLVs did not include proper length
checks for the LLDP_TLV_AA_ELEMENT_SUBTYPE and the
LLDP_TLV_AA_ISID_VLAN_ASGNS_SUBTYPE elements.  The result is that a message
without a proper boundary will cause an over read of memory, and lead to
undefined results, including crashes or other unidentified behavior.

The fix is to introduce proper bounds checking for these elements.  Introduce
a unit test to ensure that we have some proper rejection in this code
base in the future.

Fixes: be53a5c447c3 ("auto-attach: Initial support for Auto-Attach standard")
Signed-off-by: Qian Chen <cq674350529@163.com>
Co-authored-by: Aaron Conole <aconole@redhat.com>
Signed-off-by: Aaron Conole <aconole@redhat.com>
---
NOTES: This bug is publicly known and disclosed at
       https://github.com/openvswitch/ovs/pull/405 which makes this mostly
       a repost.
       I have modified the test case to ensure that it would run
       correctly when doing both 'make check-kernel' and
       'make check-system-userspace'

 lib/lldp/lldp.c         |  2 ++
 tests/system-traffic.at | 27 +++++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

Comments

Ilya Maximets Dec. 19, 2022, 8:39 p.m. UTC | #1 
On 12/19/22 20:27, Aaron Conole wrote:
> From: Qian Chen <cq674350529@163.com>
> 
> The OVS LLDP implementation includes support for AutoAttach standard, which
> the 'upstream' lldpd project does not include.  As part of adding this
> support, the message parsing for these TLVs did not include proper length
> checks for the LLDP_TLV_AA_ELEMENT_SUBTYPE and the
> LLDP_TLV_AA_ISID_VLAN_ASGNS_SUBTYPE elements.  The result is that a message
> without a proper boundary will cause an over read of memory, and lead to
> undefined results, including crashes or other unidentified behavior.
> 
> The fix is to introduce proper bounds checking for these elements.  Introduce
> a unit test to ensure that we have some proper rejection in this code
> base in the future.
> 
> Fixes: be53a5c447c3 ("auto-attach: Initial support for Auto-Attach standard")
> Signed-off-by: Qian Chen <cq674350529@163.com>
> Co-authored-by: Aaron Conole <aconole@redhat.com>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
> NOTES: This bug is publicly known and disclosed at
>        https://github.com/openvswitch/ovs/pull/405 which makes this mostly
>        a repost.
>        I have modified the test case to ensure that it would run
>        correctly when doing both 'make check-kernel' and
>        'make check-system-userspace'
> 
>  lib/lldp/lldp.c         |  2 ++
>  tests/system-traffic.at | 27 +++++++++++++++++++++++++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/lib/lldp/lldp.c b/lib/lldp/lldp.c
> index dfeb2a8002..6fdcfef569 100644
> --- a/lib/lldp/lldp.c
> +++ b/lib/lldp/lldp.c
> @@ -583,6 +583,7 @@ lldp_decode(struct lldpd *cfg OVS_UNUSED, char *frame, int s,
>  
>                  switch(tlv_subtype) {
>                  case LLDP_TLV_AA_ELEMENT_SUBTYPE:
> +                    CHECK_TLV_SIZE(50, "ELEMENT");
>                      PEEK_BYTES(&msg_auth_digest, sizeof msg_auth_digest);
>  
>                      aa_element_dword = PEEK_UINT32;
> @@ -629,6 +630,7 @@ lldp_decode(struct lldpd *cfg OVS_UNUSED, char *frame, int s,
>                      break;
>  
>                  case LLDP_TLV_AA_ISID_VLAN_ASGNS_SUBTYPE:
> +                    CHECK_TLV_SIZE(36, "ISID_VLAN_ASGNS");
>                      PEEK_BYTES(&msg_auth_digest, sizeof msg_auth_digest);
>  
>                      /* Subtract off tlv type and length (2Bytes) + OUI (3B) +
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index e5403519f2..0928bfe540 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -7440,3 +7440,30 @@ OVS_WAIT_UNTIL([cat p2.pcap | grep -E "0x0050: *0000 *0000 *5002 *2000 *b85e *00
>  
>  OVS_TRAFFIC_VSWITCHD_STOP
>  AT_CLEANUP
> +
> +AT_SETUP([autoattach - malformed lldp])
> +OVS_TRAFFIC_VSWITCHD_START()
> +
> +ADD_NAMESPACES(at_ns0)
> +
> +dnl Set up simple bridge port to receive lldp packets
> +ADD_VETH(p0, at_ns0, br0, "172.31.1.1/24", "f6:b4:26:aa:5f:00")
> +
> +NETNS_DAEMONIZE([at_ns0], [tcpdump -l -n -xx -U -i p0 > p0.pcap], [tcpdump.pid])
> +sleep 1
> +
> +dnl Enable lldp
> +AT_CHECK([ovs-vsctl set interface ovs-p0 lldp:enable=true])
> +
> +dnl Send a malformed lldp packet
> +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p0 01 80 c2 00 00 0e f6 b4 26 aa 5f 00 88 cc 02 07 04 f6 b4 26 aa 5f 00 04 03 05 76 32 06 02 00 78 0c 50 44 45 41 44 42 45 45 46 44 45 41 44 42 45 45 46 44 45 41 44 42 45 45 46 44 45 41 44 42 45 45 46 44 45 41 44 42 45 45 46 44 45 41 44 42 45 45 46 44 45 41 44 42 45 45 46 44 45 41 44 42 45 45 46 44 45 41 44 42 45 45 46 44 45 41 44 42 45 45 46 fe 05 00 04 0d 0c 01 00 00 >/dev/null])
> +
> +dnl Check the expected lldp packet by looking for the end
> +OVS_WAIT_UNTIL([cat p0.pcap | grep -E "0x0070: *4546 *fe05 *0004 *0d0c *0100 *00" 2>&1 1>/dev/null])
> +
> +AT_CHECK([grep -o "ISID_VLAN_ASGNS TLV too short" ovs-vswitchd.log], [0], [dnl
> +ISID_VLAN_ASGNS TLV too short
> +])
> +
> +OVS_TRAFFIC_VSWITCHD_STOP(["/|WARN|ISID_VLAN_ASGNS TLV too short received on ovs-p0/d"])
> +AT_CLEANUP

Do we actually need a system test here?
It looks like it can be converted to a simple unit test.  E.g.:

diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index eb4cd1896..41741d324 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -11966,3 +11966,25 @@ AT_CHECK([test 1 = `ovs-ofctl parse-pcap p2-tx.pcap | wc -l`])
 
 OVS_VSWITCHD_STOP
 AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - malformed lldp])
+OVS_VSWITCHD_START
+add_of_ports br0 1
+
+AT_CHECK([ovs-ofctl add-flow br0 action=normal])
+
+dnl Enable lldp.
+AT_CHECK([ovs-vsctl set interface p1 lldp:enable=true])
+
+dnl Send a malformed lldp packet.
+packet="0180c200000ef6b426aa5f0088cc020704f6b426aa5f000403057632060200780c"dnl
+"5044454144424545464445414442454546444541444245454644454144424545464445414"dnl
+"4424545464445414442454546444541444245454644454144424545464445414442454546"dnl
+"4445414442454546fe0500040d0c010000"
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 "$packet"], [0], [stdout])
+
+dnl Check that the packet was flagged as invalid.
+OVS_WAIT_UNTIL([grep -q 'ISID_VLAN_ASGNS TLV too short' ovs-vswitchd.log])
+
+OVS_VSWITCHD_STOP(["/|WARN|ISID_VLAN_ASGNS TLV too short received on p1/d"])
+AT_CLEANUP
---

What do you think?

Best regards, Ilya Maximets.
Aaron Conole Dec. 20, 2022, 2:32 p.m. UTC | #2 
Ilya Maximets <i.maximets@ovn.org> writes:

> On 12/19/22 20:27, Aaron Conole wrote:
>> From: Qian Chen <cq674350529@163.com>
>> 
>> The OVS LLDP implementation includes support for AutoAttach standard, which
>> the 'upstream' lldpd project does not include.  As part of adding this
>> support, the message parsing for these TLVs did not include proper length
>> checks for the LLDP_TLV_AA_ELEMENT_SUBTYPE and the
>> LLDP_TLV_AA_ISID_VLAN_ASGNS_SUBTYPE elements.  The result is that a message
>> without a proper boundary will cause an over read of memory, and lead to
>> undefined results, including crashes or other unidentified behavior.
>> 
>> The fix is to introduce proper bounds checking for these elements.  Introduce
>> a unit test to ensure that we have some proper rejection in this code
>> base in the future.
>> 
>> Fixes: be53a5c447c3 ("auto-attach: Initial support for Auto-Attach standard")
>> Signed-off-by: Qian Chen <cq674350529@163.com>
>> Co-authored-by: Aaron Conole <aconole@redhat.com>
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> ---
>> NOTES: This bug is publicly known and disclosed at
>>        https://github.com/openvswitch/ovs/pull/405 which makes this mostly
>>        a repost.
>>        I have modified the test case to ensure that it would run
>>        correctly when doing both 'make check-kernel' and
>>        'make check-system-userspace'
>> 
>>  lib/lldp/lldp.c         |  2 ++
>>  tests/system-traffic.at | 27 +++++++++++++++++++++++++++
>>  2 files changed, 29 insertions(+)
>> 
>> diff --git a/lib/lldp/lldp.c b/lib/lldp/lldp.c
>> index dfeb2a8002..6fdcfef569 100644
>> --- a/lib/lldp/lldp.c
>> +++ b/lib/lldp/lldp.c
>> @@ -583,6 +583,7 @@ lldp_decode(struct lldpd *cfg OVS_UNUSED, char *frame, int s,
>>  
>>                  switch(tlv_subtype) {
>>                  case LLDP_TLV_AA_ELEMENT_SUBTYPE:
>> +                    CHECK_TLV_SIZE(50, "ELEMENT");
>>                      PEEK_BYTES(&msg_auth_digest, sizeof msg_auth_digest);
>>  
>>                      aa_element_dword = PEEK_UINT32;
>> @@ -629,6 +630,7 @@ lldp_decode(struct lldpd *cfg OVS_UNUSED, char *frame, int s,
>>                      break;
>>  
>>                  case LLDP_TLV_AA_ISID_VLAN_ASGNS_SUBTYPE:
>> +                    CHECK_TLV_SIZE(36, "ISID_VLAN_ASGNS");
>>                      PEEK_BYTES(&msg_auth_digest, sizeof msg_auth_digest);
>>  
>>                      /* Subtract off tlv type and length (2Bytes) + OUI (3B) +
>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
>> index e5403519f2..0928bfe540 100644
>> --- a/tests/system-traffic.at
>> +++ b/tests/system-traffic.at
>> @@ -7440,3 +7440,30 @@ OVS_WAIT_UNTIL([cat p2.pcap | grep -E "0x0050: *0000 *0000 *5002 *2000 *b85e *00
>>  
>>  OVS_TRAFFIC_VSWITCHD_STOP
>>  AT_CLEANUP
>> +
>> +AT_SETUP([autoattach - malformed lldp])
>> +OVS_TRAFFIC_VSWITCHD_START()
>> +
>> +ADD_NAMESPACES(at_ns0)
>> +
>> +dnl Set up simple bridge port to receive lldp packets
>> +ADD_VETH(p0, at_ns0, br0, "172.31.1.1/24", "f6:b4:26:aa:5f:00")
>> +
>> +NETNS_DAEMONIZE([at_ns0], [tcpdump -l -n -xx -U -i p0 > p0.pcap], [tcpdump.pid])
>> +sleep 1
>> +
>> +dnl Enable lldp
>> +AT_CHECK([ovs-vsctl set interface ovs-p0 lldp:enable=true])
>> +
>> +dnl Send a malformed lldp packet
>> +NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p0 01 80 c2 00 00 0e f6 b4 26 aa 5f 00 88 cc 02 07 04 f6 b4 26 aa 5f 00 04 03 05 76 32 06 02 00 78 0c 50 44 45 41 44 42 45 45 46 44 45 41 44 42 45 45 46 44 45 41 44 42 45 45 46 44 45 41 44 42 45 45 46 44 45 41 44 42 45 45 46 44 45 41 44 42 45 45 46 44 45 41 44 42 45 45 46 44 45 41 44 42 45 45 46 44 45 41 44 42 45 45 46 44 45 41 44 42 45 45 46 fe 05 00 04 0d 0c 01 00 00 >/dev/null])
>> +
>> +dnl Check the expected lldp packet by looking for the end
>> +OVS_WAIT_UNTIL([cat p0.pcap | grep -E "0x0070: *4546 *fe05 *0004 *0d0c *0100 *00" 2>&1 1>/dev/null])
>> +
>> +AT_CHECK([grep -o "ISID_VLAN_ASGNS TLV too short" ovs-vswitchd.log], [0], [dnl
>> +ISID_VLAN_ASGNS TLV too short
>> +])
>> +
>> +OVS_TRAFFIC_VSWITCHD_STOP(["/|WARN|ISID_VLAN_ASGNS TLV too short received on ovs-p0/d"])
>> +AT_CLEANUP
>
> Do we actually need a system test here?
> It looks like it can be converted to a simple unit test.  E.g.:
>
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index eb4cd1896..41741d324 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -11966,3 +11966,25 @@ AT_CHECK([test 1 = `ovs-ofctl parse-pcap p2-tx.pcap | wc -l`])
>  
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
> +
> +AT_SETUP([ofproto-dpif - malformed lldp])
> +OVS_VSWITCHD_START
> +add_of_ports br0 1
> +
> +AT_CHECK([ovs-ofctl add-flow br0 action=normal])
> +
> +dnl Enable lldp.
> +AT_CHECK([ovs-vsctl set interface p1 lldp:enable=true])
> +
> +dnl Send a malformed lldp packet.
> +packet="0180c200000ef6b426aa5f0088cc020704f6b426aa5f000403057632060200780c"dnl
> +"5044454144424545464445414442454546444541444245454644454144424545464445414"dnl
> +"4424545464445414442454546444541444245454644454144424545464445414442454546"dnl
> +"4445414442454546fe0500040d0c010000"
> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 "$packet"], [0], [stdout])
> +
> +dnl Check that the packet was flagged as invalid.
> +OVS_WAIT_UNTIL([grep -q 'ISID_VLAN_ASGNS TLV too short' ovs-vswitchd.log])
> +
> +OVS_VSWITCHD_STOP(["/|WARN|ISID_VLAN_ASGNS TLV too short received on p1/d"])
> +AT_CLEANUP
> ---
>
> What do you think?

Makes sense to me.  Submitting v2 now.

> Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/lib/lldp/lldp.c b/lib/lldp/lldp.c
index dfeb2a8002..6fdcfef569 100644
--- a/lib/lldp/lldp.c
+++ b/lib/lldp/lldp.c
@@ -583,6 +583,7 @@  lldp_decode(struct lldpd *cfg OVS_UNUSED, char *frame, int s,
 
                 switch(tlv_subtype) {
                 case LLDP_TLV_AA_ELEMENT_SUBTYPE:
+                    CHECK_TLV_SIZE(50, "ELEMENT");
                     PEEK_BYTES(&msg_auth_digest, sizeof msg_auth_digest);
 
                     aa_element_dword = PEEK_UINT32;
@@ -629,6 +630,7 @@  lldp_decode(struct lldpd *cfg OVS_UNUSED, char *frame, int s,
                     break;
 
                 case LLDP_TLV_AA_ISID_VLAN_ASGNS_SUBTYPE:
+                    CHECK_TLV_SIZE(36, "ISID_VLAN_ASGNS");
                     PEEK_BYTES(&msg_auth_digest, sizeof msg_auth_digest);
 
                     /* Subtract off tlv type and length (2Bytes) + OUI (3B) +
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index e5403519f2..0928bfe540 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -7440,3 +7440,30 @@  OVS_WAIT_UNTIL([cat p2.pcap | grep -E "0x0050: *0000 *0000 *5002 *2000 *b85e *00
 
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
+
+AT_SETUP([autoattach - malformed lldp])
+OVS_TRAFFIC_VSWITCHD_START()
+
+ADD_NAMESPACES(at_ns0)
+
+dnl Set up simple bridge port to receive lldp packets
+ADD_VETH(p0, at_ns0, br0, "172.31.1.1/24", "f6:b4:26:aa:5f:00")
+
+NETNS_DAEMONIZE([at_ns0], [tcpdump -l -n -xx -U -i p0 > p0.pcap], [tcpdump.pid])
+sleep 1
+
+dnl Enable lldp
+AT_CHECK([ovs-vsctl set interface ovs-p0 lldp:enable=true])
+
+dnl Send a malformed lldp packet
+NS_CHECK_EXEC([at_ns0], [$PYTHON3 $srcdir/sendpkt.py p0 01 80 c2 00 00 0e f6 b4 26 aa 5f 00 88 cc 02 07 04 f6 b4 26 aa 5f 00 04 03 05 76 32 06 02 00 78 0c 50 44 45 41 44 42 45 45 46 44 45 41 44 42 45 45 46 44 45 41 44 42 45 45 46 44 45 41 44 42 45 45 46 44 45 41 44 42 45 45 46 44 45 41 44 42 45 45 46 44 45 41 44 42 45 45 46 44 45 41 44 42 45 45 46 44 45 41 44 42 45 45 46 44 45 41 44 42 45 45 46 fe 05 00 04 0d 0c 01 00 00 >/dev/null])
+
+dnl Check the expected lldp packet by looking for the end
+OVS_WAIT_UNTIL([cat p0.pcap | grep -E "0x0070: *4546 *fe05 *0004 *0d0c *0100 *00" 2>&1 1>/dev/null])
+
+AT_CHECK([grep -o "ISID_VLAN_ASGNS TLV too short" ovs-vswitchd.log], [0], [dnl
+ISID_VLAN_ASGNS TLV too short
+])
+
+OVS_TRAFFIC_VSWITCHD_STOP(["/|WARN|ISID_VLAN_ASGNS TLV too short received on ovs-p0/d"])
+AT_CLEANUP