Hi Martin, thanks for the review. Makes sense to take the fix regarding the overflow check along. I requested approval for JDK-8217344, too, and will push both patches together.
Best regards Christoph > -----Original Message----- > From: Doerr, Martin > Sent: Freitag, 31. Mai 2019 14:40 > To: Langer, Christoph <[email protected]>; jdk-updates- > [email protected] > Cc: security-dev <[email protected]> > Subject: RE: [11u] RFR: 8208698: Improved ECC Implementation > > Hi Christoph, > > looks like quite some manual resolution just because of a small conflicting > change in one file. > Backport looks good, but please backport it together with JDK-8217344. > After that, ECDHKeyAgreement.java should be identical to the jdk13 version. > > Best regards, > Martin > > > > -----Original Message----- > > From: jdk-updates-dev <[email protected]> On > > Behalf Of Langer, Christoph > > Sent: Dienstag, 28. Mai 2019 09:21 > > To: [email protected] > > Cc: security-dev <[email protected]> > > Subject: [11u] RFR: 8208698: Improved ECC Implementation > > > > Hi, > > > > please review this backport of JDK-8208698: Improved ECC > Implementation. > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8208698 > > Original Change: http://hg.openjdk.java.net/jdk/jdk/rev/752e57845ad2 > > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8208698.11u/ > > > > The patch did not apply cleanly because there were conflicts in > > src/jdk.crypto.ec/share/classes/sun/security/ec/ECDHKeyAgreement.java > > due to JDK-8205476 which is part of jdk/jdk but not of jdk11u-dev. > > Unfortunately, JDK-8205476 can not be downported as prerequisite > because > > it brings a behavioral change and is associated with a CSR. So I resolved > > the > > rejects manually. I add the rejects below. > > > > Thanks > > Christoph > > > > > > --- ECDHKeyAgreement.java > > +++ ECDHKeyAgreement.java > > @@ -99,42 +104,74 @@ > > ("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; > > } > > + 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"); > > + } > > + } > > + > > + /* > > + * 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"); > > + } > > + > > + // 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"); > > + } > > + > > + } > > + > > // see JCE spec > > @Override > > protected byte[] engineGenerateSecret() throws IllegalStateException { > > - if ((privateKey == null) || (publicValue == null)) { > > + if ((privateKey == null) || (publicKey == null)) { > > throw new IllegalStateException("Not initialized correctly"); > > } > > - byte[] s = privateKey.getS().toByteArray(); > > - byte[] encodedParams = // DER OID > > - ECUtil.encodeECParameterSpec(null, privateKey.getParams()); > > - > > - try { > > - > > - byte[] result = deriveKey(s, publicValue, encodedParams); > > - publicValue = null; > > - return result; > > - > > - } catch (GeneralSecurityException e) { > > - throw new ProviderException("Could not derive key", e); > > - } > > - > > + Optional<byte[]> resultOpt = deriveKeyImpl(privateKey, publicKey); > > + byte[] result = resultOpt.orElseGet( > > + () -> deriveKeyNative(privateKey, publicKey) > > + ); > > + publicKey = null; > > + return result; > > } > > // see JCE spec
