speakeasyjs / speakeasy

**NOT MAINTAINED** Two-factor authentication for Node.js. One-time passcode generator (HOTP/TOTP) with support for Google Authenticator.
MIT License
2.7k stars 228 forks source link

Security: brute force possible when badly implemented (please improve documentation to help implementers) #75

Open carbonphyber opened 7 years ago

carbonphyber commented 7 years ago

I love the library and it has been very helpful for me.

Recently, my implementation of speakeasy.totp failed a penetration test. I wrote a writeup on my findings (with a code sample to show how common this can happen with a bad implementation+configuration).

The issue: please improve the documentation (especially around passing in "secret" and "window" parameters). If the window parameter is too large, "false positives" (successful attacks against the TOTP) become more frequent. If the "secret" passed to the speakeasy.totp.verify function is an empty string (not undefined or null), the verify function becomes much easier to attack via brute force.

I don't think any code changes are necessarily needed (an empty string "secret" might possibly be a valid input for some use cases), but updates to the docs would be very welcome, especially to those of us who only know enough about encryption "to be dangerous".

markbao commented 7 years ago

@carbonphyber Thank you for taking the time to do the testing and writeup as well as your suggestions for improvement. I think improving the documentation in the areas that you suggest is an excellent idea. I'll do some thinking about how to communicate it in the documentation and will hope to make these changes along with our next version.