Re: Add Diffie-Hellman group negotiation to iked

2017-07-18 Thread Tim Stewart
viq <vic...@gmail.com> writes:

> 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"

I'm glad it seems to help, though it's too bad that the patch doesn't
work completely for you.

I haven't really considered msgids--I'll do some more reading to see
what I might be missing there.  I do know that resending an IKE_SA_INIT
message with a different DH group is correct, however, and this does
work on my phone.  For your reference, the first line of my strongSwan
log tells me that I'm using strongSwan 5.5.3 and Android 7.1.1.

I see that you forwarded the iked logs in a reply to this email.  Is
this the full log after a fresh iked startup with no existing SAs?
Also, would it be possible to forward an anonymized config and the
strongSwan logs so that I can compare to mine?  (I can also post my
logs, but I'll have to do it in the next day or two as I'm out of time
for today.)

Good luck!

-TimS

--
Tim Stewart
---
Mail:   t...@stoo.org
Matrix: @tim:stoo.org



Re: Add Diffie-Hellman group negotiation to iked

2017-07-25 Thread Tim Stewart
viq <vic...@gmail.com> writes:

> On 17-07-18 23:20:26, Tim Stewart wrote:
>> viq <vic...@gmail.com> writes:
>>
>> > 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"
>>
>> I'm glad it seems to help, though it's too bad that the patch doesn't
>> work completely for you.
>>
>> I haven't really considered msgids--I'll do some more reading to see
>> what I might be missing there.  I do know that resending an IKE_SA_INIT
>> message with a different DH group is correct, however, and this does
>> work on my phone.  For your reference, the first line of my strongSwan
>> log tells me that I'm using strongSwan 5.5.3 and Android 7.1.1.
>>
>> I see that you forwarded the iked logs in a reply to this email.  Is
>> this the full log after a fresh iked startup with no existing SAs?
>
> This is after a fresh startup, there exists an SA but for a separete
> site-to-site config I have in place. If completely fresh logs are
> needed I could comment that out.

Well, my thinking here was that incorrect policy matching could be
confusing the issue.  I often find it helpful to comment out other
policies to eliminate policy matching as a failure point during testing.

>> Also, would it be possible to forward an anonymized config and the
>> strongSwan logs so that I can compare to mine?  (I can also post my
>> logs, but I'll have to do it in the next day or two as I'm out of time
>> for today.)
>
> First, sorry for the delay with replying to this. Second, I'm not sure
> how to get to the logs, seeing as I'm using the built-in VPN client that
> came with Samsung S8.

Oh, for some reason I assumed you were using strongSwan!  My mistake.
Can you provide a link to some more info about the Samsung S8's built-in
client?

I haven't had any time since my last post, but I still plan to look into
the msgids.

-TimS

--
Tim Stewart
---
Mail:   t...@stoo.org
Matrix: @tim:stoo.org



Reenable ASN1_DN IDs with certificates in iked

2017-05-16 Thread Tim Stewart
A sample configuration:

ikev2 "win10host" passive esp \
  from 0.0.0.0/0 to 10.1.1.51 \
  local any peer any \
  ikesa auth hmac-sha2-384 enc aes-256 prf hmac-sha2-384 group modp2048 \
  childsa enc aes-256-gcm group modp2048 \
  srcid "/C=US/ST=New York/L=NYC/O=Stoo Labs/OU=iked/CN=foo.stoo.org" \
  dstid "/C=US/ST=New York/L=NYC/O=Stoo Labs/OU=iked/CN=bar.stoo.org" \
  rsa \
  config address 10.1.1.51 \
  config name-server 10.1.1.5 \
  config name-server 10.1.1.6 \
  tag "$name-$id"

The above configuration worked fine with iked in OpenBSD 6.0.  It broke
as of 6.1 with the following error:

set_policy_auth_method: ikeauth policy mismatch, rsa specified, but only 
rfc7427 possible
set_policy: failed to set policy auth method for
/etc/iked.conf: 17: create_ike failed
/etc/iked.conf: no valid configuration rules found

I use a CA certificate and signed host certificates generated using a
process like the EXAMPLES section in ikectl(8).  I'm a bit surprised
that I could not find anyone else who has seen this problem, so maybe
I'm doing something strange without realizing it.

The following patch restores the old functionality, though I include it
mainly for demonstration purposes.  I'm happy to improve it and
resubmit, depending on feedback.

-TimS


Index: parse.y
===
RCS file: /cvs/src/sbin/iked/parse.y,v
retrieving revision 1.65
diff -u -p -r1.65 parse.y
--- parse.y 24 Apr 2017 07:07:25 -  1.65
+++ parse.y 17 May 2017 04:58:34 -
@@ -1735,6 +1735,8 @@ set_policy_auth_method(const char *peeri
method = IKEV2_AUTH_NONE;
cert_type = IKEV2_CERT_NONE;

+   ikeauth = >pol_auth;
+
if (key != NULL) {
/* infer policy from key type */
if ((rsa = EVP_PKEY_get1_RSA(key)) != NULL) {
@@ -1767,14 +1769,16 @@ set_policy_auth_method(const char *peeri

if (method == IKEV2_AUTH_NONE || cert_type == IKEV2_CERT_NONE)
return (-1);
+   } else if (ikeauth->auth_method == IKEV2_AUTH_RSA_SIG) {
+   /* default to IKEV2_CERT_X509_CERT otherwise */
+   method = IKEV2_AUTH_RSA_SIG;
+   cert_type = IKEV2_CERT_X509_CERT;
} else {
/* default to IKEV2_CERT_X509_CERT otherwise */
method = IKEV2_AUTH_SIG;
cert_type = IKEV2_CERT_X509_CERT;
}

-   ikeauth = >pol_auth;
-
if (ikeauth->auth_method == IKEV2_AUTH_SHARED_KEY_MIC) {
if (key != NULL &&
method != IKEV2_AUTH_RSA_SIG)
@@ -1784,6 +1788,7 @@ set_policy_auth_method(const char *peeri

if (ikeauth->auth_method != IKEV2_AUTH_NONE &&
ikeauth->auth_method != IKEV2_AUTH_SIG_ANY &&
+   ikeauth->auth_method != IKEV2_AUTH_RSA_SIG &&
ikeauth->auth_method != method)
goto mismatch;



Add Diffie-Hellman group negotiation to iked

2017-05-16 Thread Tim Stewart
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.

-TimS


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 -  1.115
+++ iked.h  17 May 2017 05:21:24 -
@@ -502,6 +502,7 @@ struct iked_message {
struct iked_proposalsmsg_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.154
diff -u -p -r1.154 ikev2.c
--- ikev2.c 26 Apr 2017 10:42:38 -  1.154
+++ ikev2.c 17 May 2017 05:21:24 -
@@ -71,6 +71,8 @@ intikev2_init_done(struct iked *, stru
 voidikev2_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 @@ intikev2_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);
 voidikev2_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, ,
+   >msg_peer, msg->msg_peerlen,
+   >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 = 

Re: Reenable ASN1_DN IDs with certificates in iked

2017-05-16 Thread Tim Stewart
Here is a version of the previous patch that preserves tabs properly.
Apologies.

-TimS


Index: parse.y
===
RCS file: /cvs/src/sbin/iked/parse.y,v
retrieving revision 1.65
diff -u -p -r1.65 parse.y
--- parse.y 24 Apr 2017 07:07:25 -  1.65
+++ parse.y 17 May 2017 05:40:39 -
@@ -1735,6 +1735,8 @@ set_policy_auth_method(const char *peeri
method = IKEV2_AUTH_NONE;
cert_type = IKEV2_CERT_NONE;

+   ikeauth = >pol_auth;
+
if (key != NULL) {
/* infer policy from key type */
if ((rsa = EVP_PKEY_get1_RSA(key)) != NULL) {
@@ -1767,14 +1769,16 @@ set_policy_auth_method(const char *peeri

if (method == IKEV2_AUTH_NONE || cert_type == IKEV2_CERT_NONE)
return (-1);
+   } else if (ikeauth->auth_method == IKEV2_AUTH_RSA_SIG) {
+   /* default to IKEV2_CERT_X509_CERT otherwise */
+   method = IKEV2_AUTH_RSA_SIG;
+   cert_type = IKEV2_CERT_X509_CERT;
} else {
/* default to IKEV2_CERT_X509_CERT otherwise */
method = IKEV2_AUTH_SIG;
cert_type = IKEV2_CERT_X509_CERT;
}

-   ikeauth = >pol_auth;
-
if (ikeauth->auth_method == IKEV2_AUTH_SHARED_KEY_MIC) {
if (key != NULL &&
method != IKEV2_AUTH_RSA_SIG)
@@ -1784,6 +1788,7 @@ set_policy_auth_method(const char *peeri

if (ikeauth->auth_method != IKEV2_AUTH_NONE &&
ikeauth->auth_method != IKEV2_AUTH_SIG_ANY &&
+   ikeauth->auth_method != IKEV2_AUTH_RSA_SIG &&
ikeauth->auth_method != method)
goto mismatch;



Re: Add Diffie-Hellman group negotiation to iked

2017-06-25 Thread Tim Stewart
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.

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

Re: Add Diffie-Hellman group negotiation to iked

2017-05-21 Thread Tim Stewart
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.

-TimS

P.S.  Is there someone I could add to the To: or Cc: headers of these
iked-related messages?  Or should I simply be patient?

--
Tim Stewart
---
Mail:   t...@stoo.org
Matrix: @tim:stoo.org



Re: Add Diffie-Hellman group negotiation to iked

2017-05-21 Thread Tim Stewart

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.

-TimS


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 -  1.115
+++ iked.h  22 May 2017 05:29:17 -
@@ -502,6 +502,7 @@ struct iked_message {
struct iked_proposalsmsg_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.154
diff -u -p -r1.154 ikev2.c
--- ikev2.c 26 Apr 2017 10:42:38 -  1.154
+++ ikev2.c 22 May 2017 05:29:18 -
@@ -71,6 +71,8 @@ intikev2_init_done(struct iked *, stru
 voidikev2_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 @@ intikev2_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);
 voidikev2_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, ,
+   >msg_peer, msg->msg_peerlen,
+   >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

Re: Reenable ASN1_DN IDs with certificates in iked

2017-06-04 Thread Tim Stewart
Tim Stewart <t...@stoo.org> writes:

> A sample configuration:
>
> ikev2 "win10host" passive esp \
>   from 0.0.0.0/0 to 10.1.1.51 \
>   local any peer any \
>   ikesa auth hmac-sha2-384 enc aes-256 prf hmac-sha2-384 group modp2048 \
>   childsa enc aes-256-gcm group modp2048 \
>   srcid "/C=US/ST=New York/L=NYC/O=Stoo Labs/OU=iked/CN=foo.stoo.org" \
>   dstid "/C=US/ST=New York/L=NYC/O=Stoo Labs/OU=iked/CN=bar.stoo.org" \
>   rsa \
>   config address 10.1.1.51 \
>   config name-server 10.1.1.5 \
>   config name-server 10.1.1.6 \
>   tag "$name-$id"
>
> The above configuration worked fine with iked in OpenBSD 6.0.  It broke
> as of 6.1 with the following error:
>
> set_policy_auth_method: ikeauth policy mismatch, rsa specified, but only 
> rfc7427 possible
> set_policy: failed to set policy auth method for
> /etc/iked.conf: 17: create_ike failed
> /etc/iked.conf: no valid configuration rules found
>
> I use a CA certificate and signed host certificates generated using a
> process like the EXAMPLES section in ikectl(8).  I'm a bit surprised
> that I could not find anyone else who has seen this problem, so maybe
> I'm doing something strange without realizing it.

Is there any more information that I can provide about this issue, or
possibly suggestions for changes to the patch?  Some guidance would help
me as I am new to this code.

Perhaps this should be moved to bugs?  (I posted to tech because I had a
patch).

Thanks,

-TimS

--
Tim Stewart
---
Mail:   t...@stoo.org
Matrix: @tim:stoo.org



Re: ipsec: tdb_unlink() and dst addr update for MOBIKE

2017-10-15 Thread Tim Stewart
Hello Patrick,

Patrick Wildt <patr...@blueri.se> writes:

> Hi,
>
> I'd like to add MOBIKE support to iked, specifically first of all making
> iked as server react to mobile clients changing their IP addresses. One
> thing for that is the kernel part.

I'm excited about this!  I'm happy help test this support as it would
benefit my use case.  I have a pretty good test environment as I ride
the subway every day, and I already have StrongSwan on Android
configured connect to iked (which requires a DH group negotiation patch
that I've posted elsewhere on this list).

Is there anything I can do to help?  Meanwhile, I'll be watching this
space for more patches.

-TimS

--
Tim Stewart
---
Mail:   t...@stoo.org
Matrix: @tim:stoo.org



Re: Add Diffie-Hellman group negotiation to iked

2017-12-11 Thread Tim Stewart
Apologies for disappearing for a while.  I was moving across town and I
had to drop many things!

Stuart Henderson <s...@spacehopper.org> writes:

> On 2017/06/25 21:44, 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.
>>
>> 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 

Re: Add Diffie-Hellman group negotiation to iked

2017-12-11 Thread Tim Stewart

Patrick Wildt <patr...@blueri.se> writes:

> On Mon, Nov 27, 2017 at 06:12:22PM +0100, Patrick Wildt wrote:
>> On Mon, Nov 27, 2017 at 04:21:08PM +0100, Patrick Wildt wrote:
>> > On Wed, Nov 22, 2017 at 05:26:24PM +0100, Patrick Wildt wrote:
>> > > On 2017/06/25 21:44, Tim Stewart wrote:
>> > > > 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.
>> > >
>> > > Reading RFC 7296 it looks like throwing INVALID_KE_PAYLOAD is fine for
>> > > both establishing the IKE SA and rekeying the Child SAs.  If we select a
>> > > proposal from the msg that uses a different DH group than the one that's
>> > > used in the KEi (in the same msg) we need to throw INVALID_KE_PAYLOAD.
>> > >
>> > > Since all messages subsequent to the initial exchange must be encrypted,
>> > > the INVALID_KE_PAYLOAD message on rekeying Child SAs must be encrypted.
>> > > Apparently with the previous diff the Child SA rekeying failed.  This is
>> > > because the code sends the INVALID_KE_PAYLOAD response unencrypted.
>> > >
>> > > Also I have found inconsistencies in handling INVALID_KE_PAYLOAD with us
>> > > acting as initiator.  I will take a look at both cases and will follow
>> > > up.
>> > >
>> > > Patrick
>> >
>> > Hi,
>> >
>> > This diff adds suport to rejecting IKE SA messages.  This means that we
>> > can reply to IKE SA INIT messages with no proposal chosen, as we already
>> > do for Child SAs.  For that the error "adding" is done in a new function
>> > shared by both send error handlers.  We need two "send error" functions
>> > because the init error is unencrypted, while all later ones are not.
>> > Now we can add more cases, like Child SA not found or that the DH group
>> > is not what we expect.  I think that is quite an improvement.
>> >
>> > One "issue" is retransmits.  The RFC specifies that IKEv2 is a reliable
>> > protocol.  When a packet is lost, the initiator has to retransmit the
>> > request and the responder must retransmit the saved reply.  On IKE SA
>> > INIT error we don't save our response, which means that we will handle
>> > the request as if we never had it.  The RFC says we shouldn't do that,
>> > because DoS protection and so on.  What we should do is keep our reply
>> > around and check if it is indeed a retransmit.  If so we reply with the
>> > same reply, thus saving resources.  If it is an attempt to create a new
>> > IKE SA, drop the old SA so that iked(8) can attempt to create a new SA.
>> >
>> > Thoughts?
>> >
>> > Patrick
>
> Updated diff also responds with INVALID_KE_PAYLOAD when the responder
> has a lower SA lifetime and initiates the CHILD_SA_CREATE.
>
> Handling rejections (as in receiving INVALID_KE_PAYLOAD) is still a ToDo
> I want to tackle next.

Patrick, thanks for running with this.  I see from the CVS log that a
version of this patch and other patches have been applied.  I will
update my system and do some testing.

> ok?
>
> diff --git a/sbin/iked/iked.h b/sbin/iked/iked.h
> index a0059682bc8..01f8ac00506 100644
> --- a/sbin/iked/iked.h
> +++ b/sbin/iked/iked.h
> @@ -508,6 +508,7 @@ struct iked_message {
>   struct iked_proposalsmsg_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;
> diff --git a/sbin/iked/ikev2.c b/sbin/iked/ikev2.c
> index b505f763153..b8cef9061fc 100644
> --- a/sbin/iked/ikev2.c
> +++ b/sbin/iked/ikev2.c
> @@ -76,6 +76,7 @@ int  ikev2_resp_ike_eap(struct iked *, struct iked_sa *, 
> struct ibuf *);
>  int   ikev2_send_auth_failed(struct iked *, struct iked_sa *);
>  int   ikev2_send_error(struct iked *, struct iked_sa *,
>   struct iked_message *, uint8_t);
> +int   ikev2_send_init_error(struct ike

Re: Please test: IPsec w/o KERNEL_LOCK()

2017-10-21 Thread Tim Stewart
Tim Stewart <t...@stoo.org> writes:

> Martin Pieuchot <m...@openbsd.org> writes:
>
>> On 11/10/17(Wed) 17:01, Martin Pieuchot wrote:
>>> OpenBSD 6.2 includes nice performance and latency improvements due to
>>> the work done in the Network Stack in the previous years.  However as
>>> soon as IPsec is enabled, all network related processing are affected.
>>> In other words you cannot profit from the last MP work in the Network
>>> stack if you use IPsec.
>
> By "IPsec is enabled," do you mean as soon as an AS appears in the
> kernel?  Or is it some other condition?
>
>>> During the last 6 months I hoped that somebody else would look at the
>>> IPsec stack and tell us what needs to be done.  This didn't happen.
>>>
>>> Now that 6.2 is released, we cannot afford to continue to parallelize
>>> the Network Stack if some of our users and testers still run it under
>>> KERNEL_LOCK().
>>>
>>> So I did an audit of the IPsec stack and came with the small diff below.
>>> This diff doesn't remove the KERNEL_LOCK() (yet), but add some asserts
>>> to make sure that the global data structure are all accessed while
>>> holding the NET_LOCK().
>>
>> Here's the diff to stop grabbing the KERNEL_LOCK(), please test and
>> report back.
>
> I have several iked tunnels that are normally active.  I applied both
> patches last night against 6.2-stable, rebooted, and my router was hung
> when I woke up this morning.  Unfortunately I wasn't really set up to
> capture crash information, and there are no dumps in /var/crash/.
>
> I don't have much experience with capturing OpenBSD kernel panics.  I've
> set up screen on another system so that I'll have a log of serial
> console activity (this is an apu2c4) and have set ddb.console=1.  I will
> also reboot with bsd.gdb this time.  Is there anything else I should to
> increase my chances of getting a useful dump?

I reproduced the lockup again.  There was no output on the console.  The
router stopped forwarding traffic and I found that the root shell I had
open on the console was not respond.  I sent a BREAK and was dropped
into ddb.

Following is the output of `show panic' a `trace' for each CPU, and a
`ps':

#
# Stopped at  db_enter+0x5 
[/home/src-cvs/sys/arch/amd64/amd64/db_interface.c
ddb{0}> show panic
the kernel did not panic
ddb{0}> trace
db_enter(8000220f98f8,10,8000220f9888,302,8,8122bdd5) at 
db_enter+0x5 [/home/src-cvs/sys/arch/amd64/amd64/db_interface.c:402]
comintr(81ab42b0,800cf000,0,1,800ce480,81b225a8)
 at comintr+0x18f [/home/src-cvs/sys/dev/ic/com.c:1091]
intr_handler(8000220f9be0,800ce400,7,9,800ce480,8007af00)
 at intr_handler+0x5e [/home/src-cvs/sys/arch/amd64/amd64/intr.c:534]
Xintr_ioapic_edge4() at Xintr_ioapic_edge4+0xcc
--- interrupt ---
end of kernel
end trace frame: 0x7b91c905c700, count: -4
0x4c48246c894c5024:
ddb{0}> machine ddbcpu 1
Stopped at  x86_ipi_db+0x5 
[/home/src-cvs/sys/arch/amd64/amd64/db_interface.c:356]:popq%rbp
ddb{1}> trace
x86_ipi_db(8111dd22,10,8000220bf4a8,246,8,8122bdc5) at 
x86_ipi_db+0x5 [/home/src-cvs/sys/arch/amd64/amd64/db_interface.c:356]
x86_ipi_handler(800716b8,1388,80014300,80071000,0,0) at 
x86_ipi_handler+0x6b [/home/src-cvs/sys/arch/amd64/amd64/ipi.c:106]
Xresume_lapic_ipi() at Xresume_lapic_ipi+0x1f
--- interrupt ---
end of kernel
end trace frame: 0x7bbf6405c75250cc, count: -3
0x41cb8c419c524153:
ddb{1}> machine ddbcpu 2
Stopped at  x86_ipi_db+0x5 
[/home/src-cvs/sys/arch/amd64/amd64/db_interface.c:356]:popq%rbp
ddb{2}> trace
x86_ipi_db(8111dd22,10,8000220c57f8,246,8,8122bdc5) at 
x86_ipi_db+0x5 [/home/src-cvs/sys/arch/amd64/amd64/db_interface.c:356]
x86_ipi_handler(800726b8,1388,80014400,80072000,0,0) at 
x86_ipi_handler+0x6b [/home/src-cvs/sys/arch/amd64/amd64/ipi.c:106]
Xresume_lapic_ipi() at Xresume_lapic_ipi+0x1f
--- interrupt ---
end of kernel
end trace frame: 0x7bbf6405c75250cc, count: -3
0x41cb8c419c524153:
ddb{2}> machine ddbcpu 3
Stopped at  x86_ipi_db+0x5 
[/home/src-cvs/sys/arch/amd64/amd64/db_interface.c:356]:popq%rbp
ddb{3}> trace
x86_ipi_db(8000f4d0,10,8000f438,246,8,8122bdc5) at 
x86_ipi_db+0x5 [/home/src-cvs/sys/arch/amd64/amd64/db_interface.c:356]
x86_ipi_handler(80002221cd70,0,80074000,80002211e2f0,c,1080be) 
at x86_ipi_handler+0x6b [/home/src-cvs/sys/arch/amd64/amd64/ipi.c:106]
Xresume_lapic_ipi() at Xresume_lapic_ipi+0x1f
--- interrupt ---
end of kernel
end trace frame: 0x7bbf6405c75250cc, count: -3
0x41cb8c419c524153:
ddb{3}> machine ddbcpu 0
ddb{0}> ps
   PID TID   PPID

Re: Please test: IPsec w/o KERNEL_LOCK()

2017-10-21 Thread Tim Stewart
Martin Pieuchot <m...@openbsd.org> writes:

> On 11/10/17(Wed) 17:01, Martin Pieuchot wrote:
>> OpenBSD 6.2 includes nice performance and latency improvements due to
>> the work done in the Network Stack in the previous years.  However as
>> soon as IPsec is enabled, all network related processing are affected.
>> In other words you cannot profit from the last MP work in the Network
>> stack if you use IPsec.

By "IPsec is enabled," do you mean as soon as an AS appears in the
kernel?  Or is it some other condition?

>> During the last 6 months I hoped that somebody else would look at the
>> IPsec stack and tell us what needs to be done.  This didn't happen.
>>
>> Now that 6.2 is released, we cannot afford to continue to parallelize
>> the Network Stack if some of our users and testers still run it under
>> KERNEL_LOCK().
>>
>> So I did an audit of the IPsec stack and came with the small diff below.
>> This diff doesn't remove the KERNEL_LOCK() (yet), but add some asserts
>> to make sure that the global data structure are all accessed while
>> holding the NET_LOCK().
>
> Here's the diff to stop grabbing the KERNEL_LOCK(), please test and
> report back.

I have several iked tunnels that are normally active.  I applied both
patches last night against 6.2-stable, rebooted, and my router was hung
when I woke up this morning.  Unfortunately I wasn't really set up to
capture crash information, and there are no dumps in /var/crash/.

I don't have much experience with capturing OpenBSD kernel panics.  I've
set up screen on another system so that I'll have a log of serial
console activity (this is an apu2c4) and have set ddb.console=1.  I will
also reboot with bsd.gdb this time.  Is there anything else I should to
increase my chances of getting a useful dump?

-TimS

--
Tim Stewart
---
Mail:   t...@stoo.org
Matrix: @tim:stoo.org



Re: Please test: IPsec w/o KERNEL_LOCK()

2017-10-21 Thread Tim Stewart

Stuart Henderson <s...@spacehopper.org> writes:

> On 2017/10/21 10:33, Tim Stewart wrote:
>> I don't have much experience with capturing OpenBSD kernel panics.  I've
>> set up screen on another system so that I'll have a log of serial
>> console activity (this is an apu2c4) and have set ddb.console=1.  I will
>> also reboot with bsd.gdb this time.
>
> Don't boot bsd.gdb - boot the normal kernel, but it can sometimes be useful
> to have the debug kernel around for analysis post-crash.

I just sent another email with a reproduction and ddb output, but it was
booted into the gdb kernel.  Do you think the results will be different
if I boot with the non-GDB kernel?

-TimS

--
Tim Stewart
---
Mail:   t...@stoo.org
Matrix: @tim:stoo.org



Re: Please test: IPsec w/o KERNEL_LOCK()

2017-10-21 Thread Tim Stewart
Stuart Henderson <s...@spacehopper.org> writes:

> On 2017/10/21 12:04, Tim Stewart wrote:
>> *49727  296965  0  0  7 0x14200crynlk
>
> aha, it was that one. Try this diff on top.
>
> Index: fpu.c
> ===
> RCS file: /cvs/src/sys/arch/amd64/amd64/fpu.c,v
> retrieving revision 1.38
> diff -u -p -r1.38 fpu.c
> --- fpu.c 14 Oct 2017 04:44:43 -  1.38
> +++ fpu.c 21 Oct 2017 16:16:14 -
> @@ -347,7 +347,7 @@ void
>  fpu_kernel_enter(void)
>  {
>   struct cpu_info *ci = curcpu();
> - uint32_t cw;
> + struct savefpu  *sfp;
>   int  s;
>
>   /*
> @@ -376,10 +376,11 @@ fpu_kernel_enter(void)
>
>   /* Initialize the FPU */
>   fninit();
> - cw = __INITIAL_NPXCW__;
> - fldcw();
> - cw = __INITIAL_MXCSR__;
> - ldmxcsr();
> + sfp = _addr->u_pcb.pcb_savefpu;
> + memset(>fp_fxsave, 0, sizeof(sfp->fp_fxsave));
> + sfp->fp_fxsave.fx_fcw = __INITIAL_NPXCW__;
> + sfp->fp_fxsave.fx_mxcsr = __INITIAL_MXCSR__;
> + fxrstor(>fp_fxsave);
>  }
>
>  void

I've been running with this additional patch for a couple of hours and
the hang has not reappeared.  I'll keep the system active and confirm
thta everything looks good tomorrow.

I swear I've seen this patch before on a list but can't find the
original.  Can someone give me or point me at some context, so I know
what I've just done? :)

Thanks!

-TimS

-- 
Tim Stewart
---
Mail:   t...@stoo.org
Matrix: @tim:stoo.org



Re: Please test: IPsec w/o KERNEL_LOCK()

2017-10-21 Thread Tim Stewart

Stuart Henderson <s...@spacehopper.org> writes:

> On 2017/10/21 14:52, Tim Stewart wrote:
>> Stuart Henderson <s...@spacehopper.org> writes:
>>
>> > On 2017/10/21 12:04, Tim Stewart wrote:
>> >> *49727  296965  0  0  7 0x14200crynlk
>> >
>> > aha, it was that one. Try this diff on top.
>> >
>> > Index: fpu.c
>> > ===
>> > RCS file: /cvs/src/sys/arch/amd64/amd64/fpu.c,v
>> > retrieving revision 1.38
>> > diff -u -p -r1.38 fpu.c
>> > --- fpu.c  14 Oct 2017 04:44:43 -  1.38
>> > +++ fpu.c  21 Oct 2017 16:16:14 -
>> > @@ -347,7 +347,7 @@ void
>> >  fpu_kernel_enter(void)
>> >  {
>> >struct cpu_info *ci = curcpu();
>> > -  uint32_t cw;
>> > +  struct savefpu  *sfp;
>> >int  s;
>> >
>> >/*
>> > @@ -376,10 +376,11 @@ fpu_kernel_enter(void)
>> >
>> >/* Initialize the FPU */
>> >fninit();
>> > -  cw = __INITIAL_NPXCW__;
>> > -  fldcw();
>> > -  cw = __INITIAL_MXCSR__;
>> > -  ldmxcsr();
>> > +  sfp = _addr->u_pcb.pcb_savefpu;
>> > +  memset(>fp_fxsave, 0, sizeof(sfp->fp_fxsave));
>> > +  sfp->fp_fxsave.fx_fcw = __INITIAL_NPXCW__;
>> > +  sfp->fp_fxsave.fx_mxcsr = __INITIAL_MXCSR__;
>> > +  fxrstor(>fp_fxsave);
>> >  }
>> >
>> >  void
>>
>> I've been running with this additional patch for a couple of hours and
>> the hang has not reappeared.  I'll keep the system active and confirm
>> thta everything looks good tomorrow.
>>
>> I swear I've seen this patch before on a list but can't find the
>> original.  Can someone give me or point me at some context, so I know
>> what I've just done? :)
>
> Diff is from mikeb. It initializes the fpu more completely, we suspect
> something in the userland state wasn't getting cleared when entering the
> kernel. I saw some problems with aes-ni up after the "Correctly handle
> exceptions when restoring an invalid FPU context" commit. (aes-ni uses
> floating point registers).

I see.  So I'm guessing that an "unlocked" IPsec more likely to hit this
bug because it's using AES-NI outside of KERNEL_LOCK() now?  Am I close?

-TimS

--
Tim Stewart
---
Mail:   t...@stoo.org
Matrix: @tim:stoo.org



iked support for IKEv2 Message Fragmentation (RFC 7383)

2018-06-19 Thread Tim Stewart
Hello tech@,

My IKEv2 sessions are occasionally down due to transit networks dropping
UDP fragments for one reason or another[1].  It happens frequently
enough that I am considering implementing support for RFC 7383 in iked.

Before I dig in, I feel that I should ask if anyone has already started
on such work.  If not, perhaps someone that is familiar with the code
could suggest an approach at a high level?

Thanks for any advice,

-TimS


[1] Whenver I've asked, the reason is usually something about DDoS
prevention.

--
Tim Stewart
---
Mail:   t...@stoo.org
Matrix: @tim:stoo.org



Re: iked support for IKEv2 Message Fragmentation (RFC 7383)

2018-07-17 Thread Tim Stewart

Hello tech@,

Here is a small initial patch related to message fragmentation. 
ikev2_msg_decrypt() claims to strip the padding from the decrypted IKE 
payloads, but actually leaves it tacked on the end of the returned ibuf. 
This is fine in the unfragmented case since the inner payloads have 
their own lengths, but it is awkward to use the returned ibuf directly 
as input to a fragment queue.


The patch below sets the proper size on the returned ibuf.  The code 
still functions well for me even in the unfragmented case.


-TimS

Index: ikev2_msg.c
===
RCS file: /cvs/src/sbin/iked/ikev2_msg.c,v
retrieving revision 1.53
diff -u -p -u -p -r1.53 ikev2_msg.c
--- ikev2_msg.c 27 Nov 2017 18:39:35 -  1.53
+++ ikev2_msg.c 18 Jul 2018 00:18:45 -
@@ -616,7 +616,7 @@ ikev2_msg_decrypt(struct iked *env, stru
__func__, outlen, encrlen, pad);
print_hex(ibuf_data(out), 0, ibuf_size(out));

-   if (ibuf_setsize(out, outlen) != 0)
+   if (ibuf_setsize(out, outlen - pad - 1) != 0)
goto done;

ibuf_release(src);



Re: iked(8): add support for IKEv2 Message Fragmentation

2019-03-31 Thread Tim Stewart

On 3/30/19 3:11 PM, Tobias Heider wrote:

Hi Stuart,

I'm glad to see people are using this.
There's some smaller fixes that I haven't sent to the list yet, so
probably I'll send an updated diff on monday.


I plan to start using this patch this week, likely as soon as you send 
the updated diff.  I started on the same feature in June of 2018, but 
other tasks took priority and it stalled.


I have a pretty good testbed for this, as I have several site-to-site 
links that drop UDP fragments and several road warriors that would 
experience fragment drops depending on the cell network they use.  I 
will report back on this thread with my findings.


Thanks for the patch!

-TimS




Regards,
Tobias

On 3/30/19 6:43 PM, Stuart Henderson wrote:

This diff hasn't gone anywhere recently - I've been using it since
Tobias posted it with no problems. Any comments on whether it should
go in, and if so, before/after 6.5? The feature is disabled by default.

Index: config.c
===
RCS file: /cvs/src/sbin/iked/config.c,v
retrieving revision 1.49
diff -u -p -r1.49 config.c
--- config.c27 Nov 2017 18:39:35 -  1.49
+++ config.c30 Mar 2019 17:41:33 -
@@ -94,12 +94,30 @@ config_free_kex(struct iked_kex *kex)
  }
  
  void

+config_free_fragments(struct iked_frag *frag)
+{
+   size_t i;
+   if (frag && frag->frag_arr) {
+   for (i = 0; i < frag->frag_count; i++) {
+   free(frag->frag_arr[i]->frag_data);
+   frag->frag_arr[i]->frag_data = NULL;
+   free(frag->frag_arr[i]);
+   frag->frag_arr[i] = NULL;
+   }
+   free(frag->frag_arr);
+   frag->frag_arr = NULL;
+   bzero(frag, sizeof(struct iked_frag));
+   }
+}
+
+void
  config_free_sa(struct iked *env, struct iked_sa *sa)
  {
timer_del(env, >sa_timer);
timer_del(env, >sa_keepalive);
timer_del(env, >sa_rekey);
  
+	config_free_fragments(>sa_fragments);

config_free_proposals(>sa_proposals, 0);
config_free_childsas(env, >sa_childsas, NULL, NULL);
sa_free_flows(env, >sa_flows);
@@ -925,6 +943,29 @@ config_setkeys(struct iked *env)
EVP_PKEY_free(key);
  
  	return (ret);

+}
+
+int
+config_setfragmentation(struct iked *env)
+{
+   unsigned int boolval;
+
+   boolval = env->sc_frag;
+   proc_compose(>sc_ps, PROC_IKEV2, IMSG_CTL_FRAGMENTATION,
+   , sizeof(boolval));
+   return (0);
+}
+
+int
+config_getfragmentation(struct iked *env, struct imsg *imsg)
+{
+   unsigned int boolval;
+
+   IMSG_SIZE_CHECK(imsg, );
+   memcpy(, imsg->data, sizeof(boolval));
+   env->sc_frag = boolval;
+   log_debug("%s: %sfragmentation", __func__, env->sc_frag ? "" : "no ");
+   return (0);
  }
  
  int

Index: iked.c
===
RCS file: /cvs/src/sbin/iked/iked.c,v
retrieving revision 1.36
diff -u -p -r1.36 iked.c
--- iked.c  27 Nov 2017 18:39:35 -  1.36
+++ iked.c  30 Mar 2019 17:41:33 -
@@ -251,6 +251,7 @@ parent_configure(struct iked *env)
fatal("pledge");
  
  	config_setmobike(env);

+   config_setfragmentation(env);
config_setcoupled(env, env->sc_decoupled ? 0 : 1);
config_setmode(env, env->sc_passive ? 1 : 0);
config_setocsp(env);
@@ -282,6 +283,7 @@ parent_reload(struct iked *env, int rese
config_setcompile(env, PROC_IKEV2);
  
  		config_setmobike(env);

+   config_setfragmentation(env);
config_setcoupled(env, env->sc_decoupled ? 0 : 1);
config_setmode(env, env->sc_passive ? 1 : 0);
config_setocsp(env);
Index: iked.conf.5
===
RCS file: /cvs/src/sbin/iked/iked.conf.5,v
retrieving revision 1.53
diff -u -p -r1.53 iked.conf.5
--- iked.conf.5 31 Jan 2018 13:25:55 -  1.53
+++ iked.conf.5 30 Mar 2019 17:41:33 -
@@ -136,6 +136,12 @@ This is the default.
  .It Ic set decouple
  Don't load the negotiated SAs and flows from the kernel.
  This mode is only useful for testing and debugging.
+.It Ic set fragmentation
+Enable IKEv2 Message Fragmentation (RFC 7383) support.
+This allows IKEv2 to operate in environments that might block IP fragments.
+.It Ic set nofragmentation
+Disables IKEv2 Message Fragmentation support.
+This is the default.
  .It Ic set mobike
  Enable MOBIKE (RFC 4555) support.
  This is the default.
Index: iked.h
===
RCS file: /cvs/src/sbin/iked/iked.h,v
retrieving revision 1.119
diff -u -p -r1.119 iked.h
--- iked.h  6 Aug 2018 06:30:06 -   1.119
+++ iked.h  30 Mar 2019 17:41:33 -
@@ -362,6 +362,21 @@ struct iked_kex {
struct ibuf *kex_dhpeer;/* pointer to i or r */
  };

Re: iked(8): add support for IKEv2 Message Fragmentation

2019-04-16 Thread Tim Stewart
Tim Stewart  writes:

> On 3/30/19 3:11 PM, Tobias Heider wrote:
>> Hi Stuart,
>>
>> I'm glad to see people are using this.
>> There's some smaller fixes that I haven't sent to the list yet, so
>> probably I'll send an updated diff on monday.
>
> I plan to start using this patch this week, likely as soon as you send
> the updated diff.  I started on the same feature in June of 2018, but
> other tasks took priority and it stalled.
>
> I have a pretty good testbed for this, as I have several site-to-site
> links that drop UDP fragments and several road warriors that would
> experience fragment drops depending on the cell network they use.  I
> will report back on this thread with my findings.
>
> Thanks for the patch!
>
> -TimS

I have been using this patch for over a week and it has increased the
reliability of my road warrior VPN as I move across a variety of
networks.  It also allows one of my previously-broken site-to-site VPNs
to function again.  Thanks!


I did have one strange problem today, and I have no idea if it's related
to this patch or not.  One of my site-to-site connections went down and
I saw the following in one of the peer's log:

Apr 16 12:59:05 site-a iked[15114]: ikev2_msg_send: sendtofrom: No buffer space 
available
Apr 16 13:00:07 site-a iked[15114]: ikev2_msg_send: INFORMATIONAL request from 
5.6.7.8:500 to 1.2.3.4:500 msgid 929, 80 bytes
Apr 16 13:00:07 site-a iked[15114]: ikev2_recv: INFORMATIONAL response from 
responder 1.2.3.4:500 to 5.6.7.8:500 policy 'central' id 929, 80 bytes
Apr 16 13:01:07 site-a iked[15114]: ikev2_msg_send: INFORMATIONAL request from 
5.6.7.8:500 to 1.2.3.4:500 msgid 930, 80 bytes
Apr 16 13:01:07 site-a iked[15114]: ikev2_recv: INFORMATIONAL response from 
responder 1.2.3.4:500 to 5.6.7.8:500 policy 'central' id 930, 80 bytes
Apr 16 13:02:07 site-a iked[15114]: ikev2_msg_send: INFORMATIONAL request from 
5.6.7.8:500 to 1.2.3.4:500 msgid 931, 80 bytes
Apr 16 13:02:07 site-a iked[15114]: ikev2_recv: INFORMATIONAL response from 
responder 1.2.3.4:500 to 5.6.7.8:500 policy 'central' id 931, 80 bytes
Apr 16 13:03:00 site-a iked[15114]: ikev2_acquire_sa: flow wasn't found
Apr 16 13:03:07 site-a iked[15114]: pfkey_sa_last_used: message: No such process
Apr 16 13:03:07 site-a iked[15114]: pfkey_sa_last_used: message: No such process
Apr 16 13:03:30 site-a iked[15114]: ikev2_acquire_sa: flow wasn't found
Apr 16 13:04:00 site-a iked[15114]: ikev2_acquire_sa: flow wasn't found
Apr 16 13:04:02 site-a iked[15114]: ikev2_recv: INFORMATIONAL request from 
responder 1.2.3.4:500 to 5.6.7.8:500 policy 'central' id 0, 80 bytes
Apr 16 13:04:02 site-a iked[15114]: ikev2_msg_send: INFORMATIONAL response from 
5.6.7.8:500 to 1.2.3.4:500 msgid 0, 80 bytes
Apr 16 13:04:07 site-a iked[15114]: pfkey_sa_last_used: message: No such process
Apr 16 13:04:07 site-a iked[15114]: pfkey_sa_last_used: message: No such process
Apr 16 13:04:30 site-a iked[15114]: ikev2_acquire_sa: flow wasn't found
Apr 16 13:05:00 site-a iked[15114]: ikev2_acquire_sa: flow wasn't found
Apr 16 13:05:04 site-a iked[15114]: ikev2_recv: INFORMATIONAL request from 
responder 1.2.3.4:500 to 5.6.7.8:500 policy 'central' id 1, 80 bytes
Apr 16 13:05:04 site-a iked[15114]: ikev2_msg_send: INFORMATIONAL response from 
5.6.7.8:500 to 1.2.3.4:500 msgid 1, 80 bytes

I restarted site-a's iked and everything came back up and has been
working fine since.

I see there are some other iked patches in-flight on this list
(including a new version of this one), so I'm happy to punt on this for
now and see if it happens again after I've applied the latest versions
of all patches.

-TimS


>> On 3/30/19 6:43 PM, Stuart Henderson wrote:
>>> This diff hasn't gone anywhere recently - I've been using it since
>>> Tobias posted it with no problems. Any comments on whether it should
>>> go in, and if so, before/after 6.5? The feature is disabled by default.
>>>
>>> Index: config.c
>>> ===
>>> RCS file: /cvs/src/sbin/iked/config.c,v
>>> retrieving revision 1.49
>>> diff -u -p -r1.49 config.c
>>> --- config.c27 Nov 2017 18:39:35 -  1.49
>>> +++ config.c30 Mar 2019 17:41:33 -
>>> @@ -94,12 +94,30 @@ config_free_kex(struct iked_kex *kex)
>>>   }
>>> void
>>> +config_free_fragments(struct iked_frag *frag)
>>> +{
>>> +   size_t i;
>>> +   if (frag && frag->frag_arr) {
>>> +   for (i = 0; i < frag->frag_count; i++) {
>>> +   free(frag->frag_arr[i]->frag_data);
>>> +   frag->frag_arr[i]->frag_data = NULL;
>>> +   free(frag->frag_arr[i]);
>>> +   frag->frag_a