Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent Length of Private Key Byte Array in deriveKeyPair Method #486

Closed
sublimator opened this issue Sep 23, 2023 · 2 comments
Closed

Comments

@sublimator
Copy link

Inconsistent Length of Private Key Byte Array in deriveKeyPair Method

I stumbled upon this browsing Github and not understanding how the private key was 33 bytes without a prefix.

Description

The deriveKeyPair method occasionally produces private keys with lengths of either 32 or 33 bytes. This inconsistency is attributed to the behavior of the BigInteger.toByteArray() method. It returns the byte array in two's-complement form and occasionally prepends a zero byte to ensure the number is not negative.

      private static KeyPair deriveKeyPair(final UnsignedByteArray seedBytes, final int accountNumber) {
        Objects.requireNonNull(seedBytes);

        // private key needs to be a BigInteger so we can derive the public key by multiplying G by the private key.
        final BigInteger privateKeyInt = derivePrivateKey(seedBytes, accountNumber);
        final UnsignedByteArray publicKeyInt = derivePublicKey(privateKeyInt);

        return KeyPair.builder()
          .privateKey(PrivateKey.of(UnsignedByteArray.of( /*>>>*/ privateKeyInt.toByteArray() /*<<<*/ )))
          .publicKey(PublicKey.fromBase16EncodedPublicKey(
            UnsignedByteArray.of(publicKeyInt.toByteArray()).hexValue()
          ))
          .build();
      }

The length of the byte array byteArray is sometimes 33 bytes and sometimes 32 bytes, which could lead to potential issues downstream where a fixed-length byte array might be expected.

Expected Behavior

Consistent length for the private key's byte array.

Actual Behavior

Variable length of 32 or 33 bytes for the private key's byte array.

Why

The inconsistency is due to the two's-complement representation that BigInteger.toByteArray() uses. Sometimes a 0 byte is prepended to make sure the number is not interpreted as negative.

Note

The JavaScript edition of ripple-keypairs uses a 33-byte representation with a 0 byte prefix. The secp256k1 curve generally uses 32-byte scalars naturally.

Possible Solution

Normalize the private key to a consistent length. Either:

  • Consistently use a 33-byte representation with a 0 byte prefix for compatibility with other Ripple products.
  • Or normalize to a 32-byte scalar which aligns with the natural length used in secp256k1.

Fixtures

Seed Hex Private Key Hex Private Key Length
358c75d5ad5e2ff5e19256978f18f0b9 00fbd60c9c99ba3d4706a449c30e4d61dcc3811e23ef69291f98a886cec6a8b0b5 33
b7e99c6bb9786494238d2ba84eabe854 7030cbd40d6961e625ad73159a4b463aa42b4e88cc2248ac49e1edcb50af2924 32
@nkramer44
Copy link
Collaborator

@sublimator, I spoke with @sappenin yesterday and we've decided to normalize all secp256k1 private keys to 33 bytes, prefixed with 0x00 to be consistent with xrpl.js and xrpl-py. PR should be ready soon.

sappenin added a commit that referenced this issue Sep 28, 2023
1. Add a precondition to PrivateKey.of that enforces the length to 33 bytes (starting with either  0x00 or 0xED).
2. Update BCKeyUtils to add/remove the padding for secp keys (the way we do for Ed keys).
3. Add a new function to PrivateKey called `valueWithoutPrefix` which returns the natural 32 private key bytes.
4. Update Secp256k1KeyPairService in Seed.java to prefix with private keys with 0x00 if only 32 bytes.
5. Validate that `PrivateKey.keyType()` works properly via unit tests.
@sappenin
Copy link
Collaborator

sappenin commented Oct 5, 2023

This is now fixed via #487.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants