Copilot commented on code in PR #14:
URL:
https://github.com/apache/sling-org-apache-sling-commons-crypto/pull/14#discussion_r4017730897
##########
src/main/java/org/apache/sling/commons/crypto/internal/PbeSecretKeyProvider.java:
##########
@@ -100,11 +100,15 @@ protected void deactivate() {
public @NotNull SecretKey getSecretKey() {
final var configuration = this.configuration;
Objects.requireNonNull(configuration, "Configuration must not be
null");
+ char[] password = passwordProvider.getPassword();
+ final PBEKeySpec keySpec = new PBEKeySpec(password,
saltProvider.getSalt(), configuration.iterationCount(),
configuration.keyLength());
Review Comment:
The `PBEKeySpec` constructor runs before this `try`, so
salt/iteration/key-length validation failures leave the newly returned password
uncleared. This is reachable in the existing `testInvalidKeySpec` path; move
spec construction under cleanup and make `keySpec.clearPassword()` null-safe
when construction fails.
##########
src/main/java/org/apache/sling/commons/crypto/jca/internal/JcaPbeCryptoService.java:
##########
@@ -132,21 +145,28 @@ private static void destroyKey(SecretKey key) {
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 {
Review Comment:
This `try` starts only after `PBEKeySpec` construction, so invalid salt or
configuration values can bypass the new `finally` and leave `password`
populated. Move construction inside the protected region (and guard the clear
when construction fails) so every failure path honors the documented cleanup
contract.
This issue also appears on line 160 of the same file.
--
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]
