Download raw body.
bgpd: rework notification handling
While working on capability enforcement I realized that
parse_notification() had the handling of ERR_OPEN / ERR_OPEN_CAPA wrong.
RFC5492 is fairly clear about ERR_OPEN_CAPA (Unsupported Capability, 7):
As explained in the "Overview of Operations" section, the Unsupported
Capability NOTIFICATION is a way for a BGP speaker to complain that
its peer does not support a required capability without which the
peering cannot proceed. It MUST NOT be used when a BGP speaker
receives a capability that it does not understand; such capabilities
MUST be ignored.
So the logic in parse_notification() is reversed since it tries to turn
off the capability that the other side complains about. So remove all this
humbug.
Now since we are already in there parse the notification exclusivly with
the ibuf api. Clear the last_reason when setting last_rcvd_errcode and
last_rcvd_suberr. Parse the last_reason before calling log_notification()
do this with ibuf functions which simplifes the code a bit (since the full
buffer is memset to 0 the string does not need to be terminated anymore).
In log_notification() add handling for last_reason and also for ERR_OPEN /
ERR_OPEN_CAPA. For ERR_OPEN_CAPA only look at the first capa_code and
report that. I see little reason to dig deeper there (also it seems that
bird and zebra both violate the RFC and send no data with the
notification).
--
:wq Claudio
Index: bgpd.h
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
diff -u -p -r1.487 bgpd.h
--- bgpd.h 18 Mar 2024 10:49:24 -0000 1.487
+++ bgpd.h 20 Mar 2024 10:05:56 -0000
@@ -408,6 +408,17 @@ struct capabilities {
int8_t policy; /* Open Policy, RFC 9234, 2 = enforce */
};
+enum capa_codes {
+ CAPA_NONE = 0,
+ CAPA_MP = 1,
+ CAPA_REFRESH = 2,
+ CAPA_ROLE = 9,
+ CAPA_RESTART = 64,
+ CAPA_AS4BYTE = 65,
+ CAPA_ADD_PATH = 69,
+ CAPA_ENHANCED_RR = 70,
+};
+
/* flags for RFC 4724 - graceful restart */
#define CAPA_GR_PRESENT 0x01
#define CAPA_GR_RESTART 0x02
@@ -1546,6 +1557,7 @@ const char *log_roa(struct roa *);
const char *log_aspa(struct aspa_set *);
const char *log_rtr_error(enum rtr_error);
const char *log_policy(enum role);
+const char *log_capability(uint8_t);
int aspath_asprint(char **, struct ibuf *);
uint32_t aspath_extract(const void *, int);
int aspath_verify(struct ibuf *, int, int);
Index: logmsg.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/logmsg.c,v
diff -u -p -r1.11 logmsg.c
--- logmsg.c 16 Jan 2024 13:15:31 -0000 1.11
+++ logmsg.c 20 Mar 2024 10:15:11 -0000
@@ -133,12 +133,18 @@ log_statechange(struct peer *peer, enum
void
log_notification(const struct peer *peer, uint8_t errcode, uint8_t subcode,
- struct ibuf *data, const char *dir)
+ const struct ibuf *data, const char *dir)
{
+ struct ibuf ibuf;
char *p;
const char *suberrname = NULL;
int uk = 0;
+ if (data != NULL)
+ ibuf_from_ibuf(&ibuf, data);
+ else
+ ibuf_from_buffer(&ibuf, NULL, 0);
+
p = log_fmt_peer(&peer->conf);
switch (errcode) {
case ERR_HEADER:
@@ -154,6 +160,18 @@ log_notification(const struct peer *peer
uk = 1;
else
suberrname = suberr_open_names[subcode];
+ if (errcode == ERR_OPEN && subcode == ERR_OPEN_CAPA) {
+ uint8_t capa_code;
+
+ if (ibuf_get_n8(&ibuf, &capa_code) == -1)
+ break;
+
+ logit(LOG_ERR, "%s: %s notification: %s, %s: %s",
+ p, dir, errnames[errcode], suberrname,
+ log_capability(capa_code));
+ free(p);
+ return;
+ }
break;
case ERR_UPDATE:
if (subcode >= sizeof(suberr_update_names) / sizeof(char *) ||
@@ -168,6 +186,18 @@ log_notification(const struct peer *peer
uk = 1;
else
suberrname = suberr_cease_names[subcode];
+
+ if (subcode == ERR_CEASE_ADMIN_DOWN ||
+ subcode == ERR_CEASE_ADMIN_RESET) {
+ if (peer->stats.last_reason[0] != '\0') {
+ logit(LOG_ERR, "%s: %s notification: %s, %s: "
+ "reason \"%s\"", p, dir,
+ errnames[errcode], suberrname,
+ log_reason(peer->stats.last_reason));
+ free(p);
+ return;
+ }
+ }
break;
case ERR_HOLDTIMEREXPIRED:
if (subcode != 0)
Index: session.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
diff -u -p -r1.464 session.c
--- session.c 20 Mar 2024 09:35:46 -0000 1.464
+++ session.c 20 Mar 2024 10:05:56 -0000
@@ -2479,11 +2479,8 @@ parse_notification(struct peer *peer)
struct ibuf ibuf;
u_char *p;
uint16_t datalen;
- uint8_t errcode;
- uint8_t subcode;
- uint8_t capa_code;
- uint8_t capa_len;
- size_t reason_len;
+ uint8_t errcode, subcode;
+ uint8_t reason_len;
/* just log */
p = peer->rbuf->rptr;
@@ -2495,140 +2492,38 @@ parse_notification(struct peer *peer)
p += MSGSIZE_HEADER; /* header is already checked */
datalen -= MSGSIZE_HEADER;
- memcpy(&errcode, p, sizeof(errcode));
- p += sizeof(errcode);
- datalen -= sizeof(errcode);
-
- memcpy(&subcode, p, sizeof(subcode));
- p += sizeof(subcode);
- datalen -= sizeof(subcode);
-
/* XXX */
ibuf_from_buffer(&ibuf, p, datalen);
- log_notification(peer, errcode, subcode, &ibuf, "received");
+
+ if (ibuf_get_n8(&ibuf, &errcode) == -1 ||
+ ibuf_get_n8(&ibuf, &subcode) == -1) {
+ log_peer_warnx(&peer->conf, "received bad notification");
+ return (-1);
+ }
peer->errcnt++;
peer->stats.last_rcvd_errcode = errcode;
peer->stats.last_rcvd_suberr = subcode;
- if (errcode == ERR_OPEN && subcode == ERR_OPEN_CAPA) {
- if (datalen == 0) { /* zebra likes to send those.. humbug */
- log_peer_warnx(&peer->conf, "received \"unsupported "
- "capability\" notification without data part, "
- "disabling capability announcements altogether");
- session_capa_ann_none(peer);
- }
-
- while (datalen > 0) {
- if (datalen < 2) {
- log_peer_warnx(&peer->conf,
- "parse_notification: "
- "expect len >= 2, len is %u", datalen);
- return (-1);
- }
- memcpy(&capa_code, p, sizeof(capa_code));
- p += sizeof(capa_code);
- datalen -= sizeof(capa_code);
- memcpy(&capa_len, p, sizeof(capa_len));
- p += sizeof(capa_len);
- datalen -= sizeof(capa_len);
- if (datalen < capa_len) {
- log_peer_warnx(&peer->conf,
- "parse_notification: capa_len %u exceeds "
- "remaining msg length %u", capa_len,
- datalen);
- return (-1);
- }
- p += capa_len;
- datalen -= capa_len;
- switch (capa_code) {
- case CAPA_MP:
- memset(peer->capa.ann.mp, 0,
- sizeof(peer->capa.ann.mp));
- log_peer_warnx(&peer->conf,
- "disabling multiprotocol capability");
- break;
- case CAPA_REFRESH:
- peer->capa.ann.refresh = 0;
- log_peer_warnx(&peer->conf,
- "disabling route refresh capability");
- break;
- case CAPA_ROLE:
- if (peer->capa.ann.policy == 1) {
- peer->capa.ann.policy = 0;
- log_peer_warnx(&peer->conf,
- "disabling role capability");
- } else {
- log_peer_warnx(&peer->conf,
- "role capability enforced, "
- "not disabling");
- }
- break;
- case CAPA_RESTART:
- peer->capa.ann.grestart.restart = 0;
- log_peer_warnx(&peer->conf,
- "disabling restart capability");
- break;
- case CAPA_AS4BYTE:
- peer->capa.ann.as4byte = 0;
- log_peer_warnx(&peer->conf,
- "disabling 4-byte AS num capability");
- break;
- case CAPA_ADD_PATH:
- memset(peer->capa.ann.add_path, 0,
- sizeof(peer->capa.ann.add_path));
- log_peer_warnx(&peer->conf,
- "disabling ADD-PATH capability");
- break;
- case CAPA_ENHANCED_RR:
- peer->capa.ann.enhanced_rr = 0;
+ CTASSERT(sizeof(peer->stats.last_reason) > UINT8_MAX);
+ memset(peer->stats.last_reason, 0, sizeof(peer->stats.last_reason));
+ if (errcode == ERR_CEASE &&
+ (subcode == ERR_CEASE_ADMIN_DOWN ||
+ subcode == ERR_CEASE_ADMIN_RESET)) {
+ /* check if shutdown reason is included */
+ if (ibuf_get_n8(&ibuf, &reason_len) != -1 && reason_len != 0) {
+ if (ibuf_get(&ibuf, peer->stats.last_reason,
+ reason_len) == -1)
log_peer_warnx(&peer->conf,
- "disabling enhanced route refresh "
- "capability");
- break;
- default: /* should not happen... */
- log_peer_warnx(&peer->conf, "received "
- "\"unsupported capability\" notification "
- "for unknown capability %u, disabling "
- "capability announcements altogether",
- capa_code);
- session_capa_ann_none(peer);
- break;
- }
+ "received truncated shutdown reason");
}
-
- return (1);
}
+ log_notification(peer, errcode, subcode, &ibuf, "received");
+
if (errcode == ERR_OPEN && subcode == ERR_OPEN_OPT) {
session_capa_ann_none(peer);
return (1);
- }
-
- if (errcode == ERR_CEASE &&
- (subcode == ERR_CEASE_ADMIN_DOWN ||
- subcode == ERR_CEASE_ADMIN_RESET)) {
- if (datalen > 1) {
- reason_len = *p++;
- datalen--;
- if (datalen < reason_len) {
- log_peer_warnx(&peer->conf,
- "received truncated shutdown reason");
- return (0);
- }
- if (reason_len > REASON_LEN - 1) {
- log_peer_warnx(&peer->conf,
- "received overly long shutdown reason");
- return (0);
- }
- memcpy(peer->stats.last_reason, p, reason_len);
- peer->stats.last_reason[reason_len] = '\0';
- log_peer_warnx(&peer->conf,
- "received shutdown reason: \"%s\"",
- log_reason(peer->stats.last_reason));
- p += reason_len;
- datalen -= reason_len;
- }
}
return (0);
Index: session.h
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/session.h,v
diff -u -p -r1.167 session.h
--- session.h 16 Jan 2024 13:15:31 -0000 1.167
+++ session.h 20 Mar 2024 10:05:56 -0000
@@ -106,17 +106,6 @@ enum opt_params {
OPT_PARAM_EXT_LEN=255,
};
-enum capa_codes {
- CAPA_NONE = 0,
- CAPA_MP = 1,
- CAPA_REFRESH = 2,
- CAPA_ROLE = 9,
- CAPA_RESTART = 64,
- CAPA_AS4BYTE = 65,
- CAPA_ADD_PATH = 69,
- CAPA_ENHANCED_RR = 70,
-};
-
struct bgp_msg {
struct ibuf *buf;
enum msg_type type;
@@ -273,7 +262,7 @@ char *log_fmt_peer(const struct peer_con
void log_statechange(struct peer *, enum session_state,
enum session_events);
void log_notification(const struct peer *, uint8_t, uint8_t,
- struct ibuf *, const char *);
+ const struct ibuf *, const char *);
void log_conn_attempt(const struct peer *, struct sockaddr *,
socklen_t);
Index: util.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/util.c,v
diff -u -p -r1.83 util.c
--- util.c 20 Mar 2024 09:35:46 -0000 1.83
+++ util.c 20 Mar 2024 10:05:56 -0000
@@ -309,6 +309,32 @@ log_policy(enum role role)
}
}
+const char *
+log_capability(uint8_t capa)
+{
+ static char buf[20];
+
+ switch (capa) {
+ case CAPA_MP:
+ return "Multiprotocol Extensions";
+ case CAPA_REFRESH:
+ return "Route Refresh";
+ case CAPA_ROLE:
+ return "BGP Role";
+ case CAPA_RESTART:
+ return "Graceful Restart";
+ case CAPA_AS4BYTE:
+ return "4-octet AS number";
+ case CAPA_ADD_PATH:
+ return "ADD-PATH";
+ case CAPA_ENHANCED_RR:
+ return "Enhanced Route Refresh";
+ default:
+ snprintf(buf, sizeof(buf), "unknown %u", capa);
+ return buf;
+ }
+}
+
static const char *
aspath_delim(uint8_t seg_type, int closing)
{
bgpd: rework notification handling