RFR: 8255739: x509Certificate returns � for invalid subjectAlternativeNames

[Sean Mullan]

On Thu, 6 Jan 2022 20:28:22 GMT, Sean Mullan <mullan at openjdk.org> wrote:

>> Could you please review the JDK-8255739 bug fix?
>> 
>> I think sun.security.x509.SubjectAlternativeNameExtension() should throw an exception for incorrect SubjectAlternativeNames instead of returning the substituted characters, which is explained in the description of BugDB.
>> 
>> I modified DerValue.readStringInternal() not to read incorrect SubjectAlternativeNames and throw an IOException. sun.security.x509.X509CertInfo.parse() catch the IOExcepton and ignore it if SAN is a non-ciritical extension like the behavior of the IOException in readStringInternal(). So I added a test with -Djava.security.debug=x509 to confirm that.
>
> I understand the reasons for making the code more robust and detecting invalid DER encodings, but this may have a non-trivial compatibility risk. In general, I think detecting invalid encodings is generally the right thing to do, but compatibility needs to be considered. Sometimes other implementations have encoding bugs that we need to workaround, etc. This change affects not only certificates but other security components that use DER in the JDK. Certificates already treat non-critical extensions that are badly encoded as not a failure, so there is some compatibility built-in already. But I think it makes sense to look at other code that calls into the DerValue methods and evaluate the potential compatibility risk. At a minimum, a CSR must be filed. As a compromise, it may make sense to (at least initially) reduce the compatibility risk by allowing the caller (ex: `sun.security.x509.DNSName`) to decide if it wants a stricter parsing of the DER-encoded string.
> 
> I would like @wangweij or @valeriepeng to also review this. 
> 
> With respect to the test, it seems like overkill to launch a java process inside the test to run each test. Instead, I would just have separate methods for each test and run them in the same process as the main test.

> @seanjmullan @wangweij I have commented on what you pointed out, so could you please reply?

I need another couple of days to look at this issue again before I can reply.

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

PR: https://git.openjdk.java.net/jdk/pull/6928