From 5149f89075431ea5d8d148a02b8e41b0fe8ecb65 Mon Sep 17 00:00:00 2001 From: claudio Date: Mon, 20 May 2024 10:01:52 +0000 Subject: [PATCH] Convert more of the session engine parsers to the new ibuf API This converts OPEN and it capability parser and RREFRESH and with that all packet parser are kind of converted. There is still parse_header() and the general packet handling that needs some work now so that all the ugly ibuf_from_buffer() can be dropped. OK tb@ --- usr.sbin/bgpd/session.c | 342 +++++++++++++++++++--------------------- 1 file changed, 159 insertions(+), 183 deletions(-) diff --git a/usr.sbin/bgpd/session.c b/usr.sbin/bgpd/session.c index 6ab57978f1e..43c61adb087 100644 --- a/usr.sbin/bgpd/session.c +++ b/usr.sbin/bgpd/session.c @@ -1,4 +1,4 @@ -/* $OpenBSD: session.c,v 1.476 2024/05/16 09:38:21 claudio Exp $ */ +/* $OpenBSD: session.c,v 1.477 2024/05/20 10:01:52 claudio Exp $ */ /* * Copyright (c) 2003, 2004, 2005 Henning Brauer @@ -87,7 +87,7 @@ int parse_open(struct peer *); int parse_update(struct peer *); int parse_rrefresh(struct peer *); void parse_notification(struct peer *); -int parse_capabilities(struct peer *, u_char *, uint16_t, uint32_t *); +int parse_capabilities(struct peer *, struct ibuf *, uint32_t *); int capa_neg_calc(struct peer *); void session_dispatch_imsg(struct imsgbuf *, int, u_int *); void session_up(struct peer *); @@ -115,6 +115,11 @@ u_int peer_cnt; struct mrt_head mrthead; time_t pauseaccept; +static const uint8_t marker[MSGSIZE_HEADER_MARKER] = { + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, +}; + static inline int peer_compare(const struct peer *a, const struct peer *b) { @@ -1380,13 +1385,10 @@ session_capa_add_afi(struct ibuf *b, uint8_t aid, uint8_t flags) struct bgp_msg * session_newmsg(enum msg_type msgtype, uint16_t len) { - u_char marker[MSGSIZE_HEADER_MARKER]; struct bgp_msg *msg; struct ibuf *buf; int errs = 0; - memset(marker, 0xff, sizeof(marker)); - if ((buf = ibuf_open(len)) == NULL) return (NULL); @@ -2063,9 +2065,6 @@ parse_header(struct peer *peer, u_char *data, uint16_t *len, uint8_t *type) { u_char *p; uint16_t olen; - static const uint8_t marker[MSGSIZE_HEADER_MARKER] = { 0xff, 0xff, - 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, - 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff }; /* caller MUST make sure we are getting 19 bytes! */ p = data; @@ -2157,13 +2156,13 @@ parse_header(struct peer *peer, u_char *data, uint16_t *len, uint8_t *type) int parse_open(struct peer *peer) { - u_char *p, *op_val; + struct ibuf ibuf; + u_char *p; uint8_t version, rversion; uint16_t short_as, msglen; - uint16_t holdtime, oholdtime, myholdtime; + uint16_t holdtime, myholdtime; uint32_t as, bgpid; - uint16_t optparamlen, extlen, plen, op_len; - uint8_t op_type; + uint8_t optparamlen; p = peer->rbuf->rptr; p += MSGSIZE_HEADER_MARKER; @@ -2172,9 +2171,17 @@ parse_open(struct peer *peer) p = peer->rbuf->rptr; p += MSGSIZE_HEADER; /* header is already checked */ + msglen -= MSGSIZE_HEADER; - memcpy(&version, p, sizeof(version)); - p += sizeof(version); + /* XXX */ + ibuf_from_buffer(&ibuf, p, msglen); + + if (ibuf_get_n8(&ibuf, &version) == -1 || + ibuf_get_n16(&ibuf, &short_as) == -1 || + ibuf_get_n16(&ibuf, &holdtime) == -1 || + ibuf_get_n32(&ibuf, &bgpid) == -1 || + ibuf_get_n8(&ibuf, &optparamlen) == -1) + goto bad_len; if (version != BGP_VERSION) { log_peer_warnx(&peer->conf, @@ -2189,9 +2196,7 @@ parse_open(struct peer *peer) return (-1); } - memcpy(&short_as, p, sizeof(short_as)); - p += sizeof(short_as); - as = peer->short_as = ntohs(short_as); + as = peer->short_as = short_as; if (as == 0) { log_peer_warnx(&peer->conf, "peer requests unacceptable AS %u", as); @@ -2200,10 +2205,6 @@ parse_open(struct peer *peer) return (-1); } - memcpy(&oholdtime, p, sizeof(oholdtime)); - p += sizeof(oholdtime); - - holdtime = ntohs(oholdtime); if (holdtime && holdtime < peer->conf.min_holdtime) { log_peer_warnx(&peer->conf, "peer requests unacceptable holdtime %u", holdtime); @@ -2220,103 +2221,98 @@ parse_open(struct peer *peer) else peer->holdtime = myholdtime; - memcpy(&bgpid, p, sizeof(bgpid)); - p += sizeof(bgpid); - /* check bgpid for validity - just disallow 0 */ - if (ntohl(bgpid) == 0) { - log_peer_warnx(&peer->conf, "peer BGPID %u unacceptable", - ntohl(bgpid)); + if (bgpid == 0) { + log_peer_warnx(&peer->conf, "peer BGPID 0 unacceptable"); session_notification(peer, ERR_OPEN, ERR_OPEN_BGPID, NULL); change_state(peer, STATE_IDLE, EVNT_RCVD_OPEN); return (-1); } - peer->remote_bgpid = bgpid; + /* strore remote_bgpid in network byte order */ + peer->remote_bgpid = htonl(bgpid); - extlen = 0; - optparamlen = *p++; + if (optparamlen != 0) { + struct ibuf oparams, op; + uint8_t ext_type, op_type; + uint16_t ext_len, op_len; - if (optparamlen == 0) { - if (msglen != MSGSIZE_OPEN_MIN) { -bad_len: - log_peer_warnx(&peer->conf, - "corrupt OPEN message received: length mismatch"); - session_notification(peer, ERR_OPEN, 0, NULL); - change_state(peer, STATE_IDLE, EVNT_RCVD_OPEN); - return (-1); - } - } else { - if (msglen < MSGSIZE_OPEN_MIN + 1) - goto bad_len; + ibuf_from_ibuf(&oparams, &ibuf); - op_type = *p; - if (op_type == OPT_PARAM_EXT_LEN) { - p++; - memcpy(&optparamlen, p, sizeof(optparamlen)); - optparamlen = ntohs(optparamlen); - p += sizeof(optparamlen); - extlen = 1; + /* check for RFC9072 encoding */ + if (ibuf_get_n8(&oparams, &ext_type) == -1) + goto bad_len; + if (ext_type == OPT_PARAM_EXT_LEN) { + if (ibuf_get_n16(&oparams, &ext_len) == -1) + goto bad_len; + /* skip RFC9072 header */ + if (ibuf_skip(&ibuf, 3) == -1) + goto bad_len; + } else { + ext_len = optparamlen; + ibuf_rewind(&oparams); } - /* RFC9020 encoding has 3 extra bytes */ - if (optparamlen + 3 * extlen != msglen - MSGSIZE_OPEN_MIN) + if (ibuf_truncate(&oparams, ext_len) == -1 || + ibuf_skip(&ibuf, ext_len) == -1) goto bad_len; - } - plen = optparamlen; - while (plen > 0) { - if (plen < 2 + extlen) - goto bad_len; + while (ibuf_size(&oparams) > 0) { + if (ibuf_get_n8(&oparams, &op_type) == -1) + goto bad_len; - memcpy(&op_type, p, sizeof(op_type)); - p += sizeof(op_type); - plen -= sizeof(op_type); - if (!extlen) { - op_len = *p++; - plen--; - } else { - memcpy(&op_len, p, sizeof(op_len)); - op_len = ntohs(op_len); - p += sizeof(op_len); - plen -= sizeof(op_len); - } - if (op_len > 0) { - if (plen < op_len) + if (ext_type == OPT_PARAM_EXT_LEN) { + if (ibuf_get_n16(&oparams, &op_len) == -1) + goto bad_len; + } else { + uint8_t tmp; + if (ibuf_get_n8(&oparams, &tmp) == -1) + goto bad_len; + op_len = tmp; + } + + if (ibuf_get_ibuf(&oparams, op_len, &op) == -1) goto bad_len; - op_val = p; - p += op_len; - plen -= op_len; - } else - op_val = NULL; - switch (op_type) { - case OPT_PARAM_CAPABILITIES: /* RFC 3392 */ - if (parse_capabilities(peer, op_val, op_len, - &as) == -1) { - session_notification(peer, ERR_OPEN, 0, NULL); + switch (op_type) { + case OPT_PARAM_CAPABILITIES: /* RFC 3392 */ + if (parse_capabilities(peer, &op, &as) == -1) { + session_notification(peer, ERR_OPEN, 0, + NULL); + change_state(peer, STATE_IDLE, + EVNT_RCVD_OPEN); + return (-1); + } + break; + case OPT_PARAM_AUTH: /* deprecated */ + default: + /* + * unsupported type + * the RFCs tell us to leave the data section + * empty and notify the peer with ERR_OPEN, + * ERR_OPEN_OPT. How the peer should know + * _which_ optional parameter we don't support + * is beyond me. + */ + log_peer_warnx(&peer->conf, + "received OPEN message with unsupported " + "optional parameter: type %u", op_type); + session_notification(peer, ERR_OPEN, + ERR_OPEN_OPT, NULL); change_state(peer, STATE_IDLE, EVNT_RCVD_OPEN); return (-1); } - break; - case OPT_PARAM_AUTH: /* deprecated */ - default: - /* - * unsupported type - * the RFCs tell us to leave the data section empty - * and notify the peer with ERR_OPEN, ERR_OPEN_OPT. - * How the peer should know _which_ optional parameter - * we don't support is beyond me. - */ - log_peer_warnx(&peer->conf, - "received OPEN message with unsupported optional " - "parameter: type %u", op_type); - session_notification(peer, ERR_OPEN, ERR_OPEN_OPT, - NULL); - change_state(peer, STATE_IDLE, EVNT_RCVD_OPEN); - return (-1); } } + if (ibuf_size(&ibuf) != 0) { + bad_len: + log_peer_warnx(&peer->conf, + "corrupt OPEN message received: length mismatch"); + session_notification(peer, ERR_OPEN, 0, NULL); + change_state(peer, STATE_IDLE, EVNT_RCVD_OPEN); + return (-1); + } + /* if remote-as is zero and it's a cloned neighbor, accept any */ if (peer->template && !peer->conf.remote_as && as != AS_TRANS) { peer->conf.remote_as = as; @@ -2381,6 +2377,7 @@ int parse_rrefresh(struct peer *peer) { struct route_refresh rr; + struct ibuf ibuf; uint16_t afi, datalen; uint8_t aid, safi, subtype; u_char *p; @@ -2392,21 +2389,17 @@ parse_rrefresh(struct peer *peer) p = peer->rbuf->rptr; p += MSGSIZE_HEADER; /* header is already checked */ + datalen -= MSGSIZE_HEADER; - /* - * We could check if we actually announced the capability but - * as long as the message is correctly encoded we don't care. - */ + /* XXX */ + ibuf_from_buffer(&ibuf, p, datalen); - /* afi, 2 byte */ - memcpy(&afi, p, sizeof(afi)); - afi = ntohs(afi); - p += 2; - /* subtype, 1 byte */ - subtype = *p; - p += 1; - /* safi, 1 byte */ - safi = *p; + if (ibuf_get_n16(&ibuf, &afi) == -1 || + ibuf_get_n8(&ibuf, &subtype) == -1 || + ibuf_get_n8(&ibuf, &safi) == -1) { + /* minimum size checked in session_process_msg() */ + fatalx("%s: message too small", __func__); + } /* check subtype if peer announced enhanced route refresh */ if (peer->capa.neg.enhanced_rr) { @@ -2432,11 +2425,9 @@ parse_rrefresh(struct peer *peer) log_peer_warnx(&peer->conf, "received RREFRESH: illegal len: %u byte", datalen); - p = peer->rbuf->rptr; - p += MSGSIZE_HEADER; - datalen -= MSGSIZE_HEADER; - session_notification_data(peer, ERR_RREFRESH, - ERR_RR_INV_LEN, p, datalen); + ibuf_rewind(&ibuf); + session_notification(peer, ERR_RREFRESH, + ERR_RR_INV_LEN, &ibuf); bgp_fsm(peer, EVNT_CON_FATAL); return (-1); } @@ -2498,7 +2489,7 @@ parse_notification(struct peer *peer) /* XXX */ ibuf_from_buffer(&ibuf, p, datalen); - + if (ibuf_get_n8(&ibuf, &errcode) == -1 || ibuf_get_n8(&ibuf, &subcode) == -1) { log_peer_warnx(&peer->conf, "received bad notification"); @@ -2530,58 +2521,38 @@ done: } int -parse_capabilities(struct peer *peer, u_char *d, uint16_t dlen, uint32_t *as) +parse_capabilities(struct peer *peer, struct ibuf *buf, uint32_t *as) { - u_char *capa_val; - uint32_t remote_as; - uint16_t len; - uint16_t afi; - uint16_t gr_header; - uint8_t safi; - uint8_t aid; - uint8_t flags; - uint8_t capa_code; - uint8_t capa_len; - uint8_t i; - - len = dlen; - while (len > 0) { - if (len < 2) { + struct ibuf capabuf; + uint16_t afi, gr_header; + uint8_t capa_code, capa_len; + uint8_t safi, aid, role, flags; + + while (ibuf_size(buf) > 0) { + if (ibuf_get_n8(buf, &capa_code) == -1 || + ibuf_get_n8(buf, &capa_len) == -1) { log_peer_warnx(&peer->conf, "Bad capabilities attr " - "length: %u, too short", len); + "length: too short"); + return (-1); + } + if (ibuf_get_ibuf(buf, capa_len, &capabuf) == -1) { + log_peer_warnx(&peer->conf, + "Received bad capabilities attr length: " + "len %zu smaller than capa_len %u", + ibuf_size(buf), capa_len); return (-1); } - memcpy(&capa_code, d, sizeof(capa_code)); - d += sizeof(capa_code); - len -= sizeof(capa_code); - memcpy(&capa_len, d, sizeof(capa_len)); - d += sizeof(capa_len); - len -= sizeof(capa_len); - if (capa_len > 0) { - if (len < capa_len) { - log_peer_warnx(&peer->conf, - "Bad capabilities attr length: " - "len %u smaller than capa_len %u", - len, capa_len); - return (-1); - } - capa_val = d; - d += capa_len; - len -= capa_len; - } else - capa_val = NULL; switch (capa_code) { case CAPA_MP: /* RFC 4760 */ - if (capa_len != 4) { + if (capa_len != 4 || + ibuf_get_n16(&capabuf, &afi) == -1 || + ibuf_skip(&capabuf, 1) == -1 || + ibuf_get_n8(&capabuf, &safi) == -1) { log_peer_warnx(&peer->conf, - "Bad multi protocol capability length: " - "%u", capa_len); + "Received bad multi protocol capability"); break; } - memcpy(&afi, capa_val, sizeof(afi)); - afi = ntohs(afi); - memcpy(&safi, capa_val + 3, sizeof(safi)); if (afi2aid(afi, safi, &aid) == -1) { log_peer_warnx(&peer->conf, "Received multi protocol capability: " @@ -2595,9 +2566,10 @@ parse_capabilities(struct peer *peer, u_char *d, uint16_t dlen, uint32_t *as) peer->capa.peer.refresh = 1; break; case CAPA_ROLE: - if (capa_len != 1) { + if (capa_len != 1 || + ibuf_get_n8(&capabuf, &role) == -1) { log_peer_warnx(&peer->conf, - "Bad role capability length: %u", capa_len); + "Received bad role capability"); break; } if (!peer->conf.ebgp) { @@ -2606,7 +2578,7 @@ parse_capabilities(struct peer *peer, u_char *d, uint16_t dlen, uint32_t *as) break; } peer->capa.peer.policy = 1; - peer->remote_role = capa2role(*capa_val); + peer->remote_role = capa2role(role); break; case CAPA_RESTART: if (capa_len == 2) { @@ -2616,29 +2588,35 @@ parse_capabilities(struct peer *peer, u_char *d, uint16_t dlen, uint32_t *as) break; } else if (capa_len % 4 != 2) { log_peer_warnx(&peer->conf, - "Bad graceful restart capability length: " - "%u", capa_len); + "Bad graceful restart capability"); + peer->capa.peer.grestart.restart = 0; + peer->capa.peer.grestart.timeout = 0; + break; + } + + if (ibuf_get_n16(&capabuf, &gr_header) == -1) { + bad_gr_restart: + log_peer_warnx(&peer->conf, + "Bad graceful restart capability"); peer->capa.peer.grestart.restart = 0; peer->capa.peer.grestart.timeout = 0; break; } - memcpy(&gr_header, capa_val, sizeof(gr_header)); - gr_header = ntohs(gr_header); peer->capa.peer.grestart.timeout = gr_header & CAPA_GR_TIMEMASK; if (peer->capa.peer.grestart.timeout == 0) { log_peer_warnx(&peer->conf, "Received " - "graceful restart timeout is zero"); + "graceful restart with zero timeout"); peer->capa.peer.grestart.restart = 0; break; } - for (i = 2; i <= capa_len - 4; i += 4) { - memcpy(&afi, capa_val + i, sizeof(afi)); - afi = ntohs(afi); - safi = capa_val[i + 2]; - flags = capa_val[i + 3]; + while (ibuf_size(&capabuf) > 0) { + if (ibuf_get_n16(&capabuf, &afi) == -1 || + ibuf_get_n8(&capabuf, &safi) == -1 || + ibuf_get_n8(&capabuf, &flags) == -1) + goto bad_gr_restart; if (afi2aid(afi, safi, &aid) == -1) { log_peer_warnx(&peer->conf, "Received graceful restart capa: " @@ -2658,15 +2636,13 @@ parse_capabilities(struct peer *peer, u_char *d, uint16_t dlen, uint32_t *as) } break; case CAPA_AS4BYTE: - if (capa_len != 4) { + if (capa_len != 4 || + ibuf_get_n32(&capabuf, as) == -1) { log_peer_warnx(&peer->conf, - "Bad AS4BYTE capability length: " - "%u", capa_len); + "Received bad AS4BYTE capability"); peer->capa.peer.as4byte = 0; break; } - memcpy(&remote_as, capa_val, sizeof(remote_as)); - *as = ntohl(remote_as); if (*as == 0) { log_peer_warnx(&peer->conf, "peer requests unacceptable AS %u", *as); @@ -2679,18 +2655,18 @@ parse_capabilities(struct peer *peer, u_char *d, uint16_t dlen, uint32_t *as) break; case CAPA_ADD_PATH: if (capa_len % 4 != 0) { + bad_add_path: log_peer_warnx(&peer->conf, - "Bad ADD-PATH capability length: " - "%u", capa_len); + "Received bad ADD-PATH capability"); memset(peer->capa.peer.add_path, 0, sizeof(peer->capa.peer.add_path)); break; } - for (i = 0; i <= capa_len - 4; i += 4) { - memcpy(&afi, capa_val + i, sizeof(afi)); - afi = ntohs(afi); - safi = capa_val[i + 2]; - flags = capa_val[i + 3]; + while (ibuf_size(&capabuf) > 0) { + if (ibuf_get_n16(&capabuf, &afi) == -1 || + ibuf_get_n8(&capabuf, &safi) == -1 || + ibuf_get_n8(&capabuf, &flags) == -1) + goto bad_add_path; if (afi2aid(afi, safi, &aid) == -1) { log_peer_warnx(&peer->conf, "Received ADD-PATH capa: " -- 2.20.1