Copilot commented on code in PR #14:
URL:
https://github.com/apache/sling-org-apache-sling-commons-crypto/pull/14#discussion_r4017214624
##########
src/main/java/org/apache/sling/commons/crypto/jca/internal/JcaPbeCryptoService.java:
##########
@@ -122,31 +123,49 @@ private static void destroyKey(SecretKey key) {
}
}
+ private static void destroyData(byte[] data) {
+ if (data != null) {
+ Arrays.fill(data, (byte) 0x00);
+ }
+ }
+
+ private static void destroyData(char[] data) {
+ if (data != null) {
+ Arrays.fill(data, '\0');
+ }
+ }
+
private @NotNull SecretKey createKey(byte[] salt) throws
NoSuchAlgorithmException, InvalidKeySpecException {
final char[] password = passwordProvider.getPassword();
- // for regular PBE key this is completely ignored except for the
password (as all logic is encapsulated in the actual cipher
- // implementation, see
- //
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/com/sun/crypto/provider/PBEKeyFactory.java
- PBEKeySpec keySpec = new PBEKeySpec(
- password,
- salt,
- configuration.numKeyIterations(),
- configuration.keyLengthBits());
- SecretKeyFactory secretKeyFactory = securityProvider.isPresent()
- ?
SecretKeyFactory.getInstance(configuration.secretKeyFactoryAlgorithm(),
securityProvider.get())
- :
SecretKeyFactory.getInstance(configuration.secretKeyFactoryAlgorithm());
- SecretKey originalKey = secretKeyFactory.generateSecret(keySpec);
- keySpec.clearPassword(); // clear password from memory after use
- if
(configuration.secretKeyFactoryAlgorithm().equals(configuration.cipherAlgorithm()))
{
- // if the cipher algorithm is the same as the secret key factory
algorithm then the cipher takes care of the actual logic and
- // uses the key as is (which is just a wrapper around the given
password)
- return originalKey;
- } else {
- // wrap as key for the proper cipher algorithm (e.g., AES) instead
of the PBE algorithm (e.g., PBKDF2WithHmacSHA512)
- SecretKey derivedKey = new SecretKeySpec(originalKey.getEncoded(),
extractAlgorithmName(configuration.cipherAlgorithm()));
- destroyKey(originalKey); // destroy the original key as it is no
longer needed
- return derivedKey;
- }
+ try {
+ // for regular PBE key this is completely ignored except for the
password (as all logic is encapsulated in the actual cipher
+ // implementation, see
+ //
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/com/sun/crypto/provider/PBEKeyFactory.java
+ PBEKeySpec keySpec = new PBEKeySpec(
+ password,
+ salt,
+ configuration.numKeyIterations(),
+ configuration.keyLengthBits());
+ SecretKeyFactory secretKeyFactory = securityProvider.isPresent()
+ ?
SecretKeyFactory.getInstance(configuration.secretKeyFactoryAlgorithm(),
securityProvider.get())
+ :
SecretKeyFactory.getInstance(configuration.secretKeyFactoryAlgorithm());
+ SecretKey originalKey = secretKeyFactory.generateSecret(keySpec);
+ keySpec.clearPassword(); // clear password from memory after use
+ if
(configuration.secretKeyFactoryAlgorithm().equals(configuration.cipherAlgorithm()))
{
+ // if the cipher algorithm is the same as the secret key
factory algorithm then the cipher takes care of the actual logic and
+ // uses the key as is (which is just a wrapper around the
given password)
+ return originalKey;
+ } else {
+ // wrap as key for the proper cipher algorithm (e.g., AES)
instead of the PBE algorithm (e.g., PBKDF2WithHmacSHA512)
+ byte[] keyBytes = originalKey.getEncoded();
+ SecretKey derivedKey = new SecretKeySpec(keyBytes,
extractAlgorithmName(configuration.cipherAlgorithm()));
+ destroyData(keyBytes); // clear key bytes from memory after use
+ destroyKey(originalKey); // destroy the original key as it is
no longer needed
+ return derivedKey;
+ }
+ } finally {
Review Comment:
`keySpec.clearPassword()` is reached only after both `getInstance(...)` and
`generateSecret(...)` succeed. If either call throws, the outer `finally` wipes
only the provider's array; the password copy retained inside `PBEKeySpec`
remains in memory. Put the key-spec operations in a nested `try/finally` and
clear the spec there.
##########
src/main/java/org/apache/sling/commons/crypto/internal/PbeSecretKeyProvider.java:
##########
@@ -100,11 +99,13 @@ protected void deactivate() {
public @NotNull SecretKey getSecretKey() {
final var configuration = this.configuration;
Objects.requireNonNull(configuration, "Configuration must not be
null");
+ final PBEKeySpec keySpec = new
PBEKeySpec(passwordProvider.getPassword(), saltProvider.getSalt(),
configuration.iterationCount(), configuration.keyLength());
Review Comment:
The password array returned by `passwordProvider.getPassword()` is passed
inline here and is never cleared; `keySpec.clearPassword()` only wipes
PBEKeySpec's internal copy. This leaves the provider-owned buffer intact after
both successful and failed key generation, contrary to the new
`PasswordProvider` contract. Retain the array in a local and wipe it in an
outer `finally`, while keeping the key-spec cleanup nested inside.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
