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

8286907: keytool should warn about weak PBE algorithms #12056

Closed
wants to merge 2 commits into from

Conversation

haimaychao
Copy link
Contributor

@haimaychao haimaychao commented Jan 17, 2023

Please review the fix to address the problem in keytool -genseckey and -importpass.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8286907: keytool should warn about weak PBE algorithms

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/12056/head:pull/12056
$ git checkout pull/12056

Update a local copy of the PR:
$ git checkout pull/12056
$ git pull https://git.openjdk.org/jdk pull/12056/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12056

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12056.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 17, 2023

👋 Welcome back hchao! 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 Jan 17, 2023
@openjdk
Copy link

openjdk bot commented Jan 17, 2023

@haimaychao 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 Jan 17, 2023
@mlbridge
Copy link

mlbridge bot commented Jan 17, 2023

Webrevs

@@ -1837,6 +1837,15 @@ private void doGenSecretKey(String alias, String keyAlgName,
useDefaultPBEAlgorithm = false;
}

String[] weakAlgs = new String[] {"DES", "DESEDE", "MD5", "SHA1", "RC2", "RC4"};
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of hardcoding the weak algorithms here, the security property jdk.security.legacyAlgorithms should probably be used. We can decompose the PBE algorithm name to parts and make the comparison. For example, "PBEWithSHA1AndDESede" should only match "DESede" but not "DES".

@openjdk
Copy link

openjdk bot commented Jan 25, 2023

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

8286907: keytool should warn about weak PBE algorithms

Reviewed-by: mullan, weijun

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

  • ae0e76d: 8301120: Cleanup utility classes java.util.Arrays and java.util.Collections
  • b8e5abc: 8301097: Update GHA XCode to 12.5.1
  • 9c4bc2c: 8301132: Test update for deprecated sprintf in Xcode 14
  • 7f05d57: 8217920: Lookup.defineClass injects a class that can access private members of any class in its own module
  • 22c976a: 8177418: NPE is not apparent for methods in java.util.TimeZone API docs
  • 7aaf76c: 8300924: Method::invoke throws wrong exception type when passing wrong number of arguments to method with 4 or more parameters
  • 49ff520: 8300241: Replace NULL with nullptr in share/classfile/
  • f52d35c: 8300240: Replace NULL with nullptr in share/ci/
  • 5c1ec82: 8301077: Replace NULL with nullptr in share/services/
  • dff4131: 8285850: [AIX] unreachable code in basic_tools.m4 -> BASIC_CHECK_TAR
  • ... and 306 more: https://git.openjdk.org/jdk/compare/c595f965abcf0ea80ace87b8f2180feebbb8984e...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 Jan 25, 2023
@wangweij
Copy link
Contributor

wangweij commented Jan 25, 2023

I tried using -keyalg PBEWithSHA1AndRC2_40 and removed SHA-1 from jdk.security.legacyAlgorithms and see no warning. It looks like the disabled algorithm decomposer cannot extract "RC2" from the algorithm name. One solution is to add both "RC2_40" and "RC2_128" to the security property.

Also, since this check only looks at jdk.security.legacyAlgorithms, shall we add other disabled algorithms here as well? Like MD2.

@haimaychao
Copy link
Contributor Author

Yes, the issue for PBEWithSHA1AndRC2_40 when SHA-1 is removed from jdk.security.legacyAlgorithms is because Algorithm Decomposer decomposes it to RC2_40 accordingly and Constraints map contains RC2 which is built based on jdk.security.legacyAlgorithms. Filed JDK-8301127 for this.
Filed JDK-8301130 to track adding MD2 to jdk.security.legacyAlgorithms.

@wangweij
Copy link
Contributor

wangweij commented Jan 26, 2023

I said "one solution is to add RC2_40 and RC2_128" but now I'm not sure if it's the right solution. We can always resolve this in a separate issue, but I think we'd better have an agreement on whether the current decomposer implementation is correct about "RC2" not covering "RC2_40". If yes and one day we decide to disable AES, then we should disable all of AES_128, AES_192 and AES_256 since there are algorithm names like AES_192/OFB/NoPadding and PBEWithHmacSHA384AndAES_128. This does not sound very right to me.

Valerie is adding PBES2Core$HmacSHA512_224AndAES_256 in another PR now. In that case, SHA512 should not cover HmacSHA512_224 (although we are not likely to disable HmacSHA512 before disabling HmacSHA512_224 first). So this is a little complicated.

@seanjmullan
Copy link
Member

seanjmullan commented Jan 26, 2023

Yeah, this is a little tricky. My feeling is that if you disable an algorithm like "RC2", it should cover all uses of it no matter what the keysize. If you only want to disable certain keysizes, then you should add a keySize constraint. We would need to make the parsing smarter so that "RC2 keySize <= 40" covers RC2_40 but not RC2_128, etc.

Hmac is another good corner case. It would be nice if we could have exceptions, like "SHA512", "!HmacSHA512". But that's a little more involved, and requires some more thought as to whether that is a good idea.

@haimaychao
Copy link
Contributor Author

@wangweij @seanjmullan For the scenario, i.e. PBEWithSHA1AndRC2_40 after SHA1 removal, we probably could
treat RC2_40 as RC2 after decomposing. For another scenario, adding RC2 KeySize < 40, we currently have a similar test case (i.e. AES keySize < 256) in WeakSecretKeyTest.java, and keytool will emit warning as a result of keysize constraint checking. The question arises is does it apply to PBEWithSHA1AndRC2_40 as well? I’d think it should if we treat RC2_40 as RC2 after decomposing. However, the PBEKey generated for PBEWithSHA1AndRC2_40 will have PBEwithMD5andDES algorithm. Algorithm constraint checking on MD5 would take place earlier than keysize constraint checking. As a result, warnings for keysize constraint will not be emitted. These are my current thoughts and more thoughts surely are needed to address JDK-8301127.
I suggest we look at the possible issues with various corner cases in algorithm decomposing and keysize constraints, etc, for PBExxx and Hmacxxx in JDK-8301127. Do you agree JDK-8301127 would serve the need?

@wangweij
Copy link
Contributor

SecretKeyFactory.getInstance("PBE") also generates PBEwithMD5andDES keys. I believe that's because PBE is an alias of PBEwithMD5andDES. Therefore it you use this factory to generate keys for other algorithms, do not rely on the algorithm.

Anyway, we can resolve the RC2_40 and AES_128 issues in another fix. The current code change looks fine for this bug.

@haimaychao
Copy link
Contributor Author

@seanjmullan @wangweij Thanks for the review.

@haimaychao
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Feb 2, 2023

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

  • ee0f5b5: 8301392: Port fdlibm log1p to Java
  • f696785: 8300869: Make use of the Double.toString(double) algorithm in java.util.Formatter
  • cf6b9eb: 8301637: ThreadLocalRandom.current().doubles().parallel() contention
  • c647ae6: 8301149: Parallel: Refactor MutableNUMASpace::update_layout
  • de57733: 8301644: com/sun/jdi/JdbStopThreadTest.java fails after JDK-8300811
  • 930ec00: 8301636: Minor cleanup in CommentHelper and DocPretty
  • 725d57b: 8301659: Resolve initialization reordering issues on Windows for libawt and libsaproc
  • 2d50c7d: 8298979: Remove duplicated serviceability/jvmti/thread/GetAllThreads/allthr01/allthr01.java
  • 59b7fb1: 8300273: [IR framework] Handle message instead of bailing out
  • 5b1584b: 8298880: VectorLogicalOpIdentityTest.java IR test incorrectly use avx3 instead of avx512
  • ... and 408 more: https://git.openjdk.org/jdk/compare/c595f965abcf0ea80ace87b8f2180feebbb8984e...master

Your commit was automatically rebased without conflicts.

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

openjdk bot commented Feb 2, 2023

@haimaychao Pushed as commit b00b70c.

💡 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
3 participants