On 04/02/2020 23:24, Bradford Wetmore wrote:
> I added a simple PSS 32-bit windows crash fix, which was previously
> reviewed in security-dev earlier today [0].
> 
>     8238502: sunmscapi.dll causing EXCEPTION_ACCESS_VIOLATION
> 
> The PSS webrev is now at version .01.
> 
> Otherwise, everything is identical from last week's request below.  The
> ALPN remains at version .00.
> 
> [0]
> https://mail.openjdk.java.net/pipermail/security-dev/2020-February/021203.html
> 
> 
> ---begin---
> 
> Good morning/afternoon/evening/night,
> 
> As announced on jdk8u-dev[1], there is a Maintenance Release in progress
> for Java SE 8 (i.e. JSR 337) [2] to include two security features
> important for TLS 1.3:
> 
> 1.  Application-Layer Protocol Negotiation (ALPN) [3][4]
> 2.  RSA Signature Scheme with Appendix: Probabilistic Signature Scheme
> (RSASSA-PSS) [5][6]
> 
> As mentioned in [1], if it wasn't too much work then we would like to
> contribute the changes required by this MR to the next appropriate
> OpenJDK 8 release, most likely 8u252.
> 
> Now that the MR3 balloting successfully concluded last night, I'd like
> to make that happen, and move the code into review.
> 
> The code is essentially what was reviewed for 8u41[7][8] and internally
> for what we expect to be in Oracle's 8u251 JDK, except the code in this
> review is based on the current JDK 8u workspace.  We also included code
> to allow the Windows platform to use PSS natively.
> 
> The specific bugs/backports (requested by the JDK8u maintainers) follow:
> 
> ALPN:
> =====
> 8230977: JEP 244/8051498 - TLS Application-Layer Protocol Negotiation
> Extension (Java SE 8)
> 8144093: JEP 244/8051498 - TLS Application-Layer Protocol Negotiation
> Extension
> 8170282: Enable ALPN parameters to be supplied during the TLS handshake
> 8145849: ALPN: getHandshakeApplicationProtocol() always return null
> 8158978: ALPN not working when values are set directly on a SSLServerSocket
> 8171443: (spec) An ALPN callback function may also ignore ALPN
> 
> RSASSA-PSS:
> ===========
> 8230978: Add support for RSASSA-PSS Signature algorithm (Java SE 8)
> 8175029: StackOverflowError in X509CRL and
> X509Certificate.verify(PublicKey, Provider)
> 8146293: Add support for RSASSA-PSS Signature algorithm
> 8205445: Add RSASSA-PSS Signature support to SunMSCAPI
> 8205720: KeyFactory#getKeySpec and translateKey throws
> NullPointerException with Invalid key
> 8206171: Signature#getParameters for RSASSA-PSS throws ProviderException
> when not initialized
> 8213009: Refactoring existing SunMSCAPI classes
> 8213010: Supporting keys created with certmgr.exe
> 8214096: sun.security.util.SignatureUtil passes null parameter, so JCE
> validation fails
> 8215694: keytool cannot generate RSASSA-PSS certificates
> 8221407: Windows 32bit build error in libsunmscapi/security.cpp
> 8216039: TLS with BC and RSASSA-PSS breaks ECDHServerKeyExchange
> 8223003: SunMSCAPI keys are not cleaned up
> 8223063: Support CNG RSA keys
> 8225745: NoSuchAlgorithmException exception for SHA256withECDSA with
> RSASSA-PSS support
> 8225180: SignedObject with invalid Key not throwing the
> InvalidKeyException in Windows
> 8236470: Deal with ECDSA using ecdsa-with-SHA2 plus hash algorithm as
> AlgorithmId
> Reviewed-by: valeriep, weijun, coffeys, pkoppula
> 
> Here are the reviews:
> 
> 1.  ALPN:
>      http://cr.openjdk.java.net/~wetmore/MR3-codereview-8u252/ALPN
> 
> 2.  RSASSA-PSS:
>      http://cr.openjdk.java.net/~wetmore/MR3-codereview-8u252/PSS
> 
> Most of these changes are direct copies of the changesets applied
> in JDK 9+, but adjusted for JDK 8u.
> 
> Also, truncated MessageDigests (i.e. SHA-512/224, SHA-512/256) were
> added to the SUN Provider to support the corresponding truncated
> RSASSA-PSS Signatures.
> 
> Thanks,
> 
> Brad
> 
> [1]
> https://mail.openjdk.java.net/pipermail/jdk8u-dev/2019-November/010573.html
> [2] https://www.jcp.org/en/jsr/detail?id=337
> [3] https://bugs.openjdk.java.net/browse/JDK-8230977
> [4] https://bugs.openjdk.java.net/browse/JDK-8233417
> [5] https://bugs.openjdk.java.net/browse/JDK-8230978
> [6] https://bugs.openjdk.java.net/browse/JDK-8233418
> [7]
> https://mail.openjdk.java.net/pipermail/security-dev/2019-November/020900.html
> 
> [8] http://hg.openjdk.java.net/jdk8u/jdk8u41/
> 
> 

Hi Brad,

First of all, thanks again for posting these patches and also for the
comprehensive list of issues for both of them. They pretty much matched
up with what I saw when reviewing the patches.

The ALPN one looks pretty clean. My only concern there is the use of
"@since 8". Should this not be a reference to the maintenance release,
as these methods were not part of 8 at GA? Same applies to the usage in
the second patch too.

With the RSA-PSS patch, it's hard to tell what's going on with the
MSCAPI code, because they appear as new files in the patch. Why was it
necessary to bring in JDK-8213009? I don't see an obvious reason for
this. I know from the fix request on that bug for 11u that it's a
dependency of 8213010, but again, it's not obvious why that's required
here either. Some more details on why some of the less obvious bugs are
required would be helpful here. If the refactoring is necessary, a
Git-style patch which shows the moves as renames (hg diff -g) would
help, so we can see what is actually being changed in these files.

As you say, the RSA-PSS patch brings in truncated MessageDigests which
are part of "8051408: NIST SP 800-90A SecureRandom implementations". It
would be good if the summary field could be used to say "Contains
truncated MessageDigests from JDK-8051408", so that it then shows up if
someone is considering 8051408 at a later date.

Otherwise, looks good.

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

Attachment: signature.asc
Description: OpenPGP digital signature

Reply via email to