From 29c430f8569b90e1962d92a26f96fad7e72d27ec Mon Sep 17 00:00:00 2001 From: Ondrej Zajicek Date: Tue, 23 Jun 2009 10:50:57 +0200 Subject: [PATCH] Changes handling of AS_PATH_CONFED_* segments in AS_PATH. Although standard says that if we receive AS_PATH_CONFED_* (and we are not a part of a confederation) segment, we should drop session, nobody does that and it is unwise to do that. Now we drop session just in case that peer ASN is in AS_PATH_CONFED_* segment (to detect peer that considers BIRD as a part of its confederation). --- proto/bgp/attrs.c | 187 +++++++++++++++++++++++++--------------------- 1 file changed, 103 insertions(+), 84 deletions(-) diff --git a/proto/bgp/attrs.c b/proto/bgp/attrs.c index 8a849e73..e50f6a9c 100644 --- a/proto/bgp/attrs.c +++ b/proto/bgp/attrs.c @@ -55,25 +55,97 @@ bgp_format_origin(eattr *a, byte *buf, int buflen) } static int -bgp_check_path(byte *a, int len, int bs, int errcode) +path_segment_contains(byte *p, int bs, u32 asn) { - while (len) + int i; + int len = p[1]; + p += 2; + + for(i=0; i len) - return errcode; - len -= bs * a[1] + 2; - a += bs * a[1] + 2; + u32 asn2 = (bs == 4) ? get_u32(p) : get_u16(p); + if (asn2 == asn) + return 1; + p += bs; } + return 0; } +/* Validates path attribute, removes AS_CONFED_* segments, and also returns path length */ static int -bgp_check_as_path(struct bgp_proto *p, byte *a, int len) +validate_path(struct bgp_proto *p, int as_path, int bs, byte *idata, unsigned int *ilength) { - return bgp_check_path(a, len, p->as4_session ? 4 : 2, 11); + int res = 0; + u8 *a, *dst; + int len, plen, copy; + + dst = a = idata; + len = *ilength; + + while (len) + { + if (len < 2) + return -1; + + plen = 2 + bs * a[1]; + if (len < plen) + return -1; + + switch (a[0]) + { + case AS_PATH_SET: + copy = 1; + res++; + break; + + case AS_PATH_SEQUENCE: + copy = 1; + res += a[1]; + break; + + case AS_PATH_CONFED_SEQUENCE: + case AS_PATH_CONFED_SET: + if (as_path && path_segment_contains(a, bs, p->remote_as)) + { + log(L_WARN "%s: AS_CONFED_* segment with peer ASN found, misconfigured confederation?", p->p.name); + return -1; + } + + log(L_WARN "%s: %s_PATH attribute contains AS_CONFED_* segment, skipping segment", + p->p.name, as_path ? "AS" : "AS4"); + copy = 0; + break; + + default: + return -1; + } + + if (copy) + { + if (dst != a) + memmove(dst, a, plen); + dst += plen; + } + + len -= plen; + a += plen; + } + + *ilength = dst - idata; + return res; +} + +static inline int +validate_as_path(struct bgp_proto *p, byte *a, int *len) +{ + return validate_path(p, 1, p->as4_session ? 4 : 2, a, len); +} + +static inline int +validate_as4_path(struct bgp_proto *p, struct adata *path) +{ + return validate_path(p, 0, 4, path->data, &path->length); } static int @@ -160,7 +232,7 @@ static struct attr_desc bgp_attr_table[] = { { "origin", 1, BAF_TRANSITIVE, EAF_TYPE_INT, 1, /* BA_ORIGIN */ bgp_check_origin, bgp_format_origin }, { "as_path", -1, BAF_TRANSITIVE, EAF_TYPE_AS_PATH, 1, /* BA_AS_PATH */ - bgp_check_as_path, NULL }, + NULL, NULL }, /* is checked by validate_as_path() as a special case */ { "next_hop", 4, BAF_TRANSITIVE, EAF_TYPE_IP_ADDRESS, 1, /* BA_NEXT_HOP */ bgp_check_next_hop, NULL }, { "med", 4, BAF_OPTIONAL, EAF_TYPE_INT, 1, /* BA_MULTI_EXIT_DISC */ @@ -1061,73 +1133,6 @@ as4_aggregator_valid(struct adata *aggr) return 0; } -static int -as4_path_sanitize_and_get_length(struct adata *path) -{ - int res = 0; - u8 *p, *dst; - int len, plen, copy; - - dst = p = path->data; - len = path->length; - - while (len) - { - if (len <= 2) /* We do not allow empty segments */ - goto inconsistent_path; - - switch (p[0]) - { - case AS_PATH_SET: - plen = 2 + 4 * p[1]; - copy = 1; - res++; - break; - - case AS_PATH_SEQUENCE: - plen = 2 + 4 * p[1]; - copy = 1; - res += p[1]; - break; - - case AS_PATH_CONFED_SEQUENCE: - case AS_PATH_CONFED_SET: - log(L_WARN "BGP: AS4_PATH attribute contains AS_CONFED_* segment, skipping segment"); - plen = 2 + 4 * p[1]; - copy = 0; - break; - - default: - goto unknown_segment; - } - - if (len < plen) - goto inconsistent_path; - - if (copy) - { - if (dst != p) - memmove(dst, p, plen); - dst += plen; - } - - len -= plen; - p += plen; - } - - path->length = dst - path->data; - return res; - - inconsistent_path: - log(L_WARN "BGP: AS4_PATH attribute is inconsistent, skipping attribute"); - return -1; - - unknown_segment: - log(L_WARN "BGP: AS4_PATH attribute contains unknown segment, skipping attribute"); - return -1; -} - - /* Reconstruct 4B AS_PATH and AGGREGATOR according to RFC 4893 4.2.3 */ static void @@ -1141,7 +1146,7 @@ bgp_reconstruct_4b_atts(struct bgp_proto *p, rta *a, struct linpool *pool) if (a4 && !as4_aggregator_valid(a4->u.ptr)) { - log(L_WARN "BGP: AS4_AGGREGATOR attribute is invalid, skipping attribute"); + log(L_WARN "%s: AS4_AGGREGATOR attribute is invalid, skipping attribute", p->p.name); a4 = NULL; a4_removed = 1; } @@ -1177,15 +1182,18 @@ bgp_reconstruct_4b_atts(struct bgp_proto *p, rta *a, struct linpool *pool) a2->u.ptr = bgp_aggregator_convert_to_new(a2->u.ptr, pool); if ((a2_as == AS_TRANS) && !a4_removed) - log(L_WARN "BGP: AGGREGATOR attribute contain AS_TRANS, but AS4_AGGREGATOR is missing"); + log(L_WARN "%s: AGGREGATOR attribute contain AS_TRANS, but AS4_AGGREGATOR is missing", p->p.name); } } else if (a4) - log(L_WARN "BGP: AS4_AGGREGATOR attribute received, but AGGREGATOR attribute is missing"); + log(L_WARN "%s: AS4_AGGREGATOR attribute received, but AGGREGATOR attribute is missing", p->p.name); int p2_len = as_path_getlen(p2->u.ptr); - int p4_len = p4 ? as4_path_sanitize_and_get_length(p4->u.ptr) : -1; + int p4_len = p4 ? validate_as4_path(p, p4->u.ptr) : -1; + + if (p4 && (p4_len < 0)) + log(L_WARN "%s: AS4_PATH attribute is malformed, skipping attribute", p->p.name); if ((p4_len <= 0) || (p2_len < p4_len)) p2->u.ptr = bgp_merge_as_paths(p2->u.ptr, NULL, AS_PATH_MAXLEN, pool); @@ -1200,7 +1208,7 @@ bgp_remove_as4_attrs(struct bgp_proto *p, rta *a) unsigned id2 = EA_CODE(EAP_BGP, BA_AS4_AGGREGATOR); ea_list **el = &(a->eattrs); - /* We know that ea_lists constructed in bgp_decode_attrs have one attribute per ea_list struct */ + /* We know that ea_lists constructed in bgp_decode attrs have one attribute per ea_list struct */ while (*el != NULL) { unsigned fid = (*el)->attrs[0].id; @@ -1302,6 +1310,12 @@ bgp_decode_attrs(struct bgp_conn *conn, byte *attr, unsigned int len, struct lin if (errcode < 0) continue; } + else if (code == BA_AS_PATH) + { + /* Special case as it might also trim the attribute */ + if (validate_as_path(bgp, z, &l) < 0) + { errcode = 11; goto err; } + } type = desc->type; } else /* Unknown attribute */ @@ -1310,6 +1324,11 @@ bgp_decode_attrs(struct bgp_conn *conn, byte *attr, unsigned int len, struct lin { errcode = 2; goto err; } type = EAF_TYPE_OPAQUE; } + + // Only OPTIONAL and TRANSITIVE attributes may have non-zero PARTIAL flag + // if (!((flags & BAF_OPTIONAL) && (flags & BAF_TRANSITIVE)) && (flags & BAF_PARTIAL)) + // { errcode = 4; goto err; } + seen[code/8] |= (1 << (code%8)); ea = lp_alloc(pool, sizeof(ea_list) + sizeof(eattr)); ea->next = a->eattrs;