HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Simplified secure encryption of a String

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
simplifiedsecureencryptionstring

Problem

I have two code examples that I wrote for best practices encrypting a string that I'm looking for feedback both in terms of best practices and security. They are both using authenticated encryption. One is using the bouncy castle API to do AES-GCMand the other is for the limitation of only using the built in .NET API so it does encryption then authentication separately.

Each example has the ideal API of passing the key as a byte array (because it was randomly generated).

However each has a less secure helper method that uses a string password to derive the bytes for the key using PBKDF2 with salt and iterations. The salt is passed in the clear using the authenticated payload.

Bouncy Castle AES-GCM

```
/*
* This work (Modern Encryption of a String C#, by James Tuley),
* identified by James Tuley, is free of known copyright restrictions.
* https://gist.github.com/4336842
* http://creativecommons.org/publicdomain/mark/1.0/
*/

using System;
using System.IO;
using System.Text;
using Org.BouncyCastle.Crypto;
using Org.BouncyCastle.Crypto.Engines;
using Org.BouncyCastle.Crypto.Generators;
using Org.BouncyCastle.Crypto.Modes;
using Org.BouncyCastle.Crypto.Parameters;
using Org.BouncyCastle.Security;
namespace Encryption
{

public static class AESGCM
{
private static readonly SecureRandom Random = new SecureRandom();

//Preconfigured Encryption Parameters
public static readonly int NonceBitSize = 128;
public static readonly int MacBitSize = 128;
public static readonly int KeyBitSize = 256;

//Preconfigured Password Key Derivation Parameters
public static readonly int SaltBitSize = 128;
public static readonly int Iterations = 10000;
public static readonly int MinPasswordLength = 12;

///
/// Helper that generates a random new key on each call.
///
///
public static byte[] NewKey()
{
var key = new byte[KeyBitSize / 8];
Random.NextBytes(key);
return key;
}

///
/// S

Solution

As far as best practices go, I see a few offhand:

  • static public XXX is idiomatically written as public static XXX.



  • Since all your methods are static, the class should be made static as well to prevent (useless) instances from being created.



  • Are your bit size numbers near the top of the classes intended to be modified? If so, make them private and have public properties to access them. If not, make them const.



One more thing I just noticed: Rfc2898DeriveBytes inherits from a class which implements IDisposable in .NET 4 and beyond. So its use should be wrapped in a using block if you're targeting that version of the framework. .NET 2 (and 3.0/3.5), don't worry about it.

Context

StackExchange Code Review Q#14892, answer score: 7

Revisions (0)

No revisions yet.