Review/remove uses of MBEDTLS_PRIVATE in programs

[mpg]

In #4511 we made most struct fields private, however we took a shortcut, by allowing ourselves to use MBEDTLS_PRIVATE fields in programs.

Most of the programs are supposed to be samples that the users can look at, get inspiration from or even copy-paste-edit, so they should demonstrate best practices and respect the boundaries of the library's public API.

There are currently 74 uses of MBEDTLS_PRIVATE in 18 files in the programs directory. Each should be reviewed and handled appropriately, for example:

• modify the program to use a public API instead of accessing a private field
• when there's no suitable public API and it it seems that it would be of general use, consider making the field public or adding a suitable API (see also Document some structure fields as public #4603 and [placeholder] community feedback on private fields #4682) and then using it
• if the very purpose of the program is to display information that's normally private (for example key_app), perhaps leave it as is but very clearly document that it's not an example to be followed
• restructure the program to take a different approach (for example programs/pkey/ecdh_curve25519.c uses private fields of ecdh_context but could use a bunch of mbedtls_mpi and mbedtls_ecp_point variable instead)

Break down by structures:

• mbedtls_cipher_info_t
• mbedtls_rsa_context
• mbedtls_dhm_context
• mbedtls_ecdh_context
• mbedtls_ecp_keypair, mbedtls_ecdsa_context
• mbedtls_ecp_curve_info
• mbedtls_x509_crt
• mbedtls_x509_csr
• mbedtls_net_context

[paul-elliott-arm]

Downgrading to M, as a lot of the issues have been fixed. Also, beware that the subtasks of this are in the same epic, and thus the perceived amount of work to do is potentially doubled.

[minosgalanakis]

Having looked at the use of accessors in programs the following are being used.

RSA

rsa_encrypt:
rsa_decrypt:
	rsa.MBEDTLS_PRIVATE(len)

rsa_verify.c
    rsa.MBEDTLS_PRIVATE(N) 
    rsa.MBEDTLS_PRIVATE(E)
    rsa.MBEDTLS_PRIVATE(len)

The estimation is that not a lot is needed for it. The (N) and (E) should not be accessed and the programs need to be refactored around it, and the len has already an accessor at:

size_t mbedtls_rsa_get_len(const mbedtls_rsa_context *ctx)

ECP

pkey/gen_key.c
key_app_writer.c:
key_app.c:
	ecp->MBEDTLS_PRIVATE(Q).MBEDTLS_PRIVATE(X,Y,Z)
	ecp->MBEDTLS_PRIVATE(d)

ecdsa.c
	key->MBEDTLS_PRIVATE(grp)
	key->MBEDTLS_PRIVATE(Q)

This is a bit more tricky as discussed already in #78045

But the guidance is that the programs will need to refactored and expect that no every single member or curve coordinate is being printed in stdout.

d and Q should not be exposed but we are misssing a way for the user/app to extract the public key.

[mpg]

Note: there are also uses of MBEDTLS_ALLOW_PRIVATE_ACCESS in programs, though it's mostly testing programs (programs/fuzz, programs/test, some under programs/ssl) as opposed to demo programs, so that's probably OK.

[gilles-peskine-arm]

Good point about MBEDTLS_ALLOW_PRIVATE_ACCESS. Of the programs that use it, I consider them all to be test programs that are expected to look under the hood except benchmark.c. I'm checking that and it's all about the MBEDTLS_ECDH_LEGACY_CONTEXT block that's using the weird TLS-oriented API and printing information about “handshake”. We also have a block for ECDH that's sticking to the Everest-compatible API and printing information about “full handshake”. Do we even need the MBEDTLS_ECDH_LEGACY_CONTEXT block? What is it reporting that the “full handshake” benchmark doesn't convey?

[gilles-peskine-arm]

Since 3.0, MBEDTLS_ECDH_LEGACY_CONTEXT is only enabled when MBEDTLS_ECP_RESTARTABLE is enabled. So most users won't even see that part of the printout. The legacy-context block doesn't convey any information that would be specifically relevant to restartable ECC. So I think we should just remove that block. I think that's a post-3.0 cleanup that we just forgot about. I'll make a PR.

[mpg]

Here's the output of the "normal" ECDH block in full config:

  ECDHE-secp521r1          :     144 full handshake/s
  ECDHE-brainpoolP512r1    :      18 full handshake/s
  ECDHE-secp384r1          :     219 full handshake/s
  ECDHE-brainpoolP384r1    :      38 full handshake/s
  ECDHE-secp256r1          :     342 full handshake/s
  ECDHE-secp256k1          :     327 full handshake/s
  ECDHE-brainpoolP256r1    :      77 full handshake/s
  ECDHE-secp224r1          :     502 full handshake/s
  ECDHE-secp224k1          :     335 full handshake/s
  ECDHE-secp192r1          :     594 full handshake/s
  ECDHE-secp192k1          :     392 full handshake/s
  ECDHE-x25519             :     233 full handshake/s
  ECDHE-x448               :     114 full handshake/s

And the output of the LEGACY_CONTEXT part (also full config):

  ECDHE-secp521r1          :     291 handshake/s
  ECDHE-brainpoolP512r1    :      37 handshake/s
  ECDHE-secp384r1          :     454 handshake/s
  ECDHE-brainpoolP384r1    :      79 handshake/s
  ECDHE-secp256r1          :     703 handshake/s
  ECDHE-secp256k1          :     612 handshake/s
  ECDHE-brainpoolP256r1    :     155 handshake/s
  ECDHE-secp224r1          :    1010 handshake/s
  ECDHE-secp224k1          :     699 handshake/s
  ECDHE-secp192r1          :    1430 handshake/s
  ECDHE-secp192k1          :     929 handshake/s
  ECDHE-x25519             :     556 handshake/s
  ECDHE-x448               :     238 handshake/s
  ECDHE-Curve25519         :     565 handshake/s
  ECDHE-Curve448           :     238 handshake/s
  ECDH-secp521r1           :     379 handshake/s
  ECDH-brainpoolP512r1     :      46 handshake/s
  ECDH-secp384r1           :     576 handshake/s
  ECDH-brainpoolP384r1     :     102 handshake/s
  ECDH-secp256r1           :    1003 handshake/s
  ECDH-secp256k1           :     902 handshake/s
  ECDH-brainpoolP256r1     :     207 handshake/s
  ECDH-secp224r1           :    1399 handshake/s
  ECDH-secp224k1           :     941 handshake/s
  ECDH-secp192r1           :    1931 handshake/s
  ECDH-secp192k1           :    1253 handshake/s
  ECDH-x25519              :    1130 handshake/s
  ECDH-x448                :     466 handshake/s
  ECDH-Curve25519          :    1089 handshake/s
  ECDH-Curve448            :     469 handshake/s

The output of the LEGACY_CONTEXT block is confusing:

• It prints Montgomery curves twice under different names (as a special case for them that used to be necessary but that we forgot to remove when those curves became usable in TLS).
• It prints everything twice with the same title of "handshake/s" but numbers that are significantly different (mostly a factor two for Montomery curves). Edit: the first time is ECDHE-curve, the second one is ECHD-curve. The difference is subtle enough that I did not notice before looking at the code.

Looking at the code:

• the first part of LEGACY_CONTEXT is make_public() + calc_secret() (and for duplicated Montgomery curves, gen_public() + compute_shared() which is indeed equivalent except for TLS formatting). That's one side of an ephemeral handshake.
• the second part of LEGACY_CONTEXT is only calc_secret() (and for duplicated Montgomery curves, compute_shared() which is indeed equivalent except for TLS formatting). That's one side of a static handshake.
• the "generic" code is both sides of an ephemeral handshake.

[mpg]

When it comes to ECC computations, broadly speaking there are three things:

1. Generic scalar multiplication (GM): s * P where P is not know in advance.
2. Scalar multiplication of the base point (BP): s * G where G is the curve's conventional base point. This can be implemented the same as the generic one (what we do for Montgomery curves) or use pre-computed tables (what we do for Short Weierstrass curves when FIXED_POINT_OPTIM == 1.
3. Linear combination (LC): s * G + t * P. This can be implemented as two multiplications (1 BP + 1 GM or 2 GM) and one addition, or use a faster algorithm known as Shamir's trick (which we don't do at the moment). (Also, this one doesn't need to be constant-time.)

This maps to high-level operations as follows:

• keygen is 1 BP;
• ephemeral ECDH on one side is 1 BP + 1 GM (psa_generate_key() + psa_raw_key_agreement();
• static ECDH on one side is 1 GM (psa_raw_key_agreement());
• ECDSA signing is 1 BP + $\epsilon$ (one field inversion mostly);
• ECDSA verification is 1 LC (which for use currently is 1 BP + 1 GM)

As a developer, I'd like the benchmark output to allow me to quickly see the effects if I tune one kind of operation:

• for LC, ECDSA verification is great;
• for BP, ECDSA signature generation is close enough;
• for GM, static ECDH on one side would be great, and that's provided only by the LEGACY_CONTEXT code right now (though I need to notice the difference between ECDHE and ECDH in the output).

[gilles-peskine-arm]

So, if I understand correctly, what you'd like to add to the default-configuration benchmark is static ECDH? Can you please file an issue and put it on the Barriers board?

[mpg]

Patch incoming.

[gilles-peskine-arm]

I looked at the other programs, and most don't warrant MBEDTLS_ALLOW_PRIVATE_ACCESS. In #8855 the only uses of MBEDTLS_ALLOW_PRIVATE_ACCESS left are library/* (of course), tests/** (something we might want to restrict later, but that's a long-term thing) and programs/ssl/ssl_{client,server}2 (which are meant to include invasive tests so that's expected). So I've updated #8855 accordingly.