Initialise return values to an error #323
Conversation
yanesca
force-pushed
the
iotcrypt-942-initialise-return-values
branch
2 times, most recently
from
cbb35dc to
82b3173
Compare
yanesca
added
needs: preceding PR
|
CI fails because Mbed OS uses Mbed TLS's |
gilles-peskine-arm
added
needs: work
One of the error codes was already reserved, this commit just makes it explicit. The other one is a new error code for initializing return values in the library: `MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED` should not be returned by the library. If it is returned, then it is surely a bug in the library or somebody is tampering with the device.
Initialising the return values to and error is best practice and makes the library more robust.
yanesca
force-pushed
the
iotcrypt-942-initialise-return-values
branch
from
82b3173 to
24eed8d
Compare
yanesca
added
needs: review
|
I have added a new error code for this purpose and simplified the approach as well. The PR is ready for review again. |
|
There are many CI failures. I don't think this PR is ready. |
|
CI fails because Mbed OS uses Mbed TLS's error.h and should pass once Mbed TLS has been updated. |
|
In retrospect I probably should have started with the Mbed TLS PR, but now I would prefer to continue with the review of this one and raise the Mbed TLS counterpart when this is approved. (I would like to do them sequentially to minimise duplicating work during review.) |
|
Ok. Reviewing. |
There was a problem hiding this comment.
LGTM
There are a few places where we use int ret = 0; and we could change it to int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;. I found: mbedtls_cipher_finish, block_cipher_df, ctr_drbg_update_internal, mbedtls_ctr_drbg_random_with_add, mbedtls_ctr_drbg_update_seed_file, mbedtls_ecdsa_genkey, mbedtls_ecp_read_key, entropy_update, mbedtls_entropy_update_seed_file, mbedtls_hkdf_expand, mbedtls_hmac_drbg_update_seed_file, mbedtls_nist_kw_wrap, mbedtls_nist_kw_unwrap, mbedtls_rsa_rsassa_pkcs1_v15_verify.
include/mbedtls/error.h
Outdated
| * For historical reasons, low-level error codes are divided in even and odd, | ||
| * even codes were assigned first, and -1 is reserved for other errors. | ||
| * | ||
| * Low-level module errors (0x0002-0x007E, 0x0003-0x007F) |
There was a problem hiding this comment.
Please update this information.
include/mbedtls/error.h
Outdated
| * even codes were assigned first, and -1 is reserved for other errors. | ||
| * | ||
| * Low-level module errors (0x0002-0x007E, 0x0003-0x007F) | ||
| * Low-level module errors (0x0001-0x007E, 0x0003-0x007F) |
There was a problem hiding this comment.
Shouldn't it rather be (even, odd)?
| * Low-level module errors (0x0001-0x007E, 0x0003-0x007F) | |
| * Low-level module errors (0x0002-0x007E, 0x0001-0x007F) |
Patater
added
the
needs: work
yanesca
force-pushed
the
iotcrypt-942-initialise-return-values
branch
from
f00b840 to
9c2ccd2
Compare
yanesca
removed
the
needs: work
gilles-peskine-arm
removed
the
needs: review
Previously in d875285: * ARMmbed#333: Streamline PSA key type encodings: prepare * ARMmbed#323: Initialise return values to an error Previously in dbcb442: * ARMmbed#291: Test MBEDTLS_CTR_DRBG_USE_128_BIT_KEY * ARMmbed#334: Fix some pylint warnings Previously in ceceedb: * ARMmbed#348: Bump version to Mbed TLS 2.20.0 and crypto SO version to 4 * ARMmbed#354: Fix incrementing pointer instead of value In this commit: * ARMmbed#349: Fix minor defects found by Coverity * ARMmbed#179: Add option to build SHA-512 without SHA-384 * ARMmbed#327: Implement psa_hash_compute and psa_hash_compare * ARMmbed#330: Streamline PSA key type and curve encodings
4 participants
Initialising the return values to an error is best practice and makes the library more robust.
No new error was introduced because we are running out of error codes and we shouldn't be wasting them.We already had an error code reserved for generic errors. This PR makes it explicit and uses this error code to initialise return values.Define a new error code
MBEDTLS_ERR_ERROR_CORRUPTION_DETECTEDfor this purpose.