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

Remove MD2, MD4, RC4, Blowfish and XTEA #4588

Merged

Conversation

TRodziewicz
Copy link
Contributor

@TRodziewicz TRodziewicz commented May 31, 2021

Signed-off-by: TRodziewicz tomasz.rodziewicz@mobica.com

Description

Remove the obsolete and niche cryptographic primitives from Mbed TLS 3.0, namely MD2, MD4, RC4, Blowfish and XTEA.

Fixes: #4084

Status

IN DEVELOPMENT

Requires Backporting

NO

Todos

  • Tests
  • Documentation
  • Changelog updated

@TRodziewicz TRodziewicz added enhancement needs-work needs-ci Needs to pass CI tests mbedtls-3 size-s Estimated task size: small (~2d) labels May 31, 2021
@TRodziewicz TRodziewicz self-assigned this May 31, 2021
@TRodziewicz TRodziewicz added needs: changelog needs-ci Needs to pass CI tests 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-ci Needs to pass CI tests needs: changelog needs-work labels May 31, 2021
@gilles-peskine-arm
Copy link
Contributor

Please remove the merge commit and rebase on top of development before the first review.

@gilles-peskine-arm gilles-peskine-arm added needs-work and 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 Jun 7, 2021
@TRodziewicz TRodziewicz force-pushed the remove_MD2_MD4_RC4_Blowfish_and_XTEA branch from a7dcd33 to 4f6c032 Compare June 7, 2021 09:01
@TRodziewicz TRodziewicz 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 Jun 7, 2021
@TRodziewicz TRodziewicz added needs-work and 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 Jun 8, 2021
@TRodziewicz TRodziewicz force-pushed the remove_MD2_MD4_RC4_Blowfish_and_XTEA branch from 4f6c032 to 9b830e1 Compare June 8, 2021 14:59
Signed-off-by: TRodziewicz <tomasz.rodziewicz@mobica.com>
Signed-off-by: TRodziewicz <tomasz.rodziewicz@mobica.com>
@TRodziewicz TRodziewicz force-pushed the remove_MD2_MD4_RC4_Blowfish_and_XTEA branch from 8ff1223 to 8f91c72 Compare June 16, 2021 08:35
ChangeLog.d/issue4084.txt Show resolved Hide resolved
ChangeLog.d/issue4084.txt Show resolved Hide resolved
ChangeLog.d/issue4084.txt Show resolved Hide resolved
tests/data_files/Makefile Outdated Show resolved Hide resolved
doxygen/input/doc_hashing.h Outdated Show resolved Hide resolved
tests/data_files/Makefile Show resolved Hide resolved
tests/suites/test_suite_ccm.data Show resolved Hide resolved
tests/suites/test_suite_psa_crypto.data Show resolved Hide resolved
tests/suites/test_suite_psa_crypto.data Show resolved Hide resolved
tests/suites/test_suite_pkparse.data Outdated Show resolved Hide resolved
@gilles-peskine-arm gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Jun 17, 2021
Reverting some deleted tests and changing the deprecated algo
Deleting deprecated headers from /alt-dummy dir
Corrections to the comments
Removal of deleted functions from compat-2.x.h
Corrections to tests/data_files/Makefile

Signed-off-by: TRodziewicz <tomasz.rodziewicz@mobica.com>
@TRodziewicz TRodziewicz force-pushed the remove_MD2_MD4_RC4_Blowfish_and_XTEA branch from 646424e to 75628d5 Compare June 18, 2021 11:00
@TRodziewicz TRodziewicz added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Jun 18, 2021
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me, except a few more files that could be removed, and one test case that could be restored.

Also, some of the previous migration guide entries mention MD2 and MD4 - can you edit them in order to remove those references? It makes little sense to keep tell people about md2_starts_ret() being renamed now that it's actually entirely removed :) See docs/3.0-migration-guide.d/rename_the__ret_functions.md and docs/3.0-migration-guide.md

tests/data_files/Makefile Show resolved Hide resolved
tests/suites/test_suite_nist_kw.data Show resolved Hide resolved
tests/suites/test_suite_ccm.data Show resolved Hide resolved
@mpg mpg added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Jun 21, 2021
Signed-off-by: TRodziewicz <tomasz.rodziewicz@mobica.com>
@TRodziewicz TRodziewicz added needs-ci Needs to pass CI tests and removed needs-work labels Jun 21, 2021
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

Just one minor issue remaining.

depends_on:MBEDTLS_ARC4_C:MBEDTLS_CIPHER_MODE_CTR
# Either INVALID_ARGUMENT or NOT_SUPPORTED would be reasonable here
cipher_setup:PSA_KEY_TYPE_ARC4:"000102030405060708090a0b0c0d0e0f":PSA_ALG_CTR:PSA_ERROR_NOT_SUPPORTED
cipher_setup:PSA_KEY_TYPE_CHACHA20:"000102030405060708090a0b0c0d0e0f":PSA_ALG_CTR:PSA_ERROR_NOT_SUPPORTED
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a good test case because it has two bad things: the key type is incompatible with the algorithm, and the key size is wrong for the key type. So if psa_cipher_setup returns an error code, we don't know if it's for the reason we want. Please change the key data to be 32 bytes instead of 16.

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

@@ -1397,7 +1397,7 @@ cipher_setup:PSA_KEY_TYPE_RAW_DATA:"000102030405060708090a0b0c0d0e0f":PSA_ALG_CT
PSA cipher setup: incompatible key ChaCha20 for CTR
depends_on:MBEDTLS_ARC4_C:MBEDTLS_CIPHER_MODE_CTR
# Either INVALID_ARGUMENT or NOT_SUPPORTED would be reasonable here
cipher_setup:PSA_KEY_TYPE_CHACHA20:"000102030405060708090a0b0c0d0e0f":PSA_ALG_CTR:PSA_ERROR_NOT_SUPPORTED
cipher_setup:PSA_KEY_TYPE_CHACHA20:"000102030405060708090a0b0c0d0e0f1011121314151617":PSA_ALG_CTR:PSA_ERROR_NOT_SUPPORTED
Copy link
Contributor

Choose a reason for hiding this comment

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

This is 24 bytes, not 32

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooops! True! 1 sec....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, should be better now.

Signed-off-by: TRodziewicz <tomasz.rodziewicz@mobica.com>
@TRodziewicz TRodziewicz force-pushed the remove_MD2_MD4_RC4_Blowfish_and_XTEA branch from 69f3486 to 4a28ade Compare June 21, 2021 15:44
@TRodziewicz TRodziewicz requested a review from mpg June 21, 2021 16:43
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Thanks for addressing our feedback. Looks all good to me now.

@mpg mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-ci Needs to pass CI tests labels Jun 22, 2021
@mpg mpg merged commit a805d57 into Mbed-TLS:development Jun 22, 2021
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 enhancement size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove MD2, MD4, RC4, Blowfish and XTEA
4 participants