Add new Base32 implementation by adolfogc · Pull Request #1069 · keepassxreboot/keepassxc

adolfogc · 2017-10-15T05:06:23Z

Description

Provides a port of the new Base32 implementation, plus fixes to make it compatible with Google Authenticator's implementation, that was previously introduced in PRs #984 and #1001.

Motivation and context

See issue #1022.

How has this been tested?

Manually on Linux.

Screenshots (if appropriate):

N/A.

Types of changes

✅ Bug fix (non-breaking change which fixes an issue)

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]
✅ I have added tests to cover my changes.

adolfogc · 2017-10-19T03:41:39Z

I've rebased.

TheZ3ro · 2017-10-19T16:10:17Z

src/core/Base32.cpp

+        }
+
+        // add pad characters
+        while (o < encodedData.size())


add brackets

phoerious · 2017-10-20T14:38:45Z

src/core/Base32.cpp

+        quint64 quantum = 0;
+        int nQuantumBytes = 5;
+
+        for (int n = 0; n < 8; n++) {


Ok, will change it.

phoerious · 2017-10-20T14:54:23Z

src/core/Base32.cpp

+        quint64 mask = quint64(0xFF) << offset;
+        for (int n = offset; n >= 0; n -= 8) {
+            char c = static_cast<char>((quantum & mask) >> n);
+            data[o++] = c;


I would sleep better if you added a few range checking assertions here and for encodedData inside the encode() method when doing something like this with long-lived counters. QByteArray implicitly extends the array if you assign an element beyond the array's range. So we wouldn't get a buffer overflow, but probably wouldn't notice either.

Good catch! This helped me find a bug in Base32::decode() affecting how the number of bytes of decoded data was being calculated, even though it didn't truncate the result, it was causing the array to be resized unnecessarily. I will be submitting the changes soon.

phoerious · 2017-10-20T15:03:27Z

src/core/Optional.h

+ * by all the main compiler toolchains.
+ */
+
+template <typename T> class Optional


Do we really need our own Optional implementation? Wouldn't the same be possible with QVariant?

I think we can make this change to reduce the code base size, as the variant type can serve as an optional.

Just use QVariant::isNull() or QVariant::isValid().

I'm almost done making this change, I decided for QVariant::isNull; as I understand thatQVariant::isValid is used for detecting when a type not defined for the variant (i.e. not registered with Q_DECLARE_METATYPE) is used.

phoerious · 2017-10-20T15:05:22Z

src/totp/totp.cpp

-            | ((hmac[offset + 1] & 0xff) << 16)
-            | ((hmac[offset + 2] & 0xff) << 8)
-            | (hmac[offset + 3] & 0xff);
+    int binary = ((hmac[offset] & 0x7f) << 24) | ((hmac[offset + 1] & 0xff) << 16) | ((hmac[offset + 2] & 0xff) << 8) |


I liked the previous formatting better. It's hard to read on a single line.

I agree with you, this was the result of running clang-format; I'll change it.

The bug that was fixed, was affecting how the number of bytes of decoded data was calculated and thus, even though it didn't truncate the result, it was causing the array to be resized unnecessarily.

adolfogc · 2017-10-21T00:33:51Z

@phoerious I've finished making the changes solicited, please review them.

phoerious · 2017-10-21T11:15:19Z

Nice work. Thanks!

- Fixed entries with empty URLs being reported to KeePassHTTP clients [#1031] - Fixed YubiKey detection and enabled CLI tool for AppImage binary [#1100] - Added AppStream description [#1082] - Improved TOTP compatibility and added new Base32 implementation [#1069] - Fixed error handling when processing invalid cipher stream [#1099] - Fixed double warning display when opening a database [#1037] - Fixed unlocking databases with --pw-stdin [#1087] - Added ability to override QT_PLUGIN_PATH environment variable for AppImages [#1079] - Fixed transform seed not being regenerated when saving the database [#1068] - Fixed only one YubiKey slot being polled [#1048] - Corrected an issue with entry icons while merging [#1008] - Corrected desktop and tray icons in Snap package [#1030] - Fixed screen lock and Google fallback settings [#1029]

droidmonkey added this to the v2.2.2 milestone Oct 19, 2017

droidmonkey requested review from TheZ3ro, droidmonkey and phoerious October 19, 2017 03:13

droidmonkey added high priority 🚨 new feature labels Oct 19, 2017

TheZ3ro reviewed Oct 19, 2017

View reviewed changes

src/core/Base32.cpp Outdated

}

// add pad characters

while (o < encodedData.size())

Copy link
Copy Markdown

Contributor

TheZ3ro Oct 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add brackets

droidmonkey approved these changes Oct 19, 2017

View reviewed changes

phoerious requested changes Oct 20, 2017

View reviewed changes

adolfogc added 7 commits October 20, 2017 18:12

Add new Base32 implementation

14094e7

Fix code style

ba911a4

Fix for loop's variable increment style

3473d2d

Revert formatting change

3bca598

Replace Optional with QVariant

e28f091

Fix bug in Base32::decode and add bounds check

cd2f2f0

The bug that was fixed, was affecting how the number of bytes of decoded data was calculated and thus, even though it didn't truncate the result, it was causing the array to be resized unnecessarily.

Improve testing of Base32

2c6a214

Improve readability of code

71989b0

phoerious approved these changes Oct 21, 2017

View reviewed changes

phoerious merged commit 86cd2c0 into keepassxreboot:release/2.2.2 Oct 21, 2017

phoerious mentioned this pull request Oct 21, 2017

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

Closed

adolfogc deleted the new-base32-implementation-with-fixes branch November 6, 2017 05:11

phoerious added pr: new feature Pull request adds a new feature and removed new feature labels Nov 22, 2024

Uh oh!

Conversation

adolfogc commented Oct 15, 2017

Description

Motivation and context

How has this been tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

adolfogc commented Oct 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phoerious Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phoerious Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adolfogc commented Oct 21, 2017

Uh oh!

phoerious commented Oct 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

phoerious Oct 20, 2017 •

edited

Loading

phoerious Oct 20, 2017 •

edited

Loading