Adding shortcut for all-bits-zero payloads (mbedtls_ecp_mul_shortcuts… #4172
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
paul-elliott-arm
added
component-crypto
paul-elliott-arm
added
bug
needs-backports
Signed-off-by: Gilles Peskine <[email protected]> Signed-off-by: TRodziewicz <[email protected]>
The code will be moved in a subsequent commit. Signed-off-by: Gilles Peskine <[email protected]> Signed-off-by: TRodziewicz <[email protected]>
Rename functions to mbedtls_test_psa_xxx if they're going to be exported. Declare functions as static if they're aren't meant to be called directly from test code. Signed-off-by: Gilles Peskine <[email protected]> Signed-off-by: TRodziewicz <[email protected]>
exercise_export_key() exports the key and does sanity checks on the result. Here we've already just exported the key, so just run the sanity checks. Signed-off-by: Gilles Peskine <[email protected]> Signed-off-by: TRodziewicz <[email protected]>
The code will be moved in a subsequent commit. Signed-off-by: Gilles Peskine <[email protected]> Signed-off-by: TRodziewicz <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]> Signed-off-by: TRodziewicz <[email protected]>
Move mbedtls_test_psa_exercise_key() (formerly exercise_key()) and related functions to its own module. Export the few auxiliary functions that are also called directly. Signed-off-by: Gilles Peskine <[email protected]> Signed-off-by: TRodziewicz <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]> Signed-off-by: TRodziewicz <[email protected]>
Remove a conditional imbrication level. Get rid of some minor overhead for ECC public keys dating back from when they had ASN.1 wrapping. No behavior change. Signed-off-by: Gilles Peskine <[email protected]> Signed-off-by: TRodziewicz <[email protected]>
The const-ness has to be cast away when calling mbedtls_asn1_xxx parsing functions. This is a known flaw in the mbedtls API (Mbed-TLS#803). Signed-off-by: Gilles Peskine <[email protected]> Signed-off-by: TRodziewicz <[email protected]>
Shuffle the logic in mbedtls_test_psa_exported_key_sanity_check() somewhat. The resulting behavior changes are: * Always check the exported length against PSA_EXPORT_KEY_OUTPUT_SIZE, even for unstructured key types. * Always complain if a key type is not explicitly covered, not just for public keys. Signed-off-by: Gilles Peskine <[email protected]> Signed-off-by: TRodziewicz <[email protected]>
mbedtls_test_fail does not copy the failure explanation string, so passing a string on the stack doesn't work. This fixes a garbage message that would appear if a test triggered a non-implemented code path. More generally, just use TEST_ASSERT instead of explicitly calling mbedtls_test_fail, since we aren't playing any tricks with the error location. Signed-off-by: Gilles Peskine <[email protected]> Signed-off-by: TRodziewicz <[email protected]>
Rename functions to mbedtls_test_xxx and make them non-static if they're going to be exported. Signed-off-by: Gilles Peskine <[email protected]> Signed-off-by: TRodziewicz <[email protected]>
Persistent storage common code from test_suite_psa_crypto_slot_management.function had been duplicated in test_suite_psa_crypto_se_driver_hal.function and the copy had slightly diverged. Re-align the copy in preparation from moving the code to a common module and using that sole copy in both test suites. Signed-off-by: Gilles Peskine <[email protected]> Signed-off-by: TRodziewicz <[email protected]>
Merge the two identical definitions of TEST_USES_KEY_ID and mbedtls_test_psa_purge_key_storage from test_suite_psa_crypto_slot_management.function and test_suite_psa_crypto_se_driver_hal.function into a single copy in common test code so that it can be used in all test suites. No semantic change. Signed-off-by: Gilles Peskine <[email protected]> Signed-off-by: TRodziewicz <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]> Signed-off-by: TRodziewicz <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]> Signed-off-by: TRodziewicz <[email protected]>
This ensures that test cases won't leave persistent files behind even on failure, provided they use TEST_USES_KEY_ID(). Test cases that don't use this macro are unaffected. Tests that use PSA_DONE() midway and expect persistent keys to survive must use PSA_SESSION_DONE() instead. Signed-off-by: Gilles Peskine <[email protected]> Signed-off-by: TRodziewicz <[email protected]>
This makes failure messages easier to understand. Signed-off-by: Gilles Peskine <[email protected]> Signed-off-by: TRodziewicz <[email protected]>
MSVC started (rightfully) complaining after moving the code to a separate .c file. Signed-off-by: Gilles Peskine <[email protected]> Signed-off-by: TRodziewicz <[email protected]>
ARRAY_LENGTH has a portable but unsafe implementation, and a non-portable implementation that causes a compile-time error if the macro is accidentally used on a pointer. The safety check was only implemented for __GCC__-defining compilers, but the part that triggered the compile-time error was always used. It turns out that this part triggers a build warning with MSVC (at least with some versions: observed with Visual Studio 2013). ``` C:\builds\workspace\mbed-tls-pr-head_PR-4141-head\src\tests\src\psa_crypto_helpers.c(52): error C2220: warning treated as error - no 'object' file generated [C:\builds\workspace\mbed-tls-pr-head_PR-4141-head\src\mbedtls_test.vcxproj] C:\builds\workspace\mbed-tls-pr-head_PR-4141-head\src\tests\src\psa_crypto_helpers.c(52): warning C4116: unnamed type definition in parentheses [C:\builds\workspace\mbed-tls-pr-head_PR-4141-head\src\mbedtls_test.vcxproj] ``` Since a compile-time error is never triggered when the compile-time check for the argument type is not implemented, just use the unsafe macro directly when there's no safety check. Signed-off-by: Gilles Peskine <[email protected]> Signed-off-by: TRodziewicz <[email protected]>
The primary goal of this commit is to fix various comments where `clang -Wdocumentation` identified a discrepancy between the actual function parameters and the documented parameters. The discrepancies were due to copypasta, formatting issues or documentation that had diverged from the implementation. Signed-off-by: Gilles Peskine <[email protected]> Signed-off-by: TRodziewicz <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]> Signed-off-by: TRodziewicz <[email protected]>
Time stamps are useful when the document gets shared around, but they tend to lead to merge conflicts. Signed-off-by: Gilles Peskine <[email protected]> Signed-off-by: TRodziewicz <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]> Signed-off-by: TRodziewicz <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]> Signed-off-by: TRodziewicz <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]> Signed-off-by: TRodziewicz <[email protected]>
…()) and returning proper error code (MBEDTLS_ERR_ECP_INVALID_KEY) for that case (ecjpake_zkp_read()). Signed-off-by: TRodziewicz <[email protected]>
TRodziewicz
force-pushed
the
new_mul_shortcut
branch
from
f435953 to
5ff8aa0
Compare
Signed-off-by: TRodziewicz <[email protected]>
TRodziewicz
force-pushed
the
new_mul_shortcut
branch
from
5ff8aa0 to
9d99348
Compare
|
I'm closing this PR because it was easier to open a new one than fixing the multiple commits it caught. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…()) and returning proper error code (MBEDTLS_ERR_ECP_INVALID_KEY) for that case (ecjpake_zkp_read()).
Description
Resolves: #1792
Short issue #1792 description:
"ECDSA verification (mbedtls_ecdsa_verify or mbedtls_ecdsa_read_signature) returns MBEDTLS_ERR_ECP_INVALID_KEY when the payload (which should be a hash) is all-bits zero. The curve is a short Weierstrass curve."
root cause: ecp_mul_restartable() function is very strict and don't handle situation like that.
Fix: There was a new shortcut added to the mbedtls_ecp_mul_shortcuts() It is a shortcut for a situation when m == 0.
Additionally ecjpake_zkp_read() returns proper error code (MBEDTLS_ERR_ECP_INVALID_KEY) for that case.
Status
READY
Requires Backporting
NO
Migrations
NO
Todos
Steps to test or reproduce
Please check: #1792