Skip to content

Add TOTP support, resolves #80#380

Closed
weslly wants to merge 1 commit intokeepassxreboot:developfrom
weslly:feature/totp
Closed

Add TOTP support, resolves #80#380
weslly wants to merge 1 commit intokeepassxreboot:developfrom
weslly:feature/totp

Conversation

@weslly
Copy link
Copy Markdown
Contributor

@weslly weslly commented Mar 6, 2017

Description

This PR add basic support for TOTP code generation (as described by rfc6238) using KeepassXC.

How it Works

Right now it supports the format used by the 2 TOTP plugins listed on Keepass plugins page: KeeOtp and Tray TOTP.

The following formats are supported:

Attribute name Value
Tray TOTP TOTP Seed XXXXXXXXXXXXXXXX
KeeOtp otp key=XXXXXXXXXXXXXXXX

If your entry has one of those attributes, the "Timed One Time Password" context menu will be available.

Motivation and Context

A lot of people avoid using 2FA with TOTP because having to get your phone and opening Google Authenticator app every time you need to login is a hassle. There's also the risk of losing access to your accounts if you lose your phone.

Most modern password managers have support for generating TOTP codes and even Keepass 2.x have a couple of plugins that add this feature so it would be very useful for people who use KeepassXC to have access to their TOTP codes right from Keepass interface.

resolves #80

How Has This Been Tested?

I tested about 10 different sites which I had setup 2FA with TOTP. All of them worked fine with the generated codes.

Screenshots (if appropriate):

image

image

image

Types of changes

  • ✅ New feature (non-breaking change which adds functionality)

Checklist:

  • ✅ I have read the CONTRIBUTING document.
  • ✅ My code follows the code style of this project.
  • ✅ All new and existing tests passed.

@droidmonkey
Copy link
Copy Markdown
Member

This will be great to add to the new HTTP protocol to specifically be able to request and fill in the current totp code.

@weslly
Copy link
Copy Markdown
Contributor Author

weslly commented Mar 6, 2017

totp

The dialog looks a lot better now. 😄

ping @TheZ3ro

@TheZ3ro
Copy link
Copy Markdown
Contributor

TheZ3ro commented Mar 6, 2017

I love it! Testing this on windows RN

@phoerious
Copy link
Copy Markdown
Member

Hell, yeah. It does. 😊


TotpDialog* totpDialog = new TotpDialog(this, currentEntry);
totpDialog->setWindowFlags(totpDialog->windowFlags() | Qt::WindowStaysOnTopHint);
totpDialog->show();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would prefer opening this as a modal dialog (sheet on OS X) with open(). Then you also don't need Qt::WindowStaysOnTopHint.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually I was trying to do this before but couldn't find how since I have no experience with QT. Looks even better now :)

TotpDialog* totpDialog = new TotpDialog(this, currentEntry);
totpDialog->setWindowFlags(totpDialog->windowFlags() | Qt::WindowStaysOnTopHint);
totpDialog->show();
return;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

return here does nothing.


updateProgressBar();

QTimer *timer=new QTimer(this);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Spaces around operator missing.


void TotpDialog::updateProgressBar()
{
if(uCounter <= 100) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Space after if missing.


void TotpDialog::updateSeconds()
{
m_ui->timerLabel->setText(tr("Expires in <b>") + QString::number(30 - (uCounter*30/100)) + tr("</b> seconds"));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think <b>…</b> should be part of the translatable string. Also add spaces around operators.


if (curSeconds >= 30)
curSeconds = curSeconds - 30;
return curSeconds/30*100;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Spaces.


void TotpDialog::copyToClipboard()
{
clipboard()->setText(m_entry->totp());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Some "copy and close" functionality could be useful (just a suggestion).

${GPGERROR_LIBRARIES}
${ZLIB_LIBRARIES})
${ZLIB_LIBRARIES}
${OATH_LIBRARIES})
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since this adds a new dependency, adding a -DWITH_XC_TOTP CMake option would be useful to optionally compile KeePassXC without TOTP support.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Should I set it ON by default?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@droidmonkey @TheZ3ro What do you think?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are the libraries readily available on both MinGW and OSX? If so I say leave it as-is and update the wiki. This should be treated like core capability and not a plugin.

@phoerious phoerious added this to the v2.2.0 milestone Mar 6, 2017
<source>&amp;Clone entry</source>
<translation type="unfinished"></translation>
</message>
<message>
Copy link
Copy Markdown
Member

@phoerious phoerious Mar 6, 2017

Choose a reason for hiding this comment

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

Shouldn't this be spelled "Timed one-time password"?

gui/ChangeMasterKeyWidget.cpp
gui/Clipboard.cpp
gui/CloneDialog.cpp
gui/TotpDialog.cpp
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@weslly This list is sorted alphabetically. This line should go further down.

gui/AboutDialog.ui
gui/ChangeMasterKeyWidget.ui
gui/CloneDialog.ui
gui/TotpDialog.ui
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@weslly this list is also sorted alphabetically.

@weslly
Copy link
Copy Markdown
Contributor Author

weslly commented Mar 6, 2017

Just one thing before I submit the fixes, I'm getting a segfault crash when the progressbar/counter resets but it only happens on newly added entries, if you close the program and open again it works fine. I think it crashes when m_entry->totp() is called to get the new code on reset at TotpDialog.cpp but I can't find what is exactly wrong since it works with old entries. Can you guys take a look at it?

Edit: Seems like it happens only when using the cmd+T shortcut. When I use the mouse to click on the menu it works fine.

Edit 2: For some reason the segfault was happening because of "Cmd+T" as the shortcut so maybe it was a conflict with OSX native key bindings? I changed the shortcut to "cmd+shift+T" and now it doesn't crash anymore.

TheZ3ro
TheZ3ro previously requested changes Mar 6, 2017
Copy link
Copy Markdown
Contributor

@TheZ3ro TheZ3ro left a comment

Choose a reason for hiding this comment

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

Before merging this I would like to compile liboath on windows msys2-env. Right now it's not working. I've opened an issue here https://gitlab.com/oath-toolkit/oath-toolkit/issues/1

@droidmonkey
Copy link
Copy Markdown
Member

droidmonkey commented Mar 6, 2017

With this library are we able to also use it to setup a timed password for the database itself? I could see this "solving" our keylogger issue on entry of the master password since two-level authentication would be required. This would be similar to the Yubikey implementation.

time_t t_now = time(NULL);
char *b_secret = NULL;
size_t b_secret_len;
int rc;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This variable name is ambiguous, recommend renaming to "result"

rc = oath_totp_generate(b_secret, b_secret_len, t_now, 30, 0, 6, output);
}

if (rc != OATH_OK) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is the intention of having this as a separate if statement to also catch errors from oath_totp_generate? Seems like a different error message should be displayed, no?

}

const char *secret = qPrintable(seed.toLatin1());
char output[20];
Copy link
Copy Markdown
Member

@phoerious phoerious Mar 8, 2017

Choose a reason for hiding this comment

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

One more thing I just noticed here: isn't this over-allocated? According to the docs, this should have a size of the number of digits + 1 (for the \0 character).

I would also much prefer if you saved the intended number of digits in a constant numDigits (const unsigned), used a C99 dynamic array here to allocate the buffer with length numDigits + 1 and then passed this constant as a parameter to oath_totp_generate(). That way we avoid possible buffer overflows if someone changes the number of output digits, but doesn't adjust the buffer size or the like.

You may also want to use names for the other parameters ofoath_totp_generate() instead of magic numbers.

&result, &result_len);

if (rc == OATH_OK) {
rc = oath_totp_generate(result, result_len, t_now, 30, 0, 6, output);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this even work? The documentation of oath_base32_decode() says

If out is NULL, then *outlen will be set to what would have been the length of *out on successful encoding.

So as I understand it, result would still be NULL. If for some reason, the docs are wrong, though, it would mean that result had been allocated dynamically and would need to be deallocated again.

I'm also not a huge fan of using the NULL define in C++ code (even when using a C API). You should instead use nullptr.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe this is interested in the address of the pointer container itself (hence the reference to a pointer). After this call, it will be pointing to the allocated data made in the function.

There may be a memory leak, however, because result is never deleted.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Which is not what the docs says. That's why I'm asking. But if it gets allocated, there is a memory leak for sure.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If out is NULL, then *outlen will be set to what would have been the length of *out on successful encoding.

out contains the address of the variable result and not null...

Copy link
Copy Markdown
Member

@phoerious phoerious Mar 12, 2017

Choose a reason for hiding this comment

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

@rockihack Yes, you're right. oath_base32_decode() is being given &result as parameter, not result. So there is no dynamic allocation. However, result is a nullptr and not an allocated char array. This means we have a nice multi-byte buffer overflow here.

Copy link
Copy Markdown
Member

@phoerious phoerious Mar 13, 2017

Choose a reason for hiding this comment

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

Yes. Correct way to convert to char array is:

QByteArray seedBytes = seed.toLatin1();
const char* secret = seedBytes.constData();

This is not the fault of C code, the developer didn't read the documentation.

It kind of is. With a clean C++ API, we wouldn't need to convert to raw C strings at all. But the statement was also more targeted towards the memory leak. C does not do RAII.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

C++ std strings and different character encodings are even worse.
BTW you should replace strlen() with seed.length().

Copy link
Copy Markdown
Member

@phoerious phoerious Mar 13, 2017

Choose a reason for hiding this comment

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

The encoding should be pretty fixed in this case. That would be a non-issue here.

seed.length() and strlen are equivalent. Since what goes into the C function is actually secret and not seed, I find usage of strlen(seed) consistent, although using the length of the QByteArray directly is probably not the worst idea. But then I would prefer size() instead of length().

Copy link
Copy Markdown
Contributor

@rockihack rockihack Mar 13, 2017

Choose a reason for hiding this comment

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

secret or seed whatever the name of the variable is.
strlen() runs in time O(n) and var.length() in O(1), so they are not equivalent.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Alright, that's an argument, although basically irrelevant with a fixed string length of 6 bytes. Still would prefer size() over length(), though.

&result, &result_len);

if (rc == OATH_OK) {
rc = oath_totp_generate(result, result_len, t_now, 30, 0, 6, output);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe this is interested in the address of the pointer container itself (hence the reference to a pointer). After this call, it will be pointing to the allocated data made in the function.

There may be a memory leak, however, because result is never deleted.

const char *secret = qPrintable(seed.toLatin1());
char output[20];
time_t t_now = time(NULL);
char *result = NULL;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This variable should be renamed to: secret_decoded

} else {
return QString(tr("Invalid TOTP Seed."));
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You still need to free() secret_decoded.

const char* secret = seedBytes.constData();
char output[numDigits + 1];
time_t t_now = time(NULL);
char *secret_decoded = nullptr;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The * belongs to the type, not the identifier.

char* secret_decoded = nullptr;
size_t secret_decoded_len;

int rc = oath_init();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Return value not checked.

OATH_TOTP_DEFAULT_START_TIME, // 0
numDigits, output);
} else {
return QString(tr("Invalid TOTP Seed."));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing a call to oath_done().

}

if (rc != OATH_OK) {
return QString(tr("Invalid or unsupported OTP format."));
Copy link
Copy Markdown
Contributor

@rockihack rockihack Mar 13, 2017

Choose a reason for hiding this comment

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

Missing calls to free(secret_decoded) and oath_done().

OATH_TOTP_DEFAULT_START_TIME, // 0
numDigits, output);
} else {
oath_done();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Redundant. oath_done() is called twice when rc != OATH_OK.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

oath_done is never called twice, however its called even if oath_init fails.
This method should be refactored.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

True. I shouldn't review code so late in the night.

@@ -325,25 +325,28 @@ QString Entry::totp() const{

int rc = oath_init();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You should check the return value after this or not save it all and wrap the call in Q_UNUSED().

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You are wrong. Q_UNUSED should only wrap unused parameters and not return values.

Copy link
Copy Markdown
Member

@phoerious phoerious Mar 14, 2017

Choose a reason for hiding this comment

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

Q_UNUSED only adds (void) in front of the line which can also be used to suppress "unchecked return value" warnings.

Copy link
Copy Markdown
Contributor

@rockihack rockihack Mar 14, 2017

Choose a reason for hiding this comment

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

It doesn't matter what Q_UNUSED does. Don't try to be smarter than the qt developers if you use their api. Implementations are prone to change.
https://doc.qt.io/qt-5/qtglobal.html#Q_UNUSED

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The Qt guys themselves sometimes use it for non-parameter expressions.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you have an example? Where do they use it for return values?

Copy link
Copy Markdown
Member

@droidmonkey droidmonkey Mar 14, 2017

Choose a reason for hiding this comment

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

This seems like the easiest, most cross-platform way to accomplish a true "ignore return value"

template<typename T>
inline void ignore_result(T /* unused result */) {}

Allegedly (void) casting is going out of vogue...

Copy link
Copy Markdown
Member

@phoerious phoerious Mar 14, 2017

Choose a reason for hiding this comment

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

I've seen it being used directly on a static_cast expression (e.g. here https://github.com/qt/qtbase/blob/6bceb4a8a9292ce9f062a38d6fe143460b54370e/tests/auto/corelib/tools/qscopedpointer/tst_qscopedpointer.cpp#L200) and what you find extremely often is this:

int foo = bar();
Q_UNUSED(foo);

IMHO you can shorten that by directly using it on the rvalue of the function return, but in either case, that's not a function parameter.

@droidmonkey That creates unnecessary stack operations and pollutes the namespace, though. You also still need some extra mechanism inside ignore_result() (which would probably be void casting again or some useless operation on the parameter).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Alright they use it to prevent unused parameter AND unused local variable warnings, but I doubt that they use it for return values.

Copy link
Copy Markdown
Member

@phoerious phoerious Mar 14, 2017

Choose a reason for hiding this comment

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

There is virtually no difference. If you really want to be sure that it doesn't break if they change it to something that doesn't work with rvalues (highly unlikely, would also break their own static_cast code), you can always make an lvalue from it before using Q_UNUSED. The point is just that we don't want compiler warnings about ignored return values and Q_UNUSED can get rid of them. Whether you apply it directly to the function call or create an lvalue first, I don't care.

QByteArray seedBytes = seed.toLatin1();
const char* secret = seedBytes.constData();
char output[numDigits + 1];
time_t t_now = time(NULL);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use nullptr instead of NULL.

}

free(secret_decoded);
oath_done();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should also be wrapped in Q_UNUSED(), since it returns an integer.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No it shouldn't, see above.

droidmonkey
droidmonkey previously approved these changes Mar 14, 2017
void TotpDialog::copyToClipboard()
{
clipboard()->setText(m_entry->totp());
clipboard()->setText(m_entry->totp());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this trailing whitespace accidental?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is. Thanks, I'll fix on next commit

@TheZ3ro TheZ3ro dismissed their stale review March 19, 2017 20:03

liboath has now been removed

@Gargauth
Copy link
Copy Markdown

Gargauth commented Mar 23, 2017

Will this support more than 6 digits long codes? They can be specified like otpauth://totp/Authenticator-Test:?secret=RANDOMSECRET&digits=8. Some services I use have 8 digits long codes, so support for it would be greatly appreciated. Thank you.

@weslly
Copy link
Copy Markdown
Contributor Author

weslly commented Mar 23, 2017

@Gargauth not yet, but I'll implement it as soon as I have some time.

const unsigned numDigits = 6;

int secretLen = (key.length() + 7) / 8 * 5;
quint8 secret[100];
Copy link
Copy Markdown
Member

@phoerious phoerious Mar 25, 2017

Choose a reason for hiding this comment

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

One last thing: hard-coded array size. Were does this come from and can't we use a dynamic array here? Or maybe also use QVector instead of a plain C-Array.

Other than that: good job. Code is a lot cleaner now without the C library.

Copy link
Copy Markdown
Contributor Author

@weslly weslly Mar 25, 2017

Choose a reason for hiding this comment

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

@phoerious The 100 length is pretty much just a guess of a reasonable secret string maximum size (including spaces).

https://github.com/google/google-authenticator-libpam/blob/master/src/google-authenticator.c#L61

I can change the type to something like QVector but secret is used as an argument to base32_decode() which is basically C code from google-auth-libpam and expects a plain C-Array anyways.

Maybe I should change it to be a dynamic array using secretLen + 1 as the size?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't guess any sizes. Use the size the string will actually have.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@phoerious Updated

Copy link
Copy Markdown
Member

@droidmonkey droidmonkey Mar 26, 2017

Choose a reason for hiding this comment

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

Might be a dumb question, but why use quint8 and not char?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Doesn't make any practical difference in this case, but the source base32_decode() function from google-auth-libpam already used uint8_t instead of char.

return;
}

TotpDialog* totpDialog = new TotpDialog(this, currentEntry);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Memory leak?

Copy link
Copy Markdown
Member

@phoerious phoerious Mar 26, 2017

Choose a reason for hiding this comment

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

Shouldn't be a memory leak. The instance is parented to this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

And this refers to a DatabaseWidget, that probably is a long lived object.
Possible fix: totpDialog->setAttribute(Qt::WA_DeleteOnClose);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, good point.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rockihack @phoerious
I had Qt::WA_DeleteOnClose already set at TotpDialog.cpp line 34 before, doesn't it have the same effect?

}


QByteArray QTotp::hmacSha1(QByteArray key, QByteArray baseString)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can use libgcrypt instead of implementing your own hmac construction.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note: This file and function is part of QTotp library

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should still fix those issues. And yes, I was also thinking about using libgcrypt.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@phoerious If we need to fix those "issues" so we should open PR to QTotp, and if we think about using libgcrypt we should drop QTotp and implement our Totp interface directly

Copy link
Copy Markdown
Contributor Author

@weslly weslly Mar 26, 2017

Choose a reason for hiding this comment

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

Actually Qt has its own hmac class (QMessageAuthenticationCode) so I'll use that instead of libgcrypt. Which is kinda a relief, because I really don't want to deal with external C libraries again, especially something as low level as libgcrypt seems to be.

const unsigned numDigits = 6;

int secretLen = (key.length() + 7) / 8 * 5;
quint8 secret[secretLen];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This does not work as expected. You need to use new[] and delete[] for dynamic arrays.

Copy link
Copy Markdown
Member

@phoerious phoerious Mar 26, 2017

Choose a reason for hiding this comment

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

Only true for VCpp. GCC support dynamic (variable-length) stack arrays.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ok seems to be valid c code, c++11 standard mentions array size as a constant-expression.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note: This file and function is part of QTotp library

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's a C99 feature.

@droidmonkey droidmonkey dismissed their stale review March 26, 2017 14:32

requires tests

Copy link
Copy Markdown
Member

@droidmonkey droidmonkey left a comment

Choose a reason for hiding this comment

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

Tests of the totp code need to be included. Especially tests of the custom written decode32 function.


updateProgressBar();

QTimer *timer = new QTimer(this);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Memory leak?

Copy link
Copy Markdown
Member

@phoerious phoerious Mar 26, 2017

Choose a reason for hiding this comment

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

This should be a QScopedPointer as a member variable. But it shouldn't leak anything, as it's parented to this.

int secretLen = (key.length() + 7) / 8 * 5;
quint8 secret[secretLen];
int res = QTotp::base32_decode(reinterpret_cast<const quint8 *>(key.constData()), secret, secretLen);
QByteArray hmac = QTotp::hmacSha1(QByteArray(reinterpret_cast<const char *>(secret), res), QByteArray(reinterpret_cast<char*>(&current), sizeof(current)));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How can you convert a quint64 value (current) into a char*??

Copy link
Copy Markdown
Member

@phoerious phoerious Mar 26, 2017

Choose a reason for hiding this comment

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

A pointer to char is effectively a 64-bit integer on 64-bit systems. I'm not quite sure about the reason for these cast stunts.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Dont forget the ampersand...

Copy link
Copy Markdown
Member

@phoerious phoerious Mar 27, 2017

Choose a reason for hiding this comment

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

That's generally not the problem. I see how these casts work. It's rather that this part is utterly confusing. Both casts are just explicit casts of pointers. Once from a quint8 array to a char array and once from a pointer to quint64 to a pointer to char (why?). I'm really not sure why we don't do it more explicitly: specify the template parameter directly for the QByteArray parameter and use the correct types for the rest right from the start.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Once from a quint8 array to a char array and once from a pointer to quint64 to a pointer to char (why?).

To convert quint64 to QByteArray.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, but why not use something like QByteArray::number()?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Because it doesn't work that way.

Returns a byte array containing the string equivalent of the number

Also you need to pay attention to endianness here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm. Okay. Seems like there is really no dedicated interface for converting numbers to byte arrays. Weird. In that case…

@droidmonkey
Copy link
Copy Markdown
Member

droidmonkey commented Mar 26, 2017

@weslly I rewrote the base32decode function using QByteArrays. Please consider incorporating this into your next changeset: https://gist.github.com/droidmonkey/091ed6a5b69a5dfe2725a0ae3dd9f1c9

I also included a simple test case.

@droidmonkey
Copy link
Copy Markdown
Member

I recommend that once this PR gets finalized that the history be cleaned up. There are a ton of minor commits with very short summaries that can be easily squashed together.

@ibrokemypie
Copy link
Copy Markdown

@weslly would you consider adding in support for steam auth codes? keetraytotp has got it working with the totp settings syntax of symply 30;S;http://steampowered.com

@weslly
Copy link
Copy Markdown
Contributor Author

weslly commented Mar 29, 2017

@ibrokemypie I'll take a look at it as soon as the basic functionality is finished and merged.

@TheZ3ro
Copy link
Copy Markdown
Contributor

TheZ3ro commented Apr 1, 2017

News about this? @weslly

@TheZ3ro TheZ3ro dismissed phoerious’s stale review April 1, 2017 12:19

Report has been fixed

@TheZ3ro TheZ3ro requested a review from phoerious April 1, 2017 12:19
@weslly
Copy link
Copy Markdown
Contributor Author

weslly commented Apr 3, 2017

@TheZ3ro I still need a couple of days to finish the fixes from last review.

@ArchangeGabriel
Copy link
Copy Markdown

ArchangeGabriel commented Apr 5, 2017

No one answered this comment from @droidmonkey:

With this library are we able to also use it to setup a timed password for the database itself? I could see this "solving" our keylogger issue on entry of the master password since two-level authentication would be required. This would be similar to the Yubikey implementation.

I’m really interested in the answer to this question. Having my KeePassXC further more secured by a TOTP from my phone would be nice. ;)

@TheZ3ro
Copy link
Copy Markdown
Contributor

TheZ3ro commented Apr 5, 2017

@ArchangeGabriel I don't think this is possible.
Like we have done with Yubikey: we should use the TOTP secret directly as key-material when encrypt the database file but then you cannot get the secret from a TOTP token. You can't generate a token from the secret if it's used to encrypt the DB (you don't know the secret) and so you can't do it this way.
We discussed this in the Yubikey PR.

Anyway this is unrelated with this PR that add the ability to generato TOTP token for other services

@weslly
Copy link
Copy Markdown
Contributor Author

weslly commented Apr 13, 2017

Closing this as I'm working on a new branch with some new features and most of the current code was changed already.

I will submit a new PR as soon as I finish.

@weslly weslly closed this Apr 13, 2017
@weslly weslly mentioned this pull request Apr 21, 2017
@droidmonkey droidmonkey modified the milestone: v2.2.0 Jun 19, 2017
@phoerious phoerious added pr: new feature Pull request adds a new feature and removed new feature labels Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: TOTP pr: new feature Pull request adds a new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature request: TOTP support

10 participants