MetacoSA / NBitcoin

Comprehensive Bitcoin library for the .NET framework.
MIT License
1.88k stars 848 forks source link

A critical security bug in nbitcoin address generation #980

Closed Stonica closed 3 years ago

Stonica commented 3 years ago

Please read: https://stackoverflow.com/questions/66694686/nbitcoin-generates-duplicate-bitcoin-addresse-corresponding-to-private-keys-equa

lontivero commented 3 years ago

Who knows how the user is generating the keys, I was playing a bit with that and there are no duplicated addresses. I used different versions of this:

public void ShouldNotGenerateDuplicated()
{
    var zeros = Enumerable.Repeat((byte)0, 30).ToArray();
    var addresses = Enumerable.Range(1, 512)
        .Select(x => new Key(zeros.Concat(BitConverter.GetBytes((ushort)x)).ToArray()))
        .Select(sk => sk.PubKey.GetAddress(ScriptPubKeyType.Legacy, Network.Main).ToString())
        .ToHashSet();

    Assert.Equal(512, addresses.Count());
}
NicolasDorier commented 3 years ago

I highly doubt it. some private keys that are equal to 1 and 256 and what does it mean?

Stonica commented 3 years ago

Use following code as POC:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Numerics;
using System.Text;
using System.Threading.Tasks;
using NBitcoin;

namespace Bitcoin_address_generation
{
    class Program
    {
        static void Main(string[] args)
        {
            //generating a private key that is equal to 1
            byte[] _priv_bytes1 = IntegerToBytes(1, 32);

            Key _private_key1 = new Key();
            _private_key1.FromBytes(_priv_bytes1);

            //generating a private key that is equal to 256
            byte[] _priv_bytes256 = IntegerToBytes(256, 32);

            Key _private_key256 = new Key();
            _private_key256.FromBytes(_priv_bytes1);

            Console.WriteLine(_private_key1.PubKey.GetAddress(ScriptPubKeyType.Legacy, Network.Main).ToString());

            Console.WriteLine(_private_key256.PubKey.GetAddress(ScriptPubKeyType.Legacy, Network.Main).ToString());
            Console.ReadKey();

        }

        /// <summary>
        /// convert a big integer to byte array
        /// </summary>
        /// <param name="s"></param>
        /// <param name="qLength"></param>
        /// <returns></returns>
        public static byte[] IntegerToBytes(BigInteger s, int qLength)
        {
            byte[] bytes = s.ToByteArray();

            if (qLength < bytes.Length)
            {
                byte[] tmp = new byte[qLength];

                Array.Copy(bytes, bytes.Length - tmp.Length, tmp, 0, tmp.Length);

                return tmp;
            }

            if (qLength > bytes.Length)
            {
                byte[] tmp = new byte[qLength];

                Array.Copy(bytes, 0, tmp, tmp.Length - bytes.Length, bytes.Length);

                return tmp;
            }

            return bytes;
        }
    }
}
lontivero commented 3 years ago

First, I would initialize private_key1 with _priv_bytes1 and _private_key256 with _priv_bytes256. In your piece of code you are initializing both with the same array _priv_bytes1 and by doing that you are getting exactly the same private key. After you fix that I would suggest you to take a very careful look at your IntegerToBytes because it is obviouly wrong in many many ways.

Stonica commented 3 years ago

First, I would initialize private_key1 with _priv_bytes1 and _private_key256 with _priv_bytes256. In your piece of code you are initializing both with the same array _priv_bytes1 and by doing that you are getting exactly the same private key. After you fix that I would suggest you to take a very careful look at your IntegerToBytes because it is obviouly wrong in many many ways.

The same issue after correcting the code:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Numerics;
using System.Text;
using System.Threading.Tasks;
using NBitcoin;

namespace Bitcoin_address_generation
{
    class Program
    {
        static void Main(string[] args)
        {
            //generating a private key that is equal to 1
            byte[] _priv_bytes1 = convert_integer_to_byte(1, 32);

            Key _private_key1 = new Key();
            _private_key1.FromBytes(_priv_bytes1);

            //generating a private key that is equal to 256
            byte[] _priv_bytes256 = convert_integer_to_byte(256, 32);

            Key _private_key256 = new Key();
            _private_key256.FromBytes(_priv_bytes256);

            Console.WriteLine(_private_key1.PubKey.GetAddress(ScriptPubKeyType.Legacy, Network.Main).ToString());

            Console.WriteLine(_private_key256.PubKey.GetAddress(ScriptPubKeyType.Legacy, Network.Main).ToString());
            Console.ReadKey();

        }

        /// <summary>
        /// convert a big integer to byte array
        /// </summary>
        /// <param name="s"></param>
        /// <param name="qLength"></param>
        /// <returns></returns>
        public static byte[] convert_integer_to_byte(BigInteger _input, int qlength)
        {
            byte[] _temp_byte = _input.ToByteArray();
            byte[] _result_byte;
            if (_temp_byte.Length < qlength)
            {
                _result_byte = new byte[qlength];
                _temp_byte.CopyTo(_result_byte, qlength - _temp_byte.Length);
            }
            else
            {
                _result_byte = new byte[qlength];
                _temp_byte.CopyTo(_result_byte, 0);
            }
            return _result_byte;
        }
    }
}
Stonica commented 3 years ago

I highly doubt it. some private keys that are equal to 1 and 256 and what does it mean?

Hi Nicolas, Thanks for your reply. I am to have an in-memory list of all bitcoin addresses with balances more than zero, this list must be updated just any address balance has changed. This in-memory list is used for querying the balance of bitcoin addresses near realtime. I have Googled so much on NBxplorer documents and how to list all bitcoin addresses with balance, but I was not found any useful document or help on which code and how I must use. What I found about NBxplorer is that it only tracks one or some addresses but I want all Bitcoin address with non-zero balance, I appreciate if you help me how to accomplish this task.

lontivero commented 3 years ago

@Stonica there is no bug in NBitcoin, the bug is your code. You are creating the same private key and that's why you are getting the exact same addresses. Your convert_interger_to_bytes is creating the same array for all 256^n (1, 256, 65536 and so on).

dangershony commented 3 years ago

Before going to stackoverflow ask the devs here first? 🤔

Stonica commented 3 years ago

lontivero and dangershony, you are right, I found my mistake and thanks for your help. I answered the stackoverflow question as:

https://stackoverflow.com/questions/66694686/nbitcoin-generates-duplicate-bitcoin-addresse-corresponding-to-private-keys-equa/66783210#66783210