Closed crackedeggs1 closed 8 years ago
Thanks for sharing this vulnerability.
I think I should simply remove the DEBUG block which wasn't meant to stay in the release code. It was just for me during early stages of development, but should not remain in the final file.
I found the debug block to be useful in identifying conflicts with my nginx config.
fair enough. That's also how I used it in the initial setup. It shouldn't be too hard to implement given your input. Even better, if you don't mind sharing your whole debug bloc, I'll be glad to add it (unless you want to do it yourself, after all it's GitHub, though I'm not too much into how to use 'git' and accept merge requests, but that might be a start for me to learn this.)
Okay, I'll do it. There are a few other vulnerabilities I found, so I will do that today. Using one of them it's possible for a remote server to collect someone's one-time token and login before they do.
Do you have a changelog bundled?
Resolved. See pull request: https://github.com/Arno0x/TwoFactorAuth/pull/2
When the debug.log is activated in /nginx/auth.php by uncommenting the debug code in that file and if php.ini has display_errors turned on, then every sub-request to this file will authenticate regardless if the user is actually authorized and logged in.
The culprit is the line fwrite ($debugHandle,$key.": ".$value."\n");
This throws an "Array to string conversion" error when it hits the $_SERVER keys 'argv' and 'argc', which are arrays. Once the error is thrown, the script output is started, so you cannot successfully start a session or send the 401 response header.
It is impossible to guarantee that these keys themselves do not contain arrays. If you wish to print them to the log, then you must walk through $value recursively, checking is_array($value) in each new recursion.
I didn't care about printing deep argv/c to my debug log, so placing the following before the line fixed the issue for me: if (is_array($value)) { $vs = array();
For others it might be important and for that you would need to actually make a recursive function. Although fixing bugs is always better, alternatively you may wish to force PHP's display_errors off at the top of nginx/auth.php so that other unexpected bugs don't bypass authentication either.