From: Claudio Jeker Subject: Re: bgpd: last bit of rde_attr_parse ibuf cleanup To: Theo Buehler Cc: tech@openbsd.org Date: Tue, 30 Jan 2024 14:16:40 +0100 On Mon, Jan 29, 2024 at 11:39:30AM +0100, Theo Buehler wrote: > On Thu, Jan 25, 2024 at 02:25:51PM +0100, Claudio Jeker wrote: > > This converts the ATTR_ASPATH and ATTR_AS4_PATH handlers in > > rde_attr_parse(). To do this the various aspath validation and print > > functions got some ibuf treatment and the result is IMO nicer then the > > code before. > > > > Again there are tentacles in bgpctl so handle them as well. This converts > > IMSG_CTL_SHOW_RIB and adds a bit of a hack to the show_attr() bits -- > > those will be cleaned up soon. > > > > The util.c changes are the ones that need some careful review. The rest of > > this change is rather simple. > > I'm ok with this as it is, but I have a couple of comments on the > util.c change: > > > -size_t > > -aspath_strlen(void *data, uint16_t len) > > +static ssize_t > > +aspath_strsize(struct ibuf *in) > > { > > - uint8_t *seg; > > - int total_size; > > + struct ibuf buf; > > + ssize_t total_size = 0; > > uint32_t as; > > - uint16_t seg_size; > > uint8_t i, seg_type, seg_len; > > > > - total_size = 0; > > - seg = data; > > - for (; len > 0; len -= seg_size, seg += seg_size) { > > - seg_type = seg[0]; > > - seg_len = seg[1]; > > - seg_size = 2 + sizeof(uint32_t) * seg_len; > > - > > - if (seg_type == AS_SET) > > - if (total_size != 0) > > - total_size += 3; > > - else > > - total_size += 2; > > - else if (total_size != 0) > > + ibuf_from_ibuf(&buf, in); > > + while (ibuf_size(&buf) > 0) { > > + if (ibuf_get_n8(&buf, &seg_type) == -1 || > > + ibuf_get_n8(&buf, &seg_len) == -1) > > + return (-1); > > Should this error on seg_len == 0 like in aspath_verify()? > (I know it is currently only called after an aspath_verify()). I guess adding this check does not hurt. The code indeed assumes that no aspath function is called before aspath_verify() was called. So aspath_strsize() could depend on the fact that the aspath is valid enough to not blow up here. Still adding a check is cheap and I will add it. > > + while (ibuf_size(&buf) > 0) { > > + if (ibuf_get_n8(&buf, &seg_type) == -1 || > > + ibuf_get_n8(&buf, &seg_len) == -1) { > > + error = AS_ERR_LEN; > > Don't think it matters, but this was previously an AS_ERR_BAD, I think the reported error is a bit more correct with AS_ERR_LEN. Since the lenght of the attribute is inconsistent with the segment encoding. > > -u_char * > > -aspath_inflate(void *data, uint16_t len, uint16_t *newlen) > > +void * > > +aspath_inflate(struct ibuf *in, struct ibuf *out) > > { > > - uint8_t *seg, *nseg, *ndata; > > - uint16_t seg_size, olen, nlen; > > + struct ibuf buf; > > + uint8_t *nseg, *ndata; > > + size_t seg_size, nlen = 0; > > uint8_t seg_len; > > > > + /* clone buffer so we can pass it twice without altering in */ > > + ibuf_from_ibuf(&buf, in); > > /* first calculate the length of the aspath */ > > - seg = data; > > - nlen = 0; > > - for (olen = len; olen > 0; olen -= seg_size, seg += seg_size) { > > - seg_len = seg[1]; > > - seg_size = 2 + sizeof(uint16_t) * seg_len; > > - nlen += 2 + sizeof(uint32_t) * seg_len; > > - > > - if (seg_size > olen) { > > - errno = ERANGE; > > + while (ibuf_size(&buf) > 0) { > > + if (ibuf_skip(&buf, 1) == -1 || > > + ibuf_get_n8(&buf, &seg_len) == -1) > > return (NULL); > > - } > > + seg_size = sizeof(uint16_t) * seg_len; > > + if (ibuf_skip(&buf, seg_size) == -1) > > + return (NULL); > > + nlen += 2 + sizeof(uint32_t) * seg_len; > > } > > This is correct and preserves behavior. Would it not be acceptable to be > lazy and calculate nlen as 2 * ibuf_size(&buf)? This overestimates the > size by 2 * number of segments. Does that really matter? The out buffer > is very short lived. > > Another option (a bit of a layer violation) might be to let > aspath_verify() count the number of segments. This way you could > calculate the inflated size precisely and save one walk over the buffer. I agree that just over allocating is probably the way to go. Will change the code accordingly. > > > > - *newlen = nlen; > > - if ((ndata = malloc(nlen)) == NULL) > > + if ((nseg = ndata = malloc(nlen)) == NULL) > > return (NULL); > > > > + ibuf_from_ibuf(&buf, in); > > /* then copy the aspath */ > > - seg = data; > > - for (nseg = ndata; nseg < ndata + nlen; ) { > > - *nseg++ = *seg++; > > - *nseg++ = seg_len = *seg++; > > + while (ibuf_size(&buf) > 0) { > > + if (ibuf_get_n8(&buf, nseg++) == -1 || > > I don't like this manually advancing the nseg buffer. Should ndata not > be wrapped in an ibuf to which you add the data from buf, padding with > two zeroes for each segment? This will be a bit more code, but you'd get > a correct size in a single loop. Totally changed to code to return a struct ibuf *, so we can use ibuf_open() and ibuf_add() this makes the code a lot simpler. > > + ibuf_get_n8(&buf, &seg_len) == -1) { > > + free(ndata); > > + return (NULL); > > + } > > + *nseg++ = seg_len; > > Again, I would probably error on 0 seg_len here. > > > for (; seg_len > 0; seg_len--) { > > Might be worth using an extra ibuf for the segment. Not sure if that makes the code better. I skipped that for now. Need to check if aspath_inflate() is actually excercised in regress. It is a code path that is far less travelled then the default 4byte ASnum one. -- :wq Claudio Index: bgpd.h =================================================================== RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v diff -u -p -r1.483 bgpd.h --- bgpd.h 23 Jan 2024 16:13:35 -0000 1.483 +++ bgpd.h 30 Jan 2024 13:08:40 -0000 @@ -1542,20 +1542,19 @@ const char *log_as(uint32_t); const char *log_rd(uint64_t); const char *log_ext_subtype(int, uint8_t); const char *log_reason(const char *); +const char *log_aspath_error(int); const char *log_roa(struct roa *); const char *log_aspa(struct aspa_set *); const char *log_rtr_error(enum rtr_error); const char *log_policy(enum role); -int aspath_snprint(char *, size_t, void *, uint16_t); -int aspath_asprint(char **, void *, uint16_t); -size_t aspath_strlen(void *, uint16_t); +int aspath_asprint(char **, struct ibuf *); uint32_t aspath_extract(const void *, int); -int aspath_verify(void *, uint16_t, int, int); +int aspath_verify(struct ibuf *, int, int); #define AS_ERR_LEN -1 #define AS_ERR_TYPE -2 #define AS_ERR_BAD -3 #define AS_ERR_SOFT -4 -u_char *aspath_inflate(void *, uint16_t, uint16_t *); +struct ibuf *aspath_inflate(struct ibuf *); int extract_prefix(const u_char *, int, void *, uint8_t, uint8_t); int nlri_get_prefix(struct ibuf *, struct bgpd_addr *, uint8_t *); int nlri_get_prefix6(struct ibuf *, struct bgpd_addr *, uint8_t *); Index: rde.c =================================================================== RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v diff -u -p -r1.619 rde.c --- rde.c 25 Jan 2024 11:13:35 -0000 1.619 +++ rde.c 30 Jan 2024 13:10:31 -0000 @@ -1916,13 +1916,6 @@ rde_update_withdraw(struct rde_peer *pee */ /* attribute parser specific macros */ -#define UPD_READ(t, p, plen, n) \ - do { \ - memcpy(t, p, n); \ - p += n; \ - plen += n; \ - } while (0) - #define CHECK_FLAGS(s, t, m) \ (((s) & ~(ATTR_DEFMASK | (m))) == (t)) @@ -1932,12 +1925,10 @@ rde_attr_parse(struct ibuf *buf, struct { struct bgpd_addr nexthop; struct rde_aspath *a = &state->aspath; - struct ibuf attrbuf, tmpbuf; - u_char *p, *npath; + struct ibuf attrbuf, tmpbuf, *npath = NULL; + size_t alen, hlen; uint32_t tmp32, zero = 0; int error; - uint16_t nlen; - size_t attr_len, hlen, plen; uint8_t flags, type; ibuf_from_ibuf(&attrbuf, buf); @@ -1945,30 +1936,26 @@ rde_attr_parse(struct ibuf *buf, struct ibuf_get_n8(&attrbuf, &type) == -1) goto bad_list; - if (flags & ATTR_EXTLEN) { - uint16_t alen; - if (ibuf_get_n16(&attrbuf, &alen) == -1) + uint16_t attr_len; + if (ibuf_get_n16(&attrbuf, &attr_len) == -1) goto bad_list; - attr_len = alen; + alen = attr_len; hlen = 4; } else { - uint8_t alen; - if (ibuf_get_n8(&attrbuf, &alen) == -1) + uint8_t attr_len; + if (ibuf_get_n8(&attrbuf, &attr_len) == -1) goto bad_list; - attr_len = alen; + alen = attr_len; hlen = 3; } - if (ibuf_truncate(&attrbuf, attr_len) == -1) + if (ibuf_truncate(&attrbuf, alen) == -1) goto bad_list; /* consume the attribute in buf before moving forward */ - if (ibuf_skip(buf, hlen + attr_len) == -1) + if (ibuf_skip(buf, hlen + alen) == -1) goto bad_list; - p = ibuf_data(&attrbuf); - plen = ibuf_size(&attrbuf); - switch (type) { case ATTR_UNDEF: /* ignore and drop path attributes with a type code of 0 */ @@ -1998,44 +1985,43 @@ rde_attr_parse(struct ibuf *buf, struct case ATTR_ASPATH: if (!CHECK_FLAGS(flags, ATTR_WELL_KNOWN, 0)) goto bad_flags; - error = aspath_verify(p, attr_len, peer_has_as4byte(peer), + if (a->flags & F_ATTR_ASPATH) + goto bad_list; + error = aspath_verify(&attrbuf, peer_has_as4byte(peer), peer_accept_no_as_set(peer)); - if (error == AS_ERR_SOFT) { - /* - * soft errors like unexpected segment types are - * not considered fatal and the path is just - * marked invalid. - */ - a->flags |= F_ATTR_PARSE_ERR; - } else if (error != 0) { + if (error != 0 && error != AS_ERR_SOFT) { + log_peer_warnx(&peer->conf, "bad ASPATH, %s", + log_aspath_error(error)); rde_update_err(peer, ERR_UPDATE, ERR_UPD_ASPATH, NULL); return (-1); } - if (a->flags & F_ATTR_ASPATH) - goto bad_list; if (peer_has_as4byte(peer)) { - npath = p; - nlen = attr_len; + ibuf_from_ibuf(&tmpbuf, &attrbuf); } else { - npath = aspath_inflate(p, attr_len, &nlen); - if (npath == NULL) + if ((npath = aspath_inflate(&attrbuf)) == NULL) fatal("aspath_inflate"); + ibuf_from_ibuf(&tmpbuf, npath); } if (error == AS_ERR_SOFT) { char *str; - aspath_asprint(&str, npath, nlen); + /* + * soft errors like unexpected segment types are + * not considered fatal and the path is just + * marked invalid. + */ + a->flags |= F_ATTR_PARSE_ERR; + + aspath_asprint(&str, &tmpbuf); log_peer_warnx(&peer->conf, "bad ASPATH %s, " "path invalidated and prefix withdrawn", str ? str : "(bad aspath)"); free(str); } a->flags |= F_ATTR_ASPATH; - a->aspath = aspath_get(npath, nlen); - if (npath != p) - free(npath); - plen += attr_len; + a->aspath = aspath_get(ibuf_data(&tmpbuf), ibuf_size(&tmpbuf)); + ibuf_free(npath); break; case ATTR_NEXTHOP: if (!CHECK_FLAGS(flags, ATTR_WELL_KNOWN, 0)) @@ -2050,7 +2036,6 @@ rde_attr_parse(struct ibuf *buf, struct nexthop.aid = AID_INET; 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. @@ -2245,12 +2230,11 @@ rde_attr_parse(struct ibuf *buf, struct if (!CHECK_FLAGS(flags, ATTR_OPTIONAL|ATTR_TRANSITIVE, ATTR_PARTIAL)) goto bad_flags; - if ((error = aspath_verify(p, attr_len, 1, + if ((error = aspath_verify(&attrbuf, 1, peer_accept_no_as_set(peer))) != 0) { /* As per RFC6793 use "attribute discard" here. */ log_peer_warnx(&peer->conf, "bad AS4_PATH, " "attribute discarded"); - plen += attr_len; break; } a->flags |= F_ATTR_AS4BYTE_NEW; @@ -2295,7 +2279,6 @@ rde_attr_parse(struct ibuf *buf, struct break; } - (void)plen; /* XXX make compiler happy for now */ return (0); bad_len: @@ -2309,7 +2292,6 @@ rde_attr_parse(struct ibuf *buf, struct return (-1); } -#undef UPD_READ #undef CHECK_FLAGS int Index: util.c =================================================================== RCS file: /cvs/src/usr.sbin/bgpd/util.c,v diff -u -p -r1.79 util.c --- util.c 23 Jan 2024 16:13:35 -0000 1.79 +++ util.c 30 Jan 2024 13:08:20 -0000 @@ -32,8 +32,6 @@ #include "rde.h" #include "log.h" -const char *aspath_delim(uint8_t, int); - const char * log_addr(const struct bgpd_addr *addr) { @@ -236,6 +234,26 @@ log_aspa(struct aspa_set *aspa) } const char * +log_aspath_error(int error) +{ + static char buf[20]; + + switch (error) { + case AS_ERR_LEN: + return "inconsitent lenght"; + case AS_ERR_TYPE: + return "unknown segment type"; + case AS_ERR_BAD: + return "invalid encoding"; + case AS_ERR_SOFT: + return "soft failure"; + default: + snprintf(buf, sizeof(buf), "unknown %d", error); + return buf; + } +} + +const char * log_rtr_error(enum rtr_error err) { static char buf[20]; @@ -286,7 +304,7 @@ log_policy(enum role role) } } -const char * +static const char * aspath_delim(uint8_t seg_type, int closing) { static char db[8]; @@ -318,42 +336,38 @@ aspath_delim(uint8_t seg_type, int closi } } -int -aspath_snprint(char *buf, size_t size, void *data, uint16_t len) +static int +aspath_snprint(char *buf, size_t size, struct ibuf *in) { -#define UPDATE() \ - do { \ - if (r < 0) \ - return (-1); \ - total_size += r; \ - if ((unsigned int)r < size) { \ - size -= r; \ - buf += r; \ - } else { \ - buf += size; \ - size = 0; \ - } \ +#define UPDATE() \ + do { \ + if (r < 0 || (unsigned int)r >= size) \ + return (-1); \ + size -= r; \ + buf += r; \ } while (0) - uint8_t *seg; - int r, total_size; - uint16_t seg_size; - uint8_t i, seg_type, seg_len; - total_size = 0; - seg = data; - for (; len > 0; len -= seg_size, seg += seg_size) { - seg_type = seg[0]; - seg_len = seg[1]; - seg_size = 2 + sizeof(uint32_t) * seg_len; + struct ibuf data; + uint32_t as; + int r, n = 0; + uint8_t i, seg_type, seg_len; + + ibuf_from_ibuf(&data, in); + while (ibuf_size(&data) > 0) { + if (ibuf_get_n8(&data, &seg_type) == -1 || + ibuf_get_n8(&data, &seg_len) == -1 || + seg_len == 0) + return (-1); - r = snprintf(buf, size, "%s%s", - total_size != 0 ? " " : "", + r = snprintf(buf, size, "%s%s", n++ != 0 ? " " : "", aspath_delim(seg_type, 0)); UPDATE(); for (i = 0; i < seg_len; i++) { - r = snprintf(buf, size, "%s", - log_as(aspath_extract(seg, i))); + if (ibuf_get_n32(&data, &as) == -1) + return -1; + + r = snprintf(buf, size, "%s", log_as(as)); UPDATE(); if (i + 1 < seg_len) { r = snprintf(buf, size, " "); @@ -364,73 +378,68 @@ aspath_snprint(char *buf, size_t size, v UPDATE(); } /* ensure that we have a valid C-string especially for empty as path */ - if (size > 0) - *buf = '\0'; - - return (total_size); -#undef UPDATE -} - -int -aspath_asprint(char **ret, void *data, uint16_t len) -{ - size_t slen; - int plen; - - slen = aspath_strlen(data, len) + 1; - *ret = malloc(slen); - if (*ret == NULL) - return (-1); - - plen = aspath_snprint(*ret, slen, data, len); - if (plen == -1) { - free(*ret); - *ret = NULL; - return (-1); - } - + *buf = '\0'; return (0); +#undef UPDATE } -size_t -aspath_strlen(void *data, uint16_t len) +static ssize_t +aspath_strsize(struct ibuf *in) { - uint8_t *seg; - int total_size; + struct ibuf buf; + ssize_t total_size = 0; uint32_t as; - uint16_t seg_size; uint8_t i, seg_type, seg_len; - total_size = 0; - seg = data; - for (; len > 0; len -= seg_size, seg += seg_size) { - seg_type = seg[0]; - seg_len = seg[1]; - seg_size = 2 + sizeof(uint32_t) * seg_len; - - if (seg_type == AS_SET) - if (total_size != 0) - total_size += 3; - else - total_size += 2; - else if (total_size != 0) + ibuf_from_ibuf(&buf, in); + while (ibuf_size(&buf) > 0) { + if (ibuf_get_n8(&buf, &seg_type) == -1 || + ibuf_get_n8(&buf, &seg_len) == -1 || + seg_len == 0) + return (-1); + + if (total_size != 0) total_size += 1; + total_size += strlen(aspath_delim(seg_type, 0)); for (i = 0; i < seg_len; i++) { - as = aspath_extract(seg, i); + if (ibuf_get_n32(&buf, &as) == -1) + return (-1); do { total_size++; } while ((as = as / 10) != 0); - - if (i + 1 < seg_len) - total_size += 1; } + total_size += seg_len - 1; - if (seg_type == AS_SET) - total_size += 2; + total_size += strlen(aspath_delim(seg_type, 1)); } - return (total_size); + return (total_size + 1); +} + +int +aspath_asprint(char **ret, struct ibuf *data) +{ + ssize_t slen; + + if ((slen = aspath_strsize(data)) == -1) { + *ret = NULL; + errno = EINVAL; + return (-1); + } + + *ret = malloc(slen); + if (*ret == NULL) + return (-1); + + if (aspath_snprint(*ret, slen, data) == -1) { + free(*ret); + *ret = NULL; + errno = EINVAL; + return (-1); + } + + return (0); } /* @@ -456,32 +465,31 @@ aspath_extract(const void *seg, int pos) * Verify that the aspath is correctly encoded. */ int -aspath_verify(void *data, uint16_t len, int as4byte, int noset) +aspath_verify(struct ibuf *in, int as4byte, int noset) { - uint8_t *seg = data; - uint16_t seg_size, as_size = 2; + struct ibuf buf; + int pos, error = 0; uint8_t seg_len, seg_type; - int error = 0; - if (len & 1) + ibuf_from_ibuf(&buf, in); + if (ibuf_size(&buf) & 1) { /* odd length aspath are invalid */ - return (AS_ERR_BAD); - - if (as4byte) - as_size = 4; - - for (; len > 0; len -= seg_size, seg += seg_size) { - const uint8_t *ptr; - int pos; + error = AS_ERR_BAD; + goto done; + } - if (len < 2) /* header length check */ - return (AS_ERR_BAD); - seg_type = seg[0]; - seg_len = seg[1]; + while (ibuf_size(&buf) > 0) { + if (ibuf_get_n8(&buf, &seg_type) == -1 || + ibuf_get_n8(&buf, &seg_len) == -1) { + error = AS_ERR_LEN; + goto done; + } - if (seg_len == 0) + if (seg_len == 0) { /* empty aspath segments are not allowed */ - return (AS_ERR_BAD); + error = AS_ERR_BAD; + goto done; + } /* * BGP confederations should not show up but consider them @@ -497,70 +505,75 @@ aspath_verify(void *data, uint16_t len, if (noset && seg_type == AS_SET) error = AS_ERR_SOFT; if (seg_type != AS_SET && seg_type != AS_SEQUENCE && - seg_type != AS_CONFED_SEQUENCE && seg_type != AS_CONFED_SET) - return (AS_ERR_TYPE); - - seg_size = 2 + as_size * seg_len; - - if (seg_size > len) - return (AS_ERR_LEN); + seg_type != AS_CONFED_SEQUENCE && + seg_type != AS_CONFED_SET) { + error = AS_ERR_TYPE; + goto done; + } /* RFC 7607 - AS 0 is considered malformed */ - ptr = seg + 2; for (pos = 0; pos < seg_len; pos++) { uint32_t as; - memcpy(&as, ptr, as_size); + if (as4byte) { + if (ibuf_get_n32(&buf, &as) == -1) { + error = AS_ERR_LEN; + goto done; + } + } else { + uint16_t tmp; + if (ibuf_get_n16(&buf, &tmp) == -1) { + error = AS_ERR_LEN; + goto done; + } + as = tmp; + } if (as == 0) error = AS_ERR_SOFT; - ptr += as_size; } } + + done: return (error); /* aspath is valid but probably not loop free */ } /* * convert a 2 byte aspath to a 4 byte one. */ -u_char * -aspath_inflate(void *data, uint16_t len, uint16_t *newlen) +struct ibuf * +aspath_inflate(struct ibuf *in) { - uint8_t *seg, *nseg, *ndata; - uint16_t seg_size, olen, nlen; - uint8_t seg_len; - - /* first calculate the length of the aspath */ - seg = data; - nlen = 0; - for (olen = len; olen > 0; olen -= seg_size, seg += seg_size) { - seg_len = seg[1]; - seg_size = 2 + sizeof(uint16_t) * seg_len; - nlen += 2 + sizeof(uint32_t) * seg_len; - - if (seg_size > olen) { - errno = ERANGE; - return (NULL); - } - } + struct ibuf *out; + uint16_t short_as; + uint8_t seg_type, seg_len; - *newlen = nlen; - if ((ndata = malloc(nlen)) == NULL) + /* allocate enough space for the worst case */ + if ((out = ibuf_open(ibuf_size(in) * 2)) == NULL) return (NULL); /* then copy the aspath */ - seg = data; - for (nseg = ndata; nseg < ndata + nlen; ) { - *nseg++ = *seg++; - *nseg++ = seg_len = *seg++; + while (ibuf_size(in) > 0) { + if (ibuf_get_n8(in, &seg_type) == -1 || + ibuf_get_n8(in, &seg_len) == -1 || + seg_len == 0) + goto fail; + if (ibuf_add_n8(out, seg_type) == -1 || + ibuf_add_n8(out, seg_len) == -1) + goto fail; + for (; seg_len > 0; seg_len--) { - *nseg++ = 0; - *nseg++ = 0; - *nseg++ = *seg++; - *nseg++ = *seg++; + if (ibuf_get_n16(in, &short_as) == -1) + goto fail; + if (ibuf_add_n32(out, short_as) == -1) + goto fail; } } - return (ndata); + return (out); + +fail: + ibuf_free(out); + return (NULL); } static const u_char addrmask[] = { 0x00, 0x80, 0xc0, 0xe0, 0xf0,