RFR 8139436: sun.security.mscapi.KeyStore might load incomplete data

Tue Nov 10 22:39:48 UTC 2015

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' <mstjohns at comcast.net>
> *Cc:* security-dev at 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/~clanger/webrevs/8139436.1/ 
> <http://cr.openjdk.java.net/%7Eclanger/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:mstjohns at comcast.net]
> *Sent:* Donnerstag, 15. Oktober 2015 02:09
> *To:* Langer, Christoph <christoph.langer at sap.com 
> <mailto:christoph.langer at sap.com>>
> *Cc:* security-dev at openjdk.java.net <mailto:security-dev at 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 <christoph.langer at sap.com 
> <mailto:christoph.langer at 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-bounces at openjdk.java.net]
>     *On Behalf Of *Mike StJohns
>     *Sent:* Mittwoch, 14. Oktober 2015 02:17
>     *To:* security-dev at openjdk.java.net
>     <mailto:security-dev at 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>
>
>         Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8139436.0/
>         <http://cr.openjdk.java.net/%7Eclanger/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
>         (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
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20151110/af64fed9/attachment.htm>