TPM2: ECC Support (won't merge here) #14

reneme · 2024-10-01T12:44:17Z

This is just for self-review. The branch will be squashed and incorporated into randombit#4337 later.

This will catch linker visibility issues with shared builds

When building a specific target (e.g. 'tests'), the job dependency pointed to 'libbotan-3.so.6'. This target will explicitly build just 'libbotan-3.so.6' but fail to set up the required symlinks 'libbotan-3.so' and '.so.6.6.0'. As a result, the linker command with `-lbotan-3` will fail to find the shared object.

…bility Fix TLS::Context Visibility

reneme

@atreiber94 looks really good to me. A noted a few things and nits below.

src/lib/prov/tpm2/tpm2_algo_mappings.h

src/lib/prov/tpm2/tpm2_ecc/info.txt

src/lib/prov/tpm2/tpm2_ecc/tpm2_ecc.cpp

src/lib/prov/tpm2/tpm2_ecc/tpm2_ecc.h

src/lib/prov/tpm2/tpm2_rsa/tpm2_rsa.cpp

src/scripts/ci/start_tpm2_simulator.sh

This code point falls into the 'private code point' region (RFC 8446 4.2.3) and was used by pq.cloudflareresearch.com for hybrid key exchange using X25519+KyberR3-512.

Deprecate 0xFE30 X25519/Kyber512 code point

Response files are a means to work around the limited command line length particularly on Windows. Instead of listing all to-be-linked object files on the command line, ninja will write them into a "response file" and address that in the command using the @something.txt syntax.

reneme · 2024-10-02T12:38:32Z

src/lib/prov/tpm2/tpm2_ecc/tpm2_ecc.cpp

+   return {
+      .signature_scheme =
+         TPMT_SIG_SCHEME{
+            .scheme = TPM2_ALG_ECDSA,  // Only support ECDSA


We might have to explicitly support TPM2_ALG_SM2 here, when we're dealing with a SM2 keys.

True, actually supporting SM2 still requires some changes but I'd say the scope here is only ECDSA

src/lib/pubkey/ec_group/ec_group.h

atreiber94 · 2024-10-02T15:54:23Z

13ba56d includes now the EC(DH) Crypto Backend. In this case we require an RNG on our part as Alice and should check what RNG we should use.

src/lib/prov/tpm2/tpm2_crypto_backend/tpm2_crypto_backend.cpp

reneme · 2024-10-02T18:53:09Z

src/lib/prov/tpm2/tpm2_crypto_backend/tpm2_crypto_backend.cpp

+   return thunk([&] {
+      BOTAN_UNUSED(userdata);
+      BOTAN_ASSERT_NONNULL(key);
+      BOTAN_ASSERT_NOMSG(key->publicArea.type == TPM2_ALG_ECC);


Not sure, but I believe this could also be TPM2_ALG_SM2 if we deal with an SM2 key... We should figure out a way to test this.

Actually, type = TPM2_ALG_ECC for all SM2/ECDSA/ECDH keys. scheme will be different if the TPM key is restricted to a certain scheme like SM2/ECDSA/ECDH.

Starting a session with an explicit ECDH key works since the decrypt flag can be set for this scheme. However, if we restrict the scheme to SM2 or ECDSA, tpm2-toolsdoes not allow to set the decrypt flag since these are signature schemes.

=> The assert is fine, however we cannot test the backend with SM2 since we currently require our sessions to require the decrypt key object attribute. We could have a look at that.

I removed this assert because it's already made when loading the key as a Botan::TPM2::EC_PublicKey.

reneme · 2024-10-02T18:55:35Z

src/lib/prov/tpm2/tpm2_crypto_backend/tpm2_crypto_backend.cpp

+      // Write public ephemeral affine coordinates to TPMS_ECC_POINT (Q)
+      if(bn_x.bytes() != curve_order_byte_size || bn_y.bytes() != curve_order_byte_size) {
+         // TODO: Is actually correct to check bytes() > curve_order_byte_size?
+         return TSS2_ESYS_RC_GENERAL_FAILURE;
+      }


As far as I remember, BigInt::bytes() does not take zero-bytes into account. So if the coordinate has a zero byte by chance, <= might actually be better here, as you probably imply in the TODO.

This check is actually performed in BigInt::serialize(curve_order_byte_size) so I removed it

src/lib/prov/tpm2/tpm2_ecc/tpm2_ecc.h

reneme · 2024-10-03T12:23:57Z

src/lib/prov/tpm2/tpm2_crypto_backend/tpm2_crypto_backend.cpp

+      const auto tpm_pub_inputs = Botan::TPM2::ecc_pubkey_from_tss2_public(key);
+      const auto tpm_sw_pubkey = Botan::ECDH_PublicKey(tpm_pub_inputs.first, tpm_pub_inputs.second);


Somewhat nitty, but would safe a copy:

const auto tpm_sw_pubkey = [&] { auto tpm_pub_inputs = Botan::TPM2::ecc_pubkey_from_tss2_public(key); return Botan::ECDH_PublicKey(std::move(tpm_pub_inputs.first), std::move(tpm_pub_inputs.second)); }();

clang gives a hiccup-move-const-arg for this because ECDH_PublicKey takes const refs as inputs

src/lib/prov/tpm2/tpm2_crypto_backend/tpm2_crypto_backend.cpp

reneme · 2024-10-03T12:32:45Z

src/lib/prov/tpm2/tpm2_crypto_backend/tpm2_crypto_backend.cpp

+      BOTAN_ASSERT_NONNULL(key);
+      BOTAN_ASSERT_NOMSG(key->publicArea.type == TPM2_ALG_ECC);
+
+      const auto curve_name = Botan::TPM2::curve_id_tss2_to_botan(key->publicArea.parameters.eccDetail.curveID);


Perhaps it is worth considering to let curve_id_tss2_to_botan() perform the transformation into an EC_Group as well. I believe in the ECC code we also just transform the curve name into an EC_Group right away.

We can actually get the EC_Group if we load the TPM EC PK first - exactly the same operation is performed there. Then, there's no redundant transformation code.
(Additionally, that way all methods in tpm2_algo_mappings.h consistently remain mappings between algo strings/ids which makes me slightly happier than introducing methods that create instances of the algos).

reneme · 2024-10-03T12:34:46Z

src/lib/prov/tpm2/tpm2_crypto_backend/tpm2_crypto_backend.cpp

+      const auto curve_order_byte_size =
+         Botan::TPM2::curve_id_order_byte_size(key->publicArea.parameters.eccDetail.curveID);


... If curve_id_tss2_to_botan() were to return an EC_Group, this could then just take domain.get_p_bytes() and we don't even need this extra mapping.

With loading the TPM PK first, we can just use get_p_bytes() of the TPM PK's domain (see #14 (comment)).

reneme · 2024-10-03T12:41:58Z

On a more general note (regarding RSA and ECC support in the crypto backend): I think we should consider disentangling the availability of the tpm2_rsa and tpm2_ecc modules from support for the respective key exchanges in the crypto backend. I.e. a build configuration with --enable-modules=tpm2_crypto_backend,rsa,ecdh --disable-modules=tpm2_rsa,tpm2_ecc should probably still be able to do RSA and ECDH exchanges.

As far as I could see, we just need the public_key_from_tpm2_public() methods from the respective adapters. We could put those more centrally and guard them with #if defined(BOTAN_HAS_RSA) and #if defined(BOTAN_HAS_ECDSA) || defined(BOTAN_HAS_ECDH).

What do you think?

src/lib/prov/tpm2/tpm2_ecc/info.txt

atreiber94 · 2024-10-04T14:33:05Z

On a more general note (regarding RSA and ECC support in the crypto backend): I think we should consider disentangling the availability of the tpm2_rsa and tpm2_ecc modules from support for the respective key exchanges in the crypto backend. I.e. a build configuration with --enable-modules=tpm2_crypto_backend,rsa,ecdh --disable-modules=tpm2_rsa,tpm2_ecc should probably still be able to do RSA and ECDH exchanges.

As far as I could see, we just need the public_key_from_tpm2_public() methods from the respective adapters. We could put those more centrally and guard them with #if defined(BOTAN_HAS_RSA) and #if defined(BOTAN_HAS_ECDSA) || defined(BOTAN_HAS_ECDH).

What do you think?

I agree that we should disentangle TPM2_{ECC/RSA}_ADAPTER and RSA/ECC and implemented the suggestion in 46f063c.

That way, all kinds of build configurations work, e.g., you could do EC key exchange in the crypto backend (if BOTAN_HAS_ECDH; otherwise it fails) without having TPM2_ECC_ADAPTER.

However, I'm not sure someone would ever want to do this: If you want to establish an authenticated session with an RSA resp. ECC key, then you need tpm2_rsa resp. tpm2_ecc to load the key as input to the authenticated session. So to me it seems that having the tpm2 adapter is functionally necessary to use the corresponding backend key exchange and therefore I don't see a functional benefit. (One could further disentangle this for just using the keys as parents / for sessions vs. sign/encrypt operations but this would become very complicated. Or do you think we should strive for this kind of configurability?)

Still, I like the suggested disentanglement because it provides a much clearer distinction on dependencies (the crypto backends only require the Botan RSA/EC components and the TPM2 RSA/EC components separately provide RSA/EC functionalities based on TPM keys).

I made sure that the tests works for the different kinds of minimized builds but the coverage is not complete because sometimes the SRK is loaded, which is RSA in our case.

In a minimized build, one also needs to ensure modules necessary for the backend are present: aes,cfb,sha1,sha2_64,aead,hmac ,rsa,eme_pkcs1,eme_oaep to enable RSA key exchange, and ecdh for EC key exchange. We should discuss which modes should be required by the crypto backend module and which should not be a requirement. I'd favor requiring everything except the key exchange modules because these may not be necessary when using crypto callbacks.

reneme · 2024-10-07T06:57:45Z

We should discuss which modes should be required by the crypto backend module and which should not be a requirement.

The minimalist's answer would be: only the base modules whose headers are included directly (e.g. hash,mac, ...) should be hard requirements. Then the user is in charge (and has maximum flexibility) which concrete algorithms should be available (e.g. sha1 yay/nay?). On the other hand this requires the user to "know" which algorithms are necessary and perhaps wade through several "not implemented" errors to find a working module set.

Side note: It would perhaps be nice to have a list of "recommended module dependencies" to provide a reasonable default that is only pulled in as dependencies when configuring --with-recommendations --enable-modules=tpm2_rsa.

reneme

A few more suggestions.

src/lib/prov/tpm2/tpm2_crypto_backend/tpm2_crypto_backend.cpp

…iles_for_link_commands Use @response_files.txt for linking in Ninja

Allow using `secure_allocator` for enum types, like `std::byte`

…xample Example fix: Use more secure KEM combination in example

…o-string Reorder/rephrase output from X509_Certificate::to_string

…ariants Deprecate various aliases for the public key padding code

…port Feature: TPM2 Support

atreiber94 · 2024-10-07T11:59:10Z

Thanks! Applied the suggestions. Regarding the modules: Let's go with the minimalist view and only require the algos/modules that are "hard-coded" in the spec. That's actually what we already had - if a session with a specific algo/mode then the user needs a build supporting the corresponding module.

Update Fuzzer documentation and Configuration

To add ECDH support, different TPM2-TSS methods need to be incorporated into Key Agreement PK operations. The class hierarchy of Botan's EC keys currently makes this hard since a TPM2 EC key may not be restricted to just ECDH or ECDSA.

This includes disentangling the TPM2 Crypto Backend from the availability of the TPM2 RSA Adapter module. Instead, it now only depends on the software RSA/EC implementation of Botan.

Co-Authored-By: Amos Treiber <[email protected]>

reneme · 2024-10-08T09:25:54Z

ECC implementation and internal review done. @atreiber94 will open a PR in randombit/botan. Closing....

Logix64 and others added 8 commits August 2, 2024 22:33

Update fuzzing docs and configuration

1f584b2

Update fuzzing signature

ba935a8

Add BOTAN_PUBLIC_API to TLS::Context class

ee41b6b

Move examples target to shared build in ci;

f0ecad0

This will catch linker visibility issues with shared builds

Merge pull request randombit#4336 from Rohde-Schwarz/fix/ninja_shared

89c74e9

Merge pull request randombit#4335 from Rohde-Schwarz/fix/context_visi…

0639e82

…bility Fix TLS::Context Visibility

Use more secure KEM combination in example

1912f57

reneme commented Oct 1, 2024

View reviewed changes

reneme and others added 3 commits October 1, 2024 16:55

Deprecate 0xFE30 X25519/Kyber512 code point

b5df358

This code point falls into the 'private code point' region (RFC 8446 4.2.3) and was used by pq.cloudflareresearch.com for hybrid key exchange using X25519+KyberR3-512.

Merge pull request randombit#4347 from reneme/chore/retire_0xFE30

7f256a0

Deprecate 0xFE30 X25519/Kyber512 code point

reneme commented Oct 2, 2024

View reviewed changes

reneme force-pushed the feature/tpm2_support branch from c354200 to 18a9a16 Compare October 2, 2024 13:02

reneme mentioned this pull request Oct 2, 2024

Feature: TPM2 Support randombit/botan#4337

Merged

reneme force-pushed the feature/tpm2_support branch 2 times, most recently from 0198e61 to 51fd5dd Compare October 2, 2024 13:48

atreiber94 reviewed Oct 2, 2024

View reviewed changes

src/lib/pubkey/ec_group/ec_group.h Show resolved Hide resolved

reneme commented Oct 2, 2024

View reviewed changes

src/lib/prov/tpm2/tpm2_crypto_backend/tpm2_crypto_backend.cpp Outdated Show resolved Hide resolved

reneme commented Oct 2, 2024

View reviewed changes

src/lib/prov/tpm2/tpm2_crypto_backend/tpm2_crypto_backend.cpp Outdated Show resolved Hide resolved

reneme commented Oct 2, 2024

View reviewed changes

reneme commented Oct 3, 2024

View reviewed changes

src/lib/prov/tpm2/tpm2_ecc/tpm2_ecc.h Outdated Show resolved Hide resolved

reneme commented Oct 3, 2024

View reviewed changes

src/lib/prov/tpm2/tpm2_crypto_backend/tpm2_crypto_backend.cpp Outdated Show resolved Hide resolved

reneme commented Oct 3, 2024

View reviewed changes

src/lib/prov/tpm2/tpm2_crypto_backend/tpm2_crypto_backend.cpp Show resolved Hide resolved

reneme commented Oct 3, 2024

View reviewed changes

src/lib/prov/tpm2/tpm2_ecc/info.txt Outdated Show resolved Hide resolved

Allow using secure_allocator for enum types, like std::byte

4890fc9

reneme commented Oct 7, 2024

View reviewed changes

randombit and others added 8 commits October 7, 2024 06:11

Reorder/rephrase output from X509_Certificate::to_string

34d6b46

Deprecate various aliases for the public key padding code

097f78a

Merge pull request randombit#4350 from Rohde-Schwarz/chore/response_f…

4e2e9d5

…iles_for_link_commands Use @response_files.txt for linking in Ninja

Merge pull request randombit#4353 from Delta-dev-99/patch-1

278dc55

Allow using `secure_allocator` for enum types, like `std::byte`

Merge pull request randombit#4344 from Rohde-Schwarz/fix/hybrid_kem_e…

7e28dda

…xample Example fix: Use more secure KEM combination in example

Merge pull request randombit#4330 from randombit/jack/rephrase-x509-t…

175b1f2

…o-string Reorder/rephrase output from X509_Certificate::to_string

Merge pull request randombit#4355 from randombit/jack/deprecate-sig-v…

bd89aa3

…ariants Deprecate various aliases for the public key padding code

Merge pull request randombit#4337 from Rohde-Schwarz/feature/tpm2_sup…

7624371

…port Feature: TPM2 Support

randombit and others added 3 commits October 7, 2024 08:04

Merge pull request randombit#4114 from Logix64/update-fuzzing

00eb2d2

Update Fuzzer documentation and Configuration

Generalize TPM2 RSA Adapter in preparation of ECC support

7800449

Add TPM2 ECC Adapter to support ECDSA TPM2 keys

4d72781

To add ECDH support, different TPM2-TSS methods need to be incorporated into Key Agreement PK operations. The class hierarchy of Botan's EC keys currently makes this hard since a TPM2 EC key may not be restricted to just ECDH or ECDSA.

atreiber94 force-pushed the feature/tpm2_ecc branch 2 times, most recently from e587545 to 44692fb Compare October 7, 2024 14:24

Add ECDH Support to TPM2 Crypto Backend

fa7f055

This includes disentangling the TPM2 Crypto Backend from the availability of the TPM2 RSA Adapter module. Instead, it now only depends on the software RSA/EC implementation of Botan.

reneme force-pushed the feature/tpm2_ecc branch 2 times, most recently from 49afb70 to 8cc8c3d Compare October 7, 2024 15:24

reneme and others added 4 commits October 8, 2024 11:22

Be defensive with the input parameters in the crypto backend

78205c6

Co-Authored-By: Amos Treiber <[email protected]>

CI and Test infrastructure for TPM2 ECC support

6c9b503

Add TPM2 ECC Documentation

0596bb2

Add attribution to funding provider LANCOM

3ac8985

reneme force-pushed the feature/tpm2_ecc branch from 8cc8c3d to 3ac8985 Compare October 8, 2024 09:22

reneme closed this Oct 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TPM2: ECC Support (won't merge here) #14

TPM2: ECC Support (won't merge here) #14

reneme commented Oct 1, 2024

reneme left a comment

reneme Oct 2, 2024

atreiber94 Oct 2, 2024

atreiber94 commented Oct 2, 2024

reneme Oct 2, 2024

atreiber94 Oct 4, 2024 •

edited

Loading

reneme Oct 2, 2024

atreiber94 Oct 4, 2024

reneme Oct 3, 2024

atreiber94 Oct 4, 2024

reneme Oct 3, 2024

atreiber94 Oct 4, 2024

reneme Oct 3, 2024

atreiber94 Oct 4, 2024

reneme commented Oct 3, 2024

atreiber94 commented Oct 4, 2024

reneme commented Oct 7, 2024

reneme left a comment

atreiber94 commented Oct 7, 2024

reneme commented Oct 8, 2024

		const auto tpm_pub_inputs = Botan::TPM2::ecc_pubkey_from_tss2_public(key);
		const auto tpm_sw_pubkey = Botan::ECDH_PublicKey(tpm_pub_inputs.first, tpm_pub_inputs.second);

		const auto curve_order_byte_size =
		Botan::TPM2::curve_id_order_byte_size(key->publicArea.parameters.eccDetail.curveID);

TPM2: ECC Support (won't merge here) #14

TPM2: ECC Support (won't merge here) #14

Conversation

reneme commented Oct 1, 2024

reneme left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atreiber94 commented Oct 2, 2024

Choose a reason for hiding this comment

atreiber94 Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reneme commented Oct 3, 2024

atreiber94 commented Oct 4, 2024

reneme commented Oct 7, 2024

reneme left a comment

Choose a reason for hiding this comment

atreiber94 commented Oct 7, 2024

reneme commented Oct 8, 2024

atreiber94 Oct 4, 2024 •

edited

Loading