From: Theo Buehler Subject: Re: bgpd more ibuf in session.c parse functions To: tech@openbsd.org Date: Mon, 20 May 2024 11:06:38 +0200 On Fri, May 17, 2024 at 12:27:59PM +0200, Claudio Jeker wrote: > This converts most parse functions over to use the ibuf API. > This is still not perfect mainly because of the way these parse functions > are called. So at the start they do some scary buffer shit before finally > using an ibuf. I want to tackle that slowly but this diff is already big > enough. > > This passes regress and the cert-bgp-testcases which fuzz OPEN a fair bit. > I still need to build something to test RFC9072 support (at least it does > not affect the non extended lenght mode). Couldn't spot anything wrong with it in multiple passes. ok tb nits inline. > > -- > :wq Claudio > > Index: session.c > =================================================================== > RCS file: /cvs/src/usr.sbin/bgpd/session.c,v > diff -u -p -r1.476 session.c > --- session.c 16 May 2024 09:38:21 -0000 1.476 > +++ session.c 17 May 2024 10:15:24 -0000 > @@ -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 u_char marker[MSGSIZE_HEADER_MARKER] = { I think uint8_t would be better > + 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, uin > 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 * > { > 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 * > int > parse_open(struct peer *peer) > { > - u_char *p, *op_val; > + struct ibuf ibuf; > + u_char *p; Not clear when you use u_char and when uint8_t. I'd prefer sticking to the latter. > 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 op_type, optparamlen; The op_type could move into the if (optparamlen != 0) scope or even the while loop. > > 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_h32(&ibuf, &bgpid) == -1 || The get_h32 hiding between all the get_n* feels like a trap. I'd be inclined to use ibuf_get_n32() and convert back when assigning to peer->remote_bgpid. Your call. > + 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,97 @@ 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)); > + 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; > > - extlen = 0; > - optparamlen = *p++; > + if (optparamlen != 0) { > + struct ibuf oparams, op; > + uint8_t ext_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) There are four spaces hiding in the previuos line > + 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 +2376,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 +2388,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. > - */ > - > - /* 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; > + /* XXX */ > + ibuf_from_buffer(&ibuf, p, datalen); > + stray tab in previous line > + 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 +2424,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); > } > @@ -2530,58 +2520,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 +2565,10 @@ parse_capabilities(struct peer *peer, u_ > 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 +2577,7 @@ parse_capabilities(struct peer *peer, u_ > 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 +2587,35 @@ parse_capabilities(struct peer *peer, u_ > 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 +2635,13 @@ parse_capabilities(struct peer *peer, u_ > } > 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 +2654,18 @@ parse_capabilities(struct peer *peer, u_ > 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: " >