[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