<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<div class="moz-cite-prefix">On 1/18/2022 4:10 PM, Sean Mullan
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:9KbehHQkYt97fRvntUumETKjqIr97U-tp9FqEVuDMWs=.c40ceda1-3a60-48b5-a1af-814523e010f2@github.com">
<pre class="moz-quote-pre" wrap="">On Thu, 6 Jan 2022 20:28:22 GMT, Sean Mullan <a class="moz-txt-link-rfc2396E" href="mailto:mullan@openjdk.org"><mullan@openjdk.org></a> wrote:
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">@seanjmullan @wangweij I have commented on what you pointed out, so could you please reply?
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I need another couple of days to look at this issue again before I can reply.
-------------
PR: <a class="moz-txt-link-freetext" href="https://git.openjdk.java.net/jdk/pull/6928">https://git.openjdk.java.net/jdk/pull/6928</a>
</pre>
</blockquote>
<p>Hi - <br>
</p>
<p>Bouncycastle started enforcing properly encoded INTEGERs a while
back and that caused one of my programs to start failing due to a
TPM X509 endorsement certificate just having the TPM serial number
stuffed into the certificate serial number without normalizing it
to the appropriate INTEGER encoding. BC provided a work around
(setting "org.bouncycastle.asn1.allow_unsafe_integer") which gave
me time to re-code around the problem. If you're going to break
things, it may be useful to provide a work around similar to what
they did.<br>
</p>
<p>In any event, DerValue.java uses "new String(byteArrayValue,
charsetType)" to produce the various String values including in
getIA5String(). I.e.,<br>
</p>
<p>
<blockquote type="cite">
<pre>public String(byte[] bytes,
<a title="class in java.nio.charset">Charset</a> charset)</pre>
<div class="block">Constructs a new <code>String</code> by
decoding the specified array of bytes using the specified <a
title="class in java.nio.charset">charset</a>. The length of
the new <code>String</code> is a function of the charset, and
hence may not be equal to the length of the byte array.
<p> <u><b>This method always replaces malformed-input and
unmappable-character sequences with this charset's
default replacement string.</b></u> The <a title="class
in java.nio.charset"><code>CharsetDecoder</code></a> class
should be used when more control over the decoding process
is required.</p>
</div>
</blockquote>
</p>
<p>Perhaps it might make sense to update the various places where
this is used in DerValue to CharsetDecoder and to use
charsetDecoder.onMalformedInput() and
charsetDecoder.onUnmappableCharacter() to set the appropriate
action related to parsing the byte array into a String of a given
charset? That action could be set based on the presence of the
bypass property.</p>
<p>I don't think the change as proposed is backward-compatible safe
enough, nor does it really address the general issue of poorly
encoded DER String values in a certificate.<br>
</p>
<p>Mike</p>
<p><br>
</p>
</body>
</html>