[PATCH]: Support for brainpool curves from CurveDB in SunEC

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH]: Support for brainpool curves from CurveDB in SunEC

Tobias Wagner
Hi,

in our current project, we have the requirement to support brainpool curves for TLS connections (RFC 7027).

As part of this requirement, we introduced the brainpoolP*r1 curves to SunEC, as they are already known in sun.security.util.CurveDB. It does not introduce the twisted curves from RFC 5639. We want to share this patch, hoping it might be useful for others. Especially for public funded projects (e.g. health care or eID) in Europe, the use or at least support for these curves is often mandatory.

The attached patch adds the domain parameters for

* brainpoolP160r1 (1.3.36.3.3.2.8.1.1.1)
* brainpoolP192r1 (1.3.36.3.3.2.8.1.1.3)
* brainpoolP224r1 (1.3.36.3.3.2.8.1.1.5)
* brainpoolP256r1 (1.3.36.3.3.2.8.1.1.7)
* brainpoolP320r1 (1.3.36.3.3.2.8.1.1.9)
* brainpoolP384r1 (1.3.36.3.3.2.8.1.1.11)
* brainpoolP512r1 (1.3.36.3.3.2.8.1.1.13)

and makes them available in the same manner the other curves are available. It does not introduce new ECC algorithmics. Our understanding of legal issues around ECC is, that they are related to deployed algorithmics not on certain domain parameters.

Even though IANA has only assigned numbers for the 256, 384 and 512 bit r-curves, we still need to add all r-curves to prevent errors in the native part of SunEC, when requesting calculations on one of the other curves.

Relation to other bugs:
https://bugs.openjdk.java.net/browse/JDK-7007966 - Our patch might be a partial solution for that bug. However, it asks for support for all brainpool curves, this patch leaves out the twisted curves. Furthermore the patch presented there seems to be on a quite different code base.

https://bugs.openjdk.java.net/browse/JDK-8189594 - The error in the optimized ECC field arithmetic will definitively interfere with brainpoolP320r1 (5 word optimized methods) and brainpoolP384r1 (6 word optimized methods). I already provided a patch for that issue: http://mail.openjdk.java.net/pipermail/security-dev/2017-October/016407.html

Tests:
There are already tests in TestEC: This patch will switch the brainpool curves from unsupported to supported and the subsequent tests are as well executed with the brainpool curves. Without JDK-8189594, these tests eventually fail, when it comes to brainpoolP320r1. With the patch from JDK-8189594, all tests are executed and pass.

Tested platforms:

* Windows
* Linux
* macOS X

Unfortunately no Solaris, as we do not have such a machine at our disposal. It would be great, if someone could sponsor this patch and help with that.

Regards
Tobias

P.S.

We have another patch, which adds the three brainpool curves with IANA numbers to the sun.security.ssl.EllipticCurveExtension. This enables these curves in SunJSSE for TLS connections. We did not include that part here, as we did not want to mix two parts of the JDK in one patch. It also needs a more handy test than a shellscript using OpenSSL.

--
phone: +49 221 222896 17
fax: +49 221 222896 11

n - d e s i g n   G m b H
www.n-design.de
Alpenerstr. 16
D-50825 Köln

Amtsgericht Köln HRB 33766 B
Geschäftsführer Andy Kohl

openjdk_jdk9_jdk_17288.patch (17K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH]: Support for brainpool curves from CurveDB in SunEC

Adam Petcher
On 12/15/2017 11:31 AM, Tobias Wagner wrote:

> Hi,
>
> in our current project, we have the requirement to support brainpool curves for TLS connections (RFC 7027).
>
> As part of this requirement, we introduced the brainpoolP*r1 curves to SunEC, as they are already known in sun.security.util.CurveDB. It does not introduce the twisted curves from RFC 5639. We want to share this patch, hoping it might be useful for others. Especially for public funded projects (e.g. health care or eID) in Europe, the use or at least support for these curves is often mandatory.

Great! You are going to make it a happy Christmas for a lot of people. I
would be happy to sponsor this change, and I have a few initial
requests/comments/questions before I look at the patch in more detail:

1) Please include a test which checks the new curves against some test
vectors. Ideally, these vectors come from a standard (e.g. RFC 7027 has
some).
2) The existing tests will check for some form of correctness for all
enabled curves, but this doesn't ensure that the Brainpool curves are
enabled. So you should also add a test that ensures that the new curves
are enabled. This can be combined with the previously mentioned test
against the test vectors.
3) We can't push this change to the repo without also fixing
JDK-8189594. So I think you have a couple of options. Either submit a
separate patch for JDK-8189594 first (which may be tricky because you
will need to find a way to write a test for it) or include the fix for
JDK-8189594 in this patch.
4) How important are the 224-bit and smaller curves? We can't use them
in TLS, and I don't think we should add curves that are already obsolete
unless there is a good reason.

Reply | Threaded
Open this post in threaded view
|

[PATCH]: Support for brainpool curves from CurveDB in SunEC

Tobias Wagner
-----Ursprüngliche Nachricht-----

> Von:Adam Petcher <[hidden email]>
> Gesendet: Fre 15 Dezember 2017 20:57
> An: [hidden email]
> Betreff: Re: [PATCH]: Support for brainpool curves from CurveDB in SunEC
>
> On 12/15/2017 11:31 AM, Tobias Wagner wrote:
>
> > Hi,
> >
> > in our current project, we have the requirement to support brainpool curves for TLS connections (RFC 7027).
> >
> > As part of this requirement, we introduced the brainpoolP*r1 curves to SunEC, as they are already known in sun.security.util.CurveDB. It does not introduce the twisted curves from RFC 5639. We want to share this patch, hoping it might be useful for others. Especially for public funded projects (e.g. health care or eID) in Europe, the use or at least support for these curves is often mandatory.
>
> Great! You are going to make it a happy Christmas for a lot of people. I
> would be happy to sponsor this change, and I have a few initial
> requests/comments/questions before I look at the patch in more detail:

Thank you!

> 1) Please include a test which checks the new curves against some test
> vectors. Ideally, these vectors come from a standard (e.g. RFC 7027 has
> some).
> 2) The existing tests will check for some form of correctness for all
> enabled curves, but this doesn't ensure that the Brainpool curves are
> enabled. So you should also add a test that ensures that the new curves
> are enabled. This can be combined with the previously mentioned test
> against the test vectors.

All right, I will do that, but I can't promise it to happen this year - well because
of other obligations and christmas of course.

> 3) We can't push this change to the repo without also fixing
> JDK-8189594. So I think you have a couple of options. Either submit a
> separate patch for JDK-8189594 first (which may be tricky because you
> will need to find a way to write a test for it) or include the fix for
> JDK-8189594 in this patch.

OK, then I will include that fix into this patch.

> 4) How important are the 224-bit and smaller curves? We can't use them
> in TLS, and I don't think we should add curves that are already obsolete
> unless there is a good reason.

Curves with less than 256 bits are not of special importance for us. In fact, our
first approach did not include these curves, but this had some odd side effects,
which came out when running the jtreg tests:

These curves are already present in CurveDB, thus one can generally use their
ASN.1 oids to request calculations on them. In SunEC there is a check on length
of the oid's DER encoding, to decide wether the curve is supported or not:

[ec.h, ecdecode.c: EC_FillParams()]
10: ANSI X9.62 curves
7: SECG curves
(11: Brainpool curves)

Using that mechanism leads to errors in the native part, as it tries to access the non-
present domain parameters, if someone requests e.g. a ECDSA signature with brainpool160r1.
For that reason I added the remaining parameters.

One reason to add these curves, is to be able to support the verification of legacy signatures.

If they should not be added, I see two options:

* remove them from CurveDB as well, so they can't be addressed anymore.

or

* There should be a more explicit check than the length of the oid's DER encoding to decide wether the curve is supported or not.

I am not sure, what option should be taken in that case. As far as I understand, the first one might affect other providers, which could not
support these curves anymore as well.

Regards
Tobias




Reply | Threaded
Open this post in threaded view
|

Re: [PATCH]: Support for brainpool curves from CurveDB in SunEC

Adam Petcher
On 12/16/2017 2:52 PM, Tobias Wagner wrote:

> -----Ursprüngliche Nachricht-----
>> Von:Adam Petcher <[hidden email]>
>> Gesendet: Fre 15 Dezember 2017 20:57
>> An: [hidden email]
>> Betreff: Re: [PATCH]: Support for brainpool curves from CurveDB in SunEC
>> 4) How important are the 224-bit and smaller curves? We can't use them
>> in TLS, and I don't think we should add curves that are already obsolete
>> unless there is a good reason.
> Curves with less than 256 bits are not of special importance for us. In fact, our
> first approach did not include these curves, but this had some odd side effects,
> which came out when running the jtreg tests:
>
> These curves are already present in CurveDB, thus one can generally use their
> ASN.1 oids to request calculations on them. In SunEC there is a check on length
> of the oid's DER encoding, to decide wether the curve is supported or not:
>
> [ec.h, ecdecode.c: EC_FillParams()]
> 10: ANSI X9.62 curves
> 7: SECG curves
> (11: Brainpool curves)
>
> Using that mechanism leads to errors in the native part, as it tries to access the non-
> present domain parameters, if someone requests e.g. a ECDSA signature with brainpool160r1.
> For that reason I added the remaining parameters.
>
> One reason to add these curves, is to be able to support the verification of legacy signatures.
>
> If they should not be added, I see two options:
>
> * remove them from CurveDB as well, so they can't be addressed anymore.
>
> or
>
> * There should be a more explicit check than the length of the oid's DER encoding to decide wether the curve is supported or not.
>
> I am not sure, what option should be taken in that case. As far as I understand, the first one might affect other providers, which could not
> support these curves anymore as well.

Right. Removing them from CurveDB isn't a good option because of the
compatibility impact. Also note that CurveDB is allowed to vary
independently of the native implementation. So we always have to handle
the case that curves exist in CurveDB, but are not supported in the
native code. For example, someone could add the twisted Brainpool curves
to CurveDB in the future.

I think the explicit check that you need is already there in the code
you added to SECOID_FindOID. You just need to replace the elements in
SECOidData BRAINPOOL_oids that you don't want to support with the
"Unknown OID" block element that you are using for the twisted curves.
Of course, there are probably other ways to do this check, but this
seems to be the pattern that exists in the code already.

I'm not completely opposed to adding these small curves, but I don't
think we should do it unless there is a compelling reason. So if anyone
in the community has a need for these curves (or other thoughts on this
issue), please let us know.


>
> Regards
> Tobias
>
>
>
>