[lcdpd-users] Lcdp bugs?

[Joerg M. <jm...@lo...>]

Hello,

I recently downloaded lcdp 0.2.2 - it's really nice that there is a cdp i=
mplementation
for Linux now. Then I looked at the code in detail and found some problem=
s (see the
annotated patchfile below). The more I think about it, the more I am conv=
inced that
cdp should *not* be implemented as a kernel module but as a user space da=
emon. One
more thing about the module: cisco recently published a security advisory=
 about a
DOS on cdp that would work against your code too: Take a machine that sen=
ds out faked
cdp packets from many faked senders. If you add long version strings etc =
then it's
a thing of a few seconds and there will be no memory left - and the more =
I looked at
the code, the more I'm conviced that cdp should be done in a userspace da=
emon instead
of a kernel module. That way the normal resource limiting mechanism can b=
e used.

  Ciao
           J=F6rg

+/* update a neighbor entry with newer values */
+int cdp_update_neighbor( struct sk_buff *skb, struct s_cdp_neighbor *p )=
 {

[...]

JM: Are you sure there will never be elements that consist of TL only?

+	while ( (skb->tail-skb->data) > 4 ) {
+		type =3D 0;
+		length =3D 0;
+
+		memcpy( &type, (skb->data)++, 2 ); (skb->data)++;
+		memcpy( &length, (skb->data)++, 2 ); (skb->data)++;
+		/* make sure the byte order is right */
+		type =3D ntohs( type );
+		/* total length includes the type field and length field */
+		length =3D ntohs( length ) - 4;

JM: FIXME - make sure, that length is <=3D number of bytes remaining in t=
he packet

		if (skb->tail-skb->data < length) {
				someone is sending broken packets! (trying to
				make us copy data beyond end of packet)
				handle error...
				return 0;
		}
+
+		data =3D (unsigned char *)kmalloc(
+				sizeof(unsigned char)*length,
+				GFP_ATOMIC );

[...]
+
+
+/* do the data display for /proc/net/cdp_neighbors query */
+int cdp_get_neighbor_info( char *buffer, char **start, off_t offset, int=
 length ) {
[...]
+			break;
+	}
+
+	restore_flags( flags );
+
+	*start =3D buffer + (offset-begin);
+	len -=3D offset - begin;
+	if ( len > length )
+		len=3Dlength;
+
+	return len;

JM: I don't know enough about procfs, but what happens, when there are a =
few
    Elements with 64000 Bytes? Is the buffer big enough? Just looked at
    kernel/module.c: they make sure they don't overflow any buffers!

+}
+
+
+
+/********************************************************************
+	network functions
+ ********************************************************************/
+
+/* deletes timed out neighbor entries (ie TTL expired) */
+static void SMP_TIMER_NAME(cdp_check_expire)( unsigned long unused ) {
+	struct timeval sometime;
+	struct s_cdp_neighbor *p =3D cdp_neighbors.head;
+	unsigned	long	flags;
+
+	mod_timer( &cdp_poll_neighbors_timer, jiffies + CDP_POLL /* now + CDP_P=
OLL */ );
+
+	get_fast_time( &sometime );
+	save_flags( flags );
+	cli();
+
+	/* now go through the neighbor list */
+	while ( p ) {
+		if ( (p->timestamp.tv_sec+p->cdp_ttl) < sometime.tv_sec ) {
+			if ( p->next ) {
+				p =3D p->next;
+				cdp_delete_neighbor( p->prev );

JM: what is the idea behind this? why not cdp_delete_neighbor(p)?

+			}
+			else {
+				cdp_delete_neighbor( p );
+				break;
+			}
+		}
+		else
+			p =3D p->next;
+	}
+	restore_flags( flags );
+}
--
Joerg Mayer                                          <jm...@lo...>
I found out that "pro" means "instead of" (as in proconsul). Now I know
what proactive means.