kalaspuffar / secure-quick-reliable-login

This repository is an implementation for SQRL (Secure Quick Reliable Login) on Android.
MIT License
115 stars 29 forks source link

Mitigation for QR code decoding issue #485

Closed alexhauser closed 3 years ago

alexhauser commented 3 years ago

This proposes a temporary mitigation for issue #481 and should form the base for a discussion on the topic.

Description:

It turns out that our method of reading and concatinating the SCAN_RESULT_BYTE_SEGMENTS_{x} within Utils.readSQRLQRCode(...) would sometimes leave us with chunks of the SQRL login URL missing, as layed out in issue #481. It seems as those chunks are simply not present in the byte segments contained in the intent that gets passed in to onActivityResult(...). To me, this looks like a bug in either the zxing-android-embedded library that we're using, or even in the underlying zxing:core library.

That being said, using IntentIntegrator.parseActivityResult(...).getContents(), which returns the decoded QR code as a string, seems to be delivering correct results for non-binary inputs (which all login URLs should be).

This however does not work for identity QR codes, since they consist of binary data. This seems to be a problem in its own right, given the discussion found here. So not only do we have the uncertainty of missing byte segements, but also that binary data doesn't seem to be supported anyway. So after all, we might have only gotten lucky with our limited testing of importing identity QR codes up to now.

@kalaspuffar, I'd appreciate a short discussion on the topic before we jump to any conclusions.

What this PR proposes (as a temporary mitigation):

This now uses IntentIntegrator.parseActivityResult(...).getContents() for login QR codes and only falls back to reading the binary SCAN_RESULT_BYTE_SEGMENTS_{x} when dealing with identity QR codes. This should at least make the decoding of login QR codes much more robust.

kalaspuffar commented 3 years ago

Hi @alexhauser

I'm open to a discussion in any form you like around this issue. It seems pretty damming for this library. When I looked into this and created the first implementation of SQRL Login, there were no other libraries available. Perhaps we should do another take on the landscape.

I can see at least 3 different solutions:

Best regards Daniel

alexhauser commented 3 years ago

Right, @kalaspuffar.

I would suggest to merge this fix for now as a mitigation for #481 and then think about how to approach a general solution for this topic.

So, if you or maybe @JamesonNetworks could test this first as well, that would be great!

alexhauser commented 3 years ago

Closing this in favor of #489.