Feature unavailable unified error support #2124

RonEld · 2018-10-22T15:07:24Z

Description

This PR adds support for MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED introduced in #2054
Tests that receive this error, skip to the next step, as the underlying HW accelerator does not support that specific feature.
Exception are the entropy, when the SHA function returns such an error, as the specific entropy source is dependent on the SHA function, and not configurable, and the ctr_drbg, when the specific AES functionality is not supported, as it is dependent on it to work. Skipping the tests may result in security issue, if developer assumes the test passes.
Second PR that supersedes #1716

Status

READY

Requires Backporting

NO

Migrations

~~Error codes have been removed, so we may consider this as an API change~~

Additional comments

ctr_drbg requires MBEDTLS_CTR_DRBG_USE_128_BIT_KEY to be defined, in case AES 256 is not supported by the HW, ~~once PR #1902 will be merged.~~

~~Note: Once #2054 will be merged, I will rebase to have the support for the on target testing to run, but this PR can start the review~~

Todos

Tests
Documentation
Changelog updated
Backported

RonEld · 2018-10-23T11:04:09Z

Note: documentation updated in https://github.com/ARMmbed/mbed-tls-docs/pull/112

RonEld · 2018-10-23T12:29:15Z

updated changeLog, although I am not sure this change in tests and the benchmark application requires a ChangeLog entry.

RonEld · 2018-11-12T14:40:50Z

#2054 has been merged, so no more waiting for preceding PR

RonEld · 2018-12-11T13:27:20Z

I have rebased to resolve conflicts, and split the first commit to several commits, for readability.

@mpg Could you please review the x509 related commits?(16b688e and 4780d7e )

RonEld · 2018-12-11T13:34:58Z

After rebase I have tested on PC, to check nothing got broken.

I am currently building on target with unsupported features, and fix if new issues arise from the rebase ( such as new tests introduced that require the new macro)

RonEld · 2018-12-12T11:25:34Z

I have tested on target NRF52840_DK.
No new errors after rebase.
There is a possibility though that new tests did not fail, because no alternative implementation that returns MBEDTLS_PLATFORM_FEATURE_UNSUPPORTED exists for those tests.

RonEld · 2018-12-13T08:23:29Z

Add @mpg as a reviewer on the x509 module

mpg

I reviewed only two commits: 16b688e and 4780d7e - and they look fine to me.

I particularly like that the new flags are set in addition to the existing NOT_TRUSTED flags rather than replacing them. I think the minimizes the risk of misuse and is an excellent choice.

gilles-peskine-arm

I have reviewed the PR except for the changes to the tests to use TEST_ASSERT_RET or TEST_ASSERT_FLAGS.

With respect to the design:

This PR makes some functions return an unadorned MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED when other low-level errors are combined with a high-level error. This isn't necessarily a bad thing, but I need to think further about the possible implications, and I'd like @mpg's opinion on this change.
I'm mostly ok with the test case skipping feature, but it should use a tristate, not two booleans. Please take a look at Tolerate failures from unsupported operations in RSA test suites #1308 which introduced a test case skipping feature to see if there would be problems reconciling them. Do we want a different message for “skipped because of an unsupported feature in the hardware” and “skipped because of an unmet assumption”? Cc @hanno-arm
The change in the X509 module needs design approval from @sbutcher-arm or someone delegated by him.

gilles-peskine-arm · 2018-12-13T16:00:10Z

programs/test/benchmark.c

@@ -441,7 +449,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 )


This whole sequence is repeated many times. How about making it a macro, or a small number of macros (with and without continue?

gilles-peskine-arm · 2018-12-13T16:04:13Z

programs/test/benchmark.c

@@ -884,18 +1062,41 @@ int main( int argc, char *argv[] )
             curve_info++ )
        {
            mbedtls_ecdh_init( &ecdh );
+            mbedtls_snprintf( title, sizeof( title ), "ECDHE-%s",
+                                              curve_info->name );
+            if( mbedtls_ecp_group_load( &ecdh.grp, curve_info->grp_id ) != 0 )


Why no check for FEATURE_UNSUPPORTED here? We don't currently provide a hook there, but I'd prefer to keep the benchmark code consistent rather than micro-optimize this.

gilles-peskine-arm · 2018-12-13T16:05:56Z

programs/test/benchmark.c

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


Minor: “Feature not supported.” (here and elsewhere)

gilles-peskine-arm · 2018-12-13T16:11:26Z

tests/suites/helpers.function

@@ -119,6 +136,7 @@ static struct
    const char *test;
    const char *filename;
    int line_no;
+    int skipped;


There are only 3 possible outcomes, not 4: pass, skip, fail. I'd prefer a single variable with 3 possible states (an enum) rather than two boolean variables with one unused combination. Like #1308 but using an enum instead of magic constants (0, 1, 2).

gilles-peskine-arm · 2018-12-13T16:13:28Z

tests/suites/helpers.function



 /*----------------------------------------------------------------------------*/
 /* Macros */

+#define TEST_ASSERT_RET( TEST, EXP_RET )                                              \
+    do {                                                                              \
+        int ret = (TEST);                                                             \


Please don't use a generic name like ret inside a macro: it is likely to conflict with a name used outside of the macro. Use e.g. TEST_ASSERT_RET__ret.

yes, this is why I renamed all ret in the tests to something like exp_ret

gilles-peskine-arm · 2018-12-13T17:01:39Z

library/ecdsa.c

-            mbedtls_ecp_gen_keypair( &ctx->grp, &ctx->d, &ctx->Q, f_rng, p_rng ) );
+    int ret = 0;
+    ret = mbedtls_ecp_group_load( &ctx->grp, gid );
+    if( ret == 0 )


Minor: I think it would be more readable to use the common idiom if( ret != 0 ) return( ret );

gilles-peskine-arm · 2018-12-13T17:03:27Z

library/ecdsa.c

@@ -752,8 +752,12 @@ int mbedtls_ecdsa_read_signature_restartable( mbedtls_ecdsa_context *ctx,
 int mbedtls_ecdsa_genkey( mbedtls_ecdsa_context *ctx, mbedtls_ecp_group_id gid,
                  int (*f_rng)(void *, unsigned char *, size_t), void *p_rng )
 {
-    return( mbedtls_ecp_group_load( &ctx->grp, gid ) ||
-            mbedtls_ecp_gen_keypair( &ctx->grp, &ctx->d, &ctx->Q, f_rng, p_rng ) );
+    int ret = 0;


Oooh, the old code had a bug and the fix should be backported. f() || g() returned 1 on failure. This broke application code that looks for errors with if (mbedtls_ecdsa_genkey(…) < 0), which should be correct because the library normally returns negative values on error. This should also be mentioned in the changelog as a bugfix.

Backported to 2.7 in #2296

gilles-peskine-arm · 2018-12-13T17:05:06Z

library/pkparse.c

@@ -529,7 +530,11 @@ 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 )


The documentation in pk.h needs to be updated to add MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED as a possible error where applicable.

I disagree. See the answer to comment #2054 (comment)

The error code MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED can be returned by any underlying hw driver, and it is documented in our articles. I don't think that pk.h should have a special treatment. This is a generic error code that cipher.h could also return

My objection stands. We don't expect users to read our articles and deduce that statements in the documentation are wrong. If the documentation merely says “a negative value on error”, that's not ideal, but it's ok. But if the documentation explicitly lists the possible errors, then the list must include MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED. It can do so by name or by saying that generic platform errors are possible, or any other form of statement that doesn't explicitly exclude this error.

I disagree that we should mention generic error codes in our function description, however, I'm willing to add the generic platform errors are possible statement (MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED is also an option)

gilles-peskine-arm · 2018-12-13T17:22:08Z

library/pkparse.c

@@ -1343,7 +1361,9 @@ int mbedtls_pk_parse_key( mbedtls_pk_context *pk,
     * also ok and in line with the mbedtls_pk_free() calls
     * on failed PEM parsing attempts. */

-    return( MBEDTLS_ERR_PK_KEY_INVALID_FORMAT );
+    if( ret & MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED )


Masking an error code doesn't make sense. Did you mean “if the low-level error is feature-unsupported then mask out the high-level error”? That would be

if( ( ret & -0x7f ) == MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED )

gilles-peskine-arm · 2018-12-13T17:24:02Z

library/pkparse.c

@@ -1343,7 +1361,9 @@ int mbedtls_pk_parse_key( mbedtls_pk_context *pk,
     * also ok and in line with the mbedtls_pk_free() calls
     * on failed PEM parsing attempts. */

-    return( MBEDTLS_ERR_PK_KEY_INVALID_FORMAT );
+    if( ret & MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED )
+        return( MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED );


Why does MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED get this special treatment of masking out the high-level error? And similarly in other places why does it get the special treatment of not masking in a high-level error?

I didn't want to change behavior of function, so returning MBEDTLS_ERR_PK_KEY_INVALID_FORMAT on failure.
MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED is a special use case, as user need to know that it might not be a "real" failure, and tests should skip on this failure.
I can consider returning ret in this case, or MBEDTLS_ERR_PK_KEY_INVALID_FORMAT | MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED

RonEld · 2018-12-16T11:35:05Z

@gilles-peskine-arm Thank you for the review.

I addressed your comments, and will make the fixes shortly.

As for your general comments:

This PR makes some functions return an unadorned MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED when other low-level errors are combined with a high-level error.

I did this only when the low level flags are MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED. I don't see a case where there is more than one low level flag.

I'm mostly ok with the test case skipping feature, but it should use a tristate, not two booleans. Please take a look at #1308 which introduced a test case skipping feature to see if there would be problems reconciling them. Do we want a different message for “skipped because of an unsupported feature in the hardware” and “skipped because of an unmet assumption”? Cc @hanno-arm

Ok, I'll change. Actually, The behavior should be the same for both PRs. This PR already handle rsa feature unsupported as well. I believe in rsa_alt case as well. MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED is returned by the developer implementing the alternative implementation, for what any reason they choose.

The change in the X509 module needs design approval from @sbutcher-arm or someone delegated by him.

As you can see from #2124 (review) , @mpg already reviewed and approved the changes in the x509 module.

NirSonnenschein

Look mostly good. I've left a few fairly minor comments, please take a look.

include/mbedtls/x509.h

NirSonnenschein · 2018-12-17T16:04:54Z

library/pkparse.c


    *p += len;

-    if( mbedtls_rsa_complete( rsa ) != 0 ||
-        mbedtls_rsa_check_pubkey( rsa ) != 0 )
+    if( ( ret = mbedtls_rsa_complete( rsa ) ) != 0 ||


which value of ret do you check later? seems to me that you would only check the second one below?

This is a Boolean condition. If the first statement fails, then we return the first ret, if the second statement fails then the second ret is returned

scratch that

Actually, don't scratch it. If the first statement is true ( ret != 0 ) then we don't call the second statement

NirSonnenschein · 2018-12-17T19:53:54Z

library/rsa.c

    {
-        return( MBEDTLS_ERR_RSA_KEY_CHECK_FAILED );
+        return( ret );


same as above seems that the second error is returned in case both return error, is the this the desired behavior?

if the first statement fails, then the second statement is not even called

scratch that..

Actually, don't scratch it. If the first statement is true ( ret != 0 ) then we don't call the second statement

RonEld · 2018-12-18T11:43:29Z

@NirSonnenschein I have modified according to your comment, in order to make the code more readable.

Note that the original change in rsa actually changed behaviour, since instead of returning MBEDTLS_ERR_RSA_KEY_CHECK_FAILED , the error code from the mbedtls_rsa_check_xxxkey was returned. I reverted this change of behavior, returning an addition MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED where needed

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.

Introduce `TEST_ASSERT_RET` to skip tests with functions that return `MBEDTLS_PLATFORM_FEATURE_UNSUPPORTED`.

1. Change `TEST_ASSERT` to `TEST_ASSERT_RET` in all the tests of the symmetric ciphers, aead and digests. 2. Modify the `cipher` module to return the error returned by the underlying chacha error. 3. In aead tests, set the expected decryption return code prior to decryption, to distinguish between negative and positive paths. 4. Have xtea return error code of `mbedtls_xtea_crypt_ecb`, and zeroize output on failure.

1. Add verification failure flags for feature unsupported. 2. Add the verification flag when verifying certificate, to distinguish between verification failure because the underlying hw does not support the speciic feature, such as curve, rsa key size, and a verification failure because the certificate might be tampered.

1. Change `TEST_ASSERT` to `TEST_ASSERT_RET`. 2. Add dependency in `MBEDTLS_RSA_C` in one of the tests that was missing.

Add ChangeLog entry.

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

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.

1. Define an enum for test result. 2. Rename `ret` to a less generic name, inside `TEST_ASSERT_RET`.

Rephrase comments.

Adjust the invalid param tests to the unsupported feature error returned by the underlying platform: 1. Skip the test in case `MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED` is returned. 2. Adjust to `test_info.result` enum. 3. Change `TEST_ASSERT` to `TEST_ASSERT_RET`.

Fix typo in the x509 module.

Fix a typo in `mbedtls_test.py` script.

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.

The uint32 is given as a bigendian stream, in the tests, however, the char buffer that collected the stream read it as is, without converting it. Add a temporary buffer, to call `greentea_getc()` 8 times, and then put it in the correct endianity for input to `unhexify()`.

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

RonEld · 2019-09-18T14:40:21Z

Rebased to resolve conflicts

mpg · 2022-03-24T09:23:19Z

This PR is about improving support for _ALT, which is now on its way out (being replaced with the PSA Crypto drvier API). It has gotten a lot of conflicts over the years, so reviving it would be more effort that warranted considering _ALT support is no longer a priority.

RonEld added needs-design-approval DO-NOT-MERGE component-x509 component-crypto Crypto primitives and low-level interfaces component-platform Portability layer and build scripts needs-preceding-pr Requires another PR to be merged first labels Oct 22, 2018

RonEld force-pushed the feature_unavailable_unified_error_support branch 3 times, most recently from 4a6f04f to 9aa3d09 Compare October 31, 2018 13:26

RonEld force-pushed the feature_unavailable_unified_error_support branch from 7ecb4ba to f3d2595 Compare November 12, 2018 14:12

RonEld removed the needs-preceding-pr Requires another PR to be merged first label Nov 12, 2018

RonEld force-pushed the feature_unavailable_unified_error_support branch from f3d2595 to 9f9594b Compare November 14, 2018 14:08

RonEld removed the DO-NOT-MERGE label Nov 15, 2018

RonEld force-pushed the feature_unavailable_unified_error_support branch from 9f9594b to c2d37de Compare December 11, 2018 13:25

RonEld requested a review from mpg December 13, 2018 08:23

mpg reviewed Dec 13, 2018

View reviewed changes

gilles-peskine-arm requested changes Dec 13, 2018

View reviewed changes

gilles-peskine-arm mentioned this pull request Dec 13, 2018

ECDH: Add the Everest X25519 implementation #2073

Closed

This was referenced Dec 17, 2018

Backport 2.7: ecdsa genkey return proper error #2296

Merged

Tolerate failures from unsupported operations in RSA test suites #1308

Closed

NirSonnenschein reviewed Dec 17, 2018

View reviewed changes

RonEld force-pushed the feature_unavailable_unified_error_support branch 2 times, most recently from ce31dad to d7c33c6 Compare September 12, 2019 08:33

RonEld added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Sep 12, 2019

RonEld force-pushed the feature_unavailable_unified_error_support branch from d7c33c6 to e1e7bea Compare September 12, 2019 10:42

Ron Eldor added 17 commits September 18, 2019 17:37

Support error codes in sample applications

58a25b8

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.

Add infrastructure to skip unsupported features

ca970e4

Introduce `TEST_ASSERT_RET` to skip tests with functions that return `MBEDTLS_PLATFORM_FEATURE_UNSUPPORTED`.

Adjust x509 tests to TEST_ASSERT_RET

f39ff7c

1. Change `TEST_ASSERT` to `TEST_ASSERT_RET`. 2. Add dependency in `MBEDTLS_RSA_C` in one of the tests that was missing.

update ChangLog

747cdab

Add ChangeLog entry.

Adapt On Target Tests to Feature Unsupported

f2767d0

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

benchmark modifications

df684b2

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.

Modify tests template

fa90b37

1. Define an enum for test result. 2. Rename `ret` to a less generic name, inside `TEST_ASSERT_RET`.

Wording fixes in x509

9c1667f

Rephrase comments.

Fix typo in x509 module

6bfd6f1

Fix typo in the x509 module.

Fix typo in the On Target Test script

78415ce

Fix a typo in `mbedtls_test.py` script.

Remove extra lines in benchmark

e888b66

Remove redundant extra lines in the benchmark application.

Fix benchmark

00e2aa7

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.

Include platform.h

57643d5

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

RonEld force-pushed the feature_unavailable_unified_error_support branch from e1e7bea to 57643d5 Compare September 18, 2019 14:39

RonEld added the mbed TLS team label Oct 15, 2019

gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Apr 2, 2020

mpg added the historical-reviewing Currently reviewing (for legacy PR/issues) label Mar 17, 2022

mpg closed this Mar 24, 2022

Feature unavailable unified error support #2124

Feature unavailable unified error support #2124

Conversation

RonEld commented Oct 22, 2018 • edited Loading

Description

Status

Requires Backporting

Migrations

Additional comments

Todos

RonEld commented Oct 23, 2018

RonEld commented Oct 23, 2018

RonEld commented Nov 12, 2018

RonEld commented Dec 11, 2018

RonEld commented Dec 11, 2018

RonEld commented Dec 12, 2018

RonEld commented Dec 13, 2018

mpg left a comment

Choose a reason for hiding this comment

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

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

RonEld commented Dec 16, 2018

NirSonnenschein left a 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 Dec 18, 2018

RonEld commented Sep 18, 2019

mpg commented Mar 24, 2022

RonEld commented Oct 22, 2018 •

edited

Loading