Summary
We are using Spring Security 5.0.4. (I looked at the latest code and it is the same as what we have because no changes to BCrypt have occurred) We are getting an ArrayIndexOutOfBoundsException when it is trying to bcrypt hash the password that came in on the session. The reason for the exception is if the password passed in by the user is NULL, or Empty, BCrypt throws the exception when it is trying to bcrypt hash an empty string.
I pulled down Spring Security and added some logging statements to find out what is going on. I was able to reproduce the problem if the login page passed in an empty password. And confirmed that the parameter byte data[] in BCrypt.streamtoword(byte data[], int offp[]) has no length.
Actual Behavior
When Spring Security receives a password and tries to bcrypt hash it, it calls BCryptPasswordEncoder.matches(CharSequence rawPassword, String encodedPassword), the rawPassword is NULL, or Empty. Interestingly, this method does a NULL/Empty check on the encodedPassword, but does not do the same for the rawPassword.
BCryptPasswordEncoder
public boolean matches(CharSequence rawPassword, String encodedPassword) {
if (encodedPassword == null || encodedPassword.length() == 0) {
logger.warn("Empty encoded password");
return false;
}
if (!BCRYPT_PATTERN.matcher(encodedPassword).matches()) {
logger.warn("Encoded password does not look like BCrypt");
return false;
}
return BCrypt.checkpw(rawPassword.toString(), encodedPassword);
}
Not validating the rawPassword causes an ArrayIndexOutOfBoundsExeption later on in BCrypt with it tries to get elements from an array.
BCrypt
private static int streamtoword(byte data[], int offp[]) {
int i;
int word = 0;
int off = offp[0];
for (i = 0; i < 4; i++) {
word = (word << 8) | (data[off] & 0xff); // Line 410: byte data[] has no length
off = (off + 1) % data.length;
}
offp[0] = off;
return word;
}
Here is the stack trace we are getting:
java.lang.ArrayIndexOutOfBoundsException: 0
org.springframework.security.crypto.bcrypt.BCrypt.streamtoword(BCrypt.java:410)
org.springframework.security.crypto.bcrypt.BCrypt.ekskey(BCrypt.java:466)
org.springframework.security.crypto.bcrypt.BCrypt.crypt_raw(BCrypt.java:508)
org.springframework.security.crypto.bcrypt.BCrypt.hashpw(BCrypt.java:590)
org.springframework.security.crypto.bcrypt.BCrypt.checkpw(BCrypt.java:659)
org.springframework.security.crypto.bcrypt.BCryptPasswordEncoder.matches(BCryptPasswordEncoder.java:94)
org.springframework.security.authentication.dao.DaoAuthenticationProvider.additionalAuthenticationChecks(DaoAuthenticationProvider.java:86)
org.springframework.security.authentication.dao.AbstractUserDetailsAuthenticationProvider.authenticate(AbstractUserDetailsAuthenticationProvider.java:166)
org.springframework.security.authentication.ProviderManager.authenticate(ProviderManager.java:174)
org.springframework.security.authentication.ProviderManager.authenticate(ProviderManager.java:199)
org.springframework.security.web.authentication.UsernamePasswordAuthenticationFilter.attemptAuthentication(UsernamePasswordAuthenticationFilter.java:94)
org.springframework.security.web.authentication.AbstractAuthenticationProcessingFilter.doFilter(AbstractAuthenticationProcessingFilter.java:212)
org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:334)
org.springframework.security.web.authentication.logout.LogoutFilter.doFilter(LogoutFilter.java:116)
org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:334)
org.springframework.security.web.header.HeaderWriterFilter.doFilterInternal(HeaderWriterFilter.java:66)
org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107)
org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:334)
org.springframework.security.web.context.request.async.WebAsyncManagerIntegrationFilter.doFilterInternal(WebAsyncManagerIntegrationFilter.java:56)
org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107)
org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:334)
org.springframework.security.web.context.SecurityContextPersistenceFilter.doFilter(SecurityContextPersistenceFilter.java:105)
org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:334)
org.springframework.security.web.FilterChainProxy.doFilterInternal(FilterChainProxy.java:215)
org.springframework.security.web.FilterChainProxy.doFilter(FilterChainProxy.java:178)
org.springframework.web.filter.DelegatingFilterProxy.invokeDelegate(DelegatingFilterProxy.java:357)
org.springframework.web.filter.DelegatingFilterProxy.doFilter(DelegatingFilterProxy.java:270)
org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:240)
org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:207)
org.springframework.session.web.http.SessionRepositoryFilter.doFilterInternal(SessionRepositoryFilter.java:146)
org.springframework.session.web.http.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:81)
org.springframework.web.filter.DelegatingFilterProxy.invokeDelegate(DelegatingFilterProxy.java:357)
org.springframework.web.filter.DelegatingFilterProxy.doFilter(DelegatingFilterProxy.java:270)
org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:240)
org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:207)
org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:212)
org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:106)
org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:141)
Expected Behavior
BCrypt should not throw an ArrayIndexOutOfBoundsException. Spring Security should really check to make sure the password is NOT NULL or Empty. If there is no password being passed in, why would you try to bcrypt hash it?
What would be expected is public static String hashpw(String password, String salt) would return an empty String when the password parameter is NULL, or empty. This will eliminate the call to BCrypt.streamtoword() and keep it from throwing the ArrayIndexOutOfBoundsException, At the same time, allow BCrypt.equalsNoEarlyReturn() to return false when it tries to match an empty against the existing hashed password.
Configuration
<security:authentication-manager>
<security:authentication-provider user-service-ref="userService">
<security:password-encoder ref="bcryptEncoder" />
</security:authentication-provider>
</security:authentication-manager>
Version
5.0.4
Sample
Change BCrypt.hashpw(String password, String salt) FROM:
public static String hashpw(String password, String salt) throws IllegalArgumentException {
BCrypt B;
String real_salt;
byte passwordb[], saltb[], hashed[];
char minor = (char) 0;
int rounds, off = 0;
StringBuilder rs = new StringBuilder();
if (salt == null) {
throw new IllegalArgumentException("salt cannot be null");
}
....
....
....
....
}
TO:
public static String hashpw(String password, String salt) throws IllegalArgumentException {
BCrypt B;
String real_salt;
byte passwordb[], saltb[], hashed[];
char minor = (char) 0;
int rounds, off = 0;
StringBuilder rs = new StringBuilder();
if (password == null || password.length() == 0) {
return "";
}
if (salt == null) {
throw new IllegalArgumentException("salt cannot be null");
}
....
....
....
....
}
Also, change BCryptPasswordEncoder.equalsNoEarlyReturn(String a, String b) FROM
static boolean equalsNoEarlyReturn(String a, String b) {
char[] caa = a.toCharArray();
char[] cab = b.toCharArray();
if (caa.length != cab.length) {
return false;
}
byte ret = 0;
for (int i = 0; i < caa.length; i++) {
ret |= caa[i] ^ cab[i];
}
return ret == 0;
}
TO:
static boolean equalsNoEarlyReturn(String a, String b) {
if ((a == null || a.length() == 0) && (b == null || b.length() == 0)) {
return false;
} else if ((a != null && a.length() != 0) && (b == null || b.length() == 0)) {
return false;
} else if ((a == null || a.length() == 0) && (b != null && b.length() != 0)) {
return false;
}
char[] caa = a.toCharArray();
char[] cab = b.toCharArray();
if (caa.length != cab.length) {
return false;
}
byte ret = 0;
for (int i = 0; i < caa.length; i++) {
ret |= caa[i] ^ cab[i];
}
return ret == 0;
}
Comment From: pminearo
I pulled down the master branch and to set up a test to get the code to throw the ArrayIndexOutOfBoundsException to be thrown. Unfortunately, I could not figure out a way to get the code to throw the Exception.
The haspw() method does (I know the methods are out of sequence in terms of line numbers, but it makes more sense this way to show what is going on):
public static String hashpw(String password, String salt) throws IllegalArgumentException {
....
....
....
// Nothing is done to the 'password' parameter until the line below.
// 'minor' below will either be 'a' or empty based on whether
// you are using BCrypt2 or 2a.
// If you are using 2a, then "\000" (aka - null) will be returned
// If you are using 2, then "" (aka - empty string) will be returned
// Java treats both of these as the same. This seems to be here, just in case
// 'password' is NULL; then a NullPointerException will not be thrown.
// If that is the case, then are there not better ways.
// If there is a legit reason for it to be here, I would love to know why.
passwordb = (password + (minor >= 'a' ? "\000" : "")).getBytes("UTF-8");
// if password == null && minor >= 'a'; then passwordb = [B@5a2e4553
// if password == "" && minor >= 'a'; then passwordb = [B@5a2e4553
// if password == null && minor < 'a'; then passwordb = [B@5a2e4553
// if password == "" && minor < 'a'; then passwordb = [B@5a2e4553
....
....
B = new BCrypt();
// Nothing is done to the 'passwordb' variable until the line below.
hashed = B.crypt_raw(passwordb, saltb, rounds);
....
....
....
}
private byte[] crypt_raw(byte password[], byte salt[], int log_rounds) {
....
....
// Nothing is done to the 'password' parameter until the line below.
ekskey(salt, password);
....
....
}
private void ekskey(byte data[], byte key[]) {
int i;
int koffp[] = { 0 }, doffp[] = { 0 };
int lr[] = { 0, 0 };
int plen = P.length, slen = S.length;
// Nothing is done to the 'key[]' parameter until the line below.
for (i = 0; i < plen; i++) {
P[i] = P[i] ^ streamtoword(key, koffp);
}
....
....
}
private static int streamtoword(byte data[], int offp[]) {
int i;
int word = 0;
int off = offp[0];
// Nothing is done to the 'data[]' parameter until the line below.
for (i = 0; i < 4; i++) {
// ArrayIndexOutOfBoundsException: 0 is thrown on the line below.
word = (word << 8) | (data[off] & 0xff);
off = (off + 1) % data.length;
}
offp[0] = off;
return word;
}
So, no matter what, passwordb will have some sort of value. Which then makes this stack trace even more perplexing because how does the byte data[] parameter not have a element at 0 index?
Right now, the only thing I can think of is it is a threading issue; but I doubt that is the issue because again how does that parameter not have an element at 0 index.
Comment From: pminearo
This is does bring up the question, should Spring Security be validating NULL or Empty String passwords? While I understand it is doable since Java always returns a Byte array; even if, the passwordb is equal to "\000" or ""; should Spring Security really be doing this??
Comment From: rwinch
Can you clarify how to reproduce the issue you are experiencing?
NOTE: We really don't make any changes to BCrypt directly as that code is copied (with permission) from mindrot. So please reproduce the issue through BCryptPasswordEncoder
Comment From: pminearo
I pulled down Spring Security 5.1.1.-BUILD-SNAPSHOT and made some edits to BCrypt and BCryptPasswordEncoder. The changes I made to BCrypt were catching the ArrayIndexOutOfBoundsException and outputting information that is known at the time of the exception. The idea was to see what information was being passed into BCrypt and hopefully find an answer to why the exception is being thrown. The change I made to BCryptPasswordEncoder was I added a check to ensure the rawPassword is not empty in public boolean matches(CharSequence rawPassword, String encodedPassword).
public boolean matches(CharSequence rawPassword, String encodedPassword) {
if (encodedPassword == null || encodedPassword.length() == 0) {
logger.warn("Empty encoded password");
return false;
}
if (!BCRYPT_PATTERN.matcher(encodedPassword).matches()) {
logger.warn("Encoded password does not look like BCrypt");
return false;
}
if (rawPassword == null || rawPassword.length() == 0) {
logger.warn("Raw Password is Null or Empty.");
return false;
}
return BCrypt.checkpw(rawPassword.toString(), encodedPassword);
}
Since, I have done these 2 changes; I have not seen any ArrayIndexOutOfBoundsExceptions being thrown. I have however seen "Raw Password is Null or Empty." in the log files. Not very often, but have seen it.
The question I have is why is the rawPassword not being validated as Not Empty? If you a non-null and non-empty encodedPassword passed in AND it matches the BCrypt pattern, shouldn't the rawPassword also not be empty? Or does the BCrypt algorithm allow for this case? Since we were not getting a NullPointerException when rawPassword.toString() was called; I am assuming the CharSequence object was not Null; but an empty String.
Comment From: rwinch
@pminearo A raw empty password does not cause an ArrayIndexOutOfBoundsException for me. Can you please provide a way to reproduce your original issue?
Comment From: pminearo
Unfortunately, I have not been able to reproduce the problem consistently. We get these errors randomly, and have not figured out the conditions the error comes from. So, I am still trying to get more information for you. I have however, not seen the Exception since I put in the check for an empty or null rawPassword. I will get you more information as I get it. This may take awhile, unfortunately.
I see you put in a test for an Empty raw password, what about a Null raw password? I think you would get a NullPointerException on line 125 when you try to toString() the password.
But back to my question, is there a reason not to check for an Empty, or Null, raw password? Does Spring Security (and BCrypt) allow for a user to pass in an empty, or null, password?
As I get more information, I will post it here. Thanks for the responses.
Comment From: rwinch
Thank you for the follow up.
A null password is not valid argument, so it is not surprising to get a NPE for that. That means that your input was invalid. Let me know once you figure out a way to reproduce the issue.
Comment From: pminearo
I have been thinking about the issue for the past couple of days and I may know what is going on. It is a "leap of faith" since I don't have any real hard evidence. BUT it does seem to fit.
In our application we have Spring Session and Spring Security. In the web.xml file, Spring Session comes before Spring Security. This means Spring Session looks at the request before Spring Security. We were having a problem with Spring Session causing a Unique Constraint violation when inserting a Session Attribute. This issue comes from multiple requests causing multiple threads in the Tomcat server for the same User session. When the DB would throw the Unique Constraint violation out, Spring Session would fail. A lot of times the ArrayIndexOutOfBoundsException would occur around the same time, but not always. I put the fix, I mentioned above, in Spring Security and we got rid of the ArrayIndexOutOfBoundsException. However, we would see "Raw Password is Null or Empty." in the logs from time to time. And, we were still seeing a lot of Spring Session errors. So, I put in a fix for Spring Session to essentially suppress the Unique Constraint error and continue on. Since I put in these 2 fixes (aka - hacks), we have not seen any problems with Spring Security. No "Raw Password is Null or Empty." have occurred in the log files since then. SO....my thinking is this.....Since Spring Session intercepts the request before Spring Security it would put the whole thread into a "funky" state when it caused a Unique constraint violation which would somehow send in a rawPassword that threw the ArrayIndexOutOfBoundsException.
Still not sure what the rawPassword was when the ArrayIndexOutOfBoundsException was thrown. Unfortunately, we can not reproduce this problem consistently. This means in order to figure out the problem; I would need to revert the raw password check I put in BCryptPasswordEncoder AND the suppression of the Spring Session error in our production environment. Not something I really want to do. If I can figure out a way to reproduce the problem in a testing environment, I will. But until then, we are kind of stuck on this issue.
The only fix I can say Spring Security can do, at this time, is check for null or Empty raw passwords. If a null raw password is not valid, shouldn't BCryptPasswordEncoder be checking for that anyways? And if you are checking for null, it's just as easy to check for an 'Empty' raw password as well. Doing this would be more of a fortification of Spring Security, and not a bug in Spring Security.
If I get more time to try to reproduce this problem in a testing environment; I will post my findings. But it will take awhile. Thanks for the patience and responses.
Comment From: rwinch
The null password means something went wrong. It should thrown an Exception. A user cannot enter a null password. As the test demonstrates, an empty password works as expected.
It might be helpful to share the complete stacktrace you have as well.
Comment From: pminearo
The complete stack trace is in the original comment above.
Maybe have BCryptPasswordEncoder throw an Exception if it gets a Null Password. NullPointerExceptions are too generic and do not give enough information. What would be "nice to have" is if any Exception thrown from a PasswordEncoder Spring Security logs the the login that threw the Exception and maybe have a debug feature that spits out the request URL? This might help with debugging problems like this one. Just a thought.
Comment From: rwinch
The complete stack trace is in the original comment above.
Thanks. Sorry I had missed that.
Based on the stacktrace this his happening at the time of authentication and so Spring Session should not impact this.
What would be "nice to have" is if any Exception thrown from a PasswordEncoder Spring Security logs the the login that threw the Exception and maybe have a debug feature that spits out the request URL?
This is really outside of the scope of Spring Security as it is more about error reporting and is applicable to any error you might have. Typically you can configure your logger to include things like the request URL. For example, you can configure logback's MDCInsertingServletFilter to populate the MDC and then configure the logformat to include things like the requestURI. For example, the following would include the request URI in the log statement:
%X{req.requestURI}%n%d - %m%n
Comment From: spring-projects-issues
If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.
Comment From: spring-projects-issues
Closing due to lack of requested feedback. If you would like us to look at this issue, please provide the requested information and we will re-open the issue.