From 8c08eeb57bc697295f7bfe44663ca504967c43c2 Mon Sep 17 00:00:00 2001 From: Rob Winch Date: Mon, 31 Jan 2011 23:00:16 -0600 Subject: [PATCH] SEC-1666: Use constant time comparison for sensitive data. Constant time comparison helps to mitigate timing attacks. See the following link for more information * http://rdist.root.org/2010/07/19/exploiting-remote-timing-attacks/ * http://en.wikipedia.org/wiki/Timing_attack for more information. --- .../encoding/LdapShaPasswordEncoder.java | 2 +- .../encoding/Md4PasswordEncoder.java | 2 +- .../MessageDigestPasswordEncoder.java | 2 +- .../encoding/PasswordEncoderUtils.java | 45 +++++++++++++++++++ .../encoding/PlaintextPasswordEncoder.java | 11 +++-- .../encoding/PasswordEncoderUtilsTests.java | 33 ++++++++++++++ .../password/StandardPasswordEncoder.java | 19 ++++++-- .../StandardPasswordEncoderTests.java | 6 +++ .../TokenBasedRememberMeServices.java | 38 +++++++++++++++- 9 files changed, 145 insertions(+), 13 deletions(-) create mode 100644 core/src/main/java/org/springframework/security/authentication/encoding/PasswordEncoderUtils.java create mode 100644 core/src/test/java/org/springframework/security/authentication/encoding/PasswordEncoderUtilsTests.java diff --git a/core/src/main/java/org/springframework/security/authentication/encoding/LdapShaPasswordEncoder.java b/core/src/main/java/org/springframework/security/authentication/encoding/LdapShaPasswordEncoder.java index fbe5eed42d..f349df9c96 100644 --- a/core/src/main/java/org/springframework/security/authentication/encoding/LdapShaPasswordEncoder.java +++ b/core/src/main/java/org/springframework/security/authentication/encoding/LdapShaPasswordEncoder.java @@ -145,7 +145,7 @@ public class LdapShaPasswordEncoder implements PasswordEncoder { String encodedRawPass = encodePassword(rawPass, salt).substring(startOfHash); - return encodedRawPass.equals(encPass.substring(startOfHash)); + return PasswordEncoderUtils.equals(encodedRawPass,encPass.substring(startOfHash)); } /** diff --git a/core/src/main/java/org/springframework/security/authentication/encoding/Md4PasswordEncoder.java b/core/src/main/java/org/springframework/security/authentication/encoding/Md4PasswordEncoder.java index c9422c5373..bb2751c7dc 100644 --- a/core/src/main/java/org/springframework/security/authentication/encoding/Md4PasswordEncoder.java +++ b/core/src/main/java/org/springframework/security/authentication/encoding/Md4PasswordEncoder.java @@ -78,7 +78,7 @@ public class Md4PasswordEncoder extends BaseDigestPasswordEncoder { public boolean isPasswordValid(String encPass, String rawPass, Object salt) { String pass1 = "" + encPass; String pass2 = encodePassword(rawPass, salt); - return pass1.equals(pass2); + return PasswordEncoderUtils.equals(pass1,pass2); } public String getAlgorithm() { diff --git a/core/src/main/java/org/springframework/security/authentication/encoding/MessageDigestPasswordEncoder.java b/core/src/main/java/org/springframework/security/authentication/encoding/MessageDigestPasswordEncoder.java index cda94945fb..86f08d08ff 100644 --- a/core/src/main/java/org/springframework/security/authentication/encoding/MessageDigestPasswordEncoder.java +++ b/core/src/main/java/org/springframework/security/authentication/encoding/MessageDigestPasswordEncoder.java @@ -126,7 +126,7 @@ public class MessageDigestPasswordEncoder extends BaseDigestPasswordEncoder { String pass1 = "" + encPass; String pass2 = encodePassword(rawPass, salt); - return pass1.equals(pass2); + return PasswordEncoderUtils.equals(pass1,pass2); } public String getAlgorithm() { diff --git a/core/src/main/java/org/springframework/security/authentication/encoding/PasswordEncoderUtils.java b/core/src/main/java/org/springframework/security/authentication/encoding/PasswordEncoderUtils.java new file mode 100644 index 0000000000..f821dbf36b --- /dev/null +++ b/core/src/main/java/org/springframework/security/authentication/encoding/PasswordEncoderUtils.java @@ -0,0 +1,45 @@ +package org.springframework.security.authentication.encoding; + +import java.io.UnsupportedEncodingException; + +/** + * Utility for constant time comparison to prevent against timing attacks. + * + * @author Rob Winch + */ +class PasswordEncoderUtils { + + /** + * Constant time comparison to prevent against timing attacks. + * @param expected + * @param actual + * @return + */ + static boolean equals(String expected, String actual) { + byte[] expectedBytes = bytesUtf8(expected); + byte[] actualBytes = bytesUtf8(actual); + int expectedLength = expectedBytes == null ? -1 : expectedBytes.length; + int actualLength = actualBytes == null ? -1 : actualBytes.length; + if (expectedLength != actualLength) { + return false; + } + + int result = 0; + for (int i = 0; i < expectedLength; i++) { + result |= expectedBytes[i] ^ actualBytes[i]; + } + return result == 0; + } + + private static byte[] bytesUtf8(String s) { + if(s == null) { + return null; + } + try { + return s.getBytes("UTF-8"); + } catch (UnsupportedEncodingException e) { + throw new IllegalStateException("Could not get bytes in UTF-8 format",e); + } + } + private PasswordEncoderUtils() {} +} \ No newline at end of file diff --git a/core/src/main/java/org/springframework/security/authentication/encoding/PlaintextPasswordEncoder.java b/core/src/main/java/org/springframework/security/authentication/encoding/PlaintextPasswordEncoder.java index f414182b1c..14a996c842 100644 --- a/core/src/main/java/org/springframework/security/authentication/encoding/PlaintextPasswordEncoder.java +++ b/core/src/main/java/org/springframework/security/authentication/encoding/PlaintextPasswordEncoder.java @@ -15,6 +15,8 @@ package org.springframework.security.authentication.encoding; +import java.util.Locale; + /** *

Plaintext implementation of PasswordEncoder.

*

As callers may wish to extract the password and salts separately from the encoded password, the salt must @@ -46,11 +48,12 @@ public class PlaintextPasswordEncoder extends BasePasswordEncoder { // authentication will fail as the encodePassword never allows them) String pass2 = mergePasswordAndSalt(rawPass, salt, false); - if (!ignorePasswordCase) { - return pass1.equals(pass2); - } else { - return pass1.equalsIgnoreCase(pass2); + if (ignorePasswordCase) { + // Note: per String javadoc to get correct results for Locale insensitive, use English + pass1 = pass1.toLowerCase(Locale.ENGLISH); + pass2 = pass2.toLowerCase(Locale.ENGLISH); } + return PasswordEncoderUtils.equals(pass1,pass2); } /** diff --git a/core/src/test/java/org/springframework/security/authentication/encoding/PasswordEncoderUtilsTests.java b/core/src/test/java/org/springframework/security/authentication/encoding/PasswordEncoderUtilsTests.java new file mode 100644 index 0000000000..2406de3864 --- /dev/null +++ b/core/src/test/java/org/springframework/security/authentication/encoding/PasswordEncoderUtilsTests.java @@ -0,0 +1,33 @@ +package org.springframework.security.authentication.encoding; + +import static org.junit.Assert.*; + +import org.junit.Test; +/** + * @author Rob Winch + */ +public class PasswordEncoderUtilsTests { + + @Test + public void differentLength() { + assertFalse(PasswordEncoderUtils.equals("abc", "a")); + assertFalse(PasswordEncoderUtils.equals("a", "abc")); + } + + @Test + public void equalsNull() { + assertFalse(PasswordEncoderUtils.equals(null, "a")); + assertFalse(PasswordEncoderUtils.equals("a", null)); + assertTrue(PasswordEncoderUtils.equals(null, null)); + } + + @Test + public void equalsCaseSensitive() { + assertFalse(PasswordEncoderUtils.equals("aBc", "abc")); + } + + @Test + public void equalsSuccess() { + assertTrue(PasswordEncoderUtils.equals("abcdef", "abcdef")); + } +} diff --git a/crypto/src/main/java/org/springframework/security/crypto/password/StandardPasswordEncoder.java b/crypto/src/main/java/org/springframework/security/crypto/password/StandardPasswordEncoder.java index 71a8be7394..420a2c6772 100644 --- a/crypto/src/main/java/org/springframework/security/crypto/password/StandardPasswordEncoder.java +++ b/crypto/src/main/java/org/springframework/security/crypto/password/StandardPasswordEncoder.java @@ -21,8 +21,6 @@ import static org.springframework.security.crypto.util.EncodingUtils.hexEncode; import static org.springframework.security.crypto.util.EncodingUtils.subArray; import static org.springframework.security.crypto.util.EncodingUtils.utf8Encode; -import java.util.Arrays; - import org.springframework.security.crypto.keygen.BytesKeyGenerator; import org.springframework.security.crypto.keygen.KeyGenerators; import org.springframework.security.crypto.util.Digester; @@ -79,8 +77,21 @@ public final class StandardPasswordEncoder implements PasswordEncoder { return hexDecode(encodedPassword); } + /** + * Constant time comparison to prevent against timing attacks. + * @param expected + * @param actual + * @return + */ private boolean matches(byte[] expected, byte[] actual) { - return Arrays.equals(expected, actual); - } + if (expected.length != actual.length) { + return false; + } + int result = 0; + for (int i = 0; i < expected.length; i++) { + result |= expected[i] ^ actual[i]; + } + return result == 0; + } } \ No newline at end of file diff --git a/crypto/src/test/java/org/springframework/security/crypto/password/StandardPasswordEncoderTests.java b/crypto/src/test/java/org/springframework/security/crypto/password/StandardPasswordEncoderTests.java index 1395034b8f..044ac9a9fc 100644 --- a/crypto/src/test/java/org/springframework/security/crypto/password/StandardPasswordEncoderTests.java +++ b/crypto/src/test/java/org/springframework/security/crypto/password/StandardPasswordEncoderTests.java @@ -16,6 +16,12 @@ public class StandardPasswordEncoderTests { assertTrue(encoder.matches("password", result)); } + @Test + public void matchesLengthChecked() { + String result = encoder.encode("password"); + assertFalse(encoder.matches("password", result.substring(0,result.length()-1))); + } + @Test public void notMatches() { String result = encoder.encode("password"); diff --git a/web/src/main/java/org/springframework/security/web/authentication/rememberme/TokenBasedRememberMeServices.java b/web/src/main/java/org/springframework/security/web/authentication/rememberme/TokenBasedRememberMeServices.java index aa59a37eab..3d1a1db5a5 100644 --- a/web/src/main/java/org/springframework/security/web/authentication/rememberme/TokenBasedRememberMeServices.java +++ b/web/src/main/java/org/springframework/security/web/authentication/rememberme/TokenBasedRememberMeServices.java @@ -23,6 +23,7 @@ import org.springframework.util.StringUtils; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import java.io.UnsupportedEncodingException; import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; import java.util.Arrays; @@ -81,6 +82,7 @@ public class TokenBasedRememberMeServices extends AbstractRememberMeServices { //~ Methods ======================================================================================================== + @Override protected UserDetails processAutoLoginCookie(String[] cookieTokens, HttpServletRequest request, HttpServletResponse response) { @@ -117,9 +119,9 @@ public class TokenBasedRememberMeServices extends AbstractRememberMeServices { String expectedTokenSignature = makeTokenSignature(tokenExpiryTime, userDetails.getUsername(), userDetails.getPassword()); - if (!expectedTokenSignature.equals(cookieTokens[2])) { + if (!equals(expectedTokenSignature,cookieTokens[2])) { throw new InvalidCookieException("Cookie token[2] contained signature '" + cookieTokens[2] - + "' but expected '" + expectedTokenSignature + "'"); + + "' but expected '" + expectedTokenSignature + "'"); } return userDetails; @@ -145,6 +147,7 @@ public class TokenBasedRememberMeServices extends AbstractRememberMeServices { return tokenExpiryTime < System.currentTimeMillis(); } + @Override public void onLoginSuccess(HttpServletRequest request, HttpServletResponse response, Authentication successfulAuthentication) { @@ -216,4 +219,35 @@ public class TokenBasedRememberMeServices extends AbstractRememberMeServices { private boolean isInstanceOfUserDetails(Authentication authentication) { return authentication.getPrincipal() instanceof UserDetails; } + + /** + * Constant time comparison to prevent against timing attacks. + * @param expected + * @param actual + * @return + */ + private static boolean equals(String expected, String actual) { + byte[] expectedBytes = bytesUtf8(expected); + byte[] actualBytes = bytesUtf8(actual); + if (expectedBytes.length != actualBytes.length) { + return false; + } + + int result = 0; + for (int i = 0; i < expectedBytes.length; i++) { + result |= expectedBytes[i] ^ actualBytes[i]; + } + return result == 0; + } + + private static byte[] bytesUtf8(String s) { + if(s == null) { + return null; + } + try { + return s.getBytes("UTF-8"); + } catch (UnsupportedEncodingException e) { + throw new IllegalStateException("Could not get bytes in UTF-8 format",e); + } + } }