diff --git a/config/alfresco/authentication-services-context.xml b/config/alfresco/authentication-services-context.xml index 94a63f0682..5af7ed20c3 100644 --- a/config/alfresco/authentication-services-context.xml +++ b/config/alfresco/authentication-services-context.xml @@ -169,11 +169,6 @@ - diff --git a/source/java/org/alfresco/repo/security/authentication/CompositePasswordEncoder.java b/source/java/org/alfresco/repo/security/authentication/CompositePasswordEncoder.java index 75158c192b..330f219cd6 100644 --- a/source/java/org/alfresco/repo/security/authentication/CompositePasswordEncoder.java +++ b/source/java/org/alfresco/repo/security/authentication/CompositePasswordEncoder.java @@ -26,6 +26,7 @@ import org.alfresco.util.PropertyCheck; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import java.util.ArrayList; import java.util.Arrays; import java.util.List; import java.util.Map; @@ -75,6 +76,48 @@ public class CompositePasswordEncoder return false; } + /** + * Determines if its safe to encode the encoding chain. This applies particularly to double-hashing. + * BCRYPT uses its own internal salt so its NOT safe to use it more than once in an encoding chain + * (because the result will be different each time.) BCRYPT CAN BE USED successfully as the last element in + * an encoding chain. + * + * Anything that implements springframework PasswordEncoder is considered "unsafe" + * (because the method takes no salt param). + * + * @param encodingChain mandatory encoding chain + * @return true if it is okay to encode this chain. + */ + public boolean isSafeToEncodeChain(List encodingChain) + { + if (encodingChain!= null && encodingChain.size() > 0 ) + { + List unsafeEncoders = new ArrayList<>(); + for (String encoderKey : encodingChain) + { + Object encoder = encoders.get(encoderKey); + if (encoder == null) throw new AlfrescoRuntimeException("Invalid encoder specified: "+encoderKey); + if (encoder instanceof org.springframework.security.crypto.password.PasswordEncoder) + { + //BCRYPT uses its own internal salt so its NOT safe to use it more than once in an encoding chain. + //the Spring PasswordEncoder class doesn't require a salt and BCRYPTEncoder implements this, so + //we will count the instances of Spring PasswordEncoder + unsafeEncoders.add(encoderKey); + } + } + + if (unsafeEncoders.isEmpty()) return true; + if (unsafeEncoders.size() == 1 && unsafeEncoders.get(0).equals(encodingChain.get(encodingChain.size()-1))) + { + //The unsafe encoder is used at the end so that's ok. + return true; + } + logger.warn("Unsafe encoders in the encoding chain: "+Arrays.toString(unsafeEncoders.toArray()) + +". Only 1 unsafe encoder is allowed at the end of the chain: "+Arrays.toString(encodingChain.toArray())); + } + return false; + } + /** * Basic init method for checking mandatory properties */ diff --git a/source/java/org/alfresco/repo/security/authentication/UpgradePasswordHashWorker.java b/source/java/org/alfresco/repo/security/authentication/UpgradePasswordHashWorker.java index 73cf839ca8..5b3cdfa800 100644 --- a/source/java/org/alfresco/repo/security/authentication/UpgradePasswordHashWorker.java +++ b/source/java/org/alfresco/repo/security/authentication/UpgradePasswordHashWorker.java @@ -281,21 +281,31 @@ public class UpgradePasswordHashWorker implements ApplicationContextAware, Initi // determine if current password hash matches the preferred encoding if (!passwordEncoder.lastEncodingIsPreferred(passwordHash.getFirst())) { + String username = (String)properties.get(ContentModel.PROP_USER_USERNAME); + // We need to double hash - if (logger.isTraceEnabled()) - { - String username = (String)properties.get(ContentModel.PROP_USER_USERNAME); - logger.trace("Double hashing user '" + username + "'."); - } List nowHashed = new ArrayList(); nowHashed.addAll(passwordHash.getFirst()); nowHashed.add(passwordEncoder.getPreferredEncoding()); - Object salt = properties.get(ContentModel.PROP_SALT); - properties.put(ContentModel.PROP_PASSWORD_HASH, passwordEncoder.encodePreferred(new String(passwordHash.getSecond()), salt)); - properties.put(ContentModel.PROP_HASH_INDICATOR, (Serializable)nowHashed); - properties.remove(ContentModel.PROP_PASSWORD); - properties.remove(ContentModel.PROP_PASSWORD_SHA256); - return true; + + if (passwordEncoder.isSafeToEncodeChain(nowHashed)) + { + if (logger.isTraceEnabled()) + { + logger.trace("Double hashing user '" + username + "'."); + } + Object salt = properties.get(ContentModel.PROP_SALT); + properties.put(ContentModel.PROP_PASSWORD_HASH, passwordEncoder.encodePreferred(new String(passwordHash.getSecond()), salt)); + properties.put(ContentModel.PROP_HASH_INDICATOR, (Serializable)nowHashed); + properties.remove(ContentModel.PROP_PASSWORD); + properties.remove(ContentModel.PROP_PASSWORD_SHA256); + return true; + } + else + { + logger.warn("Unsafe to Double Hash user: " + username + "'. The user needs to login first."); + return false; + } } // ensure password hash is in the correct place diff --git a/source/test-java/org/alfresco/repo/security/authentication/CompositePasswordEncoderTest.java b/source/test-java/org/alfresco/repo/security/authentication/CompositePasswordEncoderTest.java index 9c979284d3..2bc161e07a 100644 --- a/source/test-java/org/alfresco/repo/security/authentication/CompositePasswordEncoderTest.java +++ b/source/test-java/org/alfresco/repo/security/authentication/CompositePasswordEncoderTest.java @@ -221,6 +221,30 @@ public class CompositePasswordEncoderTest assertFalse(encoder.matches("sha256", SOURCE_PASSWORD, encoded, null)); } + @Test + public void testSafeEncodingChain() throws Exception + { + List mdbChain = Arrays.asList("bcrypt10"); + + assertTrue(encoder.isSafeToEncodeChain(mdbChain)); + mdbChain = Arrays.asList("md4","bcrypt10"); + assertTrue(encoder.isSafeToEncodeChain(mdbChain)); + mdbChain = Arrays.asList("sha256","bcrypt10"); + assertTrue(encoder.isSafeToEncodeChain(mdbChain)); + mdbChain = Arrays.asList("md4","sha256","bcrypt10"); + assertTrue(encoder.isSafeToEncodeChain(mdbChain)); + mdbChain = Arrays.asList("sha256","md4"); + assertTrue(encoder.isSafeToEncodeChain(mdbChain)); + + mdbChain = Arrays.asList("bcrypt10", "sha256","md4"); + assertFalse(encoder.isSafeToEncodeChain(mdbChain)); + mdbChain = Arrays.asList("bcrypt10", "bcrypt11"); + assertFalse(encoder.isSafeToEncodeChain(mdbChain)); + mdbChain = Arrays.asList("bcrypt10", "sha256", "bcrypt11"); + assertFalse(encoder.isSafeToEncodeChain(mdbChain)); + mdbChain = Arrays.asList("md4","bcrypt10","sha256"); + assertFalse(encoder.isSafeToEncodeChain(mdbChain)); + } @Test public void testEncodeChain() throws Exception {