Due to the design of the IKEv2 protocol, the receiver does not
know which policy the initiator tries to negotiate an SA for
until the second exchange (IKE_AUTH). The IKE_AUTH request contains
the ID payload which the responder uses to match a policy (and lookup
authentication keys).
Until then, iked will fall back to the default policy and then update
it later.
The cryptographic proposal and key exchange on the other hand are
negotiated in the first exchange (IKE_SA_INIT), when the responder
does not know the initiators ID and thus the actual policy.

The attached diff adds an additional check to make sure that, when
the policy changes in IKE_AUTH due to the ID belonging to a different
policy, the negotiated SA's proposal is still compatible with the
updated policy.

ok?

Index: iked.h
===================================================================
RCS file: /mount/openbsd/cvs/src/sbin/iked/iked.h,v
retrieving revision 1.134
diff -u -p -r1.134 iked.h
--- iked.h      21 Feb 2020 15:17:34 -0000      1.134
+++ iked.h      26 Feb 2020 21:53:53 -0000
@@ -803,6 +803,8 @@ struct iked_sa *
         sa_lookup(struct iked *, uint64_t, uint64_t, unsigned int);
 struct iked_user *
         user_lookup(struct iked *, const char *);
+int     proposals_negotiate(struct iked_proposals *, struct iked_proposals *,
+           struct iked_proposals *, int);
 RB_PROTOTYPE(iked_sas, iked_sa, sa_entry, sa_cmp);
 RB_PROTOTYPE(iked_addrpool, iked_sa, sa_addrpool_entry, sa_addrpool_cmp);
 RB_PROTOTYPE(iked_addrpool6, iked_sa, sa_addrpool6_entry, sa_addrpool6_cmp);
@@ -859,8 +861,6 @@ ssize_t      dsa_verify_final(struct iked_ds
 pid_t   ikev2(struct privsep *, struct privsep_proc *);
 void    ikev2_recv(struct iked *, struct iked_message *);
 void    ikev2_init_ike_sa(struct iked *, void *);
-int     ikev2_sa_negotiate(struct iked_proposals *, struct iked_proposals *,
-           struct iked_proposals *, int);
 int     ikev2_policy2id(struct iked_static_id *, struct iked_id *, int);
 int     ikev2_childsa_enable(struct iked *, struct iked_sa *);
 int     ikev2_childsa_delete(struct iked *, struct iked_sa *,
Index: ikev2.c
===================================================================
RCS file: /mount/openbsd/cvs/src/sbin/iked/ikev2.c,v
retrieving revision 1.189
diff -u -p -r1.189 ikev2.c
--- ikev2.c     21 Feb 2020 15:17:34 -0000      1.189
+++ ikev2.c     26 Feb 2020 21:53:53 -0000
@@ -115,8 +115,6 @@ int  ikev2_childsa_negotiate(struct iked
            struct iked_kex *, struct iked_proposals *, int, int, int);
 int     ikev2_childsa_delete_proposed(struct iked *, struct iked_sa *,
            struct iked_proposals *);
-int     ikev2_match_proposals(struct iked_proposal *, struct iked_proposal *,
-           struct iked_transform **, int);
 int     ikev2_valid_proposal(struct iked_proposal *,
            struct iked_transform **, struct iked_transform **, int *);
 
@@ -749,7 +747,7 @@ ikev2_ike_auth_recv(struct iked *env, st
        }
 
        if (!TAILQ_EMPTY(&msg->msg_proposals)) {
-               if (ikev2_sa_negotiate(&sa->sa_proposals,
+               if (proposals_negotiate(&sa->sa_proposals,
                    &sa->sa_policy->pol_proposals, &msg->msg_proposals,
                    0) != 0) {
                        log_info("%s: no proposal chosen", __func__);
@@ -3509,7 +3507,7 @@ ikev2_init_create_child_sa(struct iked *
                return (-1);
        }
 
-       if (ikev2_sa_negotiate(&sa->sa_proposals, &sa->sa_proposals,
+       if (proposals_negotiate(&sa->sa_proposals, &sa->sa_proposals,
            &msg->msg_proposals, 1) != 0) {
                log_debug("%s: no proposal chosen", __func__);
                return (-1);
@@ -3622,7 +3620,7 @@ ikev2_init_create_child_sa(struct iked *
                }
                pfs = 1;
                /* XXX check group against policy ? */
-               /* XXX should ikev2_sa_negotiate do this? */
+               /* XXX should proposals_negotiate do this? */
        }
 
        /* Update responder's nonce */
@@ -3977,7 +3975,7 @@ ikev2_resp_create_child_sa(struct iked *
                        goto fail;
                }
 
-               if (ikev2_sa_negotiate(&proposals,
+               if (proposals_negotiate(&proposals,
                    &sa->sa_policy->pol_proposals, &msg->msg_proposals,
                    1) != 0) {
                        log_info("%s: no proposal chosen", __func__);
@@ -4400,178 +4398,6 @@ ikev2_psk(struct iked_sa *sa, uint8_t *d
 }
 
 int
-ikev2_match_proposals(struct iked_proposal *local, struct iked_proposal *peer,
-    struct iked_transform **xforms, int rekey)
-{
-       struct iked_transform   *tpeer, *tlocal;
-       unsigned int             i, j, type, score, requiredh = 0;
-       uint8_t                  protoid = peer->prop_protoid;
-       uint8_t                  peerxfs[IKEV2_XFORMTYPE_MAX];
-
-       bzero(peerxfs, sizeof(peerxfs));
-
-       for (i = 0; i < peer->prop_nxforms; i++) {
-               tpeer = peer->prop_xforms + i;
-               if (tpeer->xform_type > IKEV2_XFORMTYPE_MAX)
-                       continue;
-
-               /*
-                * Record all transform types from the peer's proposal,
-                * because if we want this proposal we have to select
-                * a transform for each proposed transform type.
-                */
-               peerxfs[tpeer->xform_type] = 1;
-
-               for (j = 0; j < local->prop_nxforms; j++) {
-                       tlocal = local->prop_xforms + j;
-
-                       /*
-                        * We require a DH group for ESP if there is any
-                        * local proposal with DH enabled.
-                        */
-                       if (rekey && requiredh == 0 &&
-                           protoid == IKEV2_SAPROTO_ESP &&
-                           tlocal->xform_type == IKEV2_XFORMTYPE_DH)
-                               requiredh = 1;
-
-                       /* Compare peer and local proposals */
-                       if (tpeer->xform_type != tlocal->xform_type ||
-                           tpeer->xform_id != tlocal->xform_id ||
-                           tpeer->xform_length != tlocal->xform_length)
-                               continue;
-                       type = tpeer->xform_type;
-
-                       if (xforms[type] == NULL || tlocal->xform_score <
-                           xforms[type]->xform_score) {
-                               xforms[type] = tlocal;
-                       } else
-                               continue;
-
-                       print_debug("%s: xform %d <-> %d (%d): %s %s "
-                           "(keylength %d <-> %d)", __func__,
-                           peer->prop_id, local->prop_id, tlocal->xform_score,
-                           print_map(type, ikev2_xformtype_map),
-                           print_map(tpeer->xform_id, tpeer->xform_map),
-                           tpeer->xform_keylength, tlocal->xform_keylength);
-                       if (tpeer->xform_length)
-                               print_debug(" %d", tpeer->xform_length);
-                       print_debug("\n");
-               }
-       }
-
-       for (i = score = 0; i < IKEV2_XFORMTYPE_MAX; i++) {
-               if (protoid == IKEV2_SAPROTO_IKE && xforms[i] == NULL &&
-                   (i == IKEV2_XFORMTYPE_ENCR || i == IKEV2_XFORMTYPE_PRF ||
-                    i == IKEV2_XFORMTYPE_INTEGR || i == IKEV2_XFORMTYPE_DH)) {
-                       score = 0;
-                       break;
-               } else if (protoid == IKEV2_SAPROTO_AH && xforms[i] == NULL &&
-                   (i == IKEV2_XFORMTYPE_INTEGR || i == IKEV2_XFORMTYPE_ESN)) {
-                       score = 0;
-                       break;
-               } else if (protoid == IKEV2_SAPROTO_ESP && xforms[i] == NULL &&
-                   (i == IKEV2_XFORMTYPE_ENCR || i == IKEV2_XFORMTYPE_ESN ||
-                   (requiredh && i == IKEV2_XFORMTYPE_DH))) {
-                       score = 0;
-                       break;
-               } else if (peerxfs[i] && xforms[i] == NULL) {
-                       score = 0;
-                       break;
-               } else if (xforms[i] == NULL)
-                       continue;
-
-               score += xforms[i]->xform_score;
-       }
-
-       return (score);
-}
-
-/*
- * The 'rekey' parameter indicates a CREATE_CHILD_SA exchange where
- * an extra group is necessary for PFS. For the initial IKE_AUTH exchange
- * the ESP SA proposal never includes an explicit DH group.
- */
-int
-ikev2_sa_negotiate(struct iked_proposals *result, struct iked_proposals *local,
-    struct iked_proposals *peer, int rekey)
-{
-       struct iked_proposal    *ppeer = NULL, *plocal, *prop, vpeer, vlocal;
-       struct iked_transform    chosen[IKEV2_XFORMTYPE_MAX];
-       struct iked_transform   *valid[IKEV2_XFORMTYPE_MAX];
-       struct iked_transform   *match[IKEV2_XFORMTYPE_MAX];
-       unsigned int             i, score, chosen_score = 0;
-       uint8_t                  protoid = 0;
-
-       bzero(valid, sizeof(valid));
-       bzero(&vlocal, sizeof(vlocal));
-       bzero(&vpeer, sizeof(vpeer));
-
-       if (TAILQ_EMPTY(peer)) {
-               log_debug("%s: peer did not send %s proposals", __func__,
-                   print_map(protoid, ikev2_saproto_map));
-               return (-1);
-       }
-
-       TAILQ_FOREACH(plocal, local, prop_entry) {
-               TAILQ_FOREACH(ppeer, peer, prop_entry) {
-                       if (ppeer->prop_protoid != plocal->prop_protoid)
-                               continue;
-                       bzero(match, sizeof(match));
-                       score = ikev2_match_proposals(plocal, ppeer, match,
-                           rekey);
-                       log_debug("%s: score %d", __func__, score);
-                       if (score && (!chosen_score || score < chosen_score)) {
-                               chosen_score = score;
-                               for (i = 0; i < IKEV2_XFORMTYPE_MAX; i++) {
-                                       if ((valid[i] = match[i]))
-                                               memcpy(&chosen[i], match[i],
-                                                   sizeof(chosen[0]));
-                               }
-                               memcpy(&vpeer, ppeer, sizeof(vpeer));
-                               memcpy(&vlocal, plocal, sizeof(vlocal));
-                       }
-               }
-               if (chosen_score != 0)
-                       break;
-       }
-
-       if (chosen_score == 0)
-               return (-1);
-       else if (result == NULL)
-               return (0);
-
-       (void)config_free_proposals(result, vpeer.prop_protoid);
-       prop = config_add_proposal(result, vpeer.prop_id, vpeer.prop_protoid);
-
-       if (vpeer.prop_localspi.spi_size) {
-               prop->prop_localspi.spi_size = vpeer.prop_localspi.spi_size;
-               prop->prop_peerspi = vpeer.prop_peerspi;
-       }
-       if (vlocal.prop_localspi.spi_size) {
-               prop->prop_localspi.spi_size = vlocal.prop_localspi.spi_size;
-               prop->prop_localspi.spi = vlocal.prop_localspi.spi;
-       }
-
-       for (i = 0; i < IKEV2_XFORMTYPE_MAX; i++) {
-               if (valid[i] == NULL)
-                       continue;
-               print_debug("%s: score %d: %s %s", __func__,
-                   chosen[i].xform_score, print_map(i, ikev2_xformtype_map),
-                   print_map(chosen[i].xform_id, chosen[i].xform_map));
-               if (chosen[i].xform_length)
-                       print_debug(" %d", chosen[i].xform_length);
-               print_debug("\n");
-
-               if (config_add_transform(prop, chosen[i].xform_type,
-                   chosen[i].xform_id, chosen[i].xform_length,
-                   chosen[i].xform_keylength) == NULL)
-                       break;
-       }
-
-       return (0);
-}
-
-int
 ikev2_sa_initiator_dh(struct iked_sa *sa, struct iked_message *msg,
     unsigned int proto)
 {
@@ -4663,7 +4489,7 @@ ikev2_sa_initiator(struct iked *env, str
        }
 
        /* XXX we need a better way to get this */
-       if (ikev2_sa_negotiate(&sa->sa_proposals,
+       if (proposals_negotiate(&sa->sa_proposals,
            &msg->msg_policy->pol_proposals, &msg->msg_proposals, 0) != 0) {
                log_info("%s: no proposal chosen", __func__);
                msg->msg_error = IKEV2_N_NO_PROPOSAL_CHOSEN;
@@ -4816,7 +4642,7 @@ ikev2_sa_responder(struct iked *env, str
        }
 
        /* XXX we need a better way to get this */
-       if (ikev2_sa_negotiate(&sa->sa_proposals,
+       if (proposals_negotiate(&sa->sa_proposals,
            &msg->msg_policy->pol_proposals, &msg->msg_proposals, 0) != 0) {
                log_info("%s: no proposal chosen", __func__);
                msg->msg_error = IKEV2_N_NO_PROPOSAL_CHOSEN;
Index: policy.c
===================================================================
RCS file: /mount/openbsd/cvs/src/sbin/iked/policy.c,v
retrieving revision 1.54
diff -u -p -r1.54 policy.c
--- policy.c    7 Jan 2020 15:08:28 -0000       1.54
+++ policy.c    26 Feb 2020 22:25:59 -0000
@@ -46,6 +46,8 @@ static __inline int
 static __inline int
         ts_insert_unique(struct iked_addr *, struct iked_tss *, int);
 
+static int     proposals_match(struct iked_proposal *, struct iked_proposal *,
+                   struct iked_transform **, int);
 
 void
 policy_init(struct iked *env)
@@ -58,6 +60,14 @@ policy_init(struct iked *env)
        RB_INIT(&env->sc_activeflows);
 }
 
+/*
+ * Lookup an iked policy matching the IKE_AUTH message msg
+ * and store a pointer to the found policy in msg.  If no policy
+ * matches a pointer to the default policy is stored in msg.
+ *
+ * Returns 0 on success and -1 if no matching policy was
+ * found and no default exists.
+ */
 int
 policy_lookup(struct iked *env, struct iked_message *msg)
 {
@@ -78,6 +88,7 @@ policy_lookup(struct iked *env, struct i
        if (msg->msg_id.id_type &&
            ikev2_print_id(&msg->msg_id, idstr, IKED_ID_SIZE) == 0 &&
            (s = strchr(idstr, '/')) != NULL) {
+               pol.pol_proposals = msg->msg_sa->sa_proposals;
                pol.pol_peerid.id_type = msg->msg_id.id_type;
                pol.pol_peerid.id_length = strlen(s+1);
                strlcpy(pol.pol_peerid.id_data, s+1,
@@ -100,6 +111,13 @@ policy_lookup(struct iked *env, struct i
        return (0);
 }
 
+/*
+ * Find a policy matching the query policy key in the global env.
+ * If multiple matching policies are found the policy with the highest
+ * priority is selected.
+ *
+ * Returns a pointer to a matching policy, or NULL if no policy matches.
+ */
 struct iked_policy *
 policy_test(struct iked *env, struct iked_policy *key)
 {
@@ -150,6 +168,14 @@ policy_test(struct iked *env, struct ike
                                continue;
                        }
 
+                       /* Make sure the proposals are compatible */
+                       if (TAILQ_FIRST(&key->pol_proposals) &&
+                           proposals_negotiate(NULL, &key->pol_proposals,
+                           &p->pol_proposals, 0) == -1) {
+                               p = TAILQ_NEXT(p, pol_entry);
+                               continue;
+                       }
+
                        /* Policy matched */
                        pol = p;
 
@@ -639,6 +665,183 @@ static __inline int
 user_cmp(struct iked_user *a, struct iked_user *b)
 {
        return (strcmp(a->usr_name, b->usr_name));
+}
+
+/*
+ * Find a matching subset of the proposal lists 'local' and 'peer'.
+ * The resulting proposal is stored in 'result' if 'result' is not NULL.
+ * The 'rekey' parameter indicates a CREATE_CHILD_SA exchange where
+ * an extra group is necessary for PFS. For the initial IKE_AUTH exchange
+ * the ESP SA proposal never includes an explicit DH group.
+ * 
+ * Return 0 if a matching subset was found and -1 if no subset was found
+ * or an error occured.
+ */
+int
+proposals_negotiate(struct iked_proposals *result, struct iked_proposals 
*local,
+    struct iked_proposals *peer, int rekey)
+{
+       struct iked_proposal    *ppeer = NULL, *plocal, *prop, vpeer, vlocal;
+       struct iked_transform    chosen[IKEV2_XFORMTYPE_MAX];
+       struct iked_transform   *valid[IKEV2_XFORMTYPE_MAX];
+       struct iked_transform   *match[IKEV2_XFORMTYPE_MAX];
+       unsigned int             i, score, chosen_score = 0;
+       uint8_t                  protoid = 0;
+
+       bzero(valid, sizeof(valid));
+       bzero(&vlocal, sizeof(vlocal));
+       bzero(&vpeer, sizeof(vpeer));
+
+       if (TAILQ_EMPTY(peer)) {
+               log_debug("%s: peer did not send %s proposals", __func__,
+                   print_map(protoid, ikev2_saproto_map));
+               return (-1);
+       }
+
+       TAILQ_FOREACH(plocal, local, prop_entry) {
+               TAILQ_FOREACH(ppeer, peer, prop_entry) {
+                       if (ppeer->prop_protoid != plocal->prop_protoid)
+                               continue;
+                       bzero(match, sizeof(match));
+                       score = proposals_match(plocal, ppeer, match,
+                           rekey);
+                       log_debug("%s: score %d", __func__, score);
+                       if (score && (!chosen_score || score < chosen_score)) {
+                               chosen_score = score;
+                               for (i = 0; i < IKEV2_XFORMTYPE_MAX; i++) {
+                                       if ((valid[i] = match[i]))
+                                               memcpy(&chosen[i], match[i],
+                                                   sizeof(chosen[0]));
+                               }
+                               memcpy(&vpeer, ppeer, sizeof(vpeer));
+                               memcpy(&vlocal, plocal, sizeof(vlocal));
+                       }
+               }
+               if (chosen_score != 0)
+                       break;
+       }
+
+       if (chosen_score == 0)
+               return (-1);
+       else if (result == NULL)
+               return (0);
+
+       (void)config_free_proposals(result, vpeer.prop_protoid);
+       prop = config_add_proposal(result, vpeer.prop_id, vpeer.prop_protoid);
+
+       if (vpeer.prop_localspi.spi_size) {
+               prop->prop_localspi.spi_size = vpeer.prop_localspi.spi_size;
+               prop->prop_peerspi = vpeer.prop_peerspi;
+       }
+       if (vlocal.prop_localspi.spi_size) {
+               prop->prop_localspi.spi_size = vlocal.prop_localspi.spi_size;
+               prop->prop_localspi.spi = vlocal.prop_localspi.spi;
+       }
+
+       for (i = 0; i < IKEV2_XFORMTYPE_MAX; i++) {
+               if (valid[i] == NULL)
+                       continue;
+               print_debug("%s: score %d: %s %s", __func__,
+                   chosen[i].xform_score, print_map(i, ikev2_xformtype_map),
+                   print_map(chosen[i].xform_id, chosen[i].xform_map));
+               if (chosen[i].xform_length)
+                       print_debug(" %d", chosen[i].xform_length);
+               print_debug("\n");
+
+               if (config_add_transform(prop, chosen[i].xform_type,
+                   chosen[i].xform_id, chosen[i].xform_length,
+                   chosen[i].xform_keylength) == NULL)
+                       break;
+       }
+
+       return (0);
+}
+
+static int
+proposals_match(struct iked_proposal *local, struct iked_proposal *peer,
+    struct iked_transform **xforms, int rekey)
+{
+       struct iked_transform   *tpeer, *tlocal;
+       unsigned int             i, j, type, score, requiredh = 0;
+       uint8_t                  protoid = peer->prop_protoid;
+       uint8_t                  peerxfs[IKEV2_XFORMTYPE_MAX];
+
+       bzero(peerxfs, sizeof(peerxfs));
+
+       for (i = 0; i < peer->prop_nxforms; i++) {
+               tpeer = peer->prop_xforms + i;
+               if (tpeer->xform_type > IKEV2_XFORMTYPE_MAX)
+                       continue;
+
+               /*
+                * Record all transform types from the peer's proposal,
+                * because if we want this proposal we have to select
+                * a transform for each proposed transform type.
+                */
+               peerxfs[tpeer->xform_type] = 1;
+
+               for (j = 0; j < local->prop_nxforms; j++) {
+                       tlocal = local->prop_xforms + j;
+
+                       /*
+                        * We require a DH group for ESP if there is any
+                        * local proposal with DH enabled.
+                        */
+                       if (rekey && requiredh == 0 &&
+                           protoid == IKEV2_SAPROTO_ESP &&
+                           tlocal->xform_type == IKEV2_XFORMTYPE_DH)
+                               requiredh = 1;
+
+                       /* Compare peer and local proposals */
+                       if (tpeer->xform_type != tlocal->xform_type ||
+                           tpeer->xform_id != tlocal->xform_id ||
+                           tpeer->xform_length != tlocal->xform_length)
+                               continue;
+                       type = tpeer->xform_type;
+
+                       if (xforms[type] == NULL || tlocal->xform_score <
+                           xforms[type]->xform_score) {
+                               xforms[type] = tlocal;
+                       } else
+                               continue;
+
+                       print_debug("%s: xform %d <-> %d (%d): %s %s "
+                           "(keylength %d <-> %d)", __func__,
+                           peer->prop_id, local->prop_id, tlocal->xform_score,
+                           print_map(type, ikev2_xformtype_map),
+                           print_map(tpeer->xform_id, tpeer->xform_map),
+                           tpeer->xform_keylength, tlocal->xform_keylength);
+                       if (tpeer->xform_length)
+                               print_debug(" %d", tpeer->xform_length);
+                       print_debug("\n");
+               }
+       }
+
+       for (i = score = 0; i < IKEV2_XFORMTYPE_MAX; i++) {
+               if (protoid == IKEV2_SAPROTO_IKE && xforms[i] == NULL &&
+                   (i == IKEV2_XFORMTYPE_ENCR || i == IKEV2_XFORMTYPE_PRF ||
+                    i == IKEV2_XFORMTYPE_INTEGR || i == IKEV2_XFORMTYPE_DH)) {
+                       score = 0;
+                       break;
+               } else if (protoid == IKEV2_SAPROTO_AH && xforms[i] == NULL &&
+                   (i == IKEV2_XFORMTYPE_INTEGR || i == IKEV2_XFORMTYPE_ESN)) {
+                       score = 0;
+                       break;
+               } else if (protoid == IKEV2_SAPROTO_ESP && xforms[i] == NULL &&
+                   (i == IKEV2_XFORMTYPE_ENCR || i == IKEV2_XFORMTYPE_ESN ||
+                   (requiredh && i == IKEV2_XFORMTYPE_DH))) {
+                       score = 0;
+                       break;
+               } else if (peerxfs[i] && xforms[i] == NULL) {
+                       score = 0;
+                       break;
+               } else if (xforms[i] == NULL)
+                       continue;
+
+               score += xforms[i]->xform_score;
+       }
+
+       return (score);
 }
 
 static __inline int

Reply via email to