NetBSD Problem Report #44952

From Wolfgang.Stukenbrock@nagler-company.com  Wed May 11 08:10:05 2011
Return-Path: <Wolfgang.Stukenbrock@nagler-company.com>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by www.NetBSD.org (Postfix) with ESMTP id 1338F63B95D
	for <gnats-bugs@gnats.NetBSD.org>; Wed, 11 May 2011 08:10:05 +0000 (UTC)
Message-Id: <20110511080604.6452F42F698@s0g7.nagler-company.com>
Date: Wed, 11 May 2011 10:06:04 +0200 (CEST)
From: Wolfgang.Stukenbrock@nagler-company.com
Reply-To: Wolfgang.Stukenbrock@nagler-company.com
To: gnats-bugs@gnats.NetBSD.org
Subject: setkey cannot set or display esp-frag information - fix memory whole in kernel
X-Send-Pr-Version: 3.95

>Number:         44952
>Category:       kern
>Synopsis:       setkey cannot set or display esp-frag information - fix memory whole in kernel
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed May 11 08:15:01 +0000 2011
>Closed-Date:    Tue Jan 10 09:56:57 +0000 2012
>Last-Modified:  Tue Jan 10 09:56:57 +0000 2012
>Originator:     Dr. Wolfgang Stukenbrock
>Release:        NetBSD 5.1
>Organization:
Dr. Nagler & Company GmbH
>Environment:


System: NetBSD s0g7 5.1 NetBSD 5.1 (NSW-locationGW_2) #2: Mon Mar 7 10:35:06 CET 2011 wgstuken@s012:/export/NetBSD-5.1/N+C-build/.OBJDIR_amd64/export/NetBSD-5.1/src/sys/arch/amd64/compile/NSW-locationGW_2 amd64
Architecture: x86_64
Machine: amd64
>Description:
	Setkey (8) cannot set or display esp-frag information in SA entries.
	The kernel interface allows racoon to set the information, but the
	kernel interface does not return it to the user level anymore.
	(This is the reason why both the user-level and the kernel are affected.)
>How-To-Repeat:
	Try to set esp-fragment size or try to display this information with setkey.
	You will fail ...
>Fix:
	There are three parts affected:
	- /usr/src/crypto/dist/ipsec-tools/src/setkey/{parse.y,token.l,setkey.8}
	- /usr/src/crypto/dist/ipsec-tools/src/libipsec/{pfkey,pfkey_debug,key_debug}.c
	- /usr/src/sys/{netipsc,netkey}/key.c

        The patch adds a new "extension" to setkey "-frag <num>" to allow specification
	of the fragment size. Feel free to adjust the option name and/or the manual
	description.

	The patch prints the fragment information in setkey output after the (optional)
	OA information.
	(By the way - this information is never returned by the kernel in the current
	implementation - OA is not my scope at the moment, so I don't change with this
	patch.)

	The patch expands the message dump and raw-dump routine in libipsec so that the 
	NAT_T_FRAG extions is now known.

	The patch expands and changes the way who the kernel returns the extentions
	for NAT-T - see comment in patch below

	The patch fixes a memory whole due to missing m_freem and removes a duplicate
	m_freem on error from key.c in netkey and netipsec - see comment in patch below

	remark: I'm only running FASTIPSEC. So the change in netkey/key.c is derived
		from netipsec/key.c and it compiles, but it is not tested!
		Someone should have an additional look at it - thanks


Patchinformation for files in /usr/src/crypto/dist/ipsec-tools/src/setkey


--- parse.y	2011/05/11 06:56:07	1.1
+++ parse.y	2011/05/11 06:57:16
@@ -90,6 +90,7 @@
 struct security_ctx sec_ctx;

 static u_int p_natt_type;
+static u_int p_natt_frag;
 static struct addrinfo * p_natt_oa = NULL;

 static int p_aiflags = 0, p_aifamily = PF_UNSPEC;
@@ -129,6 +130,7 @@
 %token ALG_COMP
 %token F_LIFETIME_HARD F_LIFETIME_SOFT
 %token F_LIFEBYTE_HARD F_LIFEBYTE_SOFT
+%token F_FRAG
 %token DECSTRING QUOTEDSTRING HEXSTRING STRING ANY
 	/* SPD management */
 %token SPDADD SPDDELETE SPDDUMP SPDFLUSH
@@ -532,6 +534,14 @@
 	|	F_MODE MODE { p_mode = $2; }
 	|	F_MODE ANY { p_mode = IPSEC_MODE_ANY; }
 	|	F_REQID DECSTRING { p_reqid = $2; }
+	|	F_FRAG DECSTRING
+		{
+			if ( p_natt_type == 0) {
+				yyerror("esp-fragment size only valid for NAT-T");
+				return -1;
+			}
+			p_natt_frag = $2;
+		}
 	|	F_REPLAY DECSTRING
 		{
 			if ((p_ext & SADB_X_EXT_OLD) != 0) {
@@ -1472,6 +1482,21 @@

 				memcpy(buf + l, &natt_port, len);
 				l += len;
+#ifdef SADB_X_EXT_NAT_T_FRAG
+				if (p_natt_frag) {
+					struct sadb_x_nat_t_frag natt_frag;
+					/* NATT_FRAG */
+					len = sizeof(struct sadb_x_nat_t_frag);
+					memset(&natt_frag, 0, len);
+					natt_frag.sadb_x_nat_t_frag_len = PFKEY_UNIT64(len);
+					natt_frag.sadb_x_nat_t_frag_exttype =
+						SADB_X_EXT_NAT_T_FRAG;
+					natt_frag.sadb_x_nat_t_frag_fraglen = p_natt_frag;
+					
+					memcpy(buf + l, &natt_frag, len);
+					l += len;
+				}
+#endif
 			}
 #endif
 			msg->sadb_msg_len = PFKEY_UNIT64(l);
@@ -1592,6 +1617,7 @@
 		freeaddrinfo (p_natt_oa);
 	p_natt_oa = NULL;
 	p_natt_type = 0;
+	p_natt_frag = 0;

 	return;
 }
--- token.l	2011/05/11 06:56:07	1.1
+++ token.l	2011/05/11 06:57:16
@@ -244,6 +244,7 @@
 {hyphen}bh	{ return(F_LIFEBYTE_HARD); }
 {hyphen}bs	{ return(F_LIFEBYTE_SOFT); }
 {hyphen}ctx	{ return(SECURITY_CTX); }
+{hyphen}frag	{ return(F_FRAG); }

 	/* ... */
 any		{ return(ANY); }
--- setkey.8	2011/05/11 07:34:24	1.1
+++ setkey.8	2011/05/11 07:35:21
@@ -369,6 +369,9 @@
 .It Fl bs Ar bytes
 Specify hard/soft life time duration of the SA measured in bytes transported.
 .\"
+.It Fl frag Ar bytes
+Specify esp fragment size for NAT-T (only valid for NAT-T SA's).
+.\"
 .It Fl ctx Ar doi Ar algorithm Ar context-name
 Specify an access control label. The access control label is interpreted 
 by the LSM (e.g., SELinux). Ultimately, it enables MAC on network 

Patch information for files in /usr/src/crypto/ipsec-tools/src/libipsec

--- key_debug.c	2011/05/11 07:02:54	1.1
+++ key_debug.c	2011/05/11 07:04:02
@@ -85,6 +85,9 @@
 #ifdef SADB_X_EXT_NAT_T_TYPE
 static void kdebug_sadb_x_nat_t_type __P((struct sadb_ext *ext));
 static void kdebug_sadb_x_nat_t_port __P((struct sadb_ext *ext));
+#ifdef SADB_X_EXT_NAT_T_TYPE
+static void kdebug_sadb_x_nat_t_frag __P((struct sadb_ext *ext));
+#endif
 #endif

 #ifdef SADB_X_EXT_PACKET
@@ -188,6 +191,11 @@
 		case SADB_X_EXT_NAT_T_OA:
 			kdebug_sadb_address(ext);
 			break;
+#ifdef SADB_X_EXT_NAT_T_FRAG
+		case SADB_X_EXT_NAT_T_FRAG:
+			kdebug_sadb_x_nat_t_frag(ext);
+			break;
+#endif
 #endif
 #ifdef SADB_X_EXT_PACKET
 		case SADB_X_EXT_PACKET:
@@ -534,6 +542,20 @@

 	return;
 }
+#ifdef SADB_X_EXT_NAT_T_TYPE
+static void kdebug_sadb_x_nat_t_frag (struct sadb_ext *ext)
+{
+	struct sadb_x_nat_t_frag *frag = (void *)ext;
+
+	/* sanity check */
+	if (ext == NULL)
+		panic("kdebug_sadb_x_nat_t_frag: NULL pointer was passed.\n");
+
+	printf("sadb_x_nat_t_frag{ esp-frag=%u }\n", frag->sadb_x_nat_t_frag_fraglen);
+
+	return;
+}
+#endif
 #endif

 #ifdef SADB_X_EXT_PACKET
--- pfkey.c	2011/05/11 07:02:54	1.1
+++ pfkey.c	2011/05/11 07:07:07
@@ -1993,6 +1993,9 @@
 		case SADB_X_EXT_NAT_T_TYPE:
 		case SADB_X_EXT_NAT_T_SPORT:
 		case SADB_X_EXT_NAT_T_DPORT:
+#ifdef SADB_X_EXT_NAT_T_FRAG
+		case SADB_X_EXT_NAT_T_FRAG:
+#endif
 		case SADB_X_EXT_NAT_T_OA:
 #endif
 #ifdef SADB_X_EXT_TAG
--- pfkey_dump.c	2011/05/11 07:02:54	1.1
+++ pfkey_dump.c	2011/05/11 07:04:02
@@ -254,6 +254,9 @@
 	struct sadb_x_nat_t_type *natt_type;
 	struct sadb_x_nat_t_port *natt_sport, *natt_dport;
 	struct sadb_address *natt_oa;
+#ifdef SADB_X_EXT_NAT_T_FRAG
+	struct sadb_x_nat_t_frag *natt_frag;
+#endif

 	int use_natt = 0;
 #endif
@@ -294,6 +297,9 @@
 	natt_sport = (void *)mhp[SADB_X_EXT_NAT_T_SPORT];
 	natt_dport = (void *)mhp[SADB_X_EXT_NAT_T_DPORT];
 	natt_oa = (void *)mhp[SADB_X_EXT_NAT_T_OA];
+#ifdef SADB_X_EXT_NAT_T_FRAG
+	natt_frag = (void *)mhp[SADB_X_EXT_NAT_T_FRAG];;
+#endif

 	if (natt_type && natt_type->sadb_x_nat_t_type_type)
 		use_natt = 1;
@@ -365,6 +371,11 @@
 	if (use_natt && natt_oa)
 		printf("\tNAT OA=%s\n",
 		       str_ipaddr((void *)(natt_oa + 1)));
+
+#ifdef SADB_X_EXT_NAT_T_FRAG
+	if (use_natt && natt_frag && natt_frag->sadb_x_nat_t_frag_fraglen != 0)
+		printf("\tNAT-T esp-frag=%u\n", natt_frag->sadb_x_nat_t_frag_fraglen);
+#endif
 #endif

 	/* encryption key */

Patch information for files in /usr/src/sys/netipsec

--- key.c	2011/05/10 10:21:39	1.2
+++ key.c	2011/05/11 07:12:06
@@ -424,6 +424,7 @@
 #ifdef IPSEC_NAT_T
 static struct mbuf *key_setsadbxport (u_int16_t, u_int16_t);
 static struct mbuf *key_setsadbxtype (u_int16_t);
+static struct mbuf *key_setsadbxfrag (u_int16_t);
 #endif
 static void key_porttosaddr (union sockaddr_union *, u_int16_t);
 static int key_checksalen (const union sockaddr_union *);
@@ -3616,6 +3617,9 @@
 			break;

 		case SADB_X_EXT_NAT_T_DPORT:
+// do not send useless information - e.g. if no NAT active - keep the message small
+			if (sav->natt_type == 0)
+				continue;
 			if ((m = key_setsadbxport(
 				key_portfromsaddr(&sav->sah->saidx.dst),
 				SADB_X_EXT_NAT_T_DPORT)) == NULL)
@@ -3623,6 +3627,9 @@
 			break;

 		case SADB_X_EXT_NAT_T_SPORT:
+// do not send useless information - e.g. if no NAT active - keep the message small
+			if (sav->natt_type == 0)
+				continue;
 			if ((m = key_setsadbxport(
 				key_portfromsaddr(&sav->sah->saidx.src),
 				SADB_X_EXT_NAT_T_SPORT)) == NULL)
@@ -3630,8 +3637,15 @@
 			break;

 		case SADB_X_EXT_NAT_T_OA:
-		case SADB_X_EXT_NAT_T_FRAG:
 			continue;
+
+		case SADB_X_EXT_NAT_T_FRAG:
+// do not send FRAG information if not set by upper level - keep the message small
+			if (sav->natt_type == 0 || sav->esp_frag == IP_MAXPACKET)
+				continue;
+			if ((m = key_setsadbxfrag(sav->esp_frag)) == NULL)
+				goto fail;
+			break;
 #endif

 		case SADB_EXT_ADDRESS_PROXY:
@@ -3643,8 +3657,10 @@
 			continue;
 		}

-		if ((!m && !p) || (m && p))
+		if (!m && !p)
 			goto fail;
+		if (m && p) // don't forget to free m again ...
+			{ m_freem(m); goto fail; }
 		if (p && tres) {
 			M_PREPEND(tres, l, M_DONTWAIT);
 			if (!tres)
@@ -3665,6 +3681,7 @@
 	}

 	m_cat(result, tres);
+	tres = NULL; // avoid free of concatenated mbuf in case of error below ...

 	if (result->m_len < sizeof(struct sadb_msg)) {
 		result = m_pullup(result, sizeof(struct sadb_msg));
@@ -3718,6 +3735,34 @@
 	return m;
 }
 /*
+ * set a type in sadb_x_nat_t_FRAG
+ */
+static struct mbuf *
+key_setsadbxfrag(u_int16_t flen)
+{
+	struct mbuf *m;
+	size_t len;
+	struct sadb_x_nat_t_frag *p;
+
+	len = PFKEY_ALIGN8(sizeof(struct sadb_x_nat_t_frag));
+
+	m = key_alloc_mbuf(len);
+	if (!m || m->m_next) {	/*XXX*/
+		if (m)
+			m_freem(m);
+		return NULL;
+	}
+
+	p = mtod(m, struct sadb_x_nat_t_frag *);
+
+	memset(p, 0, len);
+	p->sadb_x_nat_t_frag_len = PFKEY_UNIT64(len);
+	p->sadb_x_nat_t_frag_exttype = SADB_X_EXT_NAT_T_FRAG;
+	p->sadb_x_nat_t_frag_fraglen = flen;
+
+	return m;
+}
+/*
  * set a port in sadb_x_nat_t_port. port is in network order
  */
 static struct mbuf *

Patch information for files in /usr/src/sys/netkey

--- key.c	2011/05/11 07:14:22	1.1
+++ key.c	2011/05/11 07:17:02
@@ -391,6 +391,7 @@
 #ifdef IPSEC_NAT_T
 static struct mbuf *key_setsadbxport __P((u_int16_t, u_int16_t));
 static struct mbuf *key_setsadbxtype __P((u_int16_t));
+static struct mbuf *key_setsadbxfrag __P((u_int16_t));
 #endif
 static void key_porttosaddr __P((struct sockaddr *, u_int16_t));
 #define KEY_PORTTOSADDR(saddr, port) \
@@ -3653,6 +3654,9 @@
 			break;

 		case SADB_X_EXT_NAT_T_DPORT:
+// do not send useless information - e.g. if no NAT active - keep the message small
+                       if (sav->natt_type == 0)
+                               continue;
 			if ((m = key_setsadbxport(KEY_PORTFROMSADDR
 			    (&sav->sah->saidx.dst),
 			    SADB_X_EXT_NAT_T_DPORT)) == NULL)
@@ -3660,6 +3664,9 @@
 			break;

 		case SADB_X_EXT_NAT_T_SPORT:
+// do not send useless information - e.g. if no NAT active - keep the message small
+                       if (sav->natt_type == 0)
+                               continue;
 			if ((m = key_setsadbxport(KEY_PORTFROMSADDR
 			    (&sav->sah->saidx.src),
 			    SADB_X_EXT_NAT_T_SPORT)) == NULL)
@@ -3667,8 +3674,15 @@
 			break;

 		case SADB_X_EXT_NAT_T_OA:
-		case SADB_X_EXT_NAT_T_FRAG:
 			continue;
+
+		case SADB_X_EXT_NAT_T_FRAG:
+// do not send FRAG information if not set by upper level - keep the message small
+			if (sav->natt_type == 0 || sav->esp_frag == IP_MAXPACKET)
+				continue;
+			if ((m = key_setsadbxfrag(sav->esp_frag)) == NULL)
+				goto fail;
+			break;
 #endif
 		case SADB_EXT_ADDRESS_PROXY:
 		case SADB_EXT_IDENTITY_SRC:
@@ -3679,8 +3693,10 @@
 			continue;
 		}

-		if ((!m && !p) || (m && p))
+		if (!m && !p)
 			goto fail;
+		if (m && p) // don't forget to free m again ...
+			{ m_freem(m); goto fail; }
 		if (p && tres) {
 			M_PREPEND(tres, l, M_DONTWAIT);
 			if (!tres)
@@ -3701,6 +3717,7 @@
 	}

 	m_cat(result, tres);
+	tres = NULL; // avoid free of concatenated mbuf in case of error below ...

 	if (result->m_len < sizeof(struct sadb_msg)) {
 		result = m_pullup(result, sizeof(struct sadb_msg));
@@ -3990,6 +4007,34 @@
 	return m;
 }
 /*
+ * set a type in sadb_x_nat_t_FRAG
+ */
+static struct mbuf *
+key_setsadbxfrag(u_int16_t flen)
+{
+	struct mbuf *m;
+	size_t len;
+	struct sadb_x_nat_t_frag *p;
+
+	len = PFKEY_ALIGN8(sizeof(struct sadb_x_nat_t_frag));
+
+	m = key_alloc_mbuf(len);
+	if (!m || m->m_next) {	/*XXX*/
+		if (m)
+			m_freem(m);
+		return NULL;
+	}
+
+	p = mtod(m, struct sadb_x_nat_t_frag *);
+
+	memset(p, 0, len);
+	p->sadb_x_nat_t_frag_len = PFKEY_UNIT64(len);
+	p->sadb_x_nat_t_frag_exttype = SADB_X_EXT_NAT_T_FRAG;
+	p->sadb_x_nat_t_frag_fraglen = flen;
+
+	return m;
+}
+/*
  * set a port in sadb_x_nat_t_port. port is in network order
  */
 static struct mbuf *

>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: gnats-admin->kern-bug-people
Responsible-Changed-By: dholland@NetBSD.org
Responsible-Changed-When: Sat, 14 May 2011 19:02:07 +0000
Responsible-Changed-Why:
This landed in "pending" because gnats can't cope with the concept of a
PR with more than one category. Use "kern" because that's more likely to
attract the attention of the ipsec folks.


From: "Matthias Drochner" <drochner@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/44952 CVS commit: src/crypto/dist/ipsec-tools/src
Date: Mon, 9 Jan 2012 15:25:14 +0000

 Module Name:	src
 Committed By:	drochner
 Date:		Mon Jan  9 15:25:14 UTC 2012

 Modified Files:
 	src/crypto/dist/ipsec-tools/src/libipsec: key_debug.c pfkey.c
 	    pfkey_dump.c
 	src/crypto/dist/ipsec-tools/src/setkey: parse.y setkey.8 token.l

 Log Message:
 allow setkey(8) set and display the ESP fragment size in the NAT-T case,
 userland part of PR kern/44952 by Wolfgang Stukenbrock, just changed
 the "frag" option name to "esp_frag", for consistency to the existing
 option of similar effect in racoon(8)


 To generate a diff of this commit:
 cvs rdiff -u -r1.9 -r1.10 \
     src/crypto/dist/ipsec-tools/src/libipsec/key_debug.c
 cvs rdiff -u -r1.22 -r1.23 src/crypto/dist/ipsec-tools/src/libipsec/pfkey.c
 cvs rdiff -u -r1.19 -r1.20 \
     src/crypto/dist/ipsec-tools/src/libipsec/pfkey_dump.c
 cvs rdiff -u -r1.14 -r1.15 src/crypto/dist/ipsec-tools/src/setkey/parse.y
 cvs rdiff -u -r1.28 -r1.29 src/crypto/dist/ipsec-tools/src/setkey/setkey.8
 cvs rdiff -u -r1.16 -r1.17 src/crypto/dist/ipsec-tools/src/setkey/token.l

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Matthias Drochner" <drochner@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/44952 CVS commit: src/sys/netipsec
Date: Mon, 9 Jan 2012 15:42:08 +0000

 Module Name:	src
 Committed By:	drochner
 Date:		Mon Jan  9 15:42:08 UTC 2012

 Modified Files:
 	src/sys/netipsec: key.c

 Log Message:
 allow the ESP fragment length in the NAT-T case to be reported back
 through the pfkey interface, kernel part of PR kern/44952
 by Wolfgang Stukenbrock


 To generate a diff of this commit:
 cvs rdiff -u -r1.75 -r1.76 src/sys/netipsec/key.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->feedback
State-Changed-By: drochner@NetBSD.org
State-Changed-When: Mon, 09 Jan 2012 15:59:54 +0000
State-Changed-Why:
fraglen part applied to -current (with option name changed to esp_frag
for consistency with racoon)
memory leak issue was fixed a while ago
ok to close?


From: Wolfgang Stukenbrock <Wolfgang.Stukenbrock@nagler-company.com>
To: gnats-bugs@NetBSD.org
Cc: kern-bug-people@NetBSD.org, netbsd-bugs@NetBSD.org, gnats-admin@NetBSD.org,
        drochner@NetBSD.org, Wolfgang.Stukenbrock@nagler-company.com
Subject: Re: kern/44952 (setkey cannot set or display esp-frag information - fix memory whole in kernel)
Date: Tue, 10 Jan 2012 09:12:49 +0100

 Hi,

 it looks good.
 I think you can close this PR.

 Best reguards

 W. Stukenbrock

 drochner@NetBSD.org wrote:

 > Synopsis: setkey cannot set or display esp-frag information - fix memory whole in kernel
 > 
 > State-Changed-From-To: open->feedback
 > State-Changed-By: drochner@NetBSD.org
 > State-Changed-When: Mon, 09 Jan 2012 15:59:54 +0000
 > State-Changed-Why:
 > fraglen part applied to -current (with option name changed to esp_frag
 > for consistency with racoon)
 > memory leak issue was fixed a while ago
 > ok to close?
 > 
 > 
 > 
 > 


 -- 


 Dr. Nagler & Company GmbH
 Hauptstraße 9
 92253 Schnaittenbach

 Tel. +49 9622/71 97-42
 Fax +49 9622/71 97-50

 Wolfgang.Stukenbrock@nagler-company.com
 http://www.nagler-company.com


 Hauptsitz: Schnaittenbach
 Handelregister: Amberg HRB
 Gerichtsstand: Amberg
 Steuernummer: 201/118/51825
 USt.-ID-Nummer: DE 273143997
 Geschäftsführer: Dr. Martin Nagler, Dr. Dr. Karl-Kuno Kunze


State-Changed-From-To: feedback->closed
State-Changed-By: wiz@NetBSD.org
State-Changed-When: Tue, 10 Jan 2012 09:56:57 +0000
State-Changed-Why:
Confirmed fixed, thanks!


>Unformatted:

Home	PR Database Search