While doing my IANA review I found out some comments that might need
fixing in the draft-kampanakis-ml-kem-ikev2-08.

First of all add actual RFC title when refering to them.

It is much more reader friendly to say

    To address this concern, Mixing Preshared Keys in IKEv2 [RFC8784]
    introduced ...

instead of 

    To address this concern, [RFC8784] introduced ...

Do this throughout of the draft, and also include titles for FIPS
documents too.

--

In section 1.1 it would be more useful to explain what is transferred
between Initiator and Responder and where each of the operation is
done, i.e., Figure 1 for FIPS203 explains things properly, and I think
the list here should include bit similar overview text.

I.e., change to:

   *  Initiator: 'KeyGen() -> (pk, sk)': A probabilistic key generation
      algorithm, which generates a public / encapsulation key 'pk' and
      a private / decapsulation key 'sk'. The resulting pk is sent to
      the responder in the KEi payload.

   *  Responder: 'Encaps(pk) -> (ct, ss)': A probabilistic
      encapsulation algorithm, which takes as input a public key 'pk'
      (from the KEi) and outputs a ciphertext 'ct' and shared secret
      'ss'. The ct is sent back to intiator in the KEr payload.

   *  Initiator: 'Decaps(sk, ct) -> ss': A decapsulation algorithm,
      which takes as input a secret key 'sk' and ciphertext 'ct' (from
      the KEr) and outputs a shared secret 'ss', or in some cases a
      distinguished error value.

This will make it much clearer fo the reader what operation is run
where, and what is sent inside the KE payloads. The section 2.1 tries
to explain this but is quite confusing...

I am not sure whether the last paragraph of the section 1.1 really
belongs there. It explains the properties of the KEM, but this is not
something that IKEv2 implementors need to know for KEM overview
section, and belongs more to the security considerations section than
here. And some of it is already explained in the security
considerations section in situations where it affect implementations.

Perhaps remove the last paragraph and move some of to security
considerations section. 

--

Section 2.1 summary of RFC9370 is quite confusing. Especially the
partabout what is transferred in KEi(1) and KEr(1). Perhaps rewrite

                  KEi(1) and KEr(1) are the subsequent key
   exchange messages which carry the ML-KEM public key of a keypair (sk,
   pk) generated by the initiator with ML-KEM KeyGen() and the 256-bit
   ML-KEM shared secret SK(1) encapsulated by the responder to a
   ciphertext ct by using Encaps(pk) respectively.  The initiator then
   decapsulates the 256-bit ML-KEM shared secret SK(1) from the
   ciphertext ct by using its private key sk in Decaps(sk, ct).  Both
   peers have now reached a common SK(1) at the end of this KE(1) key
   exchange.

to

   The initiator will generate ML-KEM keypair (sk, pk) using KeyGen(),
   and sends the public key (pk) to the responder inside the KEi(1)
   payload. The responder will encapsulate shared secret using
   Encaps(pk) and resulting ciphertext (ct) is sent to initiator using
   the KEr(1). When initiator receives KEr(1) it will decapsulate it
   using Decaps(sk, ct). Both Encaps and Decaps return the shared
   secret (ss) and now both peers have common shared secret SK(1).

--

Section 2.2 first paragraph is misleadin and/or wrong. There is no
point of repeating things from the base IKEv2 specification etc. For
example the Next Payload value of the IKE header will not be 34 (Key
Exchange), it will be 46 Encrypted and authenticated etc...

So just remove the first paragraph competely:

   HDR, the IKE header, of the IKE_INTERMEDIATE messages carrying the
   ML-KEM key exchange has a Next Payload value of 34 (Key Exchange),
   Exchange Type of 43 (IKE_INTERMEDIATE) and Message ID of 1 assuming
   this is the first additional key exchange (ADDKE1).  For
   IKE_FOLLOWUP_KE messages carrying the ML-KEM key exchange, the
   Exchange Type would be 44 (IKE_FOLLOWUP_KE).

Then change

   The IKE_INTERMEDIATE or IKE_FOLLOWUP_KE payload is shown below as
   defined in Section 3.4 of [RFC7296]:

to:

   The KE payload is shown below and the fields inside it has meaning
   as defined in Section 3.4 of [RFC7296]:

But then you are actually missing what is stored in the KE payload,
the next paragraph does not really specify what is in it, so change:

   Table 1 shows the Payload Length, Key Exchange Method Num identifier
   and the Key Exchange Data Size in Octets for Key Exchange Payloads
   from the initiator and the responder for the ML-KEM variants specific
   in this document.  The public key and the ciphertext in the Key
   Exchange Data are encoded as raw byte arrays as defined in [FIPS203].


to

   The Key Exchange Data from initiator to responder contains the
   public key (pk) from the KeyGen() operation encoded as raw byte
   array (i.e., output of ByteEncode) as defined in section 7.1 of
   Module-Lattice-Based KEM standard [FIPS203).

   The Key Exchange Data from responder to initiator contains the
   ciphertext (ct) from the Encaps operation encoded as raw byte
   array. 

   Table 1 shows the Payload Length, Key Exchange Method Num identifier
   and the Key Exchange Data Size in Octets for Key Exchange Payloads
   from the initiator and the responder for the ML-KEM variants specific
   in this document.  

--

In section 2.3 the text explaining error cases should be separated to
initiator and responder cases. The responder can send INVALID_SYNTAX
notification back if encapsulation key check descripbed in the section
7.2 of Module-Lattice-Based KEM standard [FIPS203), but initiator
can't do that as error happens when we process response. What needs to
be done is similar text than we have in 2.21.2 about
AUTHENTICATION_FAILED.

So section 2.3 should change from:

   Receiving and handling of malformed ML-KEM public key or ciphertext
   SHOULD follow the input validation described in [FIPS203].  In
   particular, entities receiving the ML-KEM public key to encapsulate
   to SHOULD perform the checks in Section 7.1 of [FIPS203] and reject
   the ML-KEM public key, if malformed.  Entities receiving an ML-KEM
   ciphertext for decapsulation SHOULD perform the ciphertext checks in
   Section 7.2 of [FIPS203] and reject the ciphertext, if malformed.
   These checks could be performed separately before performing the
   encapsulation or decapsulation steps or be part of them.  The
   response message to an IKE_SA_INIT, or IKE_INTERMEDIATE, or
   IKE_FOLLOWUP_KE with malformed ML-KEM public keys or ciphertext MUST
   be a Notify payload of type INVALID_SYNTAX.

to:

   Receiving and handling of malformed ML-KEM public key or ciphertext
   SHOULD follow the input validation described in
   Module-Lattice-Based KEM standard [FIPS203].

   Responder SHOULD perform the checks specified in section 7.2 of
   Module-Lattice-Based KEM standard [FIPS203] before doing Encaps(pk)
   operation. If the checks fail it SHOULD send Notify payload of type
   INVALID_SYNTAX as a response to the request from initiator.

   Initiator SHOULD perform the checks specified in section 7.3 of
   Module-Lattice-Based KEM standard [FIPS203] before doing Decaps(sk,
   ct) operation. If the checks fail the initiator MUST reject the
   ciphertext and MUST fail the exchaneg. In this case the initiator
   MAY send Notify payload of type INVALID_SYNTAX to responder as a
   separate INFORMATIONAL exchange, usually with no other payloads.
   This is an exception for the general rule of not starting new
   exchanges based on errors in responses.

--

In the security considerations section there is text saying:

       Alternatively, the PRF combiner that derives the PQ/T
   Hybrid SKEYSEED and KEYMAT could be updated to include the
   ciphertexts which could be proven IND-CCA2 secure following similar
   methodology to [PQ-PROOF2]. 

Is this text talking about possible future change to the IKEv2 to make
it IND-CCA2 or what? If this is talking about future change, I think
we should remove that text, as it does not belong here, but in the
document that will make that change. If this is something than
actually affects current implementations then it can stay here.

--

This document is missing test vectors it would be really good if test
vectors for each of the KEMs could be added. I.e. something that
has hex printouts of the pk, sk, ct, and ss, and then the KEi and KEr
payloads, and resulting SK == ss.

I think some of the IKEv2 implementations already have test code for
ML-KEM, so perhaps someone having such implementation could generate
those outputs. This would at least allow people to test their pre
encapsulation and decapsulation tests.

--

None of the above is somehing that would block getting IANA numbers
to be allocated for this, thus I will send OK to IANA for allocations
now (Valery OKed it already, but wanted to have my input too). 

-- 
kivi...@iki.fi

_______________________________________________
IPsec mailing list -- ipsec@ietf.org
To unsubscribe send an email to ipsec-le...@ietf.org

Reply via email to