Re: [IPsec] WGLC for draft-ietf-ipsecme-ikev2-multiple-ke

[Paul Wouters <[e-mail]>]

On Fri, 30 Jul 2021, Valery Smyslov wrote:

[ replying as I got prompted by Tero on this regarding WGLC ]

>> I have reviewed the document. In general I support this document. I
>> really like the idea of renaming the DH Registry to KE. I do think it
>> is not ready yet though. My comments and questions follow below.
>>
>>  	Key exchange methods negotiated via Transform Type 4 MUST always take
>>  	place in the IKE_SA_INIT exchange.  Additional key exchanges
>>  	negotiated via newly defined transforms MUST take place in a series
>>  	of IKE_INTERMEDIATE exchanges, in an order of the values of their
>>  	transform types, so that key exchange negotiated using Transform Type
>>  	n always precedes that of Transform Type n + 1.
>>
>> I don't understand this section, specifically the use of "Transform Type 4"
>> and "Transport Type n+1", as we only have transform type 5 and nothing
>> higher and that is Extended Sequence Number.
>
> A one para above new Transform Types are introduced:
> Additional Key Exchange 1, Additional Key Exchange 2,
> Additional Key Exchange 3, etc. Once added to IANA registry,
> they will get their numbers (hopefully in an order of their names).
> The text is trying to say, that when additional KE are being negotiated,
> the order in which they will take place is defined by the order
> of assigned Transform Types (which will correspond to the order of their names).
>
> In other words, if at the end of negotiation peers decided that
> they will perform base KE (negotiated via Transform Type 4, which this draft
> renames to "Key Exchange Method (KE)") 2048-bit MODP Group,
> and two additional Key Exchanges - SIKE_P434, negotiated in
> "Additional Key Exchange 3" transform, and FRODOKEM_640_AES,
> negotiated in "Additional Key Exchange 5" transform, then
> they agree to use 2048-bit MODP Group in the IKE_SA_INIT,
> then SIKE_P434 in the first IKE_INTERMEDIATE
> followed IKE_SA_INIT and FRODOKEM_640_AES in the next
> IKE_INTERMEDIATE.

I guess this all still feels like a bit of a kludge. I wonder if there
is a way to do this with one new transform type, and an ordered list of
proposals inside. I understand the desire to specify order, but I feel
the current method is fragile and error prone. Eg if one end has
Additional Key Exchange 1 = SIKE_P434, Additional Key Exchange 2 = FRODOKEM_640_AES
and the other end has Additional Key Exchange 1 = FRODOKEM_640_AES, Additional Key Exchange 2 = SIKE_P434,
where both might not really care which goes first or second.

I think it would be worth thinking about this problem a bit more.

> It is implied here (and defined in Section 3.1). But the real message here is that the order of key exchanges
> performed in a sequence of IKE_INTERMEDIATE is defined by the order of "Additional Key Exchange *"
> transforms via which they are negotiated.

Then please say it clearly, instead of implying it.

>>  	Each IKE_INTERMEDIATE exchange MUST bear exactly one key exchange method.
>>
>> I don't understand why there is this limitation. What if some Key
>> Exchange mechanism will require 2 RTTs. Why preventively forbid that?
>
> We didn't consider such exotic key exchange methods and, I think,
> if they appear, they will deserve a separate document.

Sure, but it would be best if such a new document would not need to
Updates: this document just for this one little change. So why not:

- Each IKE_INTERMEDIATE exchange MUST bear exactly one key exchange method.
+ All Additional Key Exchanges MUST be in their own Exchange - that is
+ these KE's cannot be exchanged via a single IKE message exchange.


I also just realised that Addtional Key Exchange is not the best term
for this, as it is very close to Internet Key Exchange and people might
think this is a chain of IKEv2 and something else. Maybe "Additional KE
Exchange" would be better?

> But the real message here is that you cannot perform
> several key exchanges using a single IKE_INTERMEDIATE exchange.
> In other words, you cannot put several public keys
> of different Key Exchange methods in a single IKE_INTERMEDIATE
> message.

I tried to phrase it above, but I think it can be improved upon further.

>> So how does an intiiator convey that it deems an additional Key Exchange
>> to be mandatory?
>
> 	If the initiator includes "Additional Key Exchange N" transform,
> 	then the responder MUST choose a single method from
> 	those included in this transform. If the list of included
> 	methods doesn't contain NONE, then the responder
> 	have to choose one of them, so performing this KE
> 	method is mandatory. To make it optional the initiator
> 	includes NONE among other methods. This allows
> 	the responder to select NONE and thus to skip doing
> 	this additional KE.

That makes it clearer, but. If each Additional Key Exchange carries a
None, does that mean it these Exchanges can be fully omitted and we are
only doing DH/KE from IKE_SA_INIT ? What if the Additional Key Exchange
1 contains None, and there is an Additional Key Exchange 2 ? Does it
mean that everything can still be skipped ?

I'm still not a big fan of the specification this way. I wish we could
find a way to do this in one new transform type and somehow enforcing
an ordered list.



>>  	If the initiator includes any transform of type n (where
>>  	n is among Additional Key Exchanges) in the proposal, the responder
>>  	MUST select one of the algorithms proposed using this type.  A
>>  	transform ID NONE may be added to those transform types which contain
>>  	key exchange methods that the initiator believes are optional.
>>
>> And so I again do not understand this. What is "n" here? a new transform
>> type ? ( eg n=6 ??)  or a new entry in the Transform Type 4 Key Exchange
>> registry?
>
> 	n here is new Transform Type. We define "Additional Key Exchange 1",
> 	"Additional Key Exchange 2", "Additional Key Exchange 3" ...
> 	up to "Additional Key Exchange 7".

Understood.

>> At his point, the Additional Key Exchange is introduced, and I am
>> beginning to understand things. This should really be explained before
>> the text I pointed at above to make any sense to the reader. And see
>> below on placing "Additional Key Exchanges" into the "Key Exchanges"
>> Registry.
>
> Actually, the new transforms are introduced in 3.2.1, but I see your point
> and probably we should add more clarifications before this section.

Please please please include some asci art of this exchange in
IKE_SA_INIT. I ensure you that will make it much more easy for a new
reader to understand this.

> (I also noted that Additional Key Exchange transforms are mentioned at the end of
> 3.1 before their formal introduction, that must be fixed).
>
>> The next part explains the CREATE_CHILD_SA and IKE_FOLLOWUP_KE exchanges. I
>> personally would prefer that a different exchange than CREATE_CHILD_SA
>> is used if the completion of such an exchange does not lead to a fully
>> rekeyed state. This use of completing a CREATE_CHILD_SA and being in a
>> state that is not "rekeyed" or "failed" complicates the state machine.
>
> That's true. However, there is a tradeoff here. If you defined a new two-message exchange,
> that performed multiple key exchange methods at once, then
> the initiator would include public keys for all KE methods it proposed
> which is terrible for performance reasons, since the responder may
> reject most of them. The message size is also would grow dramatically,
> and in case the responder selected a tiny subset of what was proposed,
> this would be extremely inefficient.

I'm not convinced this is a valid reason to break open the clear state
of CREATE_CHILD_SA and a REKEYed IKE SA. This is really unpleasant for
our state machine.

>>  	The data associated with this notification is a blob meaningful only to the responder
>>
>> Why a blob? Why not the imminent new SPI it generated for this new IKE SA?
>> If you really want a blob, there should be an example of how to generate
>> the blobs. I don't see any such guidance in the document.
>
> The purpose of this data blob is to allow the responder to link
> CREATE_CHILD_SA and subsequent IKE_FOLLOWUP_KE between
> themselves in case the initiator creates several Child SAs
> (or rekeys them, or rekeys IKE SA) simultaneously.

Again, then why not simply use the MSGID of the corresponding CREATE_CHILD_SA ?

>> Below is an example of three additional key exchanges.
>>
>>     Initiator                             Responder
>>     ---------------------------------------------------------------------
>>     HDR(CREATE_CHILD_SA), SK {SA, Ni, KEi} -->
>>                               <--  HDR(CREATE_CHILD_SA), SK {SA, Nr, KEr,
>>                                        N(ADDITIONAL_KEY_EXCHANGE)(link1)}
>>
>>
>> Why does the initiator not start out with a N(ADDITIONAL_KEY_EXCHANGE) ?
>> There has been an Additional Key Exchange in the initial exchanges, so
>> why not start out with one in the rekey from the initiator?
>
> Because N(ADDITIONAL_KEY_EXCHANGE)(data blob) is for the responder
> to allow it to properly link IKE_FOLLOWUP_KE with this particular CREATE_CHILD_SA.

Understood (but see above)

>> It has given no indication it might be mandatory from the initiator's point of view.
>
> You are right, this must be clarified in the draft.

Okay.

>>  	It is possible that due to some unexpected events (e.g. reboot) the
>>  	initiator could forget that it is in the process of performing
>>  	additional key exchanges and never starts next IKE_FOLLOWUP_KE
>>  	exchanges.  The responder MUST handle this situation gracefully
>>
>> And wouldn't that solve this weird state issue if the initiator already
>> signalled this clearly in CREATE_CHILD_SA, so the responder could in
>> that case already return an error in the CREATE_CHILD_SA exchange?
>
> Sorry, I didn't follow your idea. Can you elaborate?

I'm lost here too now. But I think however something workds if it saves
state and reboots should not affect the protocol design?

>> Note that I find the argument weird, why would an IKE peer forget some
>> of its state after a reboot? Both peers should always remember AKE's
>> were used and have to be used again upon (PFS) rekeys.
>
> It's true, but in this case the initiator forgets that it started CREATE_CHILD_SA
> (with AKEs) before reboot. In this case it never continues this sequence
> after rebooting and restoring last stable IKE SA state.

Well yes. If the initiator is broken, the connection might fail. I don't
think we need to guard against that.

>>  	it MUST send back a new error type notification STATE_NOT_FOUND.
>>  	This is a non-fatal error notification
>>  	[...]
>>  	If the initiator receives this notification in
>>  	response to IKE_FOLLOWUP_KE exchange performing additional key
>>  	exchange, it MUST cancel this exchange and MUST treat the whole
>>  	series of exchanges started from the CREATE_CHILD_SA exchange as
>>  	failed.
>>
>> So why use a non-fatal error notification that leads to a guaranteed failure ?
>
> Because the failure may be recoverable - just start new CREATE_CHILD_SA
> with AKEs. I see no need to delete IKE SA in this case.
> Of course, if the failure persisted after several attempts, then
> the only thing peer can do - delete SA.

I think this just complicates things too much. We shouldn't have
confused peers or forgetful peers. Just hard fail when something
unexpected happens. assert() for the win :P

>> Note there seems to be a notion of AKE's having continuous state, as
>> opposed to (EC)DH where you start a new state with the rekey exchange.
>> Perhaps this should be clarified at the beginning of the document?
>
> Can you please provide text candidate?

I can try.

>>   This document adds the following Transform Types to the "Transform
>>     Type Values" registry:
>>
>>     Type     Description                   Used In
>>     -----------------------------------------------------------------
>>     <TBA>    Additional Key Exchange 1     (optional in IKE, AH, ESP)
>>     <TBA>    Additional Key Exchange 2     (optional in IKE, AH, ESP)
>>     <TBA>    Additional Key Exchange 3     (optional in IKE, AH, ESP)
>>     <TBA>    Additional Key Exchange 4     (optional in IKE, AH, ESP)
>>
>>
>> Why are the descriptions referring to "additional" key exchange? I would
>> assume the registry is just a list of Key Exchange types, and whether
>> one of these is "additional" or not depends on its use in IKE ? That is,
>> one of the Key Exchanges that today is "additional" might one day be
>> used as the only Key Exchange without Additional Key Exchanges? If we
>> really are making some of these exchanges as "additional use only", then
>> we should really create a new transform type registry for AKE that is
>> separate from the Key Exchange Type (4).
>
> Additional here means that KE methods negotiated via these transforms
> are always performed in addition to KE method, negotiated via Transform Type 4.

so this was again me being confused about the entire mechanism of the
draft. This is why I strongly urge you to add some ascii art about how
these payloads flow (as part of the SA payload)

>>  	the key lengths of these
>>  	transforms SHALL be at least 256 bits long in order to provide
>>  	sufficient resistance to quantum attacks.
>>
>> I would use MUST instead of SHALL.
>
> OK.
>
>>  	The main focus of this document is to prevent a passive attacker
>>  	performing a "harvest and decrypt" attack.  In other words, an
>>  	attacker that records messages exchanges today and proceeds to
>>  	decrypt them once he owns a quantum computer.  This attack is
>>  	prevented due to the hybrid nature of the key exchange.  Other
>>  	attacks involving an active attacker using a quantum-computer are not
>>  	completely solved by this document.  This is for two reasons.
>>
>> I think this part and the text right underneath this belongs in the
>> Introduction more than it belongs in the Security Considerations
>> section.
>
> This probably makes sence. I'll discuss this with my co-authors.

Okay.

>>  	Unfortunately, this design is susceptible to the following
>>  	downgrade attack.
>>
>> I'm not convinced this is an issue actually. If you really do think a
>> proper configuration would not sufficiently protect against this, the
>> initiator could send a notify in the second IKE_SA_INIT of the rejected
>> KE value from the previous response, so that this attack would be
>> detected.
>
> I won't comment on this - this is just an appendix with early
> design alternatives. I'll let my co-authors to comment on this.

Okay.

>>  	We discarded this approach because we believe that the working
>>  	group may not be happy using the RESERVED field to change the
>>  	format of a packet and that implementers may not like the
>>  	complexity added from checking the fragmentation flag in each
>>  	received payload.
>>
>> I think that is exactly why we have RESERVED fields. As implementer, I
>> don't think that this would have been too complex. But I think using
>> INTERMEDIATE is fine, especially if multiple solutions can use this
>> method for their own usage, and having a generic method is better than
>> having specific methods per postquantum/hybrid solution.
>
> OK, we are in agreement :-)

While we might be in agreement, we are in disagreement on why the other
approach was discarded :) So the real question is, should it be
considered?

I do think that this document needs some more work to clarify things,
and I would really like to see more people reviewing this document as
well.

I would also like to see a discusion of a "simpler" design that would
not use 4 Transform Type's, but only 1, and see if that would be
"simpler" (simpler in quotes twice because i can't evaluate yet
without such discussion which would be the simpler solution.

Paul