HDW: Add a getKeyByPath() method to wallet, and (experimental) sanity check precomputed keys before they're returned by the wallet API to guard against the potential of a bitflip in either the stored pubkey bytes, or the ECC precomputed tables. The chance of such a bit flip is small but such flips HAVE been seen in the wild against other bitcoin implementations, so it could happen to a bitcoinj user too. The consequences can be catastrophic (destroyed money) so best to try and do what we can.

This commit is contained in:
Mike Hearn 2014-08-15 18:06:06 +02:00
parent f8bc4d544e
commit f00aef2048
4 changed files with 79 additions and 19 deletions

View File

@ -18,10 +18,7 @@
package com.google.bitcoin.core;
import com.google.bitcoin.core.TransactionConfidence.ConfidenceType;
import com.google.bitcoin.crypto.DeterministicKey;
import com.google.bitcoin.crypto.KeyCrypter;
import com.google.bitcoin.crypto.KeyCrypterException;
import com.google.bitcoin.crypto.KeyCrypterScrypt;
import com.google.bitcoin.crypto.*;
import com.google.bitcoin.params.UnitTestParams;
import com.google.bitcoin.script.Script;
import com.google.bitcoin.script.ScriptBuilder;
@ -886,6 +883,20 @@ public class Wallet extends BaseTaggableObject implements Serializable, BlockCha
}
}
/**
* Returns a key for the given HD path, assuming it's already been derived. You normally shouldn't use this:
* use currentReceiveKey/freshReceiveKey instead.
*/
public DeterministicKey getKeyByPath(List<ChildNumber> path) {
lock.lock();
try {
maybeUpgradeToHD();
return keychain.getActiveKeyChain().getKeyByPath(path, false);
} finally {
lock.unlock();
}
}
/**
* Convenience wrapper around {@link Wallet#encrypt(com.google.bitcoin.crypto.KeyCrypter,
* org.spongycastle.crypto.params.KeyParameter)} which uses the default Scrypt key derivation algorithm and

View File

@ -24,6 +24,7 @@ import org.spongycastle.math.ec.ECPoint;
import java.math.BigInteger;
import java.nio.ByteBuffer;
import java.security.SecureRandom;
import java.util.Arrays;
import static com.google.common.base.Preconditions.checkArgument;
@ -34,6 +35,8 @@ import static com.google.common.base.Preconditions.checkState;
* deterministic wallet child key generation algorithm.
*/
public final class HDKeyDerivation {
// Some arbitrary random number. Doesn't matter what it is.
private static final BigInteger RAND_INT = new BigInteger(256, new SecureRandom());
private HDKeyDerivation() { }
@ -43,7 +46,7 @@ public final class HDKeyDerivation {
*/
public static final int MAX_CHILD_DERIVATION_ATTEMPTS = 100;
private static final HMac MASTER_HMAC_SHA512 = HDUtils.createHmacSha512Digest("Bitcoin seed".getBytes());
public static final HMac MASTER_HMAC_SHA512 = HDUtils.createHmacSha512Digest("Bitcoin seed".getBytes());
/**
* Generates a new deterministic key from the given seed, which can be any arbitrary byte array. However resist
@ -119,7 +122,7 @@ public final class HDKeyDerivation {
*/
public static DeterministicKey deriveChildKey(DeterministicKey parent, ChildNumber childNumber) throws HDDerivationException {
if (parent.isPubKeyOnly()) {
RawKeyBytes rawKey = deriveChildKeyBytesFromPublic(parent, childNumber);
RawKeyBytes rawKey = deriveChildKeyBytesFromPublic(parent, childNumber, PublicDeriveMode.NORMAL);
return new DeterministicKey(
HDUtils.append(parent.getPath(), childNumber),
rawKey.chainCode,
@ -136,7 +139,7 @@ public final class HDKeyDerivation {
}
}
private static RawKeyBytes deriveChildKeyBytesFromPrivate(DeterministicKey parent,
public static RawKeyBytes deriveChildKeyBytesFromPrivate(DeterministicKey parent,
ChildNumber childNumber) throws HDDerivationException {
checkArgument(parent.hasPrivKey(), "Parent key must have private key bytes for this method.");
byte[] parentPublicKey = ECKey.compressPoint(parent.getPubKeyPoint()).getEncoded();
@ -160,7 +163,12 @@ public final class HDKeyDerivation {
return new RawKeyBytes(ki.toByteArray(), chainCode);
}
private static RawKeyBytes deriveChildKeyBytesFromPublic(DeterministicKey parent, ChildNumber childNumber) throws HDDerivationException {
public enum PublicDeriveMode {
NORMAL,
WITH_INVERSION
}
public static RawKeyBytes deriveChildKeyBytesFromPublic(DeterministicKey parent, ChildNumber childNumber, PublicDeriveMode mode) throws HDDerivationException {
checkArgument(!childNumber.isHardened(), "Can't use private derivation with public keys only.");
byte[] parentPublicKey = ECKey.compressPoint(parent.getPubKeyPoint()).getEncoded();
assert parentPublicKey.length == 33 : parentPublicKey.length;
@ -173,7 +181,27 @@ public final class HDKeyDerivation {
byte[] chainCode = Arrays.copyOfRange(i, 32, 64);
BigInteger ilInt = new BigInteger(1, il);
assertLessThanN(ilInt, "Illegal derived key: I_L >= n");
ECPoint Ki = ECKey.CURVE.getG().multiply(ilInt).add(parent.getPubKeyPoint());
final ECPoint G = ECKey.CURVE.getG();
final BigInteger N = ECKey.CURVE.getN();
ECPoint Ki;
switch (mode) {
case NORMAL:
Ki = G.multiply(ilInt).add(parent.getPubKeyPoint());
break;
case WITH_INVERSION:
// This trick comes from Gregory Maxwell. Check the homomorphic properties of our curve hold. The
// below calculations should be redundant and give the same result as NORMAL but if the precalculated
// tables have taken a bit flip will yield a different answer. This mode is used when vending a key
// to perform a last-ditch sanity check trying to catch bad RAM.
Ki = G.multiply(ilInt.add(RAND_INT));
BigInteger additiveInverse = RAND_INT.negate().mod(N);
Ki = Ki.add(G.multiply(additiveInverse));
Ki = Ki.add(parent.getPubKeyPoint());
break;
default: throw new AssertionError();
}
assertNonInfinity(Ki, "Illegal derived key: derived public key equals infinity.");
return new RawKeyBytes(Ki.getEncoded(true), chainCode);
}
@ -193,10 +221,10 @@ public final class HDKeyDerivation {
throw new HDDerivationException(errorMessage);
}
private static class RawKeyBytes {
private final byte[] keyBytes, chainCode;
public static class RawKeyBytes {
public final byte[] keyBytes, chainCode;
private RawKeyBytes(byte[] keyBytes, byte[] chainCode) {
public RawKeyBytes(byte[] keyBytes, byte[] chainCode) {
this.keyBytes = keyBytes;
this.chainCode = chainCode;
}

View File

@ -33,10 +33,7 @@ import org.spongycastle.math.ec.ECPoint;
import javax.annotation.Nullable;
import java.math.BigInteger;
import java.security.SecureRandom;
import java.util.ArrayList;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.*;
import java.util.concurrent.Executor;
import java.util.concurrent.locks.ReentrantLock;
@ -369,7 +366,14 @@ public class DeterministicKeyChain implements EncryptableKeyChain {
List<DeterministicKey> keys = new ArrayList<DeterministicKey>(numberOfKeys);
for (int i = 0; i < numberOfKeys; i++) {
ImmutableList<ChildNumber> path = HDUtils.append(parentKey.getPath(), new ChildNumber(index - numberOfKeys + i, false));
keys.add(hierarchy.get(path, false, false));
DeterministicKey k = hierarchy.get(path, false, false);
// Just a last minute sanity check before we hand the key out to the app for usage. This isn't inspired
// by any real problem reports from bitcoinj users, but I've heard of cases via the grapevine of
// places that lost money due to bitflips causing addresses to not match keys. Of course in an
// environment with flaky RAM there's no real way to always win: bitflips could be introduced at any
// other layer. But as we're potentially retrieving from long term storage here, check anyway.
checkForBitFlip(k);
keys.add(k);
}
return keys;
} finally {
@ -377,6 +381,14 @@ public class DeterministicKeyChain implements EncryptableKeyChain {
}
}
private void checkForBitFlip(DeterministicKey k) {
DeterministicKey parent = checkNotNull(k.getParent());
byte[] rederived = HDKeyDerivation.deriveChildKeyBytesFromPublic(parent, k.getChildNumber(), HDKeyDerivation.PublicDeriveMode.WITH_INVERSION).keyBytes;
byte[] actual = k.getPubKey();
if (!Arrays.equals(rederived, actual))
throw new IllegalStateException(String.format("Bit-flip check failed: %s vs %s", Arrays.toString(rederived), Arrays.toString(actual)));
}
private void addToBasicChain(DeterministicKey key) {
basicKeyChain.importKeys(ImmutableList.of(key));
}
@ -471,12 +483,12 @@ public class DeterministicKeyChain implements EncryptableKeyChain {
}
/** Returns the deterministic key for the given absolute path in the hierarchy. */
protected DeterministicKey getKeyByPath(ImmutableList<ChildNumber> path) {
protected DeterministicKey getKeyByPath(List<ChildNumber> path) {
return getKeyByPath(path, false);
}
/** Returns the deterministic key for the given absolute path in the hierarchy, optionally creating it */
public DeterministicKey getKeyByPath(ImmutableList<ChildNumber> path, boolean create) {
public DeterministicKey getKeyByPath(List<ChildNumber> path, boolean create) {
return hierarchy.get(path, false, create);
}

View File

@ -125,6 +125,15 @@ public class ChildKeyDerivationTest {
}
}
@Test
public void inverseEqualsNormal() throws Exception {
DeterministicKey key1 = HDKeyDerivation.createMasterPrivateKey("Wired / Aug 13th 2014 / Snowden: I Left the NSA Clues, But They Couldn't Find Them".getBytes());
HDKeyDerivation.RawKeyBytes key2 = HDKeyDerivation.deriveChildKeyBytesFromPublic(key1.getPubOnly(), ChildNumber.ZERO, HDKeyDerivation.PublicDeriveMode.NORMAL);
HDKeyDerivation.RawKeyBytes key3 = HDKeyDerivation.deriveChildKeyBytesFromPublic(key1.getPubOnly(), ChildNumber.ZERO, HDKeyDerivation.PublicDeriveMode.WITH_INVERSION);
assertArrayEquals(key2.keyBytes, key3.keyBytes);
assertArrayEquals(key2.chainCode, key3.chainCode);
}
@Test
public void encryptedDerivation() throws Exception {
// Check that encrypting a parent key in the heirarchy and then deriving from it yields a DeterministicKey