Conceptual feedback on new ECC JEP

[Adam Petcher]

On 9/5/2018 5:53 PM, Michael StJohns wrote:

>
> BigInteger is not required to encode a PKCS8 key, but its trivial to 
> translate from BigInteger to PKCS8 and vice versa.  Note that 
> internally, the current BigInteger stores the magnitude as an array of 
> int in big endian order and stores the sign separately. The BigInteger 
> (int sigNum, byte[] magnitude) constructor mostly just copies the 
> magnitude bytes over (and converts 4 bytes to an integer) for positive 
> integers.   While the current BigInteger doesn't have a byte[] 
> BigInteger.getMagnitude() call, it would be trivial to subclass 
> BigInteger to copy the magnitude bytes (without branching) out.
>
> In any event, for NewEC to OldEc - you do use the sign/magnitude call 
> to create the BigInteger - no leaking on the part of NewEC, and once 
> it gets to OldEC its not your problem.  For OldEc to NewEc you copy 
> the BigInteger to NewBigInteger (trivial wrapper class) and do a 
> getMagnitude().

See the code for BigInteger[1]. The sign/magnitude constructor calls 
stripLeadingZeroBytes (line 405) on the magnitude to convert it to an 
int array. This method compares the leading values in the byte array to 
zero in a short-circuit expression in a for loop (line 4298). So this 
constructor branches on the value that is supplied to it, and it cannot 
be used in a branchless implementation.

I don't know how to write this branchless getMagnitude method that you 
are proposing. I would need to convert from a variable-length int array 
to a fixed-length byte array. In doing this, I can't branch on the 
length of the variable-length array, because doing so would leak whether 
the high-order bits of the key are zero. If you know how to do this, 
then please provide some code to illustrate how it is done.

[1] 
http://hg.openjdk.java.net/jdk/jdk/file/805807f15830/src/java.base/share/classes/java/math/BigInteger.java


> While the contract for getEncoded() explicitly allows for a null 
> result, getS() of ECPrivateKey does not.    This more than any other 
> reason appears to be why the PKCS 11 provider represents the 
> ECPrivateKey as a simple PrivateKey object.

Good point. There is no need for the private keys for this provider to 
implement ECPrivateKey, since there is only one method that is 
effectively unimplemented. We should use a new implementation class (not 
ECPrivateKeyImpl) that implements PrivateKey, not ECPrivateKey.

> So implementing your provider requires other providers to change? 
> Because?   Do you expect BouncyCastle and NSS to change as well?

I think the situation is improved by not having the new private keys 
implement ECPrivateKey. Then the JDK ECKeyFactory code does not need to 
change, and neither do other providers, assuming they behave in a 
similar way. Though I think it is acceptable if other providers don't 
behave this way, and therefore cannot translate private keys from this 
new provider. I've updated the JEP to indicate that interoperability 
with these providers is a non-goal.

>
>>
>> The only way to get private keys in or out of the new provider is 
>> through a PKCS#8 encoding. Moving keys to/from another provider that 
>> supports ECPrivateKeySpec but not PKCS#8 encoding can be accomplished 
>> by translating the specs---possibly with the help of a KeyFactory 
>> that supports both, like the one in SunEC.
>>
> *pounds head against table*   Bits are bits are bits.  Creating a 
> PKCS8EncodedKeySpec gets you a byte array which you can convert to a 
> BigInteger by decoding the PKCS8 structure and taking the PKCS8 
> PrivateKey octets and doing new BigInteger (1, privateKeyOctets).
>
> That doesn't require ASN1 integer representation (e.g. leading byte is 
> zero if high bit is set) and doesn't cause a branch.

See above for my explanation of why this solution does, in fact, branch.