NetBSD Problem Report #53646
From www@NetBSD.org Tue Oct 2 15:59:00 2018
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 "mail.NetBSD.org CA" (not verified))
by mollari.NetBSD.org (Postfix) with ESMTPS id 4CD357A151
for <gnats-bugs@gnats.NetBSD.org>; Tue, 2 Oct 2018 15:59:00 +0000 (UTC)
Message-Id: <20181002155858.D83E67A212@mollari.NetBSD.org>
Date: Tue, 2 Oct 2018 15:58:58 +0000 (UTC)
From: reimth@gmail.com
Reply-To: reimth@gmail.com
To: gnats-bugs@NetBSD.org
Subject: [racoon] CVE CVE-2016-10396 Patch Regression
X-Send-Pr-Version: www-1.0
>Number: 53646
>Category: security
>Synopsis: [racoon] CVE CVE-2016-10396 Patch Regression
>Confidential: no
>Severity: serious
>Priority: high
>Responsible: security-officer
>State: closed
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Tue Oct 02 16:00:00 +0000 2018
>Closed-Date: Fri Jun 21 22:33:59 +0000 2024
>Last-Modified: Fri Jun 21 22:33:59 +0000 2024
>Originator: Thomas Reim
>Release: -current
>Organization:
>Environment:
Ubuntu 18.04. LTS
>Description:
In 2017 a CVE was detected for raccoon (NetBSD Problem Report #51682). NetBSD maintainers provided a rough fix, as the ipsec-tools project has not provided updates for racoon since 2014. Ubuntu has included the related patch starting from artful (17.10). When applying NetBSD's CVE-2016-10396 patch Apple iPhones, which use a raccoon client for IPSec based VPN access, cannot connect anymore to racoon VPN on the racoon server. Following server log entries outline the failure:
Sep 14 06:42:28 vpnserver racoon[1775]: ERROR: Repeated fragment index mismatch
Sep 14 06:42:28 vpnserver racoon[1775]: ERROR: Repeated last fragment index mismatch
Sep 14 06:42:32 vpnserver racoon[1775]: ERROR: Repeated fragment index mismatch
Sep 14 06:42:32 vpnserver racoon[1775]: ERROR: Repeated last fragment index mismatch
Sep 14 06:42:35 vpnserver racoon[1775]: ERROR: Repeated fragment index mismatch
Sep 14 06:42:35 vpnserver racoon[1775]: ERROR: Repeated last fragment index mismatch
Sep 14 06:42:35 vpnserver racoon[1775]: ERROR: Repeated fragment index mismatch
Sep 14 06:42:35 vpnserver racoon[1775]: ERROR: Repeated last fragment index mismatch
Sep 14 06:42:39 vpnserver racoon[1775]: ERROR: phase1 negotiation failed due to time up.
>How-To-Repeat:
Code review and/or test with iPhone ios 11 IPsec VPN client
A code review clearly shows a logical error in function isakmp_frag_extract() within file crypto/dist/ipsec-tools/src/racoon/isakmp_frag.c which prevents raccoon server from identifying a completely reassembled isakmp fragment chain. This forces racoon clients like Apple iPhones that fragment isakmp messages to retransmit fragemnts which leads to a similar behaviour than the DoS attack, that developers wanted racoon servers to be protect from. So in turn, after a couple of retransmissions racoon server terminates pahse 1 negotiation. This prevents the fragmenting client from accessing the VPN. (service outage)
>Fix:
A bug fix has been provided for Ubuntu. Please refer to https://launchpad.net/~rdratlos/+archive/ubuntu/racoon. The PPA also provides a link to the related downstream bug report.
>Release-Note:
>Audit-Trail:
From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/53646 CVS commit: src/crypto/dist/ipsec-tools/src/racoon
Date: Tue, 2 Oct 2018 14:49:24 -0400
Module Name: src
Committed By: christos
Date: Tue Oct 2 18:49:24 UTC 2018
Modified Files:
src/crypto/dist/ipsec-tools/src/racoon: isakmp_frag.c
Log Message:
PR/53646: Thomas Reim: Incorrect detection of the packet complete code in
fragment list check.
While the fix in https://launchpad.net/~rdratlos/+archive/ubuntu/racoon
- if (i > last_frag) /* It is complete */
+ if (i >= last_frag) /* It is complete */
has the correct behavior, it violates the test for successful
completion of the invariant of the loop:
for (i = 1; i <= last_frag; i++) {
if (!check_fragment_index())
break;
}
if (i > last_frag)
return ok;
It is better to move the check for NULL in the loop earlier, so that
the final iteration is done and the test is kept the same. It makes
the code easier to understand and preserves the original intent.
XXX: pullup-8
To generate a diff of this commit:
cvs rdiff -u -r1.8 -r1.9 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.com (Christos Zoulas)
To: gnats-bugs@NetBSD.org, security-officer@netbsd.org,
gnats-admin@netbsd.org, security-alert@netbsd.org
Cc:
Subject: Re: security/53646: [racoon] CVE CVE-2016-10396 Patch Regression
Date: Tue, 2 Oct 2018 20:01:34 -0400
On Oct 2, 4:00pm, reimth@gmail.com (reimth@gmail.com) wrote:
-- Subject: security/53646: [racoon] CVE CVE-2016-10396 Patch Regression
The patch in https://launchpad.net/~rdratlos/+archive/ubuntu/racoon is
not entirely correct because is will misclassify a packet with an incorrect
fragment id in the last fragment as complete.
christos
State-Changed-From-To: open->feedback
State-Changed-By: christos@NetBSD.org
State-Changed-When: Wed, 03 Oct 2018 08:37:10 -0400
State-Changed-Why:
My previous comment about misclassifying the packet was incorrect.
The last iteration correctly tests the fragment id, but returns
before incrementing the index. (the original patch works properly too)
From: "Thomas A. Reim" <reimth@gmail.com>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: security/53646 ([racoon] CVE CVE-2016-10396 Patch Regression)
Date: Fri, 5 Oct 2018 02:58:02 +0200
After digging deeper into the code and the IKE extension specifications
there's still regression left in the code from introduction of the CVE
patch. Below there is a subsequent patch, whcih should fix the major
issues. Please review.
From here there is one open point left: A timeout to delete the
fragment queue would be beneficial. IKE phase 1 is a long-running
process, which can continue to receive IKE fragments (e. g. duplicates)
even after succesful reassembly of the phase 1 message. Such fragments
would be stored in a (new) queue until the VPN connection is terminated.
Feedback is welcome.
The patch can also be found in GitHub:
https://github.com/rdratlos/src/commit/b5f049ae16cb8666ac96b45b6a09e56583057b89
=========
From b5f049ae16cb8666ac96b45b6a09e56583057b89 Mon Sep 17 00:00:00 2001
From: Thomas Reim <reimth@gmail.com>
Date: Wed, 3 Oct 2018 22:24:42 +0200
Subject: [PATCH] PR/53646: Incorrect handling of last fragments
Yet another patch after fixing packet complete code:
Current racoon code cannot detect duplicate last fragments as it uses
the fragment flag instead of the fragment number.
The code does not consider that the IKE payload fragments might not be
received in the correct order. In this case, packet complete detection
will again fail and VPN clients abandoned from VPN service.
Nevertheless, clients still can add fragments to the fragment queue and
fill it up to the possible 255 fragments. Only duplicates are detected,
but not the fragments with a number greater than the last fragment
number.
The last fragment number is kept in the Phase 1 handler
after fragment queue deletion, which may lead to error notifications
after succesful reassembly of the IKE phase 1 message.
In general, the 2017's CVE fix added laconic and difficult to understand
failure notifications, which do not much help for analysis, why a VPN
client was blocked by racoon server.
This patch fixes the code and aligns it to Microsoft/Cisco IKE
fragmentation specification. It provides error logging which is in line
with above specification and adds some debug info to the logs to better
support analysis VPN client blackballing.
Signed-off-by: Thomas Reim <reimth@gmail.com>
---
.../dist/ipsec-tools/src/racoon/isakmp_frag.c | 59 ++++++++++++++-----
1 file changed, 44 insertions(+), 15 deletions(-)
diff --git a/crypto/dist/ipsec-tools/src/racoon/isakmp_frag.c
b/crypto/dist/ipsec-tools/src/racoon/isakmp_frag.c
index 29c3d7489..688d191f5 100644
--- a/crypto/dist/ipsec-tools/src/racoon/isakmp_frag.c
+++ b/crypto/dist/ipsec-tools/src/racoon/isakmp_frag.c
@@ -219,10 +219,14 @@ isakmp_frag_extract(iph1, msg)
struct isakmp_frag *frag;
struct isakmp_frag_item *item;
vchar_t *buf;
- int last_frag = 0;
char *data;
int i;
+ if (iph1->frag_chain == NULL) {
+ plog(LLV_DEBUG, LOCATION, NULL,
+ "fragmented IKE phase 1 message payload detected\n");
+ }
+
if (msg->l < sizeof(*isakmp) + sizeof(*frag)) {
plog(LLV_ERROR, LOCATION, NULL, "Message too short\n");
return -1;
@@ -260,44 +264,68 @@ isakmp_frag_extract(iph1, msg)
item->frag_next = NULL;
item->frag_packet = buf;
- /* 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) {
+
+ /* Perform required last frag checks before inserting the new item in
+ the chain */
+ if (iph1->frag_last_index != 0) {
+ /* Only one fragment payload allowed with last frag flag set */
+ if (item->frag_last) {
plog(LLV_ERROR, LOCATION, NULL,
- "Repeated last fragment index mismatch\n");
+ "Message has multiple tail fragments\n");
racoon_free(item);
vfree(buf);
return -1;
}
- last_frag = iph1->frag_last_index = item->frag_num;
+ /* Fragment payload with fragment number greater than the
+ fragment number of the last fragment is not allowed*/
+ if (item->frag_num > iph1->frag_last_index) {
+ plog(LLV_ERROR, LOCATION, NULL,
+ "Fragment number greater than tail fragment number\n");
+ racoon_free(item);
+ vfree(buf);
+ return -1;
+ }
}
/* insert fragment into chain */
if (isakmp_frag_insert(iph1, item) == -1) {
plog(LLV_ERROR, LOCATION, NULL,
- "Repeated fragment index mismatch\n");
+ "Duplicate fragment number\n");
racoon_free(item);
vfree(buf);
return -1;
}
+ plog(LLV_DEBUG, LOCATION, NULL,
+ "fragment payload #%d queued\n",
+ item->frag_num);
+
+ /* remember last frag after insertion into fragment chain */
+ if (item->frag_last)
+ iph1->frag_last_index = item->frag_num;
+
/* 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) {
+ if (iph1->frag_last_index != 0) {
item = iph1->frag_chain;
- for (i = 1; i <= last_frag; i++) {
- if (item == NULL) /* Not found */
- break;
- if (item->frag_num != i)
+ for (i = 1; i <= iph1->frag_last_index; i++) {
+ if (item == NULL ||
+ item->frag_num != i) {
+ plog(LLV_DEBUG, LOCATION, NULL,
+ "fragment payload #%d still missing\n",
+ i);
break;
+ }
item = item->frag_next;
}
- if (i > last_frag) /* It is complete */
+ if (i > iph1->frag_last_index) {/* It is complete */
+ plog(LLV_DEBUG, LOCATION, NULL,
+ "fragment #%d completed IKE phase 1 message payload.\n",
+ frag->index);
return 1;
+ }
}
return 0;
@@ -359,6 +387,7 @@ out:
} while (item != NULL);
iph1->frag_chain = NULL;
+ iph1->frag_last_index = 0;
return buf;
}
--
2.17.1
From: christos@zoulas.com (Christos Zoulas)
To: gnats-bugs@NetBSD.org, security-officer@netbsd.org,
gnats-admin@netbsd.org, security-alert@netbsd.org, reimth@gmail.com
Cc:
Subject: Re: security/53646 ([racoon] CVE CVE-2016-10396 Patch Regression)
Date: Fri, 5 Oct 2018 16:13:39 -0400
On Oct 5, 1:00am, reimth@gmail.com ("Thomas A. Reim") wrote:
-- Subject: Re: security/53646 ([racoon] CVE CVE-2016-10396 Patch Regression)
Thanks a lot, the code looks a lot better now. I just merged the error
cleanup path. Please check that I did not mess it up :-)
Best,
christos
From: Thomas Reim <reimth@gmail.com>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: security/53646 ([racoon] CVE CVE-2016-10396 Patch Regression)
Date: Thu, 11 Oct 2018 09:54:37 +0200
> Thanks a lot, the code looks a lot better now. I just merged the error
> cleanup path. Please check that I did not mess it up :-)
Looks perfect. Thanks a lot.
BR Thomas
State-Changed-From-To: feedback->needs-pullups
State-Changed-By: maya@NetBSD.org
State-Changed-When: Wed, 28 Nov 2018 07:17:51 +0000
State-Changed-Why:
Feedback ok, now need to sort out pullups.
State-Changed-From-To: needs-pullups->closed
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Fri, 21 Jun 2024 22:33:59 +0000
State-Changed-Why:
8 is eol
>Unformatted:
(Contact us)
$NetBSD: query-full-pr,v 1.47 2022/09/11 19:34:41 kim Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2024
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.
Home