Re: RFR: 8255739: x509Certificate returns � for invalid subjectAlternativeNames [v2]
On Fri, 14 Jan 2022 11:18:23 GMT, Masanori Yano 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 Thank you for discussing this with many comments. I understood that the fix is risky, requires additional matching checks, and is preferable to be selectable by a parameter. I would like to consider them, but it will take a little time to reflect them in the change. - PR: https://git.openjdk.java.net/jdk/pull/6928
Re: RFR: 8255739: x509Certificate returns � for invalid subjectAlternativeNames [v2]
On 1/24/2022 2:23 PM, Weijun Wang wrote:
On Fri, 14 Jan 2022 11:18:23 GMT, Masanori Yano 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
Hi Mike, we don't support an OAEP key (i.e. RSA keys with OAEP parameters). If
you want OAEP encryption, just use a plain RSA key and pass an
OAEPParameterSpec to the RSA Cipher's init method.
-
PR: https://git.openjdk.java.net/jdk/pull/6928
Hi -
The RSA key just used a different AlgorithmIdentifier from the normal
parameterless rsaEncryption AlgorithmIdentifier. This issue basically:
https://github.com/openssl/openssl/issues/7907. The TCG originally
specified that the EK certificate contained the TPM's endorsement public
key would be identified by the that any endorsement key public key
would be identified with the RSAES-OAEP { pkcs-1 7 } object identifier
instead of the rsaEncryption {pkcs-1 1 } OID since the key was only
supposed to be used for decrypting a credential encrypted under OAEP.
e.g. the cert contained this sequence for the SubjectPublicKeyInfo:
SEQUENCE {
175 34: SEQUENCE {
177 9: OBJECT IDENTIFIER rsaOAEP (1 2 840 113549 1 1 7)
188 21: SEQUENCE {
190 19: [2] {
192 17: SEQUENCE {
194 9: OBJECT IDENTIFIER
: rsaOAEP-pSpecified (1 2 840 113549 1 1 9)
205 4: OCTET STRING 'TCPA'
: }
: }
: }
: }
211 271: BIT STRING, encapsulates {
216 266: SEQUENCE {
220 257: INTEGER
: 00 8A 80 D6 D1 A9 AD 54 92 A4 D2 AA F1 8A C8 D3
: 48 12 2E 17 E4 2C FC 75 CF 57 A3 E4 33 EC C8 FD
: 7D 2D 5E B4 13 F9 5D A9 45 51 E1 F1 22 F9 3C DA
: 39 69 AB 8D 85 72 74 D9 97 9C A2 6E D1 BE 9B 93
: 0A 83 1A 9D 2F 30 AA B1 F8 B8 89 04 34 1B 2C 72
: 31 BF 1F 67 44 79 B9 9E 35 A8 99 06 56 7B 92 28
: C4 21 B9 90 7E C8 41 09 F5 88 C2 23 28 DE 25 CA
: AF C2 2B 2E 61 DB 8B 17 83 BB 74 9A 4F A8 D4 A3
: [ Another 129 bytes skipped ]
481 3: INTEGER 65537
: }
: }
: }
Except for the weird AlgorithmIdentifier this is a bog standard RSA
public key.
I don't know that its worth the time to fix this - it's already been
deprecated by the TCG and I haven't seen an endorsement certificate with
this particular encoding in a long while. The certificate this key is
from was issued 26 April 2012 and - for some strange reason - expires
the same date this year.
Later, Mike
Re: RFR: 8255739: x509Certificate returns � for invalid subjectAlternativeNames
On 1/24/2022 9:51 AM, Sean Mullan wrote:
On Sat, 22 Jan 2022 22:48:29 GMT, Michael StJohns wrote:
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.??
Is that still an issue? I would have expected this to be fixed since JDK 11
when we added support for RSASSA-PSS.
*sigh* doing archeology on my code, that may not have been the reason I
started using the BC factory. I *think* I ran into another rogue
certificate that the SUN library couldn't parse, but the BC library
code. However, the RSA-OAEP thing does exist. See below
-
PR: https://git.openjdk.java.net/jdk/pull/6928
Looks like it probably wasn't fixed. This was actually an OID in the
SubjectKeyInfo meant to indicate that the RSA key was solely to be used
for OAEP. I.e. 1.2.840.113549.1.1.7. The place this gets rejected is
in (looking at the current github) - sun/crypto/provider/RSACipher.java
@line 261: RSAKey rsaKey = RSAKeyFactory.toRSAKey(key);
That calls sun/security/rsa/RSAKeyFactory.java::toRSAKey(Key key), which
in turn calls
sun/security/rsa/RSAUtil.KeyType::lookup(key.getAlgorithm()) @line 128.
That fails because the key is neither the rsaEncryption OID
(1.2.840.113549.1.1.1) nor RSASSA_PSS_oid ( .10).
Here's the function I use to fix the encoding. Basically, it changes the
last octet of the key's algorithmID from a '7' to a '1' and regenerates
the key.
public static PublicKey fixPubKey (PublicKey k)
throws PrivacyCAException{
if (!k.getAlgorithm().equals("1.2.840.113549.1.1.7"))
return k; // its not a problem
byte[] encodedKey = k.getEncoded();
encodedKey[16] = (byte)1;
X509EncodedKeySpec kspec = new X509EncodedKeySpec(encodedKey);
try {
KeyFactory kf = KeyFactory.getInstance("RSA");
return kf.generatePublic(kspec);
} catch (GeneralSecurityException ex) {
throw new PrivacyCAException ("Unable to convert an OAEP RSA
Public Key to a normal RSA public key", ex);
}
}
The fixed public key is fed to this code code which worked even in JDK8:
Cipher rsaoaep = Cipher.getInstance
("RSA/ECB/oaepwithsha1andmgf1padding", "SunJCE");
String hashName = "SHA-256"; // name alg hash for EK?
String hmacName = "HmacSHA256";
int hashLen = 256;
byte[] identityLabel =
Charset.forName("UTF-8").encode("IDENTITY").array();
// zero terminate
identityLabel = Arrays.copyOf(identityLabel, identityLabel.length
+ 1);
logBuffer("Identity label", identityLabel);
OAEPParameterSpec oaepspec =
new OAEPParameterSpec (hashName, "MGF1", new MGF1ParameterSpec
(hashName),
new PSource.PSpecified(identityLabel));
rsaoaep.init (Cipher.ENCRYPT_MODE, ekPubKey, oaepspec);
byte[] encryptedSeed = rsaoaep.doFinal(seed);
If I get a chance, I'll try and take a properly formatted RSA (.1) key
and turn it into an RSAOAEP (.7) key and see what happens with the
above code. It may take a few days.
AFAIK, this convention was only used by the TPM1.2 space, and only
briefly as it was realized it was a backwards compatibility nightmare.
It was actually in an early version of the TPM spec and then removed. I
haven't seen any of these keys in any of the TPM2.0s I've played with.
Strangely, I haven't seen any PSS (.10) (vs OAEP) encoded keys.
Mike
Re: RFR: 8255739: x509Certificate returns � for invalid subjectAlternativeNames [v2]
On Fri, 14 Jan 2022 11:18:23 GMT, Masanori Yano 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 Hi Mike, we don't support an OAEP key (i.e. RSA keys with OAEP parameters). If you want OAEP encryption, just use a plain RSA key and pass an OAEPParameterSpec to the RSA Cipher's init method. - PR: https://git.openjdk.java.net/jdk/pull/6928
Re: RFR: 8255739: x509Certificate returns � for invalid subjectAlternativeNames
On Sat, 22 Jan 2022 22:48:29 GMT, Michael StJohns wrote: > 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.?? Is that still an issue? I would have expected this to be fixed since JDK 11 when we added support for RSASSA-PSS. - PR: https://git.openjdk.java.net/jdk/pull/6928
Re: RFR: 8255739: x509Certificate returns � for invalid subjectAlternativeNames [v2]
Hi Sean - Inline. On 1/21/2022 11:33 AM, Sean Mullan wrote: On Fri, 14 Jan 2022 11:18:23 GMT, Masanori Yano 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 = : 04 1A 30 18 30 16 06 05 67 81 05 02 10 31 0D 30 ..0.0...g1.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 [ : 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: : 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: : 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:0002, OID.2.23.133.2.2=SPT, OID.2.23.133.2.1=id:494E54 43 ] ]
Re: RFR: 8255739: x509Certificate returns � for invalid subjectAlternativeNames [v2]
On Fri, 14 Jan 2022 11:18:23 GMT, Masanori Yano 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? > 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 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). See also https://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
Re: RFR: 8255739: x509Certificate returns � for invalid subjectAlternativeNames
On 1/18/2022 4:10 PM, Sean Mullan wrote: On Thu, 6 Jan 2022 20:28:22 GMT, Sean Mullan 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
Re: RFR: 8255739: x509Certificate returns � for invalid subjectAlternativeNames
On Thu, 6 Jan 2022 20:28:22 GMT, Sean Mullan 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
Re: RFR: 8255739: x509Certificate returns � for invalid subjectAlternativeNames
On Thu, 6 Jan 2022 20:28:22 GMT, Sean Mullan 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
Re: RFR: 8255739: x509Certificate returns � for invalid subjectAlternativeNames [v2]
On Fri, 14 Jan 2022 11:18:23 GMT, Masanori Yano 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 Thank you for your comments. @seanjmullan I agree that the fix has a compatibility risk. I made the fix again to check only DNSName to reduce the risk. Could you please review the fix? Is it necessary to issue CSR for the fix? @wangweij I think the behavior of openssl is incorrect. According to rfc5280, a certificate-using system MUST reject the certificate if it encounters a critical extension it does not recognize. https://datatracker.ietf.org/doc/html/rfc5280#section-4.2 Each extension in a certificate is designated as either critical or non-critical. A certificate-using system MUST reject the certificate if it encounters a critical extension it does not recognize or a critical extension that contains information that it cannot process. - PR: https://git.openjdk.java.net/jdk/pull/6928
Re: RFR: 8255739: x509Certificate returns � for invalid subjectAlternativeNames [v2]
> 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/6928/files - new: https://git.openjdk.java.net/jdk/pull/6928/files/b11495d9..a777ded0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6928=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6928=00-01 Stats: 103 lines in 3 files changed: 44 ins; 46 del; 13 mod Patch: https://git.openjdk.java.net/jdk/pull/6928.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6928/head:pull/6928 PR: https://git.openjdk.java.net/jdk/pull/6928
Re: RFR: 8255739: x509Certificate returns � for invalid subjectAlternativeNames
On Thu, 23 Dec 2021 11:59:18 GMT, Masanori Yano 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. The method modified has effect on all the string reading methods in `DerValue`. I wonder if this is worth doing. How harmful is this bug? BTW, openssl seems to be reading them as UTF-8 strings: X509v3 Subject Alternative Name: DNS:℡.com, DNS:K.com - PR: https://git.openjdk.java.net/jdk/pull/6928
Re: RFR: 8255739: x509Certificate returns � for invalid subjectAlternativeNames
On Thu, 23 Dec 2021 11:59:18 GMT, Masanori Yano 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. - PR: https://git.openjdk.java.net/jdk/pull/6928
Re: RFR: 8255739: x509Certificate returns � for invalid subjectAlternativeNames
On Thu, 23 Dec 2021 11:59:18 GMT, Masanori Yano 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. Could someone please review this pull request? - PR: https://git.openjdk.java.net/jdk/pull/6928
RFR: 8255739: x509Certificate returns � for invalid subjectAlternativeNames
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. - Commit messages: - 8255739: x509Certificate returns � for invalid subjectAlternativeNames Changes: https://git.openjdk.java.net/jdk/pull/6928/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6928=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255739 Stats: 159 lines in 2 files changed: 158 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6928.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6928/head:pull/6928 PR: https://git.openjdk.java.net/jdk/pull/6928
