Use switch statement instead if-else in psa_aead_check_nonce_length() and psa_aead_set_lengths(). Fixes #5065 #5072

mprse · 2021-10-13T13:35:44Z

Description

Without this fix added test component (component_test_psa_crypto_config_chachapoly_disabled) fails as follows:

Build warning:
psa_crypto.c: In function ‘psa_aead_check_nonce_length’:
psa_crypto.c:3654:1: error: control reaches end of non-void function [-Werror=return-type]

Test failures:

FAILED (1173 / 1225 tests (131 skipped))

e.g.:
PSA Multipart State Checks, AES - GCM ............................. FAILED
  ( ( psa_aead_set_lengths( &operation, additional_data->len, input_data->len ) ) ) == ( ((psa_status_t)0) )
  at line 4746, suites/test_suite_psa_crypto.function

PSA AEAD decrypt: AES-CCM, invalid nonce length 6 ................. FAILED
  ( status ) == ( expected_result )
  at line 3773, suites/test_suite_psa_crypto.function

With the fix all tests pass for component_test_psa_crypto_config_chachapoly_disabled and there is no compilator warning.

Status

READY

Requires Backporting

No

Migrations

NO

Additional comments

I'm not sure if the name and description of the added test component is consistent with the requirements.

…PSA_WANT_ALG_CHACHA20_POLY1305 Signed-off-by: Przemyslaw Stekiel <[email protected]>

…th and psa_aead_set_lengths (fixes Mbed-TLS#5065) Signed-off-by: Przemyslaw Stekiel <[email protected]>

Signed-off-by: Przemyslaw Stekiel <[email protected]>

gilles-peskine-arm

Some minor documentation issues, other than that LGTM

gilles-peskine-arm · 2021-10-14T18:42:36Z

ChangeLog.d/issue5065.txt

+   * Use switch statement instead if-else in
+     psa_aead_check_nonce_length()
+     and psa_aead_set_lengths(). Fixes #5065.


Changelog entries are for users of the library, not for its developers. (Developers have the git commit messages.) “Use switch statement” is an implementation detail, it isn't relevant for users. Instead, the changelog entry should explain the consequence of the bug. For example:

Fix compile-time or run-time errors in PSA AEAD functions when ChachaPoly is disabled. Fixes #5065.

Good point.

gilles-peskine-arm · 2021-10-14T18:49:55Z

tests/scripts/all.sh

@@ -1596,6 +1596,19 @@ component_test_psa_crypto_config_no_driver() {
    make test
 }

+component_test_psa_crypto_config_chachapoly_disabled() {
+    # full - MBEDTLS_CHACHAPOLY_C without PSA_WANT_ALG_GCM and PSA_WANT_ALG_CHACHA20_POLY1305
+    msg "build: full - MBEDTLS_CHACHAPOLY_C without PSA_WANT_ALG_GCM and PSA_WANT_ALG_CHACHA20_POLY1305"


The description says that MBEDTLS_CHACHAPOLY_C is enabled, but the code disables it. The status of MBEDTLS_CHACHAPOLY_C is irrelevant for this test. So please remove it from both the description and the code. This is full minus GCM and Chachapoly for PSA.

The description says that MBEDTLS_CHACHAPOLY_C is disabled (full -(minus) MBEDTLS_CHACHAPOLY_C).

I think it is relevant for the test as without this I wasn't able to reproduce the issue( with BEDTLS_CHACHAPOLY_C PSA_WANT_ALG_CHACHA20_POLY1305 is always defined):
https://github.com/ARMmbed/mbedtls/blob/2bb5e9c973cf7a37eacb50a65537c921b60f2fac/include/mbedtls/config_psa.h#L647-L650

Changed - to minus in description.

The status of MBEDTLS_CHACHAPOLY_C is irrelevant, but the status of PSA_WANT_ALG_CHACHA20_POLY1305 is. There are two ways you can have PSA_WANT_ALG_CHACHA20_POLY1305 enabled:

MBEDTLS_CHACHAPOLY_C enabled. (PSA chachapoly implemented with Mbed TLS's built-in implementation.)

MBEDTLS_PSA_CRYPTO_CONFIG enabled and MBEDTLS_PSA_ACCEL_ALG_CHACHA20_POLY1305 enabled. (PSA chachapoly implemented with a third-party driver.)

The test needs to pick one, but it's not particularly important which one it picks.

Signed-off-by: Przemyslaw Stekiel <[email protected]>

mprse · 2021-10-18T06:06:49Z

It seems that backport to 2.xis not needed.

gilles-peskine-arm · 2021-10-18T08:40:40Z

The bug is in a part of the code that doesn't exist in 2.x, so there's no fix to backport. But please backport the new test.

mprse · 2021-10-18T09:50:44Z

Created backport PR #5083

gilles-peskine-arm · 2021-10-18T15:47:14Z

The latest pr-merge failed due to the known intermittent issue #3377. https://jenkins-mbedtls.oss.arm.com/blue/organizations/jenkins/mbed-tls-pr-merge/detail/PR-5072-merge/6/pipeline/ passed.

paul-elliott-arm

LGTM

backport 2.x: backport only new test in all.sh from #5072

The original goal (Mbed-TLS#5072) was to run a test with ChaChaPoly disabled in PSA. It was actually implemented with GCM also partially disabled (legacy GCM enabled but PSA GCM disabled), which distracted from the objective. It's actually useful to test both with and without GCM, so test both. Don't test inconsistencies between legacy and PSA support because that's not a common case and not one we have particular reasons to test. Signed-off-by: Gilles Peskine <[email protected]>

mprse added 2 commits October 13, 2021 13:27

all.sh: add full - MBEDTLS_CHACHAPOLY_C without PSA_WANT_ALG_GCM and …

1ecfdea

…PSA_WANT_ALG_CHACHA20_POLY1305 Signed-off-by: Przemyslaw Stekiel <[email protected]>

psa_crypto.c: use switch instead if-else in psa_aead_check_nonce_leng…

4cad4fc

…th and psa_aead_set_lengths (fixes Mbed-TLS#5065) Signed-off-by: Przemyslaw Stekiel <[email protected]>

mprse added bug needs-review Every commit must be reviewed by at least two team members, needs-backports Backports are missing or are pending review and approval. needs-reviewer This PR needs someone to pick it up for review labels Oct 13, 2021

mprse force-pushed the issue_5065 branch from 31601e3 to eba4309 Compare October 13, 2021 13:39

mprse mentioned this pull request Oct 13, 2021

PSA AEAD code doesn't work when Chachapoly is disabled #5065

Closed

mprse force-pushed the issue_5065 branch from eba4309 to 80268fa Compare October 14, 2021 06:41

Add change-log file (issue Mbed-TLS#5065)

ed61c5e

Signed-off-by: Przemyslaw Stekiel <[email protected]>

mprse force-pushed the issue_5065 branch from 80268fa to ed61c5e Compare October 14, 2021 07:14

gilles-peskine-arm requested changes Oct 14, 2021

View reviewed changes

Address review comments

316c4fa

Signed-off-by: Przemyslaw Stekiel <[email protected]>

gilles-peskine-arm approved these changes Oct 15, 2021

View reviewed changes

mprse mentioned this pull request Oct 18, 2021

backport 2.x: backport only new test in all.sh from #5072 #5083

Merged

AndrzejKurek self-requested a review October 18, 2021 15:03

gilles-peskine-arm removed the needs-reviewer This PR needs someone to pick it up for review label Oct 18, 2021

paul-elliott-arm approved these changes Oct 18, 2021

View reviewed changes

paul-elliott-arm removed the needs-review Every commit must be reviewed by at least two team members, label Oct 18, 2021

gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-backports Backports are missing or are pending review and approval. labels Oct 18, 2021

gilles-peskine-arm merged commit bf21c07 into Mbed-TLS:development Oct 18, 2021

paul-elliott-arm added the needs-backports Backports are missing or are pending review and approval. label Oct 18, 2021

gilles-peskine-arm added a commit that referenced this pull request Oct 18, 2021

Merge pull request #5083 from mprse/issue_5065_2x

a686bf4

backport 2.x: backport only new test in all.sh from #5072

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use switch statement instead if-else in psa_aead_check_nonce_length() and psa_aead_set_lengths(). Fixes #5065 #5072

Use switch statement instead if-else in psa_aead_check_nonce_length() and psa_aead_set_lengths(). Fixes #5065 #5072

mprse commented Oct 13, 2021 •

edited

Loading

gilles-peskine-arm left a comment

gilles-peskine-arm Oct 14, 2021

mprse Oct 15, 2021

gilles-peskine-arm Oct 14, 2021

mprse Oct 15, 2021

mprse Oct 15, 2021

gilles-peskine-arm Oct 15, 2021

mprse commented Oct 18, 2021

gilles-peskine-arm commented Oct 18, 2021

mprse commented Oct 18, 2021

gilles-peskine-arm commented Oct 18, 2021

paul-elliott-arm left a comment

Use switch statement instead if-else in psa_aead_check_nonce_length() and psa_aead_set_lengths(). Fixes #5065 #5072

Use switch statement instead if-else in psa_aead_check_nonce_length() and psa_aead_set_lengths(). Fixes #5065 #5072

Conversation

mprse commented Oct 13, 2021 • edited Loading

Description

Status

Requires Backporting

Migrations

Additional comments

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm Oct 14, 2021

Choose a reason for hiding this comment

mprse Oct 15, 2021

Choose a reason for hiding this comment

gilles-peskine-arm Oct 14, 2021

Choose a reason for hiding this comment

mprse Oct 15, 2021

Choose a reason for hiding this comment

mprse Oct 15, 2021

Choose a reason for hiding this comment

gilles-peskine-arm Oct 15, 2021

Choose a reason for hiding this comment

mprse commented Oct 18, 2021

gilles-peskine-arm commented Oct 18, 2021

mprse commented Oct 18, 2021

gilles-peskine-arm commented Oct 18, 2021

paul-elliott-arm left a comment

Choose a reason for hiding this comment

mprse commented Oct 13, 2021 •

edited

Loading