Yubico / ykneo-openpgp

OpenPGP applet for the YubiKey NEO
https://developers.yubico.com/ykneo-openpgp/
GNU General Public License v2.0
215 stars 67 forks source link

Support for private use data objects #33

Closed josecastillo closed 7 years ago

josecastillo commented 9 years ago

This pull request adds support for the four private use data objects defined in the OpenPGP smart card specification. These data objects can be set with the privatedo command in gpg's card-edit facility. While optional, they are useful for some workflows; in particular, data object 0103 can be used by TrueCrypt to store a passphrase for symmetric full disk encryption.

I've added test coverage for each of the data objects, and you'll notice that the testPrivateUseDo4 case appears to fail. This apparent failure is due to a known bug in jCardSim; the logic I've implemented in the applet is sound, and this test case succeeds when the applet is tested on a physical card. Once the jCardSim issue is addressed, the test case should succeed in the simulator as well.

This pull request also contains, in a separate commit, a minor fix to an error status message. When a user attempts to perform a security operation with an uninitialized key, the official OpenPGP smart card returns a status of 6A88, "referenced data not found". This is more descriptive than the status that the applet currently returns (6985, "conditions not satisfied"). I'm working on improved error messages in OpenKeychain after a user experienced this issue, and returning the correct error code in this case will allow the OpenKeychain UI to display the correct message.

I am making this contribution under the terms of the GPL 2+ license.

kasparsd commented 8 years ago

:+1:

mouse07410 commented 8 years ago

I stand by my already-expressed opinion that using a smart card like YubiKey as "a little more secure USB storage" is not a smart design. And YubiKey is an exception in that it actually allows users to write to it. Most enterprise-issued PIV tokens would not allow that - and for a good reason.

The right way to utilize the capabilities of Yubikey would be wrapping the TrueCrypt disk key with its KEY MAN key.

a-dma commented 7 years ago

Hey,

sorry for the inexcusable delay on this. I think adding this makes sense and would put this in parity with Yk4 which already supports PDO's.

I see that the test for PDO 4 fails. Would you mind looking into that or should I? Travis reports this

[junit] Testcase: testPrivateUseDo4 took 0.011 sec
[junit]     FAILED
[junit] array lengths differed, expected.length=2 actual.length=10
[junit] junit.framework.AssertionFailedError: array lengths differed, expected.length=2 actual.length=10
[junit]     at openpgpcardTest.OpenPGPAppletTest.testPrivateUseDo4(Unknown Source)
josecastillo commented 7 years ago

The failure is due to a bug in jCardSim 2.2.2 (https://github.com/licel/jcardsim/issues/67); the simulator was not clearing transient memory, and so was erroneously allowing access to DO 4 after a card reset. Testing the same scenario on a physical card succeeds, since the card correctly enforces the access conditions. This behavior was fixed in the jCardSim master repo, but they haven't done a release.

You could fix the build by having Travis pull in the 3.0.4 snapshot to test with, but I hadn't updated this issue since I figured it would make more sense to wait for jCardSim to do a proper release.

a-dma commented 7 years ago

Makes sense, I'll do a few more tests on my side and try to merge this ASAP.

Again, sorry for the humongous amount of delay

a-dma commented 7 years ago

Hi, the tests have a slight problem. They check that doVerify returns true, but that's a void function. You want to test against a Status.GOOD value.

-               assertEquals(true, doVerify("123456", (byte) 0x82));
+               doVerify("123456", (byte) 0x82, State.GOOD);

Can you fix them or do you want me to do it?

I've also updated travis to use JCardSim 3.0.4 which sould fix the checks.

jonathancross commented 7 years ago

Hi All, sounds like the only issue pending here is this one line change? @a-dma any reason you can't just make the modification and merge? Thank you both for this!