<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<div class="moz-cite-prefix">Hi Sean - <br>
</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">Inline.<br>
</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">On 1/21/2022 11:33 AM, Sean Mullan
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:X4-zHSGywbQLQTZHZ443XVnoJI15S5hlYT840pNQDHM=.9924f8f5-c253-448f-97a5-d5fc2700e72c@github.com">
<pre class="moz-quote-pre" wrap="">On Fri, 14 Jan 2022 11:18:23 GMT, Masanori Yano <a class="moz-txt-link-rfc2396E" href="mailto:myano@openjdk.org"><myano@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="">
Masanori Yano has updated the pull request incrementally with one additional commit since the last revision:
8255739: x509Certificate returns � for invalid subjectAlternativeNames
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">_Mailing list message from [Michael StJohns](<a class="moz-txt-link-freetext" href="mailto:mstjohns@comcast.net">mailto:mstjohns@comcast.net</a>) on [security-dev](<a class="moz-txt-link-freetext" href="mailto:security-dev@mail.openjdk.java.net">mailto:security-dev@mail.openjdk.java.net</a>):_
On 1/18/2022 4:10 PM, Sean Mullan wrote:
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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Do you know the behavior of the JDK X.509 CertificateFactory implementation? Did it accept or reject this serial number?</pre>
</blockquote>
<p>It accepted the serial number: <br>
</p>
<p>
<blockquote type="cite"><font face="monospace">[<br>
[<br>
Version: V3<br>
Subject:<br>
Signature Algorithm: SHA256withECDSA, OID =
1.2.840.10045.4.3.2<br>
<br>
Key: Sun RSA public key, 2048 bits<br>
modulus:
244030580540745092613654475993648434932553330564847517407727188658248<br>
32223177660143311229959664585582409529625785574450992069681603560492386013716136<br>
32364832338166792867574600908839414339721021812629840173006767022634407898831293<br>
85934831807840338360685996173024268943864659869938519459993447390570376982441341<br>
45952422087437605410761245329340711833296479315725697546185458730724484238852701<br>
80315770789789435860018869046480700786553465339467127182438028195537091676054789<br>
84722755281453369500125757853796162260084162669462853871135753998130894343437638<br>
04195331153338279046418652746376364246586098919744901616926689536893<br>
public exponent: 65537<br>
Validity: [From: Sun Nov 26 20:52:10 EST 2017,<br>
To: Thu Dec 30 19:00:00 EST 2049]<br>
Issuer: CN=www.intel.com, OU=TPM EK intermediate for
SPTH_EPID_PROD_RK_0, O=In<br>
tel Corporation, L=Santa Clara, ST=CA, C=US<br>
<u><b> SerialNumber: [ 048fe61d 2882d3cd 488ab130 b94fbc89
284b32]</b></u><br>
<br>
Certificate Extensions: 9<br>
[1]: ObjectId: 2.5.29.9 Criticality=false<br>
Extension unknown: DER encoded OCTET string =<br>
0000: 04 1A 30 18 30 16 06 05 67 81 05 02 10 31 0D 30
..0.0...g....1.0<br>
0010: 0B 0C 03 32 2E 30 02 01 00 02 01 5D
...2.0.....]<br>
<br>
<br>
[2]: ObjectId: 1.3.6.1.5.5.7.1.1 Criticality=false<br>
AuthorityInfoAccess [<br>
[<br>
accessMethod: caIssuers<br>
accessLocation: URIName:
<a class="moz-txt-link-freetext" href="http://upgrades.intel.com/content/CRL/ekcert/SPTH_EP">http://upgrades.intel.com/content/CRL/ekcert/SPTH_EP</a><br>
ID_PROD_RK_0.cer<br>
]<br>
]<br>
<br>
[3]: ObjectId: 2.5.29.35 Criticality=false<br>
AuthorityKeyIdentifier [<br>
KeyIdentifier [<br>
0000: 6C A9 DF 62 A1 AA E2 3E 0F EB 7C 3F 5E B8 E6 1E
l..b...>...?^...<br>
0010: CA C1 7C B7 ....<br>
]<br>
]<br>
<br>
[4]: ObjectId: 2.5.29.19 Criticality=true<br>
BasicConstraints:[<br>
CA:false<br>
PathLen: undefined<br>
]<br>
<br>
[5]: ObjectId: 2.5.29.31 Criticality=false<br>
CRLDistributionPoints [<br>
[DistributionPoint:<br>
[URIName:
<a class="moz-txt-link-freetext" href="http://upgrades.intel.com/content/CRL/ekcert/SPTH_EPID_PROD_RK_0">http://upgrades.intel.com/content/CRL/ekcert/SPTH_EPID_PROD_RK_0</a>.<br>
crl]<br>
]]<br>
<br>
[6]: ObjectId: 2.5.29.32 Criticality=false<br>
CertificatePolicies [<br>
[CertificatePolicyId: [1.2.840.113741.1.5.2.1]<br>
[PolicyQualifierInfo: [<br>
qualifierID: 1.3.6.1.5.5.7.2.1<br>
qualifier: 0000: 16 46 68 74 74 70 3A 2F 2F 75 70 67 72 61
64 65 .Fhttp://u<br>
pgrade<br>
0010: 73 2E 69 6E 74 65 6C 2E 63 6F 6D 2F 63 6F 6E 74
s.intel.com/cont<br>
0020: 65 6E 74 2F 43 52 4C 2F 65 6B 63 65 72 74 2F 45
ent/CRL/ekcert/E<br>
0030: 4B 63 65 72 74 50 6F 6C 69 63 79 53 74 61 74 65
KcertPolicyState<br>
0040: 6D 65 6E 74 2E 70 64 66
ment.pdf<br>
<br>
], PolicyQualifierInfo: [<br>
qualifierID: 1.3.6.1.5.5.7.2.2<br>
qualifier: 0000: 30 2A 0C 28 54 43 50 41 20 54 72 75 73 74
65 64 0*.(TCPA T<br>
rusted<br>
0010: 20 50 6C 61 74 66 6F 72 6D 20 4D 6F 64 75 6C 65
Platform Module<br>
0020: 20 45 6E 64 6F 72 73 65 6D 65 6E 74
Endorsement<br>
<br>
]] ]<br>
]<br>
<br>
[7]: ObjectId: 2.5.29.37 Criticality=false<br>
ExtendedKeyUsages [<br>
2.23.133.8.1<br>
]<br>
<br>
[8]: ObjectId: 2.5.29.15 Criticality=true<br>
KeyUsage [<br>
Key_Encipherment<br>
]<br>
<br>
[9]: ObjectId: 2.5.29.17 Criticality=true<br>
SubjectAlternativeName [<br>
OID.2.23.133.2.3=id:00020000, OID.2.23.133.2.2=SPT,
OID.2.23.133.2.1=id:494E54<br>
43<br>
]<br>
<br>
]<br>
Algorithm: [SHA256withECDSA]<br>
Signature:<br>
0000: 30 45 02 20 6C 91 72 DF DC 9E 59 AB 57 81 E2 FC 0E.
l.r...Y.W...<br>
0010: 00 EA 0A 0D A7 4D 94 0F 0E FA BA E1 30 08 19 E5
.....M......0...<br>
0020: CF 25 33 27 02 21 00 E6 F0 C0 75 EF 8E C6 C3 84
.%3'.!....u.....<br>
0030: 54 9F 7A DD D5 A6 5F E0 7C 42 D7 5A B0 8A 1A 1C
T.z..._..B.Z....<br>
0040: B3 6E D6 56 8F A2 44
.n.V..D<br>
<br>
]</font><br>
</blockquote>
<br>
</p>
<p>Here's the ASN1 for the serial number:</p>
<p>
<blockquote type="cite"> 02 14 <br>
00 04 8f e6 1d 28 82 d3 cd 48 8a b1 30 b9 4f bc 89 28 4b 32 </blockquote>
In this case, the encoding should not have included the leading
'00' byte. That 20 byte value is what I get if I query the TPM
directly for its serial number. <br>
</p>
<p>Here's what org.bouncycastle.asn1.ASN1Integer does:</p>
<p>
<blockquote type="cite"> <font face="monospace"> switch
(bytes.length)<br>
{<br>
case 0:<br>
return true;<br>
case 1:<br>
return false;<br>
default:<br>
return bytes[0] == (bytes[1] >> 7)<br>
// Apply loose validation, see note in public
constructor ASN1Integer(byte[])<br>
&&
!Properties.isOverrideSet("org.bouncycastle.asn1.allow_unsafe_integer");<br>
}</font></blockquote>
</p>
<p>Basically returns "true" to the question "isMalformatted" if
there is a leading zero byte and the high bit of the second byte
isn't set or if the first is 0xff and the high bit of the second
byte is set. <br>
</p>
<p>E.g. according to 8.3.2 of X.690:</p>
<p>
<blockquote type="cite"><span style="left: 156.924px; top:
937.781px; font-size: 16.7px; font-family: serif; transform:
scaleX(1.00892);" role="presentation" dir="ltr" class="">If
the contents octets of an integer value encoding consist of
more than one octet, then the bits of the first octet</span><br
role="presentation">
<span style="left: 90.725px; top: 957.286px; font-size: 16.7px;
font-family: serif; transform: scaleX(0.999872);"
role="presentation" dir="ltr">and bit 8 of the second octet</span><br
role="presentation">
<span style="left: 156.924px; top: 984.891px; font-size: 16.7px;
font-family: serif; transform: scaleX(0.999427);"
role="presentation" dir="ltr">a)</span><span style="left:
169.9px; top: 984.891px; font-size: 16.7px; font-family:
serif;" role="presentation" dir="ltr"> </span><span
style="left: 190.023px; top: 984.891px; font-size: 16.7px;
font-family: serif; transform: scaleX(0.999983);"
role="presentation" dir="ltr">shall not all be ones; and</span><br
role="presentation">
<span style="left: 156.924px; top: 1012.5px; font-size: 16.7px;
font-family: serif; transform: scaleX(0.9996);"
role="presentation" dir="ltr">b)</span><span style="left:
170.835px; top: 1012.5px; font-size: 16.7px; font-family:
serif;" role="presentation" dir="ltr"> </span><span
style="left: 190.023px; top: 1012.5px; font-size: 16.7px;
font-family: serif; transform: scaleX(1.00002);"
role="presentation" dir="ltr">shall not all be zero.</span><br
role="presentation">
<span style="left: 156.925px; top: 1036.76px; font-size: 15px;
font-family: serif; transform: scaleX(1.00041);"
role="presentation" dir="ltr" class="">NOTE – These rules
ensure that an integer value is always encoded in the smallest
possible number of octets</span></blockquote>
</p>
<p>So the JDK implementation violates the standard. <br>
</p>
<blockquote type="cite"
cite="mid:X4-zHSGywbQLQTZHZ443XVnoJI15S5hlYT840pNQDHM=.9924f8f5-c253-448f-97a5-d5fc2700e72c@github.com">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">In any event, DerValue.java uses "new String(byteArrayValue, charsetType)" to produce the various String values including in getIA5String().? I.e.,
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I believe that was originally suggested by the submitter as a potential solution. But it doesn't seem like it is that useful or there would be much demand for this.</pre>
</blockquote>
<p>I wish CertificateFactorylgetInstance() could accept a parameter
object describing how strict or loose an encoding would be
accepted by the parsing engine. <br>
</p>
<p>On the more general note of whether there's demand - it would be
useful if there were a way to avoid accepting certificates that
are malformatted. I originally started using the BC certificate
factory because the SUN factory didn't understand RSA-OAEP as a
key type in SubjectKeyInfo and I was getting a few of those from a
group of TPMs. But I do understand the limitations of resources.</p>
<blockquote type="cite"
cite="mid:X4-zHSGywbQLQTZHZ443XVnoJI15S5hlYT840pNQDHM=.9924f8f5-c253-448f-97a5-d5fc2700e72c@github.com">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Yes, I have come to the conclusion that this is probably too risky to fix. An application that is depending on a DNSName should be doing additional matching checks to verify that the structure of the name is correct, and it doesn't violate other types of rules, such as wildcards involving public suffixes, etc. (The JDK code does these additional checks when verifying TLS server certificates).</pre>
</blockquote>
<p>Yes.</p>
<p>Thanks - Mike</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:X4-zHSGywbQLQTZHZ443XVnoJI15S5hlYT840pNQDHM=.9924f8f5-c253-448f-97a5-d5fc2700e72c@github.com">
<pre class="moz-quote-pre" wrap="">
See also <a class="moz-txt-link-freetext" href="https://groups.google.com/g/mozilla.dev.security.policy/c/Av6oZxbjvB4/m/NW6lGkgYBwAJ">https://groups.google.com/g/mozilla.dev.security.policy/c/Av6oZxbjvB4/m/NW6lGkgYBwAJ</a> and the linked report of various anomalies in commonNames and Subject Alternative Names in server auth certificates issued by public CAs.
-------------
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><br>
</p>
</body>
</html>