NetBSD Problem Report #51682
From www@NetBSD.org Fri Dec 2 21:21:07 2016
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(Client CN "mail.netbsd.org", Issuer "Postmaster NetBSD.org" (verified OK))
by mollari.NetBSD.org (Postfix) with ESMTPS id 0A0107A284
for <gnats-bugs@gnats.NetBSD.org>; Fri, 2 Dec 2016 21:21:07 +0000 (UTC)
Message-Id: <20161202212105.B6CEE7A303@mollari.NetBSD.org>
Date: Fri, 2 Dec 2016 21:21:05 +0000 (UTC)
From: rfoggia@trustwave.com
Reply-To: rfoggia@trustwave.com
To: gnats-bugs@NetBSD.org
Subject: Remote un-authenticated denial of service
X-Send-Pr-Version: www-1.0
>Number: 51682
>Category: security
>Synopsis: Remote un-authenticated denial of service
>Confidential: no
>Severity: non-critical
>Priority: low
>Responsible: security-officer
>State: open
>Class: support
>Submitter-Id: net
>Arrival-Date: Fri Dec 02 21:25:00 +0000 2016
>Last-Modified: Thu Aug 31 08:55:01 +0000 2017
>Originator: Robert Foggia
>Release:
>Organization:
Trustwave
>Environment:
>Description:
Finding 1: Remote un-authenticated denial of service in ipsec-tools
The ipsec-tools racoon daemon contains a remotely exploitable computational complexity attack when parsing and storing isakmp fragments. The implementation permits a remote attacker to exhaust computational resources on the remote endpoint by repeatedly sending isakmp fragment packets in a particular order such that the worst-case computational complexity is realized in the algorithm utilized to determine if reassembly of the fragments can take place.
We've reached out to IpSec-tools directly. They said to also report it to you as well since you manage your own folk.
>How-To-Repeat:
The algorithm in question is a simple quadratic linked list walk and is in O(n(n+1)) hence O(n^2) for ’n’ fragments received. Since the implementation fails to identify repeated fragment indices, a remote attacker can repeatedly specify the same index. Worst-case complexity is realized if fragments are sent in reverse order, for instance:
253, 252 ... 3, 2, 1, 255 (last fragment)
The absence of fragment index 254 is not an error as this ensures fragment reassembly is not possible.
>Fix:
Only in ipsec-tools-0.8.2-new/: .DS_Store
Only in ipsec-tools-0.8.2-new/src: .DS_Store
diff -ru ipsec-tools-0.8.2/src/racoon/handler.h ipsec-tools-0.8.2-new/src/racoon/handler.h
--- ipsec-tools-0.8.2/src/racoon/handler.h 2010-11-17 10:40:41.000000000 +0000
+++ ipsec-tools-0.8.2-new/src/racoon/handler.h 2016-09-02 17:03:27.000000000 +0100
@@ -141,6 +141,7 @@
#endif
#ifdef ENABLE_FRAG
int frag; /* IKE phase 1 fragmentation */
+ int frag_last_index;
struct isakmp_frag_item *frag_chain; /* Received fragments */
#endif
diff -ru ipsec-tools-0.8.2/src/racoon/isakmp.c ipsec-tools-0.8.2-new/src/racoon/isakmp.c
--- ipsec-tools-0.8.2/src/racoon/isakmp.c 2012-08-29 09:55:26.000000000 +0100
+++ ipsec-tools-0.8.2-new/src/racoon/isakmp.c 2016-09-06 09:14:46.000000000 +0100
@@ -1069,6 +1069,7 @@
iph1->frag = 1;
else
iph1->frag = 0;
+ iph1->frag_last_index = 0;
iph1->frag_chain = NULL;
#endif
iph1->approval = NULL;
@@ -1173,6 +1174,7 @@
#endif
#ifdef ENABLE_FRAG
iph1->frag = 0;
+ iph1->frag_last_index = 0;
iph1->frag_chain = NULL;
#endif
iph1->approval = NULL;
diff -ru ipsec-tools-0.8.2/src/racoon/isakmp_frag.c ipsec-tools-0.8.2-new/src/racoon/isakmp_frag.c
--- ipsec-tools-0.8.2/src/racoon/isakmp_frag.c 2009-04-22 12:24:20.000000000 +0100
+++ ipsec-tools-0.8.2-new/src/racoon/isakmp_frag.c 2016-10-03 12:51:52.000000000 +0100
@@ -224,39 +224,66 @@
item->frag_next = NULL;
item->frag_packet = buf;
- /* Look for the last frag while inserting the new item in the chain */
- if (item->frag_last)
- last_frag = item->frag_num;
-
- if (iph1->frag_chain == NULL) {
- iph1->frag_chain = item;
- } else {
- struct isakmp_frag_item *current;
-
- current = iph1->frag_chain;
- while (current->frag_next) {
- if (current->frag_last)
- last_frag = item->frag_num;
- current = current->frag_next;
+ /* Check for the last frag before inserting the new item in the chain */
+ if (item->frag_last) {
+ /* if we have the last fragment, indices must match */
+ if (iph1->frag_last_index != 0 &&
+ item->frag_last != iph1->frag_last_index) {
+ plog(LLV_ERROR, LOCATION, NULL,
+ "Repeated last fragment index mismatch\n");
+ racoon_free(item);
+ vfree(buf);
+ return -1;
}
- current->frag_next = item;
+
+ last_frag = iph1->frag_last_index = item->frag_num;
}
- /* If we saw the last frag, check if the chain is complete */
+ /* insert fragment into chain */
+ if (iph1->frag_chain == NULL) {
+ iph1->frag_chain = item;
+ } else {
+ struct isakmp_frag_item *pitem = NULL, *citem = iph1->frag_chain;
+
+ do {
+ if (citem->frag_num == item->frag_num) {
+ plog(LLV_ERROR, LOCATION, NULL,
+ "Repeated fragment index mismatch\n");
+ racoon_free(item);
+ vfree(buf);
+ return -1;
+ }
+
+ if (citem->frag_num > item->frag_num) {
+ if (pitem)
+ pitem->next = item;
+ item->next = citem;
+ break;
+ }
+
+ pitem = citem;
+ citem = citem->frag_next;
+ } while (citem != NULL);
+
+ /* we reached the end of the list, insert */
+ if (citem == NULL)
+ pitem->next = item;
+ }
+
+ /* If we saw the last frag, check if the chain is complete
+ * we have a sorted list now, so just walk through */
if (last_frag != 0) {
+ item = iph1->frag_chain;
for (i = 1; i <= last_frag; i++) {
- item = iph1->frag_chain;
- do {
- if (item->frag_num == i)
- break;
- item = item->frag_next;
- } while (item != NULL);
+ if (item->frag_num != i)
+ break;
+ item = item->frag_next;
if (item == NULL) /* Not found */
break;
}
- if (item != NULL) /* It is complete */
+ if (i > last_frag) /* It is complete */
return 1;
}
@@ -291,15 +318,9 @@
}
data = buf->v;
+ item = iph1->frag_chain;
for (i = 1; i <= frag_count; i++) {
- item = iph1->frag_chain;
- do {
- if (item->frag_num == i)
- break;
- item = item->frag_next;
- } while (item != NULL);
-
- if (item == NULL) {
+ if (item->frag_num != i) {
plog(LLV_ERROR, LOCATION, NULL,
"Missing fragment #%d\n", i);
vfree(buf);
@@ -308,6 +329,7 @@
}
memcpy(data, item->frag_packet->v, item->frag_packet->l);
data += item->frag_packet->l;
+ item = item->frag_next;
}
out:
diff -ru ipsec-tools-0.8.2/src/racoon/isakmp_inf.c ipsec-tools-0.8.2-new/src/racoon/isakmp_inf.c
--- ipsec-tools-0.8.2/src/racoon/isakmp_inf.c 2013-04-12 10:53:52.000000000 +0100
+++ ipsec-tools-0.8.2-new/src/racoon/isakmp_inf.c 2016-09-05 11:00:40.000000000 +0100
@@ -720,6 +720,7 @@
#endif
#ifdef ENABLE_FRAG
iph1->frag = 0;
+ iph1->frag_last_index = 0;
iph1->frag_chain = NULL;
#endif
>Audit-Trail:
From: christos@zoulas.com (Christos Zoulas)
To: gnats-bugs@NetBSD.org, security-officer@netbsd.org,
gnats-admin@netbsd.org, security-alert@netbsd.org
Cc:
Subject: Re: security/51682: Remote un-authenticated denial of service
Date: Mon, 23 Jan 2017 21:34:33 -0500
On Dec 2, 9:25pm, rfoggia@trustwave.com (rfoggia@trustwave.com) wrote:
-- Subject: security/51682: Remote un-authenticated denial of service
Hi,
Have you tried this patch? It does not compile... Here's a version
that compiles. How did you test it? Do you have a test harness you
can share?
Index: handler.h
===================================================================
RCS file: /cvsroot/src/crypto/dist/ipsec-tools/src/racoon/handler.h,v
retrieving revision 1.25
diff -u -u -r1.25 handler.h
--- handler.h 17 Nov 2010 10:40:41 -0000 1.25
+++ handler.h 24 Jan 2017 02:29:15 -0000
@@ -141,6 +141,7 @@
#endif
#ifdef ENABLE_FRAG
int frag; /* IKE phase 1 fragmentation */
+ int frag_last_index;
struct isakmp_frag_item *frag_chain; /* Received fragments */
#endif
Index: isakmp.c
===================================================================
RCS file: /cvsroot/src/crypto/dist/ipsec-tools/src/racoon/isakmp.c,v
retrieving revision 1.75
diff -u -u -r1.75 isakmp.c
--- isakmp.c 9 Mar 2016 22:27:17 -0000 1.75
+++ isakmp.c 24 Jan 2017 02:29:15 -0000
@@ -1077,6 +1077,7 @@
iph1->frag = 1;
else
iph1->frag = 0;
+ iph1->frag_last_index = 0;
iph1->frag_chain = NULL;
#endif
iph1->approval = NULL;
@@ -1181,6 +1182,7 @@
#endif
#ifdef ENABLE_FRAG
iph1->frag = 0;
+ iph1->frag_last_index = 0;
iph1->frag_chain = NULL;
#endif
iph1->approval = NULL;
Index: isakmp_frag.c
===================================================================
RCS file: /cvsroot/src/crypto/dist/ipsec-tools/src/racoon/isakmp_frag.c,v
retrieving revision 1.5
diff -u -u -r1.5 isakmp_frag.c
--- isakmp_frag.c 22 Apr 2009 11:24:20 -0000 1.5
+++ isakmp_frag.c 24 Jan 2017 02:29:15 -0000
@@ -173,6 +173,38 @@
return ntohl(hp[MD5_DIGEST_LENGTH / sizeof(*hp)]);
}
+static int
+isakmp_frag_insert(struct ph1handle *iph1, struct isakmp_frag_item *item)
+{
+ struct isakmp_frag_item *pitem = NULL;
+ struct isakmp_frag_item *citem = iph1->frag_chain;
+
+ if (iph1->frag_chain == NULL) {
+ iph1->frag_chain = item;
+ return 0;
+ }
+
+ do {
+ if (citem->frag_num == item->frag_num)
+ return -1;
+
+ if (citem->frag_num > item->frag_num) {
+ if (pitem)
+ pitem->frag_next = item;
+ item->frag_next = citem;
+ break;
+ }
+
+ pitem = citem;
+ citem = citem->frag_next;
+ } while (citem != NULL);
+
+ /* we reached the end of the list, insert */
+ if (citem == NULL)
+ pitem->frag_next = item;
+ return 0;
+}
+
int
isakmp_frag_extract(iph1, msg)
struct ph1handle *iph1;
@@ -224,39 +256,43 @@
item->frag_next = NULL;
item->frag_packet = buf;
- /* Look for the last frag while inserting the new item in the chain */
- if (item->frag_last)
- last_frag = item->frag_num;
+ /* Check for the last frag before inserting the new item in the chain */
+ if (item->frag_last) {
+ /* if we have the last fragment, indices must match */
+ if (iph1->frag_last_index != 0 &&
+ item->frag_last != iph1->frag_last_index) {
+ plog(LLV_ERROR, LOCATION, NULL,
+ "Repeated last fragment index mismatch\n");
+ racoon_free(item);
+ vfree(buf);
+ return -1;
+ }
- if (iph1->frag_chain == NULL) {
- iph1->frag_chain = item;
- } else {
- struct isakmp_frag_item *current;
+ last_frag = iph1->frag_last_index = item->frag_num;
+ }
- current = iph1->frag_chain;
- while (current->frag_next) {
- if (current->frag_last)
- last_frag = item->frag_num;
- current = current->frag_next;
- }
- current->frag_next = item;
+ /* insert fragment into chain */
+ if (isakmp_frag_insert(iph1, item) == -1) {
+ plog(LLV_ERROR, LOCATION, NULL,
+ "Repeated fragment index mismatch\n");
+ racoon_free(item);
+ vfree(buf);
+ return -1;
}
- /* If we saw the last frag, check if the chain is complete */
+ /* If we saw the last frag, check if the chain is complete
+ * we have a sorted list now, so just walk through */
if (last_frag != 0) {
+ item = iph1->frag_chain;
for (i = 1; i <= last_frag; i++) {
- item = iph1->frag_chain;
- do {
- if (item->frag_num == i)
- break;
- item = item->frag_next;
- } while (item != NULL);
-
+ if (item->frag_num != i)
+ break;
+ item = item->frag_next;
if (item == NULL) /* Not found */
break;
}
- if (item != NULL) /* It is complete */
+ if (i > last_frag) /* It is complete */
return 1;
}
@@ -291,15 +327,9 @@
}
data = buf->v;
+ item = iph1->frag_chain;
for (i = 1; i <= frag_count; i++) {
- item = iph1->frag_chain;
- do {
- if (item->frag_num == i)
- break;
- item = item->frag_next;
- } while (item != NULL);
-
- if (item == NULL) {
+ if (item->frag_num != i) {
plog(LLV_ERROR, LOCATION, NULL,
"Missing fragment #%d\n", i);
vfree(buf);
@@ -308,6 +338,7 @@
}
memcpy(data, item->frag_packet->v, item->frag_packet->l);
data += item->frag_packet->l;
+ item = item->frag_next;
}
out:
Index: isakmp_inf.c
===================================================================
RCS file: /cvsroot/src/crypto/dist/ipsec-tools/src/racoon/isakmp_inf.c,v
retrieving revision 1.50
diff -u -u -r1.50 isakmp_inf.c
--- isakmp_inf.c 12 Apr 2013 09:53:10 -0000 1.50
+++ isakmp_inf.c 24 Jan 2017 02:29:15 -0000
@@ -720,6 +720,7 @@
#endif
#ifdef ENABLE_FRAG
iph1->frag = 0;
+ iph1->frag_last_index = 0;
iph1->frag_chain = NULL;
#endif
From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/51682 CVS commit: src/crypto/dist/ipsec-tools/src/racoon
Date: Tue, 24 Jan 2017 14:23:31 -0500
Module Name: src
Committed By: christos
Date: Tue Jan 24 19:23:31 UTC 2017
Modified Files:
src/crypto/dist/ipsec-tools/src/racoon: isakmp_frag.c
Log Message:
PR/51682: Avoid DoS with fragment out of order insertion; keep fragments
sorted in the list.
To generate a diff of this commit:
cvs rdiff -u -r1.5 -r1.6 src/crypto/dist/ipsec-tools/src/racoon/isakmp_frag.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/51682 CVS commit: src/crypto/dist/ipsec-tools/src/racoon
Date: Tue, 24 Jan 2017 14:23:56 -0500
Module Name: src
Committed By: christos
Date: Tue Jan 24 19:23:56 UTC 2017
Modified Files:
src/crypto/dist/ipsec-tools/src/racoon: handler.h isakmp.c isakmp_inf.c
Log Message:
PR/51682: Avoid DoS with fragment out of order insertion; keep fragments
sorted in the list.
To generate a diff of this commit:
cvs rdiff -u -r1.25 -r1.26 src/crypto/dist/ipsec-tools/src/racoon/handler.h
cvs rdiff -u -r1.75 -r1.76 src/crypto/dist/ipsec-tools/src/racoon/isakmp.c
cvs rdiff -u -r1.50 -r1.51 \
src/crypto/dist/ipsec-tools/src/racoon/isakmp_inf.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: Robert Foggia <RFoggia@trustwave.com>
To: "gnats-bugs@NetBSD.org" <gnats-bugs@NetBSD.org>,
"security-officer@netbsd.org" <security-officer@netbsd.org>,
"gnats-admin@netbsd.org" <gnats-admin@netbsd.org>,
"security-alert@netbsd.org" <security-alert@netbsd.org>
Cc:
Subject: Re: PR/51682 CVS commit: src/crypto/dist/ipsec-tools/src/racoon
Date: Wed, 28 Jun 2017 21:13:05 +0000
SGVsbG8sDQoNCkp1c3QgY2hlY2tpbmcgaW4gdG8gc2VlIGlmIHRoaXMgd2FzIGZpeGVkLiAgV2Ug
YXJlIGxvb2tpbmcgdG8gcHVibGlzaCBkZXRhaWxzIGFib3V0IHRoaXMuICBUaGFua3MhDQoNCg0K
QmVzdCByZWdhcmRzLA0KDQpSb2JlcnQgRm9nZ2lhDQpTZWN1cml0eSBSZXNlYXJjaGVyLCBJbnRl
bGxpZ2VuY2UgVGVhbSwgU3BpZGVyTGFicw0KdDogKzEgMzEyLjg3My43Njk2DQogDQpUcnVzdHdh
dmUgfCBTTUFSVCBTRUNVUklUWSBPTiBERU1BTkQNCnd3dy50cnVzdHdhdmUuY29tIDxodHRwOi8v
d3d3LnRydXN0d2F2ZS5jb20vPg0KDQoNCg0KDQoNCg0KDQpPbiAxLzI0LzE3LCAxOjI1IFBNLCAi
Q2hyaXN0b3MgWm91bGFzIiA8Y2hyaXN0b3NAbmV0YnNkLm9yZz4gd3JvdGU6DQoNCj5UaGUgZm9s
bG93aW5nIHJlcGx5IHdhcyBtYWRlIHRvIFBSIHNlY3VyaXR5LzUxNjgyOyBpdCBoYXMgYmVlbiBu
b3RlZCBieSBHTkFUUy4NCj4NCj5Gcm9tOiAiQ2hyaXN0b3MgWm91bGFzIiA8Y2hyaXN0b3NAbmV0
YnNkLm9yZz4NCj5UbzogZ25hdHMtYnVnc0BnbmF0cy5OZXRCU0Qub3JnDQo+Q2M6IA0KPlN1Ympl
Y3Q6IFBSLzUxNjgyIENWUyBjb21taXQ6IHNyYy9jcnlwdG8vZGlzdC9pcHNlYy10b29scy9zcmMv
cmFjb29uDQo+RGF0ZTogVHVlLCAyNCBKYW4gMjAxNyAxNDoyMzo1NiAtMDUwMA0KPg0KPiBNb2R1
bGUgTmFtZToJc3JjDQo+IENvbW1pdHRlZCBCeToJY2hyaXN0b3MNCj4gRGF0ZToJCVR1ZSBKYW4g
MjQgMTk6MjM6NTYgVVRDIDIwMTcNCj4gDQo+IE1vZGlmaWVkIEZpbGVzOg0KPiAJc3JjL2NyeXB0
by9kaXN0L2lwc2VjLXRvb2xzL3NyYy9yYWNvb246IGhhbmRsZXIuaCBpc2FrbXAuYyBpc2FrbXBf
aW5mLmMNCj4gDQo+IExvZyBNZXNzYWdlOg0KPiBQUi81MTY4MjogQXZvaWQgRG9TIHdpdGggZnJh
Z21lbnQgb3V0IG9mIG9yZGVyIGluc2VydGlvbjsga2VlcCBmcmFnbWVudHMNCj4gc29ydGVkIGlu
IHRoZSBsaXN0Lg0KPiANCj4gDQo+IFRvIGdlbmVyYXRlIGEgZGlmZiBvZiB0aGlzIGNvbW1pdDoN
Cj4gY3ZzIHJkaWZmIC11IC1yMS4yNSAtcjEuMjYgc3JjL2NyeXB0by9kaXN0L2lwc2VjLXRvb2xz
L3NyYy9yYWNvb24vaGFuZGxlci5oDQo+IGN2cyByZGlmZiAtdSAtcjEuNzUgLXIxLjc2IHNyYy9j
cnlwdG8vZGlzdC9pcHNlYy10b29scy9zcmMvcmFjb29uL2lzYWttcC5jDQo+IGN2cyByZGlmZiAt
dSAtcjEuNTAgLXIxLjUxIFwNCj4gICAgIHNyYy9jcnlwdG8vZGlzdC9pcHNlYy10b29scy9zcmMv
cmFjb29uL2lzYWttcF9pbmYuYw0KPiANCj4gUGxlYXNlIG5vdGUgdGhhdCBkaWZmcyBhcmUgbm90
IHB1YmxpYyBkb21haW47IHRoZXkgYXJlIHN1YmplY3QgdG8gdGhlDQo+IGNvcHlyaWdodCBub3Rp
Y2VzIG9uIHRoZSByZWxldmFudCBmaWxlcy4NCj4gDQo=
From: Taylor R Campbell <riastradh@NetBSD.org>
To: Robert Foggia <RFoggia@trustwave.com>
Cc: "gnats-bugs@NetBSD.org" <gnats-bugs@NetBSD.org>,
"security-officer@netbsd.org" <security-officer@netbsd.org>,
"gnats-admin@netbsd.org" <gnats-admin@netbsd.org>, "security-alert@netbsd.org" <security-alert@netbsd.org>
Subject: Re: PR/51682 CVS commit: src/crypto/dist/ipsec-tools/src/racoon
Date: Thu, 29 Jun 2017 01:01:53 +0000
> Date: Wed, 28 Jun 2017 21:13:05 +0000
> From: Robert Foggia <RFoggia@trustwave.com>
>
> Just checking in to see if this was fixed. We are looking to
> publish details about this. Thanks!
It appears to have been fixed in HEAD, and thus will be in NetBSD 8.0.
The change has not been applied to older branches -- netbsd-7 or
netbsd-6 -- and I have not been paying enough attention to know
whether the bug occurs there too.
From: Jiri Bohac <jbohac@suse.cz>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: security/51682
Date: Wed, 12 Jul 2017 22:24:13 +0200
While reviewing the fix for #51682, I found what I believe is a
newly introduced regression / memory leak / DoS.
Bug scenario: the fragments arrive out of order; iph1->frag_chain
already contains some fragments; the newly received fragment has
the lowest frag_num so it should be inserted in the beginning of
the list:
> RCS file: /cvsroot/src/crypto/dist/ipsec-tools/src/racoon/isakmp_frag.c,v
> retrieving revision 1.5
> diff -u -u -r1.5 isakmp_frag.c
> --- isakmp_frag.c 22 Apr 2009 11:24:20 -0000 1.5
> +++ isakmp_frag.c 24 Jan 2017 02:29:15 -0000
> @@ -173,6 +173,38 @@
> return ntohl(hp[MD5_DIGEST_LENGTH / sizeof(*hp)]);
> }
>
> +static int
> +isakmp_frag_insert(struct ph1handle *iph1, struct isakmp_frag_item *item)
> +{
> + struct isakmp_frag_item *pitem = NULL;
> + struct isakmp_frag_item *citem = iph1->frag_chain;
> +
item is not the first to arrive, iph1->frag_chain != NULL,
so the fragment is not inserted here:
> + if (iph1->frag_chain == NULL) {
> + iph1->frag_chain = item;
> + return 0;
> + }
> +
> + do {
> + if (citem->frag_num == item->frag_num)
> + return -1;
> +
item is lowest-numbered fragment so we want to insert it now ...
> + if (citem->frag_num > item->frag_num) {
... but pitem is still NULL, we break doing nothing
> + if (pitem)
> + pitem->frag_next = item;
> + item->frag_next = citem;
> + break;
> + }
> +
> + pitem = citem;
> + citem = citem->frag_next;
> + } while (citem != NULL);
> +
> + /* we reached the end of the list, insert */
> + if (citem == NULL)
> + pitem->frag_next = item;
> + return 0;
We return 0.
The newly received fragment is not inserted in the list -> it's
lost (a regression) and never freed (possible DoS).
I think the correct fix would either need more special cases or
something simpler like: (untested)
static int
isakmp_frag_insert(struct ph1handle *iph1, struct isakmp_frag_item *item)
{
struct isakmp_frag_item **pitem = &iph1->frag_chain
struct isakmp_frag_item *citem = iph1->frag_chain;
while (citem != NULL) {
if (citem->frag_num == item->frag_num)
return -1;
if (citem->frag_num > item->frag_num) {
item->frag_next = citem;
break;
}
pitem = &citem->frag_next;
citem = citem->frag_next;
}
(*pitem)->frag_next = item;
return 0;
}
--
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, Prague, Czechia
From: =?utf-8?Q?Antoine_Beaupr=C3=A9?= <anarcat@orangeseeds.org>
To: gnats-bugs@NetBSD.org
Cc: Jiri Bohac <jbohac@suse.cz>
Subject: Re: security/51682
Date: Wed, 19 Jul 2017 12:39:33 -0400
--=-=-=
Content-Type: text/plain
Hi,
I agree there's something wrong with the code, although I would also
like to have ways of reproducing this. Working on this bug right now is
kind of a shot in the dark, and it seems numerous people here have
worked on PoC or have real world conditions to reproduce those
issues. It would be nice to share those so we can fix those issues
properly.
That said, I believe the change proposed by Jiri Bohac is incorrect: it
will fail if iph1->frag_chain is NULL, as it will try to dereference
that pointer and crash. I also find it confusing that pitem actually
refers to the same item as citem - not sure that works at all.
The original patch handled the empty list explicitely before the loop,
and I'm not sure I see how else to deal with that peculiar case, so I
kept that in my version. I also return explicitly within the loop to
remove any possible confusion.
static int
isakmp_frag_insert(struct ph1handle *iph1, struct isakmp_frag_item *item)
{
struct isakmp_frag_item *pitem = NULL;
struct isakmp_frag_item *citem = iph1->frag_chain;
/* no frag yet, just insert at beginning of list */
if (iph1->frag_chain == NULL) {
iph1->frag_chain = item;
return 0;
}
do {
/* duplicate fragment number, abort (CVE-2016-10396) */
if (citem->frag_num == item->frag_num)
return -1;
/* need to insert before current item */
if (citem->frag_num > item->frag_num) {
if (pitem != NULL)
pitem->frag_next = item
else
/* insert at the beginning of the list */
iph1->frag_chain = item;
item->frag_next = citem;
return 0;
}
pitem = citem;
citem = citem->frag_next;
} while (citem != NULL);
/* we reached the end of the list, insert */
pitem->frag_next = item;
return 0;
}
I think this covers all cases (where are unit tests when you need
them!), described in the comments:
1. no frag yet: just insert at beginning of list
2. duplicate fragment number: abort (CVE-2016-10396)
3. the normal case, where we reach the end of the list and insert and
the end
4. the out of order case, where we insert elsewhere in the list
5. an out of order special case, where insert at the beginning of the
list but correctly linking with the remains of the list
The final patch is attached.
A.
--
Only in the darkness can you see the stars.
- Martin Luther King, Jr.
--=-=-=
Content-Type: text/x-diff
Content-Disposition: inline; filename=CVE-2016-10396.patch
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
--- a/src/racoon/handler.h
+++ b/src/racoon/handler.h
@@ -141,6 +141,7 @@ struct ph1handle {
#endif
#ifdef ENABLE_FRAG
int frag; /* IKE phase 1 fragmentation */
+ int frag_last_index;
struct isakmp_frag_item *frag_chain; /* Received fragments */
#endif
--- a/src/racoon/isakmp.c
+++ b/src/racoon/isakmp.c
@@ -1072,6 +1072,7 @@ isakmp_ph1begin_i(rmconf, remote, local)
iph1->frag = 1;
else
iph1->frag = 0;
+ iph1->frag_last_index = 0;
iph1->frag_chain = NULL;
#endif
iph1->approval = NULL;
@@ -1176,6 +1177,7 @@ isakmp_ph1begin_r(msg, remote, local, et
#endif
#ifdef ENABLE_FRAG
iph1->frag = 0;
+ iph1->frag_last_index = 0;
iph1->frag_chain = NULL;
#endif
iph1->approval = NULL;
--- a/src/racoon/isakmp_frag.c
+++ b/src/racoon/isakmp_frag.c
@@ -1,4 +1,4 @@
-/* $NetBSD: isakmp_frag.c,v 1.5 2009/04/22 11:24:20 tteras Exp $ */
+/* $NetBSD: isakmp_frag.c,v 1.5.36.1 2017/04/21 16:50:42 bouyer Exp $ */
/* Id: isakmp_frag.c,v 1.4 2004/11/13 17:31:36 manubsd Exp */
@@ -173,6 +173,43 @@ vendorid_frag_cap(gen)
return ntohl(hp[MD5_DIGEST_LENGTH / sizeof(*hp)]);
}
+static int
+isakmp_frag_insert(struct ph1handle *iph1, struct isakmp_frag_item *item)
+{
+ struct isakmp_frag_item *pitem = NULL;
+ struct isakmp_frag_item *citem = iph1->frag_chain;
+
+ /* no frag yet, just insert at beginning of list */
+ if (iph1->frag_chain == NULL) {
+ iph1->frag_chain = item;
+ return 0;
+ }
+
+ do {
+ /* duplicate fragment number, abort (CVE-2016-10396) */
+ if (citem->frag_num == item->frag_num)
+ return -1;
+
+ /* need to insert before current item */
+ if (citem->frag_num > item->frag_num) {
+ if (pitem != NULL)
+ pitem->frag_next = item;
+ else
+ /* insert at the beginning of the list */
+ iph1->frag_chain = item;
+ item->frag_next = citem;
+ return 0;
+ }
+
+ pitem = citem;
+ citem = citem->frag_next;
+ } while (citem != NULL);
+
+ /* we reached the end of the list, insert */
+ pitem->frag_next = item;
+ return 0;
+}
+
int
isakmp_frag_extract(iph1, msg)
struct ph1handle *iph1;
@@ -224,39 +261,43 @@ isakmp_frag_extract(iph1, msg)
item->frag_next = NULL;
item->frag_packet = buf;
- /* Look for the last frag while inserting the new item in the chain */
- if (item->frag_last)
- last_frag = item->frag_num;
+ /* Check for the last frag before inserting the new item in the chain */
+ if (item->frag_last) {
+ /* if we have the last fragment, indices must match */
+ if (iph1->frag_last_index != 0 &&
+ item->frag_last != iph1->frag_last_index) {
+ plog(LLV_ERROR, LOCATION, NULL,
+ "Repeated last fragment index mismatch\n");
+ racoon_free(item);
+ vfree(buf);
+ return -1;
+ }
- if (iph1->frag_chain == NULL) {
- iph1->frag_chain = item;
- } else {
- struct isakmp_frag_item *current;
+ last_frag = iph1->frag_last_index = item->frag_num;
+ }
- current = iph1->frag_chain;
- while (current->frag_next) {
- if (current->frag_last)
- last_frag = item->frag_num;
- current = current->frag_next;
- }
- current->frag_next = item;
+ /* insert fragment into chain */
+ if (isakmp_frag_insert(iph1, item) == -1) {
+ plog(LLV_ERROR, LOCATION, NULL,
+ "Repeated fragment index mismatch\n");
+ racoon_free(item);
+ vfree(buf);
+ return -1;
}
- /* If we saw the last frag, check if the chain is complete */
+ /* If we saw the last frag, check if the chain is complete
+ * we have a sorted list now, so just walk through */
if (last_frag != 0) {
+ item = iph1->frag_chain;
for (i = 1; i <= last_frag; i++) {
- item = iph1->frag_chain;
- do {
- if (item->frag_num == i)
- break;
- item = item->frag_next;
- } while (item != NULL);
-
+ if (item->frag_num != i)
+ break;
+ item = item->frag_next;
if (item == NULL) /* Not found */
break;
}
- if (item != NULL) /* It is complete */
+ if (i > last_frag) /* It is complete */
return 1;
}
@@ -291,15 +332,9 @@ isakmp_frag_reassembly(iph1)
}
data = buf->v;
+ item = iph1->frag_chain;
for (i = 1; i <= frag_count; i++) {
- item = iph1->frag_chain;
- do {
- if (item->frag_num == i)
- break;
- item = item->frag_next;
- } while (item != NULL);
-
- if (item == NULL) {
+ if (item->frag_num != i) {
plog(LLV_ERROR, LOCATION, NULL,
"Missing fragment #%d\n", i);
vfree(buf);
@@ -308,6 +343,7 @@ isakmp_frag_reassembly(iph1)
}
memcpy(data, item->frag_packet->v, item->frag_packet->l);
data += item->frag_packet->l;
+ item = item->frag_next;
}
out:
--=-=-=--
From: Jiri Bohac <jbohac@suse.cz>
To: Antoine =?iso-8859-1?Q?Beaupr=E9?= <anarcat@orangeseeds.org>
Cc: gnats-bugs@NetBSD.org
Subject: Re: security/51682
Date: Thu, 20 Jul 2017 12:26:14 +0200
On Wed, Jul 19, 2017 at 12:39:33PM -0400, Antoine Beaupré wrote:
> That said, I believe the change proposed by Jiri Bohac is incorrect: it
> will fail if iph1->frag_chain is NULL, as it will try to dereference
> that pointer and crash. I also find it confusing that pitem actually
> refers to the same item as citem - not sure that works at all.
Right. I wanted to write:
*pitem = item;
instead of
(*pitem)->frag_next = item;
Thanks for spotting this!
pitem is a pointer-to-a-pointer; it is initialized with
&iph1->frag_chain, so if iph1->frag_chain is NULL
*pitem = item
will assign to iph1->frag_chain.
Antoine Beaupré's patch looks good to me as well.
--
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, Prague, Czechia
From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/51682 CVS commit: src/crypto/dist/ipsec-tools/src/racoon
Date: Sun, 23 Jul 2017 01:40:28 -0400
Module Name: src
Committed By: christos
Date: Sun Jul 23 05:40:28 UTC 2017
Modified Files:
src/crypto/dist/ipsec-tools/src/racoon: isakmp_frag.c
Log Message:
PR/51682: Antoine Beaupré: Simplify and comment previous patch.
XXX: pullup-8
To generate a diff of this commit:
cvs rdiff -u -r1.6 -r1.7 src/crypto/dist/ipsec-tools/src/racoon/isakmp_frag.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: "Manuel Bouyer" <bouyer@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/51682 CVS commit: [netbsd-8] src/crypto/dist/ipsec-tools/src/racoon
Date: Thu, 31 Aug 2017 08:50:57 +0000
Module Name: src
Committed By: bouyer
Date: Thu Aug 31 08:50:57 UTC 2017
Modified Files:
src/crypto/dist/ipsec-tools/src/racoon [netbsd-8]: isakmp_frag.c
Log Message:
Pull up following revision(s) (requested by christos in ticket #233):
crypto/dist/ipsec-tools/src/racoon/isakmp_frag.c: revision 1.7
PR/51682: Antoine Beaupr?: Simplify and comment previous patch.
XXX: pullup-8
To generate a diff of this commit:
cvs rdiff -u -r1.6 -r1.6.4.1 \
src/crypto/dist/ipsec-tools/src/racoon/isakmp_frag.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
(Contact us)
$NetBSD: query-full-pr,v 1.39 2013/11/01 18:47:49 spz Exp $
$NetBSD: gnats_config.sh,v 1.8 2006/05/07 09:23:38 tsutsui Exp $
Copyright © 1994-2014
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.
Home