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

Clean up implementation for SSLOptions #5149

Closed
wants to merge 2 commits into from

Conversation

hardfalcon
Copy link
Contributor

This cleans up the implementation for the SSLOptions configuration directive:

  • Fixed the logic for DenyCBC on Linux and other gnutls platforms.
  • Fixed the logic for _httpTLSSetOptions() in cups/usersys.c to prevent unsolicited overwriting of the SSLOptions read from cupsd.conf.
  • The options / tls_options variables got changed into unsigned integers since we are using them as bit maps and not as "normal" numbers.
  • The _HTTP_TLS_* constants got shifted by one bit to accomodate the new constant _HTTP_TLS_UNCHANGED, which means that the values changed for all of these constants. This is needed so we can distinguish between "explicitly use the default settings" and "don't change/overwrite the existing configuration".
  • In some places, numerical values were replaced with the corresponding _HTTP_TLS_* constants.
  • Added explicit warnings about the AllowDH, AllowRC4 and AllowSSL3 parameters to the manual.

@michaelrsweet
Copy link
Collaborator

OK, so we can't take this pull request as-is, particularly the wording changes for the Allow* value documentation.

@hardfalcon
Copy link
Contributor Author

Any proposition for a better wording? Especially the AllowDH parameter is really dangerous, since it effectively enables MITM attacks without any computational cost. The ANON-DH ciphersuites it enables in gnutls correspond to the DH subset of the aNULL ciphersuites in OpenSSL - which means they do not offer any authentication whatsoever.

I'm still wondering how ANON-DH could make it into CUPS at all - even if there were printers which required such broken ciphersuites, it would still be better to access them via plain HTTP/IPP instead of globally forcing the whole CUPS server to offer these ciphersuites.

@hardfalcon
Copy link
Contributor Author

I've just cross-checked with cups/tls-darwin.c - on macOS, all anonymous ciphersuites are explicitly disabled with even a remark in the code that they should never be enabled because they're insecure.

It seems that gnutls simply doesn't support/implement static DH key exchanges at all, so it's simply impossible to implement the AllowDH parameter on gnutls platforms. I'd propose changing it into a no-op and updating the manual accordingly.

@michaelrsweet
Copy link
Collaborator

The wording we have is neutral about the security implications of the Allow* options because a) they are disabled by default for a reason, which a simple Google search will show, and b) typically if you have to use an Allow* option it is because you have no other choice. Calling things horrible or insecure is unnecessary.

As for the AllowDH option, if indeed GNU TLS no longer implements any DH cipher suites except for the NULL ones, then we should probably not support it there. Additionally I'll make sure we unconditionally exclude all NULL suites.

@hardfalcon
Copy link
Contributor Author

Wording: I'd prefer to explicitly warn users about insecure options, but it's your project, so your call, your rules. I've updated the PR accordingly.

AllowDH: I've turned AllowDH into a no-op on gnutls and updated the manual accordingly.

I've also added notes about the platforms on which the specific SSLOptions are supported.

@michaelrsweet
Copy link
Collaborator

[master 02c88e6] Fix cipher suite selection with GNU TLS (Issue #5145)

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.

2 participants