Re: [PATCH]: Support for brainpool curves from CurveDB in SunEC
Just FYI. SoftHSM2 from the OpenDNSSec project is a good P11 to test with, and I believe it supports brainpool in recent versions. https://github.com/opendnssec/SoftHSMv2 It works really good) Regards, Tomas On 2018-02-09 02:03, Valerie Peng wrote: > Hi Tobias, > > Just curious, which PKCS11 library did you use to test your patch? After > I applied your patch and ran the regression tests, I noticed that both > the Solaris PKCS11 library and NSS skipped testing Brainpool curves with > different error codes which may be due to lack of support... > > Regards, > Valerie > > On 1/17/2018 3:02 PM, Tobias Wagner wrote: >> -Ursprüngliche Nachricht- >>> Von:Adam Petcher >>> Gesendet: Die 16 Januar 2018 18:38 >>> An: security-dev@openjdk.java.net >>> Betreff: Re: [PATCH]: Support for brainpool curves from CurveDB in SunEC >>> >>> Great! I took a look at the patch, and I have some comments, the first >>> of which probably needs to be addressed before I can test the change: >>> >>> 1) Is this patch against the http://hg.openjdk.java.net/jdk/jdk >>> repository? I suspect it isn't because some of the paths are different >>> than what I expect. We have made a lot of changes to the repositories in >>> the last few months. If this patch is against an older repo, please send >>> a patch against http://hg.openjdk.java.net/jdk/jdk . >> I switched to that repository now. As described on >> http://openjdk.java.net/contribute/, I was >> working with the http://hg.openjdk.java.net/jdk9/jdk9 repository. >> >>> 2) TestECDH.java: It's probably better to remove the provider name check >>> on line 116 and test on any providers that support the curve. >> OK, it's removed. I was thinking the same. >> >>> 3) oid.c: I think you can remove the comments that say "XXX bounds >>> check" (e.g. line 362). If I am interpreting these comments correctly, >>> they are saying that memcmp may read out of bounds, but you fixed that >>> problem by using oideql. >> I left them in place - my interpretation is, that they are meant for a >> check >> before accessing the *_oids arrays, as C arrays have no implicit check >> for that. >> As long as only oids from CurveDB are used, there would be no problems. >> The least significant byte of the requested oid is used as index. If >> that index >> exeeds the defined array length, something odd from the memory there >> is returned. >> At least that's my understynding of C and the comment there. >> >>> 4) Is there an existing test that exercises ECDSA with the new curves? >>> Maybe there is something in the PKCS11 tests that does this already, but >>> I didn't find it. I think we should have an ECDSA test to make sure that >>> we didn't forget anything. ECDSA test vectors probably aren't >>> necessary---a simple test that signs and verifies using the new curves >>> should be sufficient. >> Yes, there are tests in TestCurves, which runs through TestEC. >> TestCurves gets a List >> of all supported ECParameterSpec by the Provider and runs ECDSA >> signatures and verifications >> with all of them. The results of all curves are logged in the jtreg >> report of TestEC. >> >> I also changed the InvalidCurve test to use brainpoolP160r1 now, as >> brainpoolP256r1 is supported >> by using this patch. >> >>> On 1/12/2018 9:12 AM, Tobias Wagner wrote: >>>> Hi, >>>> >>>> here is the next patch for brainpool curve support in SunEC. >>>> >>>> Differences from the first patch: >>>> >>>> * Brainpool curves with less than 256 bits are removed. >>>> Subsequently, the curve oid check is made more robust to avoid null >>>> pointer caused Segmentation Faults in memcmp calls. >>>> >>>> * Bug JDK-8189594 is fixed. >>>> >>>> * Known answer tests for each new curve are added to >>>> sun.security.pkcs11.ec.TestECDH. The tests are only executed, if the >>>> tested provider's name is "SunEC" and the tested provider claims to >>>> support the respective curve. For SunEC, these tests are >>>> executed during sun.security.ec.TestEC. >>>> >>>> I decided to add these test vectors to TestECDH to avoid code >>>> duplications, as TestECDH is describes exactly the test >>>> for that kind of test vectors. >>>> The superclass to TestECDH, TestPKCS11, is also adapted to provide a >>>> method to check, whether one particular curve is >>>> supported. >>>> >>>> While the test vectors for the 256, 384 and 512 bit curve are taken >>>> from [1], the test vector for brainpoolP320r1 comes from [2]. >>>> The latter one is a draft version of RFC 6954. >>>> >>>> Regards, >>>> Tobias >>>> >>>> [1] https://tools.ietf.org/html/rfc7027#appendix-A >>>> [2] >>>> https://tools.ietf.org/html/draft-merkle-ikev2-ke-brainpool-00#appendix-A.5 >>>> >>>> >>>> >
Re: [PATCH]: Support for brainpool curves from CurveDB in SunEC
Hi Tobias, Just curious, which PKCS11 library did you use to test your patch? After I applied your patch and ran the regression tests, I noticed that both the Solaris PKCS11 library and NSS skipped testing Brainpool curves with different error codes which may be due to lack of support... Regards, Valerie On 1/17/2018 3:02 PM, Tobias Wagner wrote: -Ursprüngliche Nachricht- Von:Adam Petcher Gesendet: Die 16 Januar 2018 18:38 An: security-dev@openjdk.java.net Betreff: Re: [PATCH]: Support for brainpool curves from CurveDB in SunEC Great! I took a look at the patch, and I have some comments, the first of which probably needs to be addressed before I can test the change: 1) Is this patch against the http://hg.openjdk.java.net/jdk/jdk repository? I suspect it isn't because some of the paths are different than what I expect. We have made a lot of changes to the repositories in the last few months. If this patch is against an older repo, please send a patch against http://hg.openjdk.java.net/jdk/jdk . I switched to that repository now. As described on http://openjdk.java.net/contribute/, I was working with the http://hg.openjdk.java.net/jdk9/jdk9 repository. 2) TestECDH.java: It's probably better to remove the provider name check on line 116 and test on any providers that support the curve. OK, it's removed. I was thinking the same. 3) oid.c: I think you can remove the comments that say "XXX bounds check" (e.g. line 362). If I am interpreting these comments correctly, they are saying that memcmp may read out of bounds, but you fixed that problem by using oideql. I left them in place - my interpretation is, that they are meant for a check before accessing the *_oids arrays, as C arrays have no implicit check for that. As long as only oids from CurveDB are used, there would be no problems. The least significant byte of the requested oid is used as index. If that index exeeds the defined array length, something odd from the memory there is returned. At least that's my understynding of C and the comment there. 4) Is there an existing test that exercises ECDSA with the new curves? Maybe there is something in the PKCS11 tests that does this already, but I didn't find it. I think we should have an ECDSA test to make sure that we didn't forget anything. ECDSA test vectors probably aren't necessary---a simple test that signs and verifies using the new curves should be sufficient. Yes, there are tests in TestCurves, which runs through TestEC. TestCurves gets a List of all supported ECParameterSpec by the Provider and runs ECDSA signatures and verifications with all of them. The results of all curves are logged in the jtreg report of TestEC. I also changed the InvalidCurve test to use brainpoolP160r1 now, as brainpoolP256r1 is supported by using this patch. On 1/12/2018 9:12 AM, Tobias Wagner wrote: Hi, here is the next patch for brainpool curve support in SunEC. Differences from the first patch: * Brainpool curves with less than 256 bits are removed. Subsequently, the curve oid check is made more robust to avoid null pointer caused Segmentation Faults in memcmp calls. * Bug JDK-8189594 is fixed. * Known answer tests for each new curve are added to sun.security.pkcs11.ec.TestECDH. The tests are only executed, if the tested provider's name is "SunEC" and the tested provider claims to support the respective curve. For SunEC, these tests are executed during sun.security.ec.TestEC. I decided to add these test vectors to TestECDH to avoid code duplications, as TestECDH is describes exactly the test for that kind of test vectors. The superclass to TestECDH, TestPKCS11, is also adapted to provide a method to check, whether one particular curve is supported. While the test vectors for the 256, 384 and 512 bit curve are taken from [1], the test vector for brainpoolP320r1 comes from [2]. The latter one is a draft version of RFC 6954. Regards, Tobias [1] https://tools.ietf.org/html/rfc7027#appendix-A [2] https://tools.ietf.org/html/draft-merkle-ikev2-ke-brainpool-00#appendix-A.5
Re: [PATCH]: Support for brainpool curves from CurveDB in SunEC
On 1/18/2018 2:57 PM, Adam Petcher wrote: I applied your patch and ran some initial tests, and everything looks good so far. I'm running the the regression tests on all platforms, and I'll let you know how it goes. Regression tests passed, and this patch is ready to be reviewed.
Re: [PATCH]: Support for brainpool curves from CurveDB in SunEC
I applied your patch and ran some initial tests, and everything looks good so far. I'm running the the regression tests on all platforms, and I'll let you know how it goes. We need a JDK Reviewer to review and approve the code, so I'm hoping someone will volunteer to take a look. For the benefit of the reviewer: I checked all the math parts and everything seems like it should work. Both the point and field arithmetic are done using general-purpose functions that should work for any curve over a prime field. In the case of the field arithmetic, this means the performance is going to be much worse than in other curves. This is unavoidable, though, because Brainpool primes don't have any structure that we can use to optimize the field arithmetic. There are a couple of responses inline below. On 1/17/2018 6:02 PM, Tobias Wagner wrote: I switched to that repository now. As described on http://openjdk.java.net/contribute/, I was working with the http://hg.openjdk.java.net/jdk9/jdk9 repository. That page seems to be out of date. I'm working to get it updated. Thanks for letting me know. 3) oid.c: I think you can remove the comments that say "XXX bounds check" (e.g. line 362). If I am interpreting these comments correctly, they are saying that memcmp may read out of bounds, but you fixed that problem by using oideql. I left them in place - my interpretation is, that they are meant for a check before accessing the *_oids arrays, as C arrays have no implicit check for that. As long as only oids from CurveDB are used, there would be no problems. The least significant byte of the requested oid is used as index. If that index exeeds the defined array length, something odd from the memory there is returned. At least that's my understynding of C and the comment there. You're interpretation is better than mine, so I agree it's best to leave the comment in. If you wanted to, you could address the issue by comparing against sizeof(array)/sizeof(array[0]), but this is definitely not necessary. 4) Is there an existing test that exercises ECDSA with the new curves? Maybe there is something in the PKCS11 tests that does this already, but I didn't find it. I think we should have an ECDSA test to make sure that we didn't forget anything. ECDSA test vectors probably aren't necessary---a simple test that signs and verifies using the new curves should be sufficient. Yes, there are tests in TestCurves, which runs through TestEC. TestCurves gets a List of all supported ECParameterSpec by the Provider and runs ECDSA signatures and verifications with all of them. The results of all curves are logged in the jtreg report of TestEC. I see. This satisfies my request. Thanks.
Re: [PATCH]: Support for brainpool curves from CurveDB in SunEC
Great! I took a look at the patch, and I have some comments, the first of which probably needs to be addressed before I can test the change: 1) Is this patch against the http://hg.openjdk.java.net/jdk/jdk repository? I suspect it isn't because some of the paths are different than what I expect. We have made a lot of changes to the repositories in the last few months. If this patch is against an older repo, please send a patch against http://hg.openjdk.java.net/jdk/jdk . 2) TestECDH.java: It's probably better to remove the provider name check on line 116 and test on any providers that support the curve. 3) oid.c: I think you can remove the comments that say "XXX bounds check" (e.g. line 362). If I am interpreting these comments correctly, they are saying that memcmp may read out of bounds, but you fixed that problem by using oideql. 4) Is there an existing test that exercises ECDSA with the new curves? Maybe there is something in the PKCS11 tests that does this already, but I didn't find it. I think we should have an ECDSA test to make sure that we didn't forget anything. ECDSA test vectors probably aren't necessary---a simple test that signs and verifies using the new curves should be sufficient. On 1/12/2018 9:12 AM, Tobias Wagner wrote: Hi, here is the next patch for brainpool curve support in SunEC. Differences from the first patch: * Brainpool curves with less than 256 bits are removed. Subsequently, the curve oid check is made more robust to avoid null pointer caused Segmentation Faults in memcmp calls. * Bug JDK-8189594 is fixed. * Known answer tests for each new curve are added to sun.security.pkcs11.ec.TestECDH. The tests are only executed, if the tested provider's name is "SunEC" and the tested provider claims to support the respective curve. For SunEC, these tests are executed during sun.security.ec.TestEC. I decided to add these test vectors to TestECDH to avoid code duplications, as TestECDH is describes exactly the test for that kind of test vectors. The superclass to TestECDH, TestPKCS11, is also adapted to provide a method to check, whether one particular curve is supported. While the test vectors for the 256, 384 and 512 bit curve are taken from [1], the test vector for brainpoolP320r1 comes from [2]. The latter one is a draft version of RFC 6954. Regards, Tobias [1] https://tools.ietf.org/html/rfc7027#appendix-A [2] https://tools.ietf.org/html/draft-merkle-ikev2-ke-brainpool-00#appendix-A.5
Re: [PATCH]: Support for brainpool curves from CurveDB in SunEC
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//images/jdk ".
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.
Re: [PATCH]: Support for brainpool curves from CurveDB in SunEC
On 12/16/2017 2:52 PM, Tobias Wagner wrote: -Ursprüngliche Nachricht- Von:Adam Petcher 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
Re: [PATCH]: Support for brainpool curves from CurveDB in SunEC
On 12/15/2017 11:31 AM, Tobias Wagner wrote: Hi, in our current project, we have the requirement to support brainpool curves for TLS connections (RFC 7027). As part of this requirement, we introduced the brainpoolP*r1 curves to SunEC, as they are already known in sun.security.util.CurveDB. It does not introduce the twisted curves from RFC 5639. We want to share this patch, hoping it might be useful for others. Especially for public funded projects (e.g. health care or eID) in Europe, the use or at least support for these curves is often mandatory. Great! You are going to make it a happy Christmas for a lot of people. I would be happy to sponsor this change, and I have a few initial requests/comments/questions before I look at the patch in more detail: 1) Please include a test which checks the new curves against some test vectors. Ideally, these vectors come from a standard (e.g. RFC 7027 has some). 2) The existing tests will check for some form of correctness for all enabled curves, but this doesn't ensure that the Brainpool curves are enabled. So you should also add a test that ensures that the new curves are enabled. This can be combined with the previously mentioned test against the test vectors. 3) We can't push this change to the repo without also fixing JDK-8189594. So I think you have a couple of options. Either submit a separate patch for JDK-8189594 first (which may be tricky because you will need to find a way to write a test for it) or include the fix for JDK-8189594 in this patch. 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.
