Subject: Re: bin/30437: recent NATT changes breaks racoon
To: None <manu@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org>
From: Emmanuel Dreyfus <manu@netbsd.org>
List: netbsd-bugs
Date: 09/26/2005 17:32:02
The following reply was made to PR bin/30437; it has been noted by GNATS.

From: Emmanuel Dreyfus <manu@netbsd.org>
To: jeffi@rcn.com, spz@serpens.de, tron@zhadum.de,
	gnats-bugs@netbsd.org
Cc: 
Subject: Re: bin/30437: recent NATT changes breaks racoon
Date: Mon, 26 Sep 2005 17:31:50 +0000

 --DO5DiztRLs659m5i
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: inline
 
 Here is an updated kernel patch that fixes the problem at mine.
 I have to check that NAT-T still works with that patch, and even
 with multiple endpoints behind the NAT. Help is welcome as I currently
 lack the hardware.
 
 -- 
 Emmanuel Dreyfus
 manu@netbsd.org
 
 --DO5DiztRLs659m5i
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: attachment; filename="no-nat-t.diff"
 
 Index: key.c
 ===================================================================
 RCS file: /cvsroot/src/sys/netkey/key.c,v
 retrieving revision 1.135
 diff -U2 -r1.135 key.c
 --- key.c	3 Jul 2005 22:57:09 -0000	1.135
 +++ key.c	26 Sep 2005 17:24:35 -0000
 @@ -387,8 +387,10 @@
  static struct mbuf *key_setsadbxport __P((u_int16_t, u_int16_t));
  static struct mbuf *key_setsadbxtype __P((u_int16_t));
 +#endif
  static void key_porttosaddr __P((struct sockaddr *, u_int16_t));
  #define KEY_PORTTOSADDR(saddr, port) \
 -     key_porttosaddr((struct sockaddr *)(saddr), (port))
 -#endif
 +	key_porttosaddr((struct sockaddr *)(saddr), (port))
 +static int key_checksalen __P((const struct sockaddr *));
 +#define KEY_CHECKSALEN(saddr) key_checksalen((const struct sockaddr *)(saddr))
  static struct mbuf *key_setsadblifetime __P((u_int16_t, u_int32_t,
  	u_int64_t, u_int64_t, u_int64_t));
 @@ -804,5 +806,7 @@
  			bcopy(src, &sin.sin_addr,
  			    sizeof(sin.sin_addr));
 +#ifdef IPSEC_NAT_T
  			sin.sin_port = sport;
 +#endif
  			if (key_sockaddrcmp((struct sockaddr*)&sin,
  			    (struct sockaddr *)&sav->sah->saidx.src, 
 @@ -817,5 +821,7 @@
  			bcopy(src, &sin6.sin6_addr,
  			    sizeof(sin6.sin6_addr));
 +#ifdef IPSEC_NAT_T
  			sin6.sin6_port = sport;
 +#endif
  			if (IN6_IS_SCOPE_LINKLOCAL(&sin6.sin6_addr)) {
  				/* kame fake scopeid */
 @@ -845,5 +851,7 @@
  			bcopy(dst, &sin.sin_addr,
  			    sizeof(sin.sin_addr));
 +#ifdef IPSEC_NAT_T
  			sin.sin_port = dport;
 +#endif
  			if (key_sockaddrcmp((struct sockaddr*)&sin,
  			    (struct sockaddr *)&sav->sah->saidx.dst,
 @@ -858,5 +866,7 @@
  			bcopy(dst, &sin6.sin6_addr,
  			    sizeof(sin6.sin6_addr));
 +#ifdef IPSEC_NAT_T
  			sin6.sin6_port = dport;
 +#endif
  			if (IN6_IS_SCOPE_LINKLOCAL(&sin6.sin6_addr)) {
  				/* kame fake scopeid */
 @@ -4039,5 +4049,5 @@
  	return port;
  }
 -
 +#endif /* IPSEC_NAT_T */
  
  /*
 @@ -4065,5 +4075,6 @@
  #endif
  	default:
 -		printf("key_porttosaddr: unexpected address family\n");
 +		printf("key_porttosaddr: unexpected address family %d\n", 
 +		    saddr->sa_family);
  		break;
  	}
 @@ -4071,5 +4082,32 @@
  	return;
  }
 -#endif /* IPSEC_NAT_T */
 +
 +/*
 + * Safety check sa_len 
 + */
 +static int 
 +key_checksalen(saddr)
 +	const struct sockaddr *saddr;
 +{
 +	switch (saddr->sa_family) {
 +	case AF_INET:
 +		if (saddr->sa_len != sizeof(struct sockaddr_in))
 +			return -1;
 +		break;
 +#ifdef INET6
 +	case AF_INET6:
 +		if (saddr->sa_len != sizeof(struct sockaddr_in6))
 +			return -1;
 +		break;
 +#endif
 +	default:
 +		printf("key_checksalen: unexpected sa_family %d\n", 
 +		    saddr->sa_family);
 +		return -1;
 +		break;
 +	}
 +
 +	return 0;
 +}
  
  /*
 @@ -5029,52 +5067,18 @@
  	}
  
 -	/* 
 -	 * make sure if port number is zero. 
 -	 * If using NAT-T, skip that check.
 -	 */
 -	switch (((struct sockaddr *)(src0 + 1))->sa_family) {
 -	case AF_INET:
 -		if (((struct sockaddr *)(src0 + 1))->sa_len !=
 -		    sizeof(struct sockaddr_in))
 -			return key_senderror(so, m, EINVAL);
 -#ifndef IPSEC_NAT_T
 -		((struct sockaddr_in *)(src0 + 1))->sin_port = 0;
 -#endif
 -		break;
 -	case AF_INET6:
 -		if (((struct sockaddr *)(src0 + 1))->sa_len !=
 -		    sizeof(struct sockaddr_in6))
 -			return key_senderror(so, m, EINVAL);
 -#ifndef IPSEC_NAT_T
 -		((struct sockaddr_in6 *)(src0 + 1))->sin6_port = 0;
 -#endif
 -		break;
 -	default:
 -		; /*???*/
 -	}
 -	switch (((struct sockaddr *)(dst0 + 1))->sa_family) {
 -	case AF_INET:
 -		if (((struct sockaddr *)(dst0 + 1))->sa_len !=
 -		    sizeof(struct sockaddr_in))
 -			return key_senderror(so, m, EINVAL);
 -#ifndef IPSEC_NAT_T
 -		((struct sockaddr_in *)(dst0 + 1))->sin_port = 0;
 -#endif
 -		break;
 -	case AF_INET6:
 -		if (((struct sockaddr *)(dst0 + 1))->sa_len !=
 -		    sizeof(struct sockaddr_in6))
 -			return key_senderror(so, m, EINVAL);
 -#ifndef IPSEC_NAT_T
 -		((struct sockaddr_in6 *)(dst0 + 1))->sin6_port = 0;
 -#endif
 -		break;
 -	default:
 -		; /*???*/
 -	}
 +	/* sa_len safety check */
 +	if (KEY_CHECKSALEN(src0 + 1) != 0)
 +		return key_senderror(so, m, EINVAL);
 +	if (KEY_CHECKSALEN(dst0 + 1) != 0)
 +		return key_senderror(so, m, EINVAL);
  
 -	/* XXX boundary check against sa_len */
  	KEY_SETSECASIDX(proto, mode, reqid, src0 + 1, dst0 + 1, &saidx);
  
 +	/* If not using NAT-T, make sure port numbers are set to zero. */
 +#ifndef IPSEC_NAT_T
 +	KEY_PORTTOSADDR(&saidx.src, 0);
 +	KEY_PORTTOSADDR(&saidx.dst, 0);
 +#endif
 +
  	/* SPI allocation */
  	spi = key_do_getnewspi((struct sadb_spirange *)mhp->ext[SADB_EXT_SPIRANGE],
 @@ -5327,7 +5331,18 @@
  	dst0 = (struct sadb_address *)(mhp->ext[SADB_EXT_ADDRESS_DST]);
  
 -	/* XXX boundary check against sa_len */
 +	/* sa_len safety check */
 +	if (KEY_CHECKSALEN(src0 + 1) != 0)
 +		return key_senderror(so, m, EINVAL);
 +	if (KEY_CHECKSALEN(dst0 + 1) != 0)
 +		return key_senderror(so, m, EINVAL);
 +	
  	KEY_SETSECASIDX(proto, mode, reqid, src0 + 1, dst0 + 1, &saidx);
  
 +	/* If not using NAT-T, make sure if port number is zero. */
 +#ifndef IPSEC_NAT_T
 +	KEY_PORTTOSADDR(&saidx.src, 0);
 +	KEY_PORTTOSADDR(&saidx.dst, 0);
 +#endif
 +
  	/* get a SA header */
  	if ((sah = key_getsah(&saidx)) == NULL) {
 @@ -5577,7 +5592,18 @@
  	dst0 = (struct sadb_address *)mhp->ext[SADB_EXT_ADDRESS_DST];
  
 -	/* XXX boundary check against sa_len */
 +	/* sa_len safety check */
 +	if (KEY_CHECKSALEN(src0 + 1) != 0)
 +		return key_senderror(so, m, EINVAL);
 +	if (KEY_CHECKSALEN(dst0 + 1) != 0)
 +		return key_senderror(so, m, EINVAL);
 +	
  	KEY_SETSECASIDX(proto, mode, reqid, src0 + 1, dst0 + 1, &saidx);
  
 +	/* If not using NAT-T, make sure if port number is zero. */
 +#ifndef IPSEC_NAT_T
 +	KEY_PORTTOSADDR(&saidx.src, 0);
 +	KEY_PORTTOSADDR(&saidx.dst, 0);
 +#endif
 +
  	/* get a SA header */
  	if ((newsah = key_getsah(&saidx)) == NULL) {
 @@ -5866,7 +5892,18 @@
  	dst0 = (struct sadb_address *)(mhp->ext[SADB_EXT_ADDRESS_DST]);
  
 -	/* XXX boundary check against sa_len */
 +	/* sa_len safety check */
 +	if (KEY_CHECKSALEN(src0 + 1) != 0)
 +		return key_senderror(so, m, EINVAL);
 +	if (KEY_CHECKSALEN(dst0 + 1) != 0)
 +		return key_senderror(so, m, EINVAL);
 +
  	KEY_SETSECASIDX(proto, IPSEC_MODE_ANY, 0, src0 + 1, dst0 + 1, &saidx);
  
 +	/* If not using NAT-T, make sure if port number is zero. */
 +#ifndef IPSEC_NAT_T
 +	KEY_PORTTOSADDR(&saidx.src, 0);
 +	KEY_PORTTOSADDR(&saidx.dst, 0);
 +#endif
 +
  	/* get a SA header */
  	LIST_FOREACH(sah, &sahtree, chain) {
 @@ -5933,7 +5970,18 @@
  	dst0 = (struct sadb_address *)(mhp->ext[SADB_EXT_ADDRESS_DST]);
  
 -	/* XXX boundary check against sa_len */
 +	/* sa_len safety check */
 +	if (KEY_CHECKSALEN(src0 + 1) != 0)
 +		return key_senderror(so, m, EINVAL);
 +	if (KEY_CHECKSALEN(dst0 + 1) != 0)
 +		return key_senderror(so, m, EINVAL);
 +
  	KEY_SETSECASIDX(proto, IPSEC_MODE_ANY, 0, src0 + 1, dst0 + 1, &saidx);
  
 +	/* If not using NAT-T, make sure if port number is zero. */
 +#ifndef IPSEC_NAT_T
 +	KEY_PORTTOSADDR(&saidx.src, 0);
 +	KEY_PORTTOSADDR(&saidx.dst, 0);
 +#endif
 +
  	LIST_FOREACH(sah, &sahtree, chain) {
  		if (sah->state == SADB_SASTATE_DEAD)
 @@ -6042,7 +6090,18 @@
  	dst0 = (struct sadb_address *)mhp->ext[SADB_EXT_ADDRESS_DST];
  
 -	/* XXX boundary check against sa_len */
 +	/* sa_len safety check */
 +	if (KEY_CHECKSALEN(src0 + 1) != 0)
 +		return key_senderror(so, m, EINVAL);
 +	if (KEY_CHECKSALEN(dst0 + 1) != 0)
 +		return key_senderror(so, m, EINVAL);
 +
  	KEY_SETSECASIDX(proto, IPSEC_MODE_ANY, 0, src0 + 1, dst0 + 1, &saidx);
  
 +	/* If not using NAT-T, make sure if port number is zero. */
 +#ifndef IPSEC_NAT_T
 +	KEY_PORTTOSADDR(&saidx.src, 0);
 +	KEY_PORTTOSADDR(&saidx.dst, 0);
 +#endif
 +
  	/* get a SA header */
  	LIST_FOREACH(sah, &sahtree, chain) {
 @@ -6733,7 +6792,18 @@
  	dst0 = (struct sadb_address *)mhp->ext[SADB_EXT_ADDRESS_DST];
  
 -	/* XXX boundary check against sa_len */
 +	/* sa_len safety check */
 +	if (KEY_CHECKSALEN(src0 + 1) != 0)
 +		return key_senderror(so, m, EINVAL);
 +	if (KEY_CHECKSALEN(dst0 + 1) != 0)
 +		return key_senderror(so, m, EINVAL);
 +
  	KEY_SETSECASIDX(proto, IPSEC_MODE_ANY, 0, src0 + 1, dst0 + 1, &saidx);
  
 +	/* If not using NAT-T, make sure if port number is zero. */
 +#ifndef IPSEC_NAT_T
 +	KEY_PORTTOSADDR(&saidx.src, 0);
 +	KEY_PORTTOSADDR(&saidx.dst, 0);
 +#endif
 +
  	/* get a SA index */
  	LIST_FOREACH(sah, &sahtree, chain) {
 
 --DO5DiztRLs659m5i--