On 9/6/2018 9:53 AM, Adam Petcher wrote:
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.
Simple way to do this is to extend BigInteger and fix these perceived
problems. You may have to replace/replicate pretty much everything, but
as long as it has a BigInteger signature when output, you're good to
go. Actually, do that - clone BigInteger.java as SafeBigInteger.java,
have extend java.math.BigInteger and change out the things that bother
you. I checked - BigInteger is not a final class so this should work.
E.g. as long as *I* can call the functions I need to call on the
object you exported, I'm fine with it.
In any event, you're still missing the point here. You're EXPORTING the
key from your provider to other providers which probably already don't
care that much about the one in 256 (approx - slightly larger) time
where exporting the key might leak the fact of leading zero bits.
[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.
*pounds head on table* Sure. And then you can't use any translation
features and then you break all the code.
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.
OK. At this point you're no longer calling this an EC key. Make sure
you catch all the places where ECPrivateKey is used instead of
PrivateKey. I think you're incredibly short sighted and not a lot of
folks will implement this, but go for it. I think its a *really* bad
idea and that you're fixing the wrong problems.
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.
See my commentary on why this still doesn't matter.
Later, Mike
