Subject: [IPsec] Re: Review of draft-ietf-ipsecme-ikev2-qr-alt
From: Tero Kivinen <[email-masked]>
To: "Valery Smyslov" <[email-masked]>
Cc: [email-masked]
Date: Sat, 27 Jul 2024 00:02:35 +0300
List-Id: Discussion of IPsec protocols <ipsec.ietf.org>

Valery Smyslov writes:
> > > In this case additional checks should be performed to make sure that
> > > only PPK_ID formats with confirmation are used for this extension.
> > > It's easier to check this based on the Notify Type, than on PPK_ID
> > > format. The latter is usually performed much deeper in code, when you
> > > parse the ID. In other words, having its own Notify type allows to
> > > make sanity checks easier.
> > 
> > I do not think that makes that big difference. We currently have 2 PPK ID
> formats
> > defined, and I would assume most implementations uses only one of them...
> 
> I disagree, it does. I prefer notification format to depend on
> notify type than on notification content.

But the format already depends on the ID type. I think we should
simply add new formats for the existing notify, i.e., another ones
with can be used when confirmation is needed.

I think the reason I am unhappy with the current one is that I do not
like the fixed 8-octet stuff at the end, which we can't change without
allocating yet another notification (in case someone would want to
change it to 16 octets, we need to allocate new PPK_IDENTITY_KEY2
notify for it. If there would be length of the PPK_ID and PPK
Confirmation in the beginning of the notify that would allow extending
the notify later, by adding stuff in the end if needed, and modifying
the size of the PPK Confirmation, so if we are not really defining new
format, but are just concatining two things to together, we could do
that by adding new PPK ID types which include PPK Confirmation inside. 

> > On the other hand I think having PPK ID format that includes confirmation
> would
> > benefit the old PPK method too. And when you are calling the function to
> verify that
> > PPK ID passes confirmation it would always fail for those formats which do
> not
> > have confirmaton, thus that would automatically check that only ones that
> have
> > confirmation are accepted.
> 
> It is not that simple. Actually, your proposal complicates code and
> adds potential vulnerabilities. Note, that the current draft uses
> both PPK_IDENTITY_KEY (with confirmation) in request and
> PPK_IDENTITY (w/o confirmation, the same as in RFC 8784) in
> response. All PPK_ID types (currently defined and future) are
> eligible to appear in both notifications. The reason two
> notification are used is that only responder checks the
> confirmation. The initiator proposes PPK and provides the
> confirmation, the responder checks whether it is correct. There is
> no need for initiator to check it again, thus the confirmation is
> omitted in response.

Yes, actually it would be better if the PPK_IDENTIFY notify would have
included bit more of than just one octet of the type, then we could
have taken another byte for the confirmation length and have that as
zero if it is not needed.

Anyways this is minor point, I am just bit concerned of the fixed 8
octet stuff we have there... They have a habit of causing problems.

> If it were included in the response then we'd have a bunch of
> problems: the initiator have either check it again (for what?) or
> ignore.

I would say it would be fatal protocol error if such confirmation is
used by responder when responding to initiator. Just adding text you
MUST NOT use confirmation versions of the IDs when indicating PPK from
responder to initiator. Or just ignore, what the initiator will do now
if the responder decides to use PPK_IDENTIFY_KEY in addition to
PPK_IDENTITY... And if responder does not use PPK_IDENTITY but uses
PPK_IDENTIFY_KEY I assume that is fatal error now already?

> In the former case we have to handle situations when the
> confirmation is incorrect (that actually is difficult to be handled
> gracefully, since this is probably a broken responder implementation
> in which case the initiator should not even inform the responder
> about the problem, making it difficult to debug if that happens). In
> the latter case we have a perfect covert channel (data that is
> transmitted, but never used in the protocol), which I want to avoid.
> For these reason the response doesn't include confirmation and uses
> different notification type.

IKEv2 has so many covert channels inside the IKE SA, that this is does
not even count. Just use Status Notify payloads, vendor id payloads,
or CERT or CERTREQ payloads etc. 

> > > the result is treated as new SKEYSEED.
> > >
> > > SKEYSEED' = prf+ (PPK, SK_d)
> > >
> > > This is the most straightforward and conservative use of prf (with
> > > uniformly random PPK as a key) and it also had some cryptographers
> > > checking (as co-authors of RFC 8784) this approach.
> > 
> > But you are now doing IKE_INTERMEDIATE, thus using the same method for
> > mixing entropy to the SKEYSEED as other protocols using the same exchange
> > would be good.
> 
> IKE_INTERMEDIATE (RFC 9242) doesn't define how additional keys are
> stirred into session keys computation - it is agnostic to this key
> derivation. It is Multiple Key Exchanges (RFC 9370) which does. Both
> this specification and RFC 9370 use IKE_INTERMEDIATE, but they are
> independent from each other and I see no reason why they have to use
> the same method for key derivation. I can even imagine that some new
> specification appears in the future that will rely on
> IKE_INTERMEDIATE and specify another method (more secure or more
> efficient) for session keys calculation.

If you are not going to use what we agreed on the mutliple key
exchanges rfc, then you need to justify why you think what you are
doing is better, and make sure it is secure.

We did have some discussion on the way multiple key exchanges rfc
mixes the keys in, and I remember we changed it because the one that
is currently defined there was better in some way. 

> > Btw, you still need to explain what happens if you do multiple key
> > exchanges and this in the same IKE_INTERMEDIATE exchange, i.e.,
> > currently you throw away the
> 
> Yes, this is a perfectly valid situation. I routinely test this case in my
> testbed.
> 
> > SKEYSEED generated by multiple key exchanegs, as you do:
> > 
> >     SKEYSEED(n) = prf(SK_d(n-1), SK(n) | Ni | Nr)
> > 
> > and
> > 
> >     SKEYSEED' = prf+ (PPK, SK_d)
> > 
> >     {SK_d | SK_ai | SK_ar | SK_ei | SK_er | SK_pi | SK_pr}
> >                                  = prf+ (SKEYSEED', Ni | Nr | SPIi |
> >                                  SPIr )
> > 
> > and the 2nd SKEYSEED' does not use the SKEYSEED generated before.
> 
> No, this is incorrect interpretation.
> 
> The draft says:
> 
>    Once the PPK is negotiated in the last IKE_INTERMEDIATE exchange, the
>    IKE SA keys are recalculated.  Note that if the IKE SA keys are also
>    recalculated as the result of the other actions performed in the
>    IKE_INTERMEDIATE exchange (for example, as defined in [RFC9370]),
>    then applying PPK MUST be done after all of them, so that
>    recalculating IKE SA keys with PPK is the last action before they are
>    used in the IKE_AUTH exchange.

That is what I did, I did calculate the SKEYSEED, and then calculated
the SKEYSEED' again. I did not derive encryption and authentication
keys are they were not used. 

> So, the correct sequence is:
> 
> 1) complete what RFC 9370 requires
> 
>      SKEYSEED(n) = prf(SK_d(n-1), SK(n) | Ni | Nr)
>      {SK_d(n) | SK_ai(n) | SK_ar (n)| SK_ei(n) | SK_er(n) | SK_pi(n) |
> SK_pr(n)}  = prf+ (SKEYSEED(n), Ni | Nr | SPIi | SPIr )
> 
> (actually, you only need SK_d(n) here, so if you are smart enough skip
> computation of all other SK_*)

Which is not explained in the draft. 

> 
> 2) apply PPK to the result
> 
>      SKEYSEED' = prf+ (PPK, SK_d(n))
>      {SK_d | SK_ai | SK_ar | SK_ei | SK_er | SK_pi | SK_pr}  = prf+
> (SKEYSEED', Ni | Nr | SPIi | SPIr )

But the current draft says you use SK_d, it does say SK_d(n), which
would have been clear. 

> > If you defined the operation as:
> > 
> >     SKEYSEED(n) = prf(SK_d(n-1), PPK | [ SK(n) ] | Ni | Nr)
> > 
> > where SK(n) is left out if no other key exhcnage was done in this
> > IKE_INTERMEDIATE exchange, then that would solve that problem, and
> > if we trust this method for post quantum key exchange algorithms,
> > we should trust the method also for PPK...
> 
> See above for the rationale.

I did not see any retionale why this document does the key mixing
differently than the multiple key exchanges rfc. If the multiple key
exchanges RFCs has problems in its key mixing that this method fixes,
then we need to start fixing that too. 

> In addition the proposed use of PPK is more challenging for some
> reasons. In particular, standard use of prf assumes that the first
> argument is a random (usually secret) key and the second argument is
> an arbitrary (usually public) data. When you apply secret key as a
> public argument, this may be challenging in some cryptographic
> libraries and sometimes requires hacks (well, I know that IKEv2 uses
> this inversion when SKEYSEED is being initially calculated and we
> need to handle this, but for the very good reason - you know that
> nonces are random and the result of DH computation - not, so you
> _have_ to swap them to meet the cryptographic requirements the
> standard prf requires for its arguments).

But as you mentioned IKEv2 already does that in lots of cases. We do
it that way in the initial SKEYSEED calculation where we uses nonces
as key, and diffie-hellman secret as data.

And that is not the case here the SK_d(n-1) is already suitable for
the key, it is not like nonces etc. We already use PRF+(SK_d, ...)
contruct for lots of things in the IKEv2. 

> I don't think it updates RFC 8784, since it doesn't affect
> operations described there (unless you think that the ability to
> make additional use of PPKs not mentioned there is qualified as
> "update"). So, I rely on the chairs whether this draft should be an
> update for RFC 8784.

I think it is extension to the PPK in IKE_AUTH RFC, as it defines the
PPK in CREATE_CHILD_EXCHANGE that can be used by PPK in IKE_AUTH
implementations too. So the PPK in IKE_AUTH implementators should
really read this document too and decide whether they want to
implement that not.

> In this case it should be
> 
> Mixing Preshared Keys in the IKE_INTERMEDIATE and in the CREATE_CHILD_SA
> Exchanges of IKEv2 for Post-quantum Security

True.

> (isn't it too long?)

Not really, but you might end up requiring to expand IKEv2 in title
too...

> And I also changed a short title to "Enhanced Use of PPKs in IKEv2".
> And since "alternative approach" is no more used in the text of the
> document, it makes sense to rename the notify:
> s/USE_PPK_ALT/USE_PPK_INT

Yes.

> I used the proposed text with some modifications:
> 
>    1.  The main advantage of using PPK in the IKE_INTERMEDIATE exchange
>        instead of the IKE_AUTH exchange is that it allows IKE_AUTH to be
>        fully protected.  This means that the ID payloads and any other
>        sensitive content sent in the IKE_AUTH are protected against
>        quantum computers.  The prominent example of situation when
>        cryptographic keys are transferred in the modified IKE_AUTH
>        exchange (called GSA_AUTH) of G-IKEv2 [I-D.ietf-ipsecme-g-ikev2].
> 
>    2.  In addition to the IKE_AUTH exchange being fully protected, the
>        initial IKE SA is also fully protected, which is important when
>        sensitive information, e.g. cryptographic keys, is transferred
>        over initial IKE SA.  The prominent example of such situation is
>        the GSA_REGISTRATION exchange of G-IKEv2 [I-D.ietf-ipsecme-g-ikev2].
> 
>    3.  As the PPK exchange happens as separate exchange before IKE_AUTH
>        this means that initiator can propose several PPKs and responder
>        can pick one.  This is not possible when PPK exchange happens in
>        the IKE_AUTH.  This feature could simplify PPK rollover.
> 
>    4.  With this specification there is no need for the initiator to
>        calculate the content of the AUTH payload twice (with and without
>        PPK) to support a situation when using PPK is optional for both
>        sides.

Looks good. It is funny how much those bullets mention the g-ikev2
which is not even an RFC yet :-)

Is this some kind of reminder for the lazy shephards to get writeup
done? 

> I've submitted yet another version. Please check, there are some
> other text changes, that logically followed the proposed ones.

Looks good. 

> Let's see how many iterations we need to converge :-)

I think everthing else is ok with the lates draft, but I really think
we should have good discussion about the actual mixing, i.e, whether
we use the method described in this draft or whether we use the one
defined on he Multiple key exchanges RFC.

This is something we can do during the WGLC. 
-- 
[email-masked]