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

QR code colon encoding/decoding broken #19

Closed
neilstephens opened this Issue Jun 15, 2016 · 3 comments

Comments

Projects
None yet
2 participants
@neilstephens

neilstephens commented Jun 15, 2016

Gday,

Wasn't sure to post this on the windows or android repo...

I have 1.3.2 for android (from the play store) and 1.3.3 (source from the cryptnos website) for windows (but using mono under Ubuntu 14.4)

Basically, colons in the site string prevent the qr code import/export from working because they're mistaken as a delimiter, and/or not escaped.

I would think this is particularly important because it wouldn't be uncommon for URLs to be used as site strings.

Cheers,
Neil.

@gpfjeff

This comment has been minimized.

Show comment
Hide comment
@gpfjeff

gpfjeff Jun 20, 2016

Owner

It may end up affecting both the Android build and the Windows build, since both have to treat colons the same way in order to pass data back and forth. As outlined below, however, I think I can work around having to change the Window client, since it only encodes QR data and doesn't have to parse it.

It took a recent data-breach-enforced password reset to see what's happening. (Hey, it's been a while since I looked at this.) The QR code is simply an encoded plain-text string that is similar to URL-encoding, but with different delimiters. Key/value pairs are delimited with pipes (|) while keys and values are delimited with colons. In retrospect, I don't know that the "keys" are really necessary since they're just single-letter "headers" and everything is processed sequentially, so the order matters, but it is what it is.

The problem looks to be here; it's splitting the "site token" key/value pair on colons and, if it gets more than two strings in the resulting array, it throws an error. I think it should be relatively safe to add another line or two here that essentially says, "if you get more than two strings after splitting on colons, combine everything after the first entry back into a single string with colons between the parts". That should allow colons to fit into the site token without letting it bomb, while still allowing the first colon in the key/value string to act as the delimiter. None of the rest of the values should allow colons, so I don't think this will be a problem with any other parameter field except for the site token.

It does, however, raise an interesting corollary question about pipes; would they cause the same problem? Pipes aren't allowed in URLs according to RFC 3986, but that doesn't necessarily stop someone from using them in a Cryptnos site token. I had to look this up myself to remember, but the good news is that pipes are not allowed in site tokens because they're used for internal delimiter purposes.

So if I can add the extra step to recombine the rest of the site string if colons are encountered, that should allow URLs to be used for site tokens, and I'd only have to update the Android client, since it's the only one that (for now) parses the QR output.

Thanks for the report, Neil. I'm not sure when I'll get a chance to fix this, but I'll hop on it as soon as I can. It's been long enough since I touched the Android client that I'm going to have trouble getting the old Android build system to work again. (I haven't had a chance to upgrade to the newer Android Studio platform yet.) Cross your fingers...

Owner

gpfjeff commented Jun 20, 2016

It may end up affecting both the Android build and the Windows build, since both have to treat colons the same way in order to pass data back and forth. As outlined below, however, I think I can work around having to change the Window client, since it only encodes QR data and doesn't have to parse it.

It took a recent data-breach-enforced password reset to see what's happening. (Hey, it's been a while since I looked at this.) The QR code is simply an encoded plain-text string that is similar to URL-encoding, but with different delimiters. Key/value pairs are delimited with pipes (|) while keys and values are delimited with colons. In retrospect, I don't know that the "keys" are really necessary since they're just single-letter "headers" and everything is processed sequentially, so the order matters, but it is what it is.

The problem looks to be here; it's splitting the "site token" key/value pair on colons and, if it gets more than two strings in the resulting array, it throws an error. I think it should be relatively safe to add another line or two here that essentially says, "if you get more than two strings after splitting on colons, combine everything after the first entry back into a single string with colons between the parts". That should allow colons to fit into the site token without letting it bomb, while still allowing the first colon in the key/value string to act as the delimiter. None of the rest of the values should allow colons, so I don't think this will be a problem with any other parameter field except for the site token.

It does, however, raise an interesting corollary question about pipes; would they cause the same problem? Pipes aren't allowed in URLs according to RFC 3986, but that doesn't necessarily stop someone from using them in a Cryptnos site token. I had to look this up myself to remember, but the good news is that pipes are not allowed in site tokens because they're used for internal delimiter purposes.

So if I can add the extra step to recombine the rest of the site string if colons are encountered, that should allow URLs to be used for site tokens, and I'd only have to update the Android client, since it's the only one that (for now) parses the QR output.

Thanks for the report, Neil. I'm not sure when I'll get a chance to fix this, but I'll hop on it as soon as I can. It's been long enough since I touched the Android client that I'm going to have trouble getting the old Android build system to work again. (I haven't had a chance to upgrade to the newer Android Studio platform yet.) Cross your fingers...

@gpfjeff

This comment has been minimized.

Show comment
Hide comment
@gpfjeff

gpfjeff Oct 28, 2016

Owner

Well, it took me an embarrassingly long time to migrate my development environment to my new laptop and get everything configured, and I wasn't even sure I could get the project to build again, but I was finally able to get in and toy with a fix for this. I was able to successfully create a site definition in the Windows client that used colons in the site token, then import it via QRCode into the fixed Android client.

I'm not sure when I'll be able to push this to Google Play. Now that I've got Eclipse working again, I want to make sure there aren't any other bug fixes that need to be addressed first. However, I'll try to get this pushed out as soon as I can.

Owner

gpfjeff commented Oct 28, 2016

Well, it took me an embarrassingly long time to migrate my development environment to my new laptop and get everything configured, and I wasn't even sure I could get the project to build again, but I was finally able to get in and toy with a fix for this. I was able to successfully create a site definition in the Windows client that used colons in the site token, then import it via QRCode into the fixed Android client.

I'm not sure when I'll be able to push this to Google Play. Now that I've got Eclipse working again, I want to make sure there aren't any other bug fixes that need to be addressed first. However, I'll try to get this pushed out as soon as I can.

gpfjeff added a commit that referenced this issue Oct 28, 2016

Fix for Issue #19
Fix for Issue #19 ("QR code colon encoding/decoding broken"). Updated
copyright date and bumped version number to 1.3.3. Also adding
.gitignore to keep Eclipse's bin folder from going into the repository.
@neilstephens

This comment has been minimized.

Show comment
Hide comment
@neilstephens

neilstephens Oct 28, 2016

Awesome.

On 28 Oct 2016 11:22 AM, "Jeffrey T. Darlington" notifications@github.com
wrote:

Well, it took me an embarrassingly long time to migrate my development
environment to my new laptop and get everything configured, and I wasn't
even sure I could get the project to build again, but I was finally able to
get in and toy with a fix for this. I was able to successfully create a
site definition in the Windows client that used colons in the site token,
then import it via QRCode into the fixed Android client.

I'm not sure when I'll be able to push this to Google Play. Now that I've
got Eclipse working again, I want to make sure there aren't any other bug
fixes that need to be addressed first. However, I'll try to get this pushed
out as soon as I can.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#19 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AIoHZh6Dv-zcenohuwj4oeOiXBpyufPbks5q4UBCgaJpZM4I24vB
.

neilstephens commented Oct 28, 2016

Awesome.

On 28 Oct 2016 11:22 AM, "Jeffrey T. Darlington" notifications@github.com
wrote:

Well, it took me an embarrassingly long time to migrate my development
environment to my new laptop and get everything configured, and I wasn't
even sure I could get the project to build again, but I was finally able to
get in and toy with a fix for this. I was able to successfully create a
site definition in the Windows client that used colons in the site token,
then import it via QRCode into the fixed Android client.

I'm not sure when I'll be able to push this to Google Play. Now that I've
got Eclipse working again, I want to make sure there aren't any other bug
fixes that need to be addressed first. However, I'll try to get this pushed
out as soon as I can.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#19 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AIoHZh6Dv-zcenohuwj4oeOiXBpyufPbks5q4UBCgaJpZM4I24vB
.

@gpfjeff gpfjeff closed this Oct 28, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment