6

I've recently been implementing some security improvements in one of my employer's Spring-based Java applications, and I've overridden Spring Security's AbstractUserDetailsAuthenticationProvider class in order to do some extra processing around user authentication. During this process I realised that the DefaultPreAuthenticationChecks inner class performs user account checks prior to the authentication provider running through the additionalAuthenticationChecks method which does the validating of the password. If a user is disabled, expired or locked, an exception will be thrown, and thus the relevant messages will be displayed on the screen. To me, checking a user account and providing details of this account prior to successfully validating the password is a blatant security risk, as it could expose whether a user account exists or not. Does anyone know a good reason why Spring Security may have done things this way? Obviously I can just override the DefaultPreAuthenticationChecks class by creating my own dummy class with a check method that does nothing, but it's a shame that this has to be done in the first place.

Thanks in advance.

P.S. I found a question on a related note here, but nobody seemed to ask the question as to why this potential security flaw exists.

Community
  • 1
  • 1
Jon Betts
  • 234
  • 3
  • 16
  • Could be a performance thing. If user is disabled, then don't do expensive bcrypt operation? – Neil McGuigan Feb 03 '16 at 19:07
  • That's a good point, but I would rather have that check done first so we can ensure that the person trying to access the account at least knows the password. Then we're safe in the knowledge that it is likely an authorised account holder, so we can gladly tell them that the account is unfortunately locked or disabled. I guess it's a choice of speed over extra security and processing. I'd choose the latter every time when it comes to authentication! – Jon Betts Feb 04 '16 at 16:19

2 Answers2

1

I guess I'm a little bit late to the party, but in case anyone is still wondering, the community has actually discussed this issue before

quoted from the developer

Luke Taylor said:

This isn’t the case. It’s up to you what failure message you show to the user when a login fails – there’s nothing to stop you saying “login failed” regardless of the cause. It’s also not uncommon for applications to inform a user that their account has been locked after a fixed number of login attempts. The desired behaviour will depend on exactly how you interpret the different status flags (which is not strictly defined by the framework). The exceptions also drive the generation of events by the AuthenticationManager, so they are not necessarily there for user consumption at all. A sysadmin may want to be informed anytime someone is trying to use a locked or disabled account, for example, not just when they use the correct password.

It can also be argued that checking the password before the account locked status and showing a “locked” message only when a correct password is given would also allow brute-force password checking even after the account has been locked.

So I disagree that the behaviour is “incorrect”. I think it might be useful to be able to customize things though. Perhaps we should have methods

preAuthenticationChecks(UserDetails user)

postAuthentication(UserDetails user)

which could be overridden to alter when the differed flags are checked.

tl;dr: The community was aware of this issue, but they don't think it's a potential security flaw, instead they think it depends on "how you interpret the different status flags", and yes, it's easy to change the default behaviour using setPostAuthenticationChecks(UserDetailsChecker postAuthenticationChecks) and setPreAuthenticationChecks(UserDetailsChecker preAuthenticationChecks)

edward
  • 101
  • 2
  • 3
1

It definitely seems to be for performance optimization. In addition, it is more secure to check for disabled or locked accounts before checking for correct password, rather than vice versa.

As Neil mentioned, if you do other checks before, then expensive operations like bcrypt can be avoided.

So let's consider the second point i.e. security concern. Consider your application first checks for password and if password is valid it performs other checks like account is locked/disabled etc. It won't take much long for a potential hacker to understand this behavior and applying brute force to crack user's password because he is aware that account locked/disabled message will be displayed only in case of correct password.

Now coming back to your concern that with this approach there is a risk of exposing whether user account exists or not. Firstly, I believe that risk of cracking passwords outweighs risk of exposing that account exists or not. Secondly, an application can catch the exceptions thrown by these checks (locked/disabled) and provide a custom message. E.g.

catch(LockedException e){
  // log actual reason, so that it could be used for debugging purpose
  return "Invalid credentials"; // or throw BadCrentials exception
}

More details, here: https://github.com/spring-projects/spring-security/issues/798

tryingToLearn
  • 10,691
  • 12
  • 80
  • 114