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

Tobias Wagner tobias.wagner at n-design.de
Wed Jan 17 23:02:40 UTC 2018

-----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
> >
> >
-------------- next part --------------
A non-text attachment was scrubbed...
Name: jdk_patch_48544-48551.diff
Type: application/octet-stream
Size: 24791 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/security-dev/attachments/20180118/a5e8ace0/jdk_patch_48544-48551-0001.diff>

More information about the security-dev mailing list