Fix missing cleanup in psa_cipher_setup #72

2019-02-25T10:48:39-06:00

gilles-peskine-arm commented

2019-02-25 10:48:39 -06:00

(Migrated from github.com)

Fix #68.

Add non-regression tests: in hash_setup, mac_setup and cipher_setup, after a failed setup or a successful setup+abort, call setup again with valid parameters and check that it succeeds.

Fix #68. Add non-regression tests: in `hash_setup`, `mac_setup` and `cipher_setup`, after a failed setup or a successful setup+abort, call setup again with valid parameters and check that it succeeds.

Patater (Migrated from github.com) requested changes

2019-02-25 13:07:21 -06:00

library/psa_crypto.c

#68 suspected other setup functions might have issues with leaving around partially initialized contexts. Have our tests proven that they don't? It looks like the tests we've added in this PR just make sure the context is re-usable, not that we don't leave a partially initialized context around if setup fails.

tests/suites/test_suite_psa_crypto.function

Previously we could test with a policy of PSA_KEY_USAGE_SIGN | PSA_KEY_USAGE_VERIFY, and did so from the mac_setup() test. Do we still have coverage where we use a policy of PSA_KEY_USAGE_SIGN | PSA_KEY_USAGE_VERIFY? Maybe we should add a "usage" argument to exercise_mac_setup() (and exercise_cipher_setup()).

Previously we could test with a policy of `PSA_KEY_USAGE_SIGN | PSA_KEY_USAGE_VERIFY`, and did so from the `mac_setup()` test. Do we still have coverage where we use a policy of `PSA_KEY_USAGE_SIGN | PSA_KEY_USAGE_VERIFY`? Maybe we should add a "usage" argument to `exercise_mac_setup()` (and `exercise_cipher_setup()`).

tests/suites/test_suite_psa_crypto.function

We used to test here that an application can call abort after setup fails. Do test this elsewhere or not care to test that case anymore?

tests/suites/test_suite_psa_crypto.function

k

NirSonnenschein (Migrated from github.com) reviewed

2019-02-25 14:45:56 -06:00

NirSonnenschein (Migrated from github.com) left a comment

No description provided.

changes look good to me and should probably be sufficient to pass the current compliance tests however I suspect that other setup functions may have this problem. two possible examples I found: 1. psa_cmac_setup - in this function if mbedtls_cipher_setup succeeds and mbedtls_cipher_cmac_starts fails 2. psa_generator_hkdf_setup - if psa_hmac_setup_internal succeeds but subsequesnt call to psa_hash_update fails.

NirSonnenschein (Migrated from github.com) reviewed

2019-02-25 15:03:15 -06:00

library/psa_crypto.c

some things to look at:
psa_cmac_setup - in this function if mbedtls_cipher_setup succeeds and mbedtls_cipher_cmac_starts fails
psa_generator_hkdf_setup - if psa_hmac_setup_internal succeeds but subsequesnt call to psa_hash_update fails.

some things to look at: psa_cmac_setup - in this function if mbedtls_cipher_setup succeeds and mbedtls_cipher_cmac_starts fails psa_generator_hkdf_setup - if psa_hmac_setup_internal succeeds but subsequesnt call to psa_hash_update fails.

gilles-peskine-arm (Migrated from github.com) reviewed

2019-02-25 15:18:19 -06:00

tests/suites/test_suite_psa_crypto.function

Good point. I've changed the {failed-setup; successful-setup} sequences to {failed-setup; abort; failed-setup; successful-setup}.

gilles-peskine-arm (Migrated from github.com) reviewed

2019-02-25 15:20:40 -06:00

tests/suites/test_suite_psa_crypto.function

We have coverage with different policies in mac_policy.

Since this function only tests psa_mac_sign_setup and not psa_mac_verify_setup, it only needs a USAGE_SIGN policy, just like the corresponding cipher test calls cipher_encrypt_setup with a USAGE_ENCRYPT policy. I think having both USAGE_SIGN and USAGE_VERIFY here was a leftover from the old days when MAC functions didn't check policies correctly.

We have coverage with different policies in `mac_policy`. Since this function only tests `psa_mac_sign_setup` and not `psa_mac_verify_setup`, it only needs a `USAGE_SIGN` policy, just like the corresponding cipher test calls `cipher_encrypt_setup` with a `USAGE_ENCRYPT` policy. I think having both `USAGE_SIGN` and `USAGE_VERIFY` here was a leftover from the old days when MAC functions didn't check policies correctly.

gilles-peskine-arm (Migrated from github.com) reviewed

2019-02-25 15:25:33 -06:00

library/psa_crypto.c

The tests hash_setup, mac_setup and cipher_setup check that the context is reusable. That's the only black-box way of testing that the context is properly reinitialized. There's no black-box way to test that a context isn't “partially initialized”: it's either sufficiently pristine for setup or not.

I don't think there's a way to test the case of psa_cmac_setup where mbedtls_cipher_setup succeeds and mbedtls_cipher_cmac_starts fails at the moment. An invalid key size is detected earlier, and an unsupported algorithm for CMAC can't be reached until we add support for Camellia and ARIA to psa_crypto (and at that point we may well allow them with CMAC).

I've deliberately not touched KDF code because the KDF API has upcoming changes and these bad-case tests will need to change accordingly.

The tests `hash_setup`, `mac_setup` and `cipher_setup` check that the context is reusable. That's the only black-box way of testing that the context is properly reinitialized. There's no black-box way to test that a context isn't “partially initialized”: it's either sufficiently pristine for setup or not. I don't think there's a way to test the case of `psa_cmac_setup` where `mbedtls_cipher_setup` succeeds and `mbedtls_cipher_cmac_starts` fails at the moment. An invalid key size is detected earlier, and an unsupported algorithm for CMAC can't be reached until we add support for Camellia and ARIA to `psa_crypto` (and at that point we may well allow them with CMAC). I've deliberately not touched KDF code because the KDF API has upcoming changes and these bad-case tests will need to change accordingly.

NirSonnenschein (Migrated from github.com) reviewed

2019-02-26 01:55:52 -06:00

Patater (Migrated from github.com) reviewed

2019-02-26 02:45:46 -06:00

tests/suites/test_suite_psa_crypto.function

					
				@ -120,6 +203,74 @@ static int construct_fake_rsa_key( unsigned char *buffer,

				    return( len );

Style: funny alignment here

gilles-peskine-arm (Migrated from github.com) reviewed

2019-02-26 04:29:34 -06:00

tests/suites/test_suite_psa_crypto.function

					
				@ -120,6 +203,74 @@ static int construct_fake_rsa_key( unsigned char *buffer,

				    return( len );

I rewrote the history to fix the indentation in the offending commit.

Patater (Migrated from github.com) approved these changes

2019-02-26 04:32:27 -06:00

Patater (Migrated from github.com) left a comment

LGTM

Only change is indentation since @NirSonnenschein's review.

LGTM Only change is indentation since @NirSonnenschein's review.

dgreen-arm (Migrated from github.com) approved these changes

2019-02-26 05:02:07 -06:00

dgreen-arm (Migrated from github.com) left a comment

LGTM

Sign in to join this conversation.

No reviewers

Patater

dgreen-arm

No labels

HEADER FORMATTING ISSUE

help wanted

Information

invalid

Jira status: AWAITING REVIEW

Jira status: CLOSED

Jira status: DONE

Jira status: IN PROGRESS

Jira status: IN REVIEW

Jira status: OPEN

Jira status: REOPENED

Jira status: RESOLVED

open for community contribution

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-ARMmbed/mbed-crypto!72

				`@ -120,6 +203,74 @@ static int construct_fake_rsa_key( unsigned char *buffer,`
				`return( len );`

Rows
Columns