Download raw body.
bgpd more ibuf in session.c parse functions
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: "
>
bgpd more ibuf in session.c parse functions