Update function return codes

[adrianlshaw]

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.

[gilles-peskine-arm]

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

[gilles-peskine-arm]

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]

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]

Bad search/replace?

[gilles-peskine-arm]

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]

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

[adrianlshaw]

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

[gilles-peskine-arm]

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

[adrianlshaw]

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]

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

[adrianlshaw]

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

[athoelke]

Noticed an editing glitch.

[Patater]

@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]

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

[gilles-peskine-arm]

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]

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

[Patater]

Good improvement. Thanks!

[Patater]

Order of these errors should probably match betweeen psa_cipher_encrypt() and psa_cipher_decrypt(). Not a blocker.

[Patater]

@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]

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

[Patater]

@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]

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]

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]

Changes LGTM

[adrianlshaw]

Thanks for the review!

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

[gilles-peskine-arm]

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]

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

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

• Update function return codes #213 (comment)
• Update function return codes #213 (review)

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

[gilles-peskine-arm]

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

[adrianlshaw]

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]

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

[adrianlshaw]

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

[Patater]

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]

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]

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]

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]

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]

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]

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