RFR: 8296742: Illegal X509 Extension should not be created [v6]

[Sean Mullan]

On Thu, 17 Nov 2022 23:52:02 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:
> 
>   add test
>   
>   only in patch2:
>   unchanged:

A couple of initial comments, will finish review later.

src/java.base/share/classes/sun/security/x509/CRLReasonCodeExtension.java line 72:

> 70:      *
> 71:      * @param critical true if the extension is to be treated as critical.
> 72:      * @param reason the enumerated value for the reason code, cannot be null.

s/null/0/

src/java.base/share/classes/sun/security/x509/CRLReasonCodeExtension.java line 76:

> 74:     public CRLReasonCodeExtension(boolean critical, int reason)
> 75:             throws IOException {
> 76:         if (reason == 0) {

Do you also want to reject reason codes < 0?

-------------

PR: https://git.openjdk.org/jdk/pull/11137