-
-
Notifications
You must be signed in to change notification settings - Fork 127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Security Risk: using QRServerProvider
as default provider
#104
Comments
Thanks for your concern, yes it would make sense to include some form of warning near the curl mention in readme.md as curl would also not be required with the more local providers. Feel free to PR some wording if you like or I'll add something later. I am unsure of the relationship, if any, between @RobThree and qrserver.com so couldn't really say how secure or not it would be or why it is the default QR provider, other than potentially being the most freely available at the time perhaps. As the QR code is generated by the server backend I would say there is no personally identifiable information as the request only includes the name of the issuer which is expected to be the company behind the code and not describe or identify the user in any way. You are right in saying that other entities should not be trusted with secrets unnecessarily but I would argue that the context that any QR generation URL receives will be minimal enough to not cause a problem for the casual user and more security conscious aware users would be more alarmed by something using curl directly (as opposed to something more mainstream like guzzle) so would seek to use one of the other providers. Changing a default value that relies on another dependency is likely to cause more irritation amongst users as they're suddenly compelled to add in an extra dependency they were not using which could have other ramifications for them. |
@willpower232
IF (and this if is of course the reason this isn't catastrophic) someone were to intercept requests to/at the QR api, they are able to completely bypass 2FA and possibly figure out usernames. Code: TwoFactorAuth/lib/TwoFactorAuth.php Lines 155 to 163 in 5968be2
|
Oh yeah sorry I missed that line and carefully read the others (its lunch time here 😅) If you were implementing a system with specific usernames that weren't email addresses and/or didn't make it easy to find out valid usernames with a password reset system then yes, you'd probably be annoyed that your 2fa implementation was leaking information by default but you would probably have also done more research as I mentioned and changed up the providers. Not saying it shouldn't be changed, just that it could be annoying for many users. Potentially just easier to throw exceptions if a QR provider isn't specified I guess... |
I remember voicing a similar concern during the php8 refactor, and suggesting that external providers could be removed as it seems a big issue for me that mfa secrets can be sent to a third party. BUT, as Will said, it's a balance between ease of use, history and general design. Given that the library allows using a different QR provider (I'm using the one provided by Mpdf for instance), it is not a security issue if your application uses a local qr provider. I'd vote for completely removing any external provider, but an argument can be made that they don't hurt anyone. Still, having a local provider by default would be better, but as it was mentioned, this would mean adding a required dependency (otherwise the default won't work out of the box). I for one don't want to have to pull a dependency that I will not use in fine. The best option would be to have a local qr provider integrated fully in the lib (even if super basic). But that would mean adding unnecessary code because other libs do it well already, and I'm quite happy with the current design where this lib only does what it is supposed to do, and not more (of course, one can argue qr code generation is part of what a 2fa lib should do...). So all and all, the current state is fine, and documentation could be improved to make it clear that due to the sensible nature of the mfa secret, using a third party should be carefully considered, and that the default provider uses a third party. |
There's no relationship; you were correct in assuming it being the most freely available at the time. Another reason is / was that (the then-current) libraries all relied on PHP modules (or whatever they're called) being installed like, I don't recall exactly, ImageMagick or GD which (at least back then) wasn't very common (especially in shared hosting environments). To avoid issues with needing a grab-bag of modules being installed it was much easier to 'offload' image generation to an external service. Yes, that is unsafe(r) than doing it locally. Yes, I did consider that at the time. Yes, I weighed the pro's and cons back then and decided to use QRServerProvider. Was it the best decision in hindsight? Maybe not. Is it a 'security risk'? Meh. You're (still) free to use a different provider. Also, by the time you can intercept the requests for QR codes there's a high likelyhood that the server is compromised anyway; though intercepting messages would be possible on both ends and on every hop in-between, requests are HTTPS. Good luck with that. Yes, when the 3rd party is compromised then you have a situation. But then, still, the 2FA codes are part of ... a two-factor authentication process. Yes, leaked 2FA secrets is bad, but you still need the user's password. So you still need to also get to that. I'm not saying it's impossible, it sure isn't, a simple phishing attack will probably suffice; I'm just saying that while not impossible, it certainly also isn't "easy" by any means. However, I do think we can at least consider the following:
I agree. I even made an attempt a few years ago but eventually gave up because it was too much work, especially since there were a bunch of other providers (external or local) available that worked perfectly fine. I decided not to reinvent the wheel and aborted that project. |
Sounds great 👍
Some documentation could be added for the requirements and benefits of different providers, and users can choose the best tool for the job. This would mean no new required dependencies would have to be added and users get as much flexibility as before. |
I suppose that adding a clear point to the documentation does inform it's users about the potential security risk associated with using the package. In the future switching to a locally generating QR code provider should no longer be a concern because of the higher availability of the ImageMagick or GD extensions. That being said, I strongly recommend removing the external providers from the package altogether. A package that handles 2FA should not send data to an external provider. This would not be a concern if the package only handled QR codes. |
For 3.x I'd like to split the providers into their own packages (e.g. |
Sounds good to me 👍 |
That looks like it's the best way forward! Any idea when the 3.x release is coming? |
Good decision IMHO. Splitting providers in their own package is also a good idea. Bonus points if you can still do that from the same repo! (having to maintain multiple repos is a PITA). |
So, here we are again. For 3.0 I would like to get rid of this warning. I'm not sure if we ("already") should create separate packages (maybe?) but at least change the default would be a good idea I guess. What's your opinion on this @willpower232 and @NicolasCARPi ? |
My opinion is the same as: #104 (comment) What I can suggest today:
Things to be careful about:
|
Forcing the developer to specify a provider makes the most sense to me, it gives a chance for the changelog to promote awareness of why using the remote provider might not be the most desired option. It might even be that many people aren't using the QR part at all so don't notice the change. The provider definitely works well as a proof of concept, people can get working with only one |
This change is discussed in #104 Currently, the library defaults to a QR Code Provider using an external service, thus leaking secrets. This change forces the definition of a QR Code Provider in the constructor. It is a breaking change. fixes #104 The public function getQRCodeProvider() has been removed. It is provided by the user in the constructor, so it doesn't make a lot of sense to keep a getter around if we're not using it internally.
I strongly agree with the idea of not defaulting to the qrserver api. |
Security Risk: using
QRServerProvider
as default provider:As you may already know, by default, this package sends users' two-factor authentication secrets to api.qrserver.com for QR code generation. This behavior has raised some concerns among our organization's security team, as it poses a security risk.
Our team strongly believes that two-factor authentication is an essential security feature for applications. We would like to request that you change the default behavior of this package to not send users' secrets to a third party for QR code generation.
Instead, we suggest that you implement the
EndroidQRProvider
as a default, this enables the package to generate the QR-codes locally, without the need to send their secrets to a third party. This change would greatly improve the security of consumers of this package, and we believe it would be a valuable addition to this package.Until this has been addressed, we suggest that you clearly state this issue in the README.md So that users using this package will be aware of the security risk.
Mitigation for users:
Composer require endroid/qr-code
EndroidQRProvider
in the constructor ofTwoFactorAuth
Example:
Thank you for your attention to this matter. We look forward to hearing back from you soon.
With kind regards,
The Scienta team
The text was updated successfully, but these errors were encountered: