knastase
commented
9 years ago Wow! You guys have really been working on this! I appreciate all the contributions, I'm not a developer by trade, and I could use all the help I can get. This started as more of a proof of concept to see if it could actually be done in SimpleSAMLphp, and once the basic functionality was there I figured others could improve on other aspects such as security and usability. Duo makes a great product and I figured since they had a Shibboleth integration, why not SimpleSAMLphp?
Thank you for not only telling me why you made a change but also linking to the reference. Being new to the open source community, I was very excited to see someone else contributing (my first fork actually). I'm glad you found my module useful!
erikbarbara
Hey again!
@erikbarbara and I have been using and iterating on your module. First of all, thanks for creating it, we've found it very useful! However, we've found a few issues, some security related, some not, some more serious than others.
The construct() function is required to call its parent's construct() function (source: https://simplesamlphp.org/docs/stable/simplesamlphp-authproc#section_3)
Some of the comparisons in the duo_web.php file are not as strict as they could be (as per https://www.owasp.org/index.php/PHP_Security_Cheat_Sheet#Weak_typing). This one was totally our fault, and the duo_php repository has been updated accordingly.
Some of the HTML escapes are not as strict as they could be (as per https://www.owasp.org/index.php/PHP_Security_Cheat_Sheet#No_Tags). So we added the xssafe() function.
The biggest security concern was that the Duo response wasn't actually being verified in www/getduo.php. This means that a malicious server could post any sig_response to the module and it would consider it valid, effectively a 2-factor bypass.
Finally, the username wasn't being verified after successful response verification. Not as critical as the above bug, but still something that should definitely be done.
We also changed the iframe to our new responsive iframe feature which plays better with different sized screens (i.e. mobile).
I hope this pull request was useful, and thanks again for using Duo!