RFR: 8296742: Illegal X509 Extension should not be created [v8]
Sean Mullan
mullan at openjdk.org
Tue Nov 22 19:59:31 UTC 2022
On Tue, 22 Nov 2022 17:42:06 GMT, Weijun Wang <weijun at openjdk.org> wrote:
>> Inside JDK we support a lot of X.509 certificate extensions. Almost every extension has a rule about what is legal or not. For example, the names in `SubjectAlternativeNameExtension` cannot be missing or empty. Usually, a rule is enforced in the `encode()` method, where the extension value is assigned null for illegal extension and the method throws an `IOException`. However, before the `encode()` method is called, the illegal extension can always be created successfully, whether from a constructor using extension components (For example, `new SubjectAlternativeNameExtension(names)`) or using the encoded value (for example, `new SubjectAlternativeNameExtension(derEncoding)`).
>>
>> This code change tries to prevent illegal extensions from being created right from the beginning but the solution is not complete. Precisely, for constructors using extension components, new checks are added to ensure the correct components are provided and the extension can be encoded correctly. Fortunately, most of these conditions are already met inside JDK calls to them. The only exception is inside the `keytool -gencrl` command where the reason code of a revoked certificate could be zero. This has been fixed in this code change. There are some constructors having no arguments at all. These are useless and also removed.
>>
>> On the other hand, constructors using the encoded value are complicated. Some of them check for legal values, some do not. However, since the encoding is read from the argument and already stored inside the object, there is no need to calculate the encoding in the `encode()` method and this method always succeed.
>>
>> In short, while we cannot ensure the extensions created are perfectly legal, we ensure their `encode()` methods are always able to find a non-null extension value to write out.
>>
>> More fine comments in the code change.
>
> Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
>
> IssuerAlternativeNameExtension names
More comments.
src/java.base/share/classes/sun/security/x509/CertificatePoliciesExtension.java line 113:
> 111: public CertificatePoliciesExtension(Boolean critical,
> 112: List<PolicyInformation> certPolicies) throws IOException {
> 113: if (certPolicies == null || certPolicies.isEmpty()) {
You should probably also change `PolicyInformation(CertificatePolicyId policyIdentifier, Set<PolicyQualifierInfo> policyQualifiers)` to check for a null policyIdentifier and an empty policyQualifiers set.
src/java.base/share/classes/sun/security/x509/Extension.java line 178:
> 176:
> 177: Objects.requireNonNull(extensionId,
> 178: "Null OID to encode for the extension!");
I would probably drop the exclamation points. And change "Null OID" to "No OID".
src/java.base/share/classes/sun/security/x509/NameConstraintsExtension.java line 142:
> 140: if (permitted == null && excluded == null) {
> 141: throw new IllegalArgumentException(
> 142: "permitted and exclude cannot both be null");
s/exclude/excluded/
src/java.base/share/classes/sun/security/x509/PolicyMappingsExtension.java line 78:
> 76: * Create a PolicyMappings with the List of CertificatePolicyMap.
> 77: *
> 78: * @param maps the List of CertificatePolicyMap, cannot be null or empty.
Do you want to add similar null checks to the `CertificatePolicyMap` ctor?
src/java.base/share/classes/sun/security/x509/SubjectAlternativeNameExtension.java line 91:
> 89: */
> 90: public SubjectAlternativeNameExtension(Boolean critical, GeneralNames names)
> 91: throws IOException {
`GeneralNames` should probably also be modified so that it cannot contain an empty or null List.
src/java.base/share/classes/sun/security/x509/SubjectInfoAccessExtension.java line 89:
> 87: if (accessDescriptions == null || accessDescriptions.isEmpty()) {
> 88: throw new IllegalArgumentException(
> 89: "AccessDescription cannot be null or empty");
s/AccessDescription/accessDescriptions/
src/java.base/share/classes/sun/security/x509/SubjectKeyIdentifierExtension.java line 76:
> 74: */
> 75: public SubjectKeyIdentifierExtension(byte[] octetString)
> 76: throws IOException {
Do you want to change the `KeyIdentifer` ctor to check for null instead of letting it throw NPE?
-------------
PR: https://git.openjdk.org/jdk/pull/11137
More information about the security-dev
mailing list