Add support for displaying TOTP keys as QR codes

[adolfogc]

Description

This PR is for adding the feature requested in #764. It replaces PR #964, as something strange happened with my tree of that branch and I decided to start a new one based on upstream/develop and cherry-pick the commit from the previous branch.

Changes

• Instead of using qtqrencode, a new class, QRCode is placed in src/core for enabling support for QR code generation using libqrencode. Besides not having another dependency, we have the benefit of not linking against Qt::QtSvg --which qtqrencode requires.

• QR code size is selected automatically by libqrencode based on input data.

• QR code error correction level is set to its highest.

• Our local base32 implementation is compliant with RFC 4648. Three new methods were added for compatibility with Google Authenticator's "Key Uri Format" (which requires no padding characters):

  • Base32::addPadding/1 This function adds padding to valid encoded data missing those characters. Otherwise, it returns the same input value.
  • Base32::removePadding/1 This function removes padding from valid encoded data. Otherwise, it returns the same input value.
  • Base32::sanitizeInput/1 This function sanitizes the encoded data so that it can then be decoded correctly. It also corrects "typos" (1 -> L, 8 -> B, 0 -> O).

Motivation and context

This is best explained by #764.

How has this been tested?

Currently, it is build for testing using Fedora 26 and the following nix environment:

with import <nixpkgs> {};
stdenv.mkDerivation rec {
    name="keepassxc-env";
    env = buildEnv { name = name; paths = buildInputs; };
    buildInputs = [
        cmake
        libgcrypt
        zlib
        qt5.qtbase
        qt5.qtsvg
        qt5.qttools
        libgpgerror
        glibcLocales
        libmicrohttpd
        xorg.libXtst
        libyubikey
        libqrencode
        clang-tools
    ];
}

You need to enable the option WITH_CX_TOTPDISPLAYKEY:

mkdir build
cd build
cmake -DWITH_ASAN=ON -DWITH_XC_TOTPDISPLAYKEY=ON -DCMAKE_BUILD_TYPE=Debug ..

Test barcodes

• Generate test data using: https://freeotp.github.io/qrcode.html and load it into a test DB.

• Test:

  • Barcode reads correctly from authenticator app
  • Authenticator app generates same OTPs as KeePassXC

• Tested using:

  • Google Authenticator (iOS) ✅
  • FreeOTP (iOS) ✅
  • Microsoft Authenticator (iOS) ✅
  • Yandex Authenticator AKA Ya.Key (iOS) ✅
  • LastPass Authenticator (iOS) ✅

• Also, scan the QR code from the FreeOTP generator site using one of the authenticator apps and compare the TOTPs with those from KeePassXC.

Screenshots (if appropriate):

Below are two images that illustrate the proposed interface for this feature.

Note: you can actually read the barcode above! (Tested with the Ya.Key iOS app).

Types of changes

• ✅ New feature (non-breaking change which adds functionality)
• ✅ Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• ✅ I have read the CONTRIBUTING document. [REQUIRED]
• ✅ My code follows the code style of this project. [REQUIRED]
• ✅ All new and existing tests passed. [REQUIRED]
• ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]
• ✅ My change requires a change to the documentation and I have updated it accordingly.
• ✅ I have added tests to cover my changes. [[ 📝 Except for the UI]].

[TheZ3ro]

The code seems ok and works fine, but there are a lot of ifs that don't have brackets in the QRCode.cpp/Base32.cpp files.
We always enforce brackets (also on 1-line ifs) as codestyle.

[adolfogc]

@TheZ3ro Thanks for the review. I've corrected the single statment if s, they now use braces. Also, I applied clang-format using the project's .clang-format file to all the files modified in this PR in order to correct other coding style errors.

BTW, should I modify the Dockerfile to include libqrencode?

[adolfogc]

What do you think about zeroing the buffer used by libqrencode to store the "dots"?

QRCode.cpp:

// ...

#define __STDC_WANT_LIB_EXT1__ 1
#include <string.h>

// ...

QRCodePrivate::~QRCodePrivate()
{
    if (nullptr != m_qrcode) {
        #ifdef __STDC_LIB_EXT1__
        if (nullptr != m_qrcode->data) {
            const int nBytes = m_qrcode->width * m_qrcode->width;
            memset_s(m_qrcode->data, nBytes, 0, nBytes);
        }
        #endif
        QRcode_free(m_qrcode);
    }
}

// ...

Fixed

[yan12125]

An idea: how about stripping spaces from the TOTP seed before generating the URI? Take "jzls hdx6 fvhm yzpu c6o3 rybg 4ytt uuap" for example, removing spaces saves 7*3 bytes, as each space is encoded as %20 in the URI.
I guess spaces are for users' convenience; they're not necessary for machines. And you don't need to get the seed from mobile apps as everything are already stored in the kdbx database.

[adolfogc]

@yan12125 You mean something like this?

In totp.cpp:

// ...

QUrl QTotp::generateOtpString(const QString& secret,
                              const QString& type,
                              const QString& issuer,
                              const QString& username,
                              const QString& algorithm,
                              const quint8& digits,
                              const quint8& step)
{
    QUrl keyUri;
    keyUri.setScheme("otpauth");
    keyUri.setHost(type);
    keyUri.setPath(QString("/%1:%2").arg(issuer).arg(username));
    QUrlQuery parameters;
    parameters.addQueryItem("secret", secret.simplified()); // <- CHANGE IS HERE
    parameters.addQueryItem("issuer", issuer);
    parameters.addQueryItem("algorithm", algorithm);
    parameters.addQueryItem("digits", QString::number(digits));
    parameters.addQueryItem("period", QString::number(step));
    keyUri.setQuery(parameters);

    return keyUri;
}

// ...

QString QString::simplified() const
Returns a string that has whitespace removed from the start and the end, and that has each sequence of internal whitespace replaced with a single space.
Whitespace means any character for which QChar::isSpace() returns true. This includes the ASCII characters '\t', '\n', '\v', '\f', '\r', and ' '.

Reference: http://doc.qt.io/qt-5/qstring.html#simplified

Edit: I just noticed simplified doesn't remove whitespace in the middle, but just compacts it into a single space.

[droidmonkey]

QString::replace(" ", "")

[yan12125]

Thanks @droidmonkey, that's what I think. Or better, just calling Base32::sanitizeInput. Regards, Yen Chi Hsuan

…

On Tue, Oct 3, 2017 at 2:17 AM, Jonathan White ***@***.***> wrote: QString::replace(" ", "") — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[adolfogc]

Closing this PR in favor of the new implementation in PR #1167.