Add delay when accessing unauthorized ReST endpoints #83
Conversation
| @@ -50,7 +50,9 @@ Meteor.startup -> | |||
|
|
|||
| next() | |||
|
|
|||
| it 'should not allow a user with wrong password to login', (test, next) -> | |||
| it 'should not allow a user with wrong password to login and should respond after 500 msec', (test, next) -> | |||
There was a problem hiding this comment.
I combined the two tests into one primarily because the failed response now takes at least 0.5 seconds. If I test the delay separately, we will add another second, which wasn't really necessary...
There was a problem hiding this comment.
Sounds like solid reasoning. I would just move the comment below right above this and change it to:
# This test should take 500 msec or more. To speed up testing, these two tests have been combined.Don't use "I" in any comments. It doesn't help anyone reading it, since they have no idea who "I" is.
There was a problem hiding this comment.
Sounds good. Updated.
|
I think we should add this quick-fix in for now, and convert it to middleware after the v0.8.0 release. We can include this in the 0.7.0 release. The 0.7.0 release will be a mini auth update. Do you have any other quick auth updates you'd like to include before I publish it? It's best if we leave as much until after the 0.8.0 release as possible, since the middleware makes for a much cleaner, reusable pattern. I'll review this now. |
|
I do not have any other updates.
|
| # This is a simple, but not really adequate way to slow down unauthorized scans of the server. | ||
| # It is not sufficient because hackers can DDOS across multiple threads and systems. | ||
| timeoutInMilliseconds = 500 | ||
| timeoutInMilliseconds *= (1 + Math.random()) |
There was a problem hiding this comment.
I think we should change this to
randomMultiplierUpTo2X = 1 + Math.random()
delayInMilliseconds = 500 * randomMultiplierUpTo2XI'm not in love with my variable name, but it gets rid of the magic number, and makes life a little easier for the math-challenged devs :) Also, I prefer "delay" over "timeout" for the other variable name.
There was a problem hiding this comment.
How about randomMultiplierBetweenOneAndTwo.
Agreed with delay vs timeout.
|
I left a few comments, but I've run out of time for now and will need to finish the last of the review tonight. |
|
I tend to agree, 403 is for permission errors. In either case, 401 or 403, we should delay, so I suspect my original code is still ok. |
jazeee
force-pushed
the
fix-issue-81-2
branch
2 times, most recently
from
289265a
to
bc0db9f
Compare
| # caused by bad user id vs bad password. | ||
| # In doing so, they can first scan for valid user ids regardless of valid passwords. | ||
| # Delay by a random amount to reduce the ability for a hacker to determine the response time. | ||
| # See https://en.wikipedia.org/wiki/Timing_attack |
There was a problem hiding this comment.
Updated variable names. Added more comments to help understand why we do this.
Also added some links.
There was a problem hiding this comment.
I'm thinking we should only use the second comment. I'm open to debate here, but the first comment seems like it would be better suited if we added in a section to the docs about the current state of security in Restivus, and that could be included in the concerns that haven't been addressed. It doesn't really help to clarify the code any more than the second comment already does. It should also be moved to line 211 where the first comment already is (above this chunk of code).
|
Your commit message must be wrapped at 72 chars. Also, I would remove the last bullet point (it kind of contradicts the first), and move the first bullet point to the bottom (to get to the good stuff first :D). |
|
I left some minor comments, but other than those, LGTM. |
|
I will update the commit message to 72 chars. |
- When logging in or accessing a restricted ReST endpoint with invalid credentials, the system shall delay before responding. - Note that this is not a great fix, since hackers could distribute their requests across CPUs. - This is an initial temporary fix for kahmali#81
| # caused by bad user id vs bad password. | ||
| # In doing so, they can first scan for valid user ids regardless of valid passwords. | ||
| # Delay by a random amount to reduce the ability for a hacker to determine the response time. | ||
| # See https://www.owasp.org/index.php/Blocking_Brute_Force_Attacks#Finding_Other_Countermeasures |
There was a problem hiding this comment.
I kept both links since they are relevant.
|
I use a 100 char line limit in all my other files (and will update the markdown convention to use no line limit at all), so I tend to agree, but 72 chars is a widely adopted standard for git messages that I've come to accept. I suppose in future projects I may be convinced to extend the line limit, but for now it's just a convention to allow consistency when reading through the commit history. I've reviewed everything here and it looks great to me. I'll go ahead and pull this in. I just need to update the README with a warning about the token invalidation, and then I'll bump the version and publish. I'll keep you posted on that. Thanks for another solid contribution! |
Add delay when accessing unauthorized ReST endpoints
I figured I'd create this for #81, now that we are updated. I don't know what the long term solution is, but this may be a start...
the system shall delay before responding.
EDIT: We discussed using a middleware solution. Feel free to discard this pull request...