[PATCH] iked: Incorrect definition of critical bit (IKEV2_CRITICAL_PAYLOAD)
Hi tech@,
I noticed that the definition of IKEV2_CRITICAL_PAYLOAD in ikev2.h is
incorrect. According to RFC 7296, Section 3.2 the critical bit is the
first/high bit of the second octet of the IKE payload header. An octet
with only its first bit set results in a hex value of 0x80, not 0x01.
IKEV2_CRITICAL_PAYLOAD is only used to create a log message in
ikev2_pld_payloads (ikev2_pld.c), so the impact of this bug is small,
but correctly logging whether a payload is critical seems useful.
Best regards,
Thomas
--- a/ikev2.h
+++ b/ikev2.h
@@ -78,7 +78,7 @@ struct ikev2_payload {
uint16_t pld_length;/* Payload length with header */
} __packed;
-#define IKEV2_CRITICAL_PAYLOAD 0x01/* First bit in the reserved field */
+#define IKEV2_CRITICAL_PAYLOAD 0x80/* First bit in the reserved field */
/* IKEv2 payload types */
#define IKEV2_PAYLOAD_NONE 0 /* No payload */
Re: [PATCH] iked: Bugfixes for IKE rekeying
Am 09.11.2016 um 20:36 schrieb Vincent Gross:
> On Wed, 9 Nov 2016 13:16:46 +
> Thomas Klute <thomas.kl...@achelos.de> 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;
>>
>>
>
[PATCH] iked: Bugfixes for IKE rekeying
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;
[PATCH] iked: Preserve address information during rekeying
Hi tech@,
a week ago I reported to bugs@ that iked "forgets" the local and peer addresses
associated with an IKE SA while rekeying it if iked has initiated the rekeying,
breaking any IKE requests iked tries to send after rekeying [1]. The patch
below fixes the bug by copying the addresses from the current IKE SA when
initiating the rekeying. Please review and apply if OK.
[1] https://marc.info/?l=openbsd-bugs=147739504516767=2
Index: ikev2.c
===
RCS file: /cvs/src/sbin/iked/ikev2.c,v
retrieving revision 1.131
diff -u -p -u -r1.131 ikev2.c
--- ikev2.c 2 Jun 2016 07:14:26 - 1.131
+++ ikev2.c 8 Nov 2016 14:06:12 -
@@ -2665,6 +2665,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;
