Add ML-KEM composite KEM

[falko-strenzke]

Integration of ML-KEM composites according to draft-ietf-lamps-pq-composite-kem

Already solved issues

The following problem arises in the context of the MLKEM-composite implementation.
It is based on the inconsistency between these two functions defined in pk_keys.h:

      virtual std::unique_ptr<PK_Ops::Encryption> create_encryption_op(RandomNumberGenerator& rng,
                                                                       std::string_view params,
                                                                       std::string_view provider) const;

      virtual std::unique_ptr<PK_Ops::KEM_Encryption> create_kem_encryption_op(std::string_view params,
                                                                               std::string_view provider) const;

The problem is that the implementation of create_kem_encryption_op in MLKEM_Composite_PublicKey requires an RNG to internally call create_encryption_op() for RSA.
Thus inside the constructor of the MLKEM_Composite_Encapsulation_Operation, an RNG is needed to instantiate the traditional encryption operation.

It is not apparent to me why create_encryption_op() should require an RNG in the first place – and I didn't investigate. But generally, contrary to a decryption OP, it should not require randomization in setting up precomputations due to a side channel countermeasure.

The only reasonable solution that I came to is to add an optional RNG argument to the interface:

      virtual std::unique_ptr<PK_Ops::KEM_Encryption> create_kem_encryption_op(
         std::string_view params, std::string_view provider, RandomNumberGenerator* rng_may_be_null = nullptr) const;

This also requires a new function in the FFI interface. I am quite certain that this exact approach to the problem will not find acceptance. It's purpose is to make things work for now and provide a basis for the discussion of the proper solution.

The only alternative I could think of would be to let the constructor of MLKEM_Composite_Encapsulation_Operation to store the encoded public key in the instance and create the RSA encryption operation during every KEM encapsulation. This would be a both a strain on memory and performance, thus I discarded this idea.

[randombit]

It is not apparent to me why create_encryption_op() should require an RNG in the first place – and I didn't investigate. But generally, contrary to a decryption OP, it should not require randomization in setting up precomputations due to a side channel countermeasure.

In principle blinding during encryption might be useful, and possibly that's why the argument is there. TBH I don't recall. It does seem to be the case that currently no implementation uses it, and since the encryption operation itself is provided an RNG any necessary blinding setup could just be deferred to the first invocation. Likely this argument could be removed entirely.

I would suggest rather than modifying the KEM interface in this way, take advantage of the fact that RSA's create_encryption_op ignores the RNG object anyway, pass a Null_RNG, catch PRNG_Unseeded and convert it into an Internal_Error.

[falko-strenzke]

I would suggest rather than modifying the KEM interface in this way, take advantage of the fact that RSA's create_encryption_op ignores the RNG object anyway, pass a Null_RNG, catch PRNG_Unseeded and convert it into an Internal_Error.

Absolutely makes sense, I applied that idea.

[falko-strenzke]

There are still CI failures apparently stemming from current master branch.

[reneme]

Not a full review. Just some up-front comments.

[reneme]

Sorry for the fairly shallow high-level comments again. I hope I'll have more time to do a proper review in the next few days.

[reneme]

@falko-strenzke On a general note: We should look into consolidating support for this IETF draft with the existing facilities in the pubkey/hybrid_kem and pubkey/kex_to_kem_adapter modules which are currently used in the TLS 1.3 PQC key exchange. Those provide base classes for hybrid KEMs already, and we shouldn't duplicate this logic here.

Following this idea, your MLKEM-Composite implementation should derive from Hybrid_PublicKey and Hybrid_PrivateKey. Translation from the ECDH/X25519/X448 algorithms to a KEM interface can be done conveniently using the KEX_to_KEM_Adapter. That way you should be able to avoid all dynamic casts to concrete ECDH public key classes such as ECDH_PublicKey, or X25519_PublicKey. This doesn't cover RSA-OAEP, but a similar adapter construction could be built for this.

The usage of generic adapters (for RSA and for ECDH) allow the composition to be implemented exclusively based on KEMs, without explicit handling of specific traditional algorithms. Also, such an implementation could be easily extensible to other KEMs in the future (e.g. FrodoKEM or HQC).

The existing hybrid_kem module is an internal interface and can thus be adapted as needed to create a decent foundation for both TLS's needs as well as this draft's implementation. Most notably, at the moment it is designed to be super generic by allowing arbitrary combinations of one-to-many PQ and traditional KEMs (not just specifically 1xPQ and 1xTraditional). In retrospect, this was probably a design mistake that we should fix.

I would suggest a design like so:

excalidraw.com

I could take over the relevant work in the mentioned base classes and the TLS hybrid implementation.

[FAlbertDev]

@falko-strenzke
One feature that would be very interesting for our use case is the ability to use a custom implementation of the underlying algorithms. For example, we plan to use Composite ML-KEM in combination with smart cards where the ML-KEM decapsulation and the ECDH operation are performed by the smart card. For that, we would use underlying keys of type PKCS11_EC_PrivateKey and (not yet implemented, but something like) PKCS11_ML_KEM_PrivateKey.

Therefore, it would be really nice if the interface of the composite ML-KEM private key would allow the processing of such "custom" private keys. I could imagine adding an additional constructor like:

MLKEM_Composite_PrivateKey(MLKEM_Composite_Param::id_t id, std::unique_ptr<Botan::PrivateKey> mlkem_privkey, std::unique_ptr<Botan::PrivateKey> traditional_privkey);

The information, currently obtained via dynamic casting the private key pointers, must be derived from the MLKEM_Composite_Param in this case. What do you think?

[reneme]

@falko-strenzke See #5715 please. I'm currently working on a quick'n'dirty retrofit of your work on this branch onto the changes in #5715 to see if it provides the required flexibility to accommodate this implementation of the LAMPS draft. I'll share it with you once I have something working.

[reneme]

@falko-strenzke I took the liberty and refactored your patch (mostly mlkem_comp.cpp) based on #5715 and #5717. See here: https://github.com/reneme/botan/tree/falko/mlkem-composite (squashed to one commit and based on #5715).

This also addresses plenty of the suggestions I added above. But most notably, it gets rid of almost all low-level algorithm-specific handling by reusing existing concepts.

I'd appreciate if you could have a read-through my refactorings and pull them into this PR.

[falko-strenzke]

@falko-strenzke I took the liberty and refactored your patch (mostly mlkem_comp.cpp) based on #5715 and #5717. See here: https://github.com/reneme/botan/tree/falko/mlkem-composite (squashed to one commit and based on #5715).

This also addresses plenty of the suggestions I added above. But most notably, it gets rid of almost all low-level algorithm-specific handling by reusing existing concepts.

I'd appreciate if you could have a read-through my refactorings and pull them into this PR.

@reneme As I wrote in #5715, I am not entirely convinced of the multi-algorithm framework that you rebased the implementation onto. As a plus, it reduces the code size in mlkem_comp.cpp by one third, on the negative side, the trivial combination of two KEMs becomes somewhat harder to read due to a concept that doesn't actually bring much functionaility. In any case, I am fine with your proposal, but I would like to hear from @randombit first before pulling it in. The decision to use the multi-algorithm framework throughout the library in long term seems to be fundamental one.

[falko-strenzke]

@falko-strenzke One feature that would be very interesting for our use case is the ability to use a custom implementation of the underlying algorithms. For example, we plan to use Composite ML-KEM in combination with smart cards where the ML-KEM decapsulation and the ECDH operation are performed by the smart card. For that, we would use underlying keys of type PKCS11_EC_PrivateKey and (not yet implemented, but something like) PKCS11_ML_KEM_PrivateKey.

Therefore, it would be really nice if the interface of the composite ML-KEM private key would allow the processing of such "custom" private keys. I could imagine adding an additional constructor like:

MLKEM_Composite_PrivateKey(MLKEM_Composite_Param::id_t id, std::unique_ptr<Botan::PrivateKey> mlkem_privkey, std::unique_ptr<Botan::PrivateKey> traditional_privkey);

The information, currently obtained via dynamic casting the private key pointers, must be derived from the MLKEM_Composite_Param in this case. What do you think?

Well, with @reneme 's new proposal, I think that would or should be taken care of by the hybridization framework. @reneme Is that already the case?

[reneme]

Is that already the case?

Pretty much. There are two central things to that:

1. Have a low-level constructor that takes any keypair objects from the user,
2. Refrain from any assumptions of the concrete key types (i.e. don't use dynamic_cast<RSA_PublicKey> for instance), because for a custom implementation that won't work.

The second point might need some work. The first point should be taken care of already.

[sehlen-bsi]

For the RSA composites, there seems to be the same issue as mentioned for #5451 that the RSA key sizes are not bound to the modulus size specified by the composite OID. I might be wrong here, please check!

[falko-strenzke]

For the RSA composites, there seems to be the same issue as mentioned for #5451 that the RSA key sizes are not bound to the modulus size specified by the composite OID. I might be wrong here, please check!

You are correct as far as I can see, but I first want to adopt @reneme 's refactoring before addressing it.

[falko-strenzke]

@falko-strenzke I took the liberty and refactored your patch (mostly mlkem_comp.cpp) based on #5715 and #5717. See here: https://github.com/reneme/botan/tree/falko/mlkem-composite (squashed to one commit and based on #5715).
This also addresses plenty of the suggestions I added above. But most notably, it gets rid of almost all low-level algorithm-specific handling by reusing existing concepts.
I'd appreciate if you could have a read-through my refactorings and pull them into this PR.

@reneme As I wrote in #5715, I am not entirely convinced of the multi-algorithm framework that you rebased the implementation onto. As a plus, it reduces the code size in mlkem_comp.cpp by one third, on the negative side, the trivial combination of two KEMs becomes somewhat harder to read due to a concept that doesn't actually bring much functionaility. In any case, I am fine with your proposal, but I would like to hear from @randombit first before pulling it in. The decision to use the multi-algorithm framework throughout the library in long term seems to be fundamental one.

I just made a diff of our branches. After seeing many unrelated differences, I rebased on master (where I rather should have rebased onto #5715, as I saw afterwards), and still have many unrelated differences. Coud you @reneme rebase your branch on current master (4695b4a is what this PR is based on now), too , so that we have a clean diff?

[reneme]

Coud you rebase your branch on current master?

I rebased #5715 onto a8f6c6a

[falko-strenzke]

Coud you rebase your branch on current master?

I rebased #5715 onto a8f6c6a

@reneme

Yes, but not https://github.com/reneme/botan/tree/falko/mlkem-composite as far as I could see..

But no problem, I now adopted the changes from that branch and should now have addressed all your review comments. Maybe you want to look over it again.

[falko-strenzke]

For the RSA composites, there seems to be the same issue as mentioned for #5451 that the RSA key sizes are not bound to the modulus size specified by the composite OID. I might be wrong here, please check!

You are correct as far as I can see, but I first want to adopt @reneme 's refactoring before addressing it.

Now done. We have the same key size and EC group checks as for ML-DSA composites.

[FAlbertDev]

Do we know that Composite-ML KEM will be recommended with the upcoming TR? Because, as you correctly state in the cryptodoc (sehlen-bsi/botan-docs#296), Composite ML-KEM is not fully TR compatible.

@sehlen-bsi Any insights?