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

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

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

Tobias Wagner
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 <[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
> >
> > 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.
Reply | Threaded
Open this post in threaded view
|

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

Adam Petcher
On 1/4/2018 7:42 AM, Tobias Wagner wrote:

> 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.

Making a separate test for brainpool vectors is probably good idea, but
I suggest that this new test should loop through all the providers and
simply skip the test when the curve is not supported and the provider is
not "SunEC". You could also modify the existing TestECDH to skip tests
in this way, but making a separate test is probably simple and cleaner.

>
> 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.

This has to do with the way jtreg handles paths, and the solution is to
run jtreg on an images build. To create an images build, simply "make
images". Then, when you run jtreg, you do "jtreg
-jdk:build/<platform>/images/jdk <test>".

> 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.

Now I'm wondering why the same problem hasn't come up in the existing
curves. It's likely that the execution doesn't get this far (e.g. the
curves that are not in the array in oid.c are also not in CurveDB).
Regardless, making this function more robust is a good idea, so you
should consider replacing the other memcmp calls in this function with
the oideql function. While you are at it, you can solve the other
problem with using memcmp here, which is that it can read out of bounds.
To fix this problem, you can (for example) have oideql take both lengths
and return false when they are not equal.

> 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.