Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Java Restricted Security Mode #663

Merged
merged 1 commit into from
May 25, 2023
Merged

Conversation

taoliult
Copy link
Contributor

Signed-off-by: Tao Liu [email protected]

This is a backport PR from JDKNext PR ibmruntimes/openj9-openjdk-jdk#544 to JDK8.

@taoliult
Copy link
Contributor Author

taoliult commented Apr 25, 2023

@keithc-ca @pshipton

Java Restricted Security Mode backport PR created from JDKNext PR ibmruntimes/openj9-openjdk-jdk#544 to JDK8. And there are some updates only for the JDK8. Please help to review and advice.

And for the copyright check, it said below. Do we also need to add the copyright for the java.security file?

17:04:02  The following files were modified and have incorrect copyrights
[Pipeline] echo
17:04:02  jdk/src/share/lib/security/java.security-linux
17:04:02  	IBM Portions Copyright should be used in this file

@jasonkatonica @WilburZjh

FYI.

jdk/src/share/classes/java/security/Provider.java Outdated Show resolved Hide resolved
jdk/src/share/classes/java/security/SecureRandom.java Outdated Show resolved Hide resolved
jdk/src/share/classes/java/util/ServiceLoader.java Outdated Show resolved Hide resolved
Comment on lines 109 to 134
RestrictedSecurity1.jce.provider.1 = sun.security.pkcs11.SunPKCS11 ${java.home}/lib/security/nss.fips.cfg
RestrictedSecurity1.jce.provider.2 = sun.security.provider.Sun [{CertificateFactory, X.509, *}, \
{CertStore, Collection, *}, \
{CertStore, com.sun.security.IndexedCollection, *}, \
{Policy, JavaPolicy, *}, {Configuration, JavaLoginConfig, *}, \
{CertPathBuilder, PKIX, *}, \
{CertPathValidator, PKIX, *}]
RestrictedSecurity1.jce.provider.3 = sun.security.ec.SunEC [{KeyFactory, EC, *}, \
{AlgorithmParameters, EC, *}]
RestrictedSecurity1.jce.provider.4 = com.sun.net.ssl.internal.ssl.Provider
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the configuration is expected to use fully-qualified names (as is done here), I think the code should do likewise and not remove the package name.

In newer versions, there are several "attributes" specified for these providers: why are they not copied here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In JDK8, the security.provider lists are the provider class names with packages. In JDK11 and above version, the security.provider lists are the provider names which defined in provider construction method. But no matter it is provider class name or the provider name defined in provider construction method, the new added getProvidersSimpleName() method will return the provider name defined in provider construction method, and then store it into the providersSimpleName string list.

But right now, for the following provider classes and their provider names, I did not add them into the getProvidersSimpleName() method. Because they are not FIPS allowed providers.

sun.security.jgss.SunProvider  SunJGSS
com.sun.security.sasl.Provider SunSASL
org.jcp.xml.dsig.internal.dom.XMLDSigRI. XMLDSig

In JDK8 providers SUN and SunEC, they are using the legacy method Provider.put() to register service, its alias and its attribute. And it did one by one, example as follow. For “AlgorithmParameters.EC” attributes “KeySize” and “ImplementedIn”, there are two items/attributes in the map.

        /*
         * Algorithm Parameter engine
         */
        map.put("AlgorithmParameters.EC", "sun.security.util.ECParameters");
        map.put("Alg.Alias.AlgorithmParameters.EllipticCurve", "EC");
        map.put("Alg.Alias.AlgorithmParameters.1.2.840.10045.2.1", "EC");
        map.put("AlgorithmParameters.EC KeySize", "256");
        map.put("AlgorithmParameters.EC ImplementedIn", "Software");

And then, in the Provider.parseLegacyPut() method, the items/attributes will be read one by one, and then using addAttribute() method to put into Service.

Our check is in the Provider.parseLegacyPut() method, it will check for each item/attribute in the map. So, if the service has more than two items/attributes in the map. We can’t check it correctly, because our isServiceAllowed() method will check the service and all its attributes.

So, first, what I thought is to remove the attributes (set it as *). Because when I went through those providers’ services, I did not find any service which has the same type and algorithms, but different attributes. That mean, only according to the type and algorithms, we can find the specific services, so checking attributes or not, actually doesn’t matter.

But, right now, there is a better way, that is to add the isServiceAllowed() check in Provider.removeInvalidServices() method. This method will be called after the Provider.parseLegacyPut() went through all the items in the map and added them as services into legacyMap. So, we can check each service from the legacyMap in Provider.removeInvalidServices() method, to remove them if the service is not allowed. In this case, the isServiceAllowed() check will work correctly.

@taoliult taoliult force-pushed the fips branch 3 times, most recently from 2b9e5f6 to 4b1c851 Compare May 14, 2023 23:07
@taoliult
Copy link
Contributor Author

@keithc-ca Some reviews updated the codes, the others replied the questions. Please help to review and advise.

@jasonkatonica @WilburZjh FYI.

Comment on lines +773 to +757
if (!isAsterisk(cType) && !type.equalsIgnoreCase(cType)) {
// The constraint doesn't apply to the service type.
continue;
}
if (!isAsterisk(cAlgorithm) && !algorithm.equals(cAlgorithm)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In jdk11, type is case-sensitive and algorithm is not: why is this different?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, it doesn’t affect any functions. Because the algorithms from the java.security restricted security provider constraint list, are exactly same as the ones which registered in the provider java codes. But, since the type is using “equalsIgnoreCase()”, so I would like to use the same “equalsIgnoreCase()” for the algorithms, to make them look the same(by using the same equalsIgnoreCase() method). So, I will create a PR on OpenJDK Next first and then backport this change back.

debug.println("Security constraints check."
+ " Service type: " + type
+ " Algorithm: " + algorithm
+ " is allowed in provider: " + providerName);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you changing debug message (as compared with newer versions)?
See also line 835.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compared to the other debug messages in this method, others had a “colon” right after the “is allowed in provider”. For example this debug message:

                debug.println(
                        "Security constraints check."
                                + " Service type: " + type
                                + " Algorithm: " + algorithm
                                + " Attribute: " + cAttribute
                                + " is allowed in provider: " + providerName);

So, for look as the same, I updated this line 788 and line 831 by adding a “:” in the debug message. I will make the same changes for JDK11 and above version by another PR.

jdk/src/share/classes/java/util/ServiceLoader.java Outdated Show resolved Hide resolved
Comment on lines 81 to 103
RestrictedSecurity1.desc.number = Certificate #3946
RestrictedSecurity1.desc.policy = https://csrc.nist.gov/projects/cryptographic-module-validation-program/certificate/3946
RestrictedSecurity1.desc.sunsetDate = 2026-06-06

RestrictedSecurity1.tls.disabledNamedCurves =
RestrictedSecurity1.tls.disabledAlgorithms = X25519, X448, SSLv3, TLSv1, TLSv1.1, \
TLS_CHACHA20_POLY1305_SHA256, TLS_DHE_RSA_WITH_AES_256_GCM_SHA384, \
TLS_DHE_DSS_WITH_AES_256_GCM_SHA384, TLS_DHE_RSA_WITH_AES_128_GCM_SHA256, \
TLS_DHE_DSS_WITH_AES_128_GCM_SHA256, TLS_DHE_RSA_WITH_AES_256_CBC_SHA256, \
TLS_DHE_DSS_WITH_AES_256_CBC_SHA256, TLS_DHE_RSA_WITH_AES_128_CBC_SHA256, \
TLS_DHE_DSS_WITH_AES_128_CBC_SHA256, TLS_DHE_RSA_WITH_AES_256_CBC_SHA, \
TLS_DHE_DSS_WITH_AES_256_CBC_SHA, TLS_DHE_RSA_WITH_AES_128_CBC_SHA, \
TLS_DHE_DSS_WITH_AES_128_CBC_SHA, TLS_RSA_WITH_AES_256_GCM_SHA384, \
TLS_RSA_WITH_AES_128_GCM_SHA256, TLS_RSA_WITH_AES_256_CBC_SHA256, \
TLS_RSA_WITH_AES_128_CBC_SHA256, TLS_RSA_WITH_AES_256_CBC_SHA, \
TLS_RSA_WITH_AES_128_CBC_SHA, TLS_AES_256_GCM_SHA384, \
TLS_AES_128_GCM_SHA256, TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, \
TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, \
TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, TLS_ECDH_ECDSA_WITH_AES_256_GCM_SHA384, \
TLS_ECDH_RSA_WITH_AES_256_GCM_SHA384, TLS_ECDH_ECDSA_WITH_AES_128_GCM_SHA256, \
TLS_ECDH_RSA_WITH_AES_128_GCM_SHA256, TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256, \
TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256, TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256, \
TLS_EMPTY_RENEGOTIATION_INFO_SCSV
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this section different than newer versions (e.g. Java 11)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In OpenJDK11 and above version, there are two PRs update this “RestrictedSecurity1.tls.disabledAlgorithms” list. First PR for the Java Restricted Security Mode add this list, second PR remove the AES-GCM mode from the disable list because the latest FIPS policy support the AES-GCM mode.

So, for the OpenJDK8, I updated to the latest FIPS policy and also removed the AES-GCM mode from this disable list. So, the list will be same as the current OpenJDK11 and above version, and we don’t need a separate PR for updating the FIPS policy and removing the AES-GCM mode again.

Comment on lines +109 to +134
RestrictedSecurity1.jce.provider.1 = sun.security.pkcs11.SunPKCS11 ${java.home}/lib/security/nss.fips.cfg
RestrictedSecurity1.jce.provider.2 = sun.security.provider.Sun \
[{CertificateFactory, X.509, ImplementedIn=Software}, \
{CertPathBuilder, PKIX, ValidationAlgorithm=RFC5280:ImplementedIn=Software}, \
{CertPathValidator, PKIX, ValidationAlgorithm=RFC5280:ImplementedIn=Software}, \
{CertStore, Collection, ImplementedIn=Software}, \
{CertStore, com.sun.security.IndexedCollection, ImplementedIn=Software}, \
{Configuration, JavaLoginConfig, *}, \
{Policy, JavaPolicy, *}]
RestrictedSecurity1.jce.provider.3 = sun.security.ec.SunEC \
[{AlgorithmParameters, EC, *}, \
{KeyFactory, EC, ImplementedIn=Software}]
RestrictedSecurity1.jce.provider.4 = com.sun.net.ssl.internal.ssl.Provider
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the provider names, why is this different than newer versions (e.g. Java 11)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In OpenJDK8, the attribute of KeyFactory EC is different with the OpenJDK11 and above version, KeyFactory EC only has one attribute in OpenJDK8 SunEC provider. So, that is why the KeyFactory EC attribute is different between OpenJDK8 and OpenJDK11 and above version.

@taoliult taoliult force-pushed the fips branch 2 times, most recently from 6d47c5b to f3b0944 Compare May 19, 2023 19:53
@taoliult
Copy link
Contributor Author

@keithc-ca Some reviews updated the codes, the others replied the questions. Please help to review and advise.

@keithc-ca
Copy link
Member

Jenkins test sanity,extended xlinux jdk8

Copy link
Member

@keithc-ca keithc-ca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test failure is unrelated to this change:

testJITServer_1:
//// can't bind server address: Address already in use

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants