bgpd: Remove the double-pass parsing of NLRIs
* bgpd parses NLRIs twice, a first pass "sanity check" and then a second pass
that changes actual state. For most AFI/SAFIs this is done by
bgp_nlri_sanity_check and bgp_nlri_parse, which are almost identical.
As the required action on a syntactic error in an NLRI is to NOTIFY and
shut down the session, it should be acceptable to just do a one pass
parse. There is no need to atomically handle the NLRIs.
* bgp_route.h: (bgp_nlri_sanity_check) Delete
* bgp_route.c: (bgp_nlri_parse) Make the prefixlen size check more general
and don't hard-code AFI/SAFI details, e.g. use prefix_blen library function.
Add error logs consistent with bgp_nlri_sanity_check as much as possible.
Add a "defense in depth" type check of the prefixlen against the sizeof
the (struct prefix) storage - ala bgp_nlri_parse_vpn.
Update standards text from draft RFC4271 to the actual RFC4271 text.
Extend the semantic consistency test of IPv6. E.g. it should skip mcast
NLRIs for unicast safi as v4 does.
* bgp_mplsvpn.{c,h}: Delete bgp_nlri_sanity_check_vpn and make
bgp_nlri_parse_vpn_body the bgp_nlri_parse_vpn function again.
(bgp_nlri_parse_vpn) Remove the notifies. The sanity checks were
responsible for this, but bgp_update_receive handles sending NOTIFY
generically for bgp_nlri_parse.
* bgp_attr.c: (bgp_mp_reach_parse,bgp_mp_unreach_parse) Delete sanity check.
NLRI parsing done after attr parsing by bgp_update_receive.
Arising out of discussions on the need for two-pass NLRI parse with:
Lou Berger <lberger@labn.net>
Donald Sharp <sharpd@cumulusnetworks.com>
diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c
index 98571da..d74e0ef 100644
--- a/bgpd/bgp_attr.c
+++ b/bgpd/bgp_attr.c
@@ -1602,7 +1602,6 @@
safi_t safi;
bgp_size_t nlri_len;
size_t start;
- int ret;
struct stream *s;
struct peer *const peer = args->peer;
struct attr *const attr = args->attr;
@@ -1731,14 +1730,6 @@
mp_update->nlri = stream_pnt (s);
mp_update->length = nlri_len;
- ret = bgp_nlri_sanity_check (peer, mp_update);
- if (ret < 0)
- {
- zlog_info ("%s: (%s) NLRI doesn't pass sanity check",
- __func__, peer->host);
- return BGP_ATTR_PARSE_ERROR_NOTIFYPLS;
- }
-
stream_forward_getp (s, nlri_len);
attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_MP_REACH_NLRI);
@@ -1776,9 +1767,6 @@
mp_withdraw->nlri = stream_pnt (s);
mp_withdraw->length = withdraw_len;
- if (bgp_nlri_sanity_check (peer, mp_withdraw) < 0)
- return BGP_ATTR_PARSE_ERROR_NOTIFYPLS;
-
stream_forward_getp (s, withdraw_len);
attr->flag |= ATTR_FLAG_BIT (BGP_ATTR_MP_UNREACH_NLRI);
diff --git a/bgpd/bgp_mplsvpn.c b/bgpd/bgp_mplsvpn.c
index 83bb6ca..08a4272 100644
--- a/bgpd/bgp_mplsvpn.c
+++ b/bgpd/bgp_mplsvpn.c
@@ -93,9 +93,9 @@
rd_ip->val |= (u_int16_t) *pnt;
}
-static int
-bgp_nlri_parse_vpn_body (struct peer *peer, struct attr *attr,
- struct bgp_nlri *packet, bool update)
+int
+bgp_nlri_parse_vpn (struct peer *peer, struct attr *attr,
+ struct bgp_nlri *packet)
{
u_char *pnt;
u_char *lim;
@@ -137,8 +137,6 @@
"%s [Error] Update packet error / VPNv4"
" (prefix length %d less than VPNv4 min length)",
peer->host, prefixlen);
- bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR,
- BGP_NOTIFY_UPDATE_OPT_ATTR_ERR);
return -1;
}
if ((pnt + psize) > lim)
@@ -148,8 +146,6 @@
" (psize %u exceeds packet size (%u)",
peer->host,
prefixlen, (uint)(lim-pnt));
- bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR,
- BGP_NOTIFY_UPDATE_OPT_ATTR_ERR);
return -1;
}
@@ -161,8 +157,6 @@
" (psize %u exceeds storage size (%zu)",
peer->host,
prefixlen - VPN_PREFIXLEN_MIN_BYTES*8, sizeof(p.u));
- bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR,
- BGP_NOTIFY_UPDATE_OPT_ATTR_ERR);
return -1;
}
@@ -175,8 +169,6 @@
peer->host,
prefixlen - VPN_PREFIXLEN_MIN_BYTES*8,
p.family, prefix_blen (&p));
- bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR,
- BGP_NOTIFY_UPDATE_OPT_ATTR_ERR);
return -1;
}
@@ -212,15 +204,12 @@
memcpy (&p.u.prefix, pnt + VPN_PREFIXLEN_MIN_BYTES,
psize - VPN_PREFIXLEN_MIN_BYTES);
- if (update)
- {
- if (attr)
- bgp_update (peer, &p, attr, packet->afi, SAFI_MPLS_VPN,
- ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL, &prd, tagpnt, 0);
- else
- bgp_withdraw (peer, &p, attr, packet->afi, SAFI_MPLS_VPN,
- ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL, &prd, tagpnt);
- }
+ if (attr)
+ bgp_update (peer, &p, attr, packet->afi, SAFI_MPLS_VPN,
+ ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL, &prd, tagpnt, 0);
+ else
+ bgp_withdraw (peer, &p, attr, packet->afi, SAFI_MPLS_VPN,
+ ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL, &prd, tagpnt);
}
/* Packet length consistency check. */
if (pnt != lim)
@@ -229,8 +218,6 @@
"%s [Error] Update packet error / VPNv4"
" (%zu data remaining after parsing)",
peer->host, lim - pnt);
- bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR,
- BGP_NOTIFY_UPDATE_OPT_ATTR_ERR);
return -1;
}
@@ -239,19 +226,6 @@
}
int
-bgp_nlri_sanity_check_vpn (struct peer *peer, struct bgp_nlri *nlri)
-{
- return bgp_nlri_parse_vpn_body (peer, NULL, nlri, false);
-}
-
-int
-bgp_nlri_parse_vpn (struct peer *peer, struct attr *attr,
- struct bgp_nlri *packet)
-{
- return bgp_nlri_parse_vpn_body (peer, attr, packet, true);
-}
-
-int
str2prefix_rd (const char *str, struct prefix_rd *prd)
{
int ret; /* ret of called functions */
diff --git a/bgpd/bgp_mplsvpn.h b/bgpd/bgp_mplsvpn.h
index b89b6d8..3299b9c 100644
--- a/bgpd/bgp_mplsvpn.h
+++ b/bgpd/bgp_mplsvpn.h
@@ -43,7 +43,6 @@
extern void bgp_mplsvpn_init (void);
extern int bgp_nlri_parse_vpn (struct peer *, struct attr *, struct bgp_nlri *);
-extern int bgp_nlri_sanity_check_vpn (struct peer *, struct bgp_nlri *);
extern u_int32_t decode_label (u_char *);
extern int str2prefix_rd (const char *, struct prefix_rd *);
extern int str2tag (const char *, u_char *);
diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c
index 628c333..740b0f1 100644
--- a/bgpd/bgp_packet.c
+++ b/bgpd/bgp_packet.c
@@ -1686,13 +1686,6 @@
nlris[NLRI_WITHDRAW].nlri = stream_pnt (s);
nlris[NLRI_WITHDRAW].length = withdraw_len;
- if (bgp_nlri_sanity_check (peer, &nlris[NLRI_WITHDRAW]) < 0)
- {
- bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR,
- BGP_NOTIFY_UPDATE_INVAL_NETWORK);
- return -1;
- }
-
if (BGP_DEBUG (packet, PACKET_RECV))
zlog_debug ("%s [Update:RECV] Unfeasible NLRI received", peer->host);
@@ -1782,17 +1775,6 @@
nlris[NLRI_UPDATE].nlri = stream_pnt (s);
nlris[NLRI_UPDATE].length = update_len;
- /* Check NLRI packet format and prefix length. */
- ret = bgp_nlri_sanity_check (peer, &nlris[NLRI_UPDATE]);
- if (ret < 0)
- {
- bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR,
- BGP_NOTIFY_UPDATE_INVAL_NETWORK);
- bgp_attr_unintern_sub (&attr);
- bgp_attr_flush (&attr);
- return -1;
- }
-
stream_forward_getp (s, update_len);
}
diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c
index b581fd9..2728b10 100644
--- a/bgpd/bgp_route.c
+++ b/bgpd/bgp_route.c
@@ -3242,6 +3242,9 @@
pnt = packet->nlri;
lim = pnt + packet->length;
+ /* RFC4771 6.3 The NLRI field in the UPDATE message is checked for
+ syntactic validity. If the field is syntactically incorrect,
+ then the Error Subcode is set to Invalid Network Field. */
for (; pnt < lim; pnt += psize)
{
/* Clear prefix structure. */
@@ -3249,20 +3252,41 @@
/* Fetch prefix length. */
p.prefixlen = *pnt++;
+ /* afi/safi validity already verified by caller, bgp_update_receive */
p.family = afi2family (packet->afi);
- /* Already checked in nlri_sanity_check(). We do double check
- here. */
- if ((packet->afi == AFI_IP && p.prefixlen > 32)
- || (packet->afi == AFI_IP6 && p.prefixlen > 128))
- return -1;
-
+ /* Prefix length check. */
+ if (p.prefixlen > prefix_blen (&p) * 8)
+ {
+ plog_err (peer->log,
+ "%s [Error] Update packet error"
+ " (wrong prefix length %u for afi %u)",
+ peer->host, p.prefixlen, packet->afi);
+ return -1;
+ }
+
/* Packet size overflow check. */
psize = PSIZE (p.prefixlen);
/* When packet overflow occur return immediately. */
if (pnt + psize > lim)
- return -1;
+ {
+ plog_err (peer->log,
+ "%s [Error] Update packet error"
+ " (prefix length %u overflows packet)",
+ peer->host, p.prefixlen);
+ return -1;
+ }
+
+ /* Defensive coding, double-check the psize fits in a struct prefix */
+ if (psize > (ssize_t) sizeof(p.u))
+ {
+ plog_err (peer->log,
+ "%s [Error] Update packet error"
+ " (prefix length %u too large for prefix storage %zu!?!!",
+ peer->host, p.prefixlen, sizeof(p.u));
+ return -1;
+ }
/* Fetch prefix from NLRI packet. */
memcpy (&p.u.prefix, pnt, psize);
@@ -3273,17 +3297,16 @@
if (IN_CLASSD (ntohl (p.u.prefix4.s_addr)))
{
/*
- * From draft-ietf-idr-bgp4-22, Section 6.3:
- * If a BGP router receives an UPDATE message with a
- * semantically incorrect NLRI field, in which a prefix is
- * semantically incorrect (eg. an unexpected multicast IP
- * address), it should ignore the prefix.
+ * From RFC4271 Section 6.3:
+ *
+ * If a prefix in the NLRI field is semantically incorrect
+ * (e.g., an unexpected multicast IP address), an error SHOULD
+ * be logged locally, and the prefix SHOULD be ignored.
*/
zlog (peer->log, LOG_ERR,
- "IPv4 unicast NLRI is multicast address %s",
- inet_ntoa (p.u.prefix4));
-
- return -1;
+ "%s: IPv4 unicast NLRI is multicast address %s, ignoring",
+ peer->host, inet_ntoa (p.u.prefix4));
+ continue;
}
}
@@ -3294,13 +3317,23 @@
{
char buf[BUFSIZ];
- zlog (peer->log, LOG_WARNING,
- "IPv6 link-local NLRI received %s ignore this NLRI",
+ zlog (peer->log, LOG_ERR,
+ "%s: IPv6 unicast NLRI is link-local address %s, ignoring",
+ peer->host,
inet_ntop (AF_INET6, &p.u.prefix6, buf, BUFSIZ));
-
continue;
}
- }
+ if (IN6_IS_ADDR_MULTICAST (&p.u.prefix6))
+ {
+ char buf[BUFSIZ];
+
+ zlog (peer->log, LOG_ERR,
+ "%s: IPv6 unicast NLRI is multicast address %s, ignoring",
+ peer->host,
+ inet_ntop (AF_INET6, &p.u.prefix6, buf, BUFSIZ));
+ continue;
+ }
+ }
/* Normal process. */
if (attr)
@@ -3318,99 +3351,17 @@
/* Packet length consistency check. */
if (pnt != lim)
- return -1;
-
- return 0;
-}
-
-static int
-bgp_nlri_sanity_check_ip (struct peer *peer, struct bgp_nlri *nlri)
-{
- u_char *end;
- u_char prefixlen;
- int psize;
- u_char *pnt = nlri->nlri;
- afi_t afi = nlri->afi;
- safi_t safi = nlri->safi;
- end = pnt + nlri->length;
-
- /* RFC1771 6.3 The NLRI field in the UPDATE message is checked for
- syntactic validity. If the field is syntactically incorrect,
- then the Error Subcode is set to Invalid Network Field. */
-
- while (pnt < end)
- {
- int badlength;
- prefixlen = *pnt++;
-
- /* Prefix length check. */
- badlength = 0;
- if (safi == SAFI_ENCAP) {
- if (prefixlen > 128)
- badlength = 1;
- } else {
- if ((afi == AFI_IP && prefixlen > 32) ||
- (afi == AFI_IP6 && prefixlen > 128)) {
-
- badlength = 1;
- }
- }
- if (badlength)
- {
- plog_err (peer->log,
- "%s [Error] Update packet error (wrong prefix length %d)",
- peer->host, prefixlen);
- bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR,
- BGP_NOTIFY_UPDATE_INVAL_NETWORK);
- return -1;
- }
-
- /* Packet size overflow check. */
- psize = PSIZE (prefixlen);
-
- if (pnt + psize > end)
- {
- plog_err (peer->log,
- "%s [Error] Update packet error"
- " (prefix data overflow prefix size is %d)",
- peer->host, psize);
- bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR,
- BGP_NOTIFY_UPDATE_INVAL_NETWORK);
- return -1;
- }
-
- pnt += psize;
- }
-
- /* Packet length consistency check. */
- if (pnt != end)
{
plog_err (peer->log,
- "%s [Error] Update packet error"
- " (prefix length mismatch with total length)",
- peer->host);
- bgp_notify_send (peer, BGP_NOTIFY_UPDATE_ERR,
- BGP_NOTIFY_UPDATE_INVAL_NETWORK);
+ "%s [Error] Update packet error"
+ " (prefix length mismatch with total length)",
+ peer->host);
return -1;
}
+
return 0;
}
-int
-bgp_nlri_sanity_check (struct peer *peer, struct bgp_nlri *nlri)
-{
- switch (nlri->safi)
- {
- case SAFI_MPLS_LABELED_VPN:
- case SAFI_MPLS_VPN:
- return bgp_nlri_sanity_check_vpn (peer, nlri);
- case SAFI_UNICAST:
- case SAFI_MULTICAST:
- return bgp_nlri_sanity_check_ip (peer, nlri);
- default: return -1;
- }
-}
-
static struct bgp_static *
bgp_static_new (void)
{
diff --git a/bgpd/bgp_route.h b/bgpd/bgp_route.h
index 8064598..c803759 100644
--- a/bgpd/bgp_route.h
+++ b/bgpd/bgp_route.h
@@ -196,7 +196,6 @@
extern void bgp_info_set_flag (struct bgp_node *, struct bgp_info *, u_int32_t);
extern void bgp_info_unset_flag (struct bgp_node *, struct bgp_info *, u_int32_t);
-extern int bgp_nlri_sanity_check (struct peer *, struct bgp_nlri *);
extern int bgp_nlri_parse_ip (struct peer *, struct attr *, struct bgp_nlri *);
extern int bgp_maximum_prefix_overflow (struct peer *, afi_t, safi_t, int);