Disallow use of invalid contexts #58

Patater · 2019-02-15T15:37:07Z

Disallow use of MAC, hash, and cipher contexts that haven't been set up (i.e. has a *_setup() function called on them).

gilles-peskine-arm

The implementation needs a design discussion about how to cope with bad-state errors. We have an unresolved question about the API spec at https://github.com/ARMmbed/psa-crypto/issues/45 (private link).
The tests don't cover all the cases that I'd expect. They're missing some bad-state cases for MAC and cipher.
Some of the tests need restructuring.

tests/suites/test_suite_psa_crypto.function

library/psa_crypto.c

NirSonnenschein

Looks ok to me besides the comments already made by Gilles. Are there any additional dependencies that need to be updated (e.g. example repo , usage by tls, etc..)?

Patater · 2019-02-19T12:22:16Z

Rebasing to address review feedback, extend tests, and fix #10.

Patater · 2019-02-19T12:23:41Z

Rebased. Previous branch at https://github.com/Patater/mbed-crypto/tree/disallow-invalid-context-1

Patater · 2019-02-19T13:39:18Z

I'm looking into some memory leaks.

In general, if you start a cipher operation, and some internal memory is allocated on your behalf (e.g. AES context created), and then you call finish twice and get a PSA_ERROR_BAD_STATE error, there is no way to free that lost memory anymore, is there? You can't call abort, since after you get an PSA_ERROR_BAD_STATE, all bets are off.

NirSonnenschein · 2019-02-19T13:45:32Z

wouldn't the first call to finish de-allocate the resources?

gilles-peskine-arm · 2019-02-19T13:49:55Z

after you get an PSA_ERROR_BAD_STATE, all bets are off.

That's not true. All bets are off if you have a bad state because the object was corrupted or never initialized. But if you've correctly initialized the object and subsequently only ever modified it through API functions, then abort is guaranteed to succeed.

Patater · 2019-02-19T13:50:58Z

@NirSonnenschein Yes, it likely would. Good point.

I'll change my example. How about if you call cipher finish without first setting an IV? Finish would fail with PSA_ERROR_BAD_STATE, but memory would have been allocated during cipher setup.

Patater · 2019-02-19T13:54:42Z

Thanks for the clarification, Gilles. I'll switch from using memset to all 0 between bad order tests and use abort instead.

Also note that one isn't guaranteed to get BAD_STATE if the context is corrupt or never initialized, so I'm not sure why we should be concerned, as per your comment: #58 (comment), about calling abort if BAD_STATE is encountered in these sorts of "well-formed" situations.

Can we conclude that it is OK to call abort on BAD_STATE? Are there cases we shouldn't call abort given we've gotten a BAD_STATE?

gilles-peskine-arm · 2019-02-19T14:04:43Z

#58 (comment) was about calling abort internally, not about calling it from the application. I distinguished two cases: well-formed situations, where it's debatable whether the right behavior is to abort or to not modify the object, and non-well-formed situations, where abort could make things worse.

In application code, you should always initialize your memory, and you should always call abort unless you have a guarantee that the object is in its initial state (e.g. because you never set it up or the last function that you applied to it was a finish function).

Patater · 2019-02-19T14:08:48Z

Rebase incoming to switch from memset to abort between bad order tests.

Patater · 2019-02-19T14:10:37Z

Rebased. Previous branch at https://github.com/Patater/mbed-crypto/tree/disallow-invalid-context-3

Patater · 2019-02-19T14:12:12Z

Forgot to apply a fixup commit. Rebasing again.

Patater · 2019-02-19T14:13:35Z

Rebased to apply fixup commit. Previous branch at https://github.com/Patater/mbed-crypto/tree/disallow-invalid-context-4

Patater · 2019-02-19T14:13:53Z

All done rebasing. Ready to review.

Patater · 2019-02-19T15:14:26Z

Added commits to address #58 (comment)

Will apply as fixup with a rebase if review goes well

Patater · 2019-02-19T15:51:08Z

This is getting a bit too messy. I'm going to have to rebase before review.

Patater · 2019-02-19T16:12:35Z

Rebased to apply fixups and make some style improvements. Previous branch at https://github.com/Patater/mbed-crypto/tree/disallow-invalid-context-5

Patater · 2019-02-19T16:12:41Z

Rebasing done.

Patater · 2019-02-20T09:19:00Z

Will rebase to apply fixup commit now

Patater · 2019-02-20T13:35:48Z

CI passed. Ready for review.

gilles-peskine-arm

psa_mac_update is not consistent with the other update/finish functions wrt an argument in initial state: it calls abort. This is not an absolute blocker because we still haven't decided what behavior the specification will mandate, but I'd prefer the implementation to be consistent.

library/cipher.c

library/psa_crypto.c

gilles-peskine-arm · 2019-02-20T13:38:11Z

tests/suites/test_suite_psa_crypto.function

+                PSA_ERROR_BAD_STATE );
+    PSA_ASSERT( psa_hash_abort( &operation ) );
+
+    /* Call update without calling setup beforehand. */


Technically, you're calling update after abort. This is equivalent to update without setup only if abort behaves correctly.

Would you recommend we call both abort and memset the context to 0? I'm depending on abort setting the context to 0, and if it doesn't, the test will fail.

If abort works properly, it doesn't matter. It's only a matter of how hard it will be to investigate a test failure that is ultimately due to abort returning the expected status but not having done its job properly.

I would prefer to make each unit test independent. But it's also good to have a stress test where a single object is reused.

I've raised #66 to add targeted abort tests.

tests/suites/test_suite_psa_crypto.function

Ensure that when doing MAC operations out of order, PSA_ERROR_BAD_STATE is returned as documented in crypto.h and the PSA Crypto specification.

Ensure that when doing cipher operations out of order, PSA_ERROR_BAD_STATE is returned as documented in crypto.h and the PSA Crypto specification.

If a hash context has not been set up, fail with PSA_ERROR_BAD_STATE as documented in crypto.h and the PSA Crypto specification.

Extend hash bad order test in line with the new bad order tests for MAC and cipher, covering more cases and making comments and test layout consistent. Ensure that when doing hash operations out of order, PSA_ERROR_BAD_STATE is returned as documented in crypto.h and the PSA Crypto specification.

Calling psa_*_setup() twice on a MAC, cipher, or hash context should result in a PSA_ERROR_BAD_STATE error because the operation has already been set up. Fixes ARMmbed#10

Patater · 2019-02-20T15:29:31Z

Rebasing to address review feedback.

Patater · 2019-02-20T15:33:29Z

Rebased. Previous branch at https://github.com/Patater/mbed-crypto/tree/disallow-invalid-context-9

In places where we detect a context is in a bad state and there is no sensitive data to clear, simply return PSA_ERROR_BAD_STATE and don't abort on behalf of the application. The application will choose what to do when it gets a bad state error. The motivation for this change is that an application should decide what to do when it misuses the API and encounters a PSA_ERROR_BAD_STATE error. The library should not attempt to abort on behalf of the application, as that may not be the correct thing to do in all circumstances.

Patater · 2019-02-20T17:40:23Z

Rebased to add a commit that returns bad state without aborting in more cases. Previous branch at https://github.com/Patater/mbed-crypto/tree/disallow-invalid-context-12

yanesca · 2019-02-21T15:40:56Z

library/ssl_tls.c

@@ -6529,7 +6529,7 @@ static void ssl_calc_finished_tls_sha256(
    unsigned char padbuf[32];
 #if defined(MBEDTLS_USE_PSA_CRYPTO)
    size_t hash_size;
-    psa_hash_operation_t sha256_psa;
+    psa_hash_operation_t sha256_psa = PSA_HASH_OPERATION_INIT;


Using PSA_HASH_OPERATION_INIT, psa_hash_operation_init(); or simply = 0 are all valid ways to do initialisation, right?

What are the advantages of different methods? In particular, why are we using on line 1449 and 1492 psa_hash_operation_init(); and PSA_HASH_OPERATION_INIT here?

{0} and INIT (macro) can be used to initialize global variables, but init() (function) can't. {0} is generic but some compilers warn that this implicitly initializes other fields to 0. init() is the only one that's typed, so you'll get an error if you use it in the wrong context. memset to 0 is also defined as equivalent because it allows applications to use e.g. calloc, and in practice it produces the same value as {0} on virtually all platforms.

In most cases, the choice is indifferent.

PSA_HASH_OPERATION_INIT is all static, so you can use it for static globals. The inline function can be used where functions can be used, which is a few less places, but it uses the static initializer under-the-hood. = {0} is usually ok, but some compilers will complain about missing field initializers.

I personally prefer using the macro everywhere, but previous authors inside library/ssl_tls.c liked the function. There is no functional difference in this case.

yanesca

Looks good to me!

Patater · 2019-02-21T17:36:50Z

CI failure is only FreeBSD timing test, a known-flaky test.

Patater requested review from NirSonnenschein and gilles-peskine-arm February 15, 2019 15:37

Patater added bug Something isn't working needs: review The pull request is ready for review. This generally means that it has no known issues. compliance Discrepancy between the library and the PSA Crypto API specification labels Feb 15, 2019

gilles-peskine-arm requested changes Feb 15, 2019

View reviewed changes

tests/suites/test_suite_psa_crypto.function Outdated Show resolved Hide resolved

library/psa_crypto.c Show resolved Hide resolved

NirSonnenschein reviewed Feb 17, 2019

View reviewed changes

Patater force-pushed the disallow-invalid-context branch from 0b2f1f2 to 5d0a2d3 Compare February 19, 2019 12:22

Patater force-pushed the disallow-invalid-context branch from 5d0a2d3 to 1128d08 Compare February 19, 2019 14:10

Patater force-pushed the disallow-invalid-context branch from 1128d08 to ace8e02 Compare February 19, 2019 14:13

Patater force-pushed the disallow-invalid-context branch from fbea691 to 53d9509 Compare February 19, 2019 16:11

Patater force-pushed the disallow-invalid-context branch from 2cad8f1 to ff9d2bc Compare February 20, 2019 09:19

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

Patater mentioned this pull request Feb 20, 2019

Initialize PSA Crypto operation contexts Mbed-TLS/mbedtls#2454

Merged

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

gilles-peskine-arm requested changes Feb 20, 2019

View reviewed changes

Patater added needs: work The pull request needs rework before it can be merged. and removed needs: review The pull request is ready for review. This generally means that it has no known issues. labels Feb 20, 2019

Patater added 5 commits February 20, 2019 15:27

psa: Disallow use of invalid MAC contexts

252ef28

Ensure that when doing MAC operations out of order, PSA_ERROR_BAD_STATE is returned as documented in crypto.h and the PSA Crypto specification.

psa: Disallow use of invalid cipher contexts

ab43997

Ensure that when doing cipher operations out of order, PSA_ERROR_BAD_STATE is returned as documented in crypto.h and the PSA Crypto specification.

psa: Disallow use of invalid hash contexts

a0f625a

If a hash context has not been set up, fail with PSA_ERROR_BAD_STATE as documented in crypto.h and the PSA Crypto specification.

psa: Disallow repeated setup

36ee5d0

Calling psa_*_setup() twice on a MAC, cipher, or hash context should result in a PSA_ERROR_BAD_STATE error because the operation has already been set up. Fixes ARMmbed#10

Patater mentioned this pull request Feb 20, 2019

Add tests for abort functions #66

Open

Patater force-pushed the disallow-invalid-context branch from 0cbb8c7 to 36ee5d0 Compare February 20, 2019 15:33

Patater force-pushed the disallow-invalid-context branch from ee32d9f to e236c2a Compare February 20, 2019 17:39

Patater added needs: review The pull request is ready for review. This generally means that it has no known issues. needs: ci Needs a passing full CI run and removed needs: work The pull request needs rework before it can be merged. needs: ci Needs a passing full CI run labels Feb 20, 2019

gilles-peskine-arm approved these changes Feb 21, 2019

View reviewed changes

yanesca reviewed Feb 21, 2019

View reviewed changes

yanesca approved these changes Feb 21, 2019

View reviewed changes

Patater merged commit bf61ca7 into ARMmbed:development Feb 21, 2019

Disallow use of invalid contexts #58

Disallow use of invalid contexts #58

Uh oh!

Conversation

Patater commented Feb 15, 2019

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

NirSonnenschein left a comment

Choose a reason for hiding this comment

Uh oh!

Patater commented Feb 19, 2019

Uh oh!

Patater commented Feb 19, 2019

Uh oh!

Patater commented Feb 19, 2019

Uh oh!

NirSonnenschein commented Feb 19, 2019

Uh oh!

gilles-peskine-arm commented Feb 19, 2019

Uh oh!

Patater commented Feb 19, 2019

Uh oh!

Patater commented Feb 19, 2019

Uh oh!

gilles-peskine-arm commented Feb 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Patater commented Feb 19, 2019

Uh oh!

Patater commented Feb 19, 2019

Uh oh!

Patater commented Feb 19, 2019

Uh oh!

Patater commented Feb 19, 2019

Uh oh!

Patater commented Feb 19, 2019

Uh oh!

Patater commented Feb 19, 2019

Uh oh!

Patater commented Feb 19, 2019

Uh oh!

Patater commented Feb 19, 2019

Uh oh!

Patater commented Feb 19, 2019

Uh oh!

Patater commented Feb 20, 2019

Uh oh!

Patater commented Feb 20, 2019

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gilles-peskine-arm Feb 20, 2019

Choose a reason for hiding this comment

Uh oh!

Patater Feb 20, 2019

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Feb 20, 2019

Choose a reason for hiding this comment

Uh oh!

Patater Feb 20, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Patater commented Feb 20, 2019

Uh oh!

Patater commented Feb 20, 2019

Uh oh!

Patater commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gilles-peskine-arm commented Feb 19, 2019 •

edited

Loading

Patater commented Feb 20, 2019 •

edited

Loading