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

8273670: Remove weak etypes from default krb5 etype list #5654

Closed
wants to merge 5 commits into from

Conversation

wangweij
Copy link
Contributor

@wangweij wangweij commented Sep 23, 2021

This code change removes weak etypes from the default list so it's safer to enable one of them. See the corresponding CSR at https://bugs.openjdk.java.net/browse/JDK-8274207 for more explanation. BTW, please review the CSR as well.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8273670: Remove weak etypes from default krb5 etype list

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5654

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

Using diff file

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

8273670: Remove weak etypes from default krb5 etype list
@bridgekeeper
Copy link

bridgekeeper bot commented Sep 23, 2021

👋 Welcome back weijun! 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.

@openjdk openjdk bot added the rfr Pull request is ready for review label Sep 23, 2021
@openjdk
Copy link

openjdk bot commented Sep 23, 2021

@wangweij 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 Sep 23, 2021
@wangweij
Copy link
Contributor Author

wangweij commented Sep 23, 2021

/csr

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label Sep 23, 2021
@openjdk
Copy link

openjdk bot commented Sep 23, 2021

@wangweij this pull request will not be integrated until the CSR request JDK-8274207 for issue JDK-8273670 has been approved.

@mlbridge
Copy link

mlbridge bot commented Sep 23, 2021

Webrevs

@valeriepeng
Copy link

valeriepeng commented Sep 24, 2021

I will take a look, thanks~

allowWeakCrypto = allowed;
if (allowWeakCrypto) {
result[num++] = EncryptedData.ETYPE_DES3_CBC_HMAC_SHA1_KD;
result[num++] = EncryptedData.ETYPE_ARCFOUR_HMAC;
Copy link
Member

@seanjmullan seanjmullan Sep 24, 2021

Choose a reason for hiding this comment

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

MIT still has arcfour-hmac-md5 in the enabled list - do you think there is any reason (compatibility) we should do the same? Note that I think it is better that we don't though. See "permitted_enctypes" at https://web.mit.edu/Kerberos/krb5-1.19/doc/admin/conf_files/krb5_conf.html#libdefaults.

Copy link
Contributor Author

@wangweij wangweij Sep 24, 2021

Choose a reason for hiding this comment

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

This is because MIT krb5 treats DES as weak and RC4 as deprecated. In Java, we treat both as weak after JDK-8139348 (the title is "Deprecate 3DES and RC4 in Kerberos" but this "deprecate" is not the same as the one in MIT krb5). This means when "allow_weak_crypto = true" we've already removed RC4. Since this code change is about removing weak etypes from the default "permitted_enctypes", we should be consistent.

Copy link

@valeriepeng valeriepeng Sep 24, 2021

Choose a reason for hiding this comment

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

Perhaps you meant "false" in the sentence below?

when "allow_weak_crypto = true" we've already removed RC4.

Copy link
Contributor Author

@wangweij wangweij Sep 25, 2021

Choose a reason for hiding this comment

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

Yes. Typo.

return Arrays.copyOfRange(result, 0, result.length - 4);
}
return result;
return defaultETypes;
Copy link
Member

@seanjmullan seanjmullan Sep 24, 2021

Choose a reason for hiding this comment

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

It might be safer to return a clone here since it is mutable. The previous code always returned a new array. This array gets passed back to calling code via Etype.getDefaults(), returning a clone would prevent the configured value from being accidentally modified.

Copy link
Contributor Author

@wangweij wangweij Sep 24, 2021

Choose a reason for hiding this comment

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

OK.

if (maxKeyLength >= 256) {
result[num++] = EncryptedData.ETYPE_AES256_CTS_HMAC_SHA1_96;
}
result[num++] = EncryptedData.ETYPE_AES128_CTS_HMAC_SHA1_96;
if (maxKeyLength >= 256) {
result[num++] = EncryptedData.ETYPE_AES256_CTS_HMAC_SHA384_192;
}
result[num++] = EncryptedData.ETYPE_AES128_CTS_HMAC_SHA256_128;

// By default, only AES etypes are enabled
defaultETypes = Arrays.copyOf(result, num);
Copy link

@valeriepeng valeriepeng Sep 24, 2021

Choose a reason for hiding this comment

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

nit: why not just do:

        defaultETypes = (maxKeyLength >= 256?
                new int[] {
                    EncryptedData.ETYPE_AES256_CTS_HMAC_SHA1_96,
                    EncryptedData.ETYPE_AES128_CTS_HMAC_SHA1_96,
                    EncryptedData.ETYPE_AES256_CTS_HMAC_SHA384_192,
                    EncryptedData.ETYPE_AES128_CTS_HMAC_SHA256_128,
                } : new int[] {
                    EncryptedData.ETYPE_AES128_CTS_HMAC_SHA1_96,
                    EncryptedData.ETYPE_AES128_CTS_HMAC_SHA256_128,
                });

Copy link
Contributor Author

@wangweij wangweij Sep 25, 2021

Choose a reason for hiding this comment

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

OK, I can code this way.

try {
Config cfg = Config.getInstance();
allowed = cfg.getBooleanObject("libdefaults", "allow_weak_crypto")
allowWeakCrypto = cfg.getBooleanObject("libdefaults", "allow_weak_crypto")

Choose a reason for hiding this comment

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

nit: exceeds 80 chars?

Copy link
Contributor Author

@wangweij wangweij Sep 25, 2021

Choose a reason for hiding this comment

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

A little. I've learnt to accept lines a little bit longer than 80.

if (allowWeakCrypto) {
result[num++] = EncryptedData.ETYPE_DES3_CBC_HMAC_SHA1_KD;
result[num++] = EncryptedData.ETYPE_ARCFOUR_HMAC;
result[num++] = EncryptedData.ETYPE_DES_CBC_CRC;
result[num++] = EncryptedData.ETYPE_DES_CBC_MD5;
}

// Weak crypto are also supported and can be enabled manually
if (num == result.length) {
supportedETypes = result;
} else {
supportedETypes = Arrays.copyOf(result, num);
}
Copy link

@valeriepeng valeriepeng Sep 24, 2021

Choose a reason for hiding this comment

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

nit: it seems clearer to base supportedETypes on defaultETypes, e.g.

        if (allowWeakCrypto) {
            supportedETypes = Arrays.copyOf(defaultETypes,
                defaultETypes.length + 4);
            supportedETypes[defaultETypes.length] =
                    EncryptedData.ETYPE_DES3_CBC_HMAC_SHA1_KD;
            supportedETypes[defaultETypes.length + 1] =
                    EncryptedData.ETYPE_ARCFOUR_HMAC;
            supportedETypes[defaultETypes.length + 2] =
                    EncryptedData.ETYPE_DES_CBC_CRC;
            supportedETypes[defaultETypes.length + 3] =
                    EncryptedData.ETYPE_DES_CBC_MD5;
        } else {
            supportedETypes = defaultETypes;
        }

Copy link
Contributor Author

@wangweij wangweij Sep 25, 2021

Choose a reason for hiding this comment

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

Yes.

(args.length > 0 ? ("allow_weak_crypto = " + args[0]) : "");
Files.write(Paths.get("krb5.conf"), conf.getBytes());
System.setProperty("java.security.krb5.conf", "krb5.conf");
test(null, "aes256-cts aes128-cts aes256-sha2 aes128-sha2 des3-hmac-sha1 arcfour-hmac des-cbc-crc des-cbc-md5",

Choose a reason for hiding this comment

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

nit: save this into a local String variable, e.g. strongAndWeak or allETypes, and reuse this.

@valeriepeng
Copy link

valeriepeng commented Sep 24, 2021

I've reviewed the CSR and made some edit changes. Thanks.

Copy link

@valeriepeng valeriepeng left a comment

Updates look good. Thanks.

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Sep 27, 2021
@openjdk
Copy link

openjdk bot commented Sep 27, 2021

@wangweij 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:

8273670: Remove weak etypes from default krb5 etype list

Reviewed-by: valeriep, mullan

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 190 new commits pushed to the master branch:

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 Sep 27, 2021
// common default etypes if not defined in krb5.conf
private static int[] defaultETypes;
// allow_weak_crypto in krb5.conf
public static boolean allowWeakCrypto;
Copy link
Member

@seanjmullan seanjmullan Sep 28, 2021

Choose a reason for hiding this comment

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

Can you make this package-private instead? I think it is only accessed by sun.security.krb5.internal.crypto.Cksum.

Copy link
Contributor Author

@wangweij wangweij Sep 28, 2021

Choose a reason for hiding this comment

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

OK.

"sun.security.krb5.internal.crypto.HmacSha2Aes256CksumType";
break;
}
if (cksumType == null) {
throw new KdcErrException(Krb5.KDC_ERR_SUMTYPE_NOSUPP);
Copy link
Member

@seanjmullan seanjmullan Sep 28, 2021

Choose a reason for hiding this comment

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

Could we add the checksum type that is disabled/not supported to this exception message?

@wangweij
Copy link
Contributor Author

wangweij commented Oct 1, 2021

Backout checksum related changes. Will deal with it in another issue.

@wangweij
Copy link
Contributor Author

wangweij commented Oct 5, 2021

/integrate

@openjdk
Copy link

openjdk bot commented Oct 5, 2021

Going to push as commit 03d3c03.
Since your change was applied there have been 192 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Oct 5, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 5, 2021
@openjdk
Copy link

openjdk bot commented Oct 5, 2021

@wangweij Pushed as commit 03d3c03.

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

@wangweij wangweij deleted the 8273670 branch Oct 5, 2021
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
3 participants