[Bug]: bgpd: trailing IPv4 NLRI without NEXT_HOP is accepted when MP_REACH_NLRI is present

[xinzheliu2000]

Summary

FRR bgpd appears to skip the mandatory NEXT_HOP check for trailing IPv4
unicast NLRI when the same UPDATE also contains MP_REACH_NLRI.

In dynamic testing, a malformed UPDATE containing both MP_REACH_NLRI and
ordinary trailing IPv4 NLRI, but no NEXT_HOP attribute, was accepted by FRR.
Most importantly, the trailing IPv4 prefix 203.0.113.0/24 was installed in
the BGP RIB using the IPv4 nexthop from MP_REACH_NLRI.

I am reporting this as a BGP UPDATE validation / RFC conformance issue. It is
not a claim of memory corruption or unauthenticated remote code execution.

Tested UPDATE

The UPDATE contains:

Withdrawn Routes Length       = 0
Path Attributes:
  ORIGIN                      = IGP
  AS_PATH                     = 65002
  MP_REACH_NLRI:
    AFI                       = IPv4
    SAFI                      = unicast
    nexthop                   = 192.0.2.1
    NLRI                      = 198.51.100.0/24
Trailing IPv4 NLRI            = 203.0.113.0/24
Missing attribute             = NEXT_HOP

For the trailing IPv4 unicast NLRI, NEXT_HOP is a well-known mandatory
attribute. The MP_REACH_NLRI nexthop describes the destinations encoded
inside the MP_REACH_NLRI attribute; it should not automatically replace the
ordinary NEXT_HOP attribute for a non-empty trailing IPv4 NLRI field.

The RFC 4760 exception where NEXT_HOP is optional/ignored is conditioned on
the UPDATE carrying no NLRI other than the NLRI encoded in MP_REACH_NLRI.
This test intentionally violates that condition by also carrying ordinary
trailing IPv4 NLRI.

Expected Behavior

FRR should detect that the UPDATE contains trailing IPv4 NLRI and no NEXT_HOP
attribute.

The trailing IPv4 unicast NLRI should be rejected or treated-as-withdraw due to
missing NEXT_HOP. The MP_REACH_NLRI component may need independent handling,
but the ordinary trailing IPv4 NLRI must not be installed using the
MP_REACH_NLRI nexthop.

I do not expect a session reset here; I expect the malformed trailing IPv4 NLRI
not to be installed.

Actual Behavior

FRR accepted the malformed UPDATE and, importantly, installed the ordinary
trailing IPv4 NLRI 203.0.113.0/24 using the MP_REACH_NLRI nexthop
192.0.2.1:

{
  "prefix": "203.0.113.0/24",
  "pathCount": 1,
  "paths": [
    {
      "aspath": { "string": "65002" },
      "origin": "IGP",
      "valid": true,
      "bestpath": { "overall": true },
      "nexthops": [
        {
          "ip": "192.0.2.1",
          "afi": "ipv4",
          "accessible": true,
          "used": true
        }
      ]
    }
  ]
}

No NOTIFICATION was received for the malformed UPDATE.

Dynamic Verification

This was reproduced against:

FRR bgpd 10.6.1

PoC output:

=== FRR MP_REACH + IPv4 NLRI missing NEXT_HOP PoC ===
[+] TCP connected: ('127.0.0.2', ...) -> ('127.0.0.1', 11881)
[*] Sent BGP OPEN
[*] Sent normal KEEPALIVE
[*] Sending malformed mixed UPDATE:
    MP_REACH_NLRI IPv4 nexthop 192.0.2.1, NLRI 198.51.100.0/24
    trailing IPv4 NLRI 203.0.113.0/24
    missing NEXT_HOP attribute
...
"prefix":"203.0.113.0/24"
"valid":true
"nexthops":[{"ip":"192.0.2.1", ...}]
[!] FRR accepted trailing IPv4 NLRI without NEXT_HOP and used the MP_REACH nexthop
VERDICT: NON_COMPLIANT_REPRODUCED

Source Analysis

The relevant mandatory attribute check is in bgp_attr_check():

/* RFC 2858 makes Next-Hop optional/ignored, if MP_REACH_NLRI is present
 * and NLRI is empty. We can't easily check NLRI empty here though.
 */
if (!bgp_attr_exists(attr, BGP_ATTR_NEXT_HOP) &&
    !bgp_attr_exists(attr, BGP_ATTR_MP_REACH_NLRI))
    type = BGP_ATTR_NEXT_HOP;

This checks only whether MP_REACH_NLRI is present in the UPDATE. It does not
check whether the ordinary trailing IPv4 NLRI field is non-empty. As a result,
an UPDATE with both MP_REACH_NLRI and trailing IPv4 NLRI can bypass the
missing NEXT_HOP check for the trailing IPv4 NLRI.

FRR later parses trailing IPv4 NLRI in bgp_update_receive():

update_len = end - stream_pnt(s);

if (update_len && attribute_len &&
    attr_parse_ret != BGP_ATTR_PARSE_MISSING_MANDATORY) {
    nlris[NLRI_UPDATE].afi = AFI_IP;
    nlris[NLRI_UPDATE].safi = SAFI_UNICAST;
    nlris[NLRI_UPDATE].nlri = stream_pnt(s);
    nlris[NLRI_UPDATE].length = update_len;
    stream_forward_getp(s, update_len);
}

For an IPv4 MP_REACH_NLRI nexthop, FRR may also copy the MP nexthop into
attr->nexthop if the ordinary NEXT_HOP attribute is absent:

if (attr->nexthop.s_addr == INADDR_ANY)
    memcpy(&attr->nexthop.s_addr,
           &attr->mp_nexthop_global_in, IPV4_MAX_BYTELEN);

That explains why the ordinary trailing IPv4 NLRI is installed with the
MP_REACH_NLRI nexthop in the PoC.

BIRD Comparison

For comparison, another implementation checks the actual trailing IPv4 NLRI
length and requires NEXT_HOP whenever that field is non-empty:

if (s->ip_reach_len && !BIT32_TEST(s->attrs_seen, BA_NEXT_HOP))
{ REPORT(NO_MANDATORY, "NEXT_HOP"); goto withdraw; }

The same malformed UPDATE was not installed by BIRD 2.19.0+. It logged:

Missing mandatory NEXT_HOP attribute
Invalid route 203.0.113.0/24 withdrawn
Invalid route 198.51.100.0/24 withdrawn

Impact

The immediate impact is nexthop confusion for malformed mixed UPDATEs. A peer
can send ordinary trailing IPv4 NLRI without the mandatory NEXT_HOP attribute,
while also including MP_REACH_NLRI, and FRR may install the trailing IPv4
route using the MP nexthop.

This could lead to incorrect routing or blackholing if the MP nexthop is not
appropriate for the trailing IPv4 NLRI. The attacker must be an established BGP
peer or an on-path attacker capable of injecting or modifying BGP UPDATEs.

Suggested Fix Direction

Apply the NEXT_HOP mandatory check when the UPDATE contains non-empty ordinary
trailing IPv4 NLRI, even if MP_REACH_NLRI is present.

One possible direction is to move or supplement the missing mandatory
NEXT_HOP check in bgp_update_receive(), where update_len is known. If
update_len != 0 and NEXT_HOP is absent, the trailing IPv4 NLRI should not be
processed as a valid route.