Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Driver-only hashes: PKCS5 #6145

Closed
6 of 8 tasks
mpg opened this issue Jul 28, 2022 · 5 comments · Fixed by #6244
Closed
6 of 8 tasks

Driver-only hashes: PKCS5 #6145

mpg opened this issue Jul 28, 2022 · 5 comments · Fixed by #6244
Assignees
Labels
enhancement size-m Estimated task size: medium (~1w)

Comments

@mpg
Copy link
Contributor

mpg commented Jul 28, 2022

PKCS#5, aka RFC 8018, is a standard for password-based encryption. It defines PBKDF2-HMAC and uses it to derived encryption keys from passwords. It can optionally be used by pkparse.c in order to parse some types of encrypted keys. Currently our implementation (pkcs5.c) uses MD to compute HMACs, so it doesn't work when hashes are provided only by drivers; this task is to make it work.

  • Adjust the signature of mbedtls_pkcs5_pbkdf2_hmac() so that it takes an mbedtls_md_type_t rather than an mbedtls_md_context_t as its first parameter. This frees callers from having to bother with md_info, md_init(), md_setup(), md_free() and centralizes use of MD in one place. Adapt callers including the self_test function. (Note: this is similar to what was done with mgf_mask() in Driver hashes rsa v21 #6141.)
  • Provide an implementation of mbedtls_pkcs5_pbkdf2_hmac() based on PSA, to be used only when MD_C is not available (in order to preserve backwards compatibility: the PSA version requires psa_crypto_init() to have been called, we don't want to impose this requirement on existing code, but we can impose it in builds where this just didn't work at all before). (Again, this is similar to mgf_mask() in Driver hashes rsa v21 #6141.)
  • Adjust the dependency in check_config.h: PKCS5 now only requires MD_C || PSA_CRYPTO_C (plus CIPHER_C as before).
  • Remove the unset PKCS5_C lines from all.sh components component_test_crypto_full_no_md() and component_test_psa_crypto_config_accel_hash_use_psa().
  • Adjust dependencies in test_suite_pkcs5.data, replacing MBEDTLS_SHAxxx_C with MBEDTLS_HAS_ALG_SHA_xxx_VIA_MD_OR_PSA (from legacy_or_psa.h which needs to be #included in the .function file).
  • Similarly adjust hash dependencies in test_suite_pkparse.data for tests that depend on PKCS5_C (again, legacy_or_psa.h needs to be #included in the .function file).
  • Fix any issue that may arise.
  • Check test coverage for test_suite_pkcs5 and test_suite_pkparse: see docs/architecture/psa-migration/outcome-analysis.sh (don't forget to remove unse PKCS5_C in reference_config() and edit SUITES in your copy).

Depends on: #6141, for fixed definitions of VIA_MD_OR_PSA macros (could also just cherry-pick the commit "Fix definition of MD_OR_PSA macros" from that PR).

@mpg mpg added enhancement size-m Estimated task size: medium (~1w) labels Jul 28, 2022
@AndrzejKurek AndrzejKurek self-assigned this Aug 8, 2022
@mpg
Copy link
Contributor Author

mpg commented Aug 31, 2022

Ouch, I really missed something big while writing this task: mbedtls_pkcs5_pbkdf2_hmac() is a public function, we can't change its signature like that. The only thing we can do is add a new function, mbedtls_pkcs5_pbkdf2_hmac_ext() (unless someone comes up with a better name) with the desired signature, and deprecate the old one.

Then change calling sites to use the new function instead of the old one (we're not allowed to use deprecated functions in the library, as we must compile successfully with MBEDTLS_DEPRECATED_REMOVED). And adapt tests accordingly.

In terms of implementation, mbedtls_pkcs5_pbkdf2_hmac() remains unchanged (it just becomes static when MBEDTLS_DEPRECATED_REMOVED is enabled, and not defined at all when MD is not enabled), and the new function will, if MD is available, just initialize the MD context and call mbedtls_pkcs5_pbkdf2_hmac(), or if MD is not available, use PSA directly.

I think that should work. Wdyt @AndrzejKurek ?

@AndrzejKurek
Copy link
Contributor

Sure, no problem :) It's just a cosmetic change, I'll add it.

@AndrzejKurek
Copy link
Contributor

AndrzejKurek commented Sep 1, 2022

There's one issue with this approach: we shouldn't use the deprecated functions in any configuration.
Currently, in my PR, test_full_no_deprecated_deprecated_warning fails, as mbedtls_pkcs5_pbkdf2_hmac is used when MBEDTLS_DEPRECATED_REMOVED is not defined. We can either duplicate the functionality in the non-deprecated function, or extract a common part (the whole of it) and just make a deprecated wrapper. Do you have any other idea for resolving this?

@mpg
Copy link
Contributor Author

mpg commented Sep 1, 2022

Right, I was thinking about this yesterday but apparently didn't think hard enough :) I think the easiest way out of this is to have a static function pkcs5_pbkdf2_hmac() that's identical to the current mbedtls_pkcs5_pbkdf2_hmac() and always implemented (when MBEDTLS_MD_C is defined). Then the public function mbedtls_pkcs5_pbkdf2_hmac() becomes a trivial wrapper around that. Internally (when MD is enabled) we only use the static function, which is not deprecated and always available, so no problem with DEPRECATED_WARNING or DEPRECATED_REMOVED.

So, something like:

#if defined(MBEDTLS_MD_C)
static int pkcs5_pbkdf2_hmac( mbedtls_md_context_t *ctx, ... ) {
    /* current implementation of mbedtls_pkcs5_pbkdf2_hmac() */
}
#if !defined(MBEDTLS_DEPRECATED_REMOVED)
int mbedtls_pkcs5_pbkdf2_hmac( ... ) { /* call the above */ }
#endif
#endif /* MD */

int mbedtls_pkcs5_pbkdf2_hmac_ext( mbedtls_md_type_t ... ) {
#if defined(MBEDTLS_MD_C)
    /* handle ctx and call the static function */
#else
    /* PSA implementation */
#endif
}

Wdyt?

@AndrzejKurek
Copy link
Contributor

Yep, that's exactly what I had in mind by mentioning a deprecated wrapper :) I'll update the PR with this solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement size-m Estimated task size: medium (~1w)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants