Hi and a happy new year,
I did some further work reagarding the brainpool curves.
For the points about the removal of the small curves and the challenges with
that, please see below.
Regarding the test vectors for the brainpool curves, I'm planning to add a new
jtreg test to
sun.security.ec which is simliar to the sun.security.pkcs11.ec.TestECDH test,
but uses the testdata
from RFC 7027. Adding these data to the latter test seems not to be right, as
it is designed to
work with arbitrary providers and would possibly fail with them.
A further point about the jtreg tests is, that I have trouble executing
sun.security.ec.TestEC.
Regardless of whether I run it with a patched or unpatched JVM. It has two @run
tags:
* @run main/othervm -Djdk.tls.namedGroups="secp256r1,sect193r1" TestEC
* @run main/othervm/java.security.policy=TestEC.policy
-Djdk.tls.namedGroups="secp256r1,sect193r1" TestEC
While the first run finishes with all tests passed, the second one fails
immediately as no "SunEC" provider is available.
The second tag looks somewhat malformed to me. It looks a little bit that way,
as it is meant to run the tests under an
explicit security policy. If so, I would expect it to look like that:
* @run main/othervm -Djava.security.policy=TestEC.policy
-Djdk.tls.namedGroups="secp256r1,sect193r1" TestEC
Changing it that way, all tests are executed and pass in the second run.
Regards
Tobias
> >> 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
> >
> > 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.
>
I found out that the "Unknow OID" blocks are the troublemakers in that case:
This structure
contains a SECItem with a field "unsigned char *data" set to NULL. In the last
step of
determining whether the requested OID matches the found structure, memcmp from
string.h is used to check
equality. Unfortunately, memcmp is not meant to be used with null-pointers and
therefore the JVM dies in
an infamous Segmentation Fault.
To cope with that, I introduced a "oideql" function:
BOOL oideql(unsigned char *reqoid, unsigned char *foundoid, size_t len) {
if(!reqoid || !foundoid) {
return FALSE;
}
return memcmp(reqoid, foundoid, len) == 0; }
In my patch, this function is used in SECOID_FindOID instead of using the
direct call of memcmp.
The patch it self is not yet attached, as it is a bit clumsy from trying
different ways and figuring out solutions
- and of course the testvectors are still missing.
