BouncyCastle Diffie Hellman

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 (Algor

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;
}

StackExchange Code Review Q#110952, answer score: 2