On 12/19/22 20:27, Aaron Conole wrote: > From: Qian Chen <[email protected]> > > 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 <[email protected]> > Co-authored-by: Aaron Conole <[email protected]> > Signed-off-by: Aaron Conole <[email protected]> > --- > 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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
