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