On 12/16/2017 2:52 PM, Tobias Wagner wrote:
-----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
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
