Unsupported feature support #162

RonEld · 2019-07-09T13:23:41Z

This PR adds support for MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED in tests and applications.
This supersedes the crypto part of Mbed-TLS/mbedtls#2124

RonEld · 2019-07-14T12:55:43Z

The CI failure is because the changes in platform.h and in helpers.function related to inclusion of platform.h should be merged to the TLs repository as well. (This is done in Mbed-TLS/mbedtls#2124)

RonEld · 2019-09-09T15:09:24Z

I have rebased to resolved conflicts, fixed a bug in the tests, and added support to new tests

Patater · 2019-09-09T16:28:36Z

library/aria.c

+        if( ret == MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED )
+        {
+            if( verbose )
+                mbedtls_printf( "skipped\n" );


This seems a bit funny. What's the point of an ARIA self-test if even ECB isn't supported? I understand that mechanically we want to check for this error and skip, but I can't imagine a valid use case. No change requested here, just an odd observation.

I guess some hardware might not expose ECB, but may expose other modes.

I agree, but we can't predict when a platform will return this error, and we allow returning this error for any crypto operation

A platform could support a cipher in only one direction. This is most common in the encrypt direction, which is all you need for many modern AEAD modes (e.g. CCM, GCM). This could also be in the decrypt direction only for an accelerator that's only ever meant for CBC decryption, but that's a lot rarer.

Patater · 2019-09-09T16:34:21Z

library/chacha20.c

+                mbedtls_printf( "skipped\n" );
+            continue;
+        }
+        else ASSERT( 0 == ret, ( "error code: %i\n", ret ) );


Suggest to put ASSERT on a new line and to enclose with {} explicitly, in case the macro ever goes multi-line this is easier to maintain

Patater · 2019-09-10T11:02:35Z

library/pkparse.c

+        if( ret == MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED )
+        {
+            ret = MBEDTLS_ERR_PK_INVALID_PUBKEY |
+                  MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED;


Can these be or'd together like this? Do users use them as bitfields?

The Or here is basically the high level and the low level. If I remember correct, the MBEDTLS_ERR_PK_INVALID_PUBKEY is still used in caller functions

In principle, yes: an error code from a “high-level” module of Mbed TLS can be either a high-level error code, or a bitwise-or of a high-level error code and a low-level error code. See error.h.

In practice, I've seen bugs before from code expecting a high-level error code and getting one bitwise-ored with a low-level error code. So I wouldn't be surprised if this broke some existing code. We need to document this change clearly and in a way that people will find. This used to be reasonable code (abbreviated), but no longer is:

switch (mbedtls_pk_parse_key(…)) { case 0: do stuff; break; case MBEDTLS_ERR_PK_INVALID_PUBKEY: /*algorithm not supported, try the next key*/ continue; default: printf("Fatal error parsing key. Giving up.\n"); exit(1); }

Patater · 2019-09-10T11:03:16Z

library/pkparse.c

                 */
-                if( ret != MBEDTLS_ERR_ECP_FEATURE_UNAVAILABLE )
+                if( ret == MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED )


Should this be (ret & MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED) if ret is a bitfield now?

Probably. But I f remember correct, when I tested this, in this specific branch, it was always MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED. But better to check the low leverl error

I am not sure this is needed. The low level driver could return MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED, which in this case it is returned from pk_get_ecpubkey(). In this check, if it is unsupported, we modify the code to MBEDTLS_ERR_PK_KEY_INVALID_FORMAT | MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED

Patater · 2019-09-10T11:04:43Z

tests/suites/helpers.function

-          test_fail( #TEST, __LINE__, __FILE__ );           \
-          goto exit;                                        \
-       }                                                    \
+


Don't add this newline

Patater · 2019-09-10T11:05:29Z

tests/suites/helpers.function

+    do {                                                                      \
+        if( ! (TEST) )                                                        \
+        {                                                                     \
+            set_test_result( #TEST, __LINE__, __FILE__, TEST_RESULT_FAILED ); \


Why not keep using test_fail() and test_skip()?

in order not to duplicate code.
I believe this came from this comment

Patater · 2019-09-10T11:25:55Z

tests/suites/helpers.function

@@ -423,17 +461,11 @@ jmp_buf jmp_tmp;
 /*----------------------------------------------------------------------------*/
 /* Helper Functions */

-void test_fail( const char *test, int line_no, const char* filename )


Instead of deleting test_fail, we could have it be a static function that calls set_test_result()

initially set_test_result() was static, but this caused compilation warnings, as declared functions that are not used, in modules that are configured out. The linker probably doesn't remove static functions.
In addition, since I thought putting one single function would make it cleaner

The function would have to be non-static.

This has nothing to do with the linker. It's because we compile with -Wunused-function.

We should think of putting such functions into a separate .c file — #250.

tests/suites/target_test.function

library/aria.c

Patater · 2019-09-10T12:34:23Z

Needs to pass CI

gilles-peskine-arm

This needs some documentation to explain how to write an alternative implementation that doesn't implement everything. Preferably in Mbed Crypto and not in a knowledge base article that languishes unpublished for countless months.

There are incompatible changes: some functions now return different error codes. This needs to be clearly explained in release notes. We currently don't have a changelog for Mbed Crypto, which is a pity.

gilles-peskine-arm · 2019-09-10T12:25:39Z

programs/test/benchmark.c

-    if( ret != 0 )                                                      \
+    if( ret == MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED )               \
+    {                                                                   \
+        mbedtls_printf( "Feature Not Supported. Skipping.\n" );         \


Cosmetic: “Feature not supported. Skipping.”

gilles-peskine-arm · 2019-09-10T12:50:58Z

programs/test/benchmark.c

@@ -458,7 +477,21 @@ int main( int argc, char *argv[] )

            memset( buf, 0, sizeof( buf ) );
            memset( tmp, 0, sizeof( tmp ) );
-            mbedtls_aes_setkey_enc( &aes, tmp, keysize );
+            ret = mbedtls_aes_setkey_enc( &aes, tmp, keysize );
+            if( ret != 0 )


There's a lot of boilerplate here. Please make an auxiliary function, maybe with some wrapper macros.

It's done in Mbed-TLS/mbedtls#2124 . Should I cherry-pick the changes there, or will it be merged to this repository?

Please don't introduce new code that would change when Mbed-TLS/mbedtls#2124 finds its way into Mbed Crypto. Make the same changes to helpers.function here.

gilles-peskine-arm · 2019-09-10T12:56:34Z

programs/test/benchmark.c

            TIME_PUBLIC( title, "sign",
-                    ret = mbedtls_ecdsa_write_signature( &ecdsa, MBEDTLS_MD_SHA256, buf, curve_info->bit_size,
-                                                tmp, &sig_len, myrand, NULL ) );
+                    CHECK_AND_BREAK( mbedtls_ecdsa_write_signature( &ecdsa, MBEDTLS_MD_SHA256, buf, curve_info->bit_size,


Breaking here leaves ecdsa unfreed. I think it's high time to split this code into auxiliary functions, and react to errors with goto exit.

Breaking here leaves ecdsa unfreed.

No it won't. THe break is done within the loop inside TIME_PUBLIC macro., going to next line in this code, which frees the context

gilles-peskine-arm · 2019-09-10T12:59:31Z

library/aes.c

-         * MBEDTLS_AES_ALT is defined.
-         */
-        if( ret == MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED && keybits == 192 )
+        if( ret == MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED )


I haven't reviewed the self-test changes (commit "Skip self tests if feature unsupported"). That's a lot of new boilerplate code. Strongly consider defining at least a helper macro in platform.h.

gilles-peskine-arm · 2019-09-10T13:01:57Z

tests/suites/test_suite_blowfish.function

@@ -235,7 +235,6 @@ void blowfish_encrypt_cbc( data_t * key_str, data_t * iv_str,
                                                 output ), cbc_result );
    if( cbc_result == 0 )
    {
-


The commit "Adjust symmetric tests to TEST_ASSERT_RET" does not do that at all. Please fix the commit description. Furthermore that commit does several unrelated things, so it should be split.

gilles-peskine-arm · 2019-09-10T16:16:46Z

library/pkparse.c

@@ -545,7 +546,18 @@ static int pk_get_rsapubkey( unsigned char **p,

    if( ( ret = mbedtls_rsa_import_raw( rsa, *p, len, NULL, 0, NULL, 0,
                                        NULL, 0, NULL, 0 ) ) != 0 )
-        return( MBEDTLS_ERR_PK_INVALID_PUBKEY );
+    {
+        if( ret == MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED )


I'm a bit lost. When would a function from the rsa module return MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED vs MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED | MBEDTLS_ERR_RSA_xxx?

Why would there be two low level errors? I don't see a place whereMBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED | MBEDTLS_ERR_RSA_xxx is returned.
The previous behavior was returning MBEDTLS_ERR_PK_INVALID_PUBKEY for any failure, now, in case of MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED returned by the rsa module, we still return MBEDTLS_ERR_PK_INVALID_PUBKEY but we also add the low level MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED

gilles-peskine-arm · 2019-09-10T16:17:19Z

tests/suites/test_suite_ecp.function

@@ -1057,9 +1057,6 @@ void mbedtls_ecp_gen_key( int id )
    TEST_ASSERT_RET( mbedtls_ecp_gen_key( id, &key, &rnd_pseudo_rand,
                                          &rnd_info ), 0 );

-    TEST_ASSERT_RET( mbedtls_ecp_check_pubkey( &key.grp, &key.Q ), 0 );


Why have these two lines been removed? It doesn't seem to have anything to do with "Adjust pk tests to TEST_ASSERT_RET".

Probably rebase error. I'll add them

gilles-peskine-arm · 2019-09-10T16:37:20Z

library/aria.c

+        if( ret == MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED )
+        {
+            if( verbose )
+                mbedtls_printf( "skipped\n" );


A platform could support a cipher in only one direction. This is most common in the encrypt direction, which is all you need for many modern AEAD modes (e.g. CCM, GCM). This could also be in the decrypt direction only for an accelerator that's only ever meant for CBC decryption, but that's a lot rarer.

gilles-peskine-arm · 2019-09-10T16:43:23Z

tests/suites/helpers.function

+ *          It receives an expected return code and:
+ *          - If the underlying alternative implementation does not support the feature,
+ *          by returning MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED, the test is skipped.
+ *          - If and fails in case the expression


Grammar is mistake and I'm not even sure what you were trying to say.

Actually it's a rebase error, I'll fix

gilles-peskine-arm · 2019-09-10T16:44:03Z

tests/suites/helpers.function

+ *
+ * \param   TEST    The test expression to be tested.
+ * \param   EXP_RET The expected return code from the function
+ */


Please document when this function jumps (as is done for TEST_ASSERT).

RonEld · 2019-09-11T07:30:53Z

All the failed CI tests are in the tls testing

Patater · 2019-09-11T10:35:51Z

All the failed CI tests are in the tls testing

Which means if we merge this, TLS tests will start failing when they update their submodule. The TLS tests should be passing before merge, or we need to understand how to make them pass again when the submodule is updated.

RonEld · 2019-09-11T10:59:40Z

@Patater
I guess Mbed-TLS/mbedtls#2124 should be merged before this PR then, updating the TLS PR with this branch as the submodule

RonEld · 2019-09-12T11:32:47Z

Which means if we merge this, TLS tests will start failing when they update their submodule. The TLS tests should be passing before merge, or we need to understand how to make them pass again when the submodule is updated.

Since now the CI in Mbed-TLS/mbedtls#2124 pass, I believe that once that PR will be merged, the TLS tests owuld pass here as well

gilles-peskine-arm · 2019-10-29T16:50:42Z

Relabeling as "needs: work" because there are merge conflicts. Let's arrange a time when we're available for review soon after this has been rebased, or agree to include a merge commit.

Patater · 2019-10-29T17:20:05Z

Rebase please, early and often. Each rebase individually will be easier than a giant rebase later. Yes, we don't have a way to view diffs between previous versions of your patchset easily, but that's a GitHub limitation and only an inconvenience during review. History is easier to work with when we don't have upstream to downstream merge commits in short-term branches, the sorts PRs are from, as that e.g. breaks --first-parent from being useful, among other things. Let's not inconvenience folks using the permanent record for the short term sake of review convenience.

Adjust the tests to the new unsupported feature error, skipping test if unsupported.

Skip test in self test, to the next test, when a function returns `MBEDTLS_PLATFORM_FEATURE_UNSUPPORTED`, and result in success.

1. Return error in `crypt_and_hash` in case `mbedtls_cipher_finish()` fails. 2. Support the `MBEDTLS_PLATFORM_FEATURE_UNSUPPORTED` in the benchmark application, to be skipped.

Remove redundant extra lines in the aes and blowfish test functions.

Have the wrapper code for chachapoly functionality return the error code of the underlying function, instead of a generic `MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA`.

Check error codes of `mbedtls_xtea_crypt_ecb` and clean data in case of failure.

1. Change `TEST_ASSERT` to `TEST_ASSERT_RET` in all the pk tests. 2. Have the pk modules return error code of underlying pk functions.

Add an error for Feature Unsupported to be returned to the host, and to be skipped.

Use lccal buffer for plaintext, to support different platforms, that can't access Flash, but only RAM.

Include `platform.h` even if `MBEDTLS_PLATFORM_C` is undefined, to include the feature unsupported error.

Change `TEST_ASSERT` to `TEST_ASSERT_RET` to new `ecdh` tests.

Remove `static` type for `set_test_result` to avoid compilation warnings when this function is not being used.

1. Change `test_fail` to `set_test_result`. 2. Add define of `mbedtls_snprintf` in `benchmark` in case platform is not defined.

Remove redundant newline, and put `ASSERT` macro in a new code block.

1. Change typo in log. 2. Create a macro for the symmetric set key functionality. 3. Check for Feature not supported error in des key setting.

Remove redundant extra lines in the benchmark application.

Add `CHECK_AND_BREAK` for the tests that run inside a timing loop, and add declaration of `mbedtls_snprintf` in case platform doesn't exist.

1. Change log for skipped test as it is not only for platform unsupported. 2. Remove unentered else statement. 3. Remove redundant `exit` label. 4. Revert the changes in the invalid param tests in the pk layer. 5. Update `TEST_ASSERT_RET` description.

Add helper macros for skipping self tests in case the returned error is `MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED`, since htese checks are very similar in all tests.

RonEld · 2019-12-31T12:56:11Z

All the failed tests are due to missing declaration of MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED. only on Mbed builds, probably because this definition is in a header file that wasn't imported yet to Mbed OS

gilles-peskine-arm · 2020-03-24T18:56:14Z

Mbed Crypto has been merged back into Mbed TLS. Due to the nature of this pull request, it does not make sense to apply it to Mbed Crypto separately. Please bring these changes back into Mbed-TLS/mbedtls#2124 .

RonEld added the enhancement New feature or request label Jul 9, 2019

RonEld mentioned this pull request Jul 9, 2019

Feature unavailable unified error support Mbed-TLS/mbedtls#2124

Closed

4 tasks

RonEld force-pushed the unsupported_feature branch 2 times, most recently from e6d54dd to e38049c Compare July 9, 2019 13:43

RonEld changed the title ~~Unsupported feature support~~ WIP: Unsupported feature support Jul 10, 2019

RonEld force-pushed the unsupported_feature branch 5 times, most recently from 53e6745 to 7cb3492 Compare July 14, 2019 07:34

RonEld changed the title ~~WIP: Unsupported feature support~~ Unsupported feature support Jul 14, 2019

RonEld mentioned this pull request Jul 25, 2019

Port the cryptocell 310 cmac driver ARMmbed/mbed-os#10947

Merged

RonEld force-pushed the unsupported_feature branch from 7cb3492 to beb3b88 Compare September 9, 2019 15:08

RonEld force-pushed the unsupported_feature branch from 824f645 to 76b0956 Compare September 10, 2019 07:26

Patater suggested changes Sep 10, 2019

View reviewed changes

Patater added needs: ci Needs a passing full CI run needs: review The pull request is ready for review. This generally means that it has no known issues. labels Sep 10, 2019

gilles-peskine-arm mentioned this pull request Sep 10, 2019

Support auxiliary code files in the unit test framework #250

Closed

gilles-peskine-arm requested changes Sep 10, 2019

View reviewed changes

RonEld mentioned this pull request Sep 24, 2019

Fix on target test issues #278

Merged

gilles-peskine-arm added the needs: work The pull request needs rework before it can be merged. label Oct 31, 2019

RonEld force-pushed the unsupported_feature branch 4 times, most recently from 16b47fa to 026eacf Compare October 31, 2019 15:29

Ron Eldor added 19 commits December 29, 2019 16:34

Adjust tests to unsupported feature error

1156ebd

Adjust the tests to the new unsupported feature error, skipping test if unsupported.

Skip self tests if feature unsupported

cf00902

Skip test in self test, to the next test, when a function returns `MBEDTLS_PLATFORM_FEATURE_UNSUPPORTED`, and result in success.

Support error codes in sample applications

5c95f6f

1. Return error in `crypt_and_hash` in case `mbedtls_cipher_finish()` fails. 2. Support the `MBEDTLS_PLATFORM_FEATURE_UNSUPPORTED` in the benchmark application, to be skipped.

Remove redundant extra lines

3d39811

Remove redundant extra lines in the aes and blowfish test functions.

Return the correct error code for chachapoly wrap

b015b2b

Have the wrapper code for chachapoly functionality return the error code of the underlying function, instead of a generic `MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA`.

Fixes for mbedtls_xtea_crypt_cbc

b82757c

Check error codes of `mbedtls_xtea_crypt_ecb` and clean data in case of failure.

Adjust pk tests to TEST_ASSERT_RET

d3f88c7

1. Change `TEST_ASSERT` to `TEST_ASSERT_RET` in all the pk tests. 2. Have the pk modules return error code of underlying pk functions.

Adapt On Target Tests to Feature Unsupported

41fd82b

Add an error for Feature Unsupported to be returned to the host, and to be skipped.

Enforce plaintext to be on stack

245d8a9

Use lccal buffer for plaintext, to support different platforms, that can't access Flash, but only RAM.

Include platform.h

6dceb58

Include `platform.h` even if `MBEDTLS_PLATFORM_C` is undefined, to include the feature unsupported error.

Adjust new tests to TEST_ASSERT_RET

afd2b5a

Change `TEST_ASSERT` to `TEST_ASSERT_RET` to new `ecdh` tests.

Remove static type for function

528658e

Remove `static` type for `set_test_result` to avoid compilation warnings when this function is not being used.

Fix CI failures

be7b8c7

1. Change `test_fail` to `set_test_result`. 2. Add define of `mbedtls_snprintf` in `benchmark` in case platform is not defined.

Cosmetic changes

c7127e3

Remove redundant newline, and put `ASSERT` macro in a new code block.

benchmark modifications

a6d9e5f

1. Change typo in log. 2. Create a macro for the symmetric set key functionality. 3. Check for Feature not supported error in des key setting.

Remove extra lines in benchmark

5fcbeee

Remove redundant extra lines in the benchmark application.

Fix benchmark

0eddc6c

Add `CHECK_AND_BREAK` for the tests that run inside a timing loop, and add declaration of `mbedtls_snprintf` in case platform doesn't exist.

Add a helper macro for repetetive checks

06aeaf6

Add helper macros for skipping self tests in case the returned error is `MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED`, since htese checks are very similar in all tests.

RonEld force-pushed the unsupported_feature branch from 026eacf to 06aeaf6 Compare December 29, 2019 14:36

gilles-peskine-arm mentioned this pull request Mar 23, 2020

Negative testing for unsupported algorithms #351

Closed

gilles-peskine-arm closed this Mar 24, 2020

Unsupported feature support #162

Unsupported feature support #162

Conversation

RonEld commented Jul 9, 2019

RonEld commented Jul 14, 2019 • edited Loading

RonEld commented Sep 9, 2019

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

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

Choose a reason for hiding this comment

Patater commented Sep 10, 2019

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonEld Sep 12, 2019 • edited Loading

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

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

RonEld commented Sep 11, 2019

Patater commented Sep 11, 2019

RonEld commented Sep 11, 2019

RonEld commented Sep 12, 2019

gilles-peskine-arm commented Oct 29, 2019

Patater commented Oct 29, 2019

RonEld commented Dec 31, 2019

gilles-peskine-arm commented Mar 24, 2020

RonEld commented Jul 14, 2019 •

edited

Loading

RonEld Sep 12, 2019 •

edited

Loading