Update default OTP Window for Google TOTP to 1 per recommendation in RFC 6238

davidalger commented 4 years ago

Description (*)

With the OTP Window being configurable, the description needs to describe what this number does. This default number of 30 equals a 15 minute validity period, 30 second time-step X otp_window of 30 == 900 seconds.

There is an added question here of whether a default value of 30 here even makes sense. I would tend to think it's way too high. TOTP codes are generally supposed to be valid for the time-step period, give or take a few seconds to account for different system times, but being able to use a code I generated 15 minutes ago (or 30 time-step periods ago) seems very excessive. I'll be happy to open a PR for that as well, or amend this one, if others agree here.

Fixed Issues (if relevant)

magento/magento2#: Issue title

Manual testing scenarios (*)

...
...

Questions or comments

Contribution checklist (*)

[x] Author has signed the Adobe CLA
[x] Pull request has a meaningful description of its purpose
[x] All commits are accompanied by meaningful commit messages
[x] All new or changed code is covered with unit/integration tests (if applicable)
[ ] All automated tests passed successfully (all builds are green)

nathanjosiah commented 4 years ago

Hey @davidalger! Thanks for opening this. I think that you are correct that the window is too big. It looks like this was missed when two different approaches to the automation support were merged during our internal development. The intention was that the otp's were only valid for 30 seconds by default (as is the standard) and for automation (or whatever else) this could be increased to prevent issues with immediately expiring tokens. I think it makes sense to change that in this PR as well. Can you confirm that changing the default to 1 and automation values to 2 runs correctly?

davidalger commented 4 years ago

Can you confirm that changing the default to 1 and automation values to 2 runs correctly?

@nathanjosiah I will see if I can give this a run through with those settings and followup.

On a related note, I ran a quick poll on this to see what some of my peers thought and just about every respondent agreed this should be very small window. Shortly thereafter I also found the RFC for TOTP which confirms that's what's typically expected. Seems everybody is on the same page, that the window should definitely be very small so we don't have codes valid for a long period of time.

The receiving time at the validation system and the actual OTP generation may not fall within the same time-step window that produced the same OTP. When an OTP is generated at the end of a time-step window, the receiving time most likely falls into the next time-step window. A validation system SHOULD typically set a policy for an acceptable OTP transmission delay window for validation. The validation system should compare OTPs not only with the receiving timestamp but also the past timestamps that are within the transmission delay. A larger acceptable delay window would expose a larger window for attacks. We RECOMMEND that at most one time step is allowed as the network delay. Ref: https://tools.ietf.org/html/rfc6238#section-5.2

I'm curious if you have context on why the OTP Window setting was added, as the previous 2FA implementation did not specify an OTP Window when it called OTPHP\TOTP::verify to validate against the current timestamp only. Was it strictly due to testing or were there complaints regarding the lack of a window being used providing no "grace" if the code had just expired when submitted? Largely I'm wondering if it might be good to simply remove the field in the admin for the OTP Window. I.e. leave the setting there, but don't provide the means for an admin user to change it, leaving it to be something which would have to be controlled on the CLI via a config:set since this is likely not something which should be often changed.

Reviewing the implementation of OTPHP\TOTP::verify, when a window of 1 is set, a code one time-step in the past and 1 time step in the future will be counted as valid, meaning 3 possible values are valid at any given point in time. Raise that window to '2' and you have 5 valid values, 2 in the past, the current value, and 2 in the future. So a value of 1 for the OTP Window definitely feels like a sane default, as it should allow for the system clock on user systems to be off by up to 30 seconds without causing the token generated by their device to be recognized as invalid. This value of 1 as a default would also comply with the recommendation in the RFC noted above.

nathanjosiah commented 4 years ago

@davidalger

I'm curious if you have context on why the OTP Window setting was added, as the previous 2FA implementation did not specify an OTP Window

Yes, it was added to support automation but it is also not uncommon to have this be adjusted. It's very common (even for tech giants like google/github) to allow more than one window to prevent the user from experiencing the same race conditions that automation has. That is, because of how the TOTP spec is designed, it's possible that a given token will only be valid for a few seconds which would cause it to expire before the server processes it.

but don't provide the means for an admin user to change it, leaving it to be something which would have to be controlled on the CLI via a config:set since this is likely not something which should be often changed.

Maybe, but system config isn't just for things that are often changed. There are plenty of settings that are only ever set once and never touched again.

So a value of 1 for the OTP Window definitely feels like a sane default ...

Again, the purpose of this was to prevent instantly expiring tokens as described above. The default value only allows the current token. I think a reasonable default is current+last valid tokens as well as better description of the token.

davidalger commented 4 years ago

So a value of 1 for the OTP Window definitely feels like a sane default ...
Again, the purpose of this was to prevent instantly expiring tokens as described above. The default value only allows the current token. I think a reasonable default is current+last valid tokens as well as better description of the token.

Absolutely. I've updated the PR to change the default OTP Window to 1 and have tested this on a local installation. The default value of 1 accomplishes just this, the current code plus one in the past and one in the future are considered valid. We can probably keep the settings in tests as-is without any unfortunate consequences.

davidalger commented 4 years ago

@magento run all tests

davidalger commented 4 years ago

@nathanjosiah Any chance this will make it into the pipeline for 2.4.1? I was really hoping that reporting this early in the beta period Magento would have pulled a security related fix like this into the initial 2.4 release. The window is far too wide as it stands currently.

nathanjosiah commented 4 years ago

@magento run all tests

smiverma commented 4 years ago

Changes Approved

nathanjosiah commented 4 years ago

@magento run Functional Tests CE,Functional Tests B2B

nathanjosiah commented 4 years ago

@davidalger Merged! Thank you!

davidalger commented 4 years ago

Thanks @nathanjosiah for helping getting this moved to a good working solution. Appreciate it!

magento / security-package