Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature [v2]
On Thu, 18 Nov 2021 19:27:30 GMT, Martin Balao wrote: > > > Hi @valeriepeng , > > Some comments and questions regarding Webrev.01: > > * P11Key.java > > * Would you consider replacing the 'Internal' suffix with 'Opaque'? I > believe the term 'opaque' better reflects what these keys are: you cannot see > their values -thus, opaque- but you can use them as-is. 'Internal' gives me > the impression of something not supposed to be exposed; and this is not > exactly the case. > > * Why are P11RSAPrivateKeyInternal::n, P11RSAPrivateKey:: qe, coeff>, etc. now transient? > > * Now that P11RSAPrivateKey::n private field was removed, the extra > PKCS#11 lib call I mentioned in Webrev.00 is not possible anymore. The > override of ::getModulus looks good to me, though, for the reasons you said. > > * Can P11RSAPrivateNonCRTKey use the protected 'n' field from its parent > instead of declaring a private one? > > * P11Signature.java > > * 'long errorCode = e.getErrorCode();' -> Looks to me that this change > was included into the Webrev by mistake, and is part of JDK-8236512. > > > Thanks, Martin.- Either internal or opaque suffix works for me. Fields aren't really being written out into the serialized bytes are "transient". IIRC, the serialized bytes are the "encoding" instead of the fields, we should mark these fields transient. As for the P11RSAPrivateNonCRTKey, yes, it should use the 'n' from parent instead of its own. I missed it when working on webrev.01. As for 'long errorCode = e.getErrorCode();' in P11Signature.java, yes, it should have been removed. I was manually merging the changes when refreshing the source tree and missed to remove this line. Thanks, Valerie - PR: https://git.openjdk.java.net/jdk/pull/4961
RFR: JDK-8276447 Deprecate finalization-related methods for removal
Here are the code changes for the "Deprecate finalizers in the standard Java
API" portion of JEP 421 ("Deprecate Finalization for Removal") for code review.
This change makes the indicated deprecations, and updates the API spec for JEP
421. It also updates the relevant @SuppressWarning annotations.
The CSR has been approved.
An automated test build+test run passes cleanly (FWIW :D ).
-
Commit messages:
- merge
- @SuppressWarnings(removal) in Windows CKey.java
- Add jls tag to runFinalization methods
- Update wording of Object.finalize, new paragraph is now bold
- update Object.finalize javadoc
- update Object.finalize JavaDoc and @deprecated tag
- Update Object.finalize deprecation message
- Indicate that runFinalizers does nothing if finalization is disabled or
removed
- Fix @since on @Deprecated for java.lang.Enum
- Clarify conditions for removal of Object.finalize method
- ... and 21 more: https://git.openjdk.java.net/jdk/compare/89b125f4...eca95cb2
Changes: https://git.openjdk.java.net/jdk/pull/6465/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6465=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8276447
Stats: 194 lines in 62 files changed: 54 ins; 38 del; 102 mod
Patch: https://git.openjdk.java.net/jdk/pull/6465.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/6465/head:pull/6465
PR: https://git.openjdk.java.net/jdk/pull/6465
Re: Fwd: new hurdle for applications which programatically install a SecurityManager
Thanks for the quick response and for the pointer to https://bugs.openjdk.java.net/browse/JDK-8203316 The change in the default value of java.security.manager prevents Derby from installing a SecurityManager when the user forgets to. This increases Derby's attack surface, significantly in my opinion. On 11/18/21 11:21 AM, Sean Mullan wrote: On 11/18/21 1:22 PM, Rick Hillegas wrote: Here's the output I get when I run that program against 18-ea+23-1525 WITHOUT setting java.security.manager on the boot command line: Exception in thread "main" java.lang.UnsupportedOperationException: The Security Manager is deprecated and will be removed in a future release at java.base/java.lang.System.setSecurityManager(System.java:411) at DERBY_7126_B.main(DERBY_7126_B.java:34) Here's the output I get when I run that program against 18-ea+23-1525 but do set java.security.manager on the boot command line: WARNING: A terminally deprecated method in java.lang.System has been called WARNING: System::setSecurityManager has been called by DERBY_7126_B (file:/Users/rhillegas/src/) WARNING: Please consider reporting this to the maintainers of DERBY_7126_B WARNING: System::setSecurityManager will be removed in a future release Is this asymmetry in the handling of this new system property deliberate? Yes. The system property is read early (and never again) in VM init phase 3. If so, what is the motivation for this asymmetry? One of the motivations is to improve the performance of applications that do not use the Security Manager. These applications ideally should not have to incur the cost of supporting a SecurityManager if it is not used. By reading the system property early, the code for loading and checking the security field can be removed. More details are in the CSR for this change that initially went into JDK 12: https://bugs.openjdk.java.net/browse/JDK-8203316 If not, can the new property be made to operate like the other SecurityManager properties, that is, can the JDK be amended so that java.security.manager can be set programatically? AFAIK, the "java.security.manager" system property has always been read early before the main application is launched. Also, a change like that would negate the performance benefits described above. --Sean
Re: RFR: 8274333: Redundant null comparison after Pattern.split
On Sun, 26 Sep 2021 15:10:52 GMT, Andrey Turbanov wrote: > In couple of classes, result part of arrays of Pattern.split is compared with > `null`. Pattern.split (and hence String.split) never returns `null` in array > elements. Such comparisons are redundant. Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5708
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature [v2]
On Thu, 18 Nov 2021 18:37:38 GMT, Valerie Peng wrote: >>> > ``` >>> > * By eliminating P11RSAPrivateKey::getModulus, looks to me that >>> > P11PrivateKeyRSA::getModulus and P11PrivateKeyRSA::fetchValues are now >>> > called, leading to an unnecessary call to the native library as the >>> > modulus was already received on P11RSAPrivateKey constructor. This >>> > happens to P11RSAPrivateNonCRTKey as well. >>> > ``` >>> >>> There shouldn't be another call to the native library as it is only made >>> when the modulus n is null. However, since n is already available, >>> overriding the getModulus() method makes sense as there is no need to call >>> fetchValues() which should return upon a non-null n value, but still an >>> overhead. >>> >> >> In my view (Webrev.00 based comment), the variable 'n' holding the modulus >> value is private in P11RSAPrivateKey. This means that when >> P11PrivateKeyRSA::getModulus is called, P11PrivateKeyRSA::n (which is >> protected) has a 'null' value and the PKCS#11 lib call is done again. > >> >> >> > > ``` >> > > * By eliminating P11RSAPrivateKey::getModulus, looks to me that >> > > P11PrivateKeyRSA::getModulus and P11PrivateKeyRSA::fetchValues are now >> > > called, leading to an unnecessary call to the native library as the >> > > modulus was already received on P11RSAPrivateKey constructor. This >> > > happens to P11RSAPrivateNonCRTKey as well. >> > > ``` >> > >> > >> > There shouldn't be another call to the native library as it is only made >> > when the modulus n is null. However, since n is already available, >> > overriding the getModulus() method makes sense as there is no need to call >> > fetchValues() which should return upon a non-null n value, but still an >> > overhead. >> >> In my view (Webrev.00 based comment), the variable 'n' holding the modulus >> value is private in P11RSAPrivateKey. This means that when >> P11PrivateKeyRSA::getModulus is called, P11PrivateKeyRSA::n (which is >> protected) has a 'null' value and the PKCS#11 lib call is done again. > > Hmm, this is a bug and unintended. The 'n' in the child class should be > removed as the 'n' in the parent class has scope protected and should be used > instead. > > I checked webrev.01 and this has been caught and fixed already. Good to have > a different pair of eyes and more likely to spot problems. Thanks! > > Regards, > Valerie Hi @valeriepeng , Some comments and questions regarding Webrev.01: * P11Key.java * Would you consider replacing the 'Internal' suffix with 'Opaque'? I believe the term 'opaque' better reflects what these keys are: you cannot see their values -thus, opaque- but you can use them as-is. 'Internal' gives me the impression of something not supposed to be exposed; and this is not exactly the case. * Why are P11RSAPrivateKeyInternal::n, P11RSAPrivateKey::, etc. now transient? * Now that P11RSAPrivateKey::n private field was removed, the extra PKCS#11 lib call I mentioned in Webrev.00 is not possible anymore. The override of ::getModulus looks good to me, though, for the reasons you said. * Can P11RSAPrivateNonCRTKey use the protected 'n' field from its parent instead of declaring a private one? * P11Signature.java * 'long errorCode = e.getErrorCode();' -> Looks to me that this change was included into the Webrev by mistake, and is part of JDK-8236512. Thanks, Martin.- - PR: https://git.openjdk.java.net/jdk/pull/4961
Re: RFR: 8274333: Redundant null comparison after Pattern.split
On Sun, 26 Sep 2021 15:10:52 GMT, Andrey Turbanov wrote: > In couple of classes, result part of arrays of Pattern.split is compared with > `null`. Pattern.split (and hence String.split) never returns `null` in array > elements. Such comparisons are redundant. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5708
Re: Fwd: new hurdle for applications which programatically install a SecurityManager
On 11/18/21 1:22 PM, Rick Hillegas wrote: Here's the output I get when I run that program against 18-ea+23-1525 WITHOUT setting java.security.manager on the boot command line: Exception in thread "main" java.lang.UnsupportedOperationException: The Security Manager is deprecated and will be removed in a future release at java.base/java.lang.System.setSecurityManager(System.java:411) at DERBY_7126_B.main(DERBY_7126_B.java:34) Here's the output I get when I run that program against 18-ea+23-1525 but do set java.security.manager on the boot command line: WARNING: A terminally deprecated method in java.lang.System has been called WARNING: System::setSecurityManager has been called by DERBY_7126_B (file:/Users/rhillegas/src/) WARNING: Please consider reporting this to the maintainers of DERBY_7126_B WARNING: System::setSecurityManager will be removed in a future release Is this asymmetry in the handling of this new system property deliberate? Yes. The system property is read early (and never again) in VM init phase 3. If so, what is the motivation for this asymmetry? One of the motivations is to improve the performance of applications that do not use the Security Manager. These applications ideally should not have to incur the cost of supporting a SecurityManager if it is not used. By reading the system property early, the code for loading and checking the security field can be removed. More details are in the CSR for this change that initially went into JDK 12: https://bugs.openjdk.java.net/browse/JDK-8203316 If not, can the new property be made to operate like the other SecurityManager properties, that is, can the JDK be amended so that java.security.manager can be set programatically? AFAIK, the "java.security.manager" system property has always been read early before the main application is launched. Also, a change like that would negate the performance benefits described above. --Sean
Re: RFR: 8275887: jarsigner prints invalid digest/signature algorithm warnings if keysize is weak/disabled [v2]
On Thu, 18 Nov 2021 15:03:33 GMT, Sean Mullan wrote: >> We should, but the problem is that jarsigner needs to individually test each >> algorithm, so it can properly display which algorithm is restricted. So, I >> think it will need to parse the RSSASSA params itself, and then call the >> constraints code to check each algorithm. Let me see if I can code up >> something that does that. > > I would like to defer the checking of AlgorithmParameters as part of another > bug. There are some major restructuring changes that would need to be made to > jarsigner to support this. And for RSASSA-PSS, there should not be any risk > for a while since by default jarsigner uses at least SHA-256 for the digest > algorithms in the PSS parameters. Looks so. - PR: https://git.openjdk.java.net/jdk/pull/6296
Re: RFR: 8275887: jarsigner prints invalid digest/signature algorithm warnings if keysize is weak/disabled [v2]
On Tue, 16 Nov 2021 18:10:04 GMT, Sean Mullan wrote: >> When a signature/digest algorithm was being checked, the algorithm >> constraints checked both the signature/digest algorithm and the key to see >> if they were restricted. This caused duplicate checks and was also >> problematic for `jarsigner` (and `keytool`) which need to distinguish these >> two cases, so that the output can properly indicate when the key is disabled >> but the signature or digest alg is ok. >> >> To address this issue, a new `checkKey` parameter is added to the >> `DisabledAlgorithmConstraints.permits` methods. When `true` the key (alg and >> size) is also checked, otherwise it is not. This flag is always set to >> `false` by `jarsigner` when checking algs and by the JDK when checking >> digest algorithms. Other small changes include changes in `SignerInfo` to >> use a record to store info about the algorithms to be checked, and removing >> an unnecessary CRL checking method from `AlgorithmChecker`. >> >> `keytool` will be enhanced in a subsequent CR to call the new methods. > > Sean Mullan has updated the pull request incrementally with one additional > commit since the last revision: > > Use var in for loop in SignerInfo. > In TimestampCheck test, combine/simplify what messages should not be emitted > when jar is signed with 512-bit RSA key. Marked as reviewed by weijun (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6296
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature [v2]
On Wed, 17 Nov 2021 21:25:33 GMT, Martin Balao wrote: > > > > > ``` > > > * By eliminating P11RSAPrivateKey::getModulus, looks to me that > > > P11PrivateKeyRSA::getModulus and P11PrivateKeyRSA::fetchValues are now > > > called, leading to an unnecessary call to the native library as the > > > modulus was already received on P11RSAPrivateKey constructor. This > > > happens to P11RSAPrivateNonCRTKey as well. > > > ``` > > > > > > There shouldn't be another call to the native library as it is only made > > when the modulus n is null. However, since n is already available, > > overriding the getModulus() method makes sense as there is no need to call > > fetchValues() which should return upon a non-null n value, but still an > > overhead. > > In my view (Webrev.00 based comment), the variable 'n' holding the modulus > value is private in P11RSAPrivateKey. This means that when > P11PrivateKeyRSA::getModulus is called, P11PrivateKeyRSA::n (which is > protected) has a 'null' value and the PKCS#11 lib call is done again. Hmm, this is a bug and unintended. The 'n' in the child class should be removed as the 'n' in the parent class has scope protected and should be used instead. Regards, Valerie - PR: https://git.openjdk.java.net/jdk/pull/4961
Fwd: new hurdle for applications which programatically install a SecurityManager
Re-sending from the account linked to my security-dev subscription
Forwarded Message
Build 18-ea+23-1525 has introduced another hurdle for applications which
use the SecurityManager. In order to install a SecurityManager, you now
have to set -Djava.security.manager=allow on the boot command line. This
property cannot be set programatically, unlike the other system
properties related to the SecurityManager. I have attached a simple
repro of this asymmetry (DERBY_7126_B) to
https://issues.apache.org/jira/browse/DERBY-7126. The repro
programatically sets java.security.manager. Here's the code:
import java.io.PrintWriter;
import java.util.Properties;
/**
* Demonstrate that the SecurityManager can be installed by setting
*/
@SuppressWarnings("removal")
public class DERBY_7126_B
{
private static final String PROPERTY_FILE_NAME =
"/tmp/derby-7126_B.properties";
private static final String SECURITY_POLICY_FILE_NAME =
"/tmp/derby-7126_B.policy";
private static final String SECURITY_POLICY_FILE_URL = "file:" +
SECURITY_POLICY_FILE_NAME;
private final static String POLICY_FILE_PROPERTY =
"java.security.policy";
private static final String SECURITY_FILE_CONTENTS =
"grant\n" +
"{\n" +
" permission java.io.FilePermission \"/tmp/-\",
\"read,write,delete\";\n" +
"};\n"
;
public static void main(String... args) throws Exception
{
// write the policy file
try (PrintWriter pw = new PrintWriter(SECURITY_POLICY_FILE_NAME))
{ pw.write(SECURITY_FILE_CONTENTS); }
// start up a security manager using the policy file we just wrote
System.setProperty( POLICY_FILE_PROPERTY,
SECURITY_POLICY_FILE_URL );
System.setProperty( "java.security.manager", "allow" );
System.setSecurityManager( new SecurityManager() );
}
}
Here's the output I get when I run that program against 18-ea+23-1525
WITHOUT setting java.security.manager on the boot command line:
Exception in thread "main" java.lang.UnsupportedOperationException: The
Security Manager is deprecated and will be removed in a future release
at java.base/java.lang.System.setSecurityManager(System.java:411)
at DERBY_7126_B.main(DERBY_7126_B.java:34)
Here's the output I get when I run that program against 18-ea+23-1525
but do set java.security.manager on the boot command line:
WARNING: A terminally deprecated method in java.lang.System has been called
WARNING: System::setSecurityManager has been called by DERBY_7126_B
(file:/Users/rhillegas/src/)
WARNING: Please consider reporting this to the maintainers of DERBY_7126_B
WARNING: System::setSecurityManager will be removed in a future release
Is this asymmetry in the handling of this new system property
deliberate? If so, what is the motivation for this asymmetry? If not,
can the new property be made to operate like the other SecurityManager
properties, that is, can the JDK be amended so that
java.security.manager can be set programatically?
Thanks,
-Rick
new hurdle for applications which programatically install a SecurityManager
Build 18-ea+23-1525 has introduced another hurdle for applications which
use the SecurityManager. In order to install a SecurityManager, you now
have to set -Djava.security.manager=allow on the boot command line. This
property cannot be set programatically, unlike the other system
properties related to the SecurityManager. I have attached a simple
repro of this asymmetry (DERBY_7126_B) to
https://issues.apache.org/jira/browse/DERBY-7126. The repro
programatically sets java.security.manager. Here's the code:
import java.io.PrintWriter;
import java.util.Properties;
/**
* Demonstrate that the SecurityManager can be installed by setting
*/
@SuppressWarnings("removal")
public class DERBY_7126_B
{
private static final String PROPERTY_FILE_NAME =
"/tmp/derby-7126_B.properties";
private static final String SECURITY_POLICY_FILE_NAME =
"/tmp/derby-7126_B.policy";
private static final String SECURITY_POLICY_FILE_URL = "file:" +
SECURITY_POLICY_FILE_NAME;
private final static String POLICY_FILE_PROPERTY =
"java.security.policy";
private static final String SECURITY_FILE_CONTENTS =
"grant\n" +
"{\n" +
" permission java.io.FilePermission \"/tmp/-\",
\"read,write,delete\";\n" +
"};\n"
;
public static void main(String... args) throws Exception
{
// write the policy file
try (PrintWriter pw = new PrintWriter(SECURITY_POLICY_FILE_NAME))
{ pw.write(SECURITY_FILE_CONTENTS); }
// start up a security manager using the policy file we just wrote
System.setProperty( POLICY_FILE_PROPERTY,
SECURITY_POLICY_FILE_URL );
System.setProperty( "java.security.manager", "allow" );
System.setSecurityManager( new SecurityManager() );
}
}
Here's the output I get when I run that program against 18-ea+23-1525
WITHOUT setting java.security.manager on the boot command line:
Exception in thread "main" java.lang.UnsupportedOperationException: The
Security Manager is deprecated and will be removed in a future release
at java.base/java.lang.System.setSecurityManager(System.java:411)
at DERBY_7126_B.main(DERBY_7126_B.java:34)
Here's the output I get when I run that program against 18-ea+23-1525
but do set java.security.manager on the boot command line:
WARNING: A terminally deprecated method in java.lang.System has been called
WARNING: System::setSecurityManager has been called by DERBY_7126_B
(file:/Users/rhillegas/src/)
WARNING: Please consider reporting this to the maintainers of DERBY_7126_B
WARNING: System::setSecurityManager will be removed in a future release
Is this asymmetry in the handling of this new system property
deliberate? If so, what is the motivation for this asymmetry? If not,
can the new property be made to operate like the other SecurityManager
properties, that is, can the JDK be amended so that
java.security.manager can be set programatically?
Thanks,
-Rick
Re: RFR: 8275887: jarsigner prints invalid digest/signature algorithm warnings if keysize is weak/disabled [v2]
On Tue, 16 Nov 2021 17:53:16 GMT, Sean Mullan wrote:
>> src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java line
>> 1491:
>>
>>> 1489: private static String checkWeakAlg(String alg,
>>> CertPathConstraintsParameters cpcp) {
>>> 1490: try {
>>> 1491: CERTPATH_DISABLED_CHECK.permits(alg, cpcp, false);
>>
>> Do we need to check AlgorithmParamters as well? Ex: if `alg` is RSASSA-PSS.
>
> We should, but the problem is that jarsigner needs to individually test each
> algorithm, so it can properly display which algorithm is restricted. So, I
> think it will need to parse the RSSASSA params itself, and then call the
> constraints code to check each algorithm. Let me see if I can code up
> something that does that.
I would like to defer the checking of AlgorithmParameters as part of another
bug. There are some major restructuring changes that would need to be made to
jarsigner to support this. And for RSASSA-PSS, there should not be any risk for
a while since by default jarsigner uses at least SHA-256 for the digest
algorithms in the PSS parameters.
-
PR: https://git.openjdk.java.net/jdk/pull/6296
Integrated: 4337793: Mark non-serializable fields of java.security.cert.Certificate and CertPath
On Mon, 15 Nov 2021 17:03:51 GMT, Sean Mullan wrote: > Please review this 20+ year old bug (!), which marks the non-serializable > fields of Certificate and CertPath with the transient modifier. These classes > use an alternate serialization mechanism by overriding the writeReplace > method. However, the fields of each class were never marked as transient and > as a result were incorrectly documented in the Serialized Form section of the > javadoc. > > CSR: https://bugs.openjdk.java.net/browse/JDK-8277128 This pull request has now been integrated. Changeset: a44b45fd Author:Sean Mullan URL: https://git.openjdk.java.net/jdk/commit/a44b45fdf31275a2c1e9d1d0132874a7de45f8ee Stats: 29 lines in 2 files changed: 7 ins; 0 del; 22 mod 4337793: Mark non-serializable fields of java.security.cert.Certificate and CertPath Reviewed-by: valeriep, rriggs - PR: https://git.openjdk.java.net/jdk/pull/6392
