Hi Sean -
Inline.
On 1/21/2022 11:33 AM, Sean Mullan wrote:
On Fri, 14 Jan 2022 11:18:23 GMT, Masanori Yano<my...@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.
Masanori Yano has updated the pull request incrementally with one additional
commit since the last revision:
8255739: x509Certificate returns � for invalid subjectAlternativeNames
_Mailing list message from [Michael StJohns](mailto:mstjo...@comcast.net) on
[security-dev](mailto:security-...@mail.openjdk.java.net):_
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.
Do you know the behavior of the JDK X.509 CertificateFactory implementation?
Did it accept or reject this serial number?
It accepted the serial number:
[
[
Version: V3
Subject:
Signature Algorithm: SHA256withECDSA, OID = 1.2.840.10045.4.3.2
Key: Sun RSA public key, 2048 bits
modulus:
244030580540745092613654475993648434932553330564847517407727188658248
32223177660143311229959664585582409529625785574450992069681603560492386013716136
32364832338166792867574600908839414339721021812629840173006767022634407898831293
85934831807840338360685996173024268943864659869938519459993447390570376982441341
45952422087437605410761245329340711833296479315725697546185458730724484238852701
80315770789789435860018869046480700786553465339467127182438028195537091676054789
84722755281453369500125757853796162260084162669462853871135753998130894343437638
04195331153338279046418652746376364246586098919744901616926689536893
public exponent: 65537
Validity: [From: Sun Nov 26 20:52:10 EST 2017,
To: Thu Dec 30 19:00:00 EST 2049]
Issuer: CN=www.intel.com, OU=TPM EK intermediate for
SPTH_EPID_PROD_RK_0, O=In
tel Corporation, L=Santa Clara, ST=CA, C=US
_*SerialNumber: [ 048fe61d 2882d3cd 488ab130 b94fbc89 284b32]*_
Certificate Extensions: 9
[1]: ObjectId: 2.5.29.9 Criticality=false
Extension unknown: DER encoded OCTET string =
0000: 04 1A 30 18 30 16 06 05 67 81 05 02 10 31 0D 30 ..0.0...g....1.0
0010: 0B 0C 03 32 2E 30 02 01 00 02 01 5D ...2.0.....]
[2]: ObjectId: 1.3.6.1.5.5.7.1.1 Criticality=false
AuthorityInfoAccess [
[
accessMethod: caIssuers
accessLocation: URIName:
http://upgrades.intel.com/content/CRL/ekcert/SPTH_EP
ID_PROD_RK_0.cer
]
]
[3]: ObjectId: 2.5.29.35 Criticality=false
AuthorityKeyIdentifier [
KeyIdentifier [
0000: 6C A9 DF 62 A1 AA E2 3E 0F EB 7C 3F 5E B8 E6 1E l..b...>...?^...
0010: CA C1 7C B7 ....
]
]
[4]: ObjectId: 2.5.29.19 Criticality=true
BasicConstraints:[
CA:false
PathLen: undefined
]
[5]: ObjectId: 2.5.29.31 Criticality=false
CRLDistributionPoints [
[DistributionPoint:
[URIName:
http://upgrades.intel.com/content/CRL/ekcert/SPTH_EPID_PROD_RK_0.
crl]
]]
[6]: ObjectId: 2.5.29.32 Criticality=false
CertificatePolicies [
[CertificatePolicyId: [1.2.840.113741.1.5.2.1]
[PolicyQualifierInfo: [
qualifierID: 1.3.6.1.5.5.7.2.1
qualifier: 0000: 16 46 68 74 74 70 3A 2F 2F 75 70 67 72 61 64 65
.Fhttp://u
pgrade
0010: 73 2E 69 6E 74 65 6C 2E 63 6F 6D 2F 63 6F 6E 74 s.intel.com/cont
0020: 65 6E 74 2F 43 52 4C 2F 65 6B 63 65 72 74 2F 45 ent/CRL/ekcert/E
0030: 4B 63 65 72 74 50 6F 6C 69 63 79 53 74 61 74 65 KcertPolicyState
0040: 6D 65 6E 74 2E 70 64 66 ment.pdf
], PolicyQualifierInfo: [
qualifierID: 1.3.6.1.5.5.7.2.2
qualifier: 0000: 30 2A 0C 28 54 43 50 41 20 54 72 75 73 74 65 64
0*.(TCPA T
rusted
0010: 20 50 6C 61 74 66 6F 72 6D 20 4D 6F 64 75 6C 65 Platform Module
0020: 20 45 6E 64 6F 72 73 65 6D 65 6E 74 Endorsement
]] ]
]
[7]: ObjectId: 2.5.29.37 Criticality=false
ExtendedKeyUsages [
2.23.133.8.1
]
[8]: ObjectId: 2.5.29.15 Criticality=true
KeyUsage [
Key_Encipherment
]
[9]: ObjectId: 2.5.29.17 Criticality=true
SubjectAlternativeName [
OID.2.23.133.2.3=id:00020000, OID.2.23.133.2.2=SPT,
OID.2.23.133.2.1=id:494E54
43
]
]
Algorithm: [SHA256withECDSA]
Signature:
0000: 30 45 02 20 6C 91 72 DF DC 9E 59 AB 57 81 E2 FC 0E. l.r...Y.W...
0010: 00 EA 0A 0D A7 4D 94 0F 0E FA BA E1 30 08 19 E5 .....M......0...
0020: CF 25 33 27 02 21 00 E6 F0 C0 75 EF 8E C6 C3 84 .%3'.!....u.....
0030: 54 9F 7A DD D5 A6 5F E0 7C 42 D7 5A B0 8A 1A 1C T.z..._..B.Z....
0040: B3 6E D6 56 8F A2 44 .n.V..D
]
Here's the ASN1 for the serial number:
02 14
00 04 8f e6 1d 28 82 d3 cd 48 8a b1 30 b9 4f bc 89 28 4b 32
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.
Here's what org.bouncycastle.asn1.ASN1Integer does:
switch (bytes.length)
{
case 0:
return true;
case 1:
return false;
default:
return bytes[0] == (bytes[1] >> 7)
// Apply loose validation, see note in public
constructor ASN1Integer(byte[])
&&
!Properties.isOverrideSet("org.bouncycastle.asn1.allow_unsafe_integer");
}
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.
E.g. according to 8.3.2 of X.690:
If the contents octets of an integer value encoding consist of more
than one octet, then the bits of the first octet
and bit 8 of the second octet
a)shall not all be ones; and
b)shall not all be zero.
NOTE – These rules ensure that an integer value is always encoded in
the smallest possible number of octets
So the JDK implementation violates the standard.
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 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.
I wish CertificateFactorylgetInstance() could accept a parameter object
describing how strict or loose an encoding would be accepted by the
parsing engine.
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.
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.
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).
Yes.
Thanks - Mike
See
alsohttps://groups.google.com/g/mozilla.dev.security.policy/c/Av6oZxbjvB4/m/NW6lGkgYBwAJ
and the linked report of various anomalies in commonNames and Subject
Alternative Names in server auth certificates issued by public CAs.
-------------
PR:https://git.openjdk.java.net/jdk/pull/6928
