Decryption no longer works with Yubikey Neo: SecureMessagingException

[nh2]

When I try to decrypt with my Yubikey Neo over NFC (which in the past, worked fine at some point), I get an error. For example, when using the decrypt-from-clipboard feature, or when using another program like android-password-store/Android-Password-Store#271.

Expected Behavior

Decryption succeeds.

Current Behavior

I now get Error: Transceive failed in the UI and the following stack trace in adb logcat:

E/Keychain(13043): failed to establish secure messaging
E/Keychain(13043): org.sufficientlysecure.keychain.securitytoken.SecureMessagingException: failed to retrieve secure messaging key attributes
E/Keychain(13043): 	at org.sufficientlysecure.keychain.securitytoken.SCP11bSecureMessaging.establish(SCP11bSecureMessaging.java:298)
E/Keychain(13043): 	at org.sufficientlysecure.keychain.securitytoken.SecurityTokenHelper.connectToDevice(SecurityTokenHelper.java:211)
E/Keychain(13043): 	at org.sufficientlysecure.keychain.ui.base.BaseSecurityTokenActivity.handleSecurityToken(BaseSecurityTokenActivity.java:444)
E/Keychain(13043): 	at org.sufficientlysecure.keychain.ui.base.BaseSecurityTokenActivity$1.doInBackground(BaseSecurityTokenActivity.java:172)
E/Keychain(13043): 	at org.sufficientlysecure.keychain.ui.base.BaseSecurityTokenActivity$1.doInBackground(BaseSecurityTokenActivity.java:162)
E/Keychain(13043): 	at android.os.AsyncTask$2.call(AsyncTask.java:288)
E/Keychain(13043): 	at java.util.concurrent.FutureTask.run(FutureTask.java:237)
E/Keychain(13043): 	at android.os.AsyncTask$SerialExecutor$1.run(AsyncTask.java:231)
E/Keychain(13043): 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1112)
E/Keychain(13043): 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:587)
E/Keychain(13043): 	at java.lang.Thread.run(Thread.java:841)

Possible Solution

Not sure, but this is definitely a regression, it worked in the past.

I wonder if it was this commit that broke it -- simply by the fact that it was the last one touching the code that raises the above exception.

Steps to Reproduce (for bugs)

1. Get a Yubikey Neo (for OpenKeychain devs: I'll buy you one if you don't have one)
2. Encrypt a message to yourself to the clipboard
3. Use decrypt from clipboard
4. Tap the Yubikey via NFC, see it fail

Your Environment

• Android Version: 4.4
• OpenKeychain Version: 4.2.4
• From Google Play or F-Droid?: Google Play

[nh2]

Wait, Google Play Store tricked me, the version shown there in the permissions window is the latest available version, not the version I have installed (I consider that a UI bug).

I'm using version 4.2.1, not 4.2.4.

But the problem remains the same.

[dschuermann]

@af-anssi Can you look into this?

[nh2]

I can confirm that it works with v4.1, which I just compiled.

[nh2]

I'm bisecting now.

[dschuermann]

BTW: Before releasing the changes of @af-anssi I tested with YubiKey NEO and Fidesmo card, but maybe I missed a special version or corner case.

[nh2]

It seems like the commit I suspected isn't the culprit, but we'll know in a couple minutes when the bisect is done.

It's being made a bit harder due to non-compiling commits in the history like wip: t1 tpdu loe

[nh2]

ee8cd3862f65de580ed949bc838628610e22cd98 is the first bad commit
commit ee8cd3862f65de580ed949bc838628610e22cd98
Author: Nikita Mikhailov <nikita.s.mikhailov@gmail.com>
Date:   Thu May 12 01:17:21 2016 +0600

    wip: Working nitrokey pro

:040000 040000 700283b478530e87b31fd7e7bd8efc6d742d25d9 b750e29500e19e796ddbc172360e2d43e29c629f M	OpenKeychain

Commit ee8cd38

[nh2]

I've manually confirmed that this commit is indeed what breaks it.

[dschuermann]

Are you sure that this produces the same exception?

[nh2]

No, and I think it can't, because that exception was introduced later.

adb logcat on the commit that first broke it has no exception; it prints:

D/Keychain D(13119): nfcGetFingerprints() Iso7816TLV tlv data:
D/Keychain D(13119): composite tag T   6e L 0222
D/Keychain D(13119):   tag T   4f L 0016
D/Keychain D(13119):   tag T 5f52 L 0015
D/Keychain D(13119):   composite tag T   73 L 0183
D/Keychain D(13119):     tag T   c0 L 0010
D/Keychain D(13119):     tag T   c1 L 0006
D/Keychain D(13119):     tag T   c2 L 0006
D/Keychain D(13119):     tag T   c3 L 0006
D/Keychain D(13119):     tag T   c4 L 0007
D/Keychain D(13119):     tag T   c5 L 0060
D/Keychain D(13119):     tag T   c6 L 0060
D/Keychain D(13119):     tag T   cd L 0012
D/Keychain D(13119): nfcGetFingerprints() Iso7816TLV tlv data:
D/Keychain D(13119): tag T   90 L 0000

[nh2]

In the commit before it which works, it prints:

D/Keychain D(13390): nfcGetFingerprints() Iso7816TLV tlv data:
D/Keychain D(13390): composite tag T   6e L 0222
D/Keychain D(13390):   tag T   4f L 0016
D/Keychain D(13390):   tag T 5f52 L 0015
D/Keychain D(13390):   composite tag T   73 L 0183
D/Keychain D(13390):     tag T   c0 L 0010
D/Keychain D(13390):     tag T   c1 L 0006
D/Keychain D(13390):     tag T   c2 L 0006
D/Keychain D(13390):     tag T   c3 L 0006
D/Keychain D(13390):     tag T   c4 L 0007
D/Keychain D(13390):     tag T   c5 L 0060
D/Keychain D(13390):     tag T   c6 L 0060
D/Keychain D(13390):     tag T   cd L 0012
D/Keychain D(13390): nfcGetFingerprints() Iso7816TLV tlv data:
D/Keychain D(13390): tag T   90 L 0000

(no difference in output).

[dschuermann]

Please try if it works with 27670d5 which is a commit before @af-anssi changes were merged but after your identified commit.

[nh2]

On that commit, I can't git submodule update --init:

fatal: reference is not a tree: 03b00625eb29c7e6c5aa6e131bd0d64e574a7886
Unable to checkout '03b00625eb29c7e6c5aa6e131bd0d64e574a7886' in submodule path 'extern/openpgp-api-lib'

[nh2]

@dschuermann Does that happen for you too?

[af-anssi]

I have no Yubikey Neo to try to reproduce this issue. The stack trace related to secure messaging is surprising as I didn't know Yubikey Neo was using it... Nevertheless, it is not the cause of the problem since the exception is handled properly and execution continues as if there were no secure messaging available.

[nh2]

@af-anssi I can order you one if you're up for it! Email me at the address written on nh2.me

[af-anssi]

Thank you, but I think we can first try to work with some more log. Could you post a more complete log related to OpenKeychain (grep Keychain on the logcat output) when the bug arises ?

[dschuermann]

BTW: I wasn't able to reproduce this with my YubiKey NEOs. @nh2 Are you using a special setup/applet? When have you bought the YubiKey? Do you know the OpenPGP applet version?

[nh2]

@af-anssi I think I copied all Keychain entries, but I can double check.

@dschuermann:

The OpenPGP applet has version 1.0.10.

The key's firmware version is 3.4.3.

The connection mode is set to OTP+U2F+CCID (if that has any effect on the PGP applet).

[eqyiel]

@af-anssi this is happening to me too, AFAICT the output in the issue description is all that there is to show for grep Keychain:

$ adb logcat | grep Keychain
06-11 18:29:46.935 12187 12242 E Keychain: failed to establish secure messaging
06-11 18:29:46.935 12187 12242 E Keychain: org.sufficientlysecure.keychain.securitytoken.SecureMessagingException: failed to retrieve secure messaging key attributes
06-11 18:29:46.935 12187 12242 E Keychain:      at org.sufficientlysecure.keychain.securitytoken.SCP11bSecureMessaging.establish(SCP11bSecureMessaging.java:298)
06-11 18:29:46.935 12187 12242 E Keychain:      at org.sufficientlysecure.keychain.securitytoken.SecurityTokenHelper.connectToDevice(SecurityTokenHelper.java:211)
06-11 18:29:46.935 12187 12242 E Keychain:      at org.sufficientlysecure.keychain.ui.base.BaseSecurityTokenActivity.handleSecurityToken(BaseSecurityTokenActivity.java:444)
06-11 18:29:46.935 12187 12242 E Keychain:      at org.sufficientlysecure.keychain.ui.base.BaseSecurityTokenActivity$1.doInBackground(BaseSecurityTokenActivity.java:172)
06-11 18:29:46.935 12187 12242 E Keychain:      at org.sufficientlysecure.keychain.ui.base.BaseSecurityTokenActivity$1.doInBackground(BaseSecurityTokenActivity.java:162)
06-11 18:29:46.935 12187 12242 E Keychain:      at android.os.AsyncTask$2.call(AsyncTask.java:305)
06-11 18:29:46.935 12187 12242 E Keychain:      at java.util.concurrent.FutureTask.run(FutureTask.java:237)
06-11 18:29:46.935 12187 12242 E Keychain:      at android.os.AsyncTask$SerialExecutor$1.run(AsyncTask.java:243)
06-11 18:29:46.935 12187 12242 E Keychain:      at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1133)
06-11 18:29:46.935 12187 12242 E Keychain:      at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:607)
06-11 18:29:46.935 12187 12242 E Keychain:      at java.lang.Thread.run(Thread.java:761)

That's what it says when I try to import the key on a new device.

[af-anssi]

This is really strange. This code can be triggered only when the "secure messaging" bit is set in the "extended capabilities" retrieved from the token. There are only two reasons for this bit to be set: the implementation is really capable of secure messaging which is curious as I found no information on that, or this bit is used for another purpose in this implementation which makes it not compliant with the OpenPGP card specification. Did you set some specific features during the personnalization of your token(s) ?

[nh2]

Did you set some specific features during the personnalization of your token(s) ?

@af-anssi I don't know if that helps, but here's the screenshot from the personalisation GUI:

I'm happy to provide other info that might be useful.

[af-anssi]

Actually I was wrong about the implementation used in Yubikey Neo: it supports secure messaging with DES as described in OpenPGP specification v2.x. I have fixed the code not to trigger the SmartPGP secure messaging code in case the implemented secure messaging relies on DES. This is a temporary workaround to fix the regression I introduced (sorry). I will have to find a better way to detect SmartPGP secure messaging...

[nh2]

I would like to try this out, but can't build it due to #2124

[Valodim]

Just hold your yubikey to your phone while OpenKeychain is open :)

[nh2]

Ok, branch is here:

@Valodim That branch successfully decrypts for me! Looks like the 40 lines in 3d217ef really contain the difference.

[Valodim]

Haha, oh man. This makes zero sense to me :)

I changed the branch structure a bit, can you bisect the bunch of commits to see which one actually fixes this? Really curious about what caused this.

[nh2]

can you bisect the bunch of commits to see which one actually fixes this?

Seems to really be 3d217ef, with its direct parent c39aa99 I cannot decrypt.

[Valodim]

I meant I split up this commit, please update the branch (git fetch; git reset --hard origin/experiment-token-ee8cd38)

[nh2]

Ah, sorry, I thought you were referring to the refactorings in #2252.

I'll let you know in a couple minutes.

[nh2]

@Valodim

• e3f4950 works

the one before:

• 02e677d doesn't work

So looks like this is our contender for the fix:

     public CommandApdu createDecipherCommand(byte[] data) {
-        return CommandApdu.create(CLA, INS_PERFORM_SECURITY_OPERATION, P1_PSO_DECIPHER, P2_PSO_DECIPHER, data,
-                MAX_APDU_NE_EXT);
+        return CommandApdu.create(CLA, INS_PERFORM_SECURITY_OPERATION, P1_PSO_DECIPHER, P2_PSO_DECIPHER, data);
     }

[nh2]

@Valodim Interestingly, applying that single patch e3f4950 on top of latest master == c34a64fa (git checkout c34a64fa && git cherry-pick --no-commit 3ca064d e3f4950) does NOT make latest master work, so it seems to be a combination of the patches needed.

[Valodim]

Does it work with that one plus 02e677d?

[nh2]

@Valodim Yes.

[Valodim]

Heh, this makes no sense :) What we currently have is perfectly compliant to the spec.

But -- I guess we'll just change our behavior here, if it increases compatibility and doesn't break anything else. We'll publish this in a beta sometime soon (sign up for our beta channel, if you haven't already).

Thanks for your super thorough bug reporting!

[dschuermann]

Awesome debugging @Valodim and @nh2 . Nice read!

[Valodim]

@nh2 The patch is in the beta that was released today. Please test and report 👍

[nh2]

I can confirm that it's working fine with the beta, 4.9 (49001) for me.

Thanks everyone for the hard work on this.

[Valodim]

Well, this is awkward. So in commit 1c8cc99 I removed the Le field, which was required for some reason for @nh2's Yubikey. Testing with an OpenPGP card v3.3 however, the field needs to be set to the actually expected size or the maximum size, or decryption won't work :\

The spec on page 59 clearly says the Le value should be 0. Setting it to 0 doesn't work for me with the openpgp card, neither does leaving it out.

So basically we have four options:

1. don't send Le. This breaks the OpenPGP card, but works with the Yubikey from this issue
2. send Le of 0 - this is what the spec says. This doesn't work with the OpenPGP card I have, even though the source code indicates that it should (but I don't know if the code linked from foss-shop is the one actually on the card).
3. send Le of the expected size - this is what GnuPG does. This works for the OpenPGP card, no idea about nh2's yubikey
4. send an Le value of MAX. This works for the OpenPGP card as well, and works for the sign command, but didn't work for nh2's yubikey

It's also worth pointing out that most cards don't seem to care whether the field is present and what its value is. I guess we can do what GnuPG does (as opposed to what's in the spec), and hope that this yields the best compatibility?

[nh2]

@Valodim I'm happy to try out versions for those options where you don't know yet where Yubikeys like mine would react, if that helps.

[Valodim]

Thanks! I made an experimental branch, please check out #2298. I tested this with a Yubikey Neo, Nitrokey Pro, Gnuk, and OpenPGP card v3.3.

[nh2]

#2298 (comment)

OK, I tested the commit; unfortunately I get the Transceive failed with my Yubikey Neo.

[nh2]

From #2298 (comment):

That commit 9a86d45 makes it work