search

github-af / SmartPGP

SmartPGP is a JavaCard implementation of the OpenPGP card specifications
GNU General Public License v2.0
232 stars 48 forks source link

NIST P-521 broken when running on jcardsim #40

Closed hko-s closed 3 years ago

hko-s commented 3 years ago

When running SmartPGP on jcardsim, both import and key generation fail for NIST P-521.

I have explored and will outline why this happens, but I don't understand the codebases well enough to suggest how the issue should be resolved.

The problem, when it happens, looks like this:

java.lang.ArrayIndexOutOfBoundsException
    at java.lang.System.arraycopy(Native Method)
    at javacard.framework.Util.arrayCopy(Unknown Source)
    at com.licel.jcardsim.crypto.ByteContainer.setBytes(ByteContainer.java:140)
    at com.licel.jcardsim.crypto.ECKeyImpl.setB(ECKeyImpl.java:133)
    at fr.anssi.smartpgp.ECParams.setParams(ECParams.java:75)
    at fr.anssi.smartpgp.PGPKey.importECKey(PGPKey.java:395)
    at fr.anssi.smartpgp.PGPKey.importKey(PGPKey.java:517)
    at fr.anssi.smartpgp.SmartPGPApplet.processPutData(SmartPGPApplet.java:913)
    at fr.anssi.smartpgp.SmartPGPApplet.process(SmartPGPApplet.java:1635)
    at com.licel.jcardsim.base.SimulatorRuntime.transmitCommand(SimulatorRuntime.java:303)
    at com.licel.jcardsim.base.Simulator.transmitCommand(Simulator.java:260)
    at com.licel.jcardsim.base.CardManager.dispatchApduImpl(CardManager.java:66)
    at com.licel.jcardsim.base.CardManager.dispatchApdu(CardManager.java:36)
    at com.licel.jcardsim.remote.VSmartCard$IOThread.run(VSmartCard.java:154)

Why do we get an ArrayIndexOutOfBoundsException?

The problem seems to be that a com.licel.jcardsim.crypto.ECPublicKeyImpl object gets initialized first by jcardsim. Later, new values (one of them with a larger size) are written into this object from ECPublicKeyImpl. The second attempt at setting the value b in the object fails.

The first initalization for key generation is performed at https://github.com/ANSSI-FR/SmartPGP/blob/dcd0d42b7abf2809881eb727ce68f9f5e68c63b7/src/fr/anssi/smartpgp/PGPKey.java#L249 and analogously for import, here: https://github.com/ANSSI-FR/SmartPGP/blob/dcd0d42b7abf2809881eb727ce68f9f5e68c63b7/src/fr/anssi/smartpgp/PGPKey.java#L387-L389

In this first initialization, the value b gets set to 51953eb9618e1c9a1f929a21a0b68540eea2da725b99b315f3b8b489918ef109e156193951ec7e937b1652c0bd3bb1bf073573df883d2c34f1ef451fd46b503f00, for P-521 (note, this is 65 bytes of data, without a leading zero byte).

Later, SmartPGP attempts to set the value of b again, in the same ECPublicKeyImpl object, from https://github.com/ANSSI-FR/SmartPGP/blob/dcd0d42b7abf2809881eb727ce68f9f5e68c63b7/src/fr/anssi/smartpgp/ECParams.java#L60

However, here the value that gets set is 0051953eb9618e1c9a1f929a21a0b68540eea2da725b99b315f3b8b489918ef109e156193951ec7e937b1652c0bd3bb1bf073573df883d2c34f1ef451fd46b503f00 (note: this is 66 bytes, with a leading zero byte, this value comes from SmartPGP in ECConstants.java:239. This constant ansix9p521r1_b happens to be the only one in ECConstants.java that has a leading zero byte).

This second attempt to set b fails, because the underlying jcardsim ByteContainer for b has been sized to 65 bytes when it was first set. The new value is thus too large by one byte (causing the ArrayIndexOutOfBoundsException from above).

Workaround

I have tested that commenting out ECParams.java:60 leads to SmartPGP on jcardsim working for NIST P-521.

I don't know how this should be properly resolved

It's unclear to me if re-setting b at ECParams.java:60 is actually useful in any scenario. Here, it seems to be redundant.

Also, it's unclear to me if and when the leading zero byte of b (for NIST P-521) should be stored.

I don't know enough about how EC cryptography works, but I wonder how leading zero bytes should be treated generally. Should they always be stripped?

Finally: Could there be other related bugs/problems here, when other EC values have leading zero byte(s)? (In particular: the value of the user's public or private key)

af-anssi commented 3 years ago

We have seen several differences between Javacard implementations of P-521.

Could you try to run your test with the following patch applied ? https://github.com/ANSSI-FR/SmartPGP/blob/dcd0d42b7abf2809881eb727ce68f9f5e68c63b7/patches/fix_p521_528_bits.patch

hko-s commented 3 years ago

jcardsim doesn't seem to support that approach:

javacard.security.CryptoException
    at javacard.security.CryptoException.throwIt(Unknown Source)
    at com.licel.jcardsim.crypto.ECKeyImpl.getDefaultsDomainParameters(ECKeyImpl.java:300)
    at com.licel.jcardsim.crypto.ECKeyImpl.<init>(ECKeyImpl.java:67)
    at com.licel.jcardsim.crypto.ECPrivateKeyImpl.<init>(ECPrivateKeyImpl.java:41)
    at javacard.security.KeyBuilder.buildKey(Unknown Source)
    at fr.anssi.smartpgp.PGPKey.importECKey(PGPKey.java:384)
    at fr.anssi.smartpgp.PGPKey.importKey(PGPKey.java:517)
    at fr.anssi.smartpgp.SmartPGPApplet.processPutData(SmartPGPApplet.java:913)
    at fr.anssi.smartpgp.SmartPGPApplet.process(SmartPGPApplet.java:1635)
    at com.licel.jcardsim.base.SimulatorRuntime.transmitCommand(SimulatorRuntime.java:303)
    at com.licel.jcardsim.base.Simulator.transmitCommand(Simulator.java:260)
    at com.licel.jcardsim.base.CardManager.dispatchApduImpl(CardManager.java:66)
    at com.licel.jcardsim.base.CardManager.dispatchApdu(CardManager.java:36)
    at com.licel.jcardsim.remote.VSmartCard$IOThread.run(VSmartCard.java:154)

The exception gets thrown because 528 is not handled in this switch statement:

https://github.com/licel/jcardsim/blob/8cf5c374840744d1f0f8b1aa5c1a6ffe90aa5998/src/main/java/com/licel/jcardsim/crypto/ECKeyImpl.java#L269

af-anssi commented 3 years ago

Ok, this patch is a workaround for some smartcards.

The leading zero is here to have all related parameters the same length in bytes only. It is also present in recommended parameters values (see section 2.9.1 in https://www.secg.org/SEC2-Ver-1.0.pdf)

Something that seems really odd is that in jcardsim ECKeyImpl the parameters are "automatically" set according to the key length, and are assumed to be either sect or secp curves. What about brainpool curves with exact same key sizes (256, 384) ?

From what we see in ECKeyIml it seems brainpoolP512 cannot work at all. Could you test this one ?

hko-s commented 3 years ago

Unfortunately I can't easily test brainpool.

However, just by looking at the jcardsim code, it seems to me that brainpool is not supported in the getDefaultsDomainParameters() function we're looking at.

(I also can't find any references to brainpool with jcardsim on search engines, for whatever that's worth. Maybe noone has used jcardsim with brainpool?)

af-anssi commented 3 years ago

Ok.

For P521 the problem is in jcardsim for two reasons. The first reason is that the buildKey is called with 521 as key size, which means that the underlying implementation must allocate 66 bytes for all parameters (65 bytes is not enough). The second reason is that jcardsim makes a wrong assumption by automatically/implicitly choosing a sect or secp curve, and by automatically settings the parameters. This is not the behavior expected by the JavaCard API.

af-anssi commented 3 years ago

It seems JCAlgtest reports the same problem of index out of bound with jCardSim-3.0.5-SNAPSHOT (previous versions seem to not support ECC).

See line ALG_EC_FP LENGTH_EC_FP_521 at column c53: https://www.fi.muni.cz/~xsvenda/jcalgtest/table.html

hko-s commented 3 years ago

Thank you for your very helpful assessment of the problem!

I think the root mistake in jCardSim is https://github.com/licel/jcardsim/blob/master/src/main/java/com/licel/jcardsim/crypto/ECKeyImpl.java#L63, which seems to directly contradict the javadoc comment for this constructor. However, a number of the integration tests of jCardSim apparently rely on this behavior, so I decided against attempting to change this.

Instead, I have opted for a simple workaround for the specific case that actually fails for me, namely setting a larger byte vector in a ByteContainer: https://github.com/hko-s/jcardsim/commit/ff1b4d934b24e635cb56322ec47ec7a19bebb38e - while this hack isn't entirely satisfying, I don't see how it makes anything in jCardSim worse than it was before.

And with this patch, I can successfully run my testsuite (including for NIST P-521) against SmartPGP running on jCardSim.

(FYI, in case anyone is interested, I have made container images to run CI against SmartPGP with my OpenPGP card client code: https://gitlab.com/hkos/openpgp-card-images)