mbedtls: fix possible false success in mbedtls_cipher_check_tag() #6381

tom-cosgrove-arm · 2022-09-30T16:22:31Z

Originally this was PR #2164 - creating a new PR as that was based off user's development branch

I've taken the commit in the original PR, fixed the conflict, and updated the tests to reflect the new return values in the "you shouldn't call these functions in this mode" cases.

This is not a bug fix - calling AEAD-specific functions on non-AEAD algorithms is misuse of the library - rather, this is a change of library behaviour to help badly-written calling code fail safe. Both the previous behaviour and this new behaviour comply with the documentation of these functions. Accordingly I don't think it needs a backport.

Original description:

We should report a error when the security check of the security
tag was not made. In the other case false success is possible and
is not observable by the software.

Technically this could lead to a security flaw.

Signed-off-by: Denis V. Lunev den@openvz.org

Gatekeeper checklist

Tests
ChangeLog.d entry
Backport: not required

We should report a error when the security check of the security tag was not made. In the other case false success is possible and is not observable by the software. Technically this could lead to a security flaw. Signed-off-by: Denis V. Lunev <dlunev@gmail.com>

…icated alg Signed-off-by: Tom Cosgrove <tom.cosgrove@arm.com>

tom-cosgrove-arm · 2022-09-30T16:24:20Z

library/cipher.c

-     * MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA, but at the time I write this our
-     * unit tests assume 0. */
-    ret = 0;
+    /* Status to return on a non-authenticated algorithm. */


While the removed text suggests using MBEDTLS_ERR_CIPHER_INVALID_CONTEXT or MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA, the original PR uses MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE as recommended by Gilles in a comment

Signed-off-by: Tom Cosgrove <tom.cosgrove@arm.com>

gilles-peskine-arm · 2022-10-04T19:10:17Z

FYI CI is failing

…bedtls_cipher_mode_t Signed-off-by: Tom Cosgrove <tom.cosgrove@arm.com>

daverodgman · 2022-11-07T18:13:30Z

ChangeLog.d/fix-possible-false-success-in-mbedtls_cipher_check_tag.txt

+   * Calling AEAD tag-specific functions for non-AEAD algorithms (which should not
+     be done - they are documented for use only by AES-GCM and ChaCha20+Poly1305)
+     now returns MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE instead of success (0).


Suggested change

* Calling AEAD tag-specific functions for non-AEAD algorithms (which should not

be done - they are documented for use only by AES-GCM and ChaCha20+Poly1305)

now returns MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE instead of success (0).

* Calling AEAD tag-specific functions for non-AEAD algorithms (which should

not be done - they are documented for use only by AES-GCM and

ChaCha20+Poly1305) now returns MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE

instead of success (0).

Fix line length

I did wonder about this, but the slightly longer line length makes the right margin much less ragged (I'm sure TeX would approve!)

Changes * Calling AEAD tag-specific functions for non-AEAD algorithms (which should n ot be done - they are documented for use only by AES-GCM and ChaCha20+Poly1305 ) now returns MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE instead of success (0).

The margin may not be ragged, but having not split like this is not easy to read. Please privilege readability over aesthetics.

which to my eye the first is definitely superior

The first is impossible on my system. What's currently in the file looks like what I posted above.

daverodgman

Approved (but needs trivial fix to Changelog line length)

AndrzejKurek · 2022-11-16T13:47:45Z

There's still one issue to fix with the changelog.

tom-cosgrove-arm · 2022-11-16T13:52:19Z

Does the ChangeLog absolutely need a maximum of 80 chars per line? Im my view it's much neater with the only very slightly longer lines, and we already have lines > 80 chars in that file.

gilles-peskine-arm · 2022-11-16T14:07:06Z

More than 80 characters per line won't break any automated system, but it's not neat.

   * Fix a potential side channel vulnerability in ECDSA ephemeral key generati>
     An adversary who is capable of very precise timing measurements could
     learn partial information about the leading bits of the nonce used for the
     signature, allowing the recovery of the private key after observing a
     large number of signature operations. This completes a partial fix in
     Mbed TLS 2.20.0.

tom-cosgrove-arm · 2022-11-16T14:11:30Z

here, this is the difference between

   * Calling AEAD tag-specific functions for non-AEAD algorithms (which should not
     be done - they are documented for use only by AES-GCM and ChaCha20+Poly1305)
     now returns MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE instead of success (0).

and

   * Calling AEAD tag-specific functions for non-AEAD algorithms (which should
     not be done - they are documented for use only by AES-GCM and
     ChaCha20+Poly1305) now returns MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE
     instead of success (0).

which to my eye the first is definitely superior

Signed-off-by: Tom Cosgrove <tom.cosgrove@arm.com>

tom-cosgrove-arm · 2022-11-18T10:30:16Z

CI previously passed; only a ChangeLog entry change; only OpenCI is failing with

tests/ssl-opt.sh: 989: kill: No such process
:
^^^^test_psa_crypto_config_accel_hash_use_psa: test: ssl-opt.sh, MBEDTLS_PSA_CRYPTO_CONFIG with accelerated hash and USE_PSA:

so "something weird happened" on OpenCI - same job on internal CI passes

tom-cosgrove-arm · 2022-11-25T10:43:04Z

Internal CI has passed - failures are OpenCI

dlunev and others added 2 commits September 30, 2022 17:15

Update tests to account for CIPHER_FEATURE_UNAVAILABLE on non-authent…

c621a6d

…icated alg Signed-off-by: Tom Cosgrove <tom.cosgrove@arm.com>

tom-cosgrove-arm commented Sep 30, 2022

View reviewed changes

tom-cosgrove-arm mentioned this pull request Sep 30, 2022

mbedtls: fix possible false success in mbedtls_cipher_check_tag() #2164

Closed

4 tasks

tom-cosgrove-arm marked this pull request as draft September 30, 2022 16:28

tom-cosgrove-arm added the needs-work label Sep 30, 2022

Add ChangeLog entry

51a0163

Signed-off-by: Tom Cosgrove <tom.cosgrove@arm.com>

tom-cosgrove-arm force-pushed the pr2164 branch from 1f7838b to 51a0163 Compare September 30, 2022 17:11

tom-cosgrove-arm marked this pull request as ready for review September 30, 2022 17:11

gilles-peskine-arm removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Oct 4, 2022

daverodgman added the priority-medium Medium priority - this can be reviewed as time permits label Oct 10, 2022

MBEDTLS_CIPHER_CHACHA20_POLY1305 is an mbedtls_cipher_type_t not an m…

edca207

…bedtls_cipher_mode_t Signed-off-by: Tom Cosgrove <tom.cosgrove@arm.com>

tom-cosgrove-arm added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review and removed needs-work labels Oct 14, 2022

daverodgman reviewed Nov 7, 2022

View reviewed changes

daverodgman previously approved these changes Nov 7, 2022

View reviewed changes

AndrzejKurek previously approved these changes Nov 16, 2022

View reviewed changes

AndrzejKurek added needs-work and removed needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review labels Nov 16, 2022

Limit ChangeLog entry to 80 characters

0f0b548

Signed-off-by: Tom Cosgrove <tom.cosgrove@arm.com>

tom-cosgrove-arm dismissed stale reviews from AndrzejKurek and daverodgman via 0f0b548 November 16, 2022 14:24

tom-cosgrove-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Nov 18, 2022

AndrzejKurek approved these changes Nov 18, 2022

View reviewed changes

daverodgman approved these changes Nov 24, 2022

View reviewed changes

tom-cosgrove-arm added approved Design and code approved - may be waiting for CI or backports needs-ci Needs to pass CI tests and removed needs-review Every commit must be reviewed by at least two team members, labels Nov 24, 2022

tom-cosgrove-arm added historical-reviewing Currently reviewing (for legacy PR/issues) and removed needs-ci Needs to pass CI tests labels Nov 25, 2022

daverodgman merged commit f1419db into Mbed-TLS:development Nov 25, 2022

gilles-peskine-arm mentioned this pull request Mar 19, 2024

Symmetric ciphers with invalid key sizes guidovranken/cryptofuzz#69

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mbedtls: fix possible false success in mbedtls_cipher_check_tag() #6381

mbedtls: fix possible false success in mbedtls_cipher_check_tag() #6381

tom-cosgrove-arm commented Sep 30, 2022 •

edited by daverodgman

Loading

tom-cosgrove-arm Sep 30, 2022

gilles-peskine-arm commented Oct 4, 2022

daverodgman Nov 7, 2022

tom-cosgrove-arm Nov 11, 2022

gilles-peskine-arm Nov 16, 2022

gilles-peskine-arm Nov 16, 2022

daverodgman left a comment

AndrzejKurek commented Nov 16, 2022

tom-cosgrove-arm commented Nov 16, 2022

gilles-peskine-arm commented Nov 16, 2022

tom-cosgrove-arm commented Nov 16, 2022

tom-cosgrove-arm commented Nov 18, 2022 •

edited

Loading

tom-cosgrove-arm commented Nov 25, 2022

mbedtls: fix possible false success in mbedtls_cipher_check_tag() #6381

mbedtls: fix possible false success in mbedtls_cipher_check_tag() #6381

Conversation

tom-cosgrove-arm commented Sep 30, 2022 • edited by daverodgman Loading

Gatekeeper checklist

tom-cosgrove-arm Sep 30, 2022

Choose a reason for hiding this comment

gilles-peskine-arm commented Oct 4, 2022

daverodgman Nov 7, 2022

Choose a reason for hiding this comment

tom-cosgrove-arm Nov 11, 2022

Choose a reason for hiding this comment

gilles-peskine-arm Nov 16, 2022

Choose a reason for hiding this comment

gilles-peskine-arm Nov 16, 2022

Choose a reason for hiding this comment

daverodgman left a comment

Choose a reason for hiding this comment

AndrzejKurek commented Nov 16, 2022

tom-cosgrove-arm commented Nov 16, 2022

gilles-peskine-arm commented Nov 16, 2022

tom-cosgrove-arm commented Nov 16, 2022

tom-cosgrove-arm commented Nov 18, 2022 • edited Loading

tom-cosgrove-arm commented Nov 25, 2022

tom-cosgrove-arm commented Sep 30, 2022 •

edited by daverodgman

Loading

tom-cosgrove-arm commented Nov 18, 2022 •

edited

Loading