On Thu, Aug 09, 2018 at 10:37:50PM +0200, Sebastian Benoit wrote: > Claudio Jeker(cje...@diehard.n-r-g.com) on 2018.08.09 21:59:16 +0200: > > On Thu, Aug 09, 2018 at 03:10:11PM +0200, Claudio Jeker wrote: > > > Per rfc6472 AS_SET should no longer be used but some AS still do. > > > Until now source-as would take the rightmost AS number of an AS_PATH no > > > matter if it was an AS_SEQUENCE or an AS_SET. Thit is not correct. Also > > > because AS_SET are used in aggregation source-as should match against the > > > aggregator AS (which it the rightmost as of the previous AS_PATH segment). > > > At the same time move the peer-as check out of the loop, there is no > > > reason to have that inside the for loop. > > > > > > This diff implements this and also makes aspath_extract() and the lenght > > > checks a bit more paranoid. > > > > OK, updated diff here that adds three things. > > a) it passes the filter_as struct to as_compare > > b) it ensures that if for whatever crazy reason AS 0 is passed to the > > filter it will not match (no matter what). This is more paranoia. > > I decided to do that since util.c is shared between bgpd and bgpctl > > and so the easy way of calling fatalx() is not an option here. > > c) fix the souce-as 42 >< 4242 case. Currently the >< operator is not > > correctly implemented it is more a <> as in inside but excluding the > > edges. I think 42 >< 4242 should match for AS 1 - 41 and 4243 and > > higher. > > > > Is this still OK? > > hm. yes. ok. > > although this will cause bizarre effects: > > deny from any AS 0 > > wont match even for as == 0. >
Yes, this rule should never match since there should be never an AS 0 in any path that makes it to the filter. This is why it is kind of valid to ingore that rule completly and move to the next one. > i have to admit that we check for AS 0 in the path somewhere else. > still... Yes, aspath with a 0 AS number in them are soft rejected by aspath_verify. I currently don't want to disallow AS 0 in the parse.y because ROA may need it in one way or the other. But even there it is a make sure it never matches thing. Once I have those bits together it may be possible to restrict this further. In the end having aspath_extract return 0 is in all current usecases of bgpd impossible. There is always some additional code that ensures that there is no overflow but we tripped into that hole at least once and I would prefer to not do it again. In general I would throw in a fatalx() there but as mentioned that won't fly in bgpctl which uses this code as well. I will commit this without the aspath_extract() chunk and the as == 0 check. Will put that into a new diff for later. > > -- > > :wq Claudio > > > > > > Index: bgpd.h > > =================================================================== > > RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v > > retrieving revision 1.329 > > diff -u -p -r1.329 bgpd.h > > --- bgpd.h 8 Aug 2018 14:29:05 -0000 1.329 > > +++ bgpd.h 9 Aug 2018 19:43:39 -0000 > > @@ -1156,8 +1156,6 @@ int aspath_snprint(char *, size_t, voi > > int aspath_asprint(char **, void *, u_int16_t); > > size_t aspath_strlen(void *, u_int16_t); > > int aspath_match(void *, u_int16_t, struct filter_as *, > > u_int32_t); > > -int as_compare(u_int8_t, u_int32_t, u_int32_t, u_int32_t, > > - u_int32_t); > > u_int32_t aspath_extract(const void *, int); > > int aspath_verify(void *, u_int16_t, int); > > #define AS_ERR_LEN -1 > > Index: util.c > > =================================================================== > > RCS file: /cvs/src/usr.sbin/bgpd/util.c,v > > retrieving revision 1.28 > > diff -u -p -r1.28 util.c > > --- util.c 22 Jul 2018 16:52:27 -0000 1.28 > > +++ util.c 9 Aug 2018 19:38:34 -0000 > > @@ -312,6 +312,22 @@ aspath_strlen(void *data, u_int16_t len) > > return (total_size); > > } > > > > +static int > > +as_compare(struct filter_as *f, u_int32_t as, u_int32_t match) > > +{ > > + if (as == 0) /* AS 0 is invalid and therefor never matches */ > > + return (0); > > + if ((f->op == OP_NONE || f->op == OP_EQ) && as == match) > > + return (1); > > + else if (f->op == OP_NE && as != match) > > + return (1); > > + else if (f->op == OP_RANGE && as >= f->as_min && as <= f->as_max) > > + return (1); > > + else if (f->op == OP_XRANGE && (as < f->as_min || as > f->as_max)) > > + return (1); > > + return (0); > > +} > > + > > /* we need to be able to search more than one as */ > > int > > aspath_match(void *data, u_int16_t len, struct filter_as *f, u_int32_t > > match) > > @@ -320,7 +336,7 @@ aspath_match(void *data, u_int16_t len, > > int final; > > u_int16_t seg_size; > > u_int8_t i, seg_len; > > - u_int32_t as; > > + u_int32_t as = 0, preas; > > > > if (f->type == AS_EMPTY) { > > if (len == 0) > > @@ -330,27 +346,35 @@ aspath_match(void *data, u_int16_t len, > > } > > > > seg = data; > > - for (; len > 0; len -= seg_size, seg += seg_size) { > > + > > + /* just check the first (leftmost) AS */ > > + if (f->type == AS_PEER && len >= 6) { > > + as = aspath_extract(seg, 0); > > + if (as_compare(f, as, match)) > > + return (1); > > + else > > + return (0); > > + } > > + > > + for (; len >= 6; len -= seg_size, seg += seg_size) { > > seg_len = seg[1]; > > seg_size = 2 + sizeof(u_int32_t) * seg_len; > > > > final = (len == seg_size); > > > > - /* just check the first (leftmost) AS */ > > - if (f->type == AS_PEER) { > > - as = aspath_extract(seg, 0); > > - if (as_compare(f->op, as, match, f->as_min, f->as_max)) > > - return (1); > > - else > > - return (0); > > - } > > /* just check the final (rightmost) AS */ > > if (f->type == AS_SOURCE) { > > + /* extract the rightmost AS and keep the one before */ > > + preas = as; > > + as = aspath_extract(seg, seg_len - 1); > > /* not yet in the final segment */ > > if (!final) > > continue; > > - as = aspath_extract(seg, seg_len - 1); > > - if (as_compare(f->op, as, match, f->as_min, f->as_max)) > > + if (seg[0] == AS_SET) > > + /* use aggregator AS per rfc6472 */ > > + if (preas) > > + as = preas; > > + if (as_compare(f, as, match)) > > return (1); > > else > > return (0); > > @@ -364,32 +388,17 @@ aspath_match(void *data, u_int16_t len, > > if (final && i == seg_len - 1 && f->type == AS_TRANSIT) > > return (0); > > as = aspath_extract(seg, i); > > - if (as_compare(f->op, as, match, f->as_min, f->as_max)) > > + if (as_compare(f, as, match)) > > return (1); > > } > > } > > return (0); > > } > > > > -int > > -as_compare(u_int8_t op, u_int32_t as, u_int32_t match, u_int32_t as_min, > > - u_int32_t as_max) > > -{ > > - if ((op == OP_NONE || op == OP_EQ) && as == match) > > - return (1); > > - else if (op == OP_NE && as != match) > > - return (1); > > - else if (op == OP_RANGE && as >= as_min && as <= as_max) > > - return (1); > > - else if (op == OP_XRANGE && as > as_min && as < as_max) > > - return (1); > > - return (0); > > -} > > - > > /* > > * Extract the asnum out of the as segment at the specified position. > > * Direct access is not possible because of non-aligned reads. > > - * ATTENTION: no bounds checks are done. > > + * Only works on verified and 4byte ASN paths. > > */ > > u_int32_t > > aspath_extract(const void *seg, int pos) > > @@ -397,6 +406,9 @@ aspath_extract(const void *seg, int pos) > > const u_char *ptr = seg; > > u_int32_t as; > > > > + /* minimal overflow check, return 0 since that is an invalid ASN */ > > + if (pos >= ptr[1]) > > + return (0); > > ptr += 2 + sizeof(u_int32_t) * pos; > > memcpy(&as, ptr, sizeof(u_int32_t)); > > return (ntohl(as)); > > > -- :wq Claudio