[security-dev 01197]: Re: 6840752: Provide out-of-the-box support for ECC algorithms

Andrew John Hughes gnu_andrew at member.fsf.org
Wed Sep 9 23:14:31 UTC 2009


2009/9/9 Vincent Ryan <Vincent.Ryan at sun.com>:
> Hello Andrew,
>
> I realize that you, along with others in the Linux community, are less
> than satisfied with the changeset to provide out-of-the-box support for
> ECC algorithms.
>
> As I mentioned earlier, we were quite constrained in what we could
> openly discuss before we pushed. However, now that we have pushed I
> am eager to fix any problems that I've introduced.
>

Yes, I can understand that to an extent, but I find it hard to believe
that you had to push it before it could even be discussed.  Why could
the same patch that was pushed not have been posted for public review
instead?

This seems to be a more general issue.  This is endemic behaviour that
I've seen from the majority of Sun engineers working on OpenJDK (there
are thankfully some exceptions) and I've blogged about this in more
detail: http://blog.fuseyism.com/index.php/2009/09/08/im-so-tired/

> We wish to reconcile the conflicting demands of including an ECC
> implementation for platforms without underlying ECC support with the
> exclusion of the ECC implementation on platforms with underlying ECC
> support. I'd like to solicit input from security-dev on how best to
> achieve this.
>

It's good to hear you're open to changing this.  There is a third
option you've missed; the demand of not wanting ECC support at all.
You'll be aware that there are legal issues from your own discussions
on this within Sun, and the change in direction that occurred.  Not
having ECC support needs to be an option as well.

The existing ECC implementation already fulfilled two of these
demands; it could be enabled on platforms with ECC support but this
wasn't the default case.  We can make this easier with IcedTea by
detecting NSS at build time and auto-generating the configuration if
the user wishes.  This also can be used to ship it 'out of the box' on
distributions if required; all the distro packager has to do is build
IcedTea with NSS support enabled and then make their binary depend on
it.

So the real problem here is that Sun's proprietary builds can't ship
it 'out of the box' because they don't know if the system it ends up
on will have NSS and, even if it does, where it will be located.  I
can understand how that's a problem that needs to be fixed, but we
need a way of disabling that.  If the PKCS11 provider is still
suitable, then making building the ec directory would actually be
enough:

ifndef DISABLE_NSS
  SUBDIRS += ec
endif

Job done.  A more complex solution is to link against the system NSS
instead of the provided C sources.  I've managed to do this with the
following change:

diff -r 7a23bfc44c92 make/sun/security/ec/Makefile
--- a/make/sun/security/ec/Makefile	Tue Sep 08 18:03:43 2009 +0100
+++ b/make/sun/security/ec/Makefile	Wed Sep 09 23:50:24 2009 +0100
@@ -153,7 +153,9 @@
   #
   # C and C++ files
   #
+ifndef USE_SYSTEM_NSS
   include FILES_c.gmk
+endif

   FILES_cpp = ECC_JNI.cpp

@@ -185,6 +187,11 @@
     OTHER_LDLIBS += $(JVMLIB)
   else
     OTHER_LDLIBS = -ldl $(JVMLIB) $(LIBCXX)
+    ifdef USE_SYSTEM_NSS
+      OTHER_LDLIBS += -Wl,-rpath $(SYSTEM_NSS_DIR) -Wl,-rpath
$(SYSTEM_NSPR_DIR) \
+	 -L$(SYSTEM_NSS_DIR) -L$(SYSTEM_NSPR_DIR) -lnssutil3 -lnss3 \
+	 -lplds4 -lplc4 -lnspr4 -lsoftokn -lfreebl
+    endif
   endif

   include $(BUILDDIR)/common/Mapfile-vers.gmk

but unfortunately, while the resulting sunecc library is dynamically
linked against NSS, it causes HotSpot to segfault in
sun.security.ec.ECKeyPairGenerator.generateECKeyPair(I[B[B)[J.  I'm
still looking into this, I assume there is either some mismatch in the
versions of NSS or local changes in the Sun copy.  As you say, only
part of the library was imported into OpenJDK; does this mean that the
JNI code is not using published interfaces for NSS?

> Your proposal to supply an NSS config file for the SunPKCS11 provider
> is one approach but what about platforms where an ECC-enabled NSS is
> not present?
>
>

It's only really an idea that works where we have an autoconf wrapper
to detect NSS at build time, and which also allows it to be disabled.
The patch to IcedTea automatically finds out where NSS is installed,
via pkg-config, and writes the config file based on that.  I don't
know of a portable way of doing that in OpenJDK's makefiles as
pkg-config won't be available on all platforms.

snip...

>
>>  * Which version of NSS were these sources pulled from?  Running diff
>> -bu on them, and ignoring the additional copyright headers,
>>  there are still a large number of changes.  I suspect this is
>> because the version is older than my system copy (3.12.3); notably my
>>  testing shows it does not exhibit the bug discussed in
>>
>> http://mail.openjdk.java.net/pipermail/security-dev/2009-September/001167.html
>> (which
>>  incidentally is still awaiting review).
>
> The sources were pulled from OpenSolaris 2009.06.
>

Ok, so which version of NSS does that have?

>
>>  * Why was a new provider used instead of the existing
>> sun.security.pkcs11.SunPKCS11 provider?  I noticed this has not be
>> removed, yet
>>  it appears to provide duplicate functionality unless I'm mistaken.
>> This does perhaps mean we could fix the issues with this changeset
>> simply
>>  by allowing the ec subdirectory to be turned off, but there may be
>> something about the new provider I'm missing.
>
> We introduced the new SunEC provider because we wanted a fast compact
> ECC implementation that we could ship on all platforms. We have not
> bundled all of NSS - only its ECC implementation.
>

Yeah I noticed that.  I suppose the big question is how interchangable
are SunEC and PKCS11?  Could we just turn off SunEC, given we already
have NSS support via PKCS11?  If so, just making SunEC optional would
solve this IMO.

>
>
>>  * I notice that a number of algorithms are replaced with NULL.  I
>> assume there is some (perhaps legal) reason for this?
>
> Which ones?
>

This is the change I'm referring to:

/* mapping between ECCurveName enum and pointers to ECCurveParams */
 static const ECCurveParams *ecCurve_map[] = {
     NULL,                               /* ECCurve_noName */
-    &ecCurve_NIST_P192,                 /* ECCurve_NIST_P192 */
-    &ecCurve_NIST_P224,                 /* ECCurve_NIST_P224 */
+	NULL,			/* ECCurve_NIST_P192 */
+	NULL,			/* ECCurve_NIST_P224 */
     &ecCurve_NIST_P256,                 /* ECCurve_NIST_P256 */
     &ecCurve_NIST_P384,                 /* ECCurve_NIST_P384 */
     &ecCurve_NIST_P521,                 /* ECCurve_NIST_P521 */
-    &ecCurve_NIST_K163,                 /* ECCurve_NIST_K163 */
-    &ecCurve_NIST_B163,                 /* ECCurve_NIST_B163 */
-    &ecCurve_NIST_K233,                 /* ECCurve_NIST_K233 */
-    &ecCurve_NIST_B233,                 /* ECCurve_NIST_B233 */
-    &ecCurve_NIST_K283,                 /* ECCurve_NIST_K283 */
-    &ecCurve_NIST_B283,                 /* ECCurve_NIST_B283 */
-    &ecCurve_NIST_K409,                 /* ECCurve_NIST_K409 */
-    &ecCurve_NIST_B409,                 /* ECCurve_NIST_B409 */
-    &ecCurve_NIST_K571,                 /* ECCurve_NIST_K571 */
-    &ecCurve_NIST_B571,                 /* ECCurve_NIST_B571 */
-    &ecCurve_X9_62_PRIME_192V2,         /* ECCurve_X9_62_PRIME_192V2 */
-    &ecCurve_X9_62_PRIME_192V3,         /* ECCurve_X9_62_PRIME_192V3 */
-    &ecCurve_X9_62_PRIME_239V1,         /* ECCurve_X9_62_PRIME_239V1 */
-    &ecCurve_X9_62_PRIME_239V2,         /* ECCurve_X9_62_PRIME_239V2 */
-    &ecCurve_X9_62_PRIME_239V3,         /* ECCurve_X9_62_PRIME_239V3 */
-    &ecCurve_X9_62_CHAR2_PNB163V1,      /* ECCurve_X9_62_CHAR2_PNB163V1 */
-    &ecCurve_X9_62_CHAR2_PNB163V2,      /* ECCurve_X9_62_CHAR2_PNB163V2 */
-    &ecCurve_X9_62_CHAR2_PNB163V3,      /* ECCurve_X9_62_CHAR2_PNB163V3 */
-    &ecCurve_X9_62_CHAR2_PNB176V1,      /* ECCurve_X9_62_CHAR2_PNB176V1 */
-    &ecCurve_X9_62_CHAR2_TNB191V1,      /* ECCurve_X9_62_CHAR2_TNB191V1 */
-    &ecCurve_X9_62_CHAR2_TNB191V2,      /* ECCurve_X9_62_CHAR2_TNB191V2 */
-    &ecCurve_X9_62_CHAR2_TNB191V3,      /* ECCurve_X9_62_CHAR2_TNB191V3 */
-    &ecCurve_X9_62_CHAR2_PNB208W1,      /* ECCurve_X9_62_CHAR2_PNB208W1 */
-    &ecCurve_X9_62_CHAR2_TNB239V1,      /* ECCurve_X9_62_CHAR2_TNB239V1 */
-    &ecCurve_X9_62_CHAR2_TNB239V2,      /* ECCurve_X9_62_CHAR2_TNB239V2 */
-    &ecCurve_X9_62_CHAR2_TNB239V3,      /* ECCurve_X9_62_CHAR2_TNB239V3 */
-    &ecCurve_X9_62_CHAR2_PNB272W1,      /* ECCurve_X9_62_CHAR2_PNB272W1 */
-    &ecCurve_X9_62_CHAR2_PNB304W1,      /* ECCurve_X9_62_CHAR2_PNB304W1 */
-    &ecCurve_X9_62_CHAR2_TNB359V1,      /* ECCurve_X9_62_CHAR2_TNB359V1 */
-    &ecCurve_X9_62_CHAR2_PNB368W1,      /* ECCurve_X9_62_CHAR2_PNB368W1 */
-    &ecCurve_X9_62_CHAR2_TNB431R1,      /* ECCurve_X9_62_CHAR2_TNB431R1 */
-    &ecCurve_SECG_PRIME_112R1,          /* ECCurve_SECG_PRIME_112R1 */
-    &ecCurve_SECG_PRIME_112R2,          /* ECCurve_SECG_PRIME_112R2 */
-    &ecCurve_SECG_PRIME_128R1,          /* ECCurve_SECG_PRIME_128R1 */
-    &ecCurve_SECG_PRIME_128R2,          /* ECCurve_SECG_PRIME_128R2 */
-    &ecCurve_SECG_PRIME_160K1,          /* ECCurve_SECG_PRIME_160K1 */
-    &ecCurve_SECG_PRIME_160R1,          /* ECCurve_SECG_PRIME_160R1 */
-    &ecCurve_SECG_PRIME_160R2,          /* ECCurve_SECG_PRIME_160R2 */
-    &ecCurve_SECG_PRIME_192K1,          /* ECCurve_SECG_PRIME_192K1 */
-    &ecCurve_SECG_PRIME_224K1,          /* ECCurve_SECG_PRIME_224K1 */
-    &ecCurve_SECG_PRIME_256K1,          /* ECCurve_SECG_PRIME_256K1 */
-    &ecCurve_SECG_CHAR2_113R1,          /* ECCurve_SECG_CHAR2_113R1 */
-    &ecCurve_SECG_CHAR2_113R2,          /* ECCurve_SECG_CHAR2_113R2 */
-    &ecCurve_SECG_CHAR2_131R1,          /* ECCurve_SECG_CHAR2_131R1 */
-    &ecCurve_SECG_CHAR2_131R2,          /* ECCurve_SECG_CHAR2_131R2 */
-    &ecCurve_SECG_CHAR2_163R1,          /* ECCurve_SECG_CHAR2_163R1 */
-    &ecCurve_SECG_CHAR2_193R1,          /* ECCurve_SECG_CHAR2_193R1 */
-    &ecCurve_SECG_CHAR2_193R2,          /* ECCurve_SECG_CHAR2_193R2 */
-    &ecCurve_SECG_CHAR2_239K1,          /* ECCurve_SECG_CHAR2_239K1 */
-    &ecCurve_WTLS_1,                    /* ECCurve_WTLS_1 */
-    &ecCurve_WTLS_8,                    /* ECCurve_WTLS_8 */
-    &ecCurve_WTLS_9,                    /* ECCurve_WTLS_9 */
+	NULL,			/* ECCurve_NIST_K163 */
+	NULL,			/* ECCurve_NIST_B163 */
+	NULL,			/* ECCurve_NIST_K233 */
+	NULL,			/* ECCurve_NIST_B233 */
+	NULL,			/* ECCurve_NIST_K283 */
+	NULL,			/* ECCurve_NIST_B283 */
+	NULL,			/* ECCurve_NIST_K409 */
+	NULL,			/* ECCurve_NIST_B409 */
+	NULL,			/* ECCurve_NIST_K571 */
+	NULL,			/* ECCurve_NIST_B571 */
+	NULL,			/* ECCurve_X9_62_PRIME_192V2 */
+	NULL,			/* ECCurve_X9_62_PRIME_192V3 */
+	NULL,			/* ECCurve_X9_62_PRIME_239V1 */
+	NULL,			/* ECCurve_X9_62_PRIME_239V2 */
+	NULL,			/* ECCurve_X9_62_PRIME_239V3 */
+	NULL,			/* ECCurve_X9_62_CHAR2_PNB163V1 */
+	NULL,			/* ECCurve_X9_62_CHAR2_PNB163V2 */
+	NULL,			/* ECCurve_X9_62_CHAR2_PNB163V3 */
+	NULL,			/* ECCurve_X9_62_CHAR2_PNB176V1 */
+	NULL,			/* ECCurve_X9_62_CHAR2_TNB191V1 */
+	NULL,			/* ECCurve_X9_62_CHAR2_TNB191V2 */
+	NULL,			/* ECCurve_X9_62_CHAR2_TNB191V3 */
+	NULL,			/* ECCurve_X9_62_CHAR2_PNB208W1 */
+	NULL,			/* ECCurve_X9_62_CHAR2_TNB239V1 */
+	NULL,			/* ECCurve_X9_62_CHAR2_TNB239V2 */
+	NULL,			/* ECCurve_X9_62_CHAR2_TNB239V3 */
+	NULL,			/* ECCurve_X9_62_CHAR2_PNB272W1 */
+	NULL,			/* ECCurve_X9_62_CHAR2_PNB304W1 */
+	NULL,			/* ECCurve_X9_62_CHAR2_TNB359V1 */
+	NULL,			/* ECCurve_X9_62_CHAR2_PNB368W1 */
+	NULL,			/* ECCurve_X9_62_CHAR2_TNB431R1 */
+	NULL,			/* ECCurve_SECG_PRIME_112R1 */
+	NULL,			/* ECCurve_SECG_PRIME_112R2 */
+	NULL,			/* ECCurve_SECG_PRIME_128R1 */
+	NULL,			/* ECCurve_SECG_PRIME_128R2 */
+	NULL,			/* ECCurve_SECG_PRIME_160K1 */
+	NULL,			/* ECCurve_SECG_PRIME_160R1 */
+	NULL,			/* ECCurve_SECG_PRIME_160R2 */
+	NULL,			/* ECCurve_SECG_PRIME_192K1 */
+	NULL,			/* ECCurve_SECG_PRIME_224K1 */
+	NULL,			/* ECCurve_SECG_PRIME_256K1 */
+	NULL,			/* ECCurve_SECG_CHAR2_113R1 */
+	NULL,			/* ECCurve_SECG_CHAR2_113R2 */
+	NULL,			/* ECCurve_SECG_CHAR2_131R1 */
+	NULL,			/* ECCurve_SECG_CHAR2_131R2 */
+	NULL,			/* ECCurve_SECG_CHAR2_163R1 */
+	NULL,			/* ECCurve_SECG_CHAR2_193R1 */
+	NULL,			/* ECCurve_SECG_CHAR2_193R2 */
+	NULL,			/* ECCurve_SECG_CHAR2_239K1 */
+	NULL,			/* ECCurve_WTLS_1 */
+	NULL,			/* ECCurve_WTLS_8 */
+	NULL,			/* ECCurve_WTLS_9 */
     NULL                                /* ECCurve_pastLastCurve */
 };


It could be a NSS version issue, but seems more deliberate to me.
It leaves three curves:
     &ecCurve_NIST_P256,                 /* ECCurve_NIST_P256 */
     &ecCurve_NIST_P384,                 /* ECCurve_NIST_P384 */
     &ecCurve_NIST_P521,                 /* ECCurve_NIST_P521 */

>
>>
>> I'm afraid my current impression of this changeset is that it doesn't
>> help us with packaging OpenJDK for GNU/Linux distributions at all, but
>> instead makes things ten times worse as there is now a stale NSS to
>> contend with.  Not only are there the issues with bit rot I alluded to
>> last time, but as you mention in your reply there are legal issues
>> with EC support.  Notably, I've found that Fedora doesn't ship any EC
>> support (https://bugzilla.redhat.com/show_bug.cgi?id=492124) so we'd
>> have to rip this out in packages for that distribution (and it's
>> dubious whether others should be shipping it).  IANAL, so I won't
>> comment further on such issues, but suffice to say this changeset
>> significantly reduces the options for handling NSS support downstream.
>>  In contrast, the existing support in 1.6 is almost ideal, once you've
>> discovered how it works; the distro packager can choose whether to
>> enable support or not and they don't have to worry about rotting
>> security code in OpenJDK.  Maybe I'm missing something but this makes
>> me think this would have been better as a local addition to Sun's
>> proprietary builds rather than adding it to OpenJDK.
>>
>> I try to be as positive as I can about the OpenJDK project, but I'm
>> sorry to say that changesets like this don't help.  I actually find
>> them quite depressing.  As I said in my previous email, there appears
>> to have been no discussion of this change; it was merely committed
>> with no public review and appeared in b70.  Meanwhile, myself and
>> other external contributors have to spend days trying to get replies
>> to emails to even get a simple bug fix in (I've lost count of how many
>> I still have waiting; there must be at least four or five).  That's
>> just not fair and doesn't bode well for a successful community
>> project.
>>
>> Thanks,
>
>

Cheers,
-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8



More information about the security-dev mailing list