Resolved Time Difference Between Valid and Invalid Users Trying To Log In#1599
Resolved Time Difference Between Valid and Invalid Users Trying To Log In#1599Andrew-Bonner wants to merge 2 commits into
Conversation
…lled on a null users to eliminate time difference
huss
left a comment
huss
left a comment
There was a problem hiding this comment.
@Andrew-Bonner Thank you for working on this. I've made a couple of comments to consider. Please let me know if anything is not clear or you have thoughts.
| @@ -48,6 +48,7 @@ router.post('/', credentialsRequestValidationMiddleware, async (req, res) => { | |||
| let isValid; | |||
| if (user === null) { | |||
| // User did not exist so return false. | |||
There was a problem hiding this comment.
This comment does not directly apply to the next line but the one that follows. Also, since doing the bcrypt in this case was for timing reasons, I think a very clear comment on why this was done should be here. So, can you please clean this up so it is clear.
There was a problem hiding this comment.
Thank you for adding the new comment. I think the old/removed comment should be put back above the new one since it applies to this case in the if statement. A blank line between them would make it clear that it is separate.
| let isValid; | ||
| if (user === null) { | ||
| // User did not exist so return false. | ||
| isValid = await bcrypt.compare(req.body.password, user.passwordHash); |
There was a problem hiding this comment.
I noticed that this accesses user when it is null. Sure enough, when I try an invalid login it shows this error in the terminal where OED was started:
web-1 | [ERROR@2026-04-04T20:29:57.805+00:00] Unable to check user password for bad
web-1 | Stacktrace:
web-1 | TypeError: Cannot read properties of null (reading 'passwordHash')
web-1 | at /usr/src/app/src/server/routes/login.js:51:60
web-1 | at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
Since the user does not exist, how about using req.body.password which will match but then set the value to false. It does require setting the variable and not in the case where the user existed but you already did this and I would hope the timing difference is so small that someone could not exploit this. I thought of other ideas but didn't like them as much. I'm open to alternatives but the issue needs to be addressed some way.
There was a problem hiding this comment.
I know you marked this resolved but I'm unclear on how it addressed the comment so I have unresolved it. The code is in the if because user is null. The the code de-references user to get the passwordHash. I don't think it is okay to do that since you are de-referencing null and it has no attributes. Trying to login via the web page with an unknown user give this error in the terminal starting OED:
web-1 | TypeError: Cannot read properties of null (reading 'passwordHash')
web-1 | at /usr/src/app/src/server/routes/login.js:51:50
web-1 | at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
which is consistent with what I think should happen. Please address this issue and also try to run the code through different cases to make sure it works properly.
You also told me in a meeting that you had timed the code to see it took the same amount of time. Seeing it now I'm a little surprised given this case generates an error. Putting that aside, I think any timing tests need to be redone once the code is finalized since it will act differently.
Just for future information, please do not resolve any comments but put in a reply so I can still easily see the comment chain. It is fine to say your fixed it or add a thumbs up. That way I know anything resolved was by me. Thanks.
…etting the isValid value to false. That way, this function is not being assigned to a value and is just running.
huss
left a comment
huss
left a comment
There was a problem hiding this comment.
@Andrew-Bonner Thank you for the update. I made one new comment and unresolved the two old ones with new replies. Please consider them and let me know if anything is not clear or you have questions/thoughts.
| let isValid; | ||
| if (user === null) { | ||
| // User did not exist so return false. | ||
| // call the bcrypt.compare() without assigning it valid user to eliminate time differation |
| let isValid; | ||
| if (user === null) { | ||
| // User did not exist so return false. | ||
| isValid = await bcrypt.compare(req.body.password, user.passwordHash); |
There was a problem hiding this comment.
I know you marked this resolved but I'm unclear on how it addressed the comment so I have unresolved it. The code is in the if because user is null. The the code de-references user to get the passwordHash. I don't think it is okay to do that since you are de-referencing null and it has no attributes. Trying to login via the web page with an unknown user give this error in the terminal starting OED:
web-1 | TypeError: Cannot read properties of null (reading 'passwordHash')
web-1 | at /usr/src/app/src/server/routes/login.js:51:50
web-1 | at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
which is consistent with what I think should happen. Please address this issue and also try to run the code through different cases to make sure it works properly.
You also told me in a meeting that you had timed the code to see it took the same amount of time. Seeing it now I'm a little surprised given this case generates an error. Putting that aside, I think any timing tests need to be redone once the code is finalized since it will act differently.
Just for future information, please do not resolve any comments but put in a reply so I can still easily see the comment chain. It is fine to say your fixed it or add a thumbs up. That way I know anything resolved was by me. Thanks.
| @@ -48,6 +48,7 @@ router.post('/', credentialsRequestValidationMiddleware, async (req, res) => { | |||
| let isValid; | |||
| if (user === null) { | |||
| // User did not exist so return false. | |||
There was a problem hiding this comment.
Thank you for adding the new comment. I think the old/removed comment should be put back above the new one since it applies to this case in the if statement. A blank line between them would make it clear that it is separate.
2 participants
Description
This change ensures that bcrypt.compare() is invoked for both valid and invalid login attempts. Previously, authentication requests for non-existent users returned faster because the comparison function was not executed, while valid users incurred additional processing time due to the bcrypt.compare() call. This discrepancy created a time difference that could allow attackers to enumerate valid users. By consistently calling bcrypt.compare() in all cases, the response time is normalized, mitigating the risk of user enumeration.
(In general, OED likes to have at least one issue associated with each pull request. Replace [issue] with the OED GitHub issue number. In the preview you will see an issue description if you hover over that number. You can create one yourself before doing this pull request. This is where details are normally given on what is being addressed. Note you should not use the word "Fixes" if it does not completely address the issue since the issue would automatically be closed on merging the pull request. In that case use "Partly Addresses #[issue].)
Type of change
(Check the ones that apply by placing an "x" instead of the space in the [ ] so it becomes [x])
Checklist
(Note what you have done by placing an "x" instead of the space in the [ ] so it becomes [x]. It is hoped you do all of them.)
Limitations
(Describe any issues that remain or work that should still be done.)