8293779: redundant checking in AESCrypt.makeSessionKey() method #10263

XueleiFan · 2022-09-14T05:58:10Z

Hi,

Please review this simple code cleanup.

The following checking for key in the makeSessionKey() method is redundant as it the same checking has been performance before calling the method.

        if (k == null) {
            throw new InvalidKeyException("Empty key");
        }
        if (!isKeySizeValid(k.length)) {
             throw new InvalidKeyException("Invalid AES key length: " +
                                           k.length + " bytes");
        }

No new regression test, simple cleanup.

Thanks,
Xuelei

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-8293779: redundant checking in AESCrypt.makeSessionKey() method

Reviewers

Daniel Jeliński (@djelinski - Committer)
Hai-May Chao (@haimaychao - Committer) ⚠️ Review applies to d915ca1b
Sean Mullan (@seanjmullan - Reviewer) ⚠️ Review applies to d915ca1b

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10263

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

Using diff file

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

bridgekeeper · 2022-09-14T05:58:52Z

👋 Welcome back xuelei! 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 · 2022-09-14T06:02:38Z

@XueleiFan 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.

mlbridge · 2022-09-14T06:05:16Z

Webrevs

seanjmullan · 2022-09-14T13:25:41Z

src/java.base/share/classes/com/sun/crypto/provider/AESCrypt.java

@@ -602,13 +602,6 @@ private void implDecryptBlock(byte[] in, int inOffset,
     * @exception InvalidKeyException  If the key is invalid.
     */
    private void makeSessionKey(byte[] k) throws InvalidKeyException {


Can it still throw InvalidKeyException?

Good catch. The throws is no longer needed.

seanjmullan · 2022-09-14T17:36:50Z

src/java.base/share/classes/com/sun/crypto/provider/AESCrypt.java

-        if (!isKeySizeValid(k.length)) {
-             throw new InvalidKeyException("Invalid AES key length: " +
-                                           k.length + " bytes");
-        }
        int ROUNDS          = getRounds(k.length);


You could have left the check in here and removed the duplicate check from init(). Since the key is not referenced by init unless the key is different from the last key, it seems cleaner to leave it here, and I think the check would be invoked fewer times if the same key is reused.

Actually, NM, init still has to call MessageDigest.isEqual so eliminating keys of invalid length before that is probably more efficient.

Good point. Modified to use less checking.

If the key is null, the following condition could bypass the checking, and result in NPE.

if (!MessageDigest.isEqual(key, lastKey)) {

Although it is unlikely to happen as the caller should has already been checked that the key cannot be null, but the code logic here is not that clear to read. In the new patch, I have the null checking in the init() method, and the validity checking in the makeSessionKey() method.

Actually, NM, init still has to call MessageDigest.isEqual so eliminating keys of invalid length before that is probably more efficient.

The key should be valid for common cases. For valid key, it is more efficient to have the checking in makeSessionKey() as there is less checking. For invalid key, it is more efficient to have the checking before calling MessageDigest.isEqual(). Exception itself is costly, I would prefer to have better performance for common cases (valid key).

I updated the patch before I read the comment. Please let me know your preference. I'm fine to rollback if you prefer the old patch.

If the key is null, the following condition could bypass the checking, and result in NPE.

if (!MessageDigest.isEqual(key, lastKey)) {

Although it is unlikely to happen as the caller should has already been checked that the key cannot be null, but the code logic here is not that clear to read. In the new patch, I have the null checking in the init() method, and the validity checking in the makeSessionKey() method.

I agree, the provider layer should have rejected a null Key or a null Key.getEncoded prior to this, but best to not change what this code previously did (at least not for this change).

Actually, NM, init still has to call MessageDigest.isEqual so eliminating keys of invalid length before that is probably more efficient.

The key should be valid for common cases. For valid key, it is more efficient to have the checking in makeSessionKey() as there is less checking. For invalid key, it is more efficient to have the checking before calling MessageDigest.isEqual(). Exception itself is costly, I would prefer to have better performance for common cases (valid key).

I updated the patch before I read the comment. Please let me know your preference. I'm fine to rollback if you prefer the old patch.

Yes, I think your current fix should be fine too. No need to rollback.

Speaking of MessageDigest.isEqual, we don't need constant time comparison here. We could use Arrays.equals for some extra performance.

Actually, never mind that. We need constant time comparison to avoid leaking information about differences between old and new key. Sorry for the noise.

@djelinski If both styles (w/o constant-time operations) get used in the code, it may take time to analysis the potential secret leaking issues for code readers until there is a clear comment. As may add additional human and maintenance cost, which may be as expensive as the computer cost, especially when something goes wrong. So normally, I prefer to constant-time operations for secret informations, no matter if the operations expose to attacking surfaces or not. Just my $.02.

openjdk · 2022-09-14T17:50:33Z

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

8293779: redundant checking in AESCrypt.makeSessionKey() method

Reviewed-by: djelinski, hchao, 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 17 new commits pushed to the master branch:

eeb625e: 8290169: adlc: Improve child constraints for vector unary operations
2057070: 8293815: P11PSSSignature.engineUpdate should not print debug messages during normal operation
7376c55: 8293769: RISC-V: Add a second temporary register for BarrierSetAssembler::load_at
d191e47: 8293768: Add links to JLS 19 and 20 from SourceVersion enum constants
a75ddb8: 8293122: (fs) Use file cloning in macOS version of Files::copy method
95c7c55: 8293402: hs-err file printer should reattempt stack trace printing if it fails
211fab8: 8291669: [REDO] Fix array range check hoisting for some scaled loop iv
7f3250d: 8293787: Linux aarch64 build fails after 8292591
2a38791: 8292755: Non-default method in interface leads to a stack overflow in JShell
8351b30: 8293771: runtime/handshake/SystemMembarHandshakeTransitionTest.java fails if MEMBARRIER_CMD_QUERY is unsupported
... and 7 more: https://git.openjdk.org/jdk/compare/1dc5039fed9494f4d9b6c7002d28da9bc466fb10...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.

XueleiFan · 2022-09-15T15:03:59Z

/integrate

openjdk · 2022-09-15T15:06:36Z

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

6fca9ae: 8288474: Move EventContinuationFreezeOld from try_freeze_fast to freeze_slow
fbd8b42: 8293591: Remove use of Thread.stop from jshell tests
aff5ff1: 8244681: Add a warning for possibly lossy conversion in compound assignments
15cb1fb: 8256265: G1: Improve parallelism in regions that failed evacuation
b31a03c: 8293695: Implement isInfinite intrinsic for RISC-V
8f3bbe9: 8293472: Incorrect container resource limit detection if manual cgroup fs mounts present
1caba0f: 8292948: JEditorPane ignores font-size styles in external linked css-file
eeb625e: 8290169: adlc: Improve child constraints for vector unary operations
2057070: 8293815: P11PSSSignature.engineUpdate should not print debug messages during normal operation
7376c55: 8293769: RISC-V: Add a second temporary register for BarrierSetAssembler::load_at
... and 14 more: https://git.openjdk.org/jdk/compare/1dc5039fed9494f4d9b6c7002d28da9bc466fb10...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-09-15T15:06:53Z

@XueleiFan Pushed as commit ecb456a.

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

8293779: redundant checking in AESCrypt.makeSessionKey() method

fa427fd

openjdk bot added the rfr Pull request is ready for review label Sep 14, 2022

openjdk bot added the security security-dev@openjdk.org label Sep 14, 2022

djelinski approved these changes Sep 14, 2022

View reviewed changes

seanjmullan reviewed Sep 14, 2022

View reviewed changes

XueleiFan added 2 commits September 14, 2022 08:59

remove unused throws

e42b440

remove unused throws in comment

d915ca1

haimaychao approved these changes Sep 14, 2022

View reviewed changes

seanjmullan reviewed Sep 14, 2022

View reviewed changes

seanjmullan approved these changes Sep 14, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 14, 2022

check only if necessary

af61f2e

djelinski approved these changes Sep 15, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Sep 15, 2022

openjdk bot closed this Sep 15, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 15, 2022

XueleiFan deleted the JDK-8293779 branch September 15, 2022 15:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8293779: redundant checking in AESCrypt.makeSessionKey() method #10263

8293779: redundant checking in AESCrypt.makeSessionKey() method #10263

XueleiFan commented Sep 14, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 14, 2022

openjdk bot commented Sep 14, 2022

mlbridge bot commented Sep 14, 2022 •

edited

Loading

seanjmullan Sep 14, 2022

XueleiFan Sep 14, 2022

seanjmullan Sep 14, 2022

seanjmullan Sep 14, 2022

XueleiFan Sep 14, 2022

XueleiFan Sep 14, 2022

seanjmullan Sep 14, 2022

seanjmullan Sep 14, 2022

djelinski Sep 15, 2022

djelinski Sep 15, 2022

XueleiFan Sep 15, 2022

openjdk bot commented Sep 14, 2022 •

edited

Loading

XueleiFan commented Sep 15, 2022

openjdk bot commented Sep 15, 2022

openjdk bot commented Sep 15, 2022

8293779: redundant checking in AESCrypt.makeSessionKey() method #10263

8293779: redundant checking in AESCrypt.makeSessionKey() method #10263

Conversation

XueleiFan commented Sep 14, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Sep 14, 2022

openjdk bot commented Sep 14, 2022

mlbridge bot commented Sep 14, 2022 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Sep 14, 2022 • edited Loading

XueleiFan commented Sep 15, 2022

openjdk bot commented Sep 15, 2022

openjdk bot commented Sep 15, 2022

XueleiFan commented Sep 14, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 14, 2022 •

edited

Loading

openjdk bot commented Sep 14, 2022 •

edited

Loading