BouncyCastle Diffie Hellman

Question

I want to write a complete diffie Hellman example for bouncy castle that includes key generation, key exchange, encryption, and decryption. I also want to verify that if Alice is initiating a connection to Bob, that she should send her public key, Parameter P, and Parameter G.

This is also a good reference.

Namespaces:

using System;
using System.Collections.Generic;
using System.IO;
using System.Text;

using Org.BouncyCastle.Asn1;
using Org.BouncyCastle.Asn1.X9;
using Org.BouncyCastle.Crypto;
using Org.BouncyCastle.Crypto.Engines;
using Org.BouncyCastle.Crypto.Agreement;
using Org.BouncyCastle.Crypto.Agreement.Kdf;
using Org.BouncyCastle.Crypto.Digests;
using Org.BouncyCastle.Crypto.Modes;
using Org.BouncyCastle.Crypto.Generators;
using Org.BouncyCastle.Crypto.Parameters;
using Org.BouncyCastle.Utilities;
using Org.BouncyCastle.Security;
using Org.BouncyCastle.Math;
using Org.BouncyCastle.Asn1.X509;
using System.Diagnostics;
using Org.BouncyCastle.OpenSsl;

Some constants for this test case:

const string Algorithm = "ECDH"; //What do you think about the other algorithms?
const int KeyBitSize = 256;
const int NonceBitSize = 128;
const int MacBitSize = 128;
const int DefaultPrimeProbability = 30;

Main method that starts the pairing and handles verifying encryption:

public static void TestMethod() {
    //BEGIN SETUP ALICE
    IAsymmetricCipherKeyPairGenerator aliceKeyGen = GeneratorUtilities.GetKeyPairGenerator (Algorithm);
    DHParametersGenerator aliceGenerator = new DHParametersGenerator ();
    aliceGenerator.Init (KeyBitSize, DefaultPrimeProbability, new SecureRandom ());
    DHParameters aliceParameters = aliceGenerator.GenerateParameters ();

    KeyGenerationParameters aliceKGP = new DHKeyGenerationParameters (new SecureRandom (), aliceParameters);
    aliceKeyGen.Init (aliceKGP);

    AsymmetricCipherKeyPair aliceKeyPair = aliceKeyGen.GenerateKeyPair ();
    IBasicAgreement aliceKeyAgree = AgreementUtilities.GetBasicAgreement (Algorithm);
    aliceKeyAgree.Init (aliceKeyPair.Private);

    //END SETUP ALICE

    /////AT THIS POINT, Alice's Public Key, Alice's Parameter P and Alice's Parameter G are sent unsecure to BOB

    //BEGIN SETUP BOB
    IAsymmetricCipherKeyPairGenerator bobKeyGen = GeneratorUtilities.GetKeyPairGenerator (Algorithm);
    DHParameters bobParameters = new DHParameters( aliceParameters.P, aliceParameters.G );

    KeyGenerationParameters bobKGP = new DHKeyGenerationParameters (new SecureRandom (), bobParameters);
    bobKeyGen.Init (bobKGP);

    AsymmetricCipherKeyPair bobKeyPair = bobKeyGen.GenerateKeyPair ();
    IBasicAgreement bobKeyAgree = AgreementUtilities.GetBasicAgreement (Algorithm);
    bobKeyAgree.Init (bobKeyPair.Private);
    //END SETUP BOB

    BigInteger aliceAgree = aliceKeyAgree.CalculateAgreement (bobKeyPair.Public);
    BigInteger bobAgree = bobKeyAgree.CalculateAgreement (aliceKeyPair.Public);

    if (!aliceAgree.Equals (bobAgree)) {
        throw new Exception ("Keys do not match.");
    }

    byte[] nonSecretMessage = GetBytes ("HeaderMessageForASDF");
    byte[] secretMessage = GetBytes ("Secret message contents");
    byte[] decNonSecretBytes;

    KeyParameter sharedKey = new KeyParameter (aliceAgree.ToByteArrayUnsigned ());

    var encMessage = EncryptMessage( sharedKey, nonSecretMessage, secretMessage );
    var decMessage = DecryptMessage( sharedKey, encMessage, out decNonSecretBytes );

    var decNonSecretMessage = GetString( decNonSecretBytes );
    var decSecretMessage = GetString( decMessage );

    Debug.WriteLine( decNonSecretMessage + " - " + decSecretMessage );

    return;
}

Wrapper method to Encrypt a message with given data:

public static byte[] EncryptMessage (string sharedKey, string nonSecretMessage, string secretMessage)
{
    return EncryptMessage( new KeyParameter( Convert.FromBase64String( sharedKey ) ), GetBytes( nonSecretMessage ), GetBytes( secretMessage ) );
}

Helper method to Encrypt a message with given data

public static byte[] EncryptMessage( KeyParameter sharedKey, byte[] nonSecretMessage, byte[] secretMessage ) {
    if( nonSecretMessage != null && nonSecretMessage.Length > 255 ) throw new Exception( "Non Secret Message Too Long!" );
    byte nonSecretLength = nonSecretMessage == null ? (byte)0 : (byte)nonSecretMessage.Length;

    var nonce = new byte[NonceBitSize / 8];
    var rand = new SecureRandom();
    rand.NextBytes(nonce, 0, nonce.Length);

    var cipher = new GcmBlockCipher(new AesFastEngine());
    var aeadParameters = new AeadParameters(sharedKey, MacBitSize, nonce, nonSecretMessage );
    cipher.Init(true, aeadParameters);

    //Generate Cipher Text With Auth Tag
    var cipherText = new byte[cipher.GetOutputSize(secretMessage.Length)];
    var len = cipher.ProcessBytes(secretMessage, 0, secretMessage.Length, cipherText, 0);
    cipher.DoFinal(cipherText, len);

    using (var combinedStream = new MemoryStream())
    {
        using (var binaryWriter = new BinaryWriter(combinedStream))
        {
            //Prepend Authenticated Payload
            binaryWriter.Write(nonSecretLength);
            binaryWriter.Write(nonSecretMessage);

            //Prepend Nonce
            binaryWriter.Write(nonce);
            //Write Cipher Text
            binaryWriter.Write(cipherText);
        }
        return combinedStream.ToArray();
    }
}        

Wrapper method to Decrypt a message

public static string DecryptMessage (string sharedKey, byte[] encryptedMessage, out string nonSecretPayload)
{
    byte[] nonSecretPayloadBytes;
    byte[] payload = DecryptMessage( new KeyParameter( Convert.FromBase64String( sharedKey ) ), encryptedMessage, out nonSecretPayloadBytes );

    nonSecretPayload = GetString( nonSecretPayloadBytes );
    return GetString( payload );
}

Helper method to decrypt a message

public static byte[] DecryptMessage( KeyParameter sharedKey, byte[] encryptedMessage, out byte[] nonSecretPayloadBytes )
{
    using (var cipherStream = new MemoryStream(encryptedMessage))
    using (var cipherReader = new BinaryReader(cipherStream))
    {
        //Grab Payload
        int nonSecretLength = (int)cipherReader.ReadByte();
        nonSecretPayloadBytes = cipherReader.ReadBytes(nonSecretLength);

        //Grab Nonce
        var nonce = cipherReader.ReadBytes(NonceBitSize / 8);

        var cipher = new GcmBlockCipher(new AesFastEngine());
        var parameters = new AeadParameters(sharedKey, MacBitSize, nonce, nonSecretPayloadBytes);
        cipher.Init(false, parameters);

        //Decrypt Cipher Text
        var cipherText = cipherReader.ReadBytes(encryptedMessage.Length - nonSecretLength - nonce.Length);
        var plainText = new byte[cipher.GetOutputSize(cipherText.Length)];  

        try
        {
            var len = cipher.ProcessBytes(cipherText, 0, cipherText.Length, plainText, 0);
            cipher.DoFinal(plainText, len);
        }
        catch (InvalidCipherTextException)
        {
            //Return null if it doesn't authenticate
            return null;
        }

        return plainText;
    }
}

Returns byte data for a given string:

static byte[] GetBytes(string str)
{
    if( str == null ) return null;
    return System.Text.Encoding.Unicode.GetBytes( str );
}

Returns a string for given byte data:

static string GetString(byte[] bytes)
{
    if( bytes == null ) return null;
    return System.Text.Encoding.Unicode.GetString( bytes, 0, bytes.Length );
}

Answer 1

public static byte[] EncryptMessage( KeyParameter sharedKey, byte[] nonSecretMessage, byte[] secretMessage ) {  

the spaces after and before the opening and closing () are looking strange and you aren't consistent with that style.

if( nonSecretMessage != null && nonSecretMessage.Length > 255 ) throw new Exception( "Non Secret Message Too Long!" );  

not using braces {} although they might be optional can lead to error prone code.

byte nonSecretLength = nonSecretMessage == null ? (byte)0 : (byte)nonSecretMessage.Length;

I don't really like having first a if condition checking a variable and then having a ternary which is checking the same variable again. Sometimes the good ole if..else is doing just fine.

//Generate Cipher Text With Auth Tag

instead of having this comment, why don't you just extract these lines to a method GenerateCipherTextWithAuthTag(GcmBlockCipher, byte[]) ?

var len = cipher.ProcessBytes(secretMessage, 0, secretMessage.Length, cipherText, 0);

I consider this as misusing the var type. It is not obvious what the right hand side of the assignment returns.

 using (var combinedStream = new MemoryStream())
   {
       using (var binaryWriter = new BinaryWriter(combinedStream))
       {
           //Prepend Authenticated Payload
           binaryWriter.Write(nonSecretLength);
           binaryWriter.Write(nonSecretMessage);

          //Prepend Nonce
           binaryWriter.Write(nonce);
           //Write Cipher Text
           binaryWriter.Write(cipherText);
       }
       return combinedStream.ToArray();
   }

Here you should stack the using blocks in the same way you did in the DecryptMessage() method which saves you some horizontal spacing.

By using ToArray() with a MemoryStream you are creating a new array which can be avoided if you use the GetBuffer() method.

---

DecryptMessage

try
{
    var len = cipher.ProcessBytes(cipherText, 0, cipherText.Length, plainText, 0);
    cipher.DoFinal(plainText, len);
}
catch (InvalidCipherTextException)
{
    //Return null if it doesn't authenticate
    return null;
}

return plainText;  

You only return null for an InvalidCipherTextException but you let any other exception bubble up the call tree. That can be ok, but should be explained by a comment why this is done. Comments which are telling what is done are only noise to the code and should be removed.

---

GetBytes() and GetString()

You should use the private access modifiers for this methods to make it more clear. Having public modifiers but missing private makes it harder to grasp the code at first glance.

Both of this methods already exists and are called in this methods if the passed in arguments != null. This seems to be methods which shouldn't throw an exception for a given invalid input.

I would like to suggest to use the TryGetXXX() pattern to make the intent of these methods more clear.

private static bool TryGetBytes(string str, out byte[] result)
{
    if(str == null) 
    { 
        result = null;
        return false; 
    }

    result = System.Text.Encoding.Unicode.GetBytes(str);
    return true;
}

Answer 2

This is only a small review, as Heslacher has covered most of the points I wanted to make, but I take issue with your use of var.

As Heslacher puts it, you're using var incorrectly here:

var len = cipher.ProcessBytes(secretMessage, 0, secretMessage.Length, cipherText, 0);

Because

It is not obvious what the right hand side of the assignment returns.

However you're also omitting useful uses of var here:

int nonSecretLength = (int)cipherReader.ReadByte();

And also here:

DHParametersGenerator aliceGenerator = new DHParametersGenerator ();

And there are a number of other cases as well.

The key rule to using var is Use var when the right-hand side of the variable declaration makes its type obvious.

Answer 3

I always encourage a cleanup of the usings at the top of the page, so when I look at the code I know roughly what Im going to be getting into.