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 SSL tests scripts with recent OpenSSL server with Diffie-Hellman #4429

Merged

Conversation

gilles-peskine-arm
Copy link
Contributor

compat.sh and ssl-opt.sh don't work with a modern OpenSSL (e.g. the system one on Ubuntu 20.04) because they force the use of a small DH prime. We do this for compatibility with ancient versions of OpenSSL which we still use for legacy features. Only do it for those ancient versions, not for modern versions. This way you can run the SSL test scripts with the system version of OpenSSL on a modern system (barring other issues which I haven't checked).

This may turn out not to be needed in 3.0 if we remove everything that requires legacy OpenSSL for interoperability testing. If so we can either revert the patch or keep it for similarity with LTS branches.

Backports:

Our interoperability tests fail with a recent OpenSSL server. The
reason is that they force 1024-bit Diffie-Hellman parameters, which
recent OpenSSL (e.g. 1.1.1f on Ubuntu 20.04) reject:
```
140072814650688:error:1408518A:SSL routines:ssl3_ctx_ctrl:dh key too small:../ssl/s3_lib.c:3782:
```

We've been passing custom DH parameters since
6195767 because OpenSSL <=1.0.2a
requires it. This is only concerns the version we use as
OPENSSL_LEGACY. So only use custom DH parameters for that version. In
compat.sh, use it based on the observed version of $OPENSSL_CMD.

This way, ssl-opt.sh and compat.sh work (barring other issues) for all
our reference versions of OpenSSL as well as for a modern system OpenSSL.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm gilles-peskine-arm added enhancement mbed TLS team needs-review Every commit must be reviewed by at least two team members, needs-backports Backports are missing or are pending review and approval. component-tls needs-reviewer This PR needs someone to pick it up for review labels Apr 27, 2021
@daverodgman daverodgman self-requested a review April 30, 2021 14:52
@daverodgman
Copy link
Contributor

daverodgman commented Apr 30, 2021

Tested on Ubuntu 20.04 OpenSSL 1.1.1f

Without the patch I get 35 failures, with it I saw 36. Looking more closely at the first error, I see that without the patch the failure is in o-srv-1.log:

# Fallback SCSV: default, openssl server
openssl s_server -www -cert data_files/server5.crt -key data_files/server5.key -accept 12290 -dhparam data_files/dhparams.pem
Setting temp DH parameters
Error setting temp DH parameters
140042014000448:error:1408518A:SSL routines:ssl3_ctx_ctrl:dh key too small:../ssl/s3_lib.c:3782:
SERVER START TIMEOUT

whereas with the patch, there is no failure in o-srv-1.log, but there is a failure for this test in o-cli-1.log:

...
ssl_msg.c:1963: |2| ssl->f_recv(_timeout)() returned 2 (-0xfffffffe)
ssl_msg.c:1983: |2| <= fetch input
ssl_msg.c:4659: |2| got an alert message, type: [2:70]
ssl_msg.c:4667: |1| is a fatal alert message (msg 70)
ssl_msg.c:3789: |1| mbedtls_ssl_handle_message_type() returned -30592 (-0x7780)
ssl_cli.c:2024: |1| mbedtls_ssl_read_record() returned -30592 (-0x7780)
ssl_tls.c:5451: |2| <= handshake
failed
! mbedtls_ssl_handshake returned -0x7780

Last error was: -0x7780 - SSL - A fatal alert message was received from our peer

So this does seem to fix the key length issue, but other issues remain.

@gilles-peskine-arm
Copy link
Contributor Author

Without the patch, every test case that runs an openssl server fails with a modern openssl. With the patch, some test cases still fail for another reason. For example “Fallback SCSV: default, openssl server” forces the use of TLS 1.1, and it looks like openssl in Ubuntu 20.04 disables TLS 1.1. I'm not sure: I get a different error from the server if I add -tls1_1 to its command line, and I haven't looked at the code or build options, it's not a priority at the moment.

@gilles-peskine-arm
Copy link
Contributor Author

Without the patch I get 35 failures, with it I saw 36.

I don't see how this patch could make more things fail though.

@daverodgman
Copy link
Contributor

I assume some failures were intermittent. I think this is gtg as a partial step towards Ubuntu 20.04 support though.

@gilles-peskine-arm
Copy link
Contributor Author

gilles-peskine-arm commented Apr 30, 2021

Is your “without this patch” the merge-base of this branch and development, or a different vintage of development? If it's a different vintage, it may have a different set of test cases.

I just did a run with a slightly more recent 3.0 than the base of this PR, and all the test cases present in both that fail in this PR also fail in 3.0, but some test cases fail in both.

@daverodgman
Copy link
Contributor

Is your “without this patch” the merge-base of this branch and development, or a different vintage of development?

That was comparing this branch, vs. this branch with "git revert HEAD" applied, so should be like-for-like.

@mpg
Copy link
Contributor

mpg commented May 21, 2021

The travis error seems unrelated (failure to install some python requirements) and can be ignored since Jenkins passed.

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.

LGTM

@mpg mpg 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 May 21, 2021
@mpg
Copy link
Contributor

mpg commented May 21, 2021

The Travis error is unrelated (failure to install a python requirement on windows) and can be ignored since Jenkins passed. Still labelling "needs: ci" while we're waiting for the pr-merge job on Jenkins to complete.

@mpg mpg added needs-ci Needs to pass CI tests approved Design and code approved - may be waiting for CI or backports and removed needs-backports Backports are missing or are pending review and approval. labels May 21, 2021
@mpg
Copy link
Contributor

mpg commented May 21, 2021

Actually the pr-merge job on Jenkins passed but just failed to report, so this is all good.

@mpg mpg removed the needs-ci Needs to pass CI tests label May 21, 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 component-tls enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants