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
signature.asc
Description: OpenPGP digital signature
