From 04349dff7dad1965b19c3ecc9ec605f0d55ec7c1 Mon Sep 17 00:00:00 2001 From: claudio Date: Tue, 30 Jan 2024 13:50:08 +0000 Subject: [PATCH] Convert he ATTR_ASPATH and ATTR_AS4_PATH handlers in rde_attr_parse() to new ibuf API. Various aspath functions are modified to work better with ibufs. aspath_inflate() now only works with ibufs and is a lot simpler. aspath_verify() does all the checks using the ibuf api and therefor most length checks can be skipped. aspath_asprint() and the new internal aspath_strsize() and aspath_snprint() are totally overhauled -- including some bugs that got squashed. OK tb@ --- usr.sbin/bgpd/bgpd.h | 11 +- usr.sbin/bgpd/rde.c | 80 +++++------- usr.sbin/bgpd/util.c | 295 ++++++++++++++++++++++--------------------- 3 files changed, 190 insertions(+), 196 deletions(-) diff --git a/usr.sbin/bgpd/bgpd.h b/usr.sbin/bgpd/bgpd.h index fd18f830f78..fc31312d899 100644 --- a/usr.sbin/bgpd/bgpd.h +++ b/usr.sbin/bgpd/bgpd.h @@ -1,4 +1,4 @@ -/* $OpenBSD: bgpd.h,v 1.483 2024/01/23 16:13:35 claudio Exp $ */ +/* $OpenBSD: bgpd.h,v 1.484 2024/01/30 13:50:08 claudio Exp $ */ /* * Copyright (c) 2003, 2004 Henning Brauer @@ -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 *); diff --git a/usr.sbin/bgpd/rde.c b/usr.sbin/bgpd/rde.c index c357ea67a03..17d357c5999 100644 --- a/usr.sbin/bgpd/rde.c +++ b/usr.sbin/bgpd/rde.c @@ -1,4 +1,4 @@ -/* $OpenBSD: rde.c,v 1.619 2024/01/25 11:13:35 claudio Exp $ */ +/* $OpenBSD: rde.c,v 1.620 2024/01/30 13:50:09 claudio Exp $ */ /* * Copyright (c) 2003, 2004 Henning Brauer @@ -1916,13 +1916,6 @@ rde_update_withdraw(struct rde_peer *peer, uint32_t path_id, */ /* 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 rde_peer *peer, { 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 rde_peer *peer, 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 rde_peer *peer, 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 rde_peer *peer, 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 rde_peer *peer, 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 rde_peer *peer, break; } - (void)plen; /* XXX make compiler happy for now */ return (0); bad_len: @@ -2309,7 +2292,6 @@ rde_attr_parse(struct ibuf *buf, struct rde_peer *peer, return (-1); } -#undef UPD_READ #undef CHECK_FLAGS int diff --git a/usr.sbin/bgpd/util.c b/usr.sbin/bgpd/util.c index 6fecee68adb..f3b08f945ef 100644 --- a/usr.sbin/bgpd/util.c +++ b/usr.sbin/bgpd/util.c @@ -1,4 +1,4 @@ -/* $OpenBSD: util.c,v 1.79 2024/01/23 16:13:35 claudio Exp $ */ +/* $OpenBSD: util.c,v 1.80 2024/01/30 13:50:09 claudio Exp $ */ /* * Copyright (c) 2006 Claudio Jeker @@ -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) { @@ -235,6 +233,26 @@ log_aspa(struct aspa_set *aspa) return errbuf; } +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) { @@ -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 closing) } } -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; - r = snprintf(buf, size, "%s%s", - total_size != 0 ? " " : "", + 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", 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, void *data, uint16_t len) 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, int as4byte, int noset) 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, -- 2.20.1