Mailing List Archive

On Thu, 3 Jun 2004, JJ Ludman wrote:

> Currently, Quagga's ripd does not interoperate with the in.routed
> delivered in Solaris (at least). in.routed sets the authentication
> length field to 16, as defined by RFC2082.

> Currently, ripd rejects rip route response packets with the auth
> length field set to 16 (or anything other than what Cisco sets it
> to, 20). This patch changes ripd to accept any value greater than
> or equal to 16, and on outgoing packets sets this field to 16.
> I've tested this patch against Cisco and Solaris' in.routed.
>
> Does this seem reasonable?

Just about yes, but older ripd's will cease to work though.

> thanks,
> -jj

> + */
> + if (md5->auth_len < RIP_AUTH_MD5_SIZE)

How about an upper bound or:

!( (md5->auth_len == RIP_AUTH_MD5_SIZE)
|| (md5->auth_len == RIP_AUTH_MD5_SIZE + RIP_HEADER_SIZE))

the former is the interpretation the RFC seems to suggest, the latter
is the Cisco and ripd interpretation and there are no other possible
interpretations, right?

Alternatively, we can zlog_warn and set auth_len to a guaranteed sane
size and continue and hope for best.

> - md5_process_bytes (packet, packet_len + md5->auth_len, &ctx);
> + md5_process_bytes (packet, packet_len + RIP_HEADER_SIZE + RIP_AUTH_MD5_SIZE, \
> + &ctx);

Hmm, probably should be packet_len + md5->auth_len + RIP_HEADER_SIZE,
except for this darn problem of buggy clients. Should never be
anything other than packet_len + RIP_HEADER_SIZE + RIP_AUTH_MD5_SIZE
right?

> - stream_putc (s, RIP_AUTH_MD5_SIZE + RIP_HEADER_SIZE);
> + stream_putc (s, RIP_AUTH_MD5_SIZE);

This will break older ripd clients though. This needs to be
configurable. I'll hack together a patch.

regards,
--
Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
warning: do not ever send email to spam@dishone.st
Fortune:
"You can't make a program without broken egos."

On Fri, 4 Jun 2004, Paul Jakma wrote:

> This will break older ripd clients though. This needs to be
> configurable. I'll hack together a patch.

See below. It adds a

auth-length key WORD <0-2147483647> (rfc|old-ripd)

command under RIP_NODE, to select compatibility. It doesnt yet save
these key settings though, the value is stored in the key, and the
keychains write themselves out.

Also, more sanity checking added:

rip_auth_md5:

- Take a length argument (of entire packet) for sanity checking.

- Accept only RIP_AUTH_MD5_SIZE or RIP_AUTH_MD5_COMPAT_SIZE md5->auth_len's.

- verify size md5->packet_len field, this was completely unchecked
previously AFAICT, and would be passed on to md5_process_bytes - a
trivial DoS.

- verify the 2 2-byte fields which preceed the auth-data, these
correspond to address family and tag in a normal RTE. Not sure
whether it's more appropriate to return in error or try continue.

rip_auth_md5_set:

- use the key's rip_auth_len field to decide which md5->auth_data
size to write to the packet

rip_read:

- check that fromlen and len returned from recvfrom are equal. the
function checks for upper bound on len, so this wasnt actually
exploitable. pass length to rip_auth_md5.

ripd.h:

Add RIP_AUTH_MD5_COMPAT_SIZE define for RIP_RTE_SIZE.

NB: compiles, not yet tested.

regards,
--
Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
warning: do not ever send email to spam@dishone.st
Fortune:
No house should ever be on any hill or on anything. It should be of the hill,
belonging to it.
-- Frank Lloyd Wright


? ripd/.nfs000ec28400000043
Index: lib/keychain.h
===================================================================
RCS file: /var/cvsroot/quagga/lib/keychain.h,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 keychain.h
--- lib/keychain.h 13 Dec 2002 20:15:29 -0000 1.1.1.1
+++ lib/keychain.h 4 Jun 2004 04:43:33 -0000
@@ -45,6 +45,8 @@

struct key_range send;
struct key_range accept;
+
+ u_int8_t rip_auth_len; /* hack: to allow compatibility with old ripds */
};

void keychain_init ();
Index: ripd/ripd.c
===================================================================
RCS file: /var/cvsroot/quagga/ripd/ripd.c,v
retrieving revision 1.23
diff -u -r1.23 ripd.c
--- ripd/ripd.c 4 Jun 2004 01:42:38 -0000 1.23
+++ ripd/ripd.c 4 Jun 2004 04:43:35 -0000
@@ -830,8 +830,8 @@

/* RIP version 2 authentication with MD5. */
int
-rip_auth_md5 (struct rip_packet *packet, struct sockaddr_in *from,
- struct interface *ifp)
+rip_auth_md5 (struct rip_packet *packet, struct sockaddr_in *from,
+ int length, struct interface *ifp)
{
struct rip_interface *ri;
struct rip_md5_info *md5;
@@ -854,16 +854,41 @@
if (ri->auth_type != RIP_AUTH_MD5 || ntohs (md5->type) != RIP_AUTH_MD5)
return 0;

-/*
- * If the authentication length is less than 16, then it must be wrong for
- * any interpretation of rfc2082.
- */
- if (md5->auth_len < RIP_AUTH_MD5_SIZE)
+ /* If the authentication length is less than 16, then it must be wrong for
+ * any interpretation of rfc2082. Some implementations also interpret
+ * this as RIP_HEADER_SIZE+ RIP_AUTH_MD5_SIZE, aka RIP_AUTH_MD5_COMPAT_SIZE.
+ */
+ if ( !( (md5->auth_len == RIP_AUTH_MD5_SIZE)
+ || (md5->auth_len == RIP_AUTH_MD5_COMPAT_SIZE)))
{
if (IS_RIP_DEBUG_EVENT)
- zlog_info ("RIPv2 MD5 authentication, authentication length field too \
- short");
- return 0;
+ zlog_warn ("RIPv2 MD5 authentication, strange authentication "
+ "length field %d",
+ md5->auth_len);
+ return 0;
+ }
+
+ /* grab and verify check packet length */
+ packet_len = ntohs (md5->packet_len);
+
+ if ( packet_len > (length - RIP_HEADER_SIZE - RIP_AUTH_MD5_SIZE) )
+ {
+ if (IS_RIP_DEBUG_EVENT)
+ zlog_warn ("RIPv2 MD5 authentication, packet length field %d "
+ "greater than received length %d!",
+ md5->packet_len, length);
+ return 0;
+ }
+
+ /* retrieve authentication data */
+ md5data = (struct rip_md5_data *)(((u_char *) packet) + packet_len);
+
+ /* Verify MD5 'family' and 'type' */
+ if ( ! ((md5data->family == 0xffff) && (md5data->type == 0x1)) )
+ {
+ zlog_warn ("RIPv2 MD5 authentication, MD5 family/type mismatch: "
+ "%d / %d", md5data->family, md5data->type);
+ return 0;
}

if (ri->key_chain)
@@ -886,9 +911,7 @@
return 0;

/* MD5 digest authentication. */
- packet_len = ntohs (md5->packet_len);
- md5data = (struct rip_md5_data *)(((u_char *) packet) + packet_len);
-
+
/* Save digest to pdigest. */
memcpy (pdigest, md5data->digest, RIP_AUTH_MD5_SIZE);

@@ -897,8 +920,8 @@
strncpy ((char *)md5data->digest, auth_str, RIP_AUTH_MD5_SIZE);

md5_init_ctx (&ctx);
- md5_process_bytes (packet, packet_len + RIP_HEADER_SIZE + RIP_AUTH_MD5_SIZE, \
- &ctx);
+ md5_process_bytes (packet, packet_len + RIP_HEADER_SIZE + RIP_AUTH_MD5_SIZE,
+ &ctx);
md5_finish_ctx (&ctx, digest);

if (memcmp (pdigest, digest, RIP_AUTH_MD5_SIZE) == 0)
@@ -980,9 +1003,13 @@
else
stream_putc (s, 1);

- /* Auth Data Len. Set 16 for MD5 authentication
- data. */
- stream_putc (s, RIP_AUTH_MD5_SIZE);
+ /* Auth Data Len. Set 16 for MD5 authentication data.
+ * Older ripds however expect RIP_HEADER_SIZE + RIP_AUTH_MD5_SIZE
+ */
+ if (key->rip_auth_len == RIP_AUTH_MD5_COMPAT_SIZE)
+ stream_putc (s, RIP_AUTH_MD5_COMPAT_SIZE);
+ else
+ stream_putc (s, RIP_AUTH_MD5_SIZE);

/* Sequence Number (non-decreasing). */
/* RFC2080: The value used in the sequence number is
@@ -1720,6 +1747,12 @@
rip_peer_bad_packet (&from);
return len;
}
+ if (len != fromlen)
+ {
+ zlog_warn ("message size %d is not equal to buffer length %d",
+ len, fromlen);
+ return -1;
+ }

/* Packet alignment check. */
if ((len - RIP_PACKET_MINSIZ) % 20)
@@ -1849,7 +1882,7 @@
}
else if (ntohs (packet->rte->tag) == RIP_AUTH_MD5)
{
- ret = rip_auth_md5 (packet, &from, ifp);
+ ret = rip_auth_md5 (packet, &from, len, ifp);
if (! ret)
{
if (IS_RIP_DEBUG_EVENT)
@@ -2809,6 +2842,71 @@
return CMD_SUCCESS;
}

+DEFUN (rip_key_auth_length,
+ rip_key_auth_length_cmd,
+ "auth-length key WORD <0-2147483647> (rfc|old-ripd)",
+ "RIP Authentication length compatibility setting\n"
+ "Key-chain name\n"
+ "Key identifier number\n"
+ "RFC length (16) or old ripd compatible length (20)\n")
+{
+ struct keychain *keychain;
+ struct key *key;
+ char *endptr = NULL;
+ unsigned long int index;
+
+ if ( (argc < 2) || (argc > 3) )
+ {
+ vty_out (vty, "Incorrect number of argument%s", VTY_NEWLINE);
+ return CMD_WARNING;
+ }
+
+ if ( (keychain = keychain_lookup (argv[0])) == NULL)
+ {
+ vty_out (vty, "Can't find keychain %s%s", argv[0], VTY_NEWLINE);
+ return CMD_WARNING;
+ }
+
+ index = strtoul (argv[1], &endptr, 10);
+ if (index == ULONG_MAX || *endptr != '\0')
+ {
+ vty_out (vty, "Key identifier number error%s", VTY_NEWLINE);
+ return CMD_WARNING;
+ }
+
+ key = key_lookup_for_accept (keychain, index);
+
+ if (! key)
+ {
+ vty_out (vty, "Can't find key %d%s", index, VTY_NEWLINE);
+ return CMD_WARNING;
+ }
+
+ /* negate command */
+ if (argc == 2)
+ {
+ key->rip_auth_len = RIP_AUTH_MD5_SIZE;
+ return CMD_SUCCESS;
+ }
+
+ if (strncmp (argv[2], "r", 1) == 0)
+ key->rip_auth_len = RIP_AUTH_MD5_SIZE;
+ else if (strncmp (argv[2], "o", 1) == 0)
+ key->rip_auth_len = RIP_AUTH_MD5_COMPAT_SIZE;
+ else
+ return CMD_WARNING;
+
+ return CMD_SUCCESS;
+}
+
+ALIAS (rip_key_auth_length,
+ no_rip_key_auth_length_cmd,
+ "no auth-length key WORD <0-2147483647>",
+ NO_STR
+ "RIP Authentication length compatibility setting\n"
+ "Key-chain name\n"
+ "Key identifier number\n")
+
DEFUN (rip_version,
rip_version_cmd,
"version <1-2>",
@@ -3947,6 +4045,8 @@
install_element (CONFIG_NODE, &no_router_rip_cmd);

install_default (RIP_NODE);
+ install_element (RIP_NODE, &rip_key_auth_length_cmd);
+ install_element (RIP_NODE, &no_rip_key_auth_length_cmd);
install_element (RIP_NODE, &rip_version_cmd);
install_element (RIP_NODE, &no_rip_version_cmd);
install_element (RIP_NODE, &no_rip_version_val_cmd);
Index: ripd/ripd.h
===================================================================
RCS file: /var/cvsroot/quagga/ripd/ripd.h,v
retrieving revision 1.9
diff -u -r1.9 ripd.h
--- ripd/ripd.h 23 Jan 2004 15:31:42 -0000 1.9
+++ ripd/ripd.h 4 Jun 2004 04:43:35 -0000
@@ -80,6 +80,7 @@

/* RIP MD5 authentication. */
#define RIP_AUTH_MD5_SIZE 16
+#define RIP_AUTH_MD5_COMPAT_SIZE RIP_RTE_SIZE

/* RIP structure. */
struct rip

* Paul Jakma (paul@clubi.ie) wrote:
> On Fri, 4 Jun 2004, Paul Jakma wrote:
>
> > This will break older ripd clients though. This needs to be
> > configurable. I'll hack together a patch.
>

True.

> See below. It adds a
>
> auth-length key WORD <0-2147483647> (rfc|old-ripd)
>
> command under RIP_NODE, to select compatibility. It doesnt yet save
> these key settings though, the value is stored in the key, and the
> keychains write themselves out.
>

Seems strange to be storing them inside the key. What's the argument for this
instead of an additional argument to "ip rip authentication mode md5"?

The keychain code is only used by ripd, anyway so I think it's okay to
live there.

cheers,
-jj

On Fri, 4 Jun 2004, JJ Ludman wrote:

> True.

> Seems strange to be storing them inside the key. What's the
> argument for this instead of an additional argument to "ip rip
> authentication mode md5"?

yes, it is a bit strange.. the idea being that one could still
interoperate with both in.routed and old-ripd on the same interface,
which wouldnt be possible with a per-interface setting, however,
looking at it, ripd doesnt handle interfaces that reference keychains
with multiple valid keys - first valid key is used. so this idea has
little merit.

i'm not even sure how multiple key's on an interface are supposed to
work. ??

> The keychain code is only used by ripd, anyway so I think it's okay to
> live there.

well, given above, it's probably better just to make it an interface
setting.

> cheers,
> -jj

regards,
--
Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
warning: do not ever send email to spam@dishone.st
Fortune:
Santa Claus is watching!

* Paul Jakma (paul@clubi.ie) wrote:
> On Fri, 4 Jun 2004, JJ Ludman wrote:
>
> > True.
>
> > Seems strange to be storing them inside the key. What's the
> > argument for this instead of an additional argument to "ip rip
> > authentication mode md5"?
>
> yes, it is a bit strange.. the idea being that one could still
> interoperate with both in.routed and old-ripd on the same interface,
> which wouldnt be possible with a per-interface setting, however,
> looking at it, ripd doesnt handle interfaces that reference keychains
> with multiple valid keys - first valid key is used. so this idea has
> little merit.
>
> i'm not even sure how multiple key's on an interface are supposed to
> work. ??
>

Looks like it's just a transitioning mechanism for when your keys are
about to expire. As far as I know, all implementations accept the md5
authentication length of 20, so you can interoperate with old-ripd and
cisco by simply configuring that (once it's implemented :) ). The interop
problem with in.routed was just that old-ripd didn't accept in.routed's
authentication length. It works the other way around.

Would it be better to just let it continue to use 20? This battle
is lost, and it doesn't really matter, since everyone's already adapted
to Cisco's misinterpretation. I can put out another patch this afternoon
for either the hard-coded 20, or per-interface configurable...

cheers,
-jj

> > The keychain code is only used by ripd, anyway so I think it's okay to
> > live there.
>
> well, given above, it's probably better just to make it an interface
> setting.
>
> > cheers,
> > -jj
>
> regards,
> --
> Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
> warning: do not ever send email to spam@dishone.st
> Fortune:
> Santa Claus is watching!

On Fri, 4 Jun 2004, JJ Ludman wrote:

> Looks like it's just a transitioning mechanism for when your keys
> are about to expire.

Oh that should work yes. But you cant have multiple valid keys, or at
least, you can, but it will simply use first one. What I dont know is
whether RIPv2 has any provisions for having multiple keys active at
the same time, ie could you send the same packet multiple times out
an interface once for each valid key with each appropriate MD5 hash
and keyid?

> As far as I know, all implementations accept the md5 authentication
> length of 20, so you can interoperate with old-ripd and cisco by
> simply configuring that (once it's implemented :) ). The interop
> problem with in.routed was just that old-ripd didn't accept
> in.routed's authentication length. It works the other way around.

Yes, would have thought so.

> Would it be better to just let it continue to use 20? This battle
> is lost, and it doesn't really matter, since everyone's already
> adapted to Cisco's misinterpretation.

Be liberal in what you accept, strict in what you send. Compatible
with IOS is fine, but following it bug for bug... ;) Let's go with
in.routed's behaviour and allow for backwards compatibility with
older ripd's.

> I can put out another patch this afternoon for either the
> hard-coded 20, or per-interface configurable...

See below - previous patch building on yours but with the old-ripd
compat setting as an interface setting, not key setting.

> cheers,
> -jj

regards,
--
Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
warning: do not ever send email to spam@dishone.st
Fortune:
Hating the Yankees is as American as pizza pie, unwed mothers and
cheating on your income tax.
-- Mike Royko

? ripd/.nfs000ec28400000043
Index: ripd/rip_interface.c
===================================================================
RCS file: /var/cvsroot/quagga/ripd/rip_interface.c,v
retrieving revision 1.15
diff -u -r1.15 rip_interface.c
--- ripd/rip_interface.c 8 May 2004 11:48:26 -0000 1.15
+++ ripd/rip_interface.c 4 Jun 2004 16:35:56 -0000
@@ -125,6 +125,7 @@
compatibility. */
/* ri->auth_type = RIP_NO_AUTH; */
ri->auth_type = RIP_AUTH_SIMPLE_PASSWORD;
+ ri->md5_auth_len = RIP_AUTH_MD5_SIZE;

/* Set default split-horizon behavior. If the interface is Frame
Relay or SMDS is enabled, the default value for split-horizon is
@@ -1678,6 +1679,12 @@
ifp = (struct interface *)vty->index;
ri = ifp->info;

+ if ( (argc < 1) || (argc > 2) )
+ {
+ vty_out (vty, "incorrect argument count%s", VTY_NEWLINE);
+ return CMD_WARNING;
+ }
+
if (strncmp ("md5", argv[0], strlen (argv[0])) == 0)
ri->auth_type = RIP_AUTH_MD5;
else if (strncmp ("text", argv[0], strlen (argv[0])) == 0)
@@ -1687,10 +1694,39 @@
vty_out (vty, "mode should be md5 or text%s", VTY_NEWLINE);
return CMD_WARNING;
}
+
+ if (argc == 1)
+ return CMD_SUCCESS;
+
+ if ( (argc == 2) && (ri->auth_type != RIP_AUTH_MD5) )
+ {
+ vty_out (vty, "auth length argument only valid for md5%s", VTY_NEWLINE);
+ return CMD_WARNING;
+ }
+
+ if (strncmp ("r", argv[1], 1) == 0)
+ ri->md5_auth_len = RIP_AUTH_MD5_SIZE;
+ else if (strncmp ("o", argv[1], 1) == 0)
+ ri->md5_auth_len = RIP_AUTH_MD5_COMPAT_SIZE;
+ else
+ return CMD_WARNING;

return CMD_SUCCESS;
}

+ALIAS (ip_rip_authentication_mode,
+ ip_rip_authentication_mode_authlen_cmd,
+ "ip rip authentication mode (md5|text) auth-length (rfc|old-ripd)",
+ IP_STR
+ "Routing Information Protocol\n"
+ "Authentication control\n"
+ "Authentication mode\n"
+ "Keyed message digest\n"
+ "Clear text authentication\n"
+ "MD5 authentication data length\n"
+ "RFC compatible\n"
+ "Old ripd compatible\n")
+
DEFUN (no_ip_rip_authentication_mode,
no_ip_rip_authentication_mode_cmd,
"no ip rip authentication mode",
@@ -1708,7 +1744,8 @@

/* ri->auth_type = RIP_NO_AUTH; */
ri->auth_type = RIP_AUTH_SIMPLE_PASSWORD;
-
+ ri->md5_auth_len = RIP_AUTH_MD5_SIZE;
+
return CMD_SUCCESS;
}

@@ -1723,6 +1760,20 @@
"Keyed message digest\n"
"Clear text authentication\n")

+ALIAS (no_ip_rip_authentication_mode,
+ no_ip_rip_authentication_mode_type_authlen_cmd,
+ "no ip rip authentication mode (md5|text) auth-length (rfc|old-ripd)",
+ NO_STR
+ IP_STR
+ "Routing Information Protocol\n"
+ "Authentication control\n"
+ "Authentication mode\n"
+ "Keyed message digest\n"
+ "Clear text authentication\n"
+ "MD5 authentication data length\n"
+ "RFC compatible\n"
+ "Old ripd compatible\n")
+
DEFUN (ip_rip_authentication_string,
ip_rip_authentication_string_cmd,
"ip rip authentication string LINE",
@@ -1988,6 +2039,7 @@
(ri->ri_send == RI_RIP_UNSPEC) &&
(ri->ri_receive == RI_RIP_UNSPEC) &&
(ri->auth_type != RIP_AUTH_MD5) &&
+ (ri->md5_auth_len != RIP_AUTH_MD5_SIZE) &&
(!ri->auth_str) &&
(!ri->key_chain) )
continue;
@@ -2034,8 +2086,14 @@
if (ri->auth_type == RIP_AUTH_SIMPLE_PASSWORD)
vty_out (vty, " ip rip authentication mode text%s", VTY_NEWLINE);
#endif /* 0 */
+
if (ri->auth_type == RIP_AUTH_MD5)
- vty_out (vty, " ip rip authentication mode md5%s", VTY_NEWLINE);
+ {
+ vty_out (vty, " ip rip authentication mode md5");
+ if (ri->md5_auth_len == RIP_AUTH_MD5_COMPAT_SIZE)
+ vty_out (vty, " auth-length old-ripd");
+ vty_out (vty, "%s", VTY_NEWLINE);
+ }

if (ri->auth_str)
vty_out (vty, " ip rip authentication string %s%s",
@@ -2165,8 +2223,10 @@
install_element (INTERFACE_NODE, &no_ip_rip_receive_version_num_cmd);

install_element (INTERFACE_NODE, &ip_rip_authentication_mode_cmd);
+ install_element (INTERFACE_NODE, &ip_rip_authentication_mode_authlen_cmd);
install_element (INTERFACE_NODE, &no_ip_rip_authentication_mode_cmd);
install_element (INTERFACE_NODE, &no_ip_rip_authentication_mode_type_cmd);
+ install_element (INTERFACE_NODE, &no_ip_rip_authentication_mode_type_authlen_cmd);

install_element (INTERFACE_NODE, &ip_rip_authentication_key_chain_cmd);
install_element (INTERFACE_NODE, &no_ip_rip_authentication_key_chain_cmd);
Index: ripd/ripd.c
===================================================================
RCS file: /var/cvsroot/quagga/ripd/ripd.c,v
retrieving revision 1.23
diff -u -r1.23 ripd.c
--- ripd/ripd.c 4 Jun 2004 01:42:38 -0000 1.23
+++ ripd/ripd.c 4 Jun 2004 16:35:58 -0000
@@ -830,8 +830,8 @@

/* RIP version 2 authentication with MD5. */
int
-rip_auth_md5 (struct rip_packet *packet, struct sockaddr_in *from,
- struct interface *ifp)
+rip_auth_md5 (struct rip_packet *packet, struct sockaddr_in *from,
+ int length, struct interface *ifp)
{
struct rip_interface *ri;
struct rip_md5_info *md5;
@@ -854,16 +854,41 @@
if (ri->auth_type != RIP_AUTH_MD5 || ntohs (md5->type) != RIP_AUTH_MD5)
return 0;

-/*
- * If the authentication length is less than 16, then it must be wrong for
- * any interpretation of rfc2082.
- */
- if (md5->auth_len < RIP_AUTH_MD5_SIZE)
+ /* If the authentication length is less than 16, then it must be wrong for
+ * any interpretation of rfc2082. Some implementations also interpret
+ * this as RIP_HEADER_SIZE+ RIP_AUTH_MD5_SIZE, aka RIP_AUTH_MD5_COMPAT_SIZE.
+ */
+ if ( !( (md5->auth_len == RIP_AUTH_MD5_SIZE)
+ || (md5->auth_len == RIP_AUTH_MD5_COMPAT_SIZE)))
{
if (IS_RIP_DEBUG_EVENT)
- zlog_info ("RIPv2 MD5 authentication, authentication length field too \
- short");
- return 0;
+ zlog_warn ("RIPv2 MD5 authentication, strange authentication "
+ "length field %d",
+ md5->auth_len);
+ return 0;
+ }
+
+ /* grab and verify check packet length */
+ packet_len = ntohs (md5->packet_len);
+
+ if ( packet_len > (length - RIP_HEADER_SIZE - RIP_AUTH_MD5_SIZE) )
+ {
+ if (IS_RIP_DEBUG_EVENT)
+ zlog_warn ("RIPv2 MD5 authentication, packet length field %d "
+ "greater than received length %d!",
+ md5->packet_len, length);
+ return 0;
+ }
+
+ /* retrieve authentication data */
+ md5data = (struct rip_md5_data *)(((u_char *) packet) + packet_len);
+
+ /* Verify MD5 'family' and 'type' */
+ if ( ! ((md5data->family == 0xffff) && (md5data->type == 0x1)) )
+ {
+ zlog_warn ("RIPv2 MD5 authentication, MD5 family/type mismatch: "
+ "%d / %d", md5data->family, md5data->type);
+ return 0;
}

if (ri->key_chain)
@@ -886,9 +911,7 @@
return 0;

/* MD5 digest authentication. */
- packet_len = ntohs (md5->packet_len);
- md5data = (struct rip_md5_data *)(((u_char *) packet) + packet_len);
-
+
/* Save digest to pdigest. */
memcpy (pdigest, md5data->digest, RIP_AUTH_MD5_SIZE);

@@ -897,8 +920,8 @@
strncpy ((char *)md5data->digest, auth_str, RIP_AUTH_MD5_SIZE);

md5_init_ctx (&ctx);
- md5_process_bytes (packet, packet_len + RIP_HEADER_SIZE + RIP_AUTH_MD5_SIZE, \
- &ctx);
+ md5_process_bytes (packet, packet_len + RIP_HEADER_SIZE + RIP_AUTH_MD5_SIZE,
+ &ctx);
md5_finish_ctx (&ctx, digest);

if (memcmp (pdigest, digest, RIP_AUTH_MD5_SIZE) == 0)
@@ -980,9 +1003,11 @@
else
stream_putc (s, 1);

- /* Auth Data Len. Set 16 for MD5 authentication
- data. */
- stream_putc (s, RIP_AUTH_MD5_SIZE);
+ /* Auth Data Len. Set 16 for MD5 authentication data. Older ripds
+ * however expect RIP_HEADER_SIZE + RIP_AUTH_MD5_SIZE so we allow for this
+ * to be configurable.
+ */
+ stream_putc (s, ri->md5_auth_len);

/* Sequence Number (non-decreasing). */
/* RFC2080: The value used in the sequence number is
@@ -1720,6 +1745,12 @@
rip_peer_bad_packet (&from);
return len;
}
+ if (len != fromlen)
+ {
+ zlog_warn ("message size %d is not equal to buffer length %d",
+ len, fromlen);
+ return -1;
+ }

/* Packet alignment check. */
if ((len - RIP_PACKET_MINSIZ) % 20)
@@ -1849,7 +1880,7 @@
}
else if (ntohs (packet->rte->tag) == RIP_AUTH_MD5)
{
- ret = rip_auth_md5 (packet, &from, ifp);
+ ret = rip_auth_md5 (packet, &from, len, ifp);
if (! ret)
{
if (IS_RIP_DEBUG_EVENT)
Index: ripd/ripd.h
===================================================================
RCS file: /var/cvsroot/quagga/ripd/ripd.h,v
retrieving revision 1.9
diff -u -r1.9 ripd.h
--- ripd/ripd.h 23 Jan 2004 15:31:42 -0000 1.9
+++ ripd/ripd.h 4 Jun 2004 16:35:59 -0000
@@ -80,6 +80,7 @@

/* RIP MD5 authentication. */
#define RIP_AUTH_MD5_SIZE 16
+#define RIP_AUTH_MD5_COMPAT_SIZE RIP_RTE_SIZE

/* RIP structure. */
struct rip
@@ -248,6 +249,9 @@

/* RIPv2 authentication key chain. */
char *key_chain;
+
+ /* value to use for md5->auth_len */
+ u_int8_t md5_auth_len;

/* Split horizon flag. */
split_horizon_policy_t split_horizon;

Looks good to me. thanks!

cheers,
-jj

* Paul Jakma (paul@clubi.ie) wrote:
> On Fri, 4 Jun 2004, JJ Ludman wrote:
>
> > Looks like it's just a transitioning mechanism for when your keys
> > are about to expire.
>
> Oh that should work yes. But you cant have multiple valid keys, or at
> least, you can, but it will simply use first one. What I dont know is
> whether RIPv2 has any provisions for having multiple keys active at
> the same time, ie could you send the same packet multiple times out
> an interface once for each valid key with each appropriate MD5 hash
> and keyid?
>
> > As far as I know, all implementations accept the md5 authentication
> > length of 20, so you can interoperate with old-ripd and cisco by
> > simply configuring that (once it's implemented :) ). The interop
> > problem with in.routed was just that old-ripd didn't accept
> > in.routed's authentication length. It works the other way around.
>
> Yes, would have thought so.
>
> > Would it be better to just let it continue to use 20? This battle
> > is lost, and it doesn't really matter, since everyone's already
> > adapted to Cisco's misinterpretation.
>
> Be liberal in what you accept, strict in what you send. Compatible
> with IOS is fine, but following it bug for bug... ;) Let's go with
> in.routed's behaviour and allow for backwards compatibility with
> older ripd's.
>
> > I can put out another patch this afternoon for either the
> > hard-coded 20, or per-interface configurable...
>
> See below - previous patch building on yours but with the old-ripd
> compat setting as an interface setting, not key setting.
>
> > cheers,
> > -jj
>
> regards,
> --
> Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
> warning: do not ever send email to spam@dishone.st
> Fortune:
> Hating the Yankees is as American as pizza pie, unwed mothers and
> cheating on your income tax.
> -- Mike Royko
>
> ? ripd/.nfs000ec28400000043
> Index: ripd/rip_interface.c
> ===================================================================
> RCS file: /var/cvsroot/quagga/ripd/rip_interface.c,v
> retrieving revision 1.15
> diff -u -r1.15 rip_interface.c
> --- ripd/rip_interface.c 8 May 2004 11:48:26 -0000 1.15
> +++ ripd/rip_interface.c 4 Jun 2004 16:35:56 -0000
> @@ -125,6 +125,7 @@
> compatibility. */
> /* ri->auth_type = RIP_NO_AUTH; */
> ri->auth_type = RIP_AUTH_SIMPLE_PASSWORD;
> + ri->md5_auth_len = RIP_AUTH_MD5_SIZE;
>
> /* Set default split-horizon behavior. If the interface is Frame
> Relay or SMDS is enabled, the default value for split-horizon is
> @@ -1678,6 +1679,12 @@
> ifp = (struct interface *)vty->index;
> ri = ifp->info;
>
> + if ( (argc < 1) || (argc > 2) )
> + {
> + vty_out (vty, "incorrect argument count%s", VTY_NEWLINE);
> + return CMD_WARNING;
> + }
> +
> if (strncmp ("md5", argv[0], strlen (argv[0])) == 0)
> ri->auth_type = RIP_AUTH_MD5;
> else if (strncmp ("text", argv[0], strlen (argv[0])) == 0)
> @@ -1687,10 +1694,39 @@
> vty_out (vty, "mode should be md5 or text%s", VTY_NEWLINE);
> return CMD_WARNING;
> }
> +
> + if (argc == 1)
> + return CMD_SUCCESS;
> +
> + if ( (argc == 2) && (ri->auth_type != RIP_AUTH_MD5) )
> + {
> + vty_out (vty, "auth length argument only valid for md5%s", VTY_NEWLINE);
> + return CMD_WARNING;
> + }
> +
> + if (strncmp ("r", argv[1], 1) == 0)
> + ri->md5_auth_len = RIP_AUTH_MD5_SIZE;
> + else if (strncmp ("o", argv[1], 1) == 0)
> + ri->md5_auth_len = RIP_AUTH_MD5_COMPAT_SIZE;
> + else
> + return CMD_WARNING;
>
> return CMD_SUCCESS;
> }
>
> +ALIAS (ip_rip_authentication_mode,
> + ip_rip_authentication_mode_authlen_cmd,
> + "ip rip authentication mode (md5|text) auth-length (rfc|old-ripd)",
> + IP_STR
> + "Routing Information Protocol\n"
> + "Authentication control\n"
> + "Authentication mode\n"
> + "Keyed message digest\n"
> + "Clear text authentication\n"
> + "MD5 authentication data length\n"
> + "RFC compatible\n"
> + "Old ripd compatible\n")
> +
> DEFUN (no_ip_rip_authentication_mode,
> no_ip_rip_authentication_mode_cmd,
> "no ip rip authentication mode",
> @@ -1708,7 +1744,8 @@
>
> /* ri->auth_type = RIP_NO_AUTH; */
> ri->auth_type = RIP_AUTH_SIMPLE_PASSWORD;
> -
> + ri->md5_auth_len = RIP_AUTH_MD5_SIZE;
> +
> return CMD_SUCCESS;
> }
>
> @@ -1723,6 +1760,20 @@
> "Keyed message digest\n"
> "Clear text authentication\n")
>
> +ALIAS (no_ip_rip_authentication_mode,
> + no_ip_rip_authentication_mode_type_authlen_cmd,
> + "no ip rip authentication mode (md5|text) auth-length (rfc|old-ripd)",
> + NO_STR
> + IP_STR
> + "Routing Information Protocol\n"
> + "Authentication control\n"
> + "Authentication mode\n"
> + "Keyed message digest\n"
> + "Clear text authentication\n"
> + "MD5 authentication data length\n"
> + "RFC compatible\n"
> + "Old ripd compatible\n")
> +
> DEFUN (ip_rip_authentication_string,
> ip_rip_authentication_string_cmd,
> "ip rip authentication string LINE",
> @@ -1988,6 +2039,7 @@
> (ri->ri_send == RI_RIP_UNSPEC) &&
> (ri->ri_receive == RI_RIP_UNSPEC) &&
> (ri->auth_type != RIP_AUTH_MD5) &&
> + (ri->md5_auth_len != RIP_AUTH_MD5_SIZE) &&
> (!ri->auth_str) &&
> (!ri->key_chain) )
> continue;
> @@ -2034,8 +2086,14 @@
> if (ri->auth_type == RIP_AUTH_SIMPLE_PASSWORD)
> vty_out (vty, " ip rip authentication mode text%s", VTY_NEWLINE);
> #endif /* 0 */
> +
> if (ri->auth_type == RIP_AUTH_MD5)
> - vty_out (vty, " ip rip authentication mode md5%s", VTY_NEWLINE);
> + {
> + vty_out (vty, " ip rip authentication mode md5");
> + if (ri->md5_auth_len == RIP_AUTH_MD5_COMPAT_SIZE)
> + vty_out (vty, " auth-length old-ripd");
> + vty_out (vty, "%s", VTY_NEWLINE);
> + }
>
> if (ri->auth_str)
> vty_out (vty, " ip rip authentication string %s%s",
> @@ -2165,8 +2223,10 @@
> install_element (INTERFACE_NODE, &no_ip_rip_receive_version_num_cmd);
>
> install_element (INTERFACE_NODE, &ip_rip_authentication_mode_cmd);
> + install_element (INTERFACE_NODE, &ip_rip_authentication_mode_authlen_cmd);
> install_element (INTERFACE_NODE, &no_ip_rip_authentication_mode_cmd);
> install_element (INTERFACE_NODE, &no_ip_rip_authentication_mode_type_cmd);
> + install_element (INTERFACE_NODE, &no_ip_rip_authentication_mode_type_authlen_cmd);
>
> install_element (INTERFACE_NODE, &ip_rip_authentication_key_chain_cmd);
> install_element (INTERFACE_NODE, &no_ip_rip_authentication_key_chain_cmd);
> Index: ripd/ripd.c
> ===================================================================
> RCS file: /var/cvsroot/quagga/ripd/ripd.c,v
> retrieving revision 1.23
> diff -u -r1.23 ripd.c
> --- ripd/ripd.c 4 Jun 2004 01:42:38 -0000 1.23
> +++ ripd/ripd.c 4 Jun 2004 16:35:58 -0000
> @@ -830,8 +830,8 @@
>
> /* RIP version 2 authentication with MD5. */
> int
> -rip_auth_md5 (struct rip_packet *packet, struct sockaddr_in *from,
> - struct interface *ifp)
> +rip_auth_md5 (struct rip_packet *packet, struct sockaddr_in *from,
> + int length, struct interface *ifp)
> {
> struct rip_interface *ri;
> struct rip_md5_info *md5;
> @@ -854,16 +854,41 @@
> if (ri->auth_type != RIP_AUTH_MD5 || ntohs (md5->type) != RIP_AUTH_MD5)
> return 0;
>
> -/*
> - * If the authentication length is less than 16, then it must be wrong for
> - * any interpretation of rfc2082.
> - */
> - if (md5->auth_len < RIP_AUTH_MD5_SIZE)
> + /* If the authentication length is less than 16, then it must be wrong for
> + * any interpretation of rfc2082. Some implementations also interpret
> + * this as RIP_HEADER_SIZE+ RIP_AUTH_MD5_SIZE, aka RIP_AUTH_MD5_COMPAT_SIZE.
> + */
> + if ( !( (md5->auth_len == RIP_AUTH_MD5_SIZE)
> + || (md5->auth_len == RIP_AUTH_MD5_COMPAT_SIZE)))
> {
> if (IS_RIP_DEBUG_EVENT)
> - zlog_info ("RIPv2 MD5 authentication, authentication length field too \
> - short");
> - return 0;
> + zlog_warn ("RIPv2 MD5 authentication, strange authentication "
> + "length field %d",
> + md5->auth_len);
> + return 0;
> + }
> +
> + /* grab and verify check packet length */
> + packet_len = ntohs (md5->packet_len);
> +
> + if ( packet_len > (length - RIP_HEADER_SIZE - RIP_AUTH_MD5_SIZE) )
> + {
> + if (IS_RIP_DEBUG_EVENT)
> + zlog_warn ("RIPv2 MD5 authentication, packet length field %d "
> + "greater than received length %d!",
> + md5->packet_len, length);
> + return 0;
> + }
> +
> + /* retrieve authentication data */
> + md5data = (struct rip_md5_data *)(((u_char *) packet) + packet_len);
> +
> + /* Verify MD5 'family' and 'type' */
> + if ( ! ((md5data->family == 0xffff) && (md5data->type == 0x1)) )
> + {
> + zlog_warn ("RIPv2 MD5 authentication, MD5 family/type mismatch: "
> + "%d / %d", md5data->family, md5data->type);
> + return 0;
> }
>
> if (ri->key_chain)
> @@ -886,9 +911,7 @@
> return 0;
>
> /* MD5 digest authentication. */
> - packet_len = ntohs (md5->packet_len);
> - md5data = (struct rip_md5_data *)(((u_char *) packet) + packet_len);
> -
> +
> /* Save digest to pdigest. */
> memcpy (pdigest, md5data->digest, RIP_AUTH_MD5_SIZE);
>
> @@ -897,8 +920,8 @@
> strncpy ((char *)md5data->digest, auth_str, RIP_AUTH_MD5_SIZE);
>
> md5_init_ctx (&ctx);
> - md5_process_bytes (packet, packet_len + RIP_HEADER_SIZE + RIP_AUTH_MD5_SIZE, \
> - &ctx);
> + md5_process_bytes (packet, packet_len + RIP_HEADER_SIZE + RIP_AUTH_MD5_SIZE,
> + &ctx);
> md5_finish_ctx (&ctx, digest);
>
> if (memcmp (pdigest, digest, RIP_AUTH_MD5_SIZE) == 0)
> @@ -980,9 +1003,11 @@
> else
> stream_putc (s, 1);
>
> - /* Auth Data Len. Set 16 for MD5 authentication
> - data. */
> - stream_putc (s, RIP_AUTH_MD5_SIZE);
> + /* Auth Data Len. Set 16 for MD5 authentication data. Older ripds
> + * however expect RIP_HEADER_SIZE + RIP_AUTH_MD5_SIZE so we allow for this
> + * to be configurable.
> + */
> + stream_putc (s, ri->md5_auth_len);
>
> /* Sequence Number (non-decreasing). */
> /* RFC2080: The value used in the sequence number is
> @@ -1720,6 +1745,12 @@
> rip_peer_bad_packet (&from);
> return len;
> }
> + if (len != fromlen)
> + {
> + zlog_warn ("message size %d is not equal to buffer length %d",
> + len, fromlen);
> + return -1;
> + }
>
> /* Packet alignment check. */
> if ((len - RIP_PACKET_MINSIZ) % 20)
> @@ -1849,7 +1880,7 @@
> }
> else if (ntohs (packet->rte->tag) == RIP_AUTH_MD5)
> {
> - ret = rip_auth_md5 (packet, &from, ifp);
> + ret = rip_auth_md5 (packet, &from, len, ifp);
> if (! ret)
> {
> if (IS_RIP_DEBUG_EVENT)
> Index: ripd/ripd.h
> ===================================================================
> RCS file: /var/cvsroot/quagga/ripd/ripd.h,v
> retrieving revision 1.9
> diff -u -r1.9 ripd.h
> --- ripd/ripd.h 23 Jan 2004 15:31:42 -0000 1.9
> +++ ripd/ripd.h 4 Jun 2004 16:35:59 -0000
> @@ -80,6 +80,7 @@
>
> /* RIP MD5 authentication. */
> #define RIP_AUTH_MD5_SIZE 16
> +#define RIP_AUTH_MD5_COMPAT_SIZE RIP_RTE_SIZE
>
> /* RIP structure. */
> struct rip
> @@ -248,6 +249,9 @@
>
> /* RIPv2 authentication key chain. */
> char *key_chain;
> +
> + /* value to use for md5->auth_len */
> + u_int8_t md5_auth_len;
>
> /* Split horizon flag. */
> split_horizon_policy_t split_horizon;
> _______________________________________________
> Quagga-dev mailing list
> Quagga-dev@lists.quagga.net
> http://lists.quagga.net/mailman/listinfo/quagga-dev

On Fri, 4 Jun 2004, Paul Jakma wrote:

> - check that fromlen and len returned from recvfrom are equal.
> the function checks for upper bound on len, so this wasnt actually
> exploitable. pass length to rip_auth_md5.

> + if (len != fromlen)
> + {
> + zlog_warn ("message size %d is not equal to buffer length %d",
> + len, fromlen);
> + return -1;
> + }

Sigh.. i cant read.

fromlen is sizeof address.

regards,
--
Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
warning: do not ever send email to spam@dishone.st
Fortune:
Santa Claus is watching!

Right..

A tested version of the patch attached. (yes, I do like detailed
changelog entries ;) )

It sets the default auth length to 20 and always prints out the
auth-length config command, so hopefully in time everyone will end up
with this command in their config files and one day we can switch the
default to rfc specified size.

regards,
--
Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
warning: do not ever send email to spam@dishone.st
Fortune:
QED.