Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Study: build TLS without ECP #6004

Closed
mpg opened this issue Jul 1, 2022 · 8 comments
Closed

Study: build TLS without ECP #6004

mpg opened this issue Jul 1, 2022 · 8 comments
Assignees
Labels
enhancement size-m Estimated task size: medium (~1w)

Comments

@mpg
Copy link
Contributor

mpg commented Jul 1, 2022

As of current development, when MBEDTLS_USE_PSA_CRYPTO is enabled, TLS uses only PSA Crypto for all ECDH(E) and ECDSA crypto operations; after #5847 that will be the case for EC J-PAKE operations as well. However, TLS still uses functions from the ECP module for non-crypto operations, such as information management (converting between TLS/IANA id and internal grp_id, querying curve size, etc.) and key import/export.

This task is to study how to get rid of those dependencies and create an estimated series of task achieving this result.

@mpg mpg added enhancement size-m Estimated task size: medium (~1w) labels Jul 1, 2022
@mpg mpg self-assigned this Jul 1, 2022
@mpg
Copy link
Contributor Author

mpg commented Jul 1, 2022

We can get a list of external symbols used by libmbedtls in a full config (which has MBEDTLS_USE_PSA_CRYPTO enabled) with:

scripts/config.py full
make lib
docs/architecture/psa-migration/syms.sh full

and then look at full-tls-modules and/or grep full-tls-external for functions of interest.

The good: this confirms no dependency on ecdh or ecdsa. This confirms dependencies on ecjpake which are known and will be addressed in #5847. (Also the same method for X.509 confirms no dependency on ecdh, ecdsa, ecjpake or ecp.)

The less good: grep mbedtls_ecp full-tls-external has the following non-empty output:

mbedtls_ecp_curve_info_from_grp_id
mbedtls_ecp_curve_info_from_tls_id
mbedtls_ecp_point_write_binary
mbedtls_ecp_write_key

Preliminary analysis:

  • the first two functions should be easy enough to replace by dedicated functions that translate between TLS/IANA Id and internal Id (arguably those functions should always have been in the TLS module, having thm in a low-level crypto module was a layering violation) and functions that get the other information in mbedtls_ecp_curve_info (bitsize, human-readable name for debug) from there.
  • mbedtls_ecp_point_write_binary() is called only once, from ssl_get_ecdh_params_from_cert() (the one in ssl_tls12_client.c) for static ECDH key exchange (TLS 1.2) client-side, to serialize the peer's public key from the parsed certificate. The certificate probably already has that serialized somewhere and perhaps we could get it from the pk_raw field.
  • mbedtls_ecp_write_key() is called only once, from ssl_get_ecdh_params_from_cert() (the one in ssl_tls12_server.c) for static ECDH key exchange (TLS 1.2) server-side, to export our own private key before we import it back in PSA. At this point the replacement is not clear to me. We could try using a pk_write function instead but the would only move the problem, unless we can implement PK parse/write in a way that doesn't depend on ECP.

@valeriosetti
Copy link
Contributor

Hi @mpg,
I took a quick look at this activity and here's what I found/what I would do:

  • mbedtls_ecp_curve_info_from_grp_id() seems to be used also in psa_crypto_ecp, so I don't think moving it to TLS would be the right solution. Therefore I would move both mbedtls_ecp_curve_info_from_grp_id() and mbedtls_ecp_curve_info_from_tls_id to an external common file, such as common_ecp.c. In this way is would be available to both TLS modules as well as psa_crypto_ecp
  • I would move also mbedtls_ecp_write_key() and mbedtls_ecp_read_key() in the same common file. Unfortunately this does not get rid of the BIGNUM_C dependency, but at least it should take ECP_C dependency away
  • when it comes to mbedtls_ecp_point_write_binary() I'm still working on this. At first I thought I could take the key from ssl->session_negotiate->peer_cert->raw.p (if SSL_KEEP_PEER_CERTIFICATE) or from ssl->handshake->peer_pubkey (if !SSL_KEEP_PEER_CERTIFICATE), but it seems not to work properly.

Please note that this is the first time for me to work on this repo, so I'm aware that my proposal can be totally garbage. But in case it makes any sense to you, just let me know and I'll open a PR to let you check it.

@mpg
Copy link
Contributor Author

mpg commented Nov 15, 2022

Just some quick thoughts on the first part. The type mbedtls_ecp_curve_info() has a layering problem, in that it generic crypto information with TLS-specific information (namely tls_id as the name implies). That probably made sense when it was introduced, as libmbedcrypto had no ambition of being a generic crypto library, just ad-hoc support for implementing X.509 and TLS (actually when this was introduced, make lib produced only one library with everything in it, having it build libmbedcrypto, libmbedx509 and libmbedtls as separate outputs only came later).

This is highly visible in a lot of places: what's the input/output formats of mbedtls_dhm_xxx() functions? The one defined in the TLS RFC. That of mbedtls_ecjpake_xxx() functions? You guess it, that from the EC J-PAKE in TLS draft. And it goes on and on.

But now things are changing, we intend to retire all those mbedtls_xxx() crypto APIs and leave PSA as the only Crypto API, which aims to be a generic crypto API that can be used to build all kinds of higher-level protocols, not just TLS; also the reference implementation of PSA Crypto is going to be split into its own independent project. So we should really have better layering / separation of concerns between generic crypto stuff on one hand, and TLS-specific things on the other hand.

So, all the TLS-specific constants and helper functions should now live in TLS: that's the tls_id part of curve_info, constants like MBEDTLS_ECP_TLS_NAMED_CURVE (I mean, the name's quite a hint), probably functions like mbedtls_ecp_tls_xxx() (same remark). Until Mbed TLS 4.0 we can't change or remove existing APIs, so that probably means some temporary duplication between what's already there in ECP and what we add to TLS where it belongs.

Regarding the generic data, like bit size etc, I'm not sure yet where that should live, might be a new module common_ecp.c indeed, might be something else... I just wanted to point out that we want to strive for a better separation of layers in the future.

@gilles-peskine-arm
Copy link
Contributor

Regarding the generic data, like bit size etc,

Code using the PSA API (e.g. all parts of the code under MBEDTLS_USE_PSA_CRYPTO) should be able to get this information via a PSA API, otherwise it indicates a possible gap in the API. But we may want to have the information in parts of the pk/x509/tls code that is common to both with and without USE_PSA. So for that, a generic ECP metadata module makes sense.

@mpg
Copy link
Contributor Author

mpg commented Dec 16, 2022

Note (just as I was going through old issues and noticed one that might be related): similarly to what we're gonna do for hashes identifiers in MD, we might want to make ECP group ID to make them more similar to PSA identifiers, which are a pair (family, bitsize). The primary purpose would be to save code size in the conversion functions ECP <-> PSA, but as it happens, I think that would also be a step towards #2375

@valeriosetti
Copy link
Contributor

but as it happens, I think that would also be a step towards #2375

Sorry, but it's not totally clear to me how I can help here.

  • Should I start a new PR in order to solve Avoid enumerating group IDs in the code #2375? or
  • Should I wait for a new issue (addressing we might want to make ECP group ID to make them more similar to PSA identifiers) to be created? If this is the case, can I create the issue autonomously?

@mpg
Copy link
Contributor Author

mpg commented Dec 21, 2022

@valeriosetti I'll create issues tomorrow! Sorry, been a bit busy recently and hadn't noticed you were already done with 6702.

@mpg
Copy link
Contributor Author

mpg commented Dec 27, 2022

Closing, superseded by #6839 that takes a larger view of the issues at hand.

@mpg mpg closed this as completed Dec 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement size-m Estimated task size: medium (~1w)
Projects
None yet
Development

No branches or pull requests

3 participants