Looks good to me
> On 27 Jun 2019, at 19:53, Andrew John Hughes <gnu.and...@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 >
