The new Base32 implementation doesn't accept spaces for decoding, breaking TOTP on some websites

[yan12125]

Expected Behavior

For entries with TOTP secrets having spaces, they work just fine as before. For example, "jzls hdx6 fvhm yzpu c6o3 rybg 4ytt uuap" can be successfully decoded and give a 6-digit number.

(This is just a fake example. I didn't actually use this secret on any website.)

Current Behavior

The string "Invalid TOTP secret key" is copied to the clipboard after hitting Ctrl+T. If "Show TOTP" is selected, it says "Invalid TOT secret key" (a typo? :)

Possible Solution

I suggest to ignore specific characters like Google's implementation: 522e132#diff-18f0d04bfa499ef56a17c0ba1d834e78L34. I have at least 10 entries with TOTP settings, and I don't think it's a good idea to ask users to fix them one by one.

A minor point - don't place the error message to the clipboard. Show a popup instead!

Steps to Reproduce (for bugs)

1. Create an entry
2. Setup TOTP and enter a secret like "jzls hdx6 fvhm yzpu c6o3 rybg 4ytt uuap"
3. Ctrl+T to copy the TOTP code or select "Show TOTP"

Context

Quite a few websites are broken as they provide base32-encoded secrets with spaces on the webpage, like Google or BitBucket. I believe there are more such sites.

Debug Info

KeePassXC - Version 2.2.1
Revision: 14e3d9d

Libraries:

• Qt 5.9.1
• libgcrypt 1.8.1

Operating system: Arch Linux
CPU architecture: x86_64
Kernel: linux 4.13.4-1-ARCH

Enabled extensions:

• KeePassHTTP
• Auto-Type
• YubiKey

I built keepassxc from the AUR building script with my own patch proposed at #1017 (comment)

[adolfogc]

Hi @yan12125, there is indeed a regression that breaks compatibility with authenticator apps such as Google Authenticator. This issue is resolved in PR #1001, where a function, Base32::sanitizeInput/1, is introduced to transform strings before being decoded when they don't conform to RFC 4648. This function applies the same fixes that Google Authenticator's implementation and also adds any missing pad characters (this is because the Key Uri Format assumes no padding). If the string is such that it can't be fixed, or it is the empty string, it is returned as is.

I agree with you in that displaying the error using a message dialog is more user friendly.

[yan12125]

Oh thanks @adolfogc! I missed out that PR. It does much more than its title described :)

Applied #1001 and rebuild. Works fine!

[adolfogc]

@yan12125 Thanks for testing it, glad to hear it now works correctly.

[phoerious]

I'd be amazing if you could port back your fix to release/2.2.2 (without the QR code stuff).

[adolfogc]

@phoerious No problem, I'll port that fix.

[yan12125]

From #keepassxc-dev of Freenode:

[20:01:04] <droidmonkey> I want to get 2.2.2 out next week then focus on merging in the browser code

I always build my own keepassxc version with a bunch of feature patches and bug fixes, yet I think merging this before 2.2.2 can help those who are unable to build themselves (e.g. those on Windows and don't have good network to install msys2+qt5)

[droidmonkey]

yup we have this marked for 2.2.2

[adolfogc]

Hello. I'll port the Base32 fixes from PR #1001 and submit a new PR. I'm planning on working on this tonight; should I base the new PR on develop instead of release/2.2.2? There's no new Base32 implementation in the current tree of release/2.2.2.

[droidmonkey]

See phoerious comment above, we would like your base32 implementation without the qr code in 2.2.2 (if possible) plus the fix for this issue. when we merge 2.2.2 into develop the fix will follow.

[adolfogc]

Ok, thanks, I understand now.

[frostasm]

I have another problem with the new algorithm. I have a TOTP secret key with a length of 52 characters. But the new algorithm requires that the key length be a multiple of 8. 😒

if (encodedData.size() % 8 != 0)
    return Optional<QByteArray>();

[frostasm]

false alarm, I added four "equality signs" to the end of the private key, and everything works fine :)

[phoerious]

Base32/base64 require fixed-length blocks and strings of different lengths need padding. Perhaps we should pad the string to the correct length automatically to avoid confusion.

[frostasm]

@phoerious thanks for the explanation

[phoerious]

Fixed in #1069