From 2db54c53ba998dd1b23f247ca2fcfe74c88933fd Mon Sep 17 00:00:00 2001 From: Lars Grefer Date: Wed, 26 Jun 2019 22:21:57 +0200 Subject: [PATCH 1/3] Allow upgrading between different BCrypt encodings --- .../crypto/bcrypt/BCryptPasswordEncoder.java | 33 ++++++++++++++----- .../password/DelegatingPasswordEncoder.java | 12 +++++-- .../bcrypt/BCryptPasswordEncoderTests.java | 12 +++++++ 3 files changed, 45 insertions(+), 12 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 index 21efe009462..b70a6c6597b 100644 --- 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,24 @@ 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()) { + logger.warn("Encoded password does not look like BCrypt"); + return false; + } + else { + int strength = Integer.parseInt(matcher.group(2)); + return strength < this.strength; + } + } + /** * Stores the default bcrypt version for use in configuration. * diff --git a/crypto/src/main/java/org/springframework/security/crypto/password/DelegatingPasswordEncoder.java b/crypto/src/main/java/org/springframework/security/crypto/password/DelegatingPasswordEncoder.java index c9bd476c3c9..4bc4e68083a 100644 --- a/crypto/src/main/java/org/springframework/security/crypto/password/DelegatingPasswordEncoder.java +++ b/crypto/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/crypto/src/test/java/org/springframework/security/crypto/bcrypt/BCryptPasswordEncoderTests.java b/crypto/src/test/java/org/springframework/security/crypto/bcrypt/BCryptPasswordEncoderTests.java index 9f574d6c51e..8e2062af91b 100644 --- a/crypto/src/test/java/org/springframework/security/crypto/bcrypt/BCryptPasswordEncoderTests.java +++ b/crypto/src/test/java/org/springframework/security/crypto/bcrypt/BCryptPasswordEncoderTests.java @@ -169,4 +169,16 @@ 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(); + } + } From 9c6bbb850fc9235c2d0a6a2b865b25dcd6783119 Mon Sep 17 00:00:00 2001 From: Lars Grefer Date: Thu, 27 Jun 2019 22:48:27 +0200 Subject: [PATCH 2/3] Add Tests and align behaviour --- .../crypto/bcrypt/BCryptPasswordEncoder.java | 4 ++-- .../bcrypt/BCryptPasswordEncoderTests.java | 16 ++++++++++++++++ .../password/DelegatingPasswordEncoderTests.java | 8 ++++++-- 3 files changed, 24 insertions(+), 4 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 index b70a6c6597b..fc97804adf0 100644 --- a/crypto/src/main/java/org/springframework/security/crypto/bcrypt/BCryptPasswordEncoder.java +++ b/crypto/src/main/java/org/springframework/security/crypto/bcrypt/BCryptPasswordEncoder.java @@ -126,13 +126,13 @@ public boolean matches(CharSequence rawPassword, String encodedPassword) { public boolean upgradeEncoding(String encodedPassword) { if (encodedPassword == null || encodedPassword.length() == 0) { logger.warn("Empty encoded password"); - return false; + return true; } Matcher matcher = BCRYPT_PATTERN.matcher(encodedPassword); if (!matcher.matches()) { logger.warn("Encoded password does not look like BCrypt"); - return false; + return true; } else { int strength = Integer.parseInt(matcher.group(2)); diff --git a/crypto/src/test/java/org/springframework/security/crypto/bcrypt/BCryptPasswordEncoderTests.java b/crypto/src/test/java/org/springframework/security/crypto/bcrypt/BCryptPasswordEncoderTests.java index 8e2062af91b..a715681b8bf 100644 --- a/crypto/src/test/java/org/springframework/security/crypto/bcrypt/BCryptPasswordEncoderTests.java +++ b/crypto/src/test/java/org/springframework/security/crypto/bcrypt/BCryptPasswordEncoderTests.java @@ -16,6 +16,7 @@ package org.springframework.security.crypto.bcrypt; import org.junit.Test; +import org.springframework.security.crypto.password.DelegatingPasswordEncoderTests; import java.security.SecureRandom; @@ -181,4 +182,19 @@ public void upgradeFromLowerStrength() { assertThat(strongEncoder.upgradeEncoding(weakPassword)).isTrue(); } + /** + * @see DelegatingPasswordEncoderTests#upgradeEncodingWhenNullIdThenTrue() + */ + @Test + public void upgradeFromNull() { + BCryptPasswordEncoder encoder = new BCryptPasswordEncoder(); + assertThat(encoder.upgradeEncoding(null)).isTrue(); + } + + @Test + public void upgradeFromNonBCrypt() { + BCryptPasswordEncoder encoder = new BCryptPasswordEncoder(); + assertThat(encoder.upgradeEncoding("not-a-bcrypt-password")).isTrue(); + } + } diff --git a/crypto/src/test/java/org/springframework/security/crypto/password/DelegatingPasswordEncoderTests.java b/crypto/src/test/java/org/springframework/security/crypto/password/DelegatingPasswordEncoderTests.java index cd24afca479..6fdea55a840 100644 --- a/crypto/src/test/java/org/springframework/security/crypto/password/DelegatingPasswordEncoderTests.java +++ b/crypto/src/test/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); } } From a875499ee00d4601d1592b6aa2c67533ec2f0daf Mon Sep 17 00:00:00 2001 From: Lars Grefer Date: Fri, 28 Jun 2019 17:44:55 +0200 Subject: [PATCH 3/3] Adjust behaviour for Invalid inputs --- .../crypto/bcrypt/BCryptPasswordEncoder.java | 5 ++--- .../crypto/bcrypt/BCryptPasswordEncoderTests.java | 15 +++++++++------ 2 files changed, 11 insertions(+), 9 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 index fc97804adf0..c59246320d6 100644 --- a/crypto/src/main/java/org/springframework/security/crypto/bcrypt/BCryptPasswordEncoder.java +++ b/crypto/src/main/java/org/springframework/security/crypto/bcrypt/BCryptPasswordEncoder.java @@ -126,13 +126,12 @@ public boolean matches(CharSequence rawPassword, String encodedPassword) { public boolean upgradeEncoding(String encodedPassword) { if (encodedPassword == null || encodedPassword.length() == 0) { logger.warn("Empty encoded password"); - return true; + return false; } Matcher matcher = BCRYPT_PATTERN.matcher(encodedPassword); if (!matcher.matches()) { - logger.warn("Encoded password does not look like BCrypt"); - return true; + throw new IllegalArgumentException("Encoded password does not look like BCrypt: " + encodedPassword); } else { int strength = Integer.parseInt(matcher.group(2)); diff --git a/crypto/src/test/java/org/springframework/security/crypto/bcrypt/BCryptPasswordEncoderTests.java b/crypto/src/test/java/org/springframework/security/crypto/bcrypt/BCryptPasswordEncoderTests.java index a715681b8bf..28ac723bce6 100644 --- a/crypto/src/test/java/org/springframework/security/crypto/bcrypt/BCryptPasswordEncoderTests.java +++ b/crypto/src/test/java/org/springframework/security/crypto/bcrypt/BCryptPasswordEncoderTests.java @@ -16,7 +16,6 @@ package org.springframework.security.crypto.bcrypt; import org.junit.Test; -import org.springframework.security.crypto.password.DelegatingPasswordEncoderTests; import java.security.SecureRandom; @@ -183,18 +182,22 @@ public void upgradeFromLowerStrength() { } /** - * @see DelegatingPasswordEncoderTests#upgradeEncodingWhenNullIdThenTrue() + * @see https://github.com/spring-projects/spring-security/pull/7042#issuecomment-506755496 */ @Test - public void upgradeFromNull() { + public void upgradeFromNullOrEmpty() { BCryptPasswordEncoder encoder = new BCryptPasswordEncoder(); - assertThat(encoder.upgradeEncoding(null)).isTrue(); + assertThat(encoder.upgradeEncoding(null)).isFalse(); + assertThat(encoder.upgradeEncoding("")).isFalse(); } - @Test + /** + * @see https://github.com/spring-projects/spring-security/pull/7042#issuecomment-506755496 + */ + @Test(expected = IllegalArgumentException.class) public void upgradeFromNonBCrypt() { BCryptPasswordEncoder encoder = new BCryptPasswordEncoder(); - assertThat(encoder.upgradeEncoding("not-a-bcrypt-password")).isTrue(); + encoder.upgradeEncoding("not-a-bcrypt-password"); } }