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

openssl: use OpenSSL's default ciphers by default #1846

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@kdudka
Collaborator

kdudka commented Aug 30, 2017

Up2date versions of OpenSSL maintain the default reasonably secure
without breaking compatibility, so it is better not to override the
default by curl. Suggested at https://bugzilla.redhat.com/1483972

@vszakats vszakats added the SSL/TLS label Aug 30, 2017

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 30, 2017

Coverage Status

Coverage decreased (-0.1%) to 73.081% when pulling 0b61386 on kdudka:openssl-default-ciphers into 7ec797b on curl:master.

coveralls commented Aug 30, 2017

Coverage Status

Coverage decreased (-0.1%) to 73.081% when pulling 0b61386 on kdudka:openssl-default-ciphers into 7ec797b on curl:master.

@bagder

bagder approved these changes Aug 30, 2017

I think this is awesome and takes away a burden from us that we really are better without... 👍

@frenche

This comment has been minimized.

Show comment
Hide comment
@frenche

frenche Aug 31, 2017

Contributor

+1 for this approach.

Contributor

frenche commented Aug 31, 2017

+1 for this approach.

Show outdated Hide outdated lib/vtls/openssl.c
@@ -154,8 +154,16 @@ static unsigned long OpenSSL_version_num(void)
#define OSSL_PACKAGE "OpenSSL"
#endif
#if (OPENSSL_VERSION_NUMBER >= 0x10002000L)

This comment has been minimized.

@jay

jay Aug 31, 2017

Member

I wouldn't change it for at least for OpenSSL 1.0.2, which includes RC4 MD5 support and maybe other things depending on how it was built. By default our current cipher settings disable RC4, but this would change that so -1 for that specific thing.

The commit referred to in that thread you quoted probably concerns just 1.1, and it does say RC4 is not part of the default cipher list when built in. So my suggestion is do it for OpenSSL 1.1 and above. The only other thing is @STRENGTH, I really don't know how important that is for 1.1.

@jay

jay Aug 31, 2017

Member

I wouldn't change it for at least for OpenSSL 1.0.2, which includes RC4 MD5 support and maybe other things depending on how it was built. By default our current cipher settings disable RC4, but this would change that so -1 for that specific thing.

The commit referred to in that thread you quoted probably concerns just 1.1, and it does say RC4 is not part of the default cipher list when built in. So my suggestion is do it for OpenSSL 1.1 and above. The only other thing is @STRENGTH, I really don't know how important that is for 1.1.

This comment has been minimized.

@bagder

bagder Aug 31, 2017

Member

Oh right, the @STRENGTH. I added that back in commit 0d1060f as a sort of work-around for HTTP/2 since back then OpenSSL would otherwise happily negotiate using ciphers that are deemed "illegal" for HTTP/2 ...

@bagder

bagder Aug 31, 2017

Member

Oh right, the @STRENGTH. I added that back in commit 0d1060f as a sort of work-around for HTTP/2 since back then OpenSSL would otherwise happily negotiate using ciphers that are deemed "illegal" for HTTP/2 ...

This comment has been minimized.

@t8m

t8m Aug 31, 2017

Using OPENSSL_VERSION_NUMBER >= 0x10100000L should be of course OK. However I do not see how using @strength would help to eliminate "illegal" ciphers. This just sorts the ciphersuite list so all AES256 ciphers (including the CBC ones) would come before the AES128 ciphers. I do not think this is right for most use cases. Also the black list of the ciphers should apply mostly to the HTTP/2 servers - the clients are allowed to include them.

@t8m

t8m Aug 31, 2017

Using OPENSSL_VERSION_NUMBER >= 0x10100000L should be of course OK. However I do not see how using @strength would help to eliminate "illegal" ciphers. This just sorts the ciphersuite list so all AES256 ciphers (including the CBC ones) would come before the AES128 ciphers. I do not think this is right for most use cases. Also the black list of the ciphers should apply mostly to the HTTP/2 servers - the clients are allowed to include them.

This comment has been minimized.

@bagder

bagder Aug 31, 2017

Member

Still, @STRENGTH did help for HTTP/2 and that's why we have it there. Unfortunately I can't recall the picky servers I tried against back then so I can't really check right now what happens against them if/when we drop it.

@bagder

bagder Aug 31, 2017

Member

Still, @STRENGTH did help for HTTP/2 and that's why we have it there. Unfortunately I can't recall the picky servers I tried against back then so I can't really check right now what happens against them if/when we drop it.

This comment has been minimized.

@t8m

t8m Aug 31, 2017

Also you did not make the experiment with OpenSSL 1.1, did you?

@t8m

t8m Aug 31, 2017

Also you did not make the experiment with OpenSSL 1.1, did you?

This comment has been minimized.

@t8m

t8m Aug 31, 2017

This is the actual difference between curl(-) and upstream default(+) https://paste.fedoraproject.org/paste/XPMGLMTvd4d-BI9~CYU1tw

@t8m

t8m Aug 31, 2017

This is the actual difference between curl(-) and upstream default(+) https://paste.fedoraproject.org/paste/XPMGLMTvd4d-BI9~CYU1tw

This comment has been minimized.

@t8m

t8m Aug 31, 2017

This is of course with OpenSSL 1.1.0 - to me the upstream default looks like more sane.

@t8m

t8m Aug 31, 2017

This is of course with OpenSSL 1.1.0 - to me the upstream default looks like more sane.

Show outdated Hide outdated lib/vtls/openssl.c
@@ -2116,7 +2124,7 @@ static CURLcode ossl_connect_step1(struct connectdata *conn, int sockindex)
ciphers = SSL_CONN_CONFIG(cipher_list);
if(!ciphers)
ciphers = (char *)DEFAULT_CIPHER_SELECTION;
if(!SSL_CTX_set_cipher_list(BACKEND->ctx, ciphers)) {
if(ciphers && !SSL_CTX_set_cipher_list(BACKEND->ctx, ciphers)) {
failf(data, "failed setting cipher list: %s", ciphers);
return CURLE_SSL_CIPHER;
}

This comment has been minimized.

@jay

jay Aug 31, 2017

Member

The line below needs to be corrected infof(data, "Cipher selection: %s\n", ciphers); but now ciphers can be NULL . Github won't let me select the line because it's too far out of the common area. There may be a n easy way to get it in OpenSSL but I think it would be ok to just do ciphers ? ciphers : "<default>" or something like that

@jay

jay Aug 31, 2017

Member

The line below needs to be corrected infof(data, "Cipher selection: %s\n", ciphers); but now ciphers can be NULL . Github won't let me select the line because it's too far out of the common area. There may be a n easy way to get it in OpenSSL but I think it would be ok to just do ciphers ? ciphers : "<default>" or something like that

This comment has been minimized.

@bagder

bagder Aug 31, 2017

Member

The if(ciphers ... condition surely should make sure that ciphers can't be NULL there... ?

@bagder

bagder Aug 31, 2017

Member

The if(ciphers ... condition surely should make sure that ciphers can't be NULL there... ?

This comment has been minimized.

@jay

jay Aug 31, 2017

Member

yeah it's confusing, github won't let me select the line, it's the line 2131

@jay

jay Aug 31, 2017

Member

yeah it's confusing, github won't let me select the line, it's the line 2131

This comment has been minimized.

@kdudka

kdudka Aug 31, 2017

Collaborator

@jay Thanks for review! The infof() should be fixed now, will ask OpenSSL experts about a suitable version of OpenSSL for the condition.

Btw. you can refer to any location in any revision by the Github URL: https://github.com/kdudka/curl/blob/0b613865/lib/vtls/openssl.c#L2131

@kdudka

kdudka Aug 31, 2017

Collaborator

@jay Thanks for review! The infof() should be fixed now, will ask OpenSSL experts about a suitable version of OpenSSL for the condition.

Btw. you can refer to any location in any revision by the Github URL: https://github.com/kdudka/curl/blob/0b613865/lib/vtls/openssl.c#L2131

kdudka added a commit to kdudka/curl that referenced this pull request Aug 31, 2017

openssl: use OpenSSL's default ciphers by default
Up2date versions of OpenSSL maintain the default reasonably secure
without breaking compatibility, so it is better not to override the
default by curl.  Suggested at https://bugzilla.redhat.com/1483972

Closes #1846
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 31, 2017

Coverage Status

Coverage decreased (-0.009%) to 73.125% when pulling 5bcb700 on kdudka:openssl-default-ciphers into aa2ea66 on curl:master.

coveralls commented Aug 31, 2017

Coverage Status

Coverage decreased (-0.009%) to 73.125% when pulling 5bcb700 on kdudka:openssl-default-ciphers into aa2ea66 on curl:master.

openssl: use OpenSSL's default ciphers by default
Up2date versions of OpenSSL maintain the default reasonably secure
without breaking compatibility, so it is better not to override the
default by curl.  Suggested at https://bugzilla.redhat.com/1483972

Closes #1846
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 31, 2017

Coverage Status

Coverage decreased (-0.01%) to 73.104% when pulling 85583a3 on kdudka:openssl-default-ciphers into 222e65f on curl:master.

coveralls commented Aug 31, 2017

Coverage Status

Coverage decreased (-0.01%) to 73.104% when pulling 85583a3 on kdudka:openssl-default-ciphers into 222e65f on curl:master.

@kdudka kdudka closed this in ea142a8 Sep 5, 2017

@kdudka kdudka deleted the kdudka:openssl-default-ciphers branch Sep 5, 2017

@lock lock bot locked as resolved and limited conversation to collaborators May 14, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.