From: Claudio Jeker Subject: bgpd: rde ibuf parser part 4 To: tech@openbsd.org Date: Wed, 24 Jan 2024 16:52:20 +0100 Convert all other attributes in rde_attr_parse() apart from ATTR_ASPATH and ATTR_AS4_PATH to use the ibuf API. I shuffled the checks a bit and always do CHECK_FLAGS() first then the size check. I did extra size check to be sure that there is no extra data in the attribute. -- :wq Claudio Index: rde.c =================================================================== RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v diff -u -p -r1.617 rde.c --- rde.c 24 Jan 2024 14:51:11 -0000 1.617 +++ rde.c 24 Jan 2024 15:33:04 -0000 @@ -1932,7 +1932,7 @@ rde_attr_parse(struct ibuf *buf, struct { struct bgpd_addr nexthop; struct rde_aspath *a = &state->aspath; - struct ibuf attrbuf; + struct ibuf attrbuf, tmpbuf; u_char *p, *npath; uint32_t tmp32, zero = 0; int error; @@ -1974,20 +1974,19 @@ rde_attr_parse(struct ibuf *buf, struct /* ignore and drop path attributes with a type code of 0 */ break; case ATTR_ORIGIN: - if (attr_len != 1) - goto bad_len; - if (!CHECK_FLAGS(flags, ATTR_WELL_KNOWN, 0)) goto bad_flags; - - UPD_READ(&a->origin, p, plen, 1); + if (ibuf_size(&attrbuf) != 1) + goto bad_len; + if (a->flags & F_ATTR_ORIGIN) + goto bad_list; + if (ibuf_get_n8(&attrbuf, &a->origin) == -1) + goto bad_len; if (a->origin > ORIGIN_INCOMPLETE) { rde_update_err(peer, ERR_UPDATE, ERR_UPD_ORIGIN, &attrbuf); return (-1); } - if (a->flags & F_ATTR_ORIGIN) - goto bad_list; a->flags |= F_ATTR_ORIGIN; break; case ATTR_ASPATH: @@ -2033,17 +2032,19 @@ rde_attr_parse(struct ibuf *buf, struct plen += attr_len; break; case ATTR_NEXTHOP: - if (attr_len != 4) - goto bad_len; if (!CHECK_FLAGS(flags, ATTR_WELL_KNOWN, 0)) goto bad_flags; + if (ibuf_size(&attrbuf) != 4) + goto bad_len; if (a->flags & F_ATTR_NEXTHOP) goto bad_list; a->flags |= F_ATTR_NEXTHOP; memset(&nexthop, 0, sizeof(nexthop)); nexthop.aid = AID_INET; - UPD_READ(&nexthop.v4.s_addr, p, plen, 4); + if (ibuf_get_h32(&attrbuf, &nexthop.v4.s_addr) == -1) + goto bad_len; + /* * Check if the nexthop is a valid IP address. We consider * multicast addresses as invalid. @@ -2058,80 +2059,77 @@ rde_attr_parse(struct ibuf *buf, struct state->nexthop = nexthop_get(&nexthop); break; case ATTR_MED: - if (attr_len != 4) - goto bad_len; if (!CHECK_FLAGS(flags, ATTR_OPTIONAL, 0)) goto bad_flags; + if (ibuf_size(&attrbuf) != 4) + goto bad_len; if (a->flags & F_ATTR_MED) goto bad_list; + if (ibuf_get_n32(&attrbuf, &a->med) == -1) + goto bad_len; a->flags |= F_ATTR_MED; - - UPD_READ(&tmp32, p, plen, 4); - a->med = ntohl(tmp32); break; case ATTR_LOCALPREF: - if (attr_len != 4) - goto bad_len; if (!CHECK_FLAGS(flags, ATTR_WELL_KNOWN, 0)) goto bad_flags; + if (ibuf_size(&attrbuf) != 4) + goto bad_len; if (peer->conf.ebgp) { /* ignore local-pref attr on non ibgp peers */ - plen += attr_len; break; } if (a->flags & F_ATTR_LOCALPREF) goto bad_list; + if (ibuf_get_n32(&attrbuf, &a->lpref) == -1) + goto bad_len; a->flags |= F_ATTR_LOCALPREF; - - UPD_READ(&tmp32, p, plen, 4); - a->lpref = ntohl(tmp32); break; case ATTR_ATOMIC_AGGREGATE: - if (attr_len != 0) - goto bad_len; if (!CHECK_FLAGS(flags, ATTR_WELL_KNOWN, 0)) goto bad_flags; + if (ibuf_size(&attrbuf) != 0) + goto bad_len; goto optattr; case ATTR_AGGREGATOR: - if ((!peer_has_as4byte(peer) && attr_len != 6) || - (peer_has_as4byte(peer) && attr_len != 8)) { + if (!CHECK_FLAGS(flags, ATTR_OPTIONAL|ATTR_TRANSITIVE, + ATTR_PARTIAL)) + goto bad_flags; + if ((!peer_has_as4byte(peer) && ibuf_size(&attrbuf) != 6) || + (peer_has_as4byte(peer) && ibuf_size(&attrbuf) != 8)) { /* * ignore attribute in case of error as per * RFC 7606 */ log_peer_warnx(&peer->conf, "bad AGGREGATOR, " "attribute discarded"); - plen += attr_len; break; } - if (!CHECK_FLAGS(flags, ATTR_OPTIONAL|ATTR_TRANSITIVE, - ATTR_PARTIAL)) - goto bad_flags; if (!peer_has_as4byte(peer)) { /* need to inflate aggregator AS to 4-byte */ u_char t[8]; t[0] = t[1] = 0; - UPD_READ(&t[2], p, plen, 2); - UPD_READ(&t[4], p, plen, 4); + if (ibuf_get(&attrbuf, &t[2], 6) == -1) + goto bad_list; if (memcmp(t, &zero, sizeof(uint32_t)) == 0) { /* As per RFC7606 use "attribute discard". */ log_peer_warnx(&peer->conf, "bad AGGREGATOR, " "AS 0 not allowed, attribute discarded"); break; } - if (attr_optadd(a, flags, type, t, - sizeof(t)) == -1) + if (attr_optadd(a, flags, type, t, sizeof(t)) == -1) goto bad_list; break; } /* 4-byte ready server take the default route */ - if (memcmp(p, &zero, sizeof(uint32_t)) == 0) { + ibuf_from_ibuf(&tmpbuf, &attrbuf); + if (ibuf_get_n32(&tmpbuf, &tmp32) == -1) + goto bad_len; + if (tmp32 == 0) { /* As per RFC7606 use "attribute discard" here. */ char *pfmt = log_fmt_peer(&peer->conf); log_debug("%s: bad AGGREGATOR, " "AS 0 not allowed, attribute discarded", pfmt); free(pfmt); - plen += attr_len; break; } goto optattr; @@ -2181,22 +2179,22 @@ rde_attr_parse(struct ibuf *buf, struct } break; case ATTR_ORIGINATOR_ID: - if (attr_len != 4) - goto bad_len; if (!CHECK_FLAGS(flags, ATTR_OPTIONAL, 0)) goto bad_flags; + if (ibuf_size(&attrbuf) != 4) + goto bad_len; goto optattr; case ATTR_CLUSTER_LIST: - if (attr_len % 4 != 0) - goto bad_len; if (!CHECK_FLAGS(flags, ATTR_OPTIONAL, 0)) goto bad_flags; + if (ibuf_size(&attrbuf) % 4 != 0) + goto bad_len; goto optattr; case ATTR_MP_REACH_NLRI: - if (attr_len < 5) - goto bad_len; if (!CHECK_FLAGS(flags, ATTR_OPTIONAL, 0)) goto bad_flags; + if (ibuf_size(&attrbuf) < 5) + goto bad_len; /* the validity is checked in rde_update_dispatch() */ if (a->flags & F_ATTR_MP_REACH) goto bad_list; @@ -2205,10 +2203,10 @@ rde_attr_parse(struct ibuf *buf, struct *reach = attrbuf; break; case ATTR_MP_UNREACH_NLRI: - if (attr_len < 3) - goto bad_len; if (!CHECK_FLAGS(flags, ATTR_OPTIONAL, 0)) goto bad_flags; + if (ibuf_size(&attrbuf) < 3) + goto bad_len; /* the validity is checked in rde_update_dispatch() */ if (a->flags & F_ATTR_MP_UNREACH) goto bad_list; @@ -2217,21 +2215,22 @@ rde_attr_parse(struct ibuf *buf, struct *unreach = attrbuf; break; case ATTR_AS4_AGGREGATOR: - if (attr_len != 8) { + if (!CHECK_FLAGS(flags, ATTR_OPTIONAL|ATTR_TRANSITIVE, + ATTR_PARTIAL)) + goto bad_flags; + if (ibuf_size(&attrbuf) != 8) { /* see ATTR_AGGREGATOR ... */ log_peer_warnx(&peer->conf, "bad AS4_AGGREGATOR, " "attribute discarded"); - plen += attr_len; break; } - if (!CHECK_FLAGS(flags, ATTR_OPTIONAL|ATTR_TRANSITIVE, - ATTR_PARTIAL)) - goto bad_flags; - if (memcmp(p, &zero, sizeof(uint32_t)) == 0) { + ibuf_from_ibuf(&tmpbuf, &attrbuf); + if (ibuf_get_n32(&tmpbuf, &tmp32) == -1) + goto bad_len; + if (tmp32 == 0) { /* As per RFC6793 use "attribute discard" here. */ log_peer_warnx(&peer->conf, "bad AS4_AGGREGATOR, " "AS 0 not allowed, attribute discarded"); - plen += attr_len; break; } a->flags |= F_ATTR_AS4BYTE_NEW; @@ -2251,25 +2250,24 @@ rde_attr_parse(struct ibuf *buf, struct a->flags |= F_ATTR_AS4BYTE_NEW; goto optattr; case ATTR_OTC: - if (attr_len != 4) { + if (!CHECK_FLAGS(flags, ATTR_OPTIONAL|ATTR_TRANSITIVE, + ATTR_PARTIAL)) + goto bad_flags; + if (ibuf_size(&attrbuf) != 4) { /* treat-as-withdraw */ a->flags |= F_ATTR_PARSE_ERR; log_peer_warnx(&peer->conf, "bad OTC, " "path invalidated and prefix withdrawn"); - plen += attr_len; break; } - if (!CHECK_FLAGS(flags, ATTR_OPTIONAL|ATTR_TRANSITIVE, - ATTR_PARTIAL)) - goto bad_flags; switch (peer->role) { case ROLE_PROVIDER: case ROLE_RS: a->flags |= F_ATTR_OTC_LEAK; break; case ROLE_PEER: - memcpy(&tmp32, p, sizeof(tmp32)); - tmp32 = ntohl(tmp32); + if (ibuf_get_n32(&attrbuf, &tmp32) == -1) + goto bad_len; if (tmp32 != peer->conf.remote_as) a->flags |= F_ATTR_OTC_LEAK; break; @@ -2285,10 +2283,9 @@ rde_attr_parse(struct ibuf *buf, struct return (-1); } optattr: - if (attr_optadd(a, flags, type, p, attr_len) == -1) + if (attr_optadd(a, flags, type, ibuf_data(&attrbuf), + ibuf_size(&attrbuf)) == -1) goto bad_list; - - plen += attr_len; break; }