bgpd: refine source-as matching

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

bgpd: refine source-as matching

Claudio Jeker
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.
--
:wq Claudio

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 12:58:57 -0000
@@ -320,7 +320,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,26 +330,34 @@ 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->op, as, match, f->as_min, f->as_max))
+ 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 (seg[0] == AS_SET)
+ /* use aggregator AS per rfc6472 */
+ if (preas)
+ as = preas;
  if (as_compare(f->op, as, match, f->as_min, f->as_max))
  return (1);
  else
@@ -389,7 +397,7 @@ as_compare(u_int8_t op, u_int32_t as, u_
 /*
  * 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 +405,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));

Reply | Threaded
Open this post in threaded view
|

Re: bgpd: refine source-as matching

Sebastian Benoit-3
Claudio Jeker([hidden email]) on 2018.08.09 15:10:11 +0200:

> 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.
> --
> :wq Claudio

ok benno@

with two comments on comments which you can choose to ignore.

>
> 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 12:58:57 -0000
> @@ -320,7 +320,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,26 +330,34 @@ aspath_match(void *data, u_int16_t len,
>   }
>  
>   seg = data;
> - for (; len > 0; len -= seg_size, seg += seg_size) {
> +

In my head, the peer-as is the last as (that is added to the path), not the
first. Because i learned in school that as-paths are read from right to
left:

> + /* just check the first (leftmost) AS */

  /* just check the last (leftmost) AS */

> + if (f->type == AS_PEER && len >= 6) {
> + as = aspath_extract(seg, 0);
> + if (as_compare(f->op, as, match, f->as_min, f->as_max))
> + 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 */

  /* just check the origin (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 (seg[0] == AS_SET)
> + /* use aggregator AS per rfc6472 */
> + if (preas)
> + as = preas;
>   if (as_compare(f->op, as, match, f->as_min, f->as_max))
>   return (1);
>   else
> @@ -389,7 +397,7 @@ as_compare(u_int8_t op, u_int32_t as, u_
>  /*
>   * 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 +405,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));
>

Reply | Threaded
Open this post in threaded view
|

Re: bgpd: refine source-as matching

Job Snijders-2
In reply to this post by Claudio Jeker
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.

Indeed, good find!

> 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.

tested:

        $ bgpctl show rib source-as 8530 5.39.176.0/21
        flags: * = Valid, > = Selected, I = via IBGP, A = Announced, S = Stale
        origin: i = IGP, e = EGP, ? = Incomplete

        flags destination          gateway          lpref   med aspath origin
        I*>   5.39.176.0/21        192.147.168.1      100     0 2914 8530 { 198753 } ?

OK job@

Kind regards,

Job

Reply | Threaded
Open this post in threaded view
|

Re: bgpd: refine source-as matching

Claudio Jeker
In reply to this post by Claudio Jeker
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?
--
: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));

Reply | Threaded
Open this post in threaded view
|

Re: bgpd: refine source-as matching

Sebastian Benoit-3
Claudio Jeker([hidden email]) 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.

i have to admit that we check for AS 0 in the path somewhere else.
still...


> --
> :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));
>

Reply | Threaded
Open this post in threaded view
|

Re: bgpd: refine source-as matching

Claudio Jeker-3
On Thu, Aug 09, 2018 at 10:37:50PM +0200, Sebastian Benoit wrote:

> Claudio Jeker([hidden email]) 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