On Thu, Apr 22, 2021 at 02:41:20PM +0800, cjt wrote:
> Hello.
> I have found two inconsistency about bgp error handling.
> First:
> I found that in RFC 4271 for the next hop syntactically incorrect, need to
> set invalid next hop. RFC Document fragmen as follow:
>
>
> If the NEXT_HOP attribute field is syntactically incorrect, then the Error
> Subcode MUST be set to Invalid NEXT_HOP Attribute. The Data field MUST
> contain the incorrect attribute (type, length, and value). Syntactic
> correctness means that the NEXT_HOP attribute represents a valid IP host
> address.
>
>
> We find that in the code of the openbgpd6.8, it's error code is
> ERR_UPD_NETWORK.
>
>
> /*
> * Check if the nexthop is a valid IP address. We consider
> * multicast and experimental addresses as invalid.
> */
> tmp32 = ntohl(nexthop.v4.s_addr);
> if (IN_MULTICAST(tmp32) || IN_BADCLASS(tmp32)) {
> rde_update_err(peer, ERR_UPDATE, ERR_UPD_NETWORK,
> op, len);
> return (-1);
> }
>
>
> Second:
> I found that in RFC 6286 for bgp identifer, it has two check. One is to zero,
> the other is to local bgp id. RFC Document fragmen as follow:
>
>
> For a BGP speaker that supports the AS-wide Unique BGP Identifier, the OPEN
> message error handling related to the BGP Identifier is modified as follows:
> If the BGP Identifier field of the OPEN message is zero, or if it is the
> same as the BGP Identifier of the local BGP speaker and the message is from
> an internal peer, then the Error Subcode is set to "Bad BGP Identifier".
>
>
> In the code of the openbgpd6.8, there missing the bgp id is the same check.
> （If openbgpd does not support RFC 6286, you can ignore it）
>
>
> /* check bgpid for validity - just disallow 0 */
> if (ntohl(bgpid) == 0) {
> log_peer_warnx(&peer->conf, "peer BGPID %u unacceptable",
> ntohl(bgpid));
> session_notification(peer, ERR_OPEN, ERR_OPEN_BGPID,
> NULL, 0);
> change_state(peer, STATE_IDLE, EVNT_RCVD_OPEN);
> return (-1);
> }
>
Hi,
Thanks for the report. The following diff should address both issues.
Please double check. I moved the bgpid conflict check after template
sessions have been properly setup because until then the value of
peer->conf.ebgp is not yet valid.
--
:wq Claudio
Index: rde.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
retrieving revision 1.517
diff -u -p -r1.517 rde.c
--- rde.c 16 Apr 2021 06:20:29 -0000 1.517
+++ rde.c 22 Apr 2021 08:12:04 -0000
@@ -1710,7 +1710,7 @@ bad_flags:
*/
tmp32 = ntohl(nexthop.v4.s_addr);
if (IN_MULTICAST(tmp32) || IN_BADCLASS(tmp32)) {
- rde_update_err(peer, ERR_UPDATE, ERR_UPD_NETWORK,
+ rde_update_err(peer, ERR_UPDATE, ERR_UPD_NEXTHOP,
op, len);
return (-1);
}
Index: session.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
retrieving revision 1.411
diff -u -p -r1.411 session.c
--- session.c 16 Feb 2021 08:29:16 -0000 1.411
+++ session.c 22 Apr 2021 08:12:26 -0000
@@ -2179,6 +2179,16 @@ parse_open(struct peer *peer)
return (-1);
}
+ /* on iBGP sessions check for bgpid collision */
+ if (!peer->conf.ebgp && peer->remote_bgpid == conf->bgpid) {
+ log_peer_warnx(&peer->conf, "peer BGPID %u conflicts with ours",
+ ntohl(bgpid));
+ session_notification(peer, ERR_OPEN, ERR_OPEN_BGPID,
+ NULL, 0);
+ change_state(peer, STATE_IDLE, EVNT_RCVD_OPEN);
+ return (-1);
+ }
+
if (capa_neg_calc(peer) == -1) {
log_peer_warnx(&peer->conf,
"capability negotiation calculation failed");
