RIPv1 subnet bugs + patch

[John Hay]

I'm using various versions of FreeBSD, but the bug is not related to an
OS. The patch is against zebra 0.93b, but I had a look in the cvs repo
and the ripd code looks the same there.

There are two bugs in the RIPv1 code in ripd. Both are found when
using subnet masks that are not the same as the "natural" mask of
a network, for example if you have a class A or B network and subnet
it to 24 bits.

The first is on the output side. When ripd has to broadcast the subnets
that it has learned, it will mask all of them to the natural mask, so
if you use the 10.0.0.0 network with /24 subnets, instead of seeing
all your 10.x.x.0 subnets being broadcast, you see all of them being
broadscast as 10.0.0.0, with their original metric. RFC1058 says that
RIPv1 assumes that all subnets in a network are using the same mask and
that it should broadcast the subnets as long as it stays inside the
network. Other networks should be broadcast with their "natural" mask.
In my patch I use the address and mask of the outgoing "interface" to
determine in which network we are and if we are subnetted. Only subnets
that are part of that network and have the same mask as the outgoing
"interface" are broadcast. Other networks are only broadcast with
their natural mask and I try to only output one per network.

The second bug is on the input side. The current code tries to subnet
on 8 bit boundaries and it also try to subnet any network. RFC1058
says that you should not make assumptions about other networks than
your own, so you should not try to subnet other networks and also
should not accept host routes from other networks. You should subnet
your own network though. I have decided to use the input interface's
address and mask to get the network and to determine if the network
is subnetted and what mask should be applied to subnets in our network.

With this patch I think ripd should be pretty close to RFC1058. Much
closer then it was in anycase.

There are still two issues that I know about that I haven't tried to
address in this patch.

The first, which is just an optimization, is that on the input side,
if you have a route to a subnet and receive a host route with the same
metric and nexthop and you could just thow the host route away.

The second is that I think zebra or at least ripd's idea of an interface
is too narrow. If you have a machine with two interfaces and you run
two subnets on one of those interfaces, ripd should exchange rip
info between those two interfaces, but it doesn't. it only do between
each of those subnets and the other side.

Subnets A and B              Subnet C
------------------| ripd |-----------------

In other words, it will exchange rip between A and C and also between
B and C, but not between A and B.

With this patch I have been ably to use zebra/ripd with the other
routers on our campus network, which use the class B 146.64.0.0/16
subnetted to /24. The network consists of various vendor routers
and also some FreeBSD boxes running gated 3.5.x and 3.6 and the
routed that ships with FreeBSD.

Anybody that is interested in the source of the routed that shipped
with FreeBSD 1.1.5 in 1994, I have a copy. :-)

John
-- 
John Hay -- address@hidden / address@hidden


--- ./lib/if.c.orig     Fri Jun 21 23:49:50 2002
+++ ./lib/if.c  Mon Oct 14 17:25:42 2002
@@ -553,6 +553,65 @@
   return NULL;
 }
 
+/* Find the IPv4 address on our side that will be used when packets
+   are sent to dst. */
+struct connected *
+connected_lookup_address (struct interface *ifp, struct in_addr dst)
+{
+  struct prefix addr;
+  struct prefix best;
+  listnode cnode;
+  struct prefix *p;
+  struct connected *c;
+  struct connected *match;
+
+  /* Zero structures - get rid of rubbish from stack */
+  memset(&addr, 0, sizeof(addr));
+  memset(&best, 0, sizeof(best));
+
+  addr.family = AF_INET;
+  addr.u.prefix4 = dst;
+  addr.prefixlen = IPV4_MAX_BITLEN;
+
+  match = NULL;
+
+  for (cnode = listhead (ifp->connected); cnode; nextnode (cnode))
+    {
+      c = getdata (cnode);
+
+      if (if_is_pointopoint (ifp))
+       {
+         p = c->address;
+
+         if (p && p->family == AF_INET)
+           {
+#ifdef OLD_RIB  /* PTP  links are conventionally identified 
+                   by the address of the far end - MAG */
+             if (IPV4_ADDR_SAME (&p->u.prefix4, &dst))
+               return c;
+#endif
+             p = c->destination;
+             if (p && IPV4_ADDR_SAME (&p->u.prefix4, &dst))
+               return c;
+           }
+       }
+      else
+       {
+         p = c->address;
+
+         if (p->family == AF_INET)
+           {
+             if (prefix_match (p, &addr) && p->prefixlen > best.prefixlen)
+               {
+                 best = *p;
+                 match = c;
+               }
+           }
+       }
+    }
+  return match;
+}
+
 /* Check the connected information is PtP style or not.  */
 int
 ifc_pointopoint (struct connected *ifc)
--- ./lib/if.h.orig     Fri Jun 21 23:49:50 2002
+++ ./lib/if.h  Mon Oct 14 17:25:42 2002
@@ -202,6 +202,7 @@
 void connected_free (struct connected *);
 void connected_add (struct interface *, struct connected *);
 struct connected  *connected_delete_by_prefix (struct interface *, struct 
prefix *);
+struct connected  *connected_lookup_address (struct interface *, struct 
in_addr);
 int ifc_pointopoint (struct connected *);
 
 #ifndef HAVE_IF_NAMETOINDEX
--- ./ripd/ripd.c.orig  Mon Jul  1 02:57:22 2002
+++ ./ripd/ripd.c       Tue Oct 15 10:02:11 2002
@@ -55,8 +55,8 @@
 /* Prototypes. */
 void rip_event (enum rip_event, int);
 
-void rip_output_process (struct interface *, struct sockaddr_in *, 
-                        int, u_char);
+void rip_output_process (struct interface *, struct prefix *,
+                        struct sockaddr_in *, int, u_char);
 
 /* RIP output routes type. */
 enum
@@ -955,7 +955,14 @@
 {
   caddr_t lim;
   struct rte *rte;
+  struct prefix_ipv4 ifaddr;
+  struct prefix_ipv4 ifaddrclass;
+  struct connected *c;
+  int subnetted;
       
+  /* We don't know yet. */
+  subnetted = -1;
+
   /* The Response must be ignored if it is not from the RIP
      port. (RFC2453 - Sec. 3.9.2)*/
   if (ntohs (from->sin_port) != RIP_PORT_DEFAULT) 
@@ -1108,23 +1115,51 @@
        {
          u_int32_t destination;
 
-         destination = ntohl (rte->prefix.s_addr);
-
-         if (destination & 0xff) 
+         if (subnetted == -1)
            {
-             masklen2ip (32, &rte->mask);
+             c = connected_lookup_address (ifp, from->sin_addr);
+             if (c != NULL)
+               {
+                 memcpy (&ifaddr, c->address, sizeof (struct prefix_ipv4));
+                 memcpy (&ifaddrclass, &ifaddr, sizeof (struct prefix_ipv4));
+                 apply_classful_mask_ipv4 (&ifaddrclass);
+                 subnetted = 0;
+                 if (ifaddr.prefixlen > ifaddrclass.prefixlen)
+                   subnetted = 1;
+               }
            }
-         else if ((destination & 0xff00) || IN_CLASSC (destination)) 
-           {
+
+         destination = ntohl (rte->prefix.s_addr);
+
+         if (IN_CLASSA (destination))
+             masklen2ip (8, &rte->mask);
+         else if (IN_CLASSB (destination))
+             masklen2ip (16, &rte->mask);
+         else if (IN_CLASSC (destination))
              masklen2ip (24, &rte->mask);
+
+         if (subnetted == 1)
+           masklen2ip (ifaddrclass.prefixlen,
+                       (struct in_addr *) &destination);
+         if ((subnetted == 1) && ((rte->prefix.s_addr & destination) ==
+             ifaddrclass.prefix.s_addr))
+           {
+             masklen2ip (ifaddr.prefixlen, &rte->mask);
+             if ((rte->prefix.s_addr & rte->mask.s_addr) != rte->prefix.s_addr)
+               masklen2ip (32, &rte->mask);
+             if (IS_RIP_DEBUG_EVENT)
+               zlog_info ("Subnetted route %s", inet_ntoa (rte->prefix));
            }
-         else if ((destination & 0xff0000) || IN_CLASSB (destination)) 
+         else
            {
-             masklen2ip (16, &rte->mask);
+             if ((rte->prefix.s_addr & rte->mask.s_addr) != rte->prefix.s_addr)
+               continue;
            }
-         else 
+
+         if (IS_RIP_DEBUG_EVENT)
            {
-             masklen2ip (8, &rte->mask);
+             zlog_info ("Resultant route %s", inet_ntoa (rte->prefix));
+             zlog_info ("Resultant mask %s", inet_ntoa (rte->mask));
            }
        }
 
@@ -1353,7 +1388,7 @@
       ntohl (rte->metric) == RIP_METRIC_INFINITY)
     {  
       /* All route with split horizon */
-      rip_output_process (ifp, from, rip_all_route, packet->version);
+      rip_output_process (ifp, NULL, from, rip_all_route, packet->version);
     }
   else
     {
@@ -1884,8 +1919,8 @@
 
 /* Send update to the ifp or spcified neighbor. */
 void
-rip_output_process (struct interface *ifp, struct sockaddr_in *to,
-                   int route_type, u_char version)
+rip_output_process (struct interface *ifp, struct prefix *ifaddr,
+                   struct sockaddr_in *to, int route_type, u_char version)
 {
   int ret;
   struct stream *s;
@@ -1894,8 +1929,11 @@
   struct rip_interface *ri;
   struct prefix_ipv4 *p;
   struct prefix_ipv4 classfull;
+  struct prefix_ipv4 ifaddrclass;
+  struct connected *c;
   int num;
   int rtemax;
+  int subnetted;
 
   /* Logging output event. */
   if (IS_RIP_DEBUG_EVENT)
@@ -1946,29 +1984,60 @@
          rtemax -=1;
     }
 
+  if (version == RIPv1)
+    {
+      if (ifaddr == NULL)
+       {
+         c = connected_lookup_address (ifp, to->sin_addr);
+         if (c != NULL)
+           ifaddr = c->address;
+       }
+      if (ifaddr == NULL)
+       {
+         zlog_warn ("cannot find source address for packets to neighbor %s",
+                    inet_ntoa (to->sin_addr));
+         return;
+       }
+      memcpy (&ifaddrclass, ifaddr, sizeof (struct prefix_ipv4));
+      apply_classful_mask_ipv4 (&ifaddrclass);
+      subnetted = 0;
+      if (ifaddr->prefixlen > ifaddrclass.prefixlen)
+       subnetted = 1;
+    }
+
   for (rp = route_top (rip->table); rp; rp = route_next (rp))
     if ((rinfo = rp->info) != NULL)
       {
-       /* Some inheritance stuff:                                          */
-       /* Before we process with ipv4 prefix we should mask it             */
-       /* with Classful mask if we send RIPv1 packet.That's because        */
-       /* user could set non-classful mask or we could get it by RIPv2     */
-       /* or other protocol. checked with Cisco's way of life :)           */
+       /* For RIPv1, if we are subnetted, output subnets in our network    */
+       /* that have the same mask as the output "interface". For other     */
+       /* networks, only the classfull version is output.                  */
        
        if (version == RIPv1)
          {
-           memcpy (&classfull, &rp->p, sizeof (struct prefix_ipv4));
+           p = (struct prefix_ipv4 *) &rp->p;
 
            if (IS_RIP_DEBUG_PACKET)
-             zlog_info("%s/%d before RIPv1 mask check ",
-                       inet_ntoa (classfull.prefix), classfull.prefixlen);
-
-           apply_classful_mask_ipv4 (&classfull);
-           p = &classfull;
+             zlog_info("RIPv1 mask check, %s/%d considered for output",
+                       inet_ntoa (rp->p.u.prefix4), rp->p.prefixlen);
 
+           if (subnetted &&
+               prefix_match ((struct prefix *) &ifaddrclass, &rp->p))
+             {
+               if ((ifaddr->prefixlen != rp->p.prefixlen) &&
+                   (rp->p.prefixlen != 32))
+                 continue;
+             }
+           else
+             {
+               memcpy (&classfull, &rp->p, sizeof(struct prefix_ipv4));
+               apply_classful_mask_ipv4(&classfull);
+               if (rp->p.u.prefix4.s_addr != 0 &&
+                   classfull.prefixlen != rp->p.prefixlen)
+                 continue;
+             }
            if (IS_RIP_DEBUG_PACKET)
-             zlog_info("%s/%d after RIPv1 mask check",
-                       inet_ntoa (p->prefix), p->prefixlen);
+             zlog_info("RIPv1 mask check, %s/%d made it through",
+                       inet_ntoa (rp->p.u.prefix4), rp->p.prefixlen);
          }
        else 
          p = (struct prefix_ipv4 *) &rp->p;
@@ -2109,7 +2178,7 @@
       if (IS_RIP_DEBUG_EVENT)
        zlog_info ("multicast announce on %s ", ifp->name);
 
-      rip_output_process (ifp, NULL, route_type, version);
+      rip_output_process (ifp, NULL, NULL, route_type, version);
       return;
     }
 
@@ -2136,7 +2205,8 @@
                           if_is_pointopoint (ifp) ? "unicast" : "broadcast",
                           inet_ntoa (to.sin_addr), ifp->name);
 
-             rip_output_process (ifp, &to, route_type, version);
+             rip_output_process (ifp, connected->address, &to, route_type,
+                                 version);
            }
        }
     }
@@ -2224,7 +2294,7 @@
        to.sin_port = htons (RIP_PORT_DEFAULT);
 
        /* RIP version is rip's configuration. */
-       rip_output_process (ifp, &to, route_type, rip->version);
+       rip_output_process (ifp, NULL, &to, route_type, rip->version);
       }
 }