Mbed Crypto Tests #1

itayzafrir · 2018-11-12T15:27:09Z

Description

Add mbed-crypto basic functionality tests to mbed-os/TESTS directory.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[x] Test update
[ ] Breaking change

Add tests setup code and random generator test.

Patater · 2018-11-12T15:36:28Z

TESTS/mbed-crypto/sanity/main.cpp

@@ -51,7 +82,8 @@ utest::v1::status_t test_setup(const size_t number_of_cases)
 }

 Case cases[] = {
-    Case("mbed-crypto random", test_crypto_random, case_failure_handler)
+    Case("mbed-crypto random", test_crypto_random, case_failure_handler),
+    Case("mbed-crypto asymmetric encrypt/decrypt", test_crypto_asymmetric_encrypt_decrypt, case_failure_handler)


Does it break anything to always include the comma at the end of the line here?

If we can always include commas at the end, the diffs will be more clean (won't look like we are adding new tests that were already added in a previous commit).

I'm not sure I'll check if it breaks. I can also add a line break before the closing } of the cases array instead, then it will prevent the false appearance.

I'll fix this once I raise an actual PR to mbed-os. I'll either rebase this branch or create a completely new branch.

Patater · 2018-11-12T15:48:14Z

TESTS/mbed-crypto/sanity/main.cpp

+{
+    psa_algorithm_t alg = PSA_ALG_SHA_256;
+    psa_hash_operation_t operation;
+    unsigned char hash[] = {


What is this a hash of? Include a comment in the code.

OK will do.

Patater · 2018-11-12T15:49:13Z

TESTS/mbed-crypto/sanity/main.cpp

+                                                     encrypted, sizeof(encrypted), &output_len));
+    TEST_ASSERT_EQUAL(PSA_SUCCESS, psa_cipher_finish(&operation, encrypted + output_len,
+                                                     sizeof(encrypted) - output_len, &output_len));
+    TEST_ASSERT_EQUAL(PSA_SUCCESS, psa_cipher_abort(&operation));


We should add another TEST_ASSERT_EQUAL_UINT8_ARRAY here, to check that the encrypted value matches what we expect the data to get encrypted to.

Otherwise, this test would pass if the buffer was never encrypted.

I know, but since the key is generated rather than imported I don't see how this can be done. The jira tasks states that the keys should be generated.

We have a separate key generation test. Not all tests must use generated keys.

I'll change this to use a fixed imported key.

Changed to deterministic.

Patater · 2018-11-12T15:51:07Z

TESTS/mbed-crypto/sanity/main.cpp

+    psa_algorithm_t alg = PSA_ALG_RSA_PKCS1V15_CRYPT;
+    size_t key_bits = 512;
+    psa_key_policy_t policy;
+    unsigned char input[100];


Could we test with a fixed input so we can check the result of the signature matches what we expect? Note that this would only work for deterministic signature algorithms.

Same comment as before, the key is generated.

The signature test doesn't need to use a generated key.

Ok I'll change this to use a fixed imported key then.

Changed to deterministic.

Add the test to the test cases array, this was missed previously. Fix the buffer sizes in the test.

itayzafrir

@Patater I've updated the following tests to be deterministic:

test_crypto_symmetric_cipher_encrypt_decrypt
test_crypto_asymmetric_sign_verify

The test test_crypto_asymmetric_encrypt_decrypt remains as is for now, i.e. uses a generated key and is non-deterministic. Please let me know if you want this one to be deterministic as well.

I've also addressed the rest of the comments in your review.

I haven't rebased the branch to keep the review process easier, once approved I'll either rebase and get rid of some of the commits or create an new branch altogether.

itayzafrir · 2018-11-13T13:46:44Z

TESTS/mbed-crypto/sanity/main.cpp

@@ -51,7 +82,8 @@ utest::v1::status_t test_setup(const size_t number_of_cases)
 }

 Case cases[] = {
-    Case("mbed-crypto random", test_crypto_random, case_failure_handler)
+    Case("mbed-crypto random", test_crypto_random, case_failure_handler),
+    Case("mbed-crypto asymmetric encrypt/decrypt", test_crypto_asymmetric_encrypt_decrypt, case_failure_handler)


I'll fix this once I raise an actual PR to mbed-os. I'll either rebase this branch or create a completely new branch.

itayzafrir · 2018-11-13T13:47:10Z

TESTS/mbed-crypto/sanity/main.cpp

+{
+    psa_algorithm_t alg = PSA_ALG_SHA_256;
+    psa_hash_operation_t operation;
+    unsigned char hash[] = {


itayzafrir · 2018-11-13T13:47:37Z

TESTS/mbed-crypto/sanity/main.cpp

+                                                     encrypted, sizeof(encrypted), &output_len));
+    TEST_ASSERT_EQUAL(PSA_SUCCESS, psa_cipher_finish(&operation, encrypted + output_len,
+                                                     sizeof(encrypted) - output_len, &output_len));
+    TEST_ASSERT_EQUAL(PSA_SUCCESS, psa_cipher_abort(&operation));


Changed to deterministic.

itayzafrir · 2018-11-13T13:47:50Z

TESTS/mbed-crypto/sanity/main.cpp

+    psa_algorithm_t alg = PSA_ALG_RSA_PKCS1V15_CRYPT;
+    size_t key_bits = 512;
+    psa_key_policy_t policy;
+    unsigned char input[100];


Changed to deterministic.

alekshex · 2018-11-13T15:55:17Z

TESTS/mbed-crypto/sanity/main.cpp

+        0x28, 0x8d, 0x76, 0xc0, 0xa7, 0x09, 0x50, 0x3f,
+        0x87, 0x96, 0x1e, 0x96, 0x05, 0xcb, 0xb9, 0x6d
+    };
+    unsigned char encrypted[sizeof(input)], decrypted[sizeof(input)], iv[sizeof(input)];


@itayzafrir is decrypted buffer size sizeof(input) or can be size of output, which is bigger?

The input buffer is 16 bytes (one block as this is AES), and since we're using PSA_ALG_CBC_NO_PADDING they should all be the same size.

mohammad1603 · 2018-11-15T08:31:52Z

TESTS/mbed-crypto/sanity/main.cpp

+        0x28, 0x8d, 0x76, 0xc0, 0xa7, 0x09, 0x50, 0x3f,
+        0x87, 0x96, 0x1e, 0x96, 0x05, 0xcb, 0xb9, 0x6d
+    };
+    unsigned char encrypted[sizeof(input)], decrypted[sizeof(input)], iv[sizeof(input)];


In case the input buffer will be changed in the future to larger one, this will cause to invalid IV size.
I think it should be hard-coded to 16.

Patater · 2018-11-15T10:06:33Z

TESTS/mbed-crypto/sanity/main.cpp

+    TEST_ASSERT_EQUAL(PSA_SUCCESS, psa_generate_key(slot, key_type, key_bits, NULL, 0));
+    TEST_ASSERT_EQUAL(PSA_SUCCESS, psa_get_key_information(slot, NULL, &got_bits));
+    TEST_ASSERT_EQUAL(key_bits, got_bits);
+    TEST_ASSERT_EQUAL(PSA_SUCCESS, psa_asymmetric_encrypt(slot, alg, input, sizeof(input), NULL, 0,


I think we've talked about this before, but "encrypt and then decrypt" is a bad example, as nobody will do that in practice. Encrypt should be an example. Decrypt should be a separate example.

This applies to both asymmetric and symmetric operations.

I agree for an example, but this is a test. Our test suite also has tests which encrypt and decrypt:

https://github.com/ARMmbed/mbedtls-psa/blob/c11be0943e0700526df95071208de0fd2d063baf/tests/suites/test_suite_psa_crypto.function#L2485

https://github.com/ARMmbed/mbedtls-psa/blob/c11be0943e0700526df95071208de0fd2d063baf/tests/suites/test_suite_psa_crypto.function#L244

OK, fine to make our tests better later.

Patater · 2018-11-15T10:06:43Z

TESTS/mbed-crypto/sanity/main.cpp

+{
+    psa_algorithm_t alg = PSA_ALG_SHA_256;
+    psa_hash_operation_t operation;
+    unsigned char hash[] = {


Patater · 2018-11-15T10:07:37Z

TESTS/mbed-crypto/sanity/main.cpp

+                                                     encrypted, sizeof(encrypted), &output_len));
+    TEST_ASSERT_EQUAL(PSA_SUCCESS, psa_cipher_finish(&operation, encrypted + output_len,
+                                                     sizeof(encrypted) - output_len, &output_len));
+    TEST_ASSERT_EQUAL(PSA_SUCCESS, psa_cipher_abort(&operation));


Patater · 2018-11-15T10:08:44Z

TESTS/mbed-crypto/sanity/main.cpp

+    psa_key_type_t key_type = PSA_KEY_TYPE_RSA_KEYPAIR;
+    psa_algorithm_t alg = PSA_ALG_RSA_PKCS1V15_SIGN_RAW;
+    psa_key_policy_t policy;
+    unsigned char key[] = {


Consider making this, and other large constant arrays, static const.

What do you consider a large array?

What do you consider to be excessive stack usage for an embedded target?

Changed all constant arrays to static const

Patater

OK

itayzafrir · 2018-11-15T15:33:21Z

New PR @ ARMmbed#8754

catch up from master

connections

Finish memory protection and add static assert

itayzafrir added 6 commits November 11, 2018 18:27

Introduce mbed-crypto tests

a5e6855

Add tests setup code and random generator test.

Add mbed-crypto asymmetric encrypt decrypt test

3af7d5a

Add mbed-crypto hash verify test

25d0dc6

Add mbed-crypto symmetric cipher encrypt decrypt test

832b8dd

Add mbed-crypto asymmetric sign verify test

510496e

Add mbed-crypto key derivation test

85406ae

Patater reviewed Nov 12, 2018

View reviewed changes

itayzafrir added 4 commits November 13, 2018 11:03

Add inline documentation

0f9ab82

Fix test test_crypto_asymmetric_sign_verify

a034844

Add the test to the test cases array, this was missed previously. Fix the buffer sizes in the test.

Change symmetric cipher encrypt decrypt test to be deterministic

95b1c60

Change symmetric cipher encrypt decrypt test to be deterministic

18e11af

itayzafrir commented Nov 13, 2018

View reviewed changes

alekshex reviewed Nov 13, 2018

View reviewed changes

mohammad1603 suggested changes Nov 15, 2018

View reviewed changes

Hard code IV size in test_crypto_symmetric_cipher_encrypt_decrypt

077c624

mohammad1603 approved these changes Nov 15, 2018

View reviewed changes

Patater reviewed Nov 15, 2018

View reviewed changes

Change all constant arrays to static const

f5243a9

Patater approved these changes Nov 15, 2018

View reviewed changes

itayzafrir closed this Nov 15, 2018

NirSonnenschein pushed a commit that referenced this pull request Nov 26, 2018

Merge pull request #1 from ARMmbed/master

614d058

catch up from master

itayzafrir pushed a commit that referenced this pull request Dec 4, 2018

Merge pull request #1 from paul-szczepanek-arm/vincent

b6c7bd7

connections

itayzafrir pushed a commit that referenced this pull request Mar 18, 2019

Merge pull request #1 from orenc17/flash_protected_region_config

b3f59da

Finish memory protection and add static assert

Mbed Crypto Tests #1

Mbed Crypto Tests #1

Uh oh!

Conversation

itayzafrir commented Nov 12, 2018

Description

Pull request type

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itayzafrir left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itayzafrir Nov 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itayzafrir left a comment •

edited

Loading

itayzafrir Nov 13, 2018 •

edited

Loading