Hi Thanks for your reply.
Yes, sure. This fix would also be correct. Best Regards Sibar Soumi Software Developer achelos GmbH | Vattmannstraße 1 | 33100 Paderborn | GERMANY [email protected] | www.achelos.de | www.iot.achelos.com | Follow us: LinkedIn | XING | YouTube -----Ursprüngliche Nachricht----- Von: Tobias Heider <[email protected]> Gesendet: Sonntag, 3. Juli 2022 14:30 An: Sibar Soumi <[email protected]> Cc: [email protected]; Claudia Priesterjahn <[email protected]> Betreff: Re: Bug in iked On Wed, Jun 22, 2022 at 01:02:17PM +0000, Sibar Soumi wrote: > Dear OpenBSD developers > > > > I would like to report an error in iked. > > > > The error occurs with the processing logic in case of simultaneous Child SA > rekeying. That is, by simultaneous rekeying, two Child SAs are created and > “the SA created with the lowest of the four nonces used in the two exchanges > SHOULD be closed by the endpoint that created it” (RFC7296 section 2.8.1). > > > > This decision is made in the iked implementation in ikev2.c in the if block > from L4390 > <https://github.com/openbsd/src/blob/a990b40e6d87ee721e36986e60fe36b7a033729c/sbin/iked/ikev2.c#L4390> > until L4407 > <https://github.com/openbsd/src/blob/a990b40e6d87ee721e36986e60fe36b7a033729c/sbin/iked/ikev2.c#L4407> > . > > > > But nr is not set to the minimum nonce for exchange initiated by peer but by > us, and ni which comes from sa->sa_simulat is already set to the minimum > nonce for exchange initiated by peer. > > > > Therefore, the comment in line 4393 shall be corrected and the comparison in > line 4402 shall be “ikev2_nonce_cmp(nr, ni) < 0” instead of > “ikev2_nonce_cmp(ni, nr) < 0” because the SA that has just been created by us > shall be deleted, if nr<ni. > > > > Best regards > > > > > > Sibar Soumi > > Software Developer > > > > achelos GmbH | Vattmannstraße 1 | 33100 Paderborn | GERMANY > > [email protected] <mailto:[email protected]> | www.achelos.de > <https://www.achelos.de/> | www.iot.achelos.com > <https://www.iot.achelos.com/> | Follow us: LinkedIn > <https://www.linkedin.com/company/achelos-gmbh> | XING > <https://www.xing.com/companies/achelosgmbh/updates> | YouTube > <https://www.youtube.com/channel/UCK1g0YpxJexVGYvUtr2IHUg> > > > > Die achelos GmbH ist nach ISO 9001 und ISO 27001 zertifiziert. | achelos GmbH > is certified according to ISO 9001 and ISO 27001. > > Geschäftsführung | Executive Board: Kathrin Asmuth, Thomas Freitag > > Registergericht | register court: Paderborn, HRB 8817 | USt-IdNr. | > VAT ID number: DE260414872 > > > > Diese Mitteilung ist vertraulich. Wenn Sie nicht der beabsichtigte > Empfänger sind, ist jegliche Verwendung, Beeinträchtigung, > > Offenlegung oder Vervielfältigung dieses Materials unautorisiert und > verboten. Bitte informieren Sie uns umgehend und > > vernichten Sie die E-Mail. | This communication is confidential. If > you are not the intended recipient, any use, interference with, > > disclosure or copying of this material is unauthorised and prohibited. Please > inform us immediately and destroy the email. > Hi Sibar, thanks for the report! It looks like you are right, the current comparison is indeed wrong. I think the best fix would be to switch ni and nr, which I think was the original intention here. ni should be the nonce for the exchange where we are initiator, nr is where we are responder. RFC7296 says that when our nonce < peer nonce we delete our simultaneously proposed child SA, so this should fix the comparsion below at 4402 if (ikev2_nonce_cmp(ni, nr) < 0) { 4403 ret = ikev2_childsa_delete_proposed(env, sa, 4404 &sa->sa_proposals); Below is my proposed fix. ok? Index: ikev2.c =================================================================== RCS file: /cvs/src/sbin/iked/ikev2.c,v retrieving revision 1.347 diff -u -p -r1.347 ikev2.c --- ikev2.c 28 May 2022 18:51:16 -0000 1.347 +++ ikev2.c 3 Jul 2022 12:20:33 -0000 @@ -4387,14 +4387,14 @@ ikev2_init_create_child_sa(struct iked * sa->sa_rnonce = msg->msg_nonce; msg->msg_nonce = NULL; - if (csa && (ni = sa->sa_simult) != NULL) { + if (csa && (nr = sa->sa_simult) != NULL) { log_info("%s: resolving simultaneous CHILD SA rekeying", SPI_SA(sa, __func__)); - /* set nr to minimum nonce for exchange initiated by peer */ + /* set ni to minimum nonce for exchange initiated by us */ if (ikev2_nonce_cmp(sa->sa_inonce, sa->sa_rnonce) < 0) - nr = sa->sa_inonce; + ni = sa->sa_inonce; else - nr = sa->sa_rnonce; + ni = sa->sa_rnonce; /* * If the exchange initated by us has smaller nonce, * then we have to delete our SAs.
openpgp-digital-signature.asc
Description: PGP signature
