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

8163327: Remove 3DES from the default enabled cipher suites list #7894

Closed
wants to merge 2 commits into from

Conversation

seanjmullan
Copy link
Member

@seanjmullan seanjmullan commented Mar 21, 2022

This fix removes obsolete and deprecated 3DES cipher suites from the default enabled cipher suites list of the SunJSSE provider implementation.

Note that 3DES suites are already disabled by default via the jdk.tls.disabledAlgorithms security property. This change goes one step further and provides an extra level of defense by making them unavailable by default. See the CSR for more details: https://bugs.openjdk.java.net/browse/JDK-8283450


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed
  • Change requires a CSR request to be approved

Issues

  • JDK-8163327: Remove 3DES from the default enabled cipher suites list
  • JDK-8283450: Remove 3DES from the default enabled cipher suites list (CSR)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7894/head:pull/7894
$ git checkout pull/7894

Update a local copy of the PR:
$ git checkout pull/7894
$ git pull https://git.openjdk.java.net/jdk pull/7894/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7894

View PR using the GUI difftool:
$ git pr show -t 7894

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7894.diff

"SSL_RSA_WITH_DES_CBC_SHA",
"SSL_DHE_RSA_WITH_DES_CBC_SHA",
"SSL_DHE_DSS_WITH_DES_CBC_SHA",
"SSL_RSA_EXPORT_WITH_DES40_CBC_SHA",
"SSL_DHE_RSA_EXPORT_WITH_DES40_CBC_SHA",
"SSL_DHE_DSS_EXPORT_WITH_DES40_CBC_SHA",
"SSL_RSA_EXPORT_WITH_RC4_40_MD5",
Copy link
Member Author

@seanjmullan seanjmullan Mar 21, 2022

Choose a reason for hiding this comment

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

Also added additional suites that are already disabled and had not been added to this test.

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 21, 2022

👋 Welcome back mullan! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@seanjmullan
Copy link
Member Author

seanjmullan commented Mar 21, 2022

/csr

@openjdk openjdk bot added rfr Pull request is ready for review csr Pull request needs approved CSR before integration labels Mar 21, 2022
@openjdk
Copy link

openjdk bot commented Mar 21, 2022

@seanjmullan this pull request will not be integrated until the CSR request JDK-8283450 for issue JDK-8163327 has been approved.

@openjdk
Copy link

openjdk bot commented Mar 21, 2022

@seanjmullan The following label will be automatically applied to this pull request:

  • security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the security security-dev@openjdk.org label Mar 21, 2022
@mlbridge
Copy link

mlbridge bot commented Mar 21, 2022

Webrevs

// 3DES_EDE, forward secrecy.
TLS_ECDHE_ECDSA_WITH_3DES_EDE_CBC_SHA(
0xC008, false, "TLS_ECDHE_ECDSA_WITH_3DES_EDE_CBC_SHA", "",
ProtocolVersion.PROTOCOLS_TO_12,
K_ECDHE_ECDSA, B_3DES, M_SHA, H_SHA256),
TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA(
0xC012, false, "TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA", "",
ProtocolVersion.PROTOCOLS_TO_12,
K_ECDHE_RSA, B_3DES, M_SHA, H_SHA256),
SSL_DHE_RSA_WITH_3DES_EDE_CBC_SHA(
0x0016, false, "SSL_DHE_RSA_WITH_3DES_EDE_CBC_SHA",
"TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA",
ProtocolVersion.PROTOCOLS_TO_12,
K_DHE_RSA, B_3DES, M_SHA, H_SHA256),
SSL_DHE_DSS_WITH_3DES_EDE_CBC_SHA(
0x0013, false, "SSL_DHE_DSS_WITH_3DES_EDE_CBC_SHA",
"TLS_DHE_DSS_WITH_3DES_EDE_CBC_SHA",
ProtocolVersion.PROTOCOLS_TO_12,
K_DHE_DSS, B_3DES, M_SHA, H_SHA256),

// 3DES_EDE, not forward secrecy.
TLS_ECDH_ECDSA_WITH_3DES_EDE_CBC_SHA(
0xC003, false, "TLS_ECDH_ECDSA_WITH_3DES_EDE_CBC_SHA", "",
ProtocolVersion.PROTOCOLS_TO_12,
K_ECDH_ECDSA, B_3DES, M_SHA, H_SHA256),
TLS_ECDH_RSA_WITH_3DES_EDE_CBC_SHA(
0xC00D, false, "TLS_ECDH_RSA_WITH_3DES_EDE_CBC_SHA", "",
ProtocolVersion.PROTOCOLS_TO_12,
K_ECDH_RSA, B_3DES, M_SHA, H_SHA256),
SSL_RSA_WITH_3DES_EDE_CBC_SHA(
0x000A, false, "SSL_RSA_WITH_3DES_EDE_CBC_SHA",
"TLS_RSA_WITH_3DES_EDE_CBC_SHA",
ProtocolVersion.PROTOCOLS_TO_12,
K_RSA, B_3DES, M_SHA, H_SHA256),

Copy link
Member

@XueleiFan XueleiFan Mar 22, 2022

Choose a reason for hiding this comment

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

It is good to have the supported cipher suites ordered. So it may be nice to have this block between line 348 and 349.

Copy link
Member Author

@seanjmullan seanjmullan Mar 22, 2022

Choose a reason for hiding this comment

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

Can you be more specific? I'm not following where you think they should be ordered. Are you suggesting they should be ordered before the anon suites even though most of them use stronger algorithms? Also, does the order matter if the application is going to be setting them via APIs? For example, if an application calls SSLSocket.setEnabledCipherSuites(new String[] { "TLS_ECDH_RSA_WITH_3DES_EDE_CBC_SHA", "TLS_ECDH_ECDSA_WITH_3DES_EDE_CBC_SHA" }) is the order specified respected? Or does the provider re-order it according to this file?

Copy link
Member

@XueleiFan XueleiFan Mar 22, 2022

Choose a reason for hiding this comment

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

In some situation, applications may set the supported cipher suites as the enabled cipher suites. Therefore, the supported cipher suites are also ordered in the current implementation, even though not strictly. Although 3DES suites are pretty weak, but they might be better than anon suites in practice, I think.

Copy link
Member Author

@seanjmullan seanjmullan Mar 22, 2022

Choose a reason for hiding this comment

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

I see. That makes sense. However, I will note that the current order does not reflect the preference rules defined in lines 336-348 (copied below):

    // Definition of the CipherSuites that are supported but not enabled
    // by default.
    // They are listed in preference order, preferred first, using the
    // following criteria:
    // 1. If a cipher suite has been obsoleted, we put it at the end of
    //    the list.
    // 2. Prefer the stronger bulk cipher, in the order of AES_256,
    //    AES_128, 3DES-EDE, RC-4, DES, DES40, RC4_40, NULL.
    // 3. Prefer the stronger MAC algorithm, in the order of SHA384,
    //    SHA256, SHA, MD5.
    // 4. Prefer the better performance of key exchange and digital
    //    signature algorithm, in the order of ECDHE-ECDSA, ECDHE-RSA,
    //    RSA, ECDH-ECDSA, ECDH-RSA, DHE-RSA, DHE-DSS, anonymous.

In particular, some of the RC4 suites support forward secrecy but are ordered after the anon suites. The rules above say nothing about anon suites. They seem to be ordered as higher priority than others mainly because they use stronger ciphers. But 3DES and RC4 are obsolete suites, so maybe it is correct that they should be where they are.

Thus, it isn't clear to me if the current order does or does not accurately reflect these rules. I put the 3DES suites where they were because to me they are probably/maybe stronger than the RC4 suites, but do not use strong ciphers that the anon suites do, and are obsolete. So we either need to clarify the rules and put anon suites lower, and change the obsolete rule. Otherwise, I'm reluctant to make a change that seems inconsistent with the rules.

Having said all that, I also don't want to over-rotate on this and try to decide what suite is more weaker than others, since you really shouldn't be using any of them.

Copy link
Member

@XueleiFan XueleiFan Mar 22, 2022

Choose a reason for hiding this comment

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

So we either need to clarify the rules and put anon suites lower, and change the obsolete rule.

It makes sense to me. It might be better to update the rules firstly (or just leave it alone as the priority is pretty low), but which is out of the scope of this update. I will approve this update.

Copy link
Member Author

@seanjmullan seanjmullan Mar 22, 2022

Choose a reason for hiding this comment

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

Ok, let me think about whether this is worth improving as a separate issue. I did make one change to the order however, and that is to move the TLS_ECDH_anon_WITH_3DES_EDE_CBC_SHA and SSL_DH_anon_WITH_3DES_EDE_CBC_SHA suites lower in priority after other 3DES suites as these clearly should be lower in priority because they are using 3DES and anonymous authn.

…E_CBC_SHA

lower in priority after other 3DES suites.
Copy link
Member

@XueleiFan XueleiFan left a comment

Looks good to me.

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Mar 23, 2022
@openjdk
Copy link

openjdk bot commented Mar 23, 2022

@seanjmullan This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8163327: Remove 3DES from the default enabled cipher suites list

Reviewed-by: xuelei

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 62 new commits pushed to the master branch:

  • 6ea996c: 8282422: JTable.print() failed with UnsupportedCharsetException on AIX ko_KR locale
  • 8cc1235: 8282952: Thread::exit should be immune to Thread.stop
  • 33eb89d: 8283457: [macos] libpng build failures with Xcode13.3
  • f7d21c3: 8283480: Make AbstractStringBuilder sealed
  • d29c7e7: 8282590: C2: assert(addp->is_AddP() && addp->outcnt() > 0) failed: Don't process dead nodes
  • 557ff4b: 8282625: Formatter caches Locale/DecimalFormatSymbols poorly
  • fabde3b: 8283451: C2: assert(_base == Long) failed: Not a Long
  • c0f984e: 8283456: Make CompiledICHolder::live_count/live_not_claimed_count debug only
  • 85628a8: 8282592: C2: assert(false) failed: graph should be schedulable
  • a6fd0b2: 8283087: Create a test or JDK-4715503
  • ... and 52 more: https://git.openjdk.java.net/jdk/compare/08cadb4754da0d5e68ee2df45f4098d203d14115...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Mar 23, 2022
@seanjmullan
Copy link
Member Author

seanjmullan commented Mar 23, 2022

/integrate

@openjdk
Copy link

openjdk bot commented Mar 23, 2022

Going to push as commit 138460c.
Since your change was applied there have been 78 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Mar 23, 2022
@openjdk openjdk bot closed this Mar 23, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 23, 2022
@openjdk
Copy link

openjdk bot commented Mar 23, 2022

@seanjmullan Pushed as commit 138460c.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated security security-dev@openjdk.org
2 participants