Ilya Maximets <i.maxim...@ovn.org> writes:
> On 12/19/22 20:27, Aaron Conole wrote:
>> From: Qian Chen <cq674350...@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 <cq674350...@163.com>
>> Co-authored-by: Aaron Conole <acon...@redhat.com>
>> Signed-off-by: Aaron Conole <acon...@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.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
