MetacoSA / NBitcoin

Comprehensive Bitcoin library for the .NET framework.
MIT License
1.89k stars 849 forks source link

Custom BouncyCastle.Security.SecureRandom not available on WP8.1 #53

Closed fonix232 closed 9 years ago

fonix232 commented 9 years ago

As the title says. At the moment it is the only class that implements IRandom and can be used with it - and it is not available on Windows Phone.

fonix232 commented 9 years ago

Never mind, ignore my stupidity. Solved with a custom implementation.

NicolasDorier commented 9 years ago

fonix, can you share your custom implementation ? Is it the same as the normal Windows one ? I want to create a Windows specific profile and configure the IRandom automatically in it. With a Windows specific profile, I can include the BIP70, and use the Windows crypto API instead of BC for non ecc crypto operations. (by the way, if you don't generate keys, you can use the UnsecureRandom)

fonix232 commented 9 years ago

Of course I can share it, though it is only aimed at providing a SecureRandom implementation with IRandom for WP8.1/Win8.1 using the WinRT APIs.

It also requires the Portable.BouncyCastle dependency on WP8.1 as your NBitcoin.BouncyCastle does not support that target yet.

using NBitcoin;
using System;
using System.Collections.Generic;
using System.Text;

namespace BitWallet.Helpers
{
#if WINDOWS_PHONE_APP
    using Org.BouncyCastle.Security;
#else
    using NBitcoin.BouncyCastle.Security;
#endif

    public class SecRandom : IRandom
    {
        SecureRandom _Rand = new SecureRandom();

        public void GetBytes(byte[] output)
        {
            lock(_Rand)
            {
                _Rand.NextBytes(output);
            }
        }
    }

}

It is not much to look at, but solves the WinRT inter-compatibility issue between the SecureRandom implementations. Once there's a universal SecureRandom for both platforms, SecRandom can be modified to be e.g. WinRTRandom, and use either the BouncyCastle implementation, or the simple Camera randomizer I suggested a few months back.

NicolasDorier commented 9 years ago

What do you mean by "NBitcoin.BouncyCastle does not support that target" ?

One of the profile is "portable-net45+win+wpa81+Xamarin.iOS10+MonoAndroid10+MonoTouch10" And NBitcoin.BouncyCastle is inside, so it should work. If it does not, can you tell me exactly what type of project you created ?

However, I think you refer to the fact that in my code of NBitcoin.BouncyCastle, I had to comment the underlying windows plateform specific RNG for the profile version of SecureRandom. (note CryptoApiRandomGenerator rely on RNGCryptoServiceProvider, a windows class)

    private static readonly SecureRandom master = null;//new SecureRandom(new CryptoApiRandomGenerator());
    private static SecureRandom Master
    {
        get
        {
            return master;
        }
    }

This make SecureRandom unusable in my package. But since you are in a windows project, I highly advice you to just use my package, and make your own IRandom implementation that use directly the "RNGCryptoServiceProvider" class from Windows, without using the SecureRandom

This one should work :

public class RNGCryptoServiceProviderRandom : IRandom
{
    readonly RNGCryptoServiceProvider _Instance;
    public RNGCryptoServiceProviderRandom()
    {
        _Instance = new RNGCryptoServiceProvider();
    }
    #region IRandom Members

    public void GetBytes(byte[] output)
    {
        _Instance.GetBytes(output);
    }

    #endregion
}

You should not need to use Portable.BouncyCastle, or it means I messed up something with profiles

NicolasDorier commented 9 years ago

Man, seriously, don't use the SecureRandom of Portable.BouncyCastle, 14 bits of entropy, your keys will get hacked....

    private static readonly SecureRandom[] master = { null };
    private static SecureRandom Master
    {
        get
        {
            lock (master)
            {
                if (master[0] == null)
                {
                    SecureRandom sr = master[0] = GetInstance("SHA256PRNG", false);

                    // Even though Ticks has at most 8 or 14 bits of entropy, there's no harm in adding it.
                    sr.SetSeed(DateTime.Now.Ticks);

                    // 32 will be enough when ThreadedSeedGenerator is fixed.  Until then, ThreadedSeedGenerator returns low
                    // entropy, and this is not sufficient to be secure. http://www.bouncycastle.org/csharpdevmailarchive/msg00814.html
                    sr.SetSeed(new ThreadedSeedGenerator().GenerateSeed(32, true));
                }

                return master[0];
            }
        }
    }

After checking RNGCryptoServiceProvider is not support in WinRt... So you'll likely need to use it were supported (WP), and use http://stackoverflow.com/questions/14582008/what-cryptographically-secure-options-are-there-for-creating-random-numbers-in-w for winrt... (but you should check if this last it is effectively secure by asking to ms)

realindiahotel commented 9 years ago

I am using SecureRandom in BouncyCastle PCL But I am not relying on the standard seed for entropy. I also use Sha256 of The milliseconds system has been online AND processor affinity (number of CPU cores) to build upon the entropy. The standard seed is DateTime.Now + some entropy derived from thread race conditions, it's actually not as bad as described but to be sure I expanded upon it.

realindiahotel commented 9 years ago

I should add that I am adding the additional seed data on top of the standard seed, not replacing. The SecureRandom seed is not quite as bad as it has been made out to be because different systems spit out different entropy for the thread race conditions. All the tests I have seen are using the same system so is not quite a fair test, but we can easily strengthen just to be sure as I have.

NicolasDorier commented 9 years ago

I like the fact that one can add custom entropy for key generation with the SecureRandom. However, given the fact that most platform have their native and secure rng source, I would not dare using only the clock.

It is interesting to see how many bit add the thread race condition. But I don't think it is much. One could derive it quite easily by making a distribution of the results under a large range of phone. Knowing the phone model, I would not think the entropy is higher than 12. Not knowing the phone model, I would say 16. (pure speculation)

How much bit of entropy should be considered safe ?

realindiahotel commented 9 years ago

I believe Bitcoinj (Which MultiBit uses) is using BouncyCastles SecureRandom. Not sure if they are adding extra seed material or using the default tho I believe the code is open source to check? Yes like you I am also not a fan of using the time as it can be easily reproduced, so mostly we are relying on what is derived from the thread race condition (in default implementation). Like you say it is not large entropy, amd this worried me so I added amount of milliseconds system has been running (hardish to guess right) and processor affinity (not so hard to guess but drives up the cost of computation) and I also perform many rounds of SHA256 hash on the seed, like key stretching for passwords because this then makes it "expensive" to try and brute force seeds using previous times etc do you get me? The tradeoff being it takes like 400ms to create a private key but I'll live with that if it keeps peoples BTC safe.

Not being a super duper expert, but I think anything under 96 bits of entropy is bad news if used raw, however, if you have lesser entropy and use "key stretching" so say you have only 14 bits entropy but make it expensive for each guess by forcing lots of SHA passes, you are essentially doing the same thing as high entropy if that makes sense?

NicolasDorier commented 9 years ago

good idea to use a costly key derivation function. This seems secure for me even with lower entropy, I will search about how much is acceptable in such condition. However, I think processor affinity and milliseconds the system has been running is not cross plateform, if so, why not using the native prng.

realindiahotel commented 9 years ago

Yea I think you mean cross platform in the Xamarin sense, I'm unsure as never tried to do Xamarin stuff. It is definitely Windows 8.1 and Windows Phone 8.1 Silverlight cross platform, but yea I could see issues with Xamaeim, for one iOS reports times in Mac Absolute time which is 31 years or something different to Unix Epoch Time. Well the Native prng has to get a seed from somewhere, the problem with Native prng is we don't always know where the seed data is coming from? But someone somewhere knows, and if it's just the time well we know how bad that is! That is why I like SecureRandom so much, we can control the seed. Also SecureRandom is actually very basic. It just uses hashing algorithms. So the seed data is hashed and then when you get random bytes it just hashes the seed again and gives you x bytes. Each time you need more random bytes it just does another hash. Very easy to implement cross platform and very secure and controllable by you unlike native prng which could be corrupt!

NicolasDorier commented 9 years ago

I agree. I will likely make a IRandom (NBitcoin class) implementation using SercureRandom so anyone can add his own entropy. Actually NBitcoin have a cool KDF implementation for making cracking time consuming. (Scrypt)

realindiahotel commented 9 years ago

Reading your book, I see you are suggesting pretty much what I have done, datetime.now + currentthreadID + system runtime combined with KDF, however I also thought of another neat way to get entropy, read the contents of a section of memory! What do you think?

fonix232 commented 9 years ago

@Thashiznets while it could be done. mobile platforms would represent a good part of the use of NBitcoin - and neither WinPhones nor Xamarin allows direct memory access.

For better entropy, why not read pseudorandom-defined data from a file? E.g. get a few pseudorandom numbers, based on that, get a path to a file, and get a pseudorandom number for start position to read (e.g. a 2MB file, get a number from ranges 1 - [2MB - 8kB], and read the data from there.

realindiahotel commented 9 years ago

@fonix232 I think you can create a defined size array but don't put anything in it, and then because it resides in memory allocated to the mobile app you then access the array and you get the random contents of what was in the memory isn't that right? Because arrays are by reference so you are always dealing with memory spots behind the scenes.

As for the files, remember in the mobile environments you are restricted to a sandbox, and possibly the media library so it may be feasible to sha256 a music file filename or something like that!

fonix232 commented 9 years ago

Actually WP8.1 allows direct file access in specific places, but yes, browsing into the music library is just as feasible - if it exists. For example, all my music is on Spotify, which uses its own storage.

realindiahotel commented 9 years ago

@fonix232 yea like documents, downloads, music etc, but as you say it depends on user having content in those areas not really a guarantee. There should always be junk data in memory, I actually found ways to get junk memory data when calling some native/unmanaged code when I was doing Unicode NFKD normalisation stuff so it is definitely doable in WP.

realindiahotel commented 9 years ago

@fonix232 @NicolasDorier oh also I use byte[] guidBytes = Guid.NewGuid().ToByteArray(); I get a GUID and use it as well :)

NicolasDorier commented 9 years ago

As fonix said, reading random value in memory is problematic, since there is no crossplateform way to do it. If there is no crossplateform way, then the solution is to used plateform specific PRNG (eg: SecureRandom of Java, RNGCryptoProvider for windows desktop)

Adding entropy manually would protect against the underlying PRNG bugs and backdoor though. As I talk in the book, I added a RandomUtils.AddEntropy method.

The same remark hold for reading a pseudo random file. (not portable)

The Guid.NewGuid() is interesting but the security depends on the underlying implementation of it. And I doubt it use plateform specific PRNG, which mean that the entropy only have clock time, so a very bad one.

realindiahotel commented 9 years ago

Hi @NicolasDorier like you I was skeptical about the entropy of GUID and how it remained random and so I traced the function back to the underlying Win32 functions and I found that it uses the ethernet or token ring MAC address. I imagine it starts as a hash of that and then keeps hashing for each subsequent GUID. It looks good as a source of entropy to me, of course just like you mention in your book, if attacker has access to machine they can get runtime, same would apply fir GUID but I believe it certainly helps bump up the bits of entropy. I am currently writing a method to return a hash of random memory bytes, I found I can exploit the unmanaged NormalizeString function to return bigger byte array than it should which sees it grabbing whatever is in memory so I get nice random byte array after hashing :)

Of course any of the unmanaged Win32 functions like NormalizeString are platform dependant on Windows so probably no use to your iOS and Android friendly API.

NicolasDorier commented 9 years ago

Very interesting, so if the attacker knows your MAC (which is not private info on internal network), it basically has access to your key. In some case, it might be worse than timestamp.

However, yes, if coupled with a strong prng, it bump up entropy and protects a bit if there is an underlying bug or malware in the PRNG. Are you doing the NormalizeString hack for protecting yourself against the RNGCryptoServiceProvider of windows ? Because it already hash a number of internal state.

The current process ID (GetCurrentProcessID).
The current thread ID (GetCurrentThreadID).
The tick count since boot time (GetTickCount).
The current time (GetLocalTime).
Various high-precision performance counters (QueryPerformanceCounter).
An MD4 hash of the user's environment block, which includes username, computer name, and search path. [...]
High-precision internal CPU counters, such as RDTSC, RDMSR, RDPMC

Source

realindiahotel commented 9 years ago

Yea MAC and possibly they use a timestamp as well to ensure uniqueness? Unsure but yes as with anything constant they can get it if machine compromised. RNGCryptoServiceProvider is unfortunately not common between WinRT and WinPhoneRT at the moment, they are putting crypto stuff into .NET Core at the moment they inform me, but in the mean time I am using BouncyCastles SecureRandom with my own entropy.

NicolasDorier commented 9 years ago

What about CryptographicBuffer ?

realindiahotel commented 9 years ago

Problem is CryptographicBuffer is part of Windows.Security.Cryptography and Windows.Security is WinRT only and not available for me in a PCL Targeting Windows 8.1 (WinRT) and Windows Phone 8.1 (WindowsPhoneRT).

NicolasDorier commented 9 years ago

I think you should use the CryptographicBuffer for WinRT and the RngCryptoServiceProvider for Windows Phone. The way you can do that is by having an "Initialize" method in each plateform specific project that affect RandomUtils.Random with the plateform specific implementation.

Also depending on the app, on the first start, you can ask user to ask some random input by drawing something with the finger for example. Then add the entropy

realindiahotel commented 9 years ago

I don't agree. I think your random should not chop and change between platforms. Given that it is easy to have Hashing algorithm on all platforms, it is very simple to create a common RNG on all platforms and I feel that SHA(your own entropy) is simple, efficient and safe, can be shared across all platforms and you don't have the problem where one RNG may be found insecure on one platform yet fine on another etc. Also you control the RNG and can account for it, as opposed to trusting the platform which is what happened to the Android Wallet if I'm not mistaken.

NicolasDorier commented 9 years ago

Well, you can protect against plateform RNG with your own entropy. (and yes, android one was flawed) Using your own, also mean that if there is a bug in it, then it is still present everywhere.

What I would fear by using a custom RNG is to not have a sufficient entropy, when plateform PRNG give us some assurance about it.

If you want to use your own, then I would advice that at least you use some custom random input from your user. (like moving the mouse randomly during first boot)

realindiahotel commented 9 years ago

But what if the entropy adding mechanism is flawed in the platform specific RNG? What if instead of adding seed material it clobbers it instead? So you think you are adding all this entropy but may in fact not be? You see what I mean? And yes I am handling all the entropy and seed generation in my custom random implementation so there is no way a user can go wrong. I have datetime, currentthreadid, systemruntime, guid, threadraceconditionentropy, processoraffinity all rolled into one AND kdf for certainty. This is cross platform and can be used in any WinRT/WinPhoneRT and can easily be ported to Android/iOS

realindiahotel commented 9 years ago

Oh also remember, .NET crypto stuff still lives in the land of Sha1, so Rfc2898DeriveBytes (The .NET PBKDF2) is still using Sha1, but we have been advised now to use Sha256 and stronger so you may find these platform RNGs a bit lacking!

NicolasDorier commented 9 years ago

I see, yes it is a valid concern, like I said, android prng was flawed not so long time ago, so that's not paranoid. I must say that I would not be confident enough about knowing if the entropy you are using is secure. That's why I like using plateform PRNG which delegate that to microsoft/apple/google... but well... even such can be broken and hacked, so I might be wrong to prefer them instead of your own entropy + BouncyCastle.SecureRandom + KDF

realindiahotel commented 9 years ago

But my entropy creation is in my open source code so you can see for yourself the entropy creation. You can even look at the SecureRandom class in the BouncyCastle source to check that the entropy is being correctly used, this is what I did and I have absolute confidence in my solution which is using Sha256 and finally sha512 in the kdf. Honestly, Sha2 is the best thing since sliced bread!

NicolasDorier commented 9 years ago

It is not the algorithm that I necessarily don't trust. It is more whether the entropy is breakable by bruteforcing. But I guess with the KDF it should be fine.

realindiahotel commented 9 years ago

Indeed, I'm confident that I don't need to use KDF, but I do for good measure.

realindiahotel commented 9 years ago

@NicolasDorier hey mate I forgot to mention to you however feel free to put my BIP39 implementation into NBitcoin if you wish. It would be super easy to use your IRandom instead of what I'm doing if you prefer for the cross platform!

NicolasDorier commented 9 years ago

Thanks ! I was going to ask you if I can do that at one point, but forgot after a while. In fact I had several demands for BIP39. I'll do it.

NicolasDorier commented 9 years ago

@Thashiznets I included BIP39. Your implementation helped a lot, but I did lots of refactoring. (I kept your credit on the Mnemonic class)

However the string normalization is not supported in all plateform, so I did not included it in my portable libraries :( However, it is support on Android.

I've seen your solution to get a portable Normalize, but I suspect it works only on Windows. (so don't work on other plateforms) I should ship a Windows-only portable lib at one point.

One major thing is that I'm fetching the word list on github instead of putting them in the Library, it made my visual studio crashes, and also increased the size of the assembly.

realindiahotel commented 9 years ago

@NicolasDorier nice work! Indeed you are correct, the Normalisation is the reason I removed WP Silverlight support as it is a pinvoked Win32 call and can only be done in the Full .NET or .NET Core project types, the old WP Silverlight don't allow pinvoke so that is why I have ditched the legacy Silverlight and focussing on .NET Core now as it is the future.

Correct, my Normalise solution is reliant on Win32 API so yep you are correct that it is only windows.

I saw you were getting the lists real time, the reason I don't like dynamicly changing wordlists is that if you use the indeces and it points to a changed word, you get totally different derived bytes! I mean the wordlist I wouldn't think should change, but being open source it is a danger!

NicolasDorier commented 9 years ago

I opened another #60 issue. It is impossible that they change that, since it would break existing shipped trezor. Sometimes the list is updated but I think it does not break old mnemonic.

However, I think I will hardcode the lists in the lib like you did... Because I can't port the GithubWordlistSource (rely on HttpClient, which is not in my profiles)

For the normalized form, you just had to do use String.Normalize(NormalizationForm.FormKD) if you only support WIN32.

I intend to generate a substitution table of characters to their normalized forms and just hardcode it in the portable lib. Since it is potentially big, I'll only do it for well used characters, and just throw an exception for others.

realindiahotel commented 9 years ago

Believe it or not, String.Normalize is another of those not available in .NET Core methods, only full .NET at the moment which is why I need to pinvoke the Win32 NormalizeString

NicolasDorier commented 9 years ago

Funny thing that Android support it :D

NicolasDorier commented 9 years ago

@Thashiznets I finished by hard coding the dictionaries, as well as a good deal of KD normalization. Now BIP39 is portable ! :) You can use String.Normalize for plateform that support it by compiling NBitcoin with some pre compilation constant. For those that don't support it, I provided the hard coded KD mapping.

realindiahotel commented 9 years ago

@NicolasDorier Nice! Have you tested your KD mapping against the official test vectors just for safety? Particularly it would be good to do with the Jap tests as the Jap tests catch people out when not normalising properly.

NicolasDorier commented 9 years ago

Yes I did. You can open NBitcoin.Portable.sln, and run the NBitcoin.Portable.Tests. I test both, the portable versions and the full windows version. (the test project is ported)

realindiahotel commented 9 years ago

Ok that's excellent work!

NicolasDorier commented 9 years ago

I'm closing that, I will add new Guid at initialization as entropy data only, and provide specific implementation of IRandom for specific plateform when I'll be able to compile on them.