Explicitly referencing the Sun JCE provider makes the jdk.crypto.mscapi module depend on the java.base module. And that’s OK.
I can sponsor this changeset. > On 10 Nov 2015, at 22:39, Mike StJohns <mstjo...@comcast.net> wrote: > > On 11/10/2015 4:12 PM, Langer, Christoph wrote: >> Hi folks, >> >> is there any feedback/review for this change? > > I missed the 4 Nov message when it came past. No further objections. But > still unclear if modularization will permit this. > > Mike > >> >> Thanks >> Christoph >> >> From: Langer, Christoph >> Sent: Mittwoch, 4. November 2015 12:11 >> To: 'Michael StJohns' <mstjo...@comcast.net> <mailto:mstjo...@comcast.net> >> Cc: security-dev@openjdk.java.net <mailto:security-dev@openjdk.java.net> >> Subject: RE: RFR 8139436: sun.security.mscapi.KeyStore might load incomplete >> data >> >> Hi Mike, >> >> eventually I’ve made a new webrev implementing your suggestion for a simpler >> patch: >> >> >> <http://cr.openjdk.java.net/%7Eclanger/webrevs/8139436.1/>http://cr.openjdk.java.net/~clanger/webrevs/8139436.1/ >> <http://cr.openjdk.java.net/~clanger/webrevs/8139436.1/> >> >> @All: Is that a valid fix for this issue? Would anyone mind to >> review/sponsor this? >> >> Thanks >> Christoph >> >> >> From: Michael StJohns [mailto:mstjo...@comcast.net >> <mailto:mstjo...@comcast.net>] >> Sent: Donnerstag, 15. Oktober 2015 02:09 >> To: Langer, Christoph < >> <mailto:christoph.lan...@sap.com>christoph.lan...@sap.com >> <mailto:christoph.lan...@sap.com>> >> Cc: security-dev@openjdk.java.net <mailto:security-dev@openjdk.java.net> >> Subject: Re: RFR 8139436: sun.security.mscapi.KeyStore might load incomplete >> data >> >> Sorry for the top post, I'm wring this on an iPad on a plane. >> >> >> It's perfectly acceptable for a provider to prefer its version of stuff over >> even something early in the provider list. Usually, that has more to do >> with key material ( for example pkcs11 instantiations), but there's no >> reason why it shouldn't apply to certificates. >> >> Mike >> >> >> >> Sent from my iPad >> >> On Oct 14, 2015, at 18:35, Langer, Christoph < >> <mailto:christoph.lan...@sap.com>christoph.lan...@sap.com >> <mailto:christoph.lan...@sap.com>> wrote: >> >> Hi Mike, >> >> thanks for your comments on this. >> >> I agree that the change is quite critical and my approach might not be good. >> However, as for your suggestion to specify the sun provider for the >> certificate factory, I’d say this is not quite right, too. Because, if you >> have loaded a different provider on top of the providers list, you’ll intend >> to use its facilities by default. Maybe the idea would be to use the sun >> provider as fallback in case of an exception? >> >> The other suggestion to check for certChain.length in >> KeyStore.getCertificate and return null would be an improvement compared to >> throwing an ArrayIndexOutOfBoundsException – however, it would still be >> difficult to find the root cause why a certificate could not be loaded. >> >> What do you think? >> >> @all: Are there other opinions? >> >> Thanks and best regards >> Christoph >> >> From: security-dev [ >> <mailto:security-dev-boun...@openjdk.java.net>mailto:security-dev-boun...@openjdk.java.net >> <mailto:security-dev-boun...@openjdk.java.net>] On Behalf Of Mike StJohns >> Sent: Mittwoch, 14. Oktober 2015 02:17 >> To: security-dev@openjdk.java.net <mailto:security-dev@openjdk.java.net> >> Subject: Re: RFR 8139436: sun.security.mscapi.KeyStore might load incomplete >> data >> >> Hi - >> >> I took a look and this probably isn't the correct way to fix this. >> >> A simpler change might be to specify the sun provider when requesting the >> certificate factory. I hesitate to say that definitively as >> modularization guidance may restrict that approach? >> >> The belt and suspenders approach is to catch the bad certificate exception >> and return null. That appears to be the correct contract for >> KeyStore.getCertificate(String alias). (e.g. "if (certChain.length == 0) >> return null;") >> >> Mike >> >> >> On 10/12/2015 5:04 PM, Langer, Christoph wrote: >> Hi, >> >> please review a change proposal regarding an issue in the Microsoft Security >> API (mscapi). >> >> Bug: >> <https://bugs.openjdk.java.net/browse/JDK-8139436>https://bugs.openjdk.java.net/browse/JDK-8139436 >> <https://bugs.openjdk.java.net/browse/JDK-8139436> >> Webrev: >> <http://cr.openjdk.java.net/%7Eclanger/webrevs/8139436.0/>http://cr.openjdk.java.net/~clanger/webrevs/8139436.0/ >> <http://cr.openjdk.java.net/~clanger/webrevs/8139436.0/> >> >> I stumbled over the issue when using an old IAIK security provider. It would >> throw java.security.cert.CertificateException upon parsing ECC based >> certificates when generating Certificate objects. The way it is right now, >> such exceptions are silently caught and the Windows Keystore is created with >> incomplete data. Upon accessing such ECC certificates from the Keystore >> object, e.g. when iterating over it, you’ll get exceptions like: >> Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException: 0 >> at >> sun.security.mscapi.KeyStore.engineGetCertificate(KeyStore.java:313) >> at >> sun.security.mscapi.KeyStore$ROOT.engineGetCertificate(KeyStore.java:60) >> at java.security.KeyStore.getCertificate(KeyStore.java:1095) >> At that point it is not obvious what the real root cause for that is. >> >> With my change, loading of the keystore would already throw like this: >> java.io.IOException: java.security.KeyStoreException: Exception occurred >> generating certificate object for alias DigiCert Assured ID Root G3 >> at sun.security.mscapi.KeyStore.engineLoad(KeyStore.java:780) >> at sun.security.mscapi.KeyStore$ROOT.engineLoad(KeyStore.java:60) >> at java.security.KeyStore.load(KeyStore.java:1459) >> at >> WindowsCertificateReaderTest.main(WindowsCertificateReaderTest.java:18) >> Caused by: java.security.KeyStoreException: Exception occurred generating >> certificate object for alias DigiCert Assured ID Root G3 >> at sun.security.mscapi.KeyStore.loadKeysOrCertificateChains(Native >> Method) >> at sun.security.mscapi.KeyStore.engineLoad(KeyStore.java:777) >> ... 3 more >> Caused by: java.security.cert.CertificateException: Error parsing >> certificates! iaik.asn1.DerInputException: Next ASN.1 object is no OBJECT >> IDENTIFIER! >> at iaik.x509.CertificateFactory.engineGenerateCertificates(Unknown >> Source) >> at >> java.security.cert.CertificateFactory.generateCertificates(CertificateFactory.java:462) >> at >> sun.security.mscapi.KeyStore.generateCertificate(KeyStore.java:869) >> ... 5 more >> This is more obvious when it comes to analyzing such an issue. >> >> Also, I added a property “sun.security.mscapi.ignoreFailingCertificates” >> which, when set to true, will cause skipping of certificates that failed >> with Exception. That might be a nice workaround option if one is not >> particularly interested in a failing certificate. >> >> You can reproduce all this with the test coding in the OpenJDK Bug, the IAIK >> provider 3.15 which is downloadable >> here:http://jcewww.iaik.tu-graz.ac.at/sic/Download >> <http://jcewww.iaik.tu-graz.ac.at/sic/Download> (educational/research >> version, needs registration) and ECC certificates in the Windows Root >> certificate store. >> >> Would you think this change is reasonable and worthwile? >> >> Thanks & Best regards >> Christoph >> > >
