On 1/18/2022 4:10 PM, Sean Mullan wrote:
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?
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
Hi -
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.
In any event, DerValue.java uses "new String(byteArrayValue,
charsetType)" to produce the various String values including in
getIA5String(). I.e.,
public String(byte[] bytes,
Charset charset)
Constructs a new |String| by decoding the specified array of bytes
using the specified charset. The length of the new |String| is a
function of the charset, and hence may not be equal to the length of
the byte array.
_*This method always replaces malformed-input and unmappable-character
sequences with this charset's default replacement string.*_ The
|CharsetDecoder| class should be used when more control over the
decoding process is required.
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.
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.
Mike
