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

mbedtls_cipher functions return 0 in invalid scenarios #5323

Closed
gilles-peskine-arm opened this issue Dec 13, 2021 · 2 comments
Closed

mbedtls_cipher functions return 0 in invalid scenarios #5323

gilles-peskine-arm opened this issue Dec 13, 2021 · 2 comments
Labels
api-break This issue/PR breaks the API and must wait for a new major version bug component-crypto Crypto primitives and low-level interfaces size-s Estimated task size: small (~2d)

Comments

@gilles-peskine-arm
Copy link
Contributor

gilles-peskine-arm commented Dec 13, 2021

If you call mbedtls_cipher_update_ad or mbedtls_cipher_write_tag or mbedtls_cipher_check_tag on a non-AEAD algorithm, they return 0. This looks wrong: surely it's an application error, and it's easily detectable, so we should return MBEDTLS_ERR_CIPHER_INVALID_CONTEXT or MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA?

Our unit tests indiscriminately call these functions on non-AEAD algorithms though. Is this just a test bug or is this deliberate? The documentation doesn't say anything explicit.

Similar pattern with mbedtls_cipher_set_iv on ECB.

Since the unit tests validate the current behavior, if we change it, we should probably not change LTS branches.

Note: moot if we remove cipher.h as a public API.

@gilles-peskine-arm gilles-peskine-arm added bug component-crypto Crypto primitives and low-level interfaces Product Backlog size-s Estimated task size: small (~2d) labels Dec 13, 2021
@gilles-peskine-arm
Copy link
Contributor Author

For mbedtls_cipher_set_iv, this is documented behavior:

Some ciphers do not use IVs nor nonce. For these ciphers, this function has no effect.

Also:

[iv_len] is discarded by ciphers with fixed-size IV.

This is unfortunate, but we can't change such documented behavior in a minor release. So it has to wait until 4.0, but by that time the mbedtls_cipher interface will be deprecated in favor of PSA anyway.

@gilles-peskine-arm
Copy link
Contributor Author

Moot since we're removing cipher.h as a public API (#8451).

@gilles-peskine-arm gilles-peskine-arm closed this as not planned Won't fix, can't repro, duplicate, stale Aug 7, 2024
@github-project-automation github-project-automation bot moved this from Requirements needed to Done in Mbed TLS 4.0 planning Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-break This issue/PR breaks the API and must wait for a new major version bug component-crypto Crypto primitives and low-level interfaces size-s Estimated task size: small (~2d)
Projects
None yet
Development

No branches or pull requests

3 participants