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

Switch to a fixed set of DH parameters compatible with older OpenSSL. #2026

Merged
merged 1 commit into from
Jun 8, 2023

Conversation

bbockelm
Copy link
Contributor

@bbockelm bbockelm commented Jun 6, 2023

OpenSSL 3 started generating DH parameters that are not considered valid by DH_check for older OpenSSL 1.0.2.

Since we can't change clients in the wild, I generated a set of DH params (openssl dhparam 2048) on an older OpenSSL 1.0.2 which appears to be considered acceptable by both versions of OpenSSL.

This fixes the set of DH parameters (instead of generating them each time), which is fairly typical, and also increases the size from 512 (insecure) to 2048.

Fixes #2014

@amadio
Copy link
Member

amadio commented Jun 6, 2023

Is this really necessary if we link to OpenSSL 1.1 on CentOS 7? If the RPMs I created today work fine, I can make linking to OpenSSL 1.1 the default in the next releases.

@amadio
Copy link
Member

amadio commented Jun 6, 2023

Could you please fetch at least one recent tag (i.e. v5.5.5) into your fork of xrootd? Or you could delete the 800.800.800-test-tag tag. The CI failure you see is because the version output by genversion.sh (which calls git describe) was not understood by the Python tools. On the main repo it works because the tag v5.5.5 is there.

@abh3
Copy link
Member

abh3 commented Jun 6, 2023

OK, I think we need a test using the RPM you built first, right?

Copy link
Member

@abh3 abh3 left a comment

Choose a reason for hiding this comment

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

OK, I added my 2 cents. I see the problem and am asking a) can we come up with a better solution than downgrading everything, and b) if we can't the original code needs to be kept in comment form so that anyone looking at it can see what was done without resorting to git history to make it painfully apparent.

src/XrdCrypto/XrdCryptosslCipher.cc Show resolved Hide resolved
@bbockelm
Copy link
Contributor Author

bbockelm commented Jun 6, 2023

Is this really necessary if we link to OpenSSL 1.1 on CentOS 7?

No -- but replacing all clients is not an option. For example, we cannot ask experiments to stop using the Run 2 / 3 releases and replace them.

I say this because I have no idea what the security implications are by making this compatible with OpenSSL 1.0.1 which is known to be not very secure

You can read through the OpenSSL ticket for why they made the change. The existing DH code we use leaks a single bit of the session key.

Mind you, the existing DH code we use also leaks all bits of the session key because 512 bit DH was broken in the 1990's.

That way it's easy to get back to a state where fixed DH parameters need not be used

Why? Fixed DH parameters are done everywhere, on almost every webserver on the planet and IPSec. I don't understand the emphasis on dynamically generating the DH parameters when there's no strong cryptographic motivation or risk reduction.

@bbockelm
Copy link
Contributor Author

bbockelm commented Jun 6, 2023

a) can we come up with a better solution than downgrading everything

To be clear -- this moves everything from 512-bit DH (broken) to 2048-bit DH (OK for now). Because of the DH parameter selection, the session key has 127 bits of security currently and remains at 127 bits. If we drop support for RHEL7 (e.g., LHC Run2), we get the session key back to 128 bits of security.

The difference between 127 and 128 for the session key is pretty minimal (it's something like the difference between being broken in 250 billion years and 500 billion years). The difference between 512-bit DH and 2048-bit DH is a huge step forward: it's the difference between "broken in the 90's" and "considered secure for now".

@abh3
Copy link
Member

abh3 commented Jun 6, 2023

OK, thanks for he explanation. While agree about the part that using dynamic DH parameters might not provide you better security I am always amazed that some one at some point will actually point out that fixed parameters allow a security breach even as unlikely it appeared at the time the decision to use fixed parameters was made.So, could you just comment out the deleted code and make it obvious what was changed. I know this is being pedantic and somewhat paranoid but you know how security works. After that I have no problems with the patch subject to testing.

@bbockelm
Copy link
Contributor Author

bbockelm commented Jun 6, 2023

Ok - fixed up the branch so the commit came with much more in-line context about why this is being done.

@amadio
Copy link
Member

amadio commented Jun 7, 2023

I've tested this on an Alma9 machine, it works. I can connect with an existing client from CentOS 7 linked to OpenSSL 1.0. Linking the client with OpenSSL 1.1 also lets it connect to the Alma9 server, so I am considering updating the spec file to do that from next release onwards.

@abh, I leave it to you to decide on merging this.

OpenSSL 3 started generating DH parameters that are not considered
valid by `DH_check` for older OpenSSL 1.0.2.

Since we can't change clients in the wild, I generated a set of DH
params (`openssl dhparam 2048`) on an older OpenSSL 1.0.2 which appears
to be considered acceptable by both versions of OpenSSL.

This fixes the set of DH parameters (instead of generating them each time),
which is fairly typical, and also increases the size from 512 (insecure)
to 2048.
@amadio
Copy link
Member

amadio commented Jun 7, 2023

Looks good now, thanks!

@bbockelm
Copy link
Contributor Author

bbockelm commented Jun 7, 2023

Linking the client with OpenSSL 1.1 also lets it connect to the Alma9 server, so I am considering updating the spec file to do that from next release onwards.

(This may be more of a conversation for the ticket, not the PR but...)

If we're going to have to support OpenSSL 1.0.2, I think it's better to ship against 1.0.2. I'd worry about downstream folks who link against OpenSSL 1.0.2 on RHEL7: what's the implication of having both OpenSSL 1.0.2 and 1.1.1 inside the same process?

Therefore, I would propose we drop support for OpenSSL 1.0.2 for XRootD 6 and change to OpenSSL 1.1.1 then (even on RHEL7). Seems quite strange to have a major change in OpenSSL versions in a minor or patch release. At a major release boundary, I don't mind making the downstream plugins review their linking.

@bbockelm
Copy link
Contributor Author

bbockelm commented Jun 7, 2023

I wanted to note the latest rebase to fix up the nice inputs from @amadio also changed the DH parameters to increase to a 3072-bit size.

This is because, in further reading, 2048 bit DH only provides 112 bits of security for the session key yet the default session key is 128 bits. Despite anything over 100 bits being seen as secure, 112 is not perceived to be as bulletproof as it used to.
For example, XRootD doesn't use an appropriate key derivation after the Diffie Hellman generation of the shared secret, weakening the shared key; I don't know how much that knocks off from the 112 bits of security.

Accordingly, I decided to increase the DH security to 128-bits by using the 3,072 bit parameters. This way, despite the lack of a KDF, that part of the handshake isn't introducing weaknesses..

@bbockelm
Copy link
Contributor Author

bbockelm commented Jun 7, 2023

@abh3 - got a few requests this afternoon to rush a build out to test sites. Would prefer to only do that with the final patch. Any thoughts here?

@abh3
Copy link
Member

abh3 commented Jun 8, 2023

OK, I'll merge this with the notion that @amadio dio can rerun the test with this current fix. He ran a test prior to your change and it worked. Says nothing about it now.

@abh3 abh3 merged commit d080e15 into xrootd:master Jun 8, 2023
14 checks passed
@matyasselmeci
Copy link
Contributor

OSG now has a build of XRootD 5.5.5 with this patch in the osg-testing repos if people are interested in testing. (https://opensciencegrid.atlassian.net/browse/SOFTWARE-5594)

@amadio
Copy link
Member

amadio commented Jun 20, 2023

Thank you, we did test it before merging. The new settings work to allow old clients connect to Alma9 servers. Please do let us know, however, if you have any problems with this patch on top of v5.5.5.

@ddavila0
Copy link

ddavila0 commented Jul 6, 2023

[PLEASE IGNORE this comment, see next comment for explanation]

At UCSD we are deploying some XRootD servers with Alma8 and XRootd-5.5.5 and noticed that when updating from 5.5.5-1.1 to 5.5.5-1.2 TLS stopped working for the client I use for testing. This PR seems to be the culprit.

My client is running 5.4.3-1.1.osg36.el7.x86_64 and
rpm -qa | grep openssl
openssl-1.0.2k-25.el7_9.x86_64
openssl-libs-1.0.2k-25.el7_9.x86_64
globus-gsi-openssl-error-4.4-1.el7.x86_64
globus-openssl-module-5.2-1.el7.x86_64

Auth with both GSI and Scitokens (ztn) seem broken, I keep being asked for my pem key: "Enter PEM pass phrase:"
My proxy looks like this:

voms-proxy-info
subject   : /DC=ch/DC=cern/OU=Organic Units/OU=Users/CN=ddavila/CN=815177/CN=Diego Davila Foyo/CN=76338869
issuer    : /DC=ch/DC=cern/OU=Organic Units/OU=Users/CN=ddavila/CN=815177/CN=Diego Davila Foyo
identity  : /DC=ch/DC=cern/OU=Organic Units/OU=Users/CN=ddavila/CN=815177/CN=Diego Davila Foyo
type      : RFC compliant proxy
strength  : 2048 bits
path      : /tmp/x509up_u0
timeleft  : 9:27:31

The same client works just fine with a server running 5.5.5-1.1

@ddavila0
Copy link

ddavila0 commented Jul 6, 2023

My previous comment refers to an old weird behavior that gets triggered by setting:
X509_USER_CERT and X509_USER_KEY

For some reason and only in very specific combinations of client/server versions, when I try to access an XRootD server via the 'xroot' protocol and the above env vars are set, even if I have a valid proxy in the default location and/or pointed by X509_USER_PROXY the client will try to use my cert and key files instead which triggers a password request.

Unsetting the above env vars fixes the issue.
Maybe I shouldn't be setting X509_USER_CERT and X509_USER_KEY if I'm using a proxy as @abh3 told me a long time ago but what bothers me is the change in behavior from one version to another

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gsi el7 clients fail to connect to alma9 MGM
5 participants