jstedfast / MimeKit

A .NET MIME creation and parser library with support for S/MIME, PGP, DKIM, TNEF and Unix mbox spools.
http://www.mimekit.net
MIT License
1.83k stars 371 forks source link

Question about PGP convenience methods #404

Closed polterguy closed 6 years ago

polterguy commented 6 years ago

I have (finally) set aside time to refactor my usage of MimeKit in Phosphorus Five, which was long overdue may I add, and I started looking at some of your Open PGP convenience methods - And all I've got to say is wow! It's almost an entirely new library. Gr8 work! :)

However, I have some questions. First of all, obviously I want to use as many of your convenience methods as possible. I do however have this "super paranoid" implementation of creation of PGP keys, where I allow for manually salting the SecureRandom thing, in addition to providing other overrides, such as expiration date, bit strength, etc. How much work have you put into this method? Obviously, the nightmare scenario would be some adversary being able to predict the RNG ...?

Hence, I am reluctant to using this specific method, without knowing what it actually does, and how secure it is ...

Another method I saw, where my jaw simply dropped, was the "AutoImportFromKeyServer" thing. Wow!! I've been fiddling with this myself, and I realise just how much work doing that is. How do I use it though ...?

Do you have some sample code laying around for providing a key server, etc ...?

It really feels almost like an entirely new library though, as I started browsing through your methods. Thx a billion times Jeffrey :)

jstedfast commented 6 years ago

For key generation, I didn't seed either of the 2 places where I use the SecureRandom class:

https://github.com/jstedfast/MimeKit/blob/master/MimeKit/Cryptography/OpenPgpContext.cs#L1040 and https://github.com/jstedfast/MimeKit/blob/master/MimeKit/Cryptography/OpenPgpContext.cs#L1070

but I could... actually, I should probably re-use the same SecureRandom context for both places.

Ideally, I'd say that BC's default SecureRandom should be good enough, but this reminded me of an email by Edward Ned Harvey on the BouncyCastle mailing list that I read a while back (posted here since it's easier than linking to the mail thread):

The default constructor of SecureRandom uses GetSeed(8), which is only 64 bits, but worse yet, GetSeed seeds itself from the system clock Ticks.

There are 10million ticks per second, and a single laptop CPU core can perform between 10million and 200million AES operations per second, depending on hardware acceleration, clock speed, and how efficient the software library is. This means, suppose you used SecureRandom() to generate an AES key, a completely unsophisticated brute force attacker could guess your key in a period of time that's less than the precision he has on guessing when you created your key. If he knows when you generated it +/- 12 hours, then he would expect to require something between 0.5 and 12 hours to guess your key, divided by the number of CPU cores he has available.

Even if an attacker had no idea when you generated some key, it's still limited to 64 bits, which is just nowhere near good enough.

Even if you're aware of this, and you painstakingly and consistently use things like ThreadedSeedGenerator to seed the SecureRandom, there are tons of places where SecureRandom() is used internally by Bouncy Castle. By searching code, I see 526 references. I discovered this today, because I painstakingly created strong random to seed my ECKeyPairGenerator, and then signed & verified data, only to discover that the insecure SecureRandom() is used internally by ECDSASigner.

So the questions I have are ... What to do about it.

Option 1: More education to the users. Response: I see code examples all over the internet, where people advise each other to use SecureRandom and expect it to be secure, as the name implies. So the idea of just educating people on proper usage and expectations is unrealistic. The name "SecureRandom" in a widely trusted crypto library such as Bouncy Castle is important and meaningful.

More importantly, SecureRandom() is used internally by Bouncy Castle. Which underscores the futility of "user education" as the solution. As described above, I painstakingly ensured I used good random, only to find it undermined internally by ECDSASigner.

Option 2: Instead of seeding SecureRandom from system time, seed it from ThreadedSeedGenerator, or better yet, a combination of the two, or better yet, pull from both of those sources plus the OS crypto API and everything else available to provide seed material.

In fact, that's what we're doing in TinHat Random. http://tinhatrandom.org If the solution is to seed SecureRandom from a bunch of different sources, then I would like to see TinHat Random folded into the source of Bouncy Castle. Which might be easier said than done, given the Java / C# duality.

Also, there might be some logistical difficulties, compatibility with different system types, different OSes with different threading models, etc. I would be brazen to assume there always exists an OS crypto API on every platform where BC gets used. Not to mention, the assumption that ThreadedSeedGenerator is valid to use. Which is another big assumption.

Option 3: I am rather expecting to hear a lot of "can't" in response to Option 2. But I am working for an employer who is planning to use BC for secure communications, and I cannot afford to deliver "can't" as my answer. So I am estimating probably my response will be forced, to fork BC, and fold TinHat Random into it, to make it suitable for my employer's purposes. And I'll have to go around promoting my fork to application developers, etc. But I'm hoping for a more graceful integration process somehow that doesn't result in fragmentation of community or development efforts.

Other options? Ideas? Comments?

Thanks... PS, I love BC and as always, thanks to everyone who contributed and contributes to it.

The website now seems to be a blank page, but the github repo for tinhatrandom is https://github.com/Vanaheimr/tinhat/tree/master/tinhat

Looking at more recent code, it seems to seed itself using a counter value and 32 bytes of data from RNGCryptoServiceProvider that then gets hashed using SHA256.

I honestly don't know if that is secure enough, though. If not, perhaps I can add an optional SecureRandom argument to the current CreateKeyPair() method.

As far as the AutoKeyRetrieve thing, you just need to set the URL of a KeyServer on the OpenPgpContext. By default, the AutoKeyRetrieve and KeyServer options are populated using the user's gpg.conf file.

For example, I have:

keyserver hkp://keys.gnupg.net

In code, it would just be something like:

ctx.KeyServer = new Uri ("hkp://keys.gnupg.net");
ctx.AutoKeyRetrieve = true;
jstedfast commented 6 years ago

Oh, one minor flaw in the AutoKeyRetrieve is that GnuPG supports some new key algorithms that BouncyCastle does not (yet?) support such as some elliptic curve algorithms. So the auto-key-retrieve logic is unable to import those because the pgp parser in BC throws an exception (my code catches it, but it obviously will fail to import).

polterguy commented 6 years ago

Regarding suggestions about how to seed, you might want to have a look at my code. I've just cleaned it up significantly, and among other things I am using ThreaderSeedGenerator, but also alotof other sources ...

https://github.com/polterguy/phosphorusfive/blob/master/plugins/extras/p5.crypto/CSRNG.cs#L163

And here ...

https://github.com/polterguy/phosphorusfive/blob/master/plugins/extras/p5.crypto/CSRNG.cs#L174

(Same file)

The idea, is that every places I need a SecureRandom I invoke the Active Event called ".p5.crypto.rng.secure-random.get", and then internally I store it as a static field. According to StackOverflow.com, some guy had looked at its source code, and claimed it was thread safe ...

The way I seed it, is by (among other things) allow the user to manually type a seed during installation of the server (which obviously is not something you could do in your code). Then when I instantiate a SecureRandom, I use this seed. In addition, ever time you retrieve a SecureRandom, I also allow the caller to (optionally) supply a seed argument. According to info I've read about it, the seeding of the SecureRandom adds to its existing entropy, and does not replace the existing seed. However, to make sure I guard against potentially bad usage of the seed internally, I take my entire seed, which is generated from all sorts of "weird stuff" (including a random GUID, CPU ticks, manually types seed, argument seed, etc, etc, etc) - And I hash it using SHA512.

So even if the usage of the seed internally in BC is not "optimal", and parts of the seed is "compromised", allowing an adversary to know say the first 70% of your seed, and such "predict" its result - This has little effect, since every time I seed the SecureRandom, I always has the entire seed with SHA512.

A lot of these things are probably not even possible to implement from your point of view, but at least maybe allow the caller to supply his own SecureRandom instance, with a default value of null, and if none is given, simply create your own SecureRandom - At which at least for me the case would be solved ...

The only place I currently use SecureRandom anyway, is when I create PGP keys. However, it might be that BC is internally using SecureRandom as it encrypts its PGP entities. Which might be an issue, since as far as I know, only the "session key" is actually asynchronously encrypted with RSA, and the "session key" is actually an AES password or something. Hence, if the SecureRandom is not giving unpredictable results, even if an adversary couldn't hack the actual RSA parts, he could possibly "predict" the session key (synchronous password), and such hack the content of the message, without hacking the PGP key ...

Sorry for bringing you into the "paranoia" forest here a little bit, but I'm going through my own code, looking for all sorts of potential holes, and since I am relying upon your code, well ... :)

About your KeyServer thing - Pure 100% beauty!! :D