gh-ARMmbed/mbed-crypto
Watch
1
0
Fork
You've already forked mbed-crypto
 0
Code Issues 36 Pull requests 1 Releases 7 Activity

Update function return codes #213

Merged
adrianlshaw merged 59 commits from psa-api-1.0-beta into development 2019-09-06 02:53:29 -05:00
Conversation 41 Commits 59 Files changed 1 +259 -12
adrianlshaw commented 2019-08-12 09:25:59 -05:00 (Migrated from github.com)

The Beta version of this API is missing return codes for various functions. Conversely, with some cases it's debatable whether some of the existing return codes could ever be returned by particular function implementations.

This PR tries to harmonise the return codes by adding expected return codes where they are possible. It does not attempt to document existing return codes. There are also a number of functions which need more thought (abort/close/(get|set)_capacity), which are not fully addressed here. These two drawbacks ought to be addressed in a future pull request.

Here is a summary of the changes I've made (I've excluded the error prefixes for readability):

  • It is possible for all functions to return BAD_STATE if they haven't initialised the crypto library, so this has been added everywhere. However, it is implementation dependent whether this is returned.
  • DOES_NOT_EXIST is removed in all places except psa_open_key(). With the exception of that function, INVALID_HANDLE ought to be returned instead.
  • In cases where there is corruption there is also the opportunity for STORAGE_FAILURE to occur. So this has been added.
  • Small misc cases where BUFFER_TOO_SMALL or INSUFFICIENT_MEMORY could occur.

For non-obvious changes see the commit messages. I'm happy to squash the commits if needed.

The Beta version of this API is missing return codes for various functions. Conversely, with some cases it's debatable whether some of the existing return codes could ever be returned by particular function implementations. This PR tries to harmonise the return codes by adding expected return codes where they are possible. It does not attempt to document existing return codes. There are also a number of functions which need more thought (abort/close/(get|set)_capacity), which are not fully addressed here. These two drawbacks ought to be addressed in a future pull request. Here is a summary of the changes I've made (I've excluded the error prefixes for readability): * It is possible for all functions to return **BAD_STATE** if they haven't initialised the crypto library, so this has been added everywhere. However, it is implementation dependent whether this is returned. * **DOES_NOT_EXIST** is removed in all places except `psa_open_key()`. With the exception of that function, **INVALID_HANDLE** ought to be returned instead. * In cases where there is corruption there is also the opportunity for **STORAGE_FAILURE** to occur. So this has been added. * Small misc cases where **BUFFER_TOO_SMALL** or **INSUFFICIENT_MEMORY** could occur. For non-obvious changes see the commit messages. I'm happy to squash the commits if needed.
Patater (Migrated from github.com) reviewed 2019-08-12 09:25:59 -05:00
gilles-peskine-arm commented 2019-08-12 10:33:51 -05:00 (Migrated from github.com)

There's trailing whitespace. Please make sure tests/scripts/check-files.py passes.

There's trailing whitespace. Please make sure `tests/scripts/check-files.py` passes.
gilles-peskine-arm (Migrated from github.com) reviewed 2019-08-12 11:28:21 -05:00
gilles-peskine-arm (Migrated from github.com) left a comment

Looks good so far. Based on the parameter- and name-based rules I wrote in https://github.com/ARMmbed/psa-crypto/pull/227 , which are far from perfect, some more error codes should be documented:

WARNING: psa_close_key: returns psa_status_t, PSA_ERROR_CORRUPTION_DETECTED expected but not listed
WARNING: psa_close_key: returns psa_status_t, PSA_ERROR_NOT_PERMITTED expected but not listed
WARNING: psa_hash_compute: returns psa_status_t, PSA_ERROR_INVALID_ARGUMENT expected but not listed
WARNING: psa_hash_compare: returns psa_status_t, PSA_ERROR_INVALID_ARGUMENT expected but not listed
WARNING: psa_hash_setup: returns psa_status_t, PSA_ERROR_INVALID_ARGUMENT expected but not listed
WARNING: psa_mac_sign_finish: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed
WARNING: psa_mac_verify_finish: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed
WARNING: psa_cipher_encrypt_setup: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed
WARNING: psa_cipher_decrypt_setup: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed
WARNING: psa_cipher_generate_iv: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed
WARNING: psa_cipher_set_iv: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed
WARNING: psa_cipher_update: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed
WARNING: psa_cipher_finish: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed
WARNING: psa_aead_encrypt: returns psa_status_t, PSA_ERROR_BUFFER_TOO_SMALL expected but not listed
WARNING: psa_aead_encrypt: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed
WARNING: psa_aead_generate_nonce: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed
WARNING: psa_aead_set_nonce: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed
WARNING: psa_aead_set_lengths: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed
WARNING: psa_aead_update_ad: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed
WARNING: psa_aead_update: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed
WARNING: psa_aead_finish: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed
WARNING: psa_aead_verify: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed
WARNING: psa_key_derivation_setup: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed
WARNING: psa_key_derivation_get_capacity: returns psa_status_t, PSA_ERROR_CORRUPTION_DETECTED expected but not listed
WARNING: psa_key_derivation_get_capacity: returns psa_status_t, PSA_ERROR_HARDWARE_FAILURE expected but not listed
WARNING: psa_key_derivation_set_capacity: returns psa_status_t, PSA_ERROR_CORRUPTION_DETECTED expected but not listed
WARNING: psa_key_derivation_set_capacity: returns psa_status_t, PSA_ERROR_HARDWARE_FAILURE expected but not listed
WARNING: psa_key_derivation_input_bytes: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed
WARNING: psa_key_derivation_input_key: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed
WARNING: psa_key_derivation_key_agreement: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed
WARNING: psa_key_derivation_output_bytes: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed
WARNING: psa_key_derivation_output_key: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed

Please let me know which ones you agree/disagree with.

Looks good so far. Based on the parameter- and name-based rules I wrote in https://github.com/ARMmbed/psa-crypto/pull/227 , which are far from perfect, some more error codes should be documented: ``` WARNING: psa_close_key: returns psa_status_t, PSA_ERROR_CORRUPTION_DETECTED expected but not listed WARNING: psa_close_key: returns psa_status_t, PSA_ERROR_NOT_PERMITTED expected but not listed WARNING: psa_hash_compute: returns psa_status_t, PSA_ERROR_INVALID_ARGUMENT expected but not listed WARNING: psa_hash_compare: returns psa_status_t, PSA_ERROR_INVALID_ARGUMENT expected but not listed WARNING: psa_hash_setup: returns psa_status_t, PSA_ERROR_INVALID_ARGUMENT expected but not listed WARNING: psa_mac_sign_finish: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed WARNING: psa_mac_verify_finish: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed WARNING: psa_cipher_encrypt_setup: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed WARNING: psa_cipher_decrypt_setup: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed WARNING: psa_cipher_generate_iv: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed WARNING: psa_cipher_set_iv: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed WARNING: psa_cipher_update: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed WARNING: psa_cipher_finish: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed WARNING: psa_aead_encrypt: returns psa_status_t, PSA_ERROR_BUFFER_TOO_SMALL expected but not listed WARNING: psa_aead_encrypt: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed WARNING: psa_aead_generate_nonce: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed WARNING: psa_aead_set_nonce: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed WARNING: psa_aead_set_lengths: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed WARNING: psa_aead_update_ad: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed WARNING: psa_aead_update: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed WARNING: psa_aead_finish: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed WARNING: psa_aead_verify: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed WARNING: psa_key_derivation_setup: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed WARNING: psa_key_derivation_get_capacity: returns psa_status_t, PSA_ERROR_CORRUPTION_DETECTED expected but not listed WARNING: psa_key_derivation_get_capacity: returns psa_status_t, PSA_ERROR_HARDWARE_FAILURE expected but not listed WARNING: psa_key_derivation_set_capacity: returns psa_status_t, PSA_ERROR_CORRUPTION_DETECTED expected but not listed WARNING: psa_key_derivation_set_capacity: returns psa_status_t, PSA_ERROR_HARDWARE_FAILURE expected but not listed WARNING: psa_key_derivation_input_bytes: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed WARNING: psa_key_derivation_input_key: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed WARNING: psa_key_derivation_key_agreement: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed WARNING: psa_key_derivation_output_bytes: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed WARNING: psa_key_derivation_output_key: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed ``` Please let me know which ones you agree/disagree with.
adrianlshaw commented 2019-08-13 08:29:53 -05:00 (Migrated from github.com)

I can share some of my thought so far:

  • the psa_close_key warnings feel like false positives.
  • Instinct says psa_hash_setup should return BAD_STATE if an invalid operation object is passed in. Is it worth distinguishing here between bad state and invalid argument?
  • I guess it's possible for psa_cipher_generate_iv or psa_aead_generate_nonce to return STORAGE_FAILURE if it is using, for example, a PRNG solution that itself uses secure storage?
  • I guess STORAGE_FAILURE could be asserted at any point by constrained implementations (advanced ones could suspend keys/operations to secure storage and restore them lazily).
I can share some of my thought so far: * the psa_close_key warnings feel like false positives. * Instinct says psa_hash_setup should return BAD_STATE if an invalid operation object is passed in. Is it worth distinguishing here between bad state and invalid argument? * I guess it's possible for psa_cipher_generate_iv or psa_aead_generate_nonce to return STORAGE_FAILURE if it is using, for example, a PRNG solution that itself uses secure storage? * I guess STORAGE_FAILURE could be asserted at any point by constrained implementations (advanced ones could suspend keys/operations to secure storage and restore them lazily).
gilles-peskine-arm (Migrated from github.com) reviewed 2019-08-13 12:02:23 -05:00
include/psa/crypto.h
gilles-peskine-arm (Migrated from github.com) commented 2019-08-13 12:02:23 -05:00

Bad search/replace?

Bad search/replace?
gilles-peskine-arm commented 2019-08-13 12:13:59 -05:00 (Migrated from github.com)
  • psa_close_key: CORRUPTION_DETECTED is possible. NOT_PERMITTED isn't.
  • psa_hash_setup: I expect it to return BAD_STATE if given an operation that's already in use, INVALID_ARGUMENT if given an algorithm that isn't a hash algorithm.
  • Up to now I thought random generation should not depend on storage. If the RNG saves a seed across resets, and loading or updating that seed fails, this would fall under INSUFFICIENT_ENTROPY. But now that you mention it, the PRNG state could be in storage, and in that case STORAGE_FAILURE would make sense. I'd still disallow INSUFFICIENT_STORAGE though: if you store the PRNG outside RAM, make sure you always have enough room.
  • psa_cipher_generate_iv and psa_aead_generate_nonce involve the key, hence key-related errors including STORAGE_FAILURE.
* `psa_close_key`: `CORRUPTION_DETECTED` is possible. `NOT_PERMITTED` isn't. * `psa_hash_setup`: I expect it to return `BAD_STATE` if given an operation that's already in use, `INVALID_ARGUMENT` if given an algorithm that isn't a hash algorithm. * Up to now I thought random generation should not depend on storage. If the RNG saves a seed across resets, and loading or updating that seed fails, this would fall under `INSUFFICIENT_ENTROPY`. But now that you mention it, the PRNG state could be in storage, and in that case `STORAGE_FAILURE` would make sense. I'd still disallow `INSUFFICIENT_STORAGE` though: if you store the PRNG outside RAM, make sure you always have enough room. * `psa_cipher_generate_iv` and `psa_aead_generate_nonce` involve the key, hence key-related errors including `STORAGE_FAILURE`.
gilles-peskine-arm (Migrated from github.com) reviewed 2019-08-14 08:31:54 -05:00
include/psa/crypto.h
gilles-peskine-arm (Migrated from github.com) commented 2019-08-14 08:31:53 -05:00

Apart from this, the PR is ok to merge as far as I'm concerned. It might not be perfect, but it's a major improvement.

Apart from this, the PR is ok to merge as far as I'm concerned. It might not be perfect, but it's a major improvement.
adrianlshaw (Migrated from github.com) reviewed 2019-08-15 04:57:13 -05:00
include/psa/crypto.h
adrianlshaw (Migrated from github.com) commented 2019-08-15 04:57:13 -05:00

Thanks. Not sure how I managed to paste that bit of text here, but it has been removed now.

Thanks. Not sure how I managed to paste that bit of text here, but it has been removed now.
adrianlshaw commented 2019-08-15 05:43:09 -05:00 (Migrated from github.com)

INVALID_ARGUMENT if given an algorithm that isn't a hash algorithm

I would say PSA_ERROR_NOT_SUPPORTED is more appropriate for this case. The documentation already has the following "alg is not supported or is not a hash algorithm"

But now that you mention it, the PRNG state could be in storage, and in that case STORAGE_FAILURE would make sense

Having thought more about your comments on psa_cipher_generate_iv...I would say that storage failure isn't really that important, as what matters to the caller is the fact that there is insufficient entropy. I think it might only be worth distinguishing between storage failure and insufficient entropy if one case is fatal (i.e. the caller shouldn't bother to wait for the system to gain enough entropy).

psa_close_key: CORRUPTION_DETECTED is possible

I'm not sure how this can occur. Some sort of metadata that is written back to storage?

> INVALID_ARGUMENT if given an algorithm that isn't a hash algorithm I would say PSA_ERROR_NOT_SUPPORTED is more appropriate for this case. The documentation already has the following "alg is not supported or is not a hash algorithm" > But now that you mention it, the PRNG state could be in storage, and in that case STORAGE_FAILURE would make sense Having thought more about your comments on psa_cipher_generate_iv...I would say that storage failure isn't really that important, as what matters to the caller is the fact that there is insufficient entropy. I think it might only be worth distinguishing between storage failure and insufficient entropy if one case is fatal (i.e. the caller shouldn't bother to wait for the system to gain enough entropy). > psa_close_key: CORRUPTION_DETECTED is possible I'm not sure how this can occur. Some sort of metadata that is written back to storage?
athoelke commented 2019-08-15 06:36:00 -05:00 (Migrated from github.com)
  • NOT_SUPPORTED usually means: the argument is architecturally valid (might work on some implementations) but this implementation does not support that argument.
  • INVALID_ARGUMENT usually means: the argument is not within the domain specified by the architecture/specification.

psa_close_key() might encounter a key object in the implementation that is corrupted (e.g. due to memory scribbling). In a library implementation, detection of this could report CORRUPTION_DETECTED. In a isolated implementation, this is unlikely to be reported to the caller as memory scribbling within the back end should be treated as a fatal condition within the service.

In general, CORRUPTION_DETECTED in a library implementation warns that something really bad has happened.

- `NOT_SUPPORTED` usually means: the argument is architecturally valid (might work on some implementations) but this implementation does not support that argument. - `INVALID_ARGUMENT` usually means: the argument is not within the domain specified by the architecture/specification. psa_close_key() might encounter a key object in the implementation that is corrupted (e.g. due to memory scribbling). In a library implementation, detection of this could report `CORRUPTION_DETECTED`. In a isolated implementation, this is unlikely to be reported to the caller as memory scribbling within the back end should be treated as a fatal condition within the service. In general, `CORRUPTION_DETECTED` in a library implementation warns that something really bad has happened.
👍 1
adrianlshaw commented 2019-08-15 07:36:47 -05:00 (Migrated from github.com)

Thanks for the feedback, I'm now convinced. I've pushed the remaining changes.

Thanks for the feedback, I'm now convinced. I've pushed the remaining changes.
athoelke (Migrated from github.com) requested changes 2019-08-23 08:58:04 -05:00
athoelke (Migrated from github.com) left a comment

Noticed an editing glitch.

Noticed an editing glitch.
include/psa/crypto.h
athoelke (Migrated from github.com) commented 2019-08-23 08:52:33 -05:00

Line break lost in pasting new error codes.

Line break lost in pasting new error codes.
adrianlshaw (Migrated from github.com) reviewed 2019-08-23 10:12:01 -05:00
include/psa/crypto.h
adrianlshaw (Migrated from github.com) commented 2019-08-23 10:12:01 -05:00

Oops, thanks for spotting this

Oops, thanks for spotting this
adrianlshaw (Migrated from github.com) reviewed 2019-08-23 10:19:55 -05:00
include/psa/crypto.h
adrianlshaw (Migrated from github.com) commented 2019-08-23 10:19:55 -05:00

Fixed. @Patater Gilles says this is OK to merge. Can you take a look?

Fixed. @Patater Gilles says this is OK to merge. Can you take a look?
Patater commented 2019-08-23 11:34:24 -05:00 (Migrated from github.com)

@adrianlshaw Could you please rebase on development and change the target branch of this PR to development? We can do another once over review and merge afterwards should all look good. Thanks!

@adrianlshaw Could you please rebase on `development` and change the target branch of this PR to `development`? We can do another once over review and merge afterwards should all look good. Thanks!
👍 1
adrianlshaw commented 2019-08-29 08:03:06 -05:00 (Migrated from github.com)

Thanks for the heads up @Patater. I've rebased and changed the PR target.

Thanks for the heads up @Patater. I've rebased and changed the PR target.
gilles-peskine-arm (Migrated from github.com) requested changes 2019-09-03 08:37:09 -05:00
gilles-peskine-arm (Migrated from github.com) left a comment

Please fix the trailing whitespace.

Otherwise the changes look good. I did not look at each change in detail or review for exhaustivity, but it's definitely a major improvement. We can make further fixes later.

With the current version of https://github.com/ARMmbed/psa-crypto/pull/227, I get the following warnings:

WARNING: psa_hash_compute: returns psa_status_t, PSA_ERROR_INVALID_ARGUMENT expected but not listed
WARNING: psa_cipher_set_iv: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed
WARNING: psa_cipher_finish: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed
WARNING: psa_aead_encrypt: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed
WARNING: psa_aead_set_lengths: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE listed but unexpected
WARNING: psa_key_derivation_setup: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed

I think they're all warranted.

Please fix the trailing whitespace. Otherwise the changes look good. I did not look at each change in detail or review for exhaustivity, but it's definitely a major improvement. We can make further fixes later. With the current version of https://github.com/ARMmbed/psa-crypto/pull/227, I get the following warnings: ``` WARNING: psa_hash_compute: returns psa_status_t, PSA_ERROR_INVALID_ARGUMENT expected but not listed WARNING: psa_cipher_set_iv: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed WARNING: psa_cipher_finish: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed WARNING: psa_aead_encrypt: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed WARNING: psa_aead_set_lengths: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE listed but unexpected WARNING: psa_key_derivation_setup: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE expected but not listed ``` I think they're all warranted.
adrianlshaw commented 2019-09-03 10:55:10 -05:00 (Migrated from github.com)

Thanks for the review. These issues should be fixed with the latest commits.

Thanks for the review. These issues should be fixed with the latest commits.
gilles-peskine-arm (Migrated from github.com) reviewed 2019-09-03 11:01:49 -05:00
Patater (Migrated from github.com) reviewed 2019-09-03 13:13:50 -05:00
Patater (Migrated from github.com) left a comment
No description provided.
Good improvement. Thanks!
Patater commented 2019-09-03 13:48:36 -05:00 (Migrated from github.com)

@adrianlshaw It doesn't look like this was rebased properly. The pr-head tests are failing, and they'd be fixed if you rebased on the latest development branch. Also, I see a merge commit in the commit history which should go away on a rebase.

@adrianlshaw It doesn't look like this was rebased properly. The pr-head tests are failing, and they'd be fixed if you rebased on the latest development branch. Also, I see a merge commit in the commit history which should go away on a rebase.
adrianlshaw commented 2019-09-04 07:37:57 -05:00 (Migrated from github.com)

Got into a weird state so I'm closing in favour of a new PR (#244)

Got into a weird state so I'm closing in favour of a new PR (#244)
Patater commented 2019-09-04 09:21:36 -05:00 (Migrated from github.com)

@adrianlshaw Could you try using https://github.com/Patater/mbed-crypto/tree/adrian-update-function-return-codes as the branch for this PR? We can re-open this PR and use that branch, so we have the review comment history associated with what we merge.

@adrianlshaw Could you try using https://github.com/Patater/mbed-crypto/tree/adrian-update-function-return-codes as the branch for this PR? We can re-open this PR and use that branch, so we have the review comment history associated with what we merge.
adrianlshaw commented 2019-09-04 10:21:07 -05:00 (Migrated from github.com)

Hi @Patater I just noticed that all checks have passed. Does that mean that this is good to merge? Otherwise I'll use your suggestion.

Hi @Patater I just noticed that all checks have passed. Does that mean that this is good to merge? Otherwise I'll use your suggestion.
Patater commented 2019-09-04 10:27:53 -05:00 (Migrated from github.com)

Looks like your rebase from before worked, at first glance. What weirdness had you run into?

We should review and then we can merge this.

Looks like your rebase from before worked, at first glance. What weirdness had you run into? We should review and then we can merge this.
Patater (Migrated from github.com) reviewed 2019-09-04 10:31:26 -05:00
Patater (Migrated from github.com) left a comment
No description provided.
Changes LGTM
adrianlshaw commented 2019-09-04 11:00:42 -05:00 (Migrated from github.com)

Thanks for the review!

(I thought there was some weirdness because I thought I saw some CI failures...apparently not).

Thanks for the review! (I thought there was some weirdness because I thought I saw some CI failures...apparently not).
gilles-peskine-arm (Migrated from github.com) requested changes 2019-09-04 11:04:11 -05:00
gilles-peskine-arm (Migrated from github.com) left a comment

The rebase has led to some new warnings from my script:

WARNING: psa_hash_compute: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE listed but unexpected
WARNING: psa_hash_compare: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE listed but unexpected
WARNING: psa_mac_verify: returns psa_status_t, PSA_ERROR_BUFFER_TOO_SMALL listed but unexpected
WARNING: psa_mac_abort: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE listed but unexpected
WARNING: psa_cipher_abort: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE listed but unexpected
WARNING: psa_aead_abort: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE listed but unexpected
WARNING: psa_generate_random: returns psa_status_t, PSA_ERROR_INSUFFICIENT_STORAGE listed but unexpected
WARNING: psa_generate_random: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE listed but unexpected

At a glance these look warranted.

The rebase has led to some new warnings from my script: ``` WARNING: psa_hash_compute: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE listed but unexpected WARNING: psa_hash_compare: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE listed but unexpected WARNING: psa_mac_verify: returns psa_status_t, PSA_ERROR_BUFFER_TOO_SMALL listed but unexpected WARNING: psa_mac_abort: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE listed but unexpected WARNING: psa_cipher_abort: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE listed but unexpected WARNING: psa_aead_abort: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE listed but unexpected WARNING: psa_generate_random: returns psa_status_t, PSA_ERROR_INSUFFICIENT_STORAGE listed but unexpected WARNING: psa_generate_random: returns psa_status_t, PSA_ERROR_STORAGE_FAILURE listed but unexpected ``` At a glance these look warranted.
Patater commented 2019-09-04 11:33:00 -05:00 (Migrated from github.com)

The rebase has led to some new warnings from my script:

Do we know those weren't present before the rebase?

Odd that the list is different though, so seems something uncouth happened.

> The rebase has led to some new warnings from my script: Do we know those weren't present before the rebase? - https://github.com/ARMmbed/mbed-crypto/pull/213#issuecomment-520834972 - https://github.com/ARMmbed/mbed-crypto/pull/213#pullrequestreview-282999613 Odd that the list is different though, so seems something uncouth happened.
gilles-peskine-arm commented 2019-09-04 11:35:35 -05:00 (Migrated from github.com)

Some error codes changed since my last pre-rebase review. I didn't investigate where those changes came from.

Some error codes changed since my last pre-rebase review. I didn't investigate where those changes came from.
adrianlshaw commented 2019-09-04 13:11:15 -05:00 (Migrated from github.com)

At a glance these look warranted.

I'd welcome your thoughts on the following... for generate_random these errors could legitimately return if you have a PRNG implementation with a storage dependency.
For operations that involve a key, such as the psa_mac functions, there could be storage failures if the key is lazily loaded by an implementation.
For abort and hash functions I think the warnings are valid...there doesn't seem to be a sensible scenario for those errors.

> At a glance these look warranted. I'd welcome your thoughts on the following... for generate_random these errors could legitimately return if you have a PRNG implementation with a storage dependency. For operations that involve a key, such as the psa_mac functions, there could be storage failures if the key is lazily loaded by an implementation. For abort and hash functions I think the warnings are valid...there doesn't seem to be a sensible scenario for those errors.
gilles-peskine-arm commented 2019-09-04 15:02:36 -05:00 (Migrated from github.com)

psa_mac_xxx can fail with STORAGE_FAILURE because it needs to (re)load the key, but I don't think psa_mac_abort should be allowed to fail due to storage. abort should never fail unless it's given dud arguments or it detects corruption that can only be due to bad arguments or an attack.

For psa_generate_random, I'm not sure. The RNG initialization may load and update a stored seed, but that's done in psa_crypto_init. I don't think the RNG state should be in storage, so functions that need the RNG shouldn't fail due to a storage error (unless it's for a non-RNG-related reason).

`psa_mac_xxx` can fail with `STORAGE_FAILURE` because it needs to (re)load the key, but I don't think `psa_mac_abort` should be allowed to fail due to storage. `abort` should never fail unless it's given dud arguments or it detects corruption that can only be due to bad arguments or an attack. For `psa_generate_random`, I'm not sure. The RNG initialization may load and update a stored seed, but that's done in `psa_crypto_init`. I don't think the RNG state should be in storage, so functions that need the RNG shouldn't fail due to a storage error (unless it's for a non-RNG-related reason).
👍 1
adrianlshaw commented 2019-09-05 05:02:56 -05:00 (Migrated from github.com)

Thanks for the feedback. The latest commits should resolve these warnings (I'm unable to reproduce them locally atm).

Thanks for the feedback. The latest commits should resolve these warnings (I'm unable to reproduce them locally atm).
Patater commented 2019-09-05 05:03:46 -05:00 (Migrated from github.com)

I don't think the RNG state should be in storage, so functions that need the RNG shouldn't fail due to a storage error (unless it's for a non-RNG-related reason).

How about for implementations like Mbed TLS's DRBGs with NV Seed, where storage is involved in both initialization and reseeding? I believe this is what you'd refer to as failing for a non-RNG-related reason. If my understanding is correct, with NV Seed, psa_generate_random() may need to reseed itself, which may involve storage, which may involve storage failure.

> I don't think the RNG state should be in storage, so functions that need the RNG shouldn't fail due to a storage error (unless it's for a non-RNG-related reason). How about for implementations like Mbed TLS's DRBGs with NV Seed, where storage is involved in both initialization and reseeding? I believe this is what you'd refer to as failing for a non-RNG-related reason. If my understanding is correct, with NV Seed, `psa_generate_random()` may need to reseed itself, which may involve storage, which may involve storage failure.
adrianlshaw commented 2019-09-05 05:07:18 -05:00 (Migrated from github.com)

In addition to @Patater's comment, I'll say that it might be appropriate to distinguish the STORAGE_FAILURE case from INSUFFICIENT_ENTROPY because the former seems fatal whereas the latter (to me) implies that it's possible to recover by trying again.

In addition to @Patater's comment, I'll say that it might be appropriate to distinguish the STORAGE_FAILURE case from INSUFFICIENT_ENTROPY because the former seems fatal whereas the latter (to me) implies that it's possible to recover by trying again.
gilles-peskine-arm commented 2019-09-05 12:31:19 -05:00 (Migrated from github.com)

How about for implementations like Mbed TLS's DRBGs with NV Seed, where storage is involved in both initialization and reseeding?

That's a limitation of treating the stored seed as “entropy”. It makes zero sense to use the NV seed when reseeding. The point of reseeding is to bring new non-deterministic input so that an adversary who could obtain a past RNG state is no longer able to find out the current RNG state. If the adversary knows the RNG state from before saving the NV seed, or could observe the NV seed, then “reseeding” from the NV seed does not achieve anything. Thus psa_generate_random should not access the stored seed. This is supposed to be done solely in psa_crypto_init.

That Mbed Crypto doesn't work that way is a bug that we'll resolve when we replace the Mbed TLS entropy module by new code that uses PSA entropy drivers.

it might be appropriate to distinguish the STORAGE_FAILURE case from INSUFFICIENT_ENTROPY

Indeed, but that only affects psa_crypto_init, not psa_generate_random.

> How about for implementations like Mbed TLS's DRBGs with NV Seed, where storage is involved in both initialization and reseeding? That's a limitation of treating the stored seed as “entropy”. It makes zero sense to use the NV seed when reseeding. The point of reseeding is to bring new non-deterministic input so that an adversary who could obtain a past RNG state is no longer able to find out the current RNG state. If the adversary knows the RNG state from before saving the NV seed, or could observe the NV seed, then “reseeding” from the NV seed does not achieve anything. Thus `psa_generate_random` should not access the stored seed. This is supposed to be done solely in `psa_crypto_init`. That Mbed Crypto doesn't work that way is a bug that we'll resolve when we replace the Mbed TLS entropy module by new code that uses PSA entropy drivers. > it might be appropriate to distinguish the STORAGE_FAILURE case from INSUFFICIENT_ENTROPY Indeed, but that only affects `psa_crypto_init`, not `psa_generate_random`.
gilles-peskine-arm (Migrated from github.com) approved these changes 2019-09-05 12:31:58 -05:00
gilles-peskine-arm (Migrated from github.com) reviewed 2019-09-05 12:34:15 -05:00
include/psa/crypto.h
@ -805,2 +831,4 @@
* \retval #PSA_ERROR_BUFFER_TOO_SMALL
* \p hash_size is too small
* \retval #PSA_ERROR_INSUFFICIENT_MEMORY
* \retval #PSA_ERROR_COMMUNICATION_FAILURE
gilles-peskine-arm (Migrated from github.com) commented 2019-09-05 12:34:15 -05:00

You've removed STORAGE_FAILURE from too many functions.

psa_copy_key must access storage if either the source or destination key is persistent.

verify/finish functions for MAC, cipher and AEAD use a key and therefore can access storage in implementations where key material is not systematically kept in RAM during multipart operations.

@Patater I don't want to block merging this before the upcoming Mbed Crypto release, because it's a major improvement as it is. But @adrianlshaw if you can update this before the PR is merged, it would be better.

You've removed `STORAGE_FAILURE` from too many functions. `psa_copy_key` must access storage if either the source or destination key is persistent. verify/finish functions for MAC, cipher and AEAD use a key and therefore can access storage in implementations where key material is not systematically kept in RAM during multipart operations. @Patater I don't want to block merging this before the upcoming Mbed Crypto release, because it's a major improvement as it is. But @adrianlshaw if you can update this before the PR is merged, it would be better.
adrianlshaw (Migrated from github.com) reviewed 2019-09-05 13:47:31 -05:00
include/psa/crypto.h
@ -805,2 +831,4 @@
* \retval #PSA_ERROR_BUFFER_TOO_SMALL
* \p hash_size is too small
* \retval #PSA_ERROR_INSUFFICIENT_MEMORY
* \retval #PSA_ERROR_COMMUNICATION_FAILURE
adrianlshaw (Migrated from github.com) commented 2019-09-05 13:47:31 -05:00

Sorry I'm confused. According to this PR and my copy of the source, STORAGE_FAILURE hasn't been removed at all from psa_copy_key, or any of the mac verify/finish functions. I can see that Github has a little Outdated label on your comment above. I might be mistaken but perhaps you're looking at a stale snapshot?

Sorry I'm confused. According to this PR and my copy of the source, `STORAGE_FAILURE` hasn't been removed at all from psa_copy_key, or any of the mac verify/finish functions. I can see that Github has a little Outdated label on your comment above. I might be mistaken but perhaps you're looking at a stale snapshot?
gilles-peskine-arm (Migrated from github.com) reviewed 2019-09-05 13:58:01 -05:00
include/psa/crypto.h
@ -805,2 +831,4 @@
* \retval #PSA_ERROR_BUFFER_TOO_SMALL
* \p hash_size is too small
* \retval #PSA_ERROR_INSUFFICIENT_MEMORY
* \retval #PSA_ERROR_COMMUNICATION_FAILURE
gilles-peskine-arm (Migrated from github.com) commented 2019-09-05 13:58:00 -05:00

Oh, my bad. When I posted this, I was just looking at the next-to-last commit via the git diff, and git diffs are misleading with documentation above the declaration: git infers the name of the function containing a diff fragment but that's the last declared function, not the function that's being documented. So the function after psa_copy_key had STORAGE_FAILURE removed, etc., which is not a problem. Sorry about that.

Oh, my bad. When I posted this, I was just looking at the next-to-last commit via the git diff, and git diffs are misleading with documentation above the declaration: git infers the name of the function containing a diff fragment but that's the last declared function, not the function that's being documented. So the function _after_ `psa_copy_key` had `STORAGE_FAILURE` removed, etc., which is not a problem. Sorry about that.
adrianlshaw (Migrated from github.com) reviewed 2019-09-05 14:02:47 -05:00
include/psa/crypto.h
@ -805,2 +831,4 @@
* \retval #PSA_ERROR_BUFFER_TOO_SMALL
* \p hash_size is too small
* \retval #PSA_ERROR_INSUFFICIENT_MEMORY
* \retval #PSA_ERROR_COMMUNICATION_FAILURE
adrianlshaw (Migrated from github.com) commented 2019-09-05 14:02:47 -05:00

No problem, I've also found it misleading when reviewing diffs.

No problem, I've also found it misleading when reviewing diffs.
Sign in to join this conversation.
Reviewers
No reviewers
Patater
athoelke
gilles-peskine-arm
Labels
Clear labels   
api-spec
Issue or PR about the PSA specifications

  
bug
Something isn't working

  
CLA requested

   
CLA valid

   
compliance
Discrepancy between the library and the PSA Crypto API specification

  
DO NOT MERGE
The PR is not intended to be merged (yet). Usually used for a review of worked in progress.

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
feature branch
This issue or PR is for a short-term feature branch which doesn't have its own label.

  
good first issue
Good for newcomers

  
HEADER FORMATTING ISSUE

   
help wanted
Extra attention is needed

  
Information

   
invalid
This doesn't seem right

  
Jira status: AWAITING REVIEW

   
Jira status: CLOSED

   
Jira status: DONE

   
Jira status: IN PROGRESS

   
Jira status: IN REVIEW

   
Jira status: OPEN

   
Jira status: REOPENED

   
Jira status: RESOLVED

   
mirrored

   
mirror internally

   
needs: backports
Needs backports to Mbed TLS branches

  
needs: ci
Needs a passing full CI run

  
needs: design review

   
needs: preceding PR
Requires another PR to be merged first

  
needs: review
The pull request is ready for review. This generally means that it has no known issues.

  
needs: work
The pull request needs rework before it can be merged.

  
notabug
The bug is not considered to be a defect, but by design.

  
open for community contribution

   
Product Backlog

   
question
Further information is requested

  
ready for merge
Design and code approved, CI passed, and likewise for backports. Label added by gatekeepers only.

  
security
Security expertise requested in review

  
stale
This PR is not being actively worked on. It can be considered abandoned work.

  
type: bug

   
type: enhancement

   
type: question

   
web pages
The /docs directory in the master branch is shown on https://armmbed.github.io/mbed-crypto

  
wontfix
The cost of fixing the bug or managing it in the bug list is not justified.

  
writing
Documentation wording and formatting

No labels
api-spec
bug
CLA requested
CLA valid
compliance
DO NOT MERGE
duplicate
enhancement
feature branch
good first issue
HEADER FORMATTING ISSUE
help wanted
Information
invalid
Jira status: AWAITING REVIEW
Jira status: CLOSED
Jira status: DONE
Jira status: IN PROGRESS
Jira status: IN REVIEW
Jira status: OPEN
Jira status: REOPENED
Jira status: RESOLVED
mirrored
mirror internally
needs: backports
needs: ci
needs: design review
needs: preceding PR
needs: review
needs: work
notabug
open for community contribution
Product Backlog
question
ready for merge
security
stale
type: bug
type: enhancement
type: question
web pages
wontfix
writing
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-ARMmbed/mbed-crypto!213
No description provided.