Re: [IPsec] Paul Wouters' Discuss on draft-ietf-ipsecme-ikev2-multiple-ke-10: (with DISCUSS and COMMENT)

[Valery Smyslov <[e-mail]>]

Hi Paul,

let's continue :-)

I snipped the text where we are in agreement.

> -----Original Message-----
> From: Paul Wouters [mailto:[e-mail]]
> Sent: Tuesday, November 29, 2022 9:49 PM
> To: Valery Smyslov
> Cc: 'Paul Wouters'; 'The IESG'; [e-mail]; [e-mail];
> [e-mail] WG; Tero Kivinen
> Subject: Re: [IPsec] Paul Wouters' Discuss on draft-ietf-ipsecme-ikev2-multiple-ke-10: (with DISCUSS and
> COMMENT)
> 
> On Tue, 29 Nov 2022, Valery Smyslov wrote:
> 
> >> ### IANA entries mentions in the Introduction ?
> >>
> >> Shouldn't the introduction mention this draft introduces the IKE_FOLLOWUP_KE Exchange
> >> and the STATE_NOT_FOUND Notify Message Type, along with additional entries to the
> >> (now renamed) Key Exchanges Methods registry?
> >
> > I'm a bit puzzled here. The introduction currently is silent about the details of the proposed
> > extension. Do you want to add them there? Or did you mean the Abstract,
> > (which indeed contains some details) to be augmented with more details?
> 
> Yes I meant the abstract :)

I'm a bit reluctant to add all this information to the abstract. It is already a bit too long
(since Éric and Warren suggested to augment it with the explanation text of how
this design helps in situation when PQ algorithms are less trusted). So currently
the abstract is:

        This document describes how to extend the Internet Key Exchange Protocol
        Version 2 (IKEv2) to allow multiple key exchanges to take place 
        while computing a shared secret during a Security Association (SA) setup.
	
        The primary application of this feature in IKEv2 is the ability to perform one or more 
        post-quantum key exchanges in conjunction with the classical (Elliptic Curve) Diffie-Hellman (EC)DH key exchange,
        so that the resulting shared key is resistant against quantum computer attacks.
        Since there is currently no post-quantum key exchange that is trusted at
        the level that (EC)DH is trusted for against conventional (non-quantum)
        adversaries, performing multiple key exchanges with different post-quantum algorithms along
        with the well-established classical key exchange algorithms addresses this concern, since the
        overall security is at least as strong as each individual primitive.
	
        Another possible application for this extension is the ability to combine several key exchanges 
        in situations when no single key exchange algorithm is trusted by both initiator and responder.

       This document updates RFC7296 by renaming a transform type 4 from "Diffie-Hellman Group (D-H)"
        to "Key Exchange Method (KE)" and renaming a field in the Key Exchange Payload from "Diffie-Hellman Group Num"
        to "Key Exchange Method". It also renames an IANA registry for this transform type 
        from "Transform Type 4 - Diffie-Hellman Group Transform IDs" to 
        "Transform Type 4 - Key Exchange Method Transform IDs". These changes generalize 
        key exchange algorithms that can be used in IKEv2.

Do you *really* want to make it longer? I'm not sure that if we just mention 
the names of the added exchange and notifies, that will help readers of the abstract 
to understand what this document is about...

[update]

I've just noticed your message sent in response to Warren. So, since you insist :-),
I suggest adding the following text before "This document updates...":

    This document utilizes the IKE_INTERMEDIATE exchange to perform multiple key exchanges when
    an IKE SA is being established and introduces a new IKEv2 exchange IKE_FOLLOWUP_KE to perform 
    them when IKE SA is up (during rekeys or creating additional Child SAs).

is it now OK?

[/update]

> >> ### Additional key exchanges DoS ?
> >>
> >> The following paragraph raised an issue for me:
> >> ```
> >>    If the responder selects NONE for some Additional Key Exchange types
> >>    (provided they are proposed by the initiator), then the corresponding
> >>    IKE_INTERMEDIATE exchanges MUST NOT take place.
> >> ```
> >> If the initiator's local policy requires at least one Additional Key Exchange,
> >> an attacker sending back a quick reply with only NONE replies would be a DoS.
> >> I think similar to like the original IKE_SA_INIT, perhaps we need to give some
> >> advise that if the local policy would lead to permanent failure, that it
> >> should wait for other (more legitimate) responses to this IKE_SA_INIT ?
> >
> > As you pointed out, the situation is exactly the same as described
> > in Section 2.4 of RFC 7296:
> >
> >   One type of DoS attack on the initiator of an IKE SA can be avoided
> >   if the initiator takes proper care: since the first two messages of
> >   an SA setup are not cryptographically protected, an attacker could
> >   respond to the initiator's message before the genuine responder and
> >   poison the connection setup attempt.  To prevent this, the initiator
> >   MAY be willing to accept multiple responses to its first message,
> >   treat each response as potentially legitimate, respond to each one,
> >   and then discard all the invalid half-open connections when it
> >   receives a valid cryptographically protected response to any one of
> >   its requests.  Once a cryptographically valid response is received,
> >   all subsequent responses should be ignored whether or not they are
> >   cryptographically valid.
> >
> > Since the draft adds nothing new into the negotiation mechanism
> > in the IKE_SA_INIT, I see no need to reiterate this advice here
> > (otherwise we may want to repeat a lot of text from base IKEv2 spec :-)).
> 
> I don't think it would hurt pointing to Section 2.4 of RFC 7296. I see
> this as a likely possible implemention mistake.

My problem with your proposal here is that this situation is not 
specific to this document. In other words, whether the initiator
proposes any additional key exchanges or not, if an attacker
manages to send a response before the genuine responder
and the initiator continues establishing IKE SA using this response,
then it will fail. There is nothing specific to the multiple key exchanges.
And we implicitly assume that implementations follow RFC 7296.

> >> ### State loss issue
> >> ```
> >>    After receiving this notification the initiator MAY start
> >>    a new CREATE_CHILD_SA exchange which may eventually be followed by
> >>    the IKE_FOLLOWUP_KE exchanges, to retry the failed attempt.  If the
> >>    initiator continues to receive STATE_NOT_FOUND notifications [...]
> >> ```
> >> How could this happen? If the state was lost, eg due to reboot, there would
> >> need to come a new IKE SA, that can then send a new CREATE_CHILD_SA. I don't
> >> see how that could lead to another STATE_NOT_FOUND. But the paragraph then
> >> also continues with "and delete [the] IKE SA". But this IKE SA is brand new?
> >> I would just remove this entire paragraph as I think this cannot happen. Or
> >> at least it is not a special case and existing abort code handles this already.
> >
> > No, this is not concerned with reboots. It is concerned with the situation
> > when it takes too long for the initiator to compute the next public key
> > value to send to the responder (the delay may also be caused by lost packets).
> > So, when the responder first receives the CREATE_CHILD_SA exchange
> > and negotiates using multiple key exchanges, it expects that some
> > IKE_FOLLOWUP_KE exchanges shortly follow (with some definition
> > of "shortly"). So, it creates a state that would contain all the shared
> > key computed as a result of these exchanges. But the responder
> > cannot keep this state forever (for example, the initiator may
> > be switched off and never send next request), so the responder
> > have to do housekeeping and remove this state if the responder
> > doesn't receive the next IKE_FOLLOWUP_KE exchange in a timely fashion
> > (again, with some definition of "timely fashion"). Then consider the
> > situation when initiator cannot make the next request within
> > this period (either it takes too long to compute the key or some request
> > messages were lost and the responder received the n-th retransmission).
> > In this case the responder receives the IKE_FOLLOWUP_KE exchange,
> > but it doesn't already have the state to which it can add this new
> > shared key, so it responds with NO_STATE_FOUND. This is non-fatal
> > notification, so the initiator may start all from scratch (from the
> > CREATE_CHILD_SA exchange) in a hope that this time its performance
> > or network conditions would be better.
> >
> > Does this help?
> 
> Yes, but then I think you need to split the text that is currently in
> one large paragraph into multiple ones, because the start of the
> paragraph has:
> 
>     It is possible that due to some unexpected events (e.g. reboot) the
>     initiator may lose its state
> 
> So it is not clear that a large part of this paragraph does not in fact
> involve "state lost due to reboot". So please split the paragraph a bit
> into pieces to make this more clear.

Actually, it is already split in two paras. I've added some clarification text into the 
beginning of the second para, now they look like:

    It is possible that due to some unexpected events (e.g. reboot)
    the initiator may lose its state and forget that it is in the process of performing
    additional key exchanges and thus never start the remaining IKE_FOLLOWUP_KE exchanges.
    The responder MUST handle this situation gracefully and delete
    the associated state if it does not receive the next expected 
    IKE_FOLLOWUP_KE request after some reasonable period of time.
    Note that due to various factors such as computational resource and
    key exchange algorithm used, it is not possible to give a normative
    guidance on how long this timeout period should be. In general, 5-20
    seconds of waiting time should be appropriate in most cases.  

    It is also possible that it takes too long for the initiator to 
    prepare and send the next IKE_FOLLOWUP_KE request or due to the network
    conditions it is retransmitted. In this case the request may reach the responder  
    when it may have already deleted the associated state following the advice above. 
    If the responder receives IKE_FOLLOWUP_KE and the content of this notify does not correspond
    to any active key exchange state the responder has, it MUST send back a new error type notification
    STATE_NOT_FOUND ...

Does this resolve your concern?

> >> ### IKE session resumption
> >>
> >> Should there be a section updating RFC 5723 Section 5.1, or is the method there
> >> specified quantum-safe if the initial IKE SA was protected using this document's
> >> mechanism? See https://www.rfc-editor.org/rfc/rfc5723.html#section-5.1
> >>
> >> I think the IKE resumption can work "as normal", as no KE payload is
> >> involved in the resumption, but it would be nice if a sentence somewhere
> >> in this document could confirm this.
> >
> > Yes, I agree. We didn't add anything since there is no interaction with
> > RFC 5723 (it works "as normal"). We can add some text if you think
> > it is needed.
> 
> Just a one sentence somewhere would be nice yes.

OK, if you insist :-)

I've created a new section "Interaction with IKEv2 Extensions" with a single para:

   It is believed that this specification requires no modification to the IKEv2 extensions defined so far.
   In particular, IKE SA resumption mechanism defined in [RFC5723] can be used to resume
   IKE SAs created using this specification. 

and I also put the section "Interaction with Childless IKE SA" as a subsection in this section.

> >> Also the "may" responder is unclear to me. What other response could there be and why?
> >
> > The responder has few choices here. It expects the initiator
> > starts childless IKE_AUTH, but it didn't happen.
> > First the responder may fail the IKE SA itself (e.g. by sending SYNTAX_ERROR).
> > It will be a fatal error. Then, the responder may agree to create
> > the Child SA, but immediately delete it (no guarantee that
> > no data will be sent over it). The suggested method
> > is a bit of protocol hack, but it allows to avoid creating
> > of weak Child SA and at the same time it keeps chances
> > that the IKE SA be upgraded to be PQ-safe.
> 
> Yeah these are all pretty unlikely and unsafe things to do :P

That is that :-)

> >> ### IKE_FOLLOWUP_KE name
> >>
> >> I find the name IKE_FOLLOWUP_KE a little confusing, as this exchange applies to
> >> IKE and IPsec SA rekey negotiations. Why is it not called FOLLOWUP_ADDITIONAL_KE ?
> >> Or CREATE_CHILD_SA_FOLLOWUP(_KE) (a sort of bad name too but that at least follows the
> >> bad name from the original IKEv2 spec)
> >
> > If you remember, we have had some discussion in the ipsecme WG,
> > and then you suggested that all new IKE exchanges start with IKE_ prefix.
> > We just followed this suggestion :-)
> 
> Ah yes, I remember :)
> 
> > Well, I don't care much what the name is, so I rely on my co-authors on this.
> > I only hope it won't be too long and will be easy to pronounce :-)
> 
> Just give it some thought, but if we leave it as is that is fine too.

As I'm lazy, I prefer to do nothing here (unless you or my co-authors strongly disagree :-))

> >> Related to this is text in the Security Considerations:
> >>
> >> ```
> >>    In particular, the authenticity of the SAs established
> >>    under IKEv2 is protected using a pre-shared key, RSA, DSA, or ECDSA
> >>    algorithms.
> >> ```
> >> This text is also incorrect as RFC 7427 allows us to use post-quantum authentication
> >> algorithms that have a SubjectPublicKeyInfo (SPKI) definition. There might not be any
> >> now, but there will presumbly be some in the future.
> >
> > Do you want to augment this text? Any suggestions?
> 
> How about:
> 
>     In particular, the authenticity of the SAs established under IKEv2
>     are not protected with post-quantum algorithms.

This is not accurate, using PSK for authentication provides defense against QCs
(which is described in the following sentence).

How about

        In particular, the authenticity of the SAs established
        under IKEv2 is protected using a pre-shared key or digital signature algorithms.

and note, that at the end of this para the possibility of using
post-quantum digital signature algorithms in IKEv2 is already mentioned
(I guess it was your concern).

> >> ----------------------------------------------------------------------
> >> COMMENT:
> >> ----------------------------------------------------------------------
> 
> (cutting the agreed parts out)
> 
> >> ### Section 2
> >>
> >> I would probably have moved the Design Criteria to a later Section or Appendix,
> >> after the entire protocol specification and Security Considerations. It's nice
> >> to know the background, but this is "optional information" and shouldn't be as
> >> much at the focus at the start of the document. (this is comment, you are fine
> >> to disagree and leave it where it is)
> >
> > I don't disagree with you, but I'd like to hear from my co-authors.
> 
> Ok.

Thanks to CJ, he has already done this.

> >> ### Design Criterea
> >>
> >> One design criteria I do not see mentioned is "limit the extra number of RTTs as
> >> much as possible". I do believe that was an important design criterea ?
> >>
> >> The "future proof" design criterea is probably better named as "not
> >> post-quantum specific" ?
> >
> > Well, I'd rely on my co-authors.
> 
> Ok.

Since no one of my co-authors express disagreement, I'll make this change.

> >> ### IKE_INTERMEDIATE "is encrypted"
> >> ```
> >>    The additional key exchanges are performed using
> >>    IKE_INTERMEDIATE messages; because these messages are encrypted, the
> >>    standard IKE fragmentation mechanism is available.
> >> ```
> >> I think this is confusing. It is not really "because" they are encrypted that
> >> the fragmentation mechanism "is available". Additionally, "encrypted" probably
> >> should clarify the level of encryption - eg it would not me post-quantum safe.
> >> And of course it does not need to be. Mabye something like:
> >>
> >>    The additional key exchanges are performed using IKE_INTERMEDIATE messages
> >>    that follow the IKE_SA_INIT exchange. This is to allow for standard IKE
> >>    fragmentation mechanisms to be available for the potentially large
> >>    post-quantum Key Exchange algorithm payloads. The IKE_SA_INIT exchange does
> >>    not [cannot?] support fragmentation.
> >
> > Then how about:
> >
> >            The additional key exchanges are performed using IKE_INTERMEDIATE messages
> >            that follow the IKE_SA_INIT exchange. This is to allow for standard IKE
> >            fragmentation mechanisms (which cannot be used in IKE_SA_INIT) to be available for the
> potentially large
> >            post-quantum Key Exchange algorithm payloads.
> 
> That works for me.

OK, made this change.

> >> ### Section 3.1 Childless?
> >> ```
> >>      Following that, the IKE_AUTH exchange authenticates peers
> >>      and completes IKE SA establishment.
> >> ```
> >> This made me wonder if it was required to do a Childless IKE SA. I think a
> >> clarification is in order here. Perhaps:
> >>
> >> Following that, the IKE_AUTH exchange comples at normal and authenticates the
> >> peers, completes the IKE SA establishment and when not childless, a Child SA is
> >> also established.
> >
> > I'm not sure this clarification is needed. This section is focused on IKE SA
> > establishment, so if we go into all the details, readers may not follow the main idea.
> 
> Well, when reading I was wondering if this had to be childless, so I
> think it would be good to remove that confusion. But whatever you decide
> is fine. This is just my comment.

The I'd leave the text as is unless my co-authors disagree.

> >> ### Section 3.2.1.  MUST stop SA
> >> ```
> >>    the initiator should log the
> >>    error and MUST stop creating the SA or delete the SA if it is a
> >>    rekey.
> >> ```
> >>
> >> There is ambiguity here on the "delete the SA if it is a rekey". I think you
> >> mean to say to stop creating or delete the SA being negotiated, and not to
> >> delete the SA that was attempted to be rekeyed. How about the simpler:
> >>
> >> the initiator should log the error and MUST abort the exchange with a permanent
> >> error.
> >
> > No, the text is correct (and we suggest to delete the SA that was attempted to be rekeyed).
> 
> Oh, then I do disagree here.
> 
> > The reason - the initiator has received invalid message from the responder.
> > It cannot abort the exchange since the exchange has already completed.
> > This is similar to the situation in IKEv2 when the initiator sends, for example,
> > a single proposal to use AES-GCM, and the responder responds with Chacha-Poly
> > (which was not proposed at all). The only thing the initiator can do in this case -
> > delete the SA.
> 
> The situation is not the same. In your example there is no working SA,
> so you can only delete the partial one. Unless you suggest you would
> also do this in CREATE_CHILD_SA when the algorithms suddenly mismatch.

Exactly.

> I also think that is only a reason to ignore the CREATE_CHILD_SA, and
> not a reason to destroy the existing IKE SA (and thus any existing Child
> SA)

Well, generally invalid response means that there is something wrong
with the peer thus we cannot be sure that it follows spec.

> > I think that the text may be clarified a bit. How about?
> 
> I guess first we need to agree on what to do.....

OK, let me clarify our point.

First, there is nothing specific to the multiple key exchanges here. The situation is that 
the responder sends a message that is inappropriate from the protocol point of view.
There may be many reasons to consider it inappropriate - missing mandatory
payload, incorrect choice of algorithms etc., this specification just adds one
more reason - duplicated algorithms.

What should the initiator do in this situation? It generally cannot send INVALID_SYNTAX,
since RFC 7296 discourages doing so (section 2.21.3):

   Because sending such
   error messages as an INFORMATIONAL exchange might lead to further
   errors that could cause loops, such errors SHOULD NOT be sent.  

The initiator generally cannot continue using this SA, because of possible
the states on the peers are now out of sync (the responder thinks the exchange completed successfully). 
So the safe way is to delete the problem SA. If it is not yet created, then just stop creating it,
otherwise send a Delete payload.

Note, that the text you proposed

    the initiator should log the error and MUST abort the exchange with a permanent error

actually has the same consequence. Since the responder sent a response with no error notify, 
it thinks that the exchange is completed successfully. On the other hand, your text suggests
that the initiator treat this exchange as failed. So, the states on the initiator and the responder
are now out of sync. RFC 7296 states (2.21.3):

   If errors are seen that indicate that the peers do not have the same
   state, it might be good to delete the IKE SA to clean up state and
   start over.

That's what the draft currently requires to do in this situation,
so we don't disagree with each other, the current text in the draft  
just have more details :-)

> >> ### Section 3.2.1 IKE_INTERMEDIATE
> >> ```
> >>     then the corresponding IKE_INTERMEDIATE exchanges MUST NOT take place.
> >> ```
> >> You are already then clarifying this statement, but perhaps to avoid
> >> misimplementing, rewrite this to:
> >>
> >> then the corresponding Additional Key Exchange(s) in the IKE_INTERMEDIATE
> >> exchanges MUST NOT take place. If there is no Additional Key Exchange left to
> >> negotiate, this could mean that there is no more need to perform any
> >> IKE_INTERMEDIATE exchanges. [and remove the following paragraph completely]
> >
> > I'd rather to use your text partially:
> >
> > then the corresponding Additional Key Exchange(s) in the IKE_INTERMEDIATE
> > exchanges MUST NOT take place.
> >
> > This is correct clarification. The rest of your text is not completely correct -
> > there may be needs to perform IKE_INTERMEDIATE for purposes other
> > than additional KE.
> 
> That is what I wrote? If there is no Additional Key Exchange, it could
> mean you don't need to do IKE_INTERMEDIATE? I found it more clear then
> your following paragraph :)
>
> > I'll left the rest of the para as is.
> 
> Maybe reread it one more time, but whatever you decide is fine.

OK.

> >> ### Section 3.2.2
> >> ```
> >>    The other keying materials SK_d, SK_ai, SK_ar, SK_ei,
> >>    SK_er, SK_pi, SK_pr are updated as:
> >>
> >>    [...]
> >> ```
> >> Why not say: The other keying materials SK_d, SK_ai, SK_ar, SK_ei, SK_er,
> >> SK_pi, SK_pr are generated from the SKEYSEED(n) as per RFC 7296.
> >
> > No objection.
> 
> Just to clarify, where I wrote [...] I meant to cover the illustration
> under the text as well. The point of this change was to not repeat a
> stock 7296 exchange (which might confuse implementers in believeing this
> is different from stock 7296)

I'd rather to keep the illustration. There is a subtle difference from RFC 7296 - 
while keys have "generation", the SPIs and nonces are used from the IKE_SA_INIT.
The illustration makes this difference more clear, in my opinion.

> >> ### I did?
> >> ```
> >>     Thanks to Paul Wouters for reviewing the document.
> >> ```
> >> I have no memory of this. Or was this pro-actively added? More serious, I guess
> >> I did review this a long long time ago when the document looked very different
> >> :)
> >
> > Anyway, I think that your thorough review definitely deserves this text to persist :-)
> 
> Thanks :)

You welcome :-)

> Also be aware you wrote "FIPS complaint" not "FIPS compliant". Some
> might agree that FIPS is a complaint though ;-)

Erik has already pointed to that funny typo :-)

> >> ```
> >>     Simply increasing the key length can dwarfed this attack.
> >
> > ????
> 
> My problem is that I don't understand what it means to "dwarf" an
> attack. My brain thinks of throwing Gimli,Dwalin, Balin and Thorin
> into the attack.

:-)

This text has been already changed to 

    Simply increasing the key length can mitigate this attack.

by request from Sean.

> Perhaps you mean "reduce" or "largely counter" ? :-)
> 
> Thanks for the quick response!

No problem, thank you!

The updated PR is available at:
https://github.com/post-quantum/ietf-pq-ikev2/pull/22

Regards,
Valery.

> 
> Paul