greenriver / hmis-warehouse

The Open Path HMIS Warehouse (Warehouse) project was initiated by the City of Boston's Department of Neighborhood Development office to coordinate data across various HMIS installations and supply de-duplicated client information to the Boston CAS (https://github.com/greenriver/boston-cas).

http://openpath.host

GNU General Public License v3.0

30 stars 14 forks source link

Add protection for time analysis on login attempts to ennumerate valid login usernames #4771

Closed dtgreiner closed 3 days ago

dtgreiner commented 3 days ago

Please squash merge this PR

Description

Enforce a minimum 2 second login time to protect against identifying valid usernames using login time analysis.

Type of change

[X] New feature (adds functionality)

Checklist before requesting review

[X] I have performed a self-review of my code
[X] I have run the code that is being changed under ideal conditions, and it doesn't fail
[X] My code includes comments and/or descriptive variable names to help other engineers understand the intent (or not applicable)
[X] My code follows the style guidelines of this project (rubocop)
[ ] I have updated the documentation (or not applicable)
[ ] If it's not obvious how to test this change, I have provided testing instructions in this PR or the related issue

ttoomey commented 3 days ago

wouldn't it be easier to just introduce a random wait on every login request? Something between 0 and 1.5 seconds?

If I'm understanding the problem correctly, I think Dave's approach should mask the timing issue as long as our auth is consistently less than 2 secs. If there's just a random wait time it seems like an attacker could still get useful timing information (although it would require a larger sample)

That reminds me, we might also want to check that wait_time is positive. Sleep on a negative number raises an exception and it looks like it might occur in the unlikely event that elapsed > 2.5

dtgreiner commented 3 days ago

The Time.now -> Time.current change and the wait_time.positive? change have been added to the PR