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