On Thu, 6 Jan 2022 20:28:22 GMT, Sean Mullan <mul...@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? ------------- PR: https://git.openjdk.java.net/jdk/pull/6928
