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.
