RFR 8182999: SunEC throws ProviderException on invalid curves

[Michael StJohns]

On 7/10/2017 12:59 PM, Adam Petcher wrote:
> This fix addresses an issue in which the provider behaves incorrectly 
> when initialized with parameters for a curve that is not supported by 
> the provider. If I am interpreting your suggestion correctly, it 
> sounds like you are requesting a change to the set of curves that is 
> supported by the provider. While this change may be a good idea, it is 
> not within the scope of this ticket.
>
> If you want SunEC to support arbitrary curve parameters, you will need 
> to create a separate ticket for that. I suspect this change would 
> require a fair amount of work (if it is even possible), and it may not 
> be worth the effort.

EC is designed around a general set of math.  The math depends on the 
actual curve data (e.g. an EllipticCurve, a basepoint ECPoint g and an 
order and cofactor  - all the components of ECParameterSpec).   
ECGenParameterSpec is a simple wrapper around a string that's meant to 
provide a easy way for a human to specify a preexisting set of curve 
data.  When you get down into the provider its the math and the bits in 
the ECParameterSpec that matter, not the string or oid in the 
ECGenParameterSpec.

In any event, if you already have the curve data, then the fix is to 
replace the groupings of "if (EC_DecodeParams(...) {}" with a routine to 
fill the ECParams structure from the encodedParams you're passing in. 
(at lines 118, 233,346, 429 in ECC_JNI.cpp).

That would look like a different version of EC_FillParams  (in 
impl/ecdecode.c) - which actually decoded the algorithm parameter data 
rather than just looking for an OID choice and filling in from there.

*sigh* It looks like ECParameters.java is also incomplete.

What I'm mostly trying to get at here is to decouple  or remove the list 
of curves in ecdecode.c in favor of the list in the java stuff 
(CurveDB.java) (or elsewhere).   The C code should mostly only have to 
deal with the math and not the housekeeping.

Mike






>
>
> On 7/10/2017 12:17 PM, Michael StJohns wrote:
>> Actually - wouldn't it make a lot more sense to generalize the 
>> provider so it can take ANY set of curve data? Locking this to only 
>> what has an OID to parameters mapping doesn't seem to be actually 
>> meeting the contract for an EC key generator.
>>
>> I understand a number of tools (e.g. PKIX related/keytool) can't be 
>> used without the OID, but this isn't at that level.
>>
>> The webrev feels more like a bandaid than a solution.
>>
>> Mike
>>
>>
>> On 7/10/2017 12:03 PM, Seán Coffey wrote:
>>> Thanks for the update! Looks fine to me.
>>>
>>> Regards,
>>> Sean.
>>>
>>> On 10/07/17 16:13, Adam Petcher wrote:
>>>> New webrev: http://cr.openjdk.java.net/~apetcher/8182999/webrev.01/
>>>>
>>>> Yes, this is a good idea. I made this work by printing out the 
>>>> value from AlgorithmParameters.toString(), so hopefully that means 
>>>> you should always get a useful string. At the moment (with SunEC 
>>>> AlgorithmParameters), the string prints the friendly name followed 
>>>> by the OID:
>>>>
>>>> Unsupported curve: brainpoolP256r1 (1.3.36.3.3.2.8.1.1.7)
>>>>
>>>> On 7/7/2017 4:12 PM, Seán Coffey wrote:
>>>>> Adam,
>>>>>
>>>>> would it be useful to get the curve name in the new exception ? I 
>>>>> think it would help with future debugging. Line 96 already gets 
>>>>> the curve name if we're dealing with ECGenParameterSpec instance. 
>>>>> I think the same approach could be applied to your new code.
>>>>>
>>>>> Regards,
>>>>> Sean.
>>>>>
>>>>>
>>>>> On 07/07/2017 19:59, Adam Petcher wrote:
>>>>>> This is a bug fix related to invalid curves in the SunEC 
>>>>>> provider. During ECKeyPairGenerator.initialize(), the provider 
>>>>>> only checks whether the curve is known, but it doesn't check 
>>>>>> whether the curve is actually supported by the native code. So 
>>>>>> the call to generateKeyPair() can fail in the native code and 
>>>>>> throw a ProviderException. This change adds a new native method 
>>>>>> to check whether the curve is supported. This method is called by 
>>>>>> initialize(), which will set the state to uninitialized and throw 
>>>>>> the expected exception when the curve is not supported.
>>>>>>
>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8182999
>>>>>> Webrev: http://cr.openjdk.java.net/~apetcher/8182999/webrev.00/
>>>>>>
>>>>>
>>>>
>>>
>>
>