mbedtls_ecp_point_read_binary from compressed fmt

[gstrauss]

Description

mbedtls_ecp_point_read_binary() from MBEDTLS_ECP_PF_COMPRESSED format

Status

READY

Requires Backporting

NO

Migrations

NO

Additional comments

While this is a feature, it adds major missing functionality that has cost me days of time troubleshooting and fixing in the hostap test suite. Please backport to mbedtls-2.28 branch.

Todos

• Tests
• Changelog updated
• No backport (new feature)

[gstrauss]

Related historical issues/PRs which attempted to address this shortcoming in mbedtls:
#521
#861
#1616

I hope this PR is small enough that mbedtls team will consider including it.

[gilles-peskine-arm]

Can you please explain why you want to add this feature, which as previously noted has been somewhat deprecated in TLS for several years? Is this for a protocol that's not TLS (which can be a good justification, despite the name “Mbed TLS”, the crypto library is not just for TLS)?

In any case I really don't see any justification for backporting it.

[gstrauss]

Can you please explain why you want to add this feature

As I noted above:

While this is a feature, it adds major missing functionality that has cost me days of time troubleshooting and fixing in the hostap test suite.

More details: hostap test suite uses compressed format in some places. hostap is widely used.
hostapd - user space daemon for access points, including, e.g., IEEE 802.1X/WPA/EAP Authenticator for number of Linux and BSD drivers, RADIUS client, integrated EAP server, and RADIUS authentication server
wpa_supplicant user space IEEE 802.1X/WPA supplicant (wireless client) for number of Linux, BSD, and Windows drivers

I am attempting to port hostap to be able to use mbedtls
openwrt/openwrt#10303
openwrt/openwrt#10727
and the effort has exposed some gaps in mbedtls APIs. This PR is one.

[mpg]

Hi, and thanks for your contribution. Since you've linked to previous issues and PRs, you are aware that this is a feature we have rejected a few times already. Let me sum up the reasons:

• This format tends to be optional in standards; for example when ECC support was introduced in TLS 1.0-1.2, only the uncompressed format was mandatory to implement, all others were MAY (not even SHOULD) support, see RFC 4492 sec. 5.1.2.
• This format has since been removed from some standards; for example in TLS is was removed in TLS 1.3, and by RFC 8422 (which obsoletes RFC 4492) which even makes it a MUST NOT (in TLS, obviously, that doesn't prevent non-TLS APIs such as mbedtls_ecp_point_read_binary() from supporting it).
• Supporting this format either adds code to all builds, even for people who are not using it, or needs an additional configuration option, which adds to testing complexity. This price should not be paid if it's not worth it, and the points above suggest it shouldn't be.

However, standards in general and TLS in particular are not everything. If the feature is used widely enough in practice, it might be worth paying the price. You claim this is a major missing feature; I'd like to see that claim substantiated. The fact that this has been requested for the fourth time indeed points in that direction, but I'll note that the 3 links you gave didn't explain why this was needed; there's also #1608 which just said "if it's supported in writing, it should be supported in reading", which IMO is weak. So, your PR is the first one to offer an explanation: you want this in order to pass the hostap test suite.

First, thanks for providing that justification, but I'm afraid I'll answer with a few more questions: why is the hostap test suite using this format? Is it on purpose, or just by happenstance (like, this was the default format of the tool used to generate the test data? I'm unfamiliar with the protocol hostapd implements: is support for this format necessary to interop with other implementations or is there some kind of negotiation where each implementation can advertise what it supports (as in TLS, which is why we never need support for TLS?

Whether we want to support that feature depends on the answers to those questions; for example if it's necessary to interop with other implementations of a widely-used protocol, I'd say that's a clear yes; if it's just to pass a test suite that happens to use it by accident, I think that test suite should be improved instead.

Regardless, I want to state upfront that we are unlikely to accept adding this to 2.28; the bar for adding features to LTS branches is very high and this would require very strong justification.

[mpg]

I did a quick review just to gauge the amount of work that will be needed if we accept this. Things are looking good overall, I think most of the work would be on improving testing and documentation/comments. Perhaps we'll want this to be guarded by a compile-time option as well.

Anyway, I don't recommend you spend time on updating this PR in response to my comments before we have progressed the discussion about where we want to support the feature at all.

[gstrauss]

An argument that I can give to you for why this feature should be included in mbedtls in some way is as follows:
If I am using mbedtls as a library to provide crypto and TLS support, then I should avoid writing low-level crypto and TLS code outside of the crypto/TLS library. It is generally considered poor security practices to try to reinvent crypto library code if not an expert in crypto, and this is therefore a strong argument (IMHO) why the crypto algorithms should be provided by the mbedtls library.

@mpg wrote:

I think most of the work would be on improving testing and documentation/comments. Perhaps we'll want this to be guarded by a compile-time option as well.

Agreed. What do you suggest for a compile-time option name?

With a compile-time option, perhaps default disabled, there should hopefully be less of an argument about whether or not to include this small amount of additional code in mbedtls to support the optional feature to parse the compressed format, even if it is now deprecated/forbidden by the standards.

Separately, I plan to look further into the hostap code to see if the compressed format is used only in the test suite, or if it might be produced by the widely used hostapd and wpa_supplicant code. In the latter case, if it turns out to be true, then even if the code is modified to cease using the compressed format, there would be existing code out in the wild still using the compressed format. This remains to be determined.

[mpg]

An argument that I can give to you for why this feature should be included in mbedtls in some way is as follows: If I am using mbedtls as a library to provide crypto and TLS support, then I should avoid writing low-level crypto and TLS code outside of the crypto/TLS library. It is generally considered poor security practices to try to reinvent crypto library code if not an expert in crypto, and this is therefore a strong argument (IMHO) why the crypto algorithms should be provided by the mbedtls library.

I think we fully agree on that. But it's a fairly generic argument, which applies to all crypto constructs. We don't want to implement every single crypto alg or format that ever existed (for example, we never had support for binary curves, only primes), so in addition to that general argument, we need some reason to include this format specifically - evidence that it's widely used in production.

Separately, I plan to look further into the hostap code to see if the compressed format is used only in the test suite, or if it might be produced by the widely used hostapd and wpa_supplicant code. In the latter case, if it turns out to be true, then even if the code is modified to cease using the compressed format, there would be existing code out in the wild still using the compressed format. This remains to be determined.

That's the kind of thing I'm talking about. If the format might be produced by hostapd or wpa_supplicant, then as far as I'm concerned that's clearly a very strong argument to support this format. (And I fully agree that the argument holds as soon as it's produced by versions that are still in use, even if not the latest.) If on the other hand it's just the test suite, IMO the argument is much weaker - though I can see that passing existing test suites of widely-used software, even if they're using more than they should, is desirable it itself, as it would have saved you time in your porting effort (but then again, part of me is tempted to argue the test suite would be to blame here, if it had no good reason to use this format).

Agreed. What do you suggest for a compile-time option name?

I don't know. Perhaps MBEDTLS_ECP_READ_COMPRESSED_FORMAT (to highlight that we always support writing this format).

With a compile-time option, perhaps default disabled, there should hopefully be less of an argument about whether or not to include this small amount of additional code in mbedtls to support the optional feature to parse the compressed format, even if it is now deprecated/forbidden by the standards.

As a general principle, we try to avoid adding too many config options, as they tend to add to the testing complexity. However this particular one doesn't seem likely to interact with too many things, so it's probably fine.

PS: there are a few tests failing, in a lot of components. I only had a quick look, but it seems it's mostly negative cases. We can probably just use another value for the first bytes (AFAIK, 0x01 and 0x05 are never valid). Hopefully the same tests are failing in all the components, so just getting make test to pass should fix them all.

[gilles-peskine-arm]

PS: there are a few tests failing, in a lot of components. I only had a quick look, but it seems it's mostly negative cases.

Just for information, because I don't think many people know about this: you can get a list of test cases and their outcomes (pass/fail) in a machine-processable form from the file outcomes.csv which can be downloaded from the Artifacts tab of a build. For example here I can list the test cases that are failing in at least one component:

$ awk -F';' <outcomes.csv '$5=="FAIL" {print $3 ";" $4}' | sort | uniq -c | sort -n
     95 test_suite_ecjpake;ECJPAKE round one: KKP1: unknown first point format
     95 test_suite_ecjpake;ECJPAKE round one: KKP1: unknown second point format
     95 test_suite_ecjpake;ECJPAKE round one: KKP2: unknown first point format
     95 test_suite_ecjpake;ECJPAKE round one: KKP2: unknown second point format
     95 test_suite_ecjpake;ECJPAKE round two client: unknown first point format
     95 test_suite_ecjpake;ECJPAKE round two client: unknown second point format
     95 test_suite_ecjpake;ECJPAKE round two server: unknown first point format
     95 test_suite_ecjpake;ECJPAKE round two server: unknown second point format
     96 test_suite_ecp;ECP read binary #2 (zero, invalid first byte)
     96 test_suite_ecp;ECP read binary #5 (non-zero, invalid first byte)

This confirms that indeed it's only negative cases that need to be adjusted because reading a compressed point is no longer invalid.

[gstrauss]

I have confirmed that hostapd and wpa_supplicant (in hostap repo) produce COMPRESSED format in interface

/**
 * crypto_ecdh_get_pubkey - Retrieve public key from ECDH context
 * @ecdh: ECDH context from crypto_ecdh_init() or crypto_ecdh_init2()
 * @inc_y: Whether public key should include y coordinate (explicit form)
 * or not (compressed form)
 * Returns: Binary data f the public key or %NULL on failure
 */
struct wpabuf * crypto_ecdh_get_pubkey(struct crypto_ecdh *ecdh, int inc_y);

and COMPRESSED format is used in hostapd and wpa_supplicant when the second argument is 0:

wpa_supplicant/pasn_supplicant.c:	pubkey = crypto_ecdh_get_pubkey(pasn->ecdh, 0);
src/rsn_supp/wpa.c:		pub = crypto_ecdh_get_pubkey(sm->fils_ecdh, 1);
src/rsn_supp/wpa.c:		pub = crypto_ecdh_get_pubkey(sm->fils_ecdh, 1);
src/rsn_supp/wpa.c:	pub = crypto_ecdh_get_pubkey(sm->owe_ecdh, 0);
src/rsn_supp/wpa.c:	pub = crypto_ecdh_get_pubkey(sm->owe_ecdh, 0);
src/ap/ieee802_11.c:		pub = crypto_ecdh_get_pubkey(sta->fils_ecdh, 1);
src/ap/ieee802_11.c:		pub = crypto_ecdh_get_pubkey(sta->fils_ecdh, 1);
src/ap/ieee802_11.c:	pubkey = crypto_ecdh_get_pubkey(sta->pasn->ecdh, 0);
src/ap/ieee802_11.c:	pub = crypto_ecdh_get_pubkey(sta->owe_ecdh, 0);
src/ap/ieee802_11.c:		pub = crypto_ecdh_get_pubkey(sta->owe_ecdh, 0);
src/ap/ieee802_11.c:		pub = crypto_ecdh_get_pubkey(sta->owe_ecdh, 0);
src/ap/ieee802_11.c:		pub = crypto_ecdh_get_pubkey(sta->owe_ecdh, 0);
src/common/dpp_crypto.c:	pub = crypto_ecdh_get_pubkey(pfs->ecdh, 0);

I'll try to update the PR this weekend. Thank you for the testing hints, @gilles-peskine-arm

[gilles-peskine-arm]

I have confirmed that hostapd and wpa_supplicant (in hostap repo) produce COMPRESSED format (…)

Thanks for looking into this! Since there's a popular piece of software that produces the compressed format, I agree that it's good for Mbed TLS to accept it.

However, I do not see a justification to add it in the long-time support branch. Mbed TLS 2.28.0 already did not work with hostapd. Hostapd interoperability is a new feature that will require at least Mbed TLS 3.3.

[gstrauss]

Mbed TLS 2.28.0 already did not work with hostapd.

FYI: my prototype in openwrt/openwrt#10727 currently requires

• mbedtls >= 2.27.0 for mbedtls_mpi_random()
• mbedtls >= 2.18.0 for mbedtls_ssl_tls_prf()

I am working my way up the stack and am in the process of implementing mbedtls support for hostap/wpa_supplicant EAP, SAE, and DPP. Lower layers already work with mbedlts 2.28.1 and mbedtls 3.2.1. (The prototype includes support -- outside of mbedtls -- for handling the COMPRESSED point format, and was the basis for this PR.)

[gstrauss]

Tests with invalid point encoding beginning 0x01 and 0x05 now return MBEDTLS_ERR_ECP_BAD_INPUT_DATA instead of MBEDTLS_ERR_ECP_FEATURE_UNAVAILABLE

I still need to add some tests beginning with compressed point encodings 0x02 and 0x03 to exercise expected failure cases.

[gstrauss]

Additional tests added and tests pass. Ready for review.

Please note that this PR cherry-picks cleanly onto mbedtls-2.28.1 with the exception of a trivially resolvable merge conflict in tests/suites/test_suite_ecp.data since some Curve448 tests were added between 2.28.1 and 3.x development branch.

The PR is well-contained. It is binary compatible in that it does not expose any new symbols or remove any symbols, and does not change any structure sizes. In Short Weierstrass curves with prime p where p = 3 mod 4, mbedtls_ecp_point_read_binary() handles MBEDTLS_ECP_PF_COMPRESSED binary point format where previously it returned error MBEDTLS_ERR_ECP_FEATURE_UNAVAILABLE

[gstrauss]

@mpg this change resulted the failed tests above:

     if( mbedtls_mpi_get_bit( Y, 0 ) != parity_bit )
-        MBEDTLS_MPI_CHK( mbedtls_mpi_sub_mpi( Y, &grp->P, Y ) );
+        MPI_ECP_INV( Y, Y );

Is my syntax incorrect?
I read through the underlying mbedtls_mpi_inv_mod() and the two args to MPI_ECP_INV may alias.

I force-pushed without that change and Travis CI builds now pass.

---

...I really dislike macros that have a magic implied argument rather than having all args passed.
MPI_ECP_* macros all require in scope existence of mbedtls_ecp_group *grp.
MBEDTLS_MPI_CHK() requires int ret as well as goto label named cleanup:.

[mpg]

Is my syntax incorrect?

Your syntax is correct, my suggestion wasn't, sorry. MPI_ECP_INV() gives the modular inverse (multiplicative), not the modular opposite / negation (aka additive inverse), as I was thinking yesterday when I saw it. I don't know what happened, in all our code when we write inverse we mean multiplicative, so I shouldn't have been confused like that. Sorry for wasting your time.

...I really dislike macros that have a magic implied argument rather than having all args passed.
MPI_ECP_* macros all require in scope existence of mbedtls_ecp_group *grp.
MBEDTLS_MPI_CHK() requires int ret as well as goto label named cleanup:.

You're not alone! There was some disagreement in the team about these macros, and it's not unlikely they may disappear or take a different shape in the future.

[mpg]

I expect the code may end up slightly larger due to the static inline functions, which includes while() loops.

I think compilers are allowed to ignore the inline hint, and hopefully that's what they do when optimizing for size and the function body is larger than a few instructions. Anyway, the only way to know is to measure:

   text    data     bss     dec     hex filename
   9659       4      32    9695    25df ecp-6282-no-macros.o
   9603       4      32    9639    25a7 ecp-6282-using-macros.o
   9343       4      32    9379    24a3 ecp-before-6282.o

So, the code's just a little bit smaller in the new version, but not by much.

[mpg]

Looks good to me. I just had an idea that might save code size, but this time I'll test it myself in order to avoid wasting your time again it case it doesn't work.

[mpg]

Minor: we can now remove this #endif and the next #if.

[gstrauss]

What is the style convention for #if around multiple funcs? When a macro wraps more than a single function, I tend to add a blank line above and below the #if and the #endif. Since such change does not really save any lines of code for the two unrelated functions now next to each other, I'll wait for your response before making further changes.

[gstrauss]

🚀

[mpg]

I just had an idea that might save code size, but this time I'll test it myself in order to avoid wasting your time again it case it doesn't work.

Ok, seems to work this time (at least passing test_suite_ecp) and actually saves some code too:

   text    data     bss     dec     hex filename     
   9343       4      32    9379    24a3 ecp-before.o                                                                                                                                                            
   9659       4      32    9695    25df ecp-6282-no-macros.o                                                                                                                                                  
   9603       4      32    9639    25a7 ecp-6282-using-macros.o                                                                                                                                                                                                                                                                                               
   9531       4      32    9567    255f ecp-after.o 

The idea is that computing X^3 + A X + B (with the special case for A == -3) is something we were already doing when validating public keys, so let's share that code. This is commit 88431c4 in branch 6282 in my fork if you want to cherry-pick it.

[gstrauss]

@mpg I cherry-picked your improvement and I removed the duplicated consecutive preprocessor directives.

[mpg]

LGTM

[mprse]

LGTM

[mpg]

Note: unless I'm mistaken, this PR automatically adds ability to parse EC keys with compressed points in PK parse. However, that's not tested at the moment, so I've created an issue to add tests for that: #6886

Is there anything else that's supposed to automatically work thanks to this PR but that's not tested yet?

[gstrauss]

Is there anything else that's supposed to automatically work thanks to this PR but that's not tested yet?

@mpg These were posted earlier in this PR:

PSA is missing this (and missing tests). See consecutive comments above:
#6282 (comment)
#6282 (comment)
#6282 (comment)

library/ssl_tls12_{server,client}.c
#6282 (comment)

#861 points to ssl_write_supported_point_formats_ext(), which is duplicated in library/ssl_tls12_client.c and library/ssl_tls12_server.c. Where should that code move to be a single instance rather than duplicated? Should I expand it to advertise support for MBEDTLS_ECP_PF_COMPRESSED?

[mpg]

Thanks, but I don't think those meet the first criterion: supposed work automatically thanks for this PR. I was looking for test gaps relative to the current feature set, not possible feature extensions.

Regarding PSA, this raises questions about portability and the driver API, so this is going to involve deeper discussions.

Regarding TLS, compressed points have been deprecated for more that 3 years now (RFC 8422) so I don't think we are going to ever add support for them in TLS.

[gstrauss]

Regarding PSA, this raises questions about portability and the driver API, so this is going to involve deeper discussions.

@mpg the change to library/psa_crypto_ecp.c is two lines of code plus comment, not including tests.
#6282 (comment)
The reason that patch was not included in this PR is due to PSA test gap of mbedtls not having any specific tests for mbedtls_psa_ecp_load_representation()

[mpg]

Regarding PSA, this raises questions about portability and the driver API, so this is going to involve deeper discussions.

@mpg the change to library/psa_crypto_ecp.c is two lines of code

The number of lines of code it would take is quite irrelevant here. The hard part is https://github.com/ARM-software/psa-crypto-api/issues/565

The reason that patch was not included in this PR is [...]

This might be the reason you didn't include that patch, but I think if you had included we'd have either rejected that part or delayed your PR until https://github.com/ARM-software/psa-crypto-api/issues/565 is resolved, which seems like a worst outcome.

[gilles-peskine-arm]

https://github.com/ARM-software/psa-crypto-api/issues/565 is in a private repository — it's an issue to add support for compressed points to the PSA API. It's not just a trivial matter of saying “if the first byte of a public key is 02 or 03 then …” because PSA can use drivers, and what if the driver doesn't support compressed points? PSA aims to work on minimalistic embedded systems, so we can't just have a blanket requirement to support all formats.