[8u] RFR Backport: 8208698: Improved ECC Implementation

Alvarez, David alvdavi at amazon.com
Fri Jun 28 06:14:20 UTC 2019


Looks good to me

> On 27 Jun 2019, at 19:53, Andrew John Hughes <gnu.andrew at redhat.com> wrote:
> 
> 
> 
>> On 14/06/2019 23:33, Alvarez, David wrote:
>> Hi,
>> 
>> Please review this backport of JDK-8208698: Improved ECC Implementation
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8208698
>> Original: http://hg.openjdk.java.net/jdk/jdk/rev/752e57845ad2
>> Webrev: http://cr.openjdk.java.net/~phh/8208698/webrev.8u.00/
>> 
>> JDK-8208698 is marked as jdk8u-critical-yes
>> 
>> This is the last of the three patches I was sending today, JDK-8181594, JDK-8208648 and JDK-8208698
>> 
>> This backport is effectively also fixing JDK-8205476. However, JDK-8205476 includes some Javadoc changes and a test this patch is not bringing. If needed, I could backport JDK-8205476 independently and do the webrev, or modify the existing backport to ensure we do not wipe the secret.
>> 
>> There are other reasons why this patch did not apply cleanly
>> 
>> ECDHKeyAgreement.java: These are mostly caused by the missing JDK-8205476
>> ECDSASignature.java: jdk supports SHA224inP1363Format which we don't. I adapted the patch to ignore P1363 references (P1363 format here means unencoded values)
>> ECKeyPairGeneration.java: jdk8u is missing JDK-8182999, so the patching of the initialize method had to be written manually.
>> 
>> Additionally, there were some other compilation changes, mostly to accommodate for newer API methods not present in jdk8u like Optional.isEmpty or Map.of
>> 
>> The changes I did to fix rejects are listed below
>> 
>> Thanks,
>> David
>> 
>> ----
>> 
>> diff --git a/src/jdk/src/share/classes/sun/security/ec/ECDHKeyAgreement.java b/src/jdk/src/share/classes/sun/security/ec/ECDHKeyAgreement.java
>> index e17f8393..5425179f 100644
>> --- a/src/jdk/src/share/classes/sun/security/ec/ECDHKeyAgreement.java
>> +++ b/src/jdk/src/share/classes/sun/security/ec/ECDHKeyAgreement.java
>> @@ -104,40 +104,74 @@ public final class ECDHKeyAgreement extends KeyAgreementSpi {
>>                 ("Key must be a PublicKey with algorithm EC");
>>         }
>> -        ECPublicKey ecKey = (ECPublicKey)key;
>> -        ECParameterSpec params = ecKey.getParams();
>> +        this.publicKey = (ECPublicKey) key;
>> -        if (ecKey instanceof ECPublicKeyImpl) {
>> -            publicValue = ((ECPublicKeyImpl)ecKey).getEncodedPublicValue();
>> -        } else { // instanceof ECPublicKey
>> -            publicValue =
>> -                ECUtil.encodePoint(ecKey.getW(), params.getCurve());
>> -        }
>> +        ECParameterSpec params = publicKey.getParams();
>>         int keyLenBits = params.getCurve().getField().getFieldSize();
>>         secretLen = (keyLenBits + 7) >> 3;
>>         return null;
>>     }
>> -    // see JCE spec
>> -    @Override
>> -    protected byte[] engineGenerateSecret() throws IllegalStateException {
>> -        if ((privateKey == null) || (publicValue == null)) {
>> -            throw new IllegalStateException("Not initialized correctly");
>> +    private static void validateCoordinate(BigInteger c, BigInteger mod) {
>> +        if (c.compareTo(BigInteger.ZERO) < 0) {
>> +            throw new ProviderException("invalid coordinate");
>>         }
>> -        byte[] s = privateKey.getS().toByteArray();
>> -        byte[] encodedParams =                   // DER OID
>> -            ECUtil.encodeECParameterSpec(null, privateKey.getParams());
>> +        if (c.compareTo(mod) >= 0) {
>> +            throw new ProviderException("invalid coordinate");
>> +        }
>> +    }
>> -        try {
>> +    /*
>> +     * Check whether a public key is valid. Throw ProviderException
>> +     * if it is not valid or could not be validated.
>> +     */
>> +    private static void validate(ECOperations ops, ECPublicKey key) {
>> +
>> +        // ensure that integers are in proper range
>> +        BigInteger x = key.getW().getAffineX();
>> +        BigInteger y = key.getW().getAffineY();
>> +
>> +        BigInteger p = ops.getField().getSize();
>> +        validateCoordinate(x, p);
>> +        validateCoordinate(y, p);
>> +
>> +        // ensure the point is on the curve
>> +        EllipticCurve curve = key.getParams().getCurve();
>> +        BigInteger rhs = x.modPow(BigInteger.valueOf(3), p).add(curve.getA()
>> +            .multiply(x)).add(curve.getB()).mod(p);
>> +        BigInteger lhs = y.modPow(BigInteger.valueOf(2), p).mod(p);
>> +        if (!rhs.equals(lhs)) {
>> +            throw new ProviderException("point is not on curve");
>> +        }
>> -            return deriveKey(s, publicValue, encodedParams);
>> +        // check the order of the point
>> +        ImmutableIntegerModuloP xElem = ops.getField().getElement(x);
>> +        ImmutableIntegerModuloP yElem = ops.getField().getElement(y);
>> +        AffinePoint affP = new AffinePoint(xElem, yElem);
>> +        byte[] order = key.getParams().getOrder().toByteArray();
>> +        ArrayUtil.reverse(order);
>> +        Point product = ops.multiply(affP, order);
>> +        if (!ops.isNeutral(product)) {
>> +            throw new ProviderException("point has incorrect order");
>> +        }
>> -        } catch (GeneralSecurityException e) {
>> -            throw new ProviderException("Could not derive key", e);
>> +    }
>> +
>> +    // see JCE spec
>> +    @Override
>> +    protected byte[] engineGenerateSecret() throws IllegalStateException {
>> +        if ((privateKey == null) || (publicKey == null)) {
>> +            throw new IllegalStateException("Not initialized correctly");
>>         }
>> +        Optional<byte[]> resultOpt = deriveKeyImpl(privateKey, publicKey);
>> +        byte[] result = resultOpt.orElseGet(
>> +            () -> deriveKeyNative(privateKey, publicKey)
>> +        );
>> +        publicKey = null;
>> +        return result;
>>     }
>>     // see JCE spec
>> @@ -175,7 +209,7 @@ public final class ECDHKeyAgreement extends KeyAgreementSpi {
>>         ECParameterSpec ecSpec = priv.getParams();
>>         EllipticCurve curve = ecSpec.getCurve();
>>         Optional<ECOperations> opsOpt = ECOperations.forParameters(ecSpec);
>> -        if (opsOpt.isEmpty()) {
>> +        if (!opsOpt.isPresent()) {
>>             return Optional.empty();
>>         }
>>         ECOperations ops = opsOpt.get();
>> diff --git a/src/jdk/src/share/classes/sun/security/ec/ECDSASignature.java b/src/jdk/src/share/classes/sun/security/ec/ECDSASignature.java
>> index de98f3b4..89f527e5 100644
>> --- a/src/jdk/src/share/classes/sun/security/ec/ECDSASignature.java
>> +++ b/src/jdk/src/share/classes/sun/security/ec/ECDSASignature.java
>> @@ -169,7 +169,7 @@ abstract class ECDSASignature extends SignatureSpi {
>>     // Nested class for SHA224withECDSA signatures
>>     public static final class SHA224 extends ECDSASignature {
>>         public SHA224() {
>> -           super("SHA-224");
>> +            super("SHA-224");
>>         }
>>     }
>> @@ -312,7 +312,7 @@ abstract class ECDSASignature extends SignatureSpi {
>>         int seedBits = params.getOrder().bitLength() + 64;
>>         Optional<ECDSAOperations> opsOpt =
>>             ECDSAOperations.forParameters(params);
>> -        if (opsOpt.isEmpty()) {
>> +        if (!opsOpt.isPresent()) {
>>             return Optional.empty();
>>         } else {
>>             byte[] sig = signDigestImpl(opsOpt.get(), seedBits, digest,
>> @@ -342,14 +342,33 @@ abstract class ECDSASignature extends SignatureSpi {
>>         timingArgument |= 1;
>>         try {
>> -            return encodeSignature(
>> -                signDigest(getDigestValue(), s, encodedParams, seed,
>> -                    timingArgument));
>> +            return signDigest(digest, s, encodedParams, seed,
>> +                    timingArgument);
>>         } catch (GeneralSecurityException e) {
>>             throw new SignatureException("Could not sign data", e);
>>         }
>>     }
>> +    // sign the data and return the signature. See JCA doc
>> +    @Override
>> +    protected byte[] engineSign() throws SignatureException {
>> +
>> +        if (random == null) {
>> +            random = JCAUtil.getSecureRandom();
>> +        }
>> +
>> +        byte[] digest = getDigestValue();
>> +        Optional<byte[]> sigOpt = signDigestImpl(privateKey, digest, random);
>> +        byte[] sig;
>> +        if (sigOpt.isPresent()) {
>> +            sig = sigOpt.get();
>> +        } else {
>> +            sig = signDigestNative(privateKey, digest, random);
>> +        }
>> +
>> +        return encodeSignature(sig);
>> +    }
>> +
>>     // verify the data and return the result. See JCA doc
>>     @Override
>>     protected boolean engineVerify(byte[] signature) throws SignatureException {
>> diff --git a/src/jdk/src/share/classes/sun/security/ec/ECKeyPairGenerator.java b/src/jdk/src/share/classes/sun/security/ec/ECKeyPairGenerator.java
>> index ad78d7b4..0f9b5eb2 100644
>> --- a/src/jdk/src/share/classes/sun/security/ec/ECKeyPairGenerator.java
>> +++ b/src/jdk/src/share/classes/sun/security/ec/ECKeyPairGenerator.java
>> @@ -31,14 +31,16 @@ import java.security.spec.AlgorithmParameterSpec;
>> import java.security.spec.ECGenParameterSpec;
>> import java.security.spec.ECParameterSpec;
>> import java.security.spec.ECPoint;
>> +import java.security.spec.InvalidParameterSpecException;
>> +import java.security.spec.*;
>> +import java.util.Optional;
>> -import sun.security.ec.NamedCurve;
>> -import sun.security.ec.ECParameters;
>> -import sun.security.ec.ECPrivateKeyImpl;
>> -import sun.security.ec.ECPublicKeyImpl;
>> import sun.security.jca.JCAUtil;
>> import sun.security.util.ECUtil;
>> +import sun.security.util.math.*;
>> +import sun.security.ec.point.*;
>> import static sun.security.util.SecurityProviderConstants.DEF_EC_KEY_SIZE;
>> +import static sun.security.ec.ECOperations.IntermediateValueException;
>> /**
>>  * EC keypair generator.
>> @@ -86,17 +88,19 @@ public final class ECKeyPairGenerator extends KeyPairGeneratorSpi {
>>     public void initialize(AlgorithmParameterSpec params, SecureRandom random)
>>             throws InvalidAlgorithmParameterException {
>> +        ECParameterSpec ecSpec = null;
>> +
>>         if (params instanceof ECParameterSpec) {
>> -            this.params = ECUtil.getECParameterSpec(null,
>> -                                                    (ECParameterSpec)params);
>> +            ECParameterSpec ecParams = (ECParameterSpec) params;
>> +            ecSpec = ECUtil.getECParameterSpec(null, ecParams);
>>             if (this.params == null) {
>>                 throw new InvalidAlgorithmParameterException(
>>                     "Unsupported curve: " + params);
>>             }
>>         } else if (params instanceof ECGenParameterSpec) {
>> -            String name = ((ECGenParameterSpec)params).getName();
>> -            this.params = ECUtil.getECParameterSpec(null, name);
>> -            if (this.params == null) {
>> +            String name = ((ECGenParameterSpec) params).getName();
>> +            ecSpec = ECUtil.getECParameterSpec(null, name);
>> +            if (ecSpec == null) {
>>                 throw new InvalidAlgorithmParameterException(
>>                     "Unknown curve name: " + name);
>>             }
>> @@ -104,8 +108,9 @@ public final class ECKeyPairGenerator extends KeyPairGeneratorSpi {
>>             throw new InvalidAlgorithmParameterException(
>>                 "ECParameterSpec or ECGenParameterSpec required for EC");
>>         }
>> -        this.keySize =
>> -            ((ECParameterSpec)this.params).getCurve().getField().getFieldSize();
>> +        this.params = ecSpec;
>> +
>> +        this.keySize = ecSpec.getCurve().getField().getFieldSize();
>>         this.random = random;
>>     }
>> @@ -154,7 +159,7 @@ public final class ECKeyPairGenerator extends KeyPairGeneratorSpi {
>>         ECParameterSpec ecParams = (ECParameterSpec) params;
>>         Optional<ECOperations> opsOpt = ECOperations.forParameters(ecParams);
>> -        if (opsOpt.isEmpty()) {
>> +        if (!opsOpt.isPresent()) {
>>             return Optional.empty();
>>         }
>>         ECOperations ops = opsOpt.get();
>> diff --git a/src/jdk/src/share/classes/sun/security/ec/ECOperations.java b/src/jdk/src/share/classes/sun/security/ec/ECOperations.java
>> index 2995ef75..c9414006 100644
>> --- a/src/jdk/src/share/classes/sun/security/ec/ECOperations.java
>> +++ b/src/jdk/src/share/classes/sun/security/ec/ECOperations.java
>> @@ -34,6 +34,7 @@ import java.security.ProviderException;
>> import java.security.spec.ECFieldFp;
>> import java.security.spec.ECParameterSpec;
>> import java.security.spec.EllipticCurve;
>> +import java.util.HashMap;
>> import java.util.Map;
>> import java.util.Optional;
>> @@ -55,17 +56,19 @@ public class ECOperations {
>>         private static final long serialVersionUID = 1;
>>     }
>> -    static final Map<BigInteger, IntegerFieldModuloP> fields = Map.of(
>> -        IntegerPolynomialP256.MODULUS, new IntegerPolynomialP256(),
>> -        IntegerPolynomialP384.MODULUS, new IntegerPolynomialP384(),
>> -        IntegerPolynomialP521.MODULUS, new IntegerPolynomialP521()
>> -    );
>> -
>> -    static final Map<BigInteger, IntegerFieldModuloP> orderFields = Map.of(
>> -        P256OrderField.MODULUS, new P256OrderField(),
>> -        P384OrderField.MODULUS, new P384OrderField(),
>> -        P521OrderField.MODULUS, new P521OrderField()
>> -    );
>> +    static final Map<BigInteger, IntegerFieldModuloP> fields = new HashMap<>();
>> +    static {
>> +        fields.put(IntegerPolynomialP256.MODULUS, new IntegerPolynomialP256());
>> +        fields.put(IntegerPolynomialP384.MODULUS, new IntegerPolynomialP384());
>> +        fields.put(IntegerPolynomialP521.MODULUS, new IntegerPolynomialP521());
>> +    }
>> +
>> +    static final Map<BigInteger, IntegerFieldModuloP> orderFields = new HashMap<>();
>> +    static {
>> +        orderFields.put(P256OrderField.MODULUS, new P256OrderField());
>> +        orderFields.put(P384OrderField.MODULUS, new P384OrderField());
>> +        orderFields.put(P521OrderField.MODULUS, new P521OrderField());
>> +    }
>>     public static Optional<ECOperations> forParameters(ECParameterSpec params) {
>> 
>> 
>> 
>> 
> 
> I've applied this again against the current repo with the other changes
> in place and come up with much the same as you describe:
> 
> https://cr.openjdk.java.net/~andrew/openjdk8/8208698/webrev.01/
> 
> Divergences from the 11u version are as follows:
> 
> diff --git a/src/share/classes/sun/security/ec/ECDHKeyAgreement.java
> b/src/share/classes/sun/security/ec/ECDHKeyAgreement.java
> --- a/src/share/classes/sun/security/ec/ECDHKeyAgreement.java
> +++ b/src/share/classes/sun/security/ec/ECDHKeyAgreement.java
> @@ -155,15 +145,13 @@
> -        }
> -
> +        Optional<byte[]> resultOpt = deriveKeyImpl(privateKey, publicKey);
> -+        byte[] result = resultOpt.orElseGet(
> ++        return resultOpt.orElseGet(
> +            () -> deriveKeyNative(privateKey, publicKey)
> +        );
> -+        publicKey = null;
> -+        return result;
>      }
> 
>      // see JCE spec
> 
> This needs to differ from 11u as we don't want to accidentally backport
> the behavioural change JDK-8205476. See the discussion on the 11u list [0]
> 
> @@ -183,7 +171,7 @@
> +        ECParameterSpec ecSpec = priv.getParams();
> +        EllipticCurve curve = ecSpec.getCurve();
> +        Optional<ECOperations> opsOpt =
> ECOperations.forParameters(ecSpec);
> -+        if (opsOpt.isEmpty()) {
> ++        if (!opsOpt.isPresent()) {
> +            return Optional.empty();
> +        }
> +        ECOperations ops = opsOpt.get();
> 
> Optional.isEmpty() was not introduced until Java 11, but we can simply
> invert isPresent(). There are a couple more of these, with the same
> solution.
> 
> diff --git a/src/share/classes/sun/security/ec/ECDSASignature.java
> b/src/share/classes/sun/security/ec/ECDSASignature.java
> --- a/src/share/classes/sun/security/ec/ECDSASignature.java
> +++ b/src/share/classes/sun/security/ec/ECDSASignature.java
> 
> @@ -501,15 +489,7 @@
>          }
>      }
> 
> -     // Nested class for SHA224withECDSAinP1363Format signatures
> -     public static final class SHA224inP1363Format extends ECDSASignature {
> -         public SHA224inP1363Format() {
> --           super("SHA-224", true);
> -+            super("SHA-224", true);
> -         }
> -     }
> -
> 
> 8u doesn't include JDK-8042967 (Add variant of DSA Signature algorithms
> that do not ASN.1 encode the signature bytes) so we just drop this
> chunk. It's just whitespace change anyway.
> 
> +@@ -293,14 +342,33 @@
>          timingArgument |= 1;
> 
> --        byte[] sig;
>          try {
> --            sig = signDigest(getDigestValue(), s, encodedParams, seed,
> +-            return encodeSignature(
> +-                signDigest(getDigestValue(), s, encodedParams, seed,
> +-                    timingArgument));
> +            return signDigest(digest, s, encodedParams, seed,
> -                 timingArgument);
> ++                timingArgument);
>          } catch (GeneralSecurityException e) {
>              throw new SignatureException("Could not sign data", e);
>          }
> +     }
> 
> -+    }
> -+
> +    // sign the data and return the signature. See JCA doc
> +    @Override
> +    protected byte[] engineSign() throws SignatureException {
> @@ -649,11 +628,14 @@
> +        } else {
> +            sig = signDigestNative(privateKey, digest, random);
> +        }
> ++
> ++      return encodeSignature(sig);
> ++    }
> +
> -         if (p1363Format) {
> -             return sig;
> -         } else {
> 
> Again, without JDK-8042967, this code is slightly different. We just
> move the encodeSignature call to engineSign
> 
> @@ -671,14 +653,7 @@
>          throw new UnsupportedOperationException("setParameter() not
> supported");
>      }
> 
> -     @Override
> -     protected void engineSetParameter(AlgorithmParameterSpec params)
> --            throws InvalidAlgorithmParameterException {
> -+    throws InvalidAlgorithmParameterException {
> -         if (params != null) {
> -             throw new InvalidAlgorithmParameterException("No parameter
> accepted");
> -         }
> 
> This method doesn't exist (JDK-8146293: Add support for RSASSA-PSS
> Signature algorithm) so the hunk is dropped.
> 
> new file mode 100644
> --- /dev/null
> +++ b/src/share/classes/sun/security/ec/ECOperations.java
> 
> @@ -985,18 +962,23 @@
> +        private static final long serialVersionUID = 1;
> +    }
> +
> -+    static final Map<BigInteger, IntegerFieldModuloP> fields = Map.of(
> -+        IntegerPolynomialP256.MODULUS, new IntegerPolynomialP256(),
> -+        IntegerPolynomialP384.MODULUS, new IntegerPolynomialP384(),
> -+        IntegerPolynomialP521.MODULUS, new IntegerPolynomialP521()
> -+    );
> -+
> -+    static final Map<BigInteger, IntegerFieldModuloP> orderFields =
> Map.of(
> -+        P256OrderField.MODULUS, new P256OrderField(),
> -+        P384OrderField.MODULUS, new P384OrderField(),
> -+        P521OrderField.MODULUS, new P521OrderField()
> -+    );
> ++    static final Map<BigInteger, IntegerFieldModuloP> fields;
> +
> ++    static final Map<BigInteger, IntegerFieldModuloP> orderFields;
> ++
> ++    static {
> ++      Map<BigInteger, IntegerFieldModuloP> map = new HashMap<>();
> ++      map.put(IntegerPolynomialP256.MODULUS, new IntegerPolynomialP256());
> ++      map.put(IntegerPolynomialP384.MODULUS, new IntegerPolynomialP384());
> ++        map.put(IntegerPolynomialP521.MODULUS, new
> IntegerPolynomialP521());
> ++      fields = Collections.unmodifiableMap(map);
> ++      map = new HashMap<>();
> ++        map.put(P256OrderField.MODULUS, new P256OrderField());
> ++        map.put(P384OrderField.MODULUS, new P384OrderField());
> ++      map.put(P521OrderField.MODULUS, new P521OrderField());
> ++      orderFields = Collections.unmodifiableMap(map);
> ++    }
> ++
> +    public static Optional<ECOperations> forParameters(ECParameterSpec
> params) {
> +
> +        EllipticCurve curve = params.getCurve();
> 
> Map.of was only added in Java 9, so a more lengthy equivalent is
> required. This is much the same as what you did, but I used
> unmodifiableMap to retain the same behaviour as Map.of ("Returns an
> unmodifiable map containing three mappings")
> 
> If this looks ok to you, I'll push it and credit it to us both.
> 
> [0]
> https://mail.openjdk.java.net/pipermail/jdk-updates-dev/2019-June/001374.html
> 
> Thanks,
> -- 
> Andrew :)
> 
> Senior Free Java Software Engineer
> Red Hat, Inc. (http://www.redhat.com)
> 
> PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
> Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
> https://keybase.io/gnu_andrew
> 


More information about the security-dev mailing list