search

android-password-store / Android-Password-Store

Android application compatible with ZX2C4's Pass command line application
https://passwordstore.app
GNU General Public License v3.0
2.53k stars 250 forks source link

[BUG] Authentication fails with curve25519 subkey on security token #1272

Closed MoritzMaxeiner closed 3 years ago

MoritzMaxeiner commented 3 years ago

Describe the bug Trying to clone a remote repository with OpenKeychain-provided curve25519 authentication subkey located on security token falls back on password authentication due null object reference in signing operation.

To Reproduce Steps to reproduce the behavior:

  1. Register a security token with an curve25519 authentication subkey in OpenKeychain
  2. Try to clone a remote repository (where the key is authorized)
  3. See (fallback) password prompt instead of (security token) pin prompt

Expected behavior The pin prompt for the security token should have popped up.

Screenshots key_information key_selection password_fallback

Device information (please complete the following information):

Additional context logcat.txt

msfjarvis commented 3 years ago

@fmeum the exception is java.lang.NullPointerException: Attempt to invoke virtual method 'long org.bouncycastle.openpgp.PGPPrivateKey.getKeyID()', from within the guts of the OpenKeychain sshauth library.

MoritzMaxeiner commented 3 years ago

Should I report this to OpenKeychain itself instead, then?

msfjarvis commented 3 years ago

Should I report this to OpenKeychain itself instead, then?

No it's more likely that we regressed this on our end, once Fabian has had a chance to review they'll let you know if OpenKeychain needs to be notified.

fmeum commented 3 years ago

@MoritzMaxeiner The bug you are seeing has already been reported upstream as https://github.com/open-keychain/open-keychain/issues/2538 and https://github.com/open-keychain/open-keychain/issues/2589. I will try to fix it myself when I have time, but it is not a high priority as our new Keystore-backed SSH authentication serves more or less the same purpose (albeit it is slightly less convenient to set up as you need to add a new public key to your server).

fmeum commented 3 years ago

@msfjarvis I felt like we needed a blocked-on-upstream label, so I went ahead and created one.

MoritzMaxeiner commented 3 years ago

@fmeum I see, thanks. FWIW: The key is shown as healthy and capable of signing (In contrast to https://github.com/open-keychain/open-keychain/issues/2589).

key_healthy

clumbo commented 3 years ago

Just to add to this, using a yubikey is completely broken atm, to keep things secure and not storing ssh private keys on my phone goes against the convention.

sevmonster commented 3 years ago

I don't know if this is related or not, but I feel like it might be.

Just to add to this, using a yubikey is completely broken atm, to keep things secure and not storing ssh private keys on my phone goes against the convention.

Mine works fine when I attempt to decrypt pass entries created or last edited from terminal. However, when I create a new entry or edit an existing entry and then later try to decrypt it, I am asked for PIN as normal, but then told the PIN is incorrect when I activate NFC. I try again, and OpenKeychain crashes with this error:

Attempt to invoke virtual method 'org.bouncycastle.math.ec.ECCurve org.bouncycastle.asn1.x9.X9ECParameters.getCurve()' on a null object reference

If I push, edit in terminal, and pull, I am able to open it again.

I know the PIN is correct because the counter does not decrement, and I can use the same PIN with other entries and *nix gpg just fine. Relevant info from GPG:

Application type .: OpenPGP
Version ..........: 3.4
Key attributes ...: ed25519 cv25519 ed25519
PIN retry counter : 3 0 3

Traceback:

FATAL EXCEPTION: AsyncTask #3
Process: org.sufficientlysecure.keychain, PID: 24474
java.lang.RuntimeException: An error occurred while executing doInBackground()
        at android.os.AsyncTask$3.done(AsyncTask.java:354)
        at java.util.concurrent.FutureTask.finishCompletion(FutureTask.java:383)
        at java.util.concurrent.FutureTask.setException(FutureTask.java:252)
        at java.util.concurrent.FutureTask.run(FutureTask.java:271)
        at android.os.AsyncTask$SerialExecutor$1.run(AsyncTask.java:245)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
        at java.lang.Thread.run(Thread.java:764)
Caused by: java.lang.NullPointerException: Attempt to invoke virtual method 'org.bouncycastle.math.ec.ECCurve org.bouncycastle.asn1.x9.X9ECParameters.getCurve()' on a null object reference
        at org.sufficientlysecure.keychain.securitytoken.operations.PsoDecryptTokenOp.getEcDecipherPayload(PsoDecryptTokenOp.java:203)
        at org.sufficientlysecure.keychain.securitytoken.operations.PsoDecryptTokenOp.decryptSessionKeyEcdh(PsoDecryptTokenOp.java:119)
        at org.sufficientlysecure.keychain.securitytoken.operations.PsoDecryptTokenOp.verifyAndDecryptSessionKey(PsoDecryptTokenOp.java:80)
        at org.sufficientlysecure.keychain.ui.SecurityTokenOperationActivity.doSecurityTokenInBackground(SecurityTokenOperationActivity.java:214)
        at org.sufficientlysecure.keychain.ui.base.BaseSecurityTokenActivity.handleSecurityToken(BaseSecurityTokenActivity.java:439)
        at org.sufficientlysecure.keychain.ui.base.BaseSecurityTokenActivity$1.doInBackground(BaseSecurityTokenActivity.java:149)
        at org.sufficientlysecure.keychain.ui.base.BaseSecurityTokenActivity$1.doInBackground(BaseSecurityTokenActivity.java:137)
        at android.os.AsyncTask$2.call(AsyncTask.java:333)
        at java.util.concurrent.FutureTask.run(FutureTask.java:266)
        ... 4 more

Some bugs with the same exception:

Unfortunately I am no guru so leafing through the packets does not reveal much about why this might be happening.

Android-Password-Store packets:

% gpg --list-packets example.com.gpg
gpg: encrypted with 256-bit ECDH key, ID 0xABCDEF1234567890, created 2020-09-20
      "test <test@key>"
# off=0 ctb=84 tag=1 hlen=2 plen=94
:pubkey enc packet: version 3, algo 18, keyid ABCDEF1234567890
        data: [263 bits]
        data: [392 bits]
# off=96 ctb=d2 tag=18 hlen=2 plen=148 new-ctb
:encrypted data packet:
        length: 148
        mdc_method: 2
# off=117 ctb=ac tag=11 hlen=2 plen=105
:literal data packet:
        mode b (62), created 1611380040, name="GckbLi-example.com.txt",
        raw data: 69 bytes

GnuPG on Linux packets:

% gpg --list-packets example.com.gpg
gpg: encrypted with 256-bit ECDH key, ID 0xABCDEF1234567890, created 2020-09-20
      "testkey <test@key>"
# off=0 ctb=84 tag=1 hlen=2 plen=94
:pubkey enc packet: version 3, algo 18, keyid ABCDEF1234567890
        data: [263 bits]
        data: [392 bits]
# off=96 ctb=d2 tag=18 hlen=2 plen=118 new-ctb
:encrypted data packet:
        length: 118
        mdc_method: 2
# off=117 ctb=a3 tag=8 hlen=1 plen=0 indeterminate
:compressed packet: algo=1
# off=119 ctb=cb tag=11 hlen=2 plen=74 new-ctb
:literal data packet:
        mode b (62), created 1611380080, name="",
        raw data: 68 bytes

Relevant prefs from gnupg.conf on Linux:

personal-cipher-preferences AES256 AES192 AES
personal-digest-preferences SHA512 SHA384 SHA256
personal-compress-preferences ZLIB BZIP2 ZIP Uncompressed
default-preference-list SHA512 SHA384 SHA256 AES256 AES192 AES ZLIB BZIP2 ZIP Uncompressed
cert-digest-algo SHA512
s2k-digest-algo SHA512
s2k-cipher-algo AES256
charset utf-8

The only change made was a newline removed from the end of the file, and the only visible differences in the packets to my untrained eyes is that I use compression on Linux, and the file has a name on Android. Otherwise they appear identical.

msfjarvis commented 3 years ago

This is definitely an OpenKeychain bug, but their development has been too unreliable to rely on them for fixing these things. Forking introduces a rather large maintenance burden which I am not entirely confident that I want to take on, but sooner than later it might become a necessity. Increasing priority in the mean time.

sevmonster commented 3 years ago

Ultimately it seems their support for ed25519/cv25519 is just lacking in general... Which is a shame, as they are still very secure despite having smaller keys and operations being soo much faster on my devices.

msfjarvis commented 3 years ago

It looks like there is a pull request for fixing this specific bug, I've generated a release build for people willing to test it (source for it is available at https://github.com/android-password-store/open-keychain). You'll need to uninstall the Play Store/F-Droid version of OpenKeychain before installing this one.

fmeum commented 3 years ago

@ds6 Are you using a YubiKey 5 with firmware version 5.2.6 by any chance?

This is not a bug in OpenKeychain, but rather a hardware bug in the YubiKey 5 series before firmware version 5.2.8 (not yet released). I informed Yubico about this issue in September 2020 (see https://bugs.chromium.org/p/chromium/issues/detail?id=1120933#c10), but they have not released a fix yet. Once a fixed firmware version is available, you should be able to get a replacement from Yubico.

That said, the fix is simply to retry the OID lookup with the last byte stripped off if it fails the first time. I am running this locally and have had no issues with my YubiKey 5 NFC since.

MoritzMaxeiner commented 3 years ago

It looks like there is a pull request for fixing this specific bug, I've generated a release build for people willing to test it (source for it is available at https://github.com/android-password-store/open-keychain). You'll need to uninstall the Play Store/F-Droid version of OpenKeychain before installing this one.

Thank you, but both your linked APK, as well as building OpenKeychain myself from master branch and applying the linked fix to it yield a different error:

Screenshot_20210211-022642

msfjarvis commented 3 years ago

It looks like there is a pull request for fixing this specific bug, I've generated a release build for people willing to test it (source for it is available at https://github.com/android-password-store/open-keychain). You'll need to uninstall the Play Store/F-Droid version of OpenKeychain before installing this one.

Thank you, but both your linked APK, as well as building OpenKeychain myself from master branch and applying the linked fix to it yield a different error:

Screenshot_20210211-022642

That's weird... Can you also try out the development branch of APS? If it still fails, please try to get a logcat.

codewiz commented 3 years ago

Not sure if this is the same bug, but today I created an EdDSA subkey for authentication in OpenKeychain, exported it to my ssh server, then tried to use it in PasswordStore, and authentication fails.

No security token is necessary to trigger the bug. Authentication works when using an RSA 3072 bit subkey.

msfjarvis commented 3 years ago

OpenKeychain v5.6 is out, can the people who've reported bugs here check if any of their problems are fixed?

MoritzMaxeiner commented 3 years ago

OpenKeychain v5.6 is out, can the people who've reported bugs here check if any of their problems are fixed?

Using:

I also get the unknown key format error mentioned here, logcat captured using Android Studio default settings here: yubikey_5_curve25519_password_store_logcat.txt

So, the initial error relating to the yubikey's hardware bug is solved, but the other one remains.

msfjarvis commented 3 years ago

OpenKeychain v5.6 is out, can the people who've reported bugs here check if any of their problems are fixed?

Using:

  • OpenKeychain 5.6 from google playstore (not available yet on fdroid)
  • Password Store develop branch, commit 051d455, built as freeDebug

I also get the unknown key format error mentioned here, logcat captured using Android Studio default settings here: yubikey_5_curve25519_password_store_logcat.txt

So, the initial error relating to the yubikey's hardware bug is solved, but the other one remains.

Thanks for the log, I'll take a look.

msfjarvis commented 3 years ago

https://github.com/open-keychain/open-keychain/blob/0ec0c34cb996e3bd848f0db111dca7da0db98c54/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/SshPublicKey.java#L41-L57

yeah we can't do much here :(

msfjarvis commented 3 years ago

New APK with this PR included, source pushed to android-password-store/open-keychain, and the signature is the same as the APS binaries from Google Play and GitHub Releases.

Should fix ED25519 support, please let me know here and drop a comment in the linked PR if it solves things for you.

tulir commented 3 years ago

Added the PR to my own build (I had already built open-keychain/open-keychain#2631 myself earlier), can confirm it works with Yubikey 5C NFC

MoritzMaxeiner commented 3 years ago

Should fix ED25519 support, please let me know here and drop a comment in the linked PR if it solves things for you.

Works for me (also Yubikey 5C NFC). Thank you :)

sevmonster commented 3 years ago

Yep, everything is resolved with these series of patches. I finally feel that I can safely use my YubiKey on all my devices thanks to this setup solving it in Termux w/ OkcAgent for ssh/gpg and APS for pass.

Thank you everyone for your hard work.

msfjarvis commented 3 years ago

The fix for the Yubikey bug has landed upstream 🎉

sevmonster commented 3 years ago

The fix for the Yubikey bug has landed upstream 🎉

Excellent. But https://github.com/open-keychain/open-keychain/pull/2662 is still not merged, so I think I will be sticking to the fork for a while longer.

msfjarvis commented 3 years ago

open-keychain/open-keychain#2662 has now been merged so this issue can be closed as soon as a new release is published for OpenKeychain.

msfjarvis commented 3 years ago

OpenKeychain v5.7 has been tagged.