Re: OpenJDK11u: Backward incompatible behavior
> Webrev JDK11: http://cr.openjdk.java.net/~bae/8239788/webrev.v3/
> getSession() method is implemented identically to JDK8, so it's
> behaviour is backward compatible to JDK8
I know, but I would like to see if there is really a compatibility
impact if the getSession() is consistent with other IO operations. We
could fix the problem later if there is a need.
> I may try to catch the InterruptedIOException, rather than handle it
> in the IOException catching block or the Exception catching block.
try {
...
} catch (Exception e) {
if (e instanceof ... ) {
...
} else (e instanceof ...) {
...
}
}
vs
try {
} catch (AException e) {
...
} catch (IOException e) {
...
} catch (Exception e) {
...
}
the later is the common coding style
SSLSocketInputRecord:
52 private byte[] inputBuffer = new byte[maxRecordSize];
Would you mind consider an improvement to use less memory?
If you have webrev for JDK 15, I could help to run more testing for you.
Thanks,
Xuelei
On 2/27/2020 9:05 AM, Alexey Bakhtin wrote:
Hello Xuelei,
You are right, SSLSocketInputRecord.read() method could be interrupted
before all requested data is received.
I have updated SSLSocketInputRecord class to use single buffer for
incoming data. Also I’ve updated read() method to take into account
every chunk of incoming data. It should prevent possible loss of data if
socket timed out.
Webrev JDK11: http://cr.openjdk.java.net/~bae/8239788/webrev.v3/
Tested with sun/security/ssl and javax/net/ssl jtreg tests
Thank you
Alexey
On 27 Feb 2020, at 01:04, Xuelei Fan > wrote:
> Webrev for JDK15 : http://cr.openjdk.java.net/~yan/8239788/webrev.2/
For the SSLSocketInputRecord.java update, is it possible that the
read() implementation interrupted before reading the exact specified
bytes of data? The received data may be lost if it is interrupted.
BTW, it might not be effective to use three fields to store the data,
temporary, header and inputBuffer. Is it possible to use just one
buffer (temporary, for example) and one integer to remember the
received data length?
Thanks,
Xuelei
On 2/26/2020 4:22 AM, Alexey Bakhtin wrote:
Hello Xuelei,
Thank you for review.
getSession() method is implemented identically to JDK8, so it's
behaviour is backward compatible to JDK8
I have updated review with some modifications:
1) Enabled sun/security/ssl/SSLSocketImpl/ClientTimeout.java jtreg
test. This test emulates socket timeout during handshake and app data
transfer.
2) I have added cache for incoming data received from socket
(sun.security.ssl.SSLSocketInputRecord class). Socket timeout could
happen while reading single handshake message by small chunks. It is
implemented similarly to JDK8 and allows to pass
sun/security/ssl/SSLSocketImpl/ClientTimeout test
3) I have added SocketTimeoutException handling into
sun/security/ssl/SSLSocketImpl/SSLExceptionForIOIssue.java jtreg
test. This test also verifies SocketExceptions during handshake.
Webrev for JDK11 : http://cr.openjdk.java.net/~yan/8239788/webrev.1/
Webrev for JDK15 : http://cr.openjdk.java.net/~yan/8239788/webrev.2/
Tested with sun/security/ssl and javax/net/ssl jtreg tests
Thank you
Alexey
On 25 Feb 2020, at 19:42, Xuelei Fan > wrote:
JDK11 webrev: http://cr.openjdk.java.net/~yan/8239788/webrev.0/
Maybe, the getSession() could throw InterruptedIOException as well.
I may try to catch the InterruptedIOException, rather than handle it
in the IOException catching block.
Thanks,
Xuelei
On 2/24/2020 11:04 AM, Alexey Bakhtin wrote:
Hi Xuelei,
Thank you. It would be glad if you can review this fix.
The patch almost cleanly applied to JDK15.
Also, As Kumar mentioned, the patch does not include regression test.
I’m going to add regression test and patch for JDK15 tomorrow.
Thank you.
Alexey
On 24 Feb 2020, at 21:41, Xuelei Fan > wrote:
Hi Alexey,
Thanks for working on this issue. Do you plan to fix it for JDK
15, the current JDK reposiroty?
I need more time for evaluate the fix. For example, I'm not sure
if we could always throw InterruptedIOException to applications,
even for getSession().
Regards,
Xuelei
On 2/24/2020 7:58 AM, Alexey Bakhtin wrote:
Hello,
I have been working on this issue for some time already.
The patch below adds java.net.SocketTimeoutException handling
during TLS handshake negotiation. This functionality seems to
have been missed during the TLSv1.3 implementation ( JDK-8196584 )
Tested with JDK11 and higher.
JDK11 webrev: http://cr.openjdk.java.net/~yan/8239788/webrev.0/
jbs: https://bugs.openjdk.java.net/browse/JDK-8239798 and
https://bugs.openjdk.java.net/browse/JDK-8239788
Thank you,
Alexey
On 22 Feb 2020, at 15:00, security-dev-requ...@openjdk.java.net
wrote:
I will look into the issue.
BTW, I closed
Re: RFR 8238555: Allow Initialization of SunPKCS11 with NSS when there are external FIPS modules in the NSSDB
Sure, you can push since release note wording does not affect the changes. Valerie On 2/28/2020 5:26 AM, Martin Balao wrote: Hi Valerie, Thanks for having a look at this. I've seen your changes. Can I proceed with the push? Thanks, Martin.- On 2/27/20 11:48 PM, Valerie Peng wrote: I have looked over the release note subtask and made some minor changes on wordings and added RN-Change label. Sean may have additional comments to add though. Also, when you mark it as delivered, the tech writer will also make their edit. Just FYI.
Re: RFR 8237218: Support NIST Curves verification in java implementation
On 2/28/2020 3:05 PM, Michael StJohns wrote:
Sorry - running behind on this thread.
In ECUtil.decodePoint(),
Since this code is open, I'm wondering if it might be useful to add
the checks specified in NIST SP800-186 Appendix D.1. or SP800-56Ar1
5.6.2.3 E.g.
D.1.2 Montgomery Curves
D.1.2.1 Partial Public Key Validation
Inputs:
1. Montgomery curve MA,B defined over the prime field GF(p)
2. Point Q=(u, v) 1712
Output: ACCEPT or REJECT Q as an affine point on MA,B.
Process:
1. If Q is the point at infinity ∅, output REJECT.
2. Verify that both u and v are integers in the interval [0, p−1].
Output REJECT if verification fails.
3. Verify that (u, v) is a point on the MA,B by checking that (u, v)
satisfies the defining equation v2 = u (u2 + A u + 1) where
computations are carried out in GF(p). Output REJECT if verification
fails.
4. Otherwise output ACCEPT.
D.1.2.2 Full Public Key Validation
Inputs:
1. Montgomery curve MA,B defined over the prime field GF(p)
2. Point Q
Output: ACCEPT or REJECT Q as a point on MA,B of order n.
Process:
1. Perform partial public key validation on Q using the procedure of
Appendix D.1.2.1. Output REJECT if this procedure outputs REJECT.
2. Verify that n Q = ∅. Output REJECT if verification fails.
3. Otherwise output ACCEPT.
This mainly ensures that the X/Y provided is actually a point on the
curve. The threat to receiving a bad public key is more on the ECDH
side, but this appears to be the code that would need to be modified
so...
Later, Mike
Here's the function I use in my application code - the last check
(needed for full verification) uses BouncyCastle as I didn't have access
to the internal methods in the SunEC provider. Would have to be
refactored slightly to be used in ECUtil.decodePoint().
/**
* This function performs the checks described in NIST SP800-56A,
* section 5.6.2.5 over an ECPublicKey. It throws a
* GeneralSecurityException if the key does not validate
*
* @param k the key to validate
*
* @throws InvalidKeyException if the key is invalid
*/
public static void checkECPublicKey (ECPublicKey k)
throws InvalidKeyException {
// Step 0 - not in the SP document, but we don't support F2M
// curves
if (!((k.getParams().getCurve().getField()) instanceof ECFieldFp)) {
throw new InvalidKeyException ("ECPublicKey is not on a Prime
Curve - not supported");
}
ECPoint point = k.getW();
// Step 1:
if (point.equals(ECPoint.POINT_INFINITY)) {
throw new InvalidKeyException ("ECPublic key is point at Infinity");
}
// Step 2:
EllipticCurve curve = k.getParams().getCurve();
BigInteger p = ((ECFieldFp)curve.getField()).getP();
BigInteger x = point.getAffineX();
BigInteger y = point.getAffineY();
if (x.compareTo(BigInteger.ZERO) <= 0 || x.compareTo(p) >= 0)
throw new InvalidKeyException ("ECPublicKey X out of Range");
if (y.compareTo(BigInteger.ZERO) <= 0 || y.compareTo(p) >= 0)
throw new InvalidKeyException ("ECPublicKey Y out of Range");
// Step 3:
BigInteger y2 = y.pow(2).mod(p);
BigInteger x3 = x.pow(3);
BigInteger ax = curve.getA().multiply(x);
BigInteger sum = x3.add(ax).add(curve.getB()).mod(p);
if (!y2.equals(sum))
throw new InvalidKeyException ("ECPublicKey Point is not on Curve");
// Step 4: check nQ == INFINITY
BigInteger n = k.getParams().getOrder();
org.bouncycastle.math.ec.ECPoint bcPoint =
// EC5Util.convertPoint(k.getParams(), point, false); A/O 1.64
EC5Util.convertPoint(k.getParams(), point);
org.bouncycastle.math.ec.ECPoint testPoint =
bcPoint.multiply(n);
if (!testPoint.isInfinity())
throw new InvalidKeyException ("ECPublicKey invalid order");
}
Re: RFR 8237218: Support NIST Curves verification in java implementation
Sorry - running behind on this thread. In ECUtil.decodePoint(), Since this code is open, I'm wondering if it might be useful to add the checks specified in NIST SP800-186 Appendix D.1. or SP800-56Ar1 5.6.2.3 E.g. D.1.2 Montgomery Curves D.1.2.1 Partial Public Key Validation Inputs: 1. Montgomery curve MA,B defined over the prime field GF(p) 2. Point Q=(u, v) 1712 Output: ACCEPT or REJECT Q as an affine point on MA,B. Process: 1. If Q is the point at infinity ∅, output REJECT. 2. Verify that both u and v are integers in the interval [0, p−1]. Output REJECT if verification fails. 3. Verify that (u, v) is a point on the MA,B by checking that (u, v) satisfies the defining equation v2 = u (u2 + A u + 1) where computations are carried out in GF(p). Output REJECT if verification fails. 4. Otherwise output ACCEPT. D.1.2.2 Full Public Key Validation Inputs: 1. Montgomery curve MA,B defined over the prime field GF(p) 2. Point Q Output: ACCEPT or REJECT Q as a point on MA,B of order n. Process: 1. Perform partial public key validation on Q using the procedure of Appendix D.1.2.1. Output REJECT if this procedure outputs REJECT. 2. Verify that n Q = ∅. Output REJECT if verification fails. 3. Otherwise output ACCEPT. This mainly ensures that the X/Y provided is actually a point on the curve. The threat to receiving a bad public key is more on the ECDH side, but this appears to be the code that would need to be modified so... Later, Mike On 2/20/2020 11:03 PM, Anthony Scarpino wrote: I'm ok with this update Tony On 2/19/20 5:35 AM, Weijun Wang wrote: New webrev at http://cr.openjdk.java.net/~weijun/8237218/webrev.04/ Only test change. For each signature, modify it a little and check if verify fails. Thanks, Max On Feb 18, 2020, at 2:09 AM, Anthony Scarpino wrote: The change looks fine. I'm trusting that the existing Known Answer Tests are proving your verifySignedDigest() is correct. You may want to comment in the code that your test depends on these method names. I was going to suggest simplifying the all the verifySigned*() methods until I saw the test was dependent on it. Tony On 2/13/20 3:06 AM, Weijun Wang wrote: Webrev updated at http://cr.openjdk.java.net/~weijun/8237218/webrev.03/ The test is modified. Instead of adding a hacked ECDSASignature I'm using JDI to detect if the Java impl or the native impl is used. Two method names in ECDSASignature are modified to ease method lookup in the test. Thanks, Max On Feb 11, 2020, at 7:52 PM, Weijun Wang wrote: Please take a review at http://cr.openjdk.java.net/~weijun/8237218/webrev.02/ A test is added that uses a patched ECDSASignature.java that exposes how the signature is verified. BTW, I also updated ECDSASignature.java a little to accept non SunEC keys, so that I can do some interop testing. If you believe this is unnecessary I can revert the change. Thanks, Max
Re: RFR 8238555: Allow Initialization of SunPKCS11 with NSS when there are external FIPS modules in the NSSDB
Hi Valerie, Thanks for having a look at this. I've seen your changes. Can I proceed with the push? Thanks, Martin.- On 2/27/20 11:48 PM, Valerie Peng wrote: > > I have looked over the release note subtask and made some minor changes > on wordings and added RN-Change label. > > Sean may have additional comments to add though. Also, when you mark it > as delivered, the tech writer will also make their edit. Just FYI. >
