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.
