[11u] RFR: 8208698: Improved ECC Implementation

[Langer, Christoph]

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 <christoph.langer at sap.com>; jdk-updates-
> dev at openjdk.java.net
> Cc: security-dev <security-dev at openjdk.java.net>
> 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 <jdk-updates-dev-bounces at openjdk.java.net> On
> > Behalf Of Langer, Christoph
> > Sent: Dienstag, 28. Mai 2019 09:21
> > To: jdk-updates-dev at openjdk.java.net
> > Cc: security-dev <security-dev at openjdk.java.net>
> > 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