[PATCH net-next 2/4] tools: ynl: reject zero-length attributes instead of looping forever

[Taylor Bates]

The following bug was found in the ynl tooling while parsing
MDBA_ROUTER_PORT entries on a real bridge device with IGMP snooping
enabled, not through fuzzing.

This occurs if the parser walks into a nested attribute containing a
headerless entry (such as MDBA_ROUTER_PORT) and reads the ifindex
parameter as a length, rather than data.

If the parser reads this now misaligned data and parses a field
containing a zero byte, it will run in an infinite loop. It will
continuously append empty attribute objects and consume 100% CPU until
it exhausts the system's memory.

The most straightforward way to trigger this systematically is as
follows:

  1. Create a bridge device with multicast_vlan_snooping enabled.
  2. Add a permanent multicast router port to the bridge that lands
     on ifindex 8. (The mcast_router 2 state ensures that its timer
     is zero.)
  3. Feed NlAttrs() from pyynl the MDBA_ROUTER_PORT netlink payload.

This example creates a bytes object that reproduces the same shape
as the payload:

  msg  = struct.pack('HH', 8, 1) + struct.pack('I', 0xdeadbeef)
  msg += struct.pack('HH', 0, 2)
  NlAttrs(msg)

Fixes: e4b48ed460d3 ("tools: ynl: add a completely generic client")
Signed-off-by: Taylor Bates <tmbates12@xxxxxxxxx>
---
Full reproducer, run under "unshare -Urn" so the namespace starts with
only lo and ifindex allocation restarts from 1:

  ip link add br0 type bridge vlan_filtering 1 mcast_snooping 1 \
      mcast_vlan_snooping 1
  ip link set br0 up
  n=0
  while :; do
      n=$((n + 1))
      ip link add d$n type dummy
      idx=$(ip -o link show d$n | cut -d: -f1 | tr -d ' ')
      [ $((idx & 0xffff)) = 8 ] && break
      [ $n -gt 200 ] && { echo "gave up"; exit 1; }
  done
  ip link set d$n master br0
  ip link set d$n up
  bridge vlan add dev d$n vid 10
  bridge vlan set dev d$n vid 10 mcast_router 2
  bridge vlan global set dev br0 vid 10 mcast_snooping 1

The BRIDGE_VLANDB_GOPTS_MCAST_ROUTER_PORTS payload the kernel then sends:

  34 00 02 00   MDBA_ROUTER, len 52
  30 00 01 00   MDBA_ROUTER_PORT, len 48
  08 00 00 00   bare ifindex 8, written by nla_put_nohdr()
  08 00 01 00   MDBA_ROUTER_PATTR_TIMER, len 8
  00 00 00 00   timer value, 0 for a permanent router

Read as a header, the ifindex claims eight bytes and consumes the
MDBA_ROUTER_PATTR_TIMER header along with itself. The walk then lands on
the timer value, four zero bytes, and nla_len is 0. full_len is 0 too,
so the offset never advances.
---
 tools/net/ynl/pyynl/lib/ynl.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/net/ynl/pyynl/lib/ynl.py b/tools/net/ynl/pyynl/lib/ynl.py
index 8682bf588e1f..375a15d83a34 100644
--- a/tools/net/ynl/pyynl/lib/ynl.py
+++ b/tools/net/ynl/pyynl/lib/ynl.py
@@ -317,6 +317,10 @@ class NlAttrs:
 
         while offset < len(msg):
             attr = NlAttr(msg, offset)
+            if attr.full_len < 4:
+                raise YnlException(
+                    f'Malformed attribute at offset {offset}: '
+                    f'length {attr.payload_len} is shorter than the header')
             offset += attr.full_len
             self.attrs.append(attr)
 

-- 
2.55.0