From beb044e94766735d8610d1d011f04e1adb25425d Mon Sep 17 00:00:00 2001 From: claudio Date: Fri, 22 Mar 2024 07:19:28 +0000 Subject: [PATCH] Rework parse_notification() to use the ibuf API for everything. While there fix the RFC5492 handling of ERR_OPEN_CAPA (the current code has the logic inversed). ERR_OPEN_CAPA is there to signal that a needed capability is missing in our OPEN message. Just add the handling of ERR_OPEN_CAPA to log_notification(). Also rework the handling of the shutdown reason and move the printing into log_notification(). OK tb@ --- usr.sbin/bgpd/bgpd.h | 14 +++- usr.sbin/bgpd/logmsg.c | 34 +++++++++- usr.sbin/bgpd/session.c | 147 ++++++---------------------------------- usr.sbin/bgpd/session.h | 15 +--- usr.sbin/bgpd/util.c | 28 +++++++- 5 files changed, 95 insertions(+), 143 deletions(-) diff --git a/usr.sbin/bgpd/bgpd.h b/usr.sbin/bgpd/bgpd.h index 6e72746cd48..212b10ff223 100644 --- a/usr.sbin/bgpd/bgpd.h +++ b/usr.sbin/bgpd/bgpd.h @@ -1,4 +1,4 @@ -/* $OpenBSD: bgpd.h,v 1.487 2024/03/18 10:49:24 claudio Exp $ */ +/* $OpenBSD: bgpd.h,v 1.488 2024/03/22 07:19:28 claudio Exp $ */ /* * Copyright (c) 2003, 2004 Henning Brauer @@ -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); diff --git a/usr.sbin/bgpd/logmsg.c b/usr.sbin/bgpd/logmsg.c index dfa89f2393b..359f94e7362 100644 --- a/usr.sbin/bgpd/logmsg.c +++ b/usr.sbin/bgpd/logmsg.c @@ -1,4 +1,4 @@ -/* $OpenBSD: logmsg.c,v 1.11 2024/01/16 13:15:31 claudio Exp $ */ +/* $OpenBSD: logmsg.c,v 1.12 2024/03/22 07:19:28 claudio Exp $ */ /* * Copyright (c) 2003, 2004 Henning Brauer @@ -133,12 +133,18 @@ log_statechange(struct peer *peer, enum session_state nstate, 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, uint8_t errcode, uint8_t subcode, 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, uint8_t errcode, uint8_t subcode, 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) diff --git a/usr.sbin/bgpd/session.c b/usr.sbin/bgpd/session.c index 3aeec7767df..0d8ac4e15ed 100644 --- a/usr.sbin/bgpd/session.c +++ b/usr.sbin/bgpd/session.c @@ -1,4 +1,4 @@ -/* $OpenBSD: session.c,v 1.464 2024/03/20 09:35:46 claudio Exp $ */ +/* $OpenBSD: session.c,v 1.465 2024/03/22 07:19:28 claudio Exp $ */ /* * Copyright (c) 2003, 2004, 2005 Henning Brauer @@ -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,142 +2492,40 @@ 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); } diff --git a/usr.sbin/bgpd/session.h b/usr.sbin/bgpd/session.h index 13cbd92d747..1f459b79dcf 100644 --- a/usr.sbin/bgpd/session.h +++ b/usr.sbin/bgpd/session.h @@ -1,4 +1,4 @@ -/* $OpenBSD: session.h,v 1.167 2024/01/16 13:15:31 claudio Exp $ */ +/* $OpenBSD: session.h,v 1.168 2024/03/22 07:19:28 claudio Exp $ */ /* * Copyright (c) 2003, 2004 Henning Brauer @@ -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_config *); 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); diff --git a/usr.sbin/bgpd/util.c b/usr.sbin/bgpd/util.c index db4f5e75765..cdbcc576d2b 100644 --- a/usr.sbin/bgpd/util.c +++ b/usr.sbin/bgpd/util.c @@ -1,4 +1,4 @@ -/* $OpenBSD: util.c,v 1.83 2024/03/20 09:35:46 claudio Exp $ */ +/* $OpenBSD: util.c,v 1.84 2024/03/22 07:19:28 claudio Exp $ */ /* * Copyright (c) 2006 Claudio Jeker @@ -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) { -- 2.20.1