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

Fix RC4 based ciphersuites #13378

Closed
wants to merge 3 commits into from
Closed

Conversation

@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Nov 11, 2020

Commit be9d82b inadvertently disabled ciphersuite testing. This masked two separate issues in the RC4 ciphersuites.

In 1 issue RC4 based ciphersuites fail the properly extract the MAC from decrypted TLS data.

In the 2nd issue the RC4-MD5 stitched ciphersuites fails to adjust the length of the decrypted data appropriately to account for the MAC.

This PR fixes both of those issues, as well as fixing the ciphersuite testing. Note that in fixing the testing I had to make some adjustments to take account of the FIPS module. This required some changes to the DH handling in the test. Currently that test uses the old "low-level" DH object instead of an EVP_PKEY - therefore this PR does the same in order to play nicely with the existing code. That issue is fixed by another PR (#13368). Whichever PR goes in second (this one or #13368) will have to be adjusted to use EVP_PKEY instead of DH.

Fixes #13363

@t8m
t8m approved these changes Nov 11, 2020
Copy link
Member

@t8m t8m left a comment

LGTM. Approved if CI passes.

@paulidale
Copy link
Contributor

@paulidale paulidale commented Nov 11, 2020

Travis is relevant.

@slontis slontis requested a review from t8m Nov 11, 2020
@paulidale paulidale modified the milestones: 3.0.0 beta1, 3.0.0 Nov 17, 2020
@mattcaswell
Copy link
Member Author

@mattcaswell mattcaswell commented Nov 17, 2020

Fixup pushed to address Travis failure

@openssl-machine
Copy link

@openssl-machine openssl-machine commented Nov 18, 2020

This pull request is ready to merge

mattcaswell added 3 commits Nov 10, 2020
We previously updated the block ciphers to know how to remove a TLS
MAC when using Encrypt-then-MAC. We also need to do the same for stream
ciphers.

Fixes #13363
The RC4-MD5 ciphersuites were not removing the length of the MAC when
calculating the length of decrypted TLS data. Since RC4 is a streamed
cipher that doesn't use padding we separate out the concepts of fixed
length TLS data to be removed, and TLS padding.
Commit be9d82b inadvertently disabled ciphersuite testing. This masked
some issues. Therefore we fix this testing.
@mattcaswell mattcaswell force-pushed the mattcaswell:stream-mac-removal branch to 21305bf Nov 19, 2020
@mattcaswell
Copy link
Member Author

@mattcaswell mattcaswell commented Nov 19, 2020

Unfortunately #13368 was merged which touched the same ssltest_old.c code as this PR. Therefore I had to rewrite some of that code.

@paulidale - please can you reconfirm your approval?

@mattcaswell
Copy link
Member Author

@mattcaswell mattcaswell commented Nov 23, 2020

Travis failures are not relevant.

Ping @paulidale.

@t8m
t8m approved these changes Nov 23, 2020
@openssl-machine
Copy link

@openssl-machine openssl-machine commented Nov 24, 2020

This pull request is ready to merge

@mattcaswell
Copy link
Member Author

@mattcaswell mattcaswell commented Nov 25, 2020

Pushed. Thanks.

openssl-machine pushed a commit that referenced this pull request Nov 25, 2020
We previously updated the block ciphers to know how to remove a TLS
MAC when using Encrypt-then-MAC. We also need to do the same for stream
ciphers.

Fixes #13363

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #13378)
openssl-machine pushed a commit that referenced this pull request Nov 25, 2020
The RC4-MD5 ciphersuites were not removing the length of the MAC when
calculating the length of decrypted TLS data. Since RC4 is a streamed
cipher that doesn't use padding we separate out the concepts of fixed
length TLS data to be removed, and TLS padding.

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #13378)
openssl-machine pushed a commit that referenced this pull request Nov 25, 2020
Commit be9d82b inadvertently disabled ciphersuite testing. This masked
some issues. Therefore we fix this testing.

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #13378)
@nelsonopenssl nelsonopenssl added this to Ready to merge in 3.0.0 estimator Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
3.0.0 estimator
Ready to merge
Linked issues

Successfully merging this pull request may close these issues.

5 participants