Re: [PATCH] iked: Bugfixes for IKE rekeying

[Thomas Klute]

Am 09.11.2016 um 20:36 schrieb Vincent Gross:
> On Wed, 9 Nov 2016 13:16:46 +
> Thomas Klute  wrote:
> 
>> Hi tech@,
>>
>> this patch contains fixes for two bugs that break IKE rekeying
>> initiated by iked. Please review, and apply or let me know what has to
>> be changed! Both bugs are fixed by initializing the respective
>> structures of the new IKE SA (struct iked_sa *nsa in the
>> ikev2_ike_sa_rekey function):
> 
> Thanks, we are looking into it.

Hi, is there any progress on this?

>> For [1]: Copying the address information is required to send any
>> request messages over the new IKE SA after rekeying, otherwise errors
>> like the following happen because the IP addresses and ports remain
>> initialized to zero:
>>
>> ikev2_msg_send: INFORMATIONAL request from any to any msgid 1, 80
>> bytes ikev2_msg_send: sendtofrom: Invalid argument
>>
>> For [2]: Setting the DH group based on the currently used one is
>> necessary because iked proposes only the currently used transforms
>> during IKE rekeying, so trying to use any other group for the DH
>> exchange will fail even if it is preferred by local policy (see
>> comment in the patch for details).
>>
>> This patch includes and supersedes the one for only the first bug I
>> sent yesterday.
>>
>> Best regards,
>> Thomas
>>
>> [1] https://marc.info/?l=openbsd-bugs=147739504516767=2
>> [2] https://marc.info/?l=openbsd-bugs=147747405806461=2
>>
>> Index: src/sbin/iked/ikev2.c
>> ===
>> RCS file: /cvs/src/sbin/iked/ikev2.c,v
>> retrieving revision 1.131
>> diff -u -p -u -r1.131 ikev2.c
>> --- src/sbin/iked/ikev2.c2 Jun 2016 07:14:26 -
>> 1.131 +++ src/sbin/iked/ikev2.c  9 Nov 2016 13:12:32 -
>> @@ -2658,6 +2658,18 @@ ikev2_ike_sa_rekey(struct iked *env, voi
>>  goto done;
>>  }
>>  
>> +/* Select the DH group ID based on the currently used
>> + * one. Otherwise the call to ikev2_sa_initiator below would
>> + * set it to the first DH transform in the policy, while the
>> + * SA payload contains only one proposal matching the
>> + * currently used transforms. If a different DH transform has
>> + * been negotiated this means KE payload and negotiated DH
>> + * transform cannot match, causing rekeying to fail. */
>> +if ((nsa->sa_dhgroup = group_get(sa->sa_dhgroup->id)) ==
>> NULL) {
>> +log_debug("%s: failed to initialize DH group",
>> __func__);
>> +goto done;
>> +}
>> +
>>  if (ikev2_sa_initiator(env, nsa, sa, NULL)) {
>>  log_debug("%s: failed to setup DH", __func__);
>>  goto done;
>> @@ -2665,6 +2677,13 @@ ikev2_ike_sa_rekey(struct iked *env, voi
>>  sa_state(env, nsa, IKEV2_STATE_AUTH_SUCCESS);
>>  nonce = nsa->sa_inonce;
>>  
>> +/* Copy local and peer address from the old SA */
>> +if (sa_address(nsa, >sa_peer, >sa_peer.addr) == -1
>> ||
>> +sa_address(nsa, >sa_local, >sa_local.addr) ==
>> -1) {
>> +log_debug("%s: failed copy address data", __func__);
>> +goto done;
>> +}
>> +
>>  if ((e = ibuf_static()) == NULL)
>>  goto done;
>>  
>>
> 

Re: [PATCH] iked: Bugfixes for IKE rekeying

[Vincent Gross]

On Wed, 9 Nov 2016 13:16:46 +
Thomas Klute  wrote:

> Hi tech@,
> 
> this patch contains fixes for two bugs that break IKE rekeying
> initiated by iked. Please review, and apply or let me know what has to
> be changed! Both bugs are fixed by initializing the respective
> structures of the new IKE SA (struct iked_sa *nsa in the
> ikev2_ike_sa_rekey function):

Thanks, we are looking into it.

> 
> For [1]: Copying the address information is required to send any
> request messages over the new IKE SA after rekeying, otherwise errors
> like the following happen because the IP addresses and ports remain
> initialized to zero:
> 
> ikev2_msg_send: INFORMATIONAL request from any to any msgid 1, 80
> bytes ikev2_msg_send: sendtofrom: Invalid argument
> 
> For [2]: Setting the DH group based on the currently used one is
> necessary because iked proposes only the currently used transforms
> during IKE rekeying, so trying to use any other group for the DH
> exchange will fail even if it is preferred by local policy (see
> comment in the patch for details).
> 
> This patch includes and supersedes the one for only the first bug I
> sent yesterday.
> 
> Best regards,
> Thomas
> 
> [1] https://marc.info/?l=openbsd-bugs=147739504516767=2
> [2] https://marc.info/?l=openbsd-bugs=147747405806461=2
> 
> Index: src/sbin/iked/ikev2.c
> ===
> RCS file: /cvs/src/sbin/iked/ikev2.c,v
> retrieving revision 1.131
> diff -u -p -u -r1.131 ikev2.c
> --- src/sbin/iked/ikev2.c 2 Jun 2016 07:14:26 -
> 1.131 +++ src/sbin/iked/ikev2.c   9 Nov 2016 13:12:32 -
> @@ -2658,6 +2658,18 @@ ikev2_ike_sa_rekey(struct iked *env, voi
>   goto done;
>   }
>  
> + /* Select the DH group ID based on the currently used
> +  * one. Otherwise the call to ikev2_sa_initiator below would
> +  * set it to the first DH transform in the policy, while the
> +  * SA payload contains only one proposal matching the
> +  * currently used transforms. If a different DH transform has
> +  * been negotiated this means KE payload and negotiated DH
> +  * transform cannot match, causing rekeying to fail. */
> + if ((nsa->sa_dhgroup = group_get(sa->sa_dhgroup->id)) ==
> NULL) {
> + log_debug("%s: failed to initialize DH group",
> __func__);
> + goto done;
> + }
> +
>   if (ikev2_sa_initiator(env, nsa, sa, NULL)) {
>   log_debug("%s: failed to setup DH", __func__);
>   goto done;
> @@ -2665,6 +2677,13 @@ ikev2_ike_sa_rekey(struct iked *env, voi
>   sa_state(env, nsa, IKEV2_STATE_AUTH_SUCCESS);
>   nonce = nsa->sa_inonce;
>  
> + /* Copy local and peer address from the old SA */
> + if (sa_address(nsa, >sa_peer, >sa_peer.addr) == -1
> ||
> + sa_address(nsa, >sa_local, >sa_local.addr) ==
> -1) {
> + log_debug("%s: failed copy address data", __func__);
> + goto done;
> + }
> +
>   if ((e = ibuf_static()) == NULL)
>   goto done;
>  
> 

[PATCH] iked: Bugfixes for IKE rekeying

[Thomas Klute]

Hi tech@,

this patch contains fixes for two bugs that break IKE rekeying
initiated by iked. Please review, and apply or let me know what has to
be changed! Both bugs are fixed by initializing the respective
structures of the new IKE SA (struct iked_sa *nsa in the
ikev2_ike_sa_rekey function):

For [1]: Copying the address information is required to send any
request messages over the new IKE SA after rekeying, otherwise errors
like the following happen because the IP addresses and ports remain
initialized to zero:

ikev2_msg_send: INFORMATIONAL request from any to any msgid 1, 80 bytes
ikev2_msg_send: sendtofrom: Invalid argument

For [2]: Setting the DH group based on the currently used one is
necessary because iked proposes only the currently used transforms
during IKE rekeying, so trying to use any other group for the DH
exchange will fail even if it is preferred by local policy (see
comment in the patch for details).

This patch includes and supersedes the one for only the first bug I
sent yesterday.

Best regards,
Thomas

[1] https://marc.info/?l=openbsd-bugs=147739504516767=2
[2] https://marc.info/?l=openbsd-bugs=147747405806461=2

Index: src/sbin/iked/ikev2.c
===
RCS file: /cvs/src/sbin/iked/ikev2.c,v
retrieving revision 1.131
diff -u -p -u -r1.131 ikev2.c
--- src/sbin/iked/ikev2.c   2 Jun 2016 07:14:26 -   1.131
+++ src/sbin/iked/ikev2.c   9 Nov 2016 13:12:32 -
@@ -2658,6 +2658,18 @@ ikev2_ike_sa_rekey(struct iked *env, voi
goto done;
}
 
+   /* Select the DH group ID based on the currently used
+* one. Otherwise the call to ikev2_sa_initiator below would
+* set it to the first DH transform in the policy, while the
+* SA payload contains only one proposal matching the
+* currently used transforms. If a different DH transform has
+* been negotiated this means KE payload and negotiated DH
+* transform cannot match, causing rekeying to fail. */
+   if ((nsa->sa_dhgroup = group_get(sa->sa_dhgroup->id)) == NULL) {
+   log_debug("%s: failed to initialize DH group", __func__);
+   goto done;
+   }
+
if (ikev2_sa_initiator(env, nsa, sa, NULL)) {
log_debug("%s: failed to setup DH", __func__);
goto done;
@@ -2665,6 +2677,13 @@ ikev2_ike_sa_rekey(struct iked *env, voi
sa_state(env, nsa, IKEV2_STATE_AUTH_SUCCESS);
nonce = nsa->sa_inonce;
 
+   /* Copy local and peer address from the old SA */
+   if (sa_address(nsa, >sa_peer, >sa_peer.addr) == -1 ||
+   sa_address(nsa, >sa_local, >sa_local.addr) == -1) {
+   log_debug("%s: failed copy address data", __func__);
+   goto done;
+   }
+
if ((e = ibuf_static()) == NULL)
goto done;