Journalist interface logins with invalid usernames are not throttled

[emkll]

Description

Invalid (nonexistent) journalist accounts login attempts are not throttled. This may allow an attacker with ATHS credentials to potentially enumerate journalist usernames on the journalist interface.

Steps to Reproduce

• go to journalist interface
• attempt to log in with valid journalist username 6 times
• observe that the 6th login is throttled
• attempt to log in with invalid journalist username 6 times
• observe that the 6th login is not throttled

Expected Behavior

Invalid usernames should exhibit the same behavior as valid usernames, otherwise would allow an attacker to distinguish between journalist and non-journalist accounts and potentially enumerate journalist accounts.

Actual Behavior

Invalid usernames are not throttled.

Comments

Because the journalist interface requires an ATHS token to login, and this would, in the worst case, disclose usernames, the risk is quite low. More discussion (including a regression test) can be found here: #3564

[deepthinidwannaya]

I'll try to pick this one.

[deepthinidwannaya]

So, currently, failed logins are tracked in "journalist_login_attempt" only for valid users.
Throttling is done only if a "valid" user exceeds 5 login attempts.
So, there are two ways to go about this

1. We throttle for every invalid user
2. We start logging invalid login attempts in the table and then throttle them when it fails more than 6 times.

Could I get suggestions on which of the two sounds reasonable or share any other feedback?

[heartsucker]

Because of the foreign keys on that table, it would not be possible to track log in attempts that are not associated with someone in the journalist table in the journalist_login_attempt table. Any tracking we do would need to persisted somewhere and not in memory to allow the multiple workers behind Apache to share state on failed login attempts.

A possible mitigation would be to change the logic of the error message to be something more vague to prevent attackers from realizing they have gotten a correct user name. A secondary consideration to strengthen this would be to always run the password hashing functions to increase the difficulty timing attacks (though this itself may be exploitable).

However, the above notes would degrade the user experience on the web. That said, once we move to the Journalist Workstation, some credentials (username) may be cached locally so that a user only has to type in their passphrase/OTP token. This would mean a user would only get un-helpful error messages on the set of first login attempts on their workstation, and then from there we could infer it was only their passphrase/OTP token that was incorrect.

(This isn't terribly helpful at the moment, I know, but we could discuss it during standup today.)

[deepthinidwannaya]

@heartsucker Thank you for the comment!
Currently, we display the message 'Login failed' without letting the user know if its due to bad username or password.
So, if we go with option 1 where we throttle for every invalid user, the error message would be "Please wait at least {seconds} second before logging in again"
(I read your comment this morning- so couldn't join the standup. And I won't be able to join today either as I have an early morning flight to catch. So, we can discuss this in tomorrow's standup)

[deepthinidwannaya]

Summarizing the fix after discussion -
We're planning to drop the foreign key constraint from the journalistLoginAttempt table and start keeping track of invalid users and throttle when they exceed 6 failed attempts within a period

[redshiftzero]

We're planning to drop the foreign key constraint from the journalistLoginAttempt table and start keeping track of invalid users and throttle when they exceed 6 failed attempts within a period

👍 note that we'll have to also move up the login throttling in Journalist.login, i.e. if the user doesn't exist, then a InvalidUsernameException should only be raised after the JournalistLoginAttempt has been added to the database

[deepthinidwannaya]

Adding on to this - We're planning to drop the "id" column in JournalistLoginAttempt and add a user name column instead and there'll be no reference to journalists table.

[ultimatecoder]

@deepthinidwannaya Are you still working on this issue? Please write back if you are stuck and need any help. If not, I am happy to pick this. Thanks!