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

Thu Nov 12 23:12:55 UTC 2015

Hi Vincent,

I just updated the changeset with a testcase: http://cr.openjdk.java.net/~clanger/webrevs/8139436.3/

The testcase would run through with or without my patch, unless you specify the IAIK provider as explained in the test description.

Can the fix be pushed now?

Thanks

Christoph

From: Vincent Ryan [mailto:vincent.x.ryan at oracle.com]
Sent: Mittwoch, 11. November 2015 13:18
To: Langer, Christoph <christoph.langer at sap.com>
Subject: Re: RFR 8139436: sun.security.mscapi.KeyStore might load incomplete data

Unfortunately I cannot push a changeset without a jtreg test.

Can you validate the fix another way - without using the IAIK provider?

   On 11 Nov 2015, at 12:13, Langer, Christoph <christoph.langer at sap.com<mailto:christoph.langer at sap.com>> wrote:

   Well, the test coding is available in the description of the Bug: https://bugs.openjdk.java.net/browse/JDK-8139436

   As reproduction of the issue would involve an older IAIK jce provider and a Windows certificate store containing EC certificates, I think it is not a feasible candidate for creating a jtreg test.

   Best regards

   Christoph

   From: Vincent Ryan [mailto:vincent.x.ryan at oracle.com]
   Sent: Mittwoch, 11. November 2015 13:10
   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>; Mike StJohns <mstjohns at comcast.net<mailto:mstjohns at comcast.net>>
   Subject: Re: RFR 8139436: sun.security.mscapi.KeyStore might load incomplete data

   I don’t see your testcase in the webrev can you add it in and re-generate.

   Thanks.

      On 11 Nov 2015, at 12:08, Langer, Christoph <christoph.langer at sap.com<mailto:christoph.langer at sap.com>> wrote:

      Hi Vincent,

      so this is the final version which should be pushed:

      http://cr.openjdk.java.net/~clanger/webrevs/8139436.2/

      I removed an unnecessary blank in a method call and updated the copyright information. I also ran another build and verified with my test case.

      Thanks in advance for taking care.

      Christoph

      From: Vincent Ryan [mailto:vincent.x.ryan at oracle.com]
      Sent: Mittwoch, 11. November 2015 08:46
      To: Mike StJohns <mstjohns at comcast.net<mailto:mstjohns at comcast.net>>; 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

      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 <mstjohns at comcast.net<mailto:mstjohns at 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' <mstjohns at comcast.net><mailto:mstjohns at comcast.net>
            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

            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/

            @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

                  Webrev: 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 (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/20151112/e8f70b97/attachment.htm>