[src/trunk]: src/crypto/dist/ipsec-tools/src/racoon From Thomas Reim:

[christos <christos%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/d257050b7902
branches:  trunk
changeset: 836241:d257050b7902
user:      christos <christos%NetBSD.org@localhost>
date:      Fri Oct 05 20:12:37 2018 +0000

description:
>From Thomas Reim:

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.

XXX: pullup-8

diffstat:

 crypto/dist/ipsec-tools/src/racoon/isakmp_frag.c |  77 +++++++++++++++--------
 1 files changed, 51 insertions(+), 26 deletions(-)

diffs (125 lines):

diff -r 920c3200834a -r d257050b7902 crypto/dist/ipsec-tools/src/racoon/isakmp_frag.c
--- a/crypto/dist/ipsec-tools/src/racoon/isakmp_frag.c  Fri Oct 05 19:53:47 2018 +0000
+++ b/crypto/dist/ipsec-tools/src/racoon/isakmp_frag.c  Fri Oct 05 20:12:37 2018 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: isakmp_frag.c,v 1.9 2018/10/02 18:49:24 christos Exp $ */
+/*     $NetBSD: isakmp_frag.c,v 1.10 2018/10/05 20:12:37 christos Exp $        */
 
 /* Id: isakmp_frag.c,v 1.4 2004/11/13 17:31:36 manubsd Exp */
 
@@ -219,10 +219,15 @@
        struct isakmp_frag *frag;
        struct isakmp_frag_item *item;
        vchar_t *buf;
-       int last_frag = 0;
+       const char *m;
        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,47 +265,66 @@
        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) {
-                       plog(LLV_ERROR, LOCATION, NULL,
-                            "Repeated last fragment index mismatch\n");
-                       racoon_free(item);
-                       vfree(buf);
-                       return -1;
+
+       /* 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) {
+                       m = "Message has multiple tail fragments\n";
+                       goto out;
                }
 
-               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) {
+                       m = "Fragment number greater than tail fragment number\n";
+                       goto out;
+               }
        }
 
        /* 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;
+               m = "Duplicate fragment number\n";
+               goto out;
        }
 
+       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)
-                               break;
+               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 */
-                       return 1;
+               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;
+out:
+       plog(LLV_ERROR, LOCATION, NULL, m);
+       racoon_free(item);
+       vfree(buf);
+       return -1;
 }
 
 vchar_t *
@@ -359,6 +383,7 @@
        } while (item != NULL);
 
        iph1->frag_chain = NULL;
+       iph1->frag_last_index = 0;
 
        return buf;
 }