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

crypt_and_hash program is not working with CHACHA20-POLY1305 or CCM or ECB #5445

Open
Oldes opened this issue Jan 20, 2022 · 4 comments
Open
Labels
bug component-crypto Crypto primitives and low-level interfaces good-first-issue Good for newcomers help-wanted This issue is not being actively worked on, but PRs welcome. historical-reviewed Reviewed & agreed to keep legacy PR/issue

Comments

@Oldes
Copy link
Contributor

Oldes commented Jan 20, 2022

Summary

When running a crypt_and_hash example program without arguments, it lists also CHACHA20-POLY1305 as an available cipher.
But when used, it throws: mbedtls_cipher_update() returned error from here:
https://github.com/ARMmbed/mbedtls/blob/d2da19b8eb60b75962f45c38e0b8222d917696f6/library/chachapoly.c#L194-L198

I suppose it is not supported for use with the generic cipher api as it requires an unique nonce input for every encryption operation.

@gilles-peskine-arm gilles-peskine-arm changed the title crypt_and_hash program is not working with CHACHA20-POLY1305 cipher crypt_and_hash program is not working with CHACHA20-POLY1305 or CCM Jan 20, 2022
@gilles-peskine-arm gilles-peskine-arm changed the title crypt_and_hash program is not working with CHACHA20-POLY1305 or CCM crypt_and_hash program is not working with CHACHA20-POLY1305 or CCM or ECB Jan 20, 2022
@gilles-peskine-arm
Copy link
Contributor

An error also occurs with CCM. But GCM works. This program was evidently not intended for AEAD, but the API is similar enough that GCM works.

For CCM, the problem is that the program doesn't call mbedtls_cipher_set_lengths, which is required for CCM.

For ChaChaPoly and CCM and ECB, there's also the problem that the program passes a 16-byte IV, but these modes require a smaller IV (or none at all for ECB). It's weird that mbedtls_cipher_set_iv isn't failing though, this may be a separate bug.

Possible solutions for crypt_and_hash itself:

  • Make it filter out the cipher names that it doesn't support in its help text.
  • Make it actually work with all the modes.
  • Remove it because it's obsolete. It's DIY AEAD, which I guess was all we had when the program was written, but these days we should encourage standard AEAD modes. And it does a weird key stretching by hashing 8192 times, which is unnecessary if the key is a proper one and insufficient if the “key” is actually a password. This is also bad practice.

@gilles-peskine-arm gilles-peskine-arm added bug component-crypto Crypto primitives and low-level interfaces Product Backlog labels Jan 20, 2022
@gilles-peskine-arm
Copy link
Contributor

Actually mbedtls_cipher_set_iv does nothing, silently, for ECB, and truncates the IV for ChachaPoly. These are known issues.

The reason for mbedtls_cipher_update failing for ECB is that it requires exactly one block: our ECB implementation doesn't support padding. The program actually works with ECB if the input size is a multiple of the cipher's block size.

For Chachapoly, it looks like the API does not support skipping a call to mbedtls_cipher_set_ad even if it's empty. This is user-unfriendly and inconsistent with other AEAD modes so we should fix it..

@Oldes
Copy link
Contributor Author

Oldes commented Jan 20, 2022

I wonder if this generic layer used in this example is still recommended as a generic approach to support multiple ciphers?

@gilles-peskine-arm
Copy link
Contributor

We now recommend psa_xxx instead of mbedtls_xxx functions for cryptography where possible. Going forward, driver support and optimizations will focus on psa_xxx. However, if you're currently using mbedtls_xxx, you may want to wait because some cases are not yet supported. In particular, there's no discovery interface yet: nothing similar to mbedtls_cipher_list(). So if you want to list all supported ciphers, mbedtls_cipher_list() is still your only choice.

@daverodgman daverodgman added historical-reviewing Currently reviewing (for legacy PR/issues) help-wanted This issue is not being actively worked on, but PRs welcome. good-first-issue Good for newcomers historical-reviewed Reviewed & agreed to keep legacy PR/issue and removed historical-reviewing Currently reviewing (for legacy PR/issues) labels Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug component-crypto Crypto primitives and low-level interfaces good-first-issue Good for newcomers help-wanted This issue is not being actively worked on, but PRs welcome. historical-reviewed Reviewed & agreed to keep legacy PR/issue
Projects
None yet
Development

No branches or pull requests

4 participants