search

LedgerHQ / app-openpgp

OpenPGP Card Application
Apache License 2.0
123 stars 21 forks source link

OpenPGP implementation issue : should reject unsupported curves #66

Closed antonio-fr closed 8 months ago

antonio-fr commented 3 years ago

There's an OpenPGP implementation issue in this app. I'm adding OpenPGP devices using the OpenPGPpy library, in the uniblow wallet. And I faced a strange behavior.

Here's a little recap on how I found out this issue :

I can't make the SIG key generation work. It works well for the ENC key there, but no luck with the SIG key. When generating a key, there is always this error message "no data found in this slot".

I seek at 2 points where the issue can be :

ECDSA with secp256k1 works perfectly on a Yubico 5. Also, as the developer of this current app is using it "day to day with ed-25519", I checked with this setting, and it works right with Ed25519 SIG key generation (curve OID 1.3.6.1.4.1.11591.15.1 = h"2B060104019755010501").

So, there's an issue with others curves. Either when reading OID code (function gpg_oid2curve), or when reading the associated public key (keygpg->pub_key.ecfp256).

Finally, I found out that many curves are commented, including the 256k1 curve. This appears to be like this from the very start of this app, 4 years ago. No. This comes from this October 2018 commit, where the author "removed code belonging curves other than Ed25519/NISTP256", leaving only Ed/X25519 and 256r1 curves.

So the issue is not about the "reference not found issue", but the command before, when we setup the key slot, and the app responds OK. It should responds not supported by using the status code "incorrect data". The app blindly copies the data given for the key algorithm to use, and checks it only during the generation of the key. During this generation, if the algorithm data setup is not correct, not recognized, then it throws the key slot is not found. But this is incorrect, as the OpenPGP 3.3.1 standard states the app "should reject unsupported values in the data [algorithm attributes] object" (§4.4.3.7 Algorithm Attributes). This is the way the computer software knows that this algorithm desired (sent in the C1 DO writing) will not work and the app is not compatible with this algorithm.

Here's the OpenPGPpy debug output annotated :


 - Ledger Nano X 0

Select App
 Sending 0xA4 command with 6 bytes data
-> 00 A4 04 00 06 D2 76 00 01 24 01
 Received 0 bytes data : SW 0x9000 - duration: 5.0 ms

An OpenPGP applet detected, using Ledger Nano X 0

Get Application Related Data
Read Data  in 0x006E
Sending 0xCA command with 0 bytes data
-> 00 CA 00 6E 00
 Received 233 bytes data : SW 0x9000 - duration: 23.9 ms
<- 4F 10 D2 76 00 01 24 01 03 03 2C 97 C8 A5 42 41 00 00 5F 52 0F 00 31 C5 73 C0 01 80 07 90 00 00 00 00 00 00 7F 66 08 02 02 00 FE 02 02 00 FE 73 81 B7 C0 0A 7E 00 00 FE 0A 00 02 00 00 01 C1 06 01 08 00 00 20 01 C2 06 01 08 00 00 20 01 C3 06 01 08 00 00 20 01 C4 07 01 0C 0C 0C 03 00 03 C5 3C 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 C6 3C 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 CD 0C 00 00 00 00 00 00 00 00 00 00 00 00

Get Full Application Identifier
Read Data  in 0x004F
 Sending 0xCA command with 0 bytes data
-> 00 CA 00 4F 00
 Received 16 bytes data : SW 0x9000 - duration: 12.0 ms
<- D2 76 00 01 24 01 03 03 2C 97 xx xx xx xx 00 00

Get Extended Length info
Read Data  in 0x7F66
 Sending 0xCA command with 0 bytes data
-> 00 CA 7F 66 00
 Received 8 bytes data : SW 0x9000 - duration: 4.0 ms
<- 02 02 00 FE 02 02 00 FE

Get optional features
Read Data  in 0x7F74
 Sending 0xCA command with 0 bytes data
-> 00 CA 7F 74 00
 Received 0 bytes data : SW 0x6A88 - duration: 4.0 ms

Read public key : SIG key = 0xB600
Sending 0x47 command with 2 bytes data
-> 00 47 81 00 02 B6 00
 Received 0 bytes data : SW 0x6A88 - duration: 5.0 ms
No existing key -> Need to generate one

send "admin PIN", PIN3
 Sending 0x20 command with 8 bytes data
-> 00 20 00 83 08 31 32 33 34 35 36 37 38
 Received 0 bytes data : SW 0x9000 - duration: 15.4 ms

Set SIG key C1 as ECP256k1, EC with OID = 1.3.132.0.10
Put data h132B8104000A in 0x00C1
 Sending 0xDA command with 6 bytes data
-> 00 DA 00 C1 06 13 2B 81 04 00 0A
 Received 0 bytes data : SW 0x9000 - duration: 9.0 ms <- Issue is actually here
Card should throw 0x6A80 status word "incorrect data parameter" to signal the desired algorithm for this key slot is not valid.

Generates Assymetric Key, SIG key = B600
 Sending 0x47 command with 2 bytes data
-> 00 47 80 00 02 B6 00
 Received 0 bytes data : SW 0x6A88 - duration: 4.6 ms
-> Error status : 0x6A88
REFERENCED DATA NOT FOUND, unexplained error

For the fun and as a demonstration of the issue, I sent the encoded OID = "FUCK_OFF"

Put data "x\13FUCK_OFF" in 0x00C1 Sending 0xDA command with 9 bytes data -> 00 DA 00 C1 09 13 46 55 43 4B 5F 30 46 46 Received 0 bytes data : SW 0x9000 - duration: 12.3 ms Reading the Application Related Data Object Read Data in 0x006E Sending 0xCA command -> 00 CA 00 6E 00 Received 236 bytes data : SW 0x9000 - duration: 30.1 ms <- 4F 10 D2 76 00 01 24 01 03 03 2C 97 FC 92 1A 89 00 00 5F 52 0F 00 31 C5 73 C0 01 80 07 90 00 00 00 00 00 00 7F 66 08 02 02 00 FE 02 02 00 FE 73 81 BA C0 0A 7E 00 00 FE 0A 00 02 00 00 01 C1 09 13 46 55 43 4B 5F 30 46 46 C2 06 01 08 00 00 20 01 C3 06 01 08 00 00 20 01 C4 07 01 0C 0C 0C 03 00 03 C5 3C 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 C6 3C 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 CD 0C 00 00 00 00 00 00 00 00 00 00 00 00

The app responds "OK I copied this data in the SIG key algorithm attributes", and is very rude when one reads the data, in the C1 DO.

I suggest to enforce the OID check during the PUT DATA C1/2/3 command. That means calling _gpgoid2curve with the data, and checking whether it returns CX_CURVE_NONE or not. basically, this is a copy/paste from the code in GENERATE command, where it reads and checks the setup OID during this command. One must add something like this around these lines, for each C1-2-3 slots :

curve = gpg_oid2curve(data...);
if (curve == CX_CURVE_NONE) { // or !=

I also suggest to check data length bound and consistency with data provided, before writing in the memory.

Then, what is the rational of having the 256k1 and others curves not supported by the app? Why curves other than Ed25519/NISTP256 were removed at some point? Can they be activated easily? by just removing the comment?

By the way, there is a typo in this code comment (384r1)

As we're here to check OID information by the app (at least during the right stage), one has to consider there are 2 different 25519 curves. To start, the second curve comment is not updated with the correct OID : 1.3.6.1.4.1.3029.1.5.1. First comment ..15.1 is mostly correct : Twisted Edwards Curve 25519 for Ed25519 Second comment ...1.5.1 is not correct, as this is the Montgomery Curve25519 for X25519 (not Ed25519)

This code comments should be :

// "twisted" curve25519 for Ed25519 : 1.3.6.1.4.1.11591.15.1
const unsigned char C_OID_Ed25519[9] = {
    0x2B, 0x06, 0x01, 0x04, 0x01, 0xDA, 0x47, 0x0F, 0x01,
};

// curve25519 for X25519 : 1.3.6.1.4.1.3029.1.5.1
const unsigned char C_OID_cv25519[10] = {
    0x2B, 0x06, 0x01, 0x04, 0x01, 0x97, 0x55, 0x01, 0x05, 0x01,
};
And then the app should check the correct curve parameter for the right key. That means 1.3.6.1.4.1.11591.15.1 for Ref1 C1 (B6/SIG/CDS key) and Ref3 C3 (A4/AUT key) slots, and use 1.3.6.1.4.1.3029.1.5.1 for Ref2 C2 (B8/ENC/DEC key) slot. curve \ slot C1 C2 C3
Ed25519 OK X OK
X25519 X OK X

OK : PUT DATA in key slot performed and 0x9000 "OK" X : PUT DATA in key slot rejected with 0x6A80 "incorrect data"

_And the same applies for all algorithms: ECDSA for C1 and C3, RSA for C1, C2 and C3. I'm thinking of a practical approach would be to add a second argument input describing the use "SIG" or "ENC", or slot 1-2-3 in the gpgoid2curve and this function filters with this info, giving back None when the current OID is not matching the use. Wrong, see below

antonio-fr commented 3 years ago

About the very last point, after careful thinking, there is no issue. Because what the user inputs in the algorithm Cx slots is not the algorithm but the curve (for EC), and all the curves can be used for ECDSA, and also ECDH exchange. So no need to filter out, as RSA and ECDSA can work whatever the key slot and its use. Except for the "25519" familly, where the "twisted" curve used for signing (SIGN/AUT), and the other "original" used for key exchange (ENC) are slightly different. Yes, in a way there are the same with a bijection, but their parameters are different, and they have different OID.

antonio-fr commented 3 years ago

Related to that, there are lot of discrepancies between the docs and the code
In the doc :

The application supports:


In the repository Readme (front page) : The application supports:


In the code :

That means for EC :

antonio-fr commented 3 years ago

Any feedback on this ?

cedelavergne-ledger commented 8 months ago

Hi, Will check all that soon...

cedelavergne-ledger commented 8 months ago

Hi, Most of the points are now fixed in v2.1 (still in develop branch). There is still a limitation: Decrypt with cv25519 dosen't work. This issue will be tracked separately.