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

[Valerie Peng]

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 <adam.petcher at oracle.com>
>> Gesendet: Die 16 Januar 2018 18:38
>> An: security-dev at 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
>>>
>>>