On 17-06-25 21:44:24, Tim Stewart wrote:
> Hi,
> 
> In this message I've tried to encode everything I've done to allow
> strongSwan on Android to connect with iked, including the latest patch.
> I have also verified that it breaks neither initial negotiation nor
> Child SA rekeying for OpenBSD, Windows, and strongSwan (on Android)
> clients.

 This patch gets my android phone much closer to being able to negotiate
 a connection, but there are still issues. Paraphrasing analysis mikeb
 performed on IRC:
 android sends incorrect (for us) group, and with this patch we now send
 a failure message and android retries. But, we don't increment msgid
 "because we did sa_free and restarted, so we can assume that android
 thinks that negotiation continues, that's why it re-sends the
 IKE_SA_INIT"
 
> Stuart Henderson <s...@spacehopper.org> writes:
> 
> > On 2017/05/22 01:52, Tim Stewart wrote:
> >> Hello again,
> >>
> >> Tim Stewart <t...@stoo.org> writes:
> >>
> >> > Tim Stewart <t...@stoo.org> writes:
> >> >
> >> >> This patch teaches iked to reject a KE with a Notify payload of type
> >> >> INVALID_KE_PAYLOAD when the KE uses a different Diffie-Hellman group
> >> >> than is configured locally.  The rejection indicates the desired
> >> >> group.
> >> >>
> >> >> In my environment, this patch allows stock strongSwan on Android from
> >> >> the Google Play store to interop with iked.  strongSwan's logs show
> >> >> the following once iked is patched:
> >> >>
> >> >>   [IKE] initiating IKE_SA android[7] to 192.0.2.1
> >> >>   [ENC] generating IKE_SA_INIT request 0 [ SA KE No N(NATD_S_IP) 
> >> >> N(NATD_D_IP) N(FRAG_SUP) N(HASH_ALG) N(REDIR_SUP) ]
> >> >>   [ENC] parsed IKE_SA_INIT response 0 [ N(INVAL_KE) ]
> >> >>   [IKE] peer didn't accept DH group ECP_256, it requested MODP_2048
> >> >>   [IKE] initiating IKE_SA android[7] to 192.0.2.1
> >> >>   [ENC] generating IKE_SA_INIT request 0 [ SA KE No N(NATD_S_IP) 
> >> >> N(NATD_D_IP) N(FRAG_SUP) N(HASH_ALG) N(REDIR_SUP) ]
> >> >>   [ENC] parsed IKE_SA_INIT response 0 [ SA KE No N(NATD_S_IP) 
> >> >> N(NATD_D_IP) CERTREQ N(HASH_ALG) ]
> >> >>
> >> >> I'm happy to iterate on this patch to get it into proper shape for
> >> >> inclusion.
> >> >
> >> > I discovered a bug in the previous patch that broke renegotiation of
> >> > CHILD SAs.  I was ignoring "other than NONE" in the following sentence
> >> > from RFC 5996 section 3.4:
> >> >
> >> >   If the selected proposal uses a different Diffie-Hellman group
> >> >   (other than NONE), the message MUST be rejected with a Notify
> >> >   payload of type INVALID_KE_PAYLOAD.
> >> >
> >> > The new patch below repairs the flaw.
> >>
> >> After re-reading relevant parts of the RFC I'm not convinced that my fix
> >> (rejecting with INVALID_KE_PAYLOAD unless msg->msg_dhgroup is
> >> IKEV2_XFORMDH_NONE) is correct.  It happens to resolve my local issue
> >> but I think it may accidentally work due to a side effect of the code
> >> path for rekeying a child SA.
> >>
> >> I will look at it more closely this week.
> 
> My first patch did, in fact, break Child SAs rekeying.  I have a new
> patch at the end of this message that simply restricts DH group
> negotiation to IKE SAs (I *think* that DH group guessing only applies to
> IKE SAs, and perhaps only the IKE_SA_INIT exchange, but I'm still
> working through the RFC).  This may not be the ultimate solution, but it
> does allow us to move forward.
> 
> > Hi, I'm interested in this, but wasn't able to get strongswan to connect
> > with either of your patches (and had iked exiting on one attempt, though
> > I haven't been able to repeat that).
> 
> After making changes I usually rebuild all of /usr/src/sbin/iked/ (make
> clean && make).  I started doing this after experiencing similar exits
> and the problem went away.  That could just be a coincidence, though!
> 
> > If you have any updates please do send them here, it can be a bit slow
> > getting feedback on iked diffs at times but it definitely is worth sending
> > them out.
> 
> I'll summarize what I know about strongSwan (on Android) and iked
> interop.  strongSwan chooses ECP_256 for its DH group guess when it
> starts the IKE_SA_INIT exchange.  The negotiation gets pretty far if I
> specify `group ecp256' in the ikesa directive, but there there is some
> incompatibility between iked and strongSwan that causes the following:
> 
> ...
> ikev2_recv: IKE_AUTH request from initiator 5.6.7.8:49317 to 1.2.3.4:4500 
> policy 'default' id 1, 2040 bytes
> ikev2_recv: ispi 0x4a0221ee629489c0 rspi 0x2459b2780209a1c8
> ikev2_recv: updated SA to peer 5.6.7.8:49317 local 1.2.3.4:4500
> ikev2_pld_parse: header ispi 0x4a0221ee629489c0 rspi 0x2459b2780209a1c8 
> nextpayload SK version 0x20 exchange IKE_AUTH flags 0x08 msgid 1 length 2040 
> response 0
> ikev2_pld_payloads: payload SK nextpayload IDi critical 0x00 length 2012
> ikev2_msg_decrypt: IV length 16
> ikev2_msg_decrypt: encrypted payload length 1968
> ikev2_msg_decrypt: integrity checksum length 24
> ikev2_msg_decrypt: integrity check failed
> ikev2_resp_recv: failed to parse message
> ...
> 
> I suspect this is the failure many people hit if they try to configure
> iked to match strongSwan on Android's built-in settings.  I don't know
> what the issue is with ecp256, but I needed to use modp2048 anyway so I
> decided to write this patch.
> 
> I also configured iked to match strongSwan's built-in configuration for
> Child SA rekeying.  Many other cipher combinations result in
> NO_PROPOSAL_CHOSEN when iked initiates the Child SA rekey and strongSwan
> (usually) decides to rebuild the whole SA, which is awkward and slow
> (and not always successful).  Here is the configuration I've settled on:
> 
> ikev2 "strongswan" passive esp \
>     from 0.0.0.0/0 to 10.1.1.50 \
>     local em2 peer any \
>     ikesa auth hmac-sha2-384 enc aes-256 prf hmac-sha2-384 group modp2048 \
>     childsa enc chacha20-poly1305 group modp3072 \
>     srcid "/C=US/ST=New York/L=New York City/O=Stoo 
> Labs/OU=iked/CN=foo.example.com/emailAddress=ad...@example.com" \
>     dstid "bar.stoo.org" \
>     rsa \
>     config address 10.1.1.50 \
>     config name-server 10.1.1.8 \
>     config name-server 10.1.1.9 \
>     tag "$name-$id"
> 
> Some notes:
> - em2 is my Internet-facing IPv4 interface.
> - In my case, strongSwan presents the dstid as a FQDN, not an ASN1_DN.
>   I'm using certificates generated using `ikectl ca'.
> - `childsa enc chacha20-poly1305 group modp3072' is accpeted by
>   strongSwan on Android for rekeying Child SAs.  It took me a few tries
>   to find a compatible set of parameters here.
> 
> 
> Finally, the latest patch against -current:
> 
> 
> Index: iked.h
> ===================================================================
> RCS file: /cvs/src/sbin/iked/iked.h,v
> retrieving revision 1.115
> diff -u -p -r1.115 iked.h
> --- iked.h    26 Apr 2017 10:42:38 -0000      1.115
> +++ iked.h    26 Jun 2017 01:30:22 -0000
> @@ -502,6 +502,7 @@ struct iked_message {
>       struct iked_proposals    msg_proposals;
>       struct iked_spi          msg_rekey;
>       struct ibuf             *msg_nonce;     /* dh NONCE */
> +     uint16_t                 msg_dhgroup;   /* dh group */
>       struct ibuf             *msg_ke;        /* dh key exchange */
>       struct iked_id           msg_auth;      /* AUTH payload */
>       struct iked_id           msg_id;
> Index: ikev2.c
> ===================================================================
> RCS file: /cvs/src/sbin/iked/ikev2.c,v
> retrieving revision 1.155
> diff -u -p -r1.155 ikev2.c
> --- ikev2.c   1 Jun 2017 15:23:43 -0000       1.155
> +++ ikev2.c   26 Jun 2017 01:30:22 -0000
> @@ -71,6 +71,8 @@ int  ikev2_init_done(struct iked *, stru
>  void  ikev2_resp_recv(struct iked *, struct iked_message *,
>           struct ike_header *);
>  int   ikev2_resp_ike_sa_init(struct iked *, struct iked_message *);
> +int   ikev2_resp_ike_invalid_ke(struct iked *, struct iked_message *,
> +         struct iked_kex *);
>  int   ikev2_resp_ike_auth(struct iked *, struct iked_sa *);
>  int   ikev2_resp_ike_eap(struct iked *, struct iked_sa *, struct ibuf *);
>  int   ikev2_send_auth_failed(struct iked *, struct iked_sa *);
> @@ -96,8 +98,8 @@ int  ikev2_sa_responder(struct iked *, s
>           struct iked_message *);
>  int   ikev2_sa_initiator_dh(struct iked_sa *, struct iked_message *,
>           unsigned int);
> -int   ikev2_sa_responder_dh(struct iked_kex *, struct iked_proposals *,
> -         struct iked_message *, unsigned int);
> +int   ikev2_sa_responder_dh(struct iked *, struct iked_kex *,
> +         struct iked_proposals *, struct iked_message *, unsigned int, int);
>  void  ikev2_sa_cleanup_dh(struct iked_sa *);
>  int   ikev2_sa_keys(struct iked *, struct iked_sa *, struct ibuf *);
>  int   ikev2_sa_tag(struct iked_sa *, struct iked_id *);
> @@ -2279,6 +2281,84 @@ ikev2_resp_ike_sa_init(struct iked *env,
>  }
> 
>  int
> +ikev2_resp_ike_invalid_ke(struct iked *env, struct iked_message *msg,
> +    struct iked_kex *kex)
> +{
> +     struct iked_message              resp;
> +     struct ike_header               *hdr;
> +     struct ikev2_payload            *pld;
> +     struct ikev2_notify             *n;
> +     struct iked_sa                  *sa = msg->msg_sa;
> +     struct ibuf                     *buf;
> +     uint8_t                         *ptr;
> +     ssize_t                          len;
> +     uint16_t                         group;
> +     int                              ret = -1;
> +
> +     if (sa->sa_hdr.sh_initiator) {
> +             log_debug("%s: called by initiator", __func__);
> +             return (-1);
> +     }
> +
> +     log_debug("%s: rejecting with INVALID_KE_PAYLOAD", __func__);
> +
> +     if ((buf = ikev2_msg_init(env, &resp,
> +         &msg->msg_peer, msg->msg_peerlen,
> +         &msg->msg_local, msg->msg_locallen, 1)) == NULL)
> +             goto done;
> +
> +     resp.msg_sa = sa;
> +     resp.msg_fd = msg->msg_fd;
> +     resp.msg_natt = msg->msg_natt;
> +     resp.msg_msgid = 0;
> +
> +     /* IKE header */
> +     if ((hdr = ikev2_add_header(buf, sa, resp.msg_msgid,
> +         IKEV2_PAYLOAD_NOTIFY, IKEV2_EXCHANGE_IKE_SA_INIT,
> +         IKEV2_FLAG_RESPONSE)) == NULL)
> +             goto done;
> +
> +     /* NOTIFY payload */
> +     if ((pld = ikev2_add_payload(buf)) == NULL)
> +             goto done;
> +     if ((n = ibuf_advance(buf, sizeof(*n))) == NULL)
> +             goto done;
> +     n->n_protoid = 0;
> +     n->n_spisize = 0;
> +     n->n_type = htobe16(IKEV2_N_INVALID_KE_PAYLOAD);
> +
> +     /* add desired dh group to NOTIFY */
> +     group = htobe16(kex->kex_dhgroup->id);
> +     len = sizeof(group);
> +     if ((ptr = ibuf_advance(buf, len)) == NULL)
> +             goto done;
> +     memcpy(ptr, &group, sizeof(group));
> +     len += sizeof(*n);
> +
> +     if (ikev2_next_payload(pld, len, IKEV2_PAYLOAD_NONE) == -1)
> +             goto done;
> +
> +     if (ikev2_set_header(hdr, ibuf_size(buf) - sizeof(*hdr)) == -1)
> +             goto done;
> +
> +     (void)ikev2_pld_parse(env, hdr, &resp, 0);
> +
> +     ibuf_release(sa->sa_2ndmsg);
> +     if ((sa->sa_2ndmsg = ibuf_dup(buf)) == NULL) {
> +             log_debug("%s: failed to copy 2nd message", __func__);
> +             goto done;
> +     }
> +
> +     resp.msg_sa = NULL;     /* Don't save the response */
> +     ret = ikev2_msg_send(env, &resp);
> +
> +done:
> +     ikev2_msg_cleanup(env, &resp);
> +
> +     return (ret);
> +}
> +
> +int
>  ikev2_send_auth_failed(struct iked *env, struct iked_sa *sa)
>  {
>       struct ikev2_notify             *n;
> @@ -3397,8 +3477,8 @@ ikev2_resp_create_child_sa(struct iked *
>               /* check KE payload for PFS */
>               if (ibuf_length(msg->msg_parent->msg_ke)) {
>                       log_debug("%s: using PFS", __func__);
> -                     if (ikev2_sa_responder_dh(kex, &proposals,
> -                         msg->msg_parent, protoid) < 0) {
> +                     if (ikev2_sa_responder_dh(env, kex, &proposals,
> +                         msg->msg_parent, protoid, 1) < 0) {
>                               log_debug("%s: failed to setup DH", __func__);
>                               goto fail;
>                       }
> @@ -4102,8 +4182,9 @@ ikev2_sa_initiator(struct iked *env, str
>  }
> 
>  int
> -ikev2_sa_responder_dh(struct iked_kex *kex, struct iked_proposals *proposals,
> -    struct iked_message *msg, unsigned int proto)
> +ikev2_sa_responder_dh(struct iked *env, struct iked_kex *kex,
> +    struct iked_proposals *proposals, struct iked_message *msg,
> +    unsigned int proto, int child_sa)
>  {
>       struct iked_transform   *xform;
> 
> @@ -4121,6 +4202,18 @@ ikev2_sa_responder_dh(struct iked_kex *k
>               }
>       }
> 
> +     /* Look for dhgroup mismatch during an IKE SA negotiation */
> +     if (!child_sa && msg->msg_dhgroup != kex->kex_dhgroup->id) {
> +             log_debug("%s: want dh %s, KE has %s", __func__,
> +                 print_map(kex->kex_dhgroup->id, ikev2_xformdh_map),
> +                 print_map(msg->msg_dhgroup, ikev2_xformdh_map));
> +             if (ikev2_resp_ike_invalid_ke(env, msg,
> +                 kex) != 0)
> +                     log_debug("%s: failed to send init response",
> +                         __func__);
> +             return (-1);
> +     }
> +
>       if (!ibuf_length(kex->kex_dhrexchange)) {
>               if ((kex->kex_dhrexchange = ibuf_new(NULL,
>                   dh_getlen(kex->kex_dhgroup))) == NULL) {
> @@ -4226,7 +4319,8 @@ ikev2_sa_responder(struct iked *env, str
>               }
>       }
> 
> -     if (ikev2_sa_responder_dh(&sa->sa_kex, &sa->sa_proposals, msg, 0) < 0)
> +     if (ikev2_sa_responder_dh(env, &sa->sa_kex, &sa->sa_proposals,
> +         msg, 0, 0) < 0)
>               return (-1);
> 
>       return (ikev2_sa_keys(env, sa, osa ? osa->sa_key_d : NULL));
> Index: ikev2_pld.c
> ===================================================================
> RCS file: /cvs/src/sbin/iked/ikev2_pld.c,v
> retrieving revision 1.62
> diff -u -p -r1.62 ikev2_pld.c
> --- ikev2_pld.c       13 Apr 2017 07:04:09 -0000      1.62
> +++ ikev2_pld.c       26 Jun 2017 01:30:22 -0000
> @@ -680,6 +680,7 @@ ikev2_pld_ke(struct iked *env, struct ik
>       log_debug("%s: dh group %s reserved %d", __func__,
>           print_map(betoh16(kex.kex_dhgroup), ikev2_xformdh_map),
>           betoh16(kex.kex_reserved));
> +     msg->msg_dhgroup = betoh16(kex.kex_dhgroup);
> 
>       buf = msgbuf + offset + sizeof(kex);
>       len = betoh16(pld->pld_length) - sizeof(*pld) - sizeof(kex);
> 

Reply via email to