Download raw body.
bgpd more ibuf in session.c parse functions
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).
--
: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] = {
+ 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;
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;
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 ||
+ 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)
+ 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);
+
+ 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