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

[Tomas Gustavsson]

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