Skip to content
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

Yubikey is not re-detected after replugging. #712

Closed
CRCinAU opened this issue Jun 29, 2017 · 24 comments
Closed

Yubikey is not re-detected after replugging. #712

CRCinAU opened this issue Jun 29, 2017 · 24 comments

Comments

@CRCinAU
Copy link

CRCinAU commented Jun 29, 2017

When using the Challenge/Response mode with a Yubikey, if you unplug the key and replug it, the key is no longer detected or able to do the challenge / response authentication.

This leads to being unable to save the database after any changes.

Error message states:
Writing the database failed. Unable to issue challenge-response.

Expected Behavior

Key should be redetected on replug - or only accessed when the functionality is needed.

Current Behavior

  1. Open a challenge / response secured database
  2. Remove the yubikey
  3. Make a change to something
  4. Plug in the Yubikey
  5. Try to save the database.

Possible Solution

Should only access the Yubikey when a challenge is required. Once the challenge is complete, the key should be released.

Debug Info

KeePassXC - Version 2.2.0
Revision: caa49a8

Libraries:

  • Qt 5.7.1
  • libgcrypt 1.7.7

Operating system: Fedora 26 (Twenty Six)
CPU architecture: x86_64
Kernel: linux 4.11.6-301.fc26.x86_64

Enabled extensions:

  • KeePassHTTP
  • Auto-Type
  • YubiKey
@phoerious
Copy link
Member

I suppose this is rather a YubiKey lib issue.

@phoerious phoerious added the bug label Jun 29, 2017
@CRCinAU
Copy link
Author

CRCinAU commented Jun 29, 2017

Not sure - but I've found out that it locks you out of slot 1 operation while you use it for Keepassxc - which makes me think that the key isn't closed between required uses...

EDIT: I should clarify that my Challenge/Response config is in slot 2.

@matyx
Copy link

matyx commented Jul 4, 2017

I believe that this is Keepassxc issue. I have found a workaround - you can redetect yubikey without quitting keepassxc. You need to go to Databases -> Change master key -> Click refresh under Challenge response section. Keepassxc will be able to use your yubikey.

I would suppose to call Challenge/Response refresh procedure before saving the database if the database is opened via yubikey and there is no yubikey present.

@CRCinAU
Copy link
Author

CRCinAU commented Jul 4, 2017

These bits of code interest me - but I'm absolutely hopeless at C++.....

#ifdef WITH_XC_YUBIKEY

#ifdef WITH_XC_YUBIKEY

One is for the Open Database dialog, the other for modifying the master key. They seem to be somewhat different - but my C++ skills are that useless that I can't track things further to figure out what's going on here - but it seems like a prime candidate.

sainaen added a commit to sainaen/keepassxc that referenced this issue Jul 4, 2017
@sainaen
Copy link

sainaen commented Jul 4, 2017

The loop inside YkChallengeResponseKey::challenge(const QByteArray& challenge, unsigned retries) in its current form doesn't retry when argument retries is equal to 1 (as in this method's single invocation point in YkChallengeResponseKey::challenge(const QByteArray&)), so it performs only one attempt.

I wrote a simple patch to fix this, but there still is a problem that KeePassXC doesn't display a message asking to press a button on the key in case of an error. I suspect it's caused by how fast a sequence of displayGlobalMessage(), hideGlobalMessage() and then again displayGlobalMessage() is called. I don't know how to fix that yet. (Plus, I'm not sure this is the best way to fix this: is it ok to allow unsigned variable to underflow on the last iteration? I'm worried that since I'm not a C++ programmer I don't recognize some hidden danger in doing this, even though it seems to work.)

sainaen added a commit to sainaen/keepassxc that referenced this issue Jul 6, 2017
Instead of retrying a predefined number of times in case of an error,
always try to re-init Yubikey before issuing a challenge.

Fixes keepassxreboot#712.
@sainaen
Copy link

sainaen commented Jul 6, 2017

So I thought about this issue a bit more, and now I think using retries is not a proper way to address this: it's just too general to make this one additional challenge attempt.

Here's a patch that replaces the retry loop with a call to YubiKey::init(). This way

  1. we'll learn if Yubikey was unplugged after the last use before issuing a new challenge
  2. if it was unplugged and reinserted old YK_KEY structure will be properly freed, and a new one created
  3. if we still have a good reference init() will do nothing except checking the status

One problem this could cause would be a possible performance hit for calling yk_get_status() too often, but I couldn't find any info on how expensive this call is. My Yubikey is configured to wait for the button press so I probably wouldn't notice additional delay anyway.

What do you think?

Edit: Forgot to remove challenge(seed, retries) from the header file. Fixed in the new commit.

sainaen added a commit to sainaen/keepassxc that referenced this issue Jul 6, 2017
Instead of retrying a predefined number of times in case of an error,
always try to re-init Yubikey before issuing a challenge.

Fixes keepassxreboot#712.
@CRCinAU
Copy link
Author

CRCinAU commented Jul 6, 2017

Not specifically related to the patch - but regarding the dual slot functionality.

Right now, I have OTP mode in slot 1, and non-button press Challenge/Response in slot 2.

When the C/R happens, and after you have unlocked your KP database, I find that the slot 1 function no longer works until I replug the Yubikey.

I would expect that proper behaviour would be to release the Yubikey until it is needed again - which in theory would allow functionality in the other slot to work again.

Is this your understanding? or is there something else I'm not taking into account here?

@sainaen
Copy link

sainaen commented Jul 6, 2017

@CRCinAU
I have almost the same configuration but I don't use OTP, so didn't notice this before. Yes, I suspect that releasing the key properly would fix it, but then again I'm only on my day 3 of trying to figure out this Yubikey thing. :)

Edit: Oh, I think the issue #698 is the same, but in that case they're unable to use static password instead of OTP.

@droidmonkey
Copy link
Member

This is particularly nasty because if you try to save the database with the yubikey in a "disconnected" state you can corrupt the database file. I just did this playing around with a test database (thankfully). This bug needs to be fixed ASAP.

@cjustin88
Copy link

Is there any progress on this? I was the one who filed #864 and as @droidmonkey described, I corrupted my database. Thankfully, I have revision history for the database file. This needs to be fixed urgently.

@lopopolo
Copy link

Another consequence of this issue is that keepassxc will only recognize a yubikey if it is plugged in before keepassxc is launched.

So if you are working with a database and then attempt to open another one that requires the yubikey, you must quit and relaunch keepassxc.

@phoerious
Copy link
Member

You can click the Refresh button to detect a key that was inserted after KeePassXC has been started.

@droidmonkey
Copy link
Member

I'll be working on this tomorrow.

@CRCinAU
Copy link
Author

CRCinAU commented Aug 19, 2017

@droidmonkey Out of interest, does this fix releasing the Yubikey after a C/R cycle?

I use an OTP in slot 1, C/R in slot 2.

Once I use the C/R within KeepassXC, I cannot use the OTP function without re-plugging in the Yubikey...

@droidmonkey
Copy link
Member

It might not, i didnt see any obvious hold on the yubikey except when performing the c/r. I am able to use a static password on slot 2 and c/r in slot 1 without issue.

@CRCinAU
Copy link
Author

CRCinAU commented Aug 19, 2017

If you use just the C/R in slot 2 for KeepassXC, with Slot 1 configured for an OTP, can you generate an OTP into a text editor afterwards?

My OTP is used purely for an OpenVPN connection...

@droidmonkey
Copy link
Member

I'll try that

@droidmonkey
Copy link
Member

droidmonkey commented Aug 20, 2017

I configured the Yubikey per your method and it works great (even with the origin 2.2.0 release). I even required a press to issue the c/r. This might be an issue on your end that is unrelated to KeePassXC. I did notice some flakiness when the Yubikey Personalization Tool was open, it seemed to be disconnecting and reconnecting the yubikey several times.

@CRCinAU
Copy link
Author

CRCinAU commented Aug 20, 2017

Interesting... Which version key was this? Did it do the same if you didn't require a press for the C/R?

Mine is the original Yubikey - and the light comes back on after the C/R - but when I press it to do the OTP, the light goes out and never returns...

ie my sequence:

  1. Launch KeepassXC
  2. Select the Yubikey to C/R
  3. Minimise KeepassXC
  4. Open kwrite (or any text editor)
  5. Press button for OTP (slot 1 function)
  6. Light on YK goes out and doesn't return & no OTP is issued.

@droidmonkey
Copy link
Member

I have the Neo, just recently purchased for testing purposes. I am also on Windows 10 so that may be a difference. I will have to try this in Linux to see if the behavior replicates.

@CRCinAU
Copy link
Author

CRCinAU commented Aug 20, 2017

Oh yeah, Windows.... I forgot that was a thing :\

@cjustin88
Copy link

@droidmonkey I am experiencing this issue under Linux (Fedora 26, 4.12.5-300.fc26.x86_64). My report #864 has some more debug info, if that's useful at all...

@CRCinAU
Copy link
Author

CRCinAU commented Sep 23, 2017

Notes on closure?

@TheZ3ro
Copy link
Contributor

TheZ3ro commented Sep 23, 2017

Fixed in #880

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

No branches or pull requests

8 participants