Allow upgrading between different BCrypt encodings

Fixes spring-projectsgh-7042
jkubrynski · Jul 3, 2019 · d3d6a87 · d3d6a87
1 parent 4b0fb19
commit d3d6a87
Show file tree

Hide file tree

Showing 4 changed files with 69 additions and 14 deletions.
diff --git a/crypto/src/main/java/org/springframework/security/crypto/bcrypt/BCryptPasswordEncoder.java b/crypto/src/main/java/org/springframework/security/crypto/bcrypt/BCryptPasswordEncoder.java
@@ -20,6 +20,7 @@
 import org.springframework.security.crypto.password.PasswordEncoder;
 
 import java.security.SecureRandom;
+import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
 /**
@@ -32,7 +33,7 @@
  */
 public class BCryptPasswordEncoder implements PasswordEncoder {
 	private Pattern BCRYPT_PATTERN = Pattern
-			.compile("\\A\\$2(a|y|b)?\\$\\d\\d\\$[./0-9A-Za-z]{53}");
+			.compile("\\A\\$2(a|y|b)?\\$(\\d\\d)\\$[./0-9A-Za-z]{53}");
 	private final Log logger = LogFactory.getLog(getClass());
 
 	private final int strength;
@@ -93,20 +94,16 @@ public BCryptPasswordEncoder(BCryptVersion version, int strength, SecureRandom r
 			throw new IllegalArgumentException("Bad strength");
 		}
 		this.version = version;
-		this.strength = strength;
+		this.strength = strength == -1 ? 10 : strength;
 		this.random = random;
 	}
 
 	public String encode(CharSequence rawPassword) {
 		String salt;
-		if (strength > 0) {
-			if (random != null) {
-				salt = BCrypt.gensalt(version.getVersion(), strength, random);
-			} else {
-				salt = BCrypt.gensalt(version.getVersion(), strength);
-			}
+		if (random != null) {
+			salt = BCrypt.gensalt(version.getVersion(), strength, random);
 		} else {
-			salt = BCrypt.gensalt(version.getVersion());
+			salt = BCrypt.gensalt(version.getVersion(), strength);
 		}
 		return BCrypt.hashpw(rawPassword.toString(), salt);
 	}
@@ -125,6 +122,23 @@ public boolean matches(CharSequence rawPassword, String encodedPassword) {
 		return BCrypt.checkpw(rawPassword.toString(), encodedPassword);
 	}
 
+	@Override
+	public boolean upgradeEncoding(String encodedPassword) {
+		if (encodedPassword == null || encodedPassword.length() == 0) {
+			logger.warn("Empty encoded password");
+			return false;
+		}
+
+		Matcher matcher = BCRYPT_PATTERN.matcher(encodedPassword);
+		if (!matcher.matches()) {
+			throw new IllegalArgumentException("Encoded password does not look like BCrypt: " + encodedPassword);
+		}
+		else {
+			int strength = Integer.parseInt(matcher.group(2));
+			return strength < this.strength;
+		}
+	}
+
 	/**
 	 * Stores the default bcrypt version for use in configuration.
 	 *

diff --git a/...src/main/java/org/springframework/security/crypto/password/DelegatingPasswordEncoder.java b/...src/main/java/org/springframework/security/crypto/password/DelegatingPasswordEncoder.java
@@ -217,9 +217,15 @@ private String extractId(String prefixEncodedPassword) {
 	}
 
 	@Override
-	public boolean upgradeEncoding(String encodedPassword) {
-		String id = extractId(encodedPassword);
-		return !this.idForEncode.equalsIgnoreCase(id);
+	public boolean upgradeEncoding(String prefixEncodedPassword) {
+		String id = extractId(prefixEncodedPassword);
+		if (!this.idForEncode.equalsIgnoreCase(id)) {
+			return true;
+		}
+		else {
+			String encodedPassword = extractEncodedPassword(prefixEncodedPassword);
+			return this.idToPasswordEncoder.get(id).upgradeEncoding(encodedPassword);
+		}
 	}
 
 	private String extractEncodedPassword(String prefixEncodedPassword) {

diff --git a/.../src/test/java/org/springframework/security/crypto/bcrypt/BCryptPasswordEncoderTests.java b/.../src/test/java/org/springframework/security/crypto/bcrypt/BCryptPasswordEncoderTests.java
@@ -169,4 +169,35 @@ public void doesntMatchBogusEncodedValue() {
 		assertThat(encoder.matches("password", "012345678901234567890123456789")).isFalse();
 	}
 
+	@Test
+	public void upgradeFromLowerStrength() {
+		BCryptPasswordEncoder weakEncoder = new BCryptPasswordEncoder(5);
+		BCryptPasswordEncoder strongEncoder = new BCryptPasswordEncoder(15);
+
+		String weakPassword = weakEncoder.encode("password");
+		String strongPassword = strongEncoder.encode("password");
+
+		assertThat(weakEncoder.upgradeEncoding(strongPassword)).isFalse();
+		assertThat(strongEncoder.upgradeEncoding(weakPassword)).isTrue();
+	}
+
+	/**
+	 * @see <a href="https://github.com/spring-projects/spring-security/pull/7042#issuecomment-506755496">https://github.com/spring-projects/spring-security/pull/7042#issuecomment-506755496</>
+	 */
+	@Test
+	public void upgradeFromNullOrEmpty() {
+		BCryptPasswordEncoder encoder = new BCryptPasswordEncoder();
+		assertThat(encoder.upgradeEncoding(null)).isFalse();
+		assertThat(encoder.upgradeEncoding("")).isFalse();
+	}
+
+	/**
+	 * @see <a href="https://github.com/spring-projects/spring-security/pull/7042#issuecomment-506755496">https://github.com/spring-projects/spring-security/pull/7042#issuecomment-506755496</>
+	 */
+	@Test(expected = IllegalArgumentException.class)
+	public void upgradeFromNonBCrypt() {
+		BCryptPasswordEncoder encoder = new BCryptPasswordEncoder();
+		encoder.upgradeEncoding("not-a-bcrypt-password");
+	}
+
 }
diff --git a/...est/java/org/springframework/security/crypto/password/DelegatingPasswordEncoderTests.java b/...est/java/org/springframework/security/crypto/password/DelegatingPasswordEncoderTests.java
@@ -215,12 +215,16 @@ public void upgradeEncodingWhenIdInvalidFormatThenTrue() {
 	}
 
 	@Test
-	public void upgradeEncodingWhenSameIdThenFalse() {
-		assertThat(this.passwordEncoder.upgradeEncoding(this.bcryptEncodedPassword)).isFalse();
+	public void upgradeEncodingWhenSameIdThenEncoderDecides() {
+		this.passwordEncoder.upgradeEncoding(this.bcryptEncodedPassword);
+
+		verify(bcrypt).upgradeEncoding(this.encodedPassword);
 	}
 
 	@Test
 	public void upgradeEncodingWhenDifferentIdThenTrue() {
 		assertThat(this.passwordEncoder.upgradeEncoding(this.noopEncodedPassword)).isTrue();
+
+		verifyZeroInteractions(bcrypt);
 	}
 }