Try to remove sensitive parts from memory more aggressively

[kwin]

Call SecretKey.destroy() and PBEKeySpec.clearPassword() to overwrite memory locations with 0 instead of just waiting for GC. Unfortunately right now not well supported by Java (https://bugs.openjdk.org/browse/JDK-8389121).

[Copilot]

Pull request overview

This PR aims to more aggressively reduce the lifetime of sensitive key material in memory during AES-GCM encryption/decryption by explicitly clearing password-derived key data instead of relying solely on garbage collection.

Changes:

• Clears PBEKeySpec password material via clearPassword() after key derivation.
• Attempts to destroy derived SecretKey instances via a new destroyQuietly(...) helper.
• Destroys encryption/decryption keys after doFinal(...).

Comments suppressed due to low confidence (2)

src/main/java/org/codehaus/plexus/components/secdispatcher/internal/cipher/AESGCMNoPadding.java:91

• secretKey is only destroyed on the happy path. If decoding, key derivation, init(...), or doFinal(...) throws, the key won't be destroyed. Wrap the method body so destroyQuietly(secretKey) always runs in finally.

            SecretKey secretKey = getAESKeyFromPassword(password.toCharArray(), salt);
            Cipher cipher = Cipher.getInstance(CIPHER_ALG);
            cipher.init(Cipher.DECRYPT_MODE, secretKey, new GCMParameterSpec(TAG_LENGTH_BIT, iv));
            byte[] plainText = cipher.doFinal(cipherText);
            destroyQuietly(secretKey);
            return new String(plainText, StandardCharsets.UTF_8);

src/main/java/org/codehaus/plexus/components/secdispatcher/internal/cipher/AESGCMNoPadding.java:110

• spec.clearPassword() and destroyQuietly(originalKey) should run even if generateSecret(...) or getEncoded() throws. Using try/finally here ensures password/key material is cleared on both success and failure paths.

        PBEKeySpec spec = new PBEKeySpec(password, salt, PBE_ITERATIONS, PBE_KEY_SIZE);
        SecretKey originalKey = factory.generateSecret(spec);
        spec.clearPassword();
        SecretKey derivedKey = new SecretKeySpec(originalKey.getEncoded(), KEY_ALGORITHM);
        destroyQuietly(originalKey);

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

🟡 Changes recommended

Cleanup is not guaranteed on several failure paths, and one sensitive key copy remains uncleared.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

src/main/java/org/codehaus/plexus/components/secdispatcher/internal/cipher/AESGCMNoPadding.java:63

• The encrypt path destroys secretKey only after doFinal() succeeds. If cipher creation, initialization, or encryption throws, control jumps to the outer catch and this derived key is never destroyed, so cleanup is skipped on failure paths. Wrap the cipher operations in a try/finally, as the decrypt path does.

            destroyQuietly(secretKey);

src/main/java/org/codehaus/plexus/components/secdispatcher/internal/cipher/AESGCMNoPadding.java:113

• originalKey.getEncoded() creates another byte array containing the derived key, and SecretKeySpec copies it. Destroying originalKey does not clear that temporary array, so this sensitive copy remains until garbage collection; zero the encoded array in a finally after constructing the returned key.

            return new SecretKeySpec(originalKey.getEncoded(), KEY_ALGORITHM);

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Moderate cleanup and compilation issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

src/main/java/org/codehaus/plexus/components/secdispatcher/internal/cipher/AESGCMNoPadding.java:64

• The key is destroyed only after doFinal() succeeds. If cipher creation, initialization, or encryption fails, this method exits through the catch block while the derived secretKey is still live, which defeats the new sensitive-memory cleanup on an error path. Put the cipher setup and doFinal() in a try/finally, as is already done in decrypt.

            destroyQuietly(secretKey);

src/main/java/org/codehaus/plexus/components/secdispatcher/internal/cipher/AESGCMNoPadding.java:133

• SecretKey does not extend Destroyable, so this call is not declared on the static type and the class will not compile. Invoke destroy() only after checking/casting to javax.security.auth.Destroyable; non-destroyable implementations should remain a no-op as the comment describes.

            key.destroy();

src/main/java/org/codehaus/plexus/components/secdispatcher/internal/cipher/AESGCMNoPadding.java:115

• SecretKeySpec makes its own copy of encodedKey and is not a Destroyable. After encodedKey is zeroed, the actual AES key retained by the returned SecretKeySpec is still present, and destroyQuietly will be a no-op for it, so the active key is not removed from memory as intended. Use a key implementation with a destroyable backing store if this guarantee is required.

                return new SecretKeySpec(encodedKey, KEY_ALGORITHM);

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Cleanup gaps can leave sensitive key and password material uncleared when operations fail.

Review details

Suppressed comments (2)

src/main/java/org/codehaus/plexus/components/secdispatcher/internal/cipher/AESGCMNoPadding.java:64

• The derived key is destroyed only on the successful path. If Cipher.getInstance, init, doFinal, or the output encoding throws, control jumps to the catch block and this key remains live, which defeats the cleanup goal for failed encryptions. Put the whole operation after key derivation in a try/finally, as is already done in decrypt, so cleanup also runs on exceptions.

            SecretKey secretKey = getAESKeyFromPassword(password.toCharArray(), salt);
            Cipher cipher = Cipher.getInstance(CIPHER_ALG);
            cipher.init(Cipher.ENCRYPT_MODE, secretKey, new GCMParameterSpec(TAG_LENGTH_BIT, iv));
            byte[] cipherText = cipher.doFinal(clearText.getBytes(StandardCharsets.UTF_8));
            destroyQuietly(secretKey);

src/main/java/org/codehaus/plexus/components/secdispatcher/internal/cipher/AESGCMNoPadding.java:110

• The cleanup scope starts only after SecretKeyFactory.getInstance and new PBEKeySpec have completed. If provider lookup or PBEKeySpec construction fails, the caller-created password array is never filled and remains until GC, which leaves a gap in the sensitive-memory cleanup this change introduces. Move those operations inside a scope that always fills password, clearing spec only when construction succeeds.

        PBEKeySpec spec = new PBEKeySpec(password, salt, PBE_ITERATIONS, PBE_KEY_SIZE);

• Files reviewed: 1/1 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Fix the key destruction call so the code compiles and handles unsupported destruction safely.

Review details

Suppressed comments (1)

src/main/java/org/codehaus/plexus/components/secdispatcher/internal/cipher/AESGCMNoPadding.java:138

• This does not compile: javax.crypto.SecretKey extends java.security.Key, not javax.security.auth.Destroyable, so destroy() is not available on the declared type. Check whether the key implements Destroyable and invoke it through that interface (otherwise treat destruction as unsupported).

    private static void destroyQuietly(SecretKey key) {
        try {
            key.destroy();

• Files reviewed: 1/1 changed files
• Comments generated: 0 new
• Review effort level: Lite