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

Return an error from mbedtls_cipher_set_iv for an invalid IV length with ChaCha20 and ChaCha20+Poly #5253

Merged

Conversation

AndrzejKurek
Copy link
Contributor

mbedtls_cipher_set_iv was silently overwritting the iv_len set by the caller, thus leading to potential API misuse.
Fixes #4301.

@AndrzejKurek AndrzejKurek added mbed TLS team needs-review Every commit must be reviewed by at least two team members, component-crypto Crypto primitives and low-level interfaces needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review labels Dec 1, 2021
@AndrzejKurek AndrzejKurek force-pushed the chacha-iv-len-16-fixes branch from c27cd42 to 72f0905 Compare December 2, 2021 00:14
@AndrzejKurek AndrzejKurek removed the needs-ci Needs to pass CI tests label Dec 2, 2021
@mprse mprse self-requested a review January 4, 2022 13:00
Copy link
Contributor

@mprse mprse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks good. I left few minor comments.


ChaCha20 IV Length 12
depends_on:MBEDTLS_CHACHA20_C
check_iv:MBEDTLS_CIPHER_CHACHA20:"CHACHA20":12:0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use macro also on success.


if( cipher_info->type == MBEDTLS_CIPHER_CHACHA20 ||
cipher_info->type == MBEDTLS_CIPHER_CHACHA20_POLY1305 )
iv_len = 12;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that 16 byes for iv is default (and max) length and for those two cipher types we are only limiting the length, but maybe we could first determine the iv length and then allocate memory?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done that :) It has an additional advantage as described in my comment below.

Andrzej Kurek added 5 commits January 14, 2022 16:31
The implementation was silently overwriting the IV length to 12
even though the caller passed a different value.
Change the behavior to signal that a different length is not supported.
Signed-off-by: Andrzej Kurek <[email protected]>
The implementation was silently overwriting the IV length to 12
even though the caller passed a different value.
Change the behavior to signal that a different length is not supported.
Signed-off-by: Andrzej Kurek <[email protected]>
@AndrzejKurek AndrzejKurek force-pushed the chacha-iv-len-16-fixes branch from 4739eab to b9fbc11 Compare January 14, 2022 15:32
@AndrzejKurek AndrzejKurek requested a review from mprse January 14, 2022 15:32
@gabor-mezei-arm gabor-mezei-arm self-requested a review January 18, 2022 14:25
@gabor-mezei-arm gabor-mezei-arm removed the needs-reviewer This PR needs someone to pick it up for review label Jan 18, 2022
Copy link
Contributor

@mprse mprse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@gabor-mezei-arm gabor-mezei-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@gabor-mezei-arm gabor-mezei-arm removed the needs-review Every commit must be reviewed by at least two team members, label Jan 19, 2022
@AndrzejKurek AndrzejKurek added the approved Design and code approved - may be waiting for CI or backports label Jan 19, 2022
@gilles-peskine-arm gilles-peskine-arm merged commit fe271b9 into Mbed-TLS:development Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports component-crypto Crypto primitives and low-level interfaces
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CHACHA20 POLY1305 succeeds with IV size of 16 (OpenSSL CVE-2019-1543)
4 participants