SwitchEV / RISE-V2G

The only fully-featured reference implementation of the Vehicle-2-Grid communication interface ISO 15118
MIT License
220 stars 92 forks source link

GenerateSharedSecret - invalid coordinate #41

Closed calvinsantana closed 4 years ago

calvinsantana commented 4 years ago

Dear Marc,

Not sure if this is a bug and I know it is not of your responsibility but when I generate a new public/private EC key pair (using openSSL c++ API) to encrypt ContractCertPrivateKey I get the following exception on RISE-V2G (evcc):

Exception in thread "Thread-0" java.security.ProviderException: invalid coordinate at jdk.crypto.ec/sun.security.ec.ECDHKeyAgreement.validateCoordinate(ECDHKeyAgreement.java:118) at jdk.crypto.ec/sun.security.ec.ECDHKeyAgreement.validate(ECDHKeyAgreement.java:137) at jdk.crypto.ec/sun.security.ec.ECDHKeyAgreement.deriveKeyImpl(ECDHKeyAgreement.java:222) at jdk.crypto.ec/sun.security.ec.ECDHKeyAgreement.engineGenerateSecret(ECDHKeyAgreement.java:169) at java.base/javax.crypto.KeyAgreement.generateSecret(KeyAgreement.java:598) at com.v2gclarity.risev2g.shared.utils.SecurityUtils.generateSharedSecret(SecurityUtils.java:1278) at com.v2gclarity.risev2g.shared.utils.SecurityUtils.decryptContractCertPrivateKey(SecurityUtils.java:1507) at com.v2gclarity.risev2g.evcc.states.WaitForCertificateInstallationRes.processIncomingMessage(WaitForCertificateInstallationRes.java:77) at com.v2gclarity.risev2g.evcc.session.V2GCommunicationSessionEVCC.update(V2GCommunicationSessionEVCC.java:183) at java.base/java.util.Observable.notifyObservers(Observable.java:173) at com.v2gclarity.risev2g.evcc.transportLayer.StatefulTransportLayerClient.processIncomingMessage(StatefulTransportLayerClient.java:124) at com.v2gclarity.risev2g.evcc.transportLayer.TLSClient.run(TLSClient.java:169) at java.base/java.lang.Thread.run(Thread.java:834)

Where:

private static void validateCoordinate(BigInteger c, BigInteger mod) { if (c.compareTo(BigInteger.ZERO) < 0) { throw new ProviderException("invalid coordinate"); } `` if (c.compareTo(mod) >= 0) { throw new ProviderException("invalid coordinate"); } }

The only way to avoid this exception is to force that the generated public key has both it's coordinates positive.

With this workaround CertificateInstallation always works, but I don't believe that such a workaround should be needed. Analysing the elliptic curve and its domain parameters it's seems reasonable that the coordinates could be lower than zero.

At the end of the day I don't know if this bug is on the generate key pair of openssl API or on the sun.security API side.

Would like to hear your opinion and for the sake of the security of PnC this possible issue should be clarified.

MarcMueltin commented 4 years ago

Dear @calvinsantana, thanks for reporting this issue. At first sight, I believe it's a bug in the OpenSSL API, but I would have to spend more time to dig deeper into this issue and see if I can help solve the problem.

But you are right regarding the coordinate: it can also be negative, there's no reason why it shouldn't be possible.

I have it on my TODO list and will get back to you asap.

oliviermartin commented 4 years ago

As I had the same error in my project, I was googling for this error and ended up on this github issue - the only result for this issue.

I finally found my issue (all good from OpenSSL and sun.security), you have to make sure you load the X and Y of your ECPoint as positive number (default is signed number).

MarcMueltin commented 4 years ago

@oliviermartin Can you please elaborate on where exactly you would make a change in the code and explain why the X and Y of my ECPoint must be loaded as positive numbers? It's perfectly fine for an X and Y coordinate of an ECPoint to be negative, so I am not convinced yet that this is the right solution.

oliviermartin commented 4 years ago

Sorry @MarcMueltin , I would need some time (which I do not have right now) to go back into the code. But when I had the issue in my project it was clear this project has the same issue considering the error message raised by @calvinsantana. The change I did fixed mine. But if you are sure your issue is different, I guess I am wrong.

chargehere commented 4 years ago

I could observe the same problem and did therefore a little workaround.

It seemed to me, that the EC Public Key Generator with the Parameter Spec "secp256r1" only produces ECPoints with positive coordinates. But in the SecurityUtils class at the method getPublicKey(byte[] publicKeyBytes) the construction of the PublicKey from the encoded raw bytes returns from time to time a negative signum within the BigInteger(byte[] val) constructor and therefore generates the negative coordinates. My Solution was to change the constructor of the BigInteger(byte[] val) to BigInteger(int signum, byte[] magnitude) to determine a positive EC Point (see below).

KeyFactory kf = KeyFactory.getInstance("EC");

AlgorithmParameters parameters = AlgorithmParameters.getInstance("EC");
parameters.init(new ECGenParameterSpec("secp256r1"));
ECParameterSpec ecParameterSpec = parameters.getParameterSpec(ECParameterSpec.class);

ECPublicKeySpec ecPublicKeySpec = new ECPublicKeySpec(new ECPoint(new BigInteger(1, x), new BigInteger(1, y)), ecParameterSpec);
ECPublicKey ecPublicKey = (ECPublicKey) kf.generatePublic(ecPublicKeySpec);
MarcMueltin commented 4 years ago

@chargehere I agree, this seems to be the easiest and most straight forward solution. I took care of it with commit fa9f2ee.