Unsupported feature support #162

2019-07-09T08:23:41-05:00

RonEld commented

2019-07-09 08:23:41 -05:00

(Migrated from github.com)

This PR adds support for MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED in tests and applications.
This supersedes the crypto part of https://github.com/ARMmbed/mbedtls/pull/2124

This PR adds support for `MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED` in tests and applications. This supersedes the crypto part of https://github.com/ARMmbed/mbedtls/pull/2124

RonEld commented

2019-07-14 07:55:43 -05:00

(Migrated from github.com)

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 https://github.com/ARMmbed/mbedtls/pull/2124)

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 https://github.com/ARMmbed/mbedtls/pull/2124)

RonEld commented

2019-09-09 10:09:24 -05:00

(Migrated from github.com)

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

Patater (Migrated from github.com) requested changes

2019-09-10 06:30:31 -05:00

library/aria.c

					
				@ -960,15 +959,21 @@ int mbedtls_aria_self_test( int verbose )

				        /* test ECB encryption */

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.

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.

We are adding a lot of branches to handle FEATURE_UNSUPPORTED from many places. How will we test this? Our code coverage is going to suffer a lot unless we come up with and write tests to hit each of these new branches. This can also help us find bugs if we don't properly handle FEATURE_UNSUPPORTED errors somewhere.

library/chacha20.c

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

library/pkparse.c

					
				@ -550,1 +559,4 @@

				        }

				        return( ret );

				    }

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

library/pkparse.c

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

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

tests/suites/helpers.function

					
				@ -107,15 +108,68 @@ typedef enum

				 *

Don't add this newline

tests/suites/helpers.function

					
				@ -119,0 +139,4 @@

				 *

				 *          In case the returned error code is MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED,

				 *          the test is skipped.

				 *

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

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

tests/suites/helpers.function

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

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

tests/suites/target_test.function

Why does this need to change as part of "Unsupported feature support"

RonEld (Migrated from github.com) reviewed

2019-09-10 06:34:34 -05:00

library/aria.c

					
				@ -960,15 +959,21 @@ int mbedtls_aria_self_test( int verbose )

				        /* test ECB encryption */

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

RonEld (Migrated from github.com) reviewed

2019-09-10 06:34:40 -05:00

library/chacha20.c

ok

RonEld (Migrated from github.com) reviewed

2019-09-10 06:36:08 -05:00

library/pkparse.c

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

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

RonEld (Migrated from github.com) reviewed

2019-09-10 06:36:58 -05:00

library/pkparse.c

					
				@ -550,1 +559,4 @@

				        }

				        return( ret );

				    }

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

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

RonEld (Migrated from github.com) reviewed

2019-09-10 06:37:06 -05:00

tests/suites/helpers.function

					
				@ -107,15 +108,68 @@ typedef enum

				 *

ok

RonEld (Migrated from github.com) reviewed

2019-09-10 06:40:43 -05:00

tests/suites/helpers.function

					
				@ -119,0 +139,4 @@

				 *

				 *          In case the returned error code is MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED,

				 *          the test is skipped.

				 *

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

in order not to duplicate code. I believe this came from [this comment](https://github.com/ARMmbed/mbedtls/pull/2124#discussion_r241462535)

RonEld (Migrated from github.com) reviewed

2019-09-10 06:41:45 -05:00

tests/suites/target_test.function

It doesn't but this bug fix was discovered as part of this PR, so I put it as a seperate commit.

RonEld (Migrated from github.com) reviewed

2019-09-10 06:42:43 -05:00

library/aria.c

					
				@ -960,15 +959,21 @@ int mbedtls_aria_self_test( int verbose )

				        /* test ECB encryption */

I agree, but as mentioned, since we allow this eror to be returned in every function, I don't see another option.
My suggestion was to have "dummy targets" that will return this error in different functions

I agree, but as mentioned, since we allow this eror to be returned in every function, I don't see another option. My suggestion was to have "dummy targets" that will return this error in different functions

Patater (Migrated from github.com) reviewed

2019-09-10 06:44:19 -05:00

tests/suites/target_test.function

OK

RonEld (Migrated from github.com) reviewed

2019-09-10 06:44:42 -05:00

tests/suites/helpers.function

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

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

Patater (Migrated from github.com) reviewed

2019-09-10 06:45:02 -05:00

library/aria.c

					
				@ -960,15 +959,21 @@ int mbedtls_aria_self_test( int verbose )

				        /* test ECB encryption */

Yes, we need dummy targets (more commonly known as test mocks) in order to complete this feature.

Patater commented

2019-09-10 07:34:23 -05:00

(Migrated from github.com)

Needs to pass CI

👍 1

RonEld (Migrated from github.com) reviewed

2019-09-10 09:22:52 -05:00

library/pkparse.c

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

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`

gilles-peskine-arm (Migrated from github.com) requested changes

2019-09-10 11:44:29 -05:00

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

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.

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.

library/aes.c

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.

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`.

library/aria.c

					
				@ -960,15 +959,21 @@ int mbedtls_aria_self_test( int verbose )

				        /* test ECB encryption */

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.

library/pkparse.c

					
				@ -550,0 +556,4 @@

				        else

				        {

				            ret = MBEDTLS_ERR_PK_INVALID_PUBKEY;

				        }

This block of code is repeated. Please make an auxiliary function.

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?

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`?

library/pkparse.c

					
				@ -550,1 +559,4 @@

				        }

				        return( ret );

				    }

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);
}

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); } ```

programs/test/benchmark.c

Cosmetic: “Feature not supported. Skipping.”

programs/test/benchmark.c

					
				@ -420,9 +475,26 @@ int main( int argc, char *argv[] )

				    {

				        mbedtls_des_context des;

				        mbedtls_des_init( &des );

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

programs/test/benchmark.c

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. I think it's high time to split this code into auxiliary functions, and react to errors with `goto exit`.

tests/suites/helpers.function

					
				@ -107,15 +108,68 @@ typedef enum

				 *

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

tests/suites/helpers.function

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

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

tests/suites/helpers.function

The function would have to be non-static.

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

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 — https://github.com/ARMmbed/mbed-crypto/issues/250.

We should think of putting such functions into a separate `.c` file — https://github.com/ARMmbed/mbed-crypto/issues/250.

tests/suites/host_test.function

There can be reasons to skip a test other than “Feature not supported by the platform”.

tests/suites/test_suite_aes.function

					
				@ -205,7 +211,8 @@ void aes_crypt_xts_size( int size, int retval )

				    /* Valid pointers are passed for builds with MBEDTLS_CHECK_PARAMS, as

				     * otherwise we wouldn't get to the size check we're interested in. */

Minor: exit: return; is not necessary in test functions (the test code generator script adds it) and we traditionally don't include it.

Minor: `exit: return;` is not necessary in test functions (the test code generator script adds it) and we traditionally don't include it.

tests/suites/test_suite_blowfish.function

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.

tests/suites/test_suite_ecp.function

					
				@ -1062,3 +1057,1 @@

				    TEST_ASSERT( mbedtls_ecp_check_pubkey( &key.grp, &key.Q ) == 0 );

				    TEST_ASSERT( mbedtls_ecp_check_privkey( &key.grp, &key.d ) == 0 );

				    TEST_ASSERT_RET( mbedtls_ecp_gen_key( id, &key, &rnd_pseudo_rand,

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

tests/suites/test_suite_pk.function

Valid/invalid parameter tests should never reach an alt implementation, should they? The rejection should happen in the pk layer. So they should never return FEATURE_UNSUPPORTED, so the code should keep using TEST_ASSERT and not TEST_ASSERT_RET.

Valid/invalid parameter tests should never reach an alt implementation, should they? The rejection should happen in the pk layer. So they should never return `FEATURE_UNSUPPORTED`, so the code should keep using `TEST_ASSERT` and not `TEST_ASSERT_RET`.

RonEld commented

2019-09-11 02:30:53 -05:00

(Migrated from github.com)

All the failed CI tests are in the tls testing

Patater commented

2019-09-11 05:35:51 -05:00

(Migrated from github.com)

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.

> 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 commented

2019-09-11 05:59:40 -05:00

(Migrated from github.com)

@Patater
I guess https://github.com/ARMmbed/mbedtls/pull/2124 should be merged before this PR then, updating the TLS PR with this branch as the submodule

@Patater I guess https://github.com/ARMmbed/mbedtls/pull/2124 should be merged before this PR then, updating the TLS PR with this branch as the submodule

RonEld (Migrated from github.com) reviewed

2019-09-12 04:42:15 -05:00

programs/test/benchmark.c

					
				@ -420,9 +475,26 @@ int main( int argc, char *argv[] )

				    {

				        mbedtls_des_context des;

				        mbedtls_des_init( &des );

It's done in https://github.com/ARMmbed/mbedtls/pull/2124 . Should I cherry-pick the changes there, or will it be merged to this repository?

RonEld (Migrated from github.com) reviewed

2019-09-12 04:49:22 -05:00

tests/suites/test_suite_ecp.function

					
				@ -1062,3 +1057,1 @@

				    TEST_ASSERT( mbedtls_ecp_check_pubkey( &key.grp, &key.Q ) == 0 );

				    TEST_ASSERT( mbedtls_ecp_check_privkey( &key.grp, &key.d ) == 0 );

				    TEST_ASSERT_RET( mbedtls_ecp_gen_key( id, &key, &rnd_pseudo_rand,

Probably rebase error. I'll add them

RonEld commented

2019-09-12 06:32:47 -05:00

(Migrated from github.com)

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 https://github.com/ARMmbed/mbedtls/pull/2124 pass, I believe that once that PR will be merged, the TLS tests owuld pass here as well

>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 https://github.com/ARMmbed/mbedtls/pull/2124 pass, I believe that once that PR will be merged, the TLS tests owuld pass here as well

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

2019-09-12 08:04:53 -05:00

programs/test/benchmark.c

					
				@ -420,9 +475,26 @@ int main( int argc, char *argv[] )

				    {

				        mbedtls_des_context des;

				        mbedtls_des_init( &des );

Please don't introduce new code that would change when https://github.com/ARMmbed/mbedtls/pull/2124 finds its way into Mbed Crypto. Make the same changes to helpers.function here.

Please don't introduce new code that would change when https://github.com/ARMmbed/mbedtls/pull/2124 finds its way into Mbed Crypto. Make the same changes to `helpers.function` here.

RonEld (Migrated from github.com) reviewed

2019-09-18 10:07:34 -05:00

tests/suites/test_suite_pk.function

Valid/invalid parameter tests should never reach an alt implementation, should they?

I agree that for the PK layer the rejection should be in the pk layer, so perhaps no need for TEST_ASSERT_RET here, but in general, valid\invalid parameter tests should reach an alt implementation.

>Valid/invalid parameter tests should never reach an alt implementation, should they? I agree that for the PK layer the rejection should be in the pk layer, so perhaps no need for `TEST_ASSERT_RET` here, but in general, valid\invalid parameter tests should reach an alt implementation.

RonEld (Migrated from github.com) reviewed

2019-09-18 10:48:55 -05:00

library/pkparse.c

					
				@ -550,0 +556,4 @@

				        else

				        {

				            ret = MBEDTLS_ERR_PK_INVALID_PUBKEY;

				        }

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

Why would there be two low level errors? I don't see a place where`MBEDTLS_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`

RonEld (Migrated from github.com) reviewed

2019-09-18 11:13:41 -05:00

programs/test/benchmark.c

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

>Breaking here leaves ecdsa unfreed. No it won't. THe break is done within [the loop inside `TIME_PUBLIC`](https://github.com/ARMmbed/mbed-crypto/blob/development/programs/test/benchmark.c#L185) macro., going to next line in this code, which frees the context

RonEld (Migrated from github.com) reviewed

2019-09-19 05:46:16 -05:00

tests/suites/helpers.function

					
				@ -107,15 +108,68 @@ typedef enum

				 *

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

gilles-peskine-arm commented

2019-10-29 11:50:42 -05:00

(Migrated from github.com)

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 commented

2019-10-29 12:20:05 -05:00

(Migrated from github.com)

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.

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.

RonEld commented

2019-12-31 06:56:11 -06:00

(Migrated from github.com)

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

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 commented

2020-03-24 13:56:14 -05:00

(Migrated from github.com)

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 https://github.com/ARMmbed/mbedtls/pull/2124 .

[Mbed Crypto has been merged back into Mbed TLS](https://github.com/ARMmbed/mbed-crypto/issues/374). 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 https://github.com/ARMmbed/mbedtls/pull/2124 .