Use PSA EC-JPAKE in TLS (1.2) #5886

superna9999 · 2022-05-31T12:58:41Z

Description

TLS 1.2 has optional support for a key exchange based on EC J-PAKE, defined as part of the Thread standard, see MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED. Currently the implementation always uses the mbedtls_ecjpake API regardless of whether MBEDTLS_USE_PSA_CRYPTO is enabled.

This task it to make it use the PSA API when MBEDTLS_USE_PSA_CRYPTO is enabled.

Resolves #5847

Status

IN DEVELOPMENT

Requires Backporting

NO

Migrations

NO

Additional comments

Depends on #6115 to be merged

Todos

Tests

Steps to test or reproduce

ssl-opt.sh must run clean

mpg · 2022-09-13T10:47:32Z

Next time you update this, can you also remove the line about EC J-PAKE in docs/use-psa-crypto.md?

mpg · 2022-09-16T08:19:14Z

Note: the failure in ABI-API checking is not a problem, see #6115 (comment)

mpg · 2022-09-28T09:40:27Z

Now that #6115 has been merged, this should be rebased. @superna9999 Can you take care of this? Otherwise please let us know and we'll handle it.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>

superna9999 · 2022-09-28T09:50:01Z

@mpg done!

mpg · 2022-09-28T10:02:45Z

Great, thanks!

mpg

I only found very minor things by reviewing, but did some more testing and the results were not good, see my next message.

mpg · 2022-09-30T09:24:21Z

library/ssl_tls.c

+        psa_role = PSA_PAKE_ROLE_CLIENT;
+
+
+    if( pw_len > 0 )


Minor: unless I'm missing something, we could merge this we the next if( pw_len > 0 ) block. This would avoid the need to destroy the key in the failure path of the next two blocks, and keep related things together. (While at it, I think we could make attributes local to that block as well.)

mpg · 2022-09-30T09:29:40Z

library/ssl_tls.c

+#if defined(MBEDTLS_USE_PSA_CRYPTO)
+    psa_pake_abort( &handshake->psa_pake_ctx );
+    psa_destroy_key( handshake->psa_pake_password );
+    handshake->psa_pake_password = MBEDTLS_SVC_KEY_ID_INIT;


Note: not setting psa_pake_ctx_is_ok is correct because it's going to be done by the call to zeroize() near the end of this function.

mpg · 2022-09-30T09:38:52Z

library/ssl_tls.c

@@ -5731,6 +5807,55 @@ static int ssl_compute_master( mbedtls_ssl_handshake_params *handshake,
    else
 #endif
    {
+#if defined(MBEDTLS_USE_PSA_CRYPTO) &&                              \


Partially pre-existing: with the new EC J-PAKE block added, the structure become hard to follow:

if( direct_psk_to_ms ) { /* get MS directly */ } else { if( ecjpake ) { /* compute PMS, dozens of line */ } // here I have lost track of where in are in the control flow /* derive MS from PMS */ }

I think we could directly return success at the end of the "get MS directly" block, saving one nesting level (no else).

mpg · 2022-09-30T09:41:31Z

library/ssl_tls.c

+                return( MBEDTLS_ERR_SSL_HW_ACCEL_FAILED );
+            }
+        }
+#endif


Minor and partially pre-existing (the few lines below): I find it weird that we are first computing the MS from the PMS, and only then printing out the PMS. It's a bit worse IMO with the addition of this new block: now it's (1) compute the PMS, (2) compute the MS, (3) print the PMS, while 1-3-2 would seem more natural to me.

mpg · 2022-09-30T09:42:24Z

library/ssl_tls.c

@@ -5769,6 +5894,7 @@ int mbedtls_ssl_derive_keys( mbedtls_ssl_context *ssl )
        return( ret );
    }

+


Minor: seems spurious to me, we usually don't leave two blank lines in a row.

mpg · 2022-09-30T09:46:54Z

library/ssl_tls12_client.c

+            {
+                status = psa_pake_output( &ssl->handshake->psa_pake_ctx,
+                                          step, p + 2 + output_offset,
+                                          end - p - output_offset - 2,


Minor: I think p - 2 - output_offset would be slightly better for consistency (and as it happens, alignment) with the line above.

mpg · 2022-09-30T10:03:45Z

library/ssl_tls12_client.c

+            output_offset += output_len;
+        }
+
+        content_len = output_offset;


Question: why do we stop there instead of also computing the PMS as we do in the non-PSA case? If there's a reason, it's not obvious to me.

Generally speaking, it's a pre-existing issue with the code that the PMS is computed in various places for various key exchanges and this makes things really hard to follow at times. Let's not introduce more variations unless we have a good reason.

Hmm it wasn't necessary before the addition of the custom SHA256 PMS for PAKE, so indeed I may have forgotten a step while migrating and it may explain why it doesn't work with the non-PSA implementation.

mpg · 2022-09-30T10:29:08Z

It's hard to validate correctness (are we using the correct format? computing the expected value for the shared secret? etc), not just for this PR, but for the entire EC J-PAKE line of work, because the key exchange is randomized and we can't use known-answer tests.

So, I guess the only thing we can do is test interop with other implementations. I'm not aware of other implementations of the ECJPAKE key exchange in TLS out there, but now we have two in Mbed TLS: the version that uses PSA and the one that doesn't. Let's see if they can talk to each other:

git checkout -- include/mbedtls/mbedtls_config.h
scripts/config.py set MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED
make clean

make -C programs ssl/ssl_server2 ssl/ssl_client2
cp programs/ssl/ssl_server2 s2_no_use_psa
cp programs/ssl/ssl_client2 c2_no_use_psa


scripts/config.py set MBEDTLS_USE_PSA_CRYPTO
make clean

make -C programs ssl/ssl_server2 ssl/ssl_client2
make -C programs test/udp_proxy test/query_compile_time_config

P_SRV=../s2_no_use_psa tests/ssl-opt.sh -f ECJPAKE
P_CLI=../c2_no_use_psa tests/ssl-opt.sh -f ECJPAKE

And unfortunately the results are not looking good on my machine:

ECJPAKE: client not configured ......................................... PASS
ECJPAKE: server not configured ......................................... PASS
ECJPAKE: working, TLS .................................................. FAIL
  ! bad client exit code (expected 0, got 1)
  ! outputs saved to o-XXX-3.log
ECJPAKE: password mismatch, TLS ........................................ FAIL
  ! pattern 'SSL - Verification of the message MAC failed' MUST be present in the Server output
  ! outputs saved to o-XXX-4.log
ECJPAKE: working, DTLS ................................................. FAIL
  ! bad client exit code (expected 0, got 1)
  ! outputs saved to o-XXX-5.log
ECJPAKE: working, DTLS, no cookie ...................................... FAIL
  ! bad client exit code (expected 0, got 1)
  ! outputs saved to o-XXX-6.log
ECJPAKE: password mismatch, DTLS ....................................... FAIL
  ! pattern 're-using cached ecjpake parameters' MUST be present in the Client output
  ! outputs saved to o-XXX-7.log
ECJPAKE: working, DTLS, nolog .......................................... FAIL
  ! bad client exit code (expected 0, got 1)
  ! outputs saved to o-XXX-8.log
------------------------------------------------------------------------
FAILED (2 / 8 tests (0 skipped))

I don't have time to debug now, I can have a look next week until you beat be to it, @superna9999

Also, based on this, I'm going to advocate that we add an all.sh component doing something similar to the above.

superna9999 · 2022-09-30T13:19:06Z

@mpg thanks, I think I may missed something when adding support for #6115, in the first implementation it worked while developement when I implemented only the client and left the server using non-PSA. But when migrated to the custom KDF I may have missed step.

I'll add the all.sh component to this PR as you suggest.

mpg · 2022-10-28T09:48:01Z

Note: this will need rework once the I/O format of psa_pake_input/output() has been fixed to match the spec, as done in #6390

superna9999 · 2022-10-28T13:24:10Z

Finally I'm lacking time to review and update those PRs, I'm OK if those are taken over by @mprse @AndrzejKurek or @valeriosetti

valeriosetti · 2022-11-03T14:32:31Z

Finally I'm lacking time to review and update those PRs, I'm OK if those are taken over by @mprse @AndrzejKurek or @valeriosetti

I'm currently trying to fix test issues reported by @mpg by rebasing this PR on top of PR #6390. Once I'll have something ready I'll open a new PR to continue this work

superna9999 · 2022-11-04T07:57:23Z

Taken over by @valeriosetti in #6533

superna9999 added needs-work needs-ci Needs to pass CI tests needs-preceding-pr Requires another PR to be merged first labels May 31, 2022

superna9999 force-pushed the 5847-use-psa-pake-in-tls branch 4 times, most recently from d5424e0 to 546840c Compare June 1, 2022 15:14

daverodgman added the priority-medium Medium priority - this can be reviewed as time permits label Jun 6, 2022

mpg removed the needs-ci Needs to pass CI tests label Jun 8, 2022

mpg mentioned this pull request Jun 15, 2022

Expose ECJPAKE through the PSA Crypto API #5871

Merged

1 task

superna9999 force-pushed the 5847-use-psa-pake-in-tls branch from 546840c to 5cc2469 Compare July 21, 2022 08:20

superna9999 force-pushed the 5847-use-psa-pake-in-tls branch from 5cc2469 to 3f9437d Compare August 31, 2022 13:31

superna9999 force-pushed the 5847-use-psa-pake-in-tls branch from 3f9437d to aee9b3e Compare September 9, 2022 12:38

superna9999 force-pushed the 5847-use-psa-pake-in-tls branch 3 times, most recently from 076d18a to 68e86da Compare September 15, 2022 07:30

superna9999 marked this pull request as ready for review September 15, 2022 07:37

superna9999 added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review and removed needs-work labels Sep 15, 2022

superna9999 force-pushed the 5847-use-psa-pake-in-tls branch from 68e86da to 266e2e7 Compare September 15, 2022 09:44

mpg mentioned this pull request Sep 19, 2022

Driver only hashes wrap-up #6283

Merged

mpg added needs-work and removed needs-preceding-pr Requires another PR to be merged first labels Sep 28, 2022

mpg removed the needs-review Every commit must be reviewed by at least two team members, label Sep 28, 2022

superna9999 added 2 commits September 28, 2022 11:44

Use PSA PAKE API when MBEDTLS_USE_PSA_CRYPTO is selected

714da3f

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>

Remove TLS 1.2 exception about EC J-PAKE and PSA Crypto

f254b85

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>

superna9999 force-pushed the 5847-use-psa-pake-in-tls branch from 266e2e7 to f254b85 Compare September 28, 2022 09:49

mpg self-requested a review September 28, 2022 10:02

mpg requested changes Sep 30, 2022

View reviewed changes

mpg added the needs-preceding-pr Requires another PR to be merged first label Oct 28, 2022

mpg mentioned this pull request Oct 28, 2022

Fix ecjpake PSA format #6390

Merged

valeriosetti mentioned this pull request Nov 3, 2022

Use PSA EC-JPAKE in TLS (1.2) - Part 2 #6533

Merged

3 tasks

superna9999 closed this Nov 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use PSA EC-JPAKE in TLS (1.2) #5886

Use PSA EC-JPAKE in TLS (1.2) #5886

superna9999 commented May 31, 2022 •

edited

Loading

mpg commented Sep 13, 2022

mpg commented Sep 16, 2022

mpg commented Sep 28, 2022

superna9999 commented Sep 28, 2022

mpg commented Sep 28, 2022

mpg left a comment

mpg Sep 30, 2022

mpg Sep 30, 2022

mpg Sep 30, 2022

mpg Sep 30, 2022

mpg Sep 30, 2022

mpg Sep 30, 2022

mpg Sep 30, 2022

superna9999 Sep 30, 2022

mpg commented Sep 30, 2022

superna9999 commented Sep 30, 2022

mpg commented Oct 28, 2022

superna9999 commented Oct 28, 2022

valeriosetti commented Nov 3, 2022

superna9999 commented Nov 4, 2022

		@@ -5769,6 +5894,7 @@ int mbedtls_ssl_derive_keys( mbedtls_ssl_context *ssl )
		return( ret );
		}

Use PSA EC-JPAKE in TLS (1.2) #5886

Use PSA EC-JPAKE in TLS (1.2) #5886

Conversation

superna9999 commented May 31, 2022 • edited Loading

Description

Status

Requires Backporting

Migrations

Additional comments

Todos

Steps to test or reproduce

mpg commented Sep 13, 2022

mpg commented Sep 16, 2022

mpg commented Sep 28, 2022

superna9999 commented Sep 28, 2022

mpg commented Sep 28, 2022

mpg left a 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

Choose a reason for hiding this comment

mpg commented Sep 30, 2022

superna9999 commented Sep 30, 2022

mpg commented Oct 28, 2022

superna9999 commented Oct 28, 2022

valeriosetti commented Nov 3, 2022

superna9999 commented Nov 4, 2022

superna9999 commented May 31, 2022 •

edited

Loading