-----Ursprüngliche Nachricht----- > Von:Adam Petcher <adam.petc...@oracle.com> > Gesendet: Fre 15 Dezember 2017 20:57 > An: security-dev@openjdk.java.net > 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
