be more strict when parsing netmasks for IPv6

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

be more strict when parsing netmasks for IPv6

Claudio Jeker
When parsing a network mask into prefixlen be more paranoid and make sure
no value bigger then 128 is returned. In general this should never happen
but if it does the result can be bad.

This is for bgpd but there are other users in the tree. I will adjust them
if we dicide to go this way.
--
:wq Claudio

Index: kroute.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
retrieving revision 1.225
diff -u -p -r1.225 kroute.c
--- kroute.c 5 Nov 2018 07:01:15 -0000 1.225
+++ kroute.c 19 Nov 2018 12:46:23 -0000
@@ -2406,7 +2406,8 @@ mask2prefixlen(in_addr_t ina)
 u_int8_t
 mask2prefixlen6(struct sockaddr_in6 *sa_in6)
 {
- u_int8_t l = 0, *ap, *ep;
+ u_int8_t *ap, *ep;
+ u_int l = 0;
 
  /*
  * sin6_len is the size of the sockaddr so substract the offset of
@@ -2422,32 +2423,35 @@ mask2prefixlen6(struct sockaddr_in6 *sa_
  break;
  case 0xfe:
  l += 7;
- return (l);
+ goto done;
  case 0xfc:
  l += 6;
- return (l);
+ goto done;
  case 0xf8:
  l += 5;
- return (l);
+ goto done;
  case 0xf0:
  l += 4;
- return (l);
+ goto done;
  case 0xe0:
  l += 3;
- return (l);
+ goto done;
  case 0xc0:
  l += 2;
- return (l);
+ goto done;
  case 0x80:
  l += 1;
- return (l);
+ goto done;
  case 0x00:
- return (l);
+ goto done;
  default:
  fatalx("non contiguous inet6 netmask");
  }
  }
 
+ done:
+ if (l > sizeof(struct in6_addr) * 8)
+ fatalx("%s: prefixlen %d out of bound", __func__, l);
  return (l);
 }
 

Reply | Threaded
Open this post in threaded view
|

Re: be more strict when parsing netmasks for IPv6

Remi Locherer
On Wed, Dec 05, 2018 at 09:22:22AM +0100, Claudio Jeker wrote:
> When parsing a network mask into prefixlen be more paranoid and make sure
> no value bigger then 128 is returned. In general this should never happen
> but if it does the result can be bad.
>
> This is for bgpd but there are other users in the tree. I will adjust them
> if we dicide to go this way.
> --
> :wq Claudio
>

makes sense to me.

OK remi@

> Index: kroute.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
> retrieving revision 1.225
> diff -u -p -r1.225 kroute.c
> --- kroute.c 5 Nov 2018 07:01:15 -0000 1.225
> +++ kroute.c 19 Nov 2018 12:46:23 -0000
> @@ -2406,7 +2406,8 @@ mask2prefixlen(in_addr_t ina)
>  u_int8_t
>  mask2prefixlen6(struct sockaddr_in6 *sa_in6)
>  {
> - u_int8_t l = 0, *ap, *ep;
> + u_int8_t *ap, *ep;
> + u_int l = 0;
>  
>   /*
>   * sin6_len is the size of the sockaddr so substract the offset of
> @@ -2422,32 +2423,35 @@ mask2prefixlen6(struct sockaddr_in6 *sa_
>   break;
>   case 0xfe:
>   l += 7;
> - return (l);
> + goto done;
>   case 0xfc:
>   l += 6;
> - return (l);
> + goto done;
>   case 0xf8:
>   l += 5;
> - return (l);
> + goto done;
>   case 0xf0:
>   l += 4;
> - return (l);
> + goto done;
>   case 0xe0:
>   l += 3;
> - return (l);
> + goto done;
>   case 0xc0:
>   l += 2;
> - return (l);
> + goto done;
>   case 0x80:
>   l += 1;
> - return (l);
> + goto done;
>   case 0x00:
> - return (l);
> + goto done;
>   default:
>   fatalx("non contiguous inet6 netmask");
>   }
>   }
>  
> + done:
> + if (l > sizeof(struct in6_addr) * 8)
> + fatalx("%s: prefixlen %d out of bound", __func__, l);
>   return (l);
>  }
>  
>

Reply | Threaded
Open this post in threaded view
|

Re: be more strict when parsing netmasks for IPv6

Claudio Jeker
On Wed, Dec 05, 2018 at 11:53:48PM +0100, Remi Locherer wrote:

> On Wed, Dec 05, 2018 at 09:22:22AM +0100, Claudio Jeker wrote:
> > When parsing a network mask into prefixlen be more paranoid and make sure
> > no value bigger then 128 is returned. In general this should never happen
> > but if it does the result can be bad.
> >
> > This is for bgpd but there are other users in the tree. I will adjust them
> > if we dicide to go this way.
> > --
> > :wq Claudio
> >
>
> makes sense to me.
>
> OK remi@
>

Here the same diff against other users of mask2prefixlen6().
IIRC there are some other users with different function names which I need
to hunt down (unless someone else wants to do that job).

Iked is a bit special since it returns 0 for non-contiguous netmasks.
Wonder if we should put a fatalx() there too - like in the other daemons.

--
:wq Claudio

Index: sbin/iked/util.c
===================================================================
RCS file: /cvs/src/sbin/iked/util.c,v
retrieving revision 1.36
diff -u -p -r1.36 util.c
--- sbin/iked/util.c 22 Jun 2018 13:20:08 -0000 1.36
+++ sbin/iked/util.c 6 Dec 2018 12:51:14 -0000
@@ -553,7 +553,8 @@ uint8_t
 mask2prefixlen6(struct sockaddr *sa)
 {
  struct sockaddr_in6 *sa_in6 = (struct sockaddr_in6 *)sa;
- uint8_t l = 0, *ap, *ep;
+ uint8_t *ap, *ep;
+ unsigned int l = 0;
 
  /*
  * sin6_len is the size of the sockaddr so substract the offset of
@@ -569,32 +570,35 @@ mask2prefixlen6(struct sockaddr *sa)
  break;
  case 0xfe:
  l += 7;
- return (l);
+ goto done;
  case 0xfc:
  l += 6;
- return (l);
+ goto done;
  case 0xf8:
  l += 5;
- return (l);
+ goto done;
  case 0xf0:
  l += 4;
- return (l);
+ goto done;
  case 0xe0:
  l += 3;
- return (l);
+ goto done;
  case 0xc0:
  l += 2;
- return (l);
+ goto done;
  case 0x80:
  l += 1;
- return (l);
+ goto done;
  case 0x00:
- return (l);
+ goto done;
  default:
  return (0);
  }
  }
 
+done:
+ if (l > sizeof(struct in6_addr) * 8)
+ fatalx("%s: prefixlen %d out of bound", __func__, l);
  return (l);
 }
 
Index: usr.sbin/eigrpd/util.c
===================================================================
RCS file: /cvs/src/usr.sbin/eigrpd/util.c,v
retrieving revision 1.9
diff -u -p -r1.9 util.c
--- usr.sbin/eigrpd/util.c 2 Sep 2016 16:36:33 -0000 1.9
+++ usr.sbin/eigrpd/util.c 6 Dec 2018 14:18:32 -0000
@@ -38,7 +38,8 @@ mask2prefixlen(in_addr_t ina)
 uint8_t
 mask2prefixlen6(struct sockaddr_in6 *sa_in6)
 {
- uint8_t l = 0, *ap, *ep;
+ unsigned int l = 0;
+ uint8_t *ap, *ep;
 
  /*
  * sin6_len is the size of the sockaddr so substract the offset of
@@ -54,32 +55,35 @@ mask2prefixlen6(struct sockaddr_in6 *sa_
  break;
  case 0xfe:
  l += 7;
- return (l);
+ goto done;
  case 0xfc:
  l += 6;
- return (l);
+ goto done;
  case 0xf8:
  l += 5;
- return (l);
+ goto done;
  case 0xf0:
  l += 4;
- return (l);
+ goto done;
  case 0xe0:
  l += 3;
- return (l);
+ goto done;
  case 0xc0:
  l += 2;
- return (l);
+ goto done;
  case 0x80:
  l += 1;
- return (l);
+ goto done;
  case 0x00:
- return (l);
+ goto done;
  default:
  fatalx("non contiguous inet6 netmask");
  }
  }
 
+done:
+ if (l > sizeof(struct in6_addr) * 8)
+ fatalx("inet6 prefixlen out of bound");
  return (l);
 }
 
Index: usr.sbin/ldpd/util.c
===================================================================
RCS file: /cvs/src/usr.sbin/ldpd/util.c,v
retrieving revision 1.4
diff -u -p -r1.4 util.c
--- usr.sbin/ldpd/util.c 23 May 2016 18:58:48 -0000 1.4
+++ usr.sbin/ldpd/util.c 6 Dec 2018 14:19:00 -0000
@@ -37,7 +37,8 @@ mask2prefixlen(in_addr_t ina)
 uint8_t
 mask2prefixlen6(struct sockaddr_in6 *sa_in6)
 {
- uint8_t l = 0, *ap, *ep;
+ unsigned int l = 0;
+ uint8_t *ap, *ep;
 
  /*
  * sin6_len is the size of the sockaddr so substract the offset of
@@ -53,32 +54,35 @@ mask2prefixlen6(struct sockaddr_in6 *sa_
  break;
  case 0xfe:
  l += 7;
- return (l);
+ goto done;
  case 0xfc:
  l += 6;
- return (l);
+ goto done;
  case 0xf8:
  l += 5;
- return (l);
+ goto done;
  case 0xf0:
  l += 4;
- return (l);
+ goto done;
  case 0xe0:
  l += 3;
- return (l);
+ goto done;
  case 0xc0:
  l += 2;
- return (l);
+ goto done;
  case 0x80:
  l += 1;
- return (l);
+ goto done;
  case 0x00:
- return (l);
+ goto done;
  default:
  fatalx("non contiguous inet6 netmask");
  }
  }
 
+done:
+ if (l > sizeof(struct in6_addr) * 8)
+ fatalx("inet6 prefixlen out of bound");
  return (l);
 }
 
Index: usr.sbin/snmpd/kroute.c
===================================================================
RCS file: /cvs/src/usr.sbin/snmpd/kroute.c,v
retrieving revision 1.36
diff -u -p -r1.36 kroute.c
--- usr.sbin/snmpd/kroute.c 10 Oct 2018 11:46:59 -0000 1.36
+++ usr.sbin/snmpd/kroute.c 6 Dec 2018 14:19:24 -0000
@@ -1009,7 +1009,8 @@ prefixlen2mask(u_int8_t prefixlen)
 u_int8_t
 mask2prefixlen6(struct sockaddr_in6 *sa_in6)
 {
- u_int8_t l = 0, *ap, *ep;
+ unsigned int l = 0;
+ u_int8_t *ap, *ep;
 
  /*
  * sin6_len is the size of the sockaddr so substract the offset of
@@ -1025,32 +1026,35 @@ mask2prefixlen6(struct sockaddr_in6 *sa_
  break;
  case 0xfe:
  l += 7;
- return (l);
+ goto done;
  case 0xfc:
  l += 6;
- return (l);
+ goto done;
  case 0xf8:
  l += 5;
- return (l);
+ goto done;
  case 0xf0:
  l += 4;
- return (l);
+ goto done;
  case 0xe0:
  l += 3;
- return (l);
+ goto done;
  case 0xc0:
  l += 2;
- return (l);
+ goto done;
  case 0x80:
  l += 1;
- return (l);
+ goto done;
  case 0x00:
- return (l);
+ goto done;
  default:
  fatalx("non contiguous inet6 netmask");
  }
  }
 
+done:
+ if (l > sizeof(struct in6_addr) * 8)
+ fatalx("inet6 prefixlen out of bound");
  return (l);
 }
 

Reply | Threaded
Open this post in threaded view
|

Re: be more strict when parsing netmasks for IPv6

Florian Obser-2
On Thu, Dec 06, 2018 at 03:24:52PM +0100, Claudio Jeker wrote:

> On Wed, Dec 05, 2018 at 11:53:48PM +0100, Remi Locherer wrote:
> > On Wed, Dec 05, 2018 at 09:22:22AM +0100, Claudio Jeker wrote:
> > > When parsing a network mask into prefixlen be more paranoid and make sure
> > > no value bigger then 128 is returned. In general this should never happen
> > > but if it does the result can be bad.
> > >
> > > This is for bgpd but there are other users in the tree. I will adjust them
> > > if we dicide to go this way.
> > > --
> > > :wq Claudio
> > >
> >
> > makes sense to me.
> >
> > OK remi@
> >
>
> Here the same diff against other users of mask2prefixlen6().
> IIRC there are some other users with different function names which I need
> to hunt down (unless someone else wants to do that job).

rad(8) and slaacd(8) use

int
in6_mask2prefixlen(struct in6_addr *in6)
{
        u_char *nam = (u_char *)in6;
        int byte, bit, plen = 0, size = sizeof(struct in6_addr);

        for (byte = 0; byte < size; byte++, plen += 8)
                if (nam[byte] != 0xff)
                        break;
        if (byte == size)
                return (plen);
        for (bit = 7; bit != 0; bit--, plen++)
                if (!(nam[byte] & (1 << bit)))
                        break;
        for (; bit != 0; bit--)
                if (nam[byte] & (1 << bit))
                        return (0);
        byte++;
        for (; byte < size; byte++)
                if (nam[byte])
                        return (0);
        return (plen);
}

which came from ifconfig where it's called prefix() and is actually AF
independent.

Note that it operates on in6_addr not struct sockaddr_in6...

rad(8) could be easily adapted since it's operating on struct sockaddr_in6
anyway. slaacd(8) is a bit more difficult since it passes struct
in6_addr around.

I'm not sure it's worth the effort though. Its not like one version is
massively better than the other. Having only one version is an
improvement though...

--
I'm not entirely sure you are real.

Reply | Threaded
Open this post in threaded view
|

Re: be more strict when parsing netmasks for IPv6

Claudio Jeker
On Thu, Dec 06, 2018 at 05:14:45PM +0100, Florian Obser wrote:

> On Thu, Dec 06, 2018 at 03:24:52PM +0100, Claudio Jeker wrote:
> > On Wed, Dec 05, 2018 at 11:53:48PM +0100, Remi Locherer wrote:
> > > On Wed, Dec 05, 2018 at 09:22:22AM +0100, Claudio Jeker wrote:
> > > > When parsing a network mask into prefixlen be more paranoid and make sure
> > > > no value bigger then 128 is returned. In general this should never happen
> > > > but if it does the result can be bad.
> > > >
> > > > This is for bgpd but there are other users in the tree. I will adjust them
> > > > if we dicide to go this way.
> > > > --
> > > > :wq Claudio
> > > >
> > >
> > > makes sense to me.
> > >
> > > OK remi@
> > >
> >
> > Here the same diff against other users of mask2prefixlen6().
> > IIRC there are some other users with different function names which I need
> > to hunt down (unless someone else wants to do that job).
>
> rad(8) and slaacd(8) use
>
> int
> in6_mask2prefixlen(struct in6_addr *in6)
> {
> u_char *nam = (u_char *)in6;
> int byte, bit, plen = 0, size = sizeof(struct in6_addr);
>
> for (byte = 0; byte < size; byte++, plen += 8)
> if (nam[byte] != 0xff)
> break;
> if (byte == size)
> return (plen);
> for (bit = 7; bit != 0; bit--, plen++)
> if (!(nam[byte] & (1 << bit)))
> break;
> for (; bit != 0; bit--)
> if (nam[byte] & (1 << bit))
> return (0);
> byte++;
> for (; byte < size; byte++)
> if (nam[byte])
> return (0);
> return (plen);
> }
>
> which came from ifconfig where it's called prefix() and is actually AF
> independent.
>
> Note that it operates on in6_addr not struct sockaddr_in6...
>
> rad(8) could be easily adapted since it's operating on struct sockaddr_in6
> anyway. slaacd(8) is a bit more difficult since it passes struct
> in6_addr around.
>
> I'm not sure it's worth the effort though. Its not like one version is
> massively better than the other. Having only one version is an
> improvement though...

I'm not suggesting to change functions that are working correctly.
At the moment I'm just looking at all those that are similar to the bgpd
one and will adjust those if possible.
The version you pasted returns 0 for any non-consecutive netmask which is not
ideal but that is another story.

--
:wq Claudio

Reply | Threaded
Open this post in threaded view
|

Re: be more strict when parsing netmasks for IPv6

Remi Locherer
In reply to this post by Claudio Jeker
On Thu, Dec 06, 2018 at 03:24:52PM +0100, Claudio Jeker wrote:

> On Wed, Dec 05, 2018 at 11:53:48PM +0100, Remi Locherer wrote:
> > On Wed, Dec 05, 2018 at 09:22:22AM +0100, Claudio Jeker wrote:
> > > When parsing a network mask into prefixlen be more paranoid and make sure
> > > no value bigger then 128 is returned. In general this should never happen
> > > but if it does the result can be bad.
> > >
> > > This is for bgpd but there are other users in the tree. I will adjust them
> > > if we dicide to go this way.
> > > --
> > > :wq Claudio
> > >
> >
> > makes sense to me.
> >
> > OK remi@
> >
>
> Here the same diff against other users of mask2prefixlen6().
> IIRC there are some other users with different function names which I need
> to hunt down (unless someone else wants to do that job).
>
> Iked is a bit special since it returns 0 for non-contiguous netmasks.
> Wonder if we should put a fatalx() there too - like in the other daemons.

I think it should be fatalx() also for iked.

Your diff looks good to me, OK remi@.

Below  the same diff for ospf6d. Feel free to commit it together with
the rest.



cvs diff: Diffing .
Index: util.c
===================================================================
RCS file: /cvs/src/usr.sbin/ospf6d/util.c,v
retrieving revision 1.2
diff -u -p -r1.2 util.c
--- util.c 22 Oct 2012 07:28:49 -0000 1.2
+++ util.c 6 Dec 2018 18:49:51 -0000
@@ -91,7 +91,8 @@ clearscope(struct in6_addr *in6)
 u_int8_t
 mask2prefixlen(struct sockaddr_in6 *sa_in6)
 {
- u_int8_t l = 0, *ap, *ep;
+ u_int8_t *ap, *ep;
+ u_int l = 0;
 
  /*
  * sin6_len is the size of the sockaddr so substract the offset of
@@ -107,32 +108,35 @@ mask2prefixlen(struct sockaddr_in6 *sa_i
  break;
  case 0xfe:
  l += 7;
- return (l);
+ goto done;
  case 0xfc:
  l += 6;
- return (l);
+ goto done;
  case 0xf8:
  l += 5;
- return (l);
+ goto done;
  case 0xf0:
  l += 4;
- return (l);
+ goto done;
  case 0xe0:
  l += 3;
- return (l);
+ goto done;
  case 0xc0:
  l += 2;
- return (l);
+ goto done;
  case 0x80:
  l += 1;
- return (l);
+ goto done;
  case 0x00:
- return (l);
+ goto done;
  default:
  fatalx("non contiguous inet6 netmask");
  }
  }
 
+done:
+ if (l > sizeof(struct in6_addr) * 8)
+ fatalx("%s: prefixlen %d out of bound", __func__, l);
  return (l);
 }
 

Reply | Threaded
Open this post in threaded view
|

Re: be more strict when parsing netmasks for IPv6

Claudio Jeker
On Thu, Dec 06, 2018 at 08:04:54PM +0100, Remi Locherer wrote:

> On Thu, Dec 06, 2018 at 03:24:52PM +0100, Claudio Jeker wrote:
> > On Wed, Dec 05, 2018 at 11:53:48PM +0100, Remi Locherer wrote:
> > > On Wed, Dec 05, 2018 at 09:22:22AM +0100, Claudio Jeker wrote:
> > > > When parsing a network mask into prefixlen be more paranoid and make sure
> > > > no value bigger then 128 is returned. In general this should never happen
> > > > but if it does the result can be bad.
> > > >
> > > > This is for bgpd but there are other users in the tree. I will adjust them
> > > > if we dicide to go this way.
> > > > --
> > > > :wq Claudio
> > > >
> > >
> > > makes sense to me.
> > >
> > > OK remi@
> > >
> >
> > Here the same diff against other users of mask2prefixlen6().
> > IIRC there are some other users with different function names which I need
> > to hunt down (unless someone else wants to do that job).
> >
> > Iked is a bit special since it returns 0 for non-contiguous netmasks.
> > Wonder if we should put a fatalx() there too - like in the other daemons.
>
> I think it should be fatalx() also for iked.
>
> Your diff looks good to me, OK remi@.
>
> Below  the same diff for ospf6d. Feel free to commit it together with
> the rest.
>

Na, you can do that one :). OK claudio@
 

>
> cvs diff: Diffing .
> Index: util.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ospf6d/util.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 util.c
> --- util.c 22 Oct 2012 07:28:49 -0000 1.2
> +++ util.c 6 Dec 2018 18:49:51 -0000
> @@ -91,7 +91,8 @@ clearscope(struct in6_addr *in6)
>  u_int8_t
>  mask2prefixlen(struct sockaddr_in6 *sa_in6)
>  {
> - u_int8_t l = 0, *ap, *ep;
> + u_int8_t *ap, *ep;
> + u_int l = 0;
>  
>   /*
>   * sin6_len is the size of the sockaddr so substract the offset of
> @@ -107,32 +108,35 @@ mask2prefixlen(struct sockaddr_in6 *sa_i
>   break;
>   case 0xfe:
>   l += 7;
> - return (l);
> + goto done;
>   case 0xfc:
>   l += 6;
> - return (l);
> + goto done;
>   case 0xf8:
>   l += 5;
> - return (l);
> + goto done;
>   case 0xf0:
>   l += 4;
> - return (l);
> + goto done;
>   case 0xe0:
>   l += 3;
> - return (l);
> + goto done;
>   case 0xc0:
>   l += 2;
> - return (l);
> + goto done;
>   case 0x80:
>   l += 1;
> - return (l);
> + goto done;
>   case 0x00:
> - return (l);
> + goto done;
>   default:
>   fatalx("non contiguous inet6 netmask");
>   }
>   }
>  
> +done:
> + if (l > sizeof(struct in6_addr) * 8)
> + fatalx("%s: prefixlen %d out of bound", __func__, l);
>   return (l);
>  }
>  
>

--
:wq Claudio