Secure way to store and load password in app config

Question

I am trying to figure out a good way to store and load a password in the application configuration of my C# application. To achieve this I use the BouncyCastle library and the DPAPI from Windows.

To protect the password from getting decrypted by other applications just using DPAPI under the same account it got encrypted I generate entropy from a salt I generate with BouncyCastle (this is stored in the registry) and a secret that is embedded in my application. To protect the secret I thought about obfuscating the code.

In the code below first the raw password is loaded from the configuration then encrypted and wrote back to the configuration as base64 string.

 private const string SECRET = "D9E789B1-0151-4DBE-91D4-361633A5C64C";
 private const string REGKEY = "Software\\myCompany\\myApplication";
 private const string SALT_REGKEY = "data";
 private const string CERT_SECTION = "CertificateConfiguration";
 private const int SALT_LENGTH = 256;

 public static void EncryptPassword()
 {
     //Load Config
     var config = ConfigurationManager.OpenExeConfiguration(ConfigurationUserLevel.None);
     var section = (CertificateConfiguration)config.GetSection(CERT_SECTION);

     if (section != null
         && !section.SectionInformation.IsProtected
         && !section.SectionInformation.IsLocked
         && !section.IsEncrypted)
    {
        byte[] shared = Encoding.Unicode.GetBytes(SECRET);
        byte[] salt = GenerateSalt();
        SaveSalt(salt);
        var entropy = salt.Concat(shared).ToArray();

        byte[] encryptedData = ProtectedData.Protect(
                Encoding.Unicode.GetBytes(section.Password),
                entropy,
                DataProtectionScope.CurrentUser);
        section.Password = Convert.ToBase64String(encryptedData);
        section.IsEncrypted = true;

        //Save Config
        section.SectionInformation.ForceSave = true;
        config.Save(ConfigurationSaveMode.Full);
    }
} 

private static byte[] GenerateSalt()
{
    SecureRandom secureRandom = new SecureRandom(); // Do *not* seed secureRandom! Automatically seeded from system entropy.    
    CipherKeyGenerator keyGen = new CipherKeyGenerator();
    keyGen.Init(new KeyGenerationParameters(secureRandom, SALT_LENGTH));
    return keyGen.GenerateKey(); //AES key
}

private static void SaveSalt(byte[] salt)
{
    var key = Registry.CurrentUser.CreateSubKey(REGKEY, RegistryKeyPermissionCheck.ReadWriteSubTree);
    if (key != null)
    {
        key.SetValue(SALT_REGKEY, salt, RegistryValueKind.Binary);
        key.Close();
    }
}  

In the second part the encrypted password is loaded from the configuration and used to load a certificate.

private static X509Certificate2 LoadCertificate()
{
    try
    {
        var config = ConfigurationManager.OpenExeConfiguration(ConfigurationUserLevel.None);
         var section = (CertificateConfiguration)config.GetSection(CERT_SECTION);

         if (section == null)
           return null;

         string password = section.Password;

         X509Certificate2 certificate;
         using (SecureString s = new SecureString())
         {
             char[] chars = new char[0];
             if (section.IsEncrypted)
             {
                 var registryEntry = Registry.CurrentUser.OpenSubKey(REGKEY);
                 if (registryEntry != null)
                 {
                      byte[] salt = (byte[])registryEntry.GetValue(SALT_REGKEY);
                      byte[] shared = Encoding.Unicode.GetBytes(SECRET);
                      var entropy = salt.Concat(shared).ToArray();

                      byte[] decryptedData = ProtectedData.Unprotect(
                            Convert.FromBase64String(password),
                            entropy,
                            DataProtectionScope.CurrentUser);
                      chars = Encoding.Unicode.GetChars(decryptedData);
                      Array.Clear(decryptedData, 0, decryptedData.Length);
                      // ReSharper disable once RedundantAssignment
                      decryptedData = null;
                 }
             }
             else
             {
                  chars = password.ToCharArray();
             }

             foreach (char c in chars)
             {
                 s.AppendChar(c);
             }
             s.MakeReadOnly();

             Array.Clear(chars, 0, chars.Length);
             // ReSharper disable once RedundantAssignment
             chars = null;
             GC.Collect();
             certificate = new X509Certificate2(section.CertificateLocation, s);
         }
   }
   catch
   { 
        return null; 
   }
}

What problems you see with this approach?

Are there any problems which will leave the clear password in memory longer than needed in the function that decrypts the password?

Are there any better ways to generate a salt?

Should the Secret be calculated in any form so the process gets obfuscated?

Answer 1

In LoadCertificate you create a SecureString inside the using statement, but then you create a char[] array to act as a "middle man". I think that you could change some things around so that you can get rid of the insecure char[] array, I mean that is the purpose of creating the SecureString, right?

First I would get rid of the Char array variable, so make this work you have to change the Encoding.Unicode.GetChars(decryptedData) call and change it to

s = Encoding.Unicode.GetString(decryptedData);

The GetString method

decodes all the bytes in the specified byte array into a string.

MSDN Encoding.GetString Method (Byte[])

Then you can get rid of the foreach

 foreach (char c in chars)
 {
     s.AppendChar(c);
 }

---

EDIT:

or you could use the following to append the chars without creating a new char array

foreach (char c in Encoding.Unicode.GetChars(decryptedData))
{
    s.AppendChar(c);
}

(please see edit below)

---

as well as the code that clears out the array (that you no longer need)

 Array.Clear(chars, 0, chars.Length);
 // ReSharper disable once RedundantAssignment
 chars = null;

also change the else statement so that the password goes straight into the SecureString

I cannot see a reason that you would need to create the X509Certificate2 object before you actually assign something to it, so I would move that all to one line as well.

After making these changes, you would end up with

private static X509Certificate2 LoadCertificate()
{
    try
    {
        var config = ConfigurationManager.OpenExeConfiguration(ConfigurationUserLevel.None);
        var section = (CertificateConfiguration)config.GetSection(CERT_SECTION);

        if (section == null)
            return null;

        string password = section.Password;

        using (SecureString s = new SecureString())
        {
            if (section.IsEncrypted)
            {
                var registryEntry = Registry.CurrentUser.OpenSubKey(REGKEY);
                if (registryEntry != null)
                {
                    byte[] salt = (byte[])registryEntry.GetValue(SALT_REGKEY);
                    byte[] shared = Encoding.Unicode.GetBytes(SECRET);
                    var entropy = salt.Concat(shared).ToArray();

                    byte[] decryptedData = ProtectedData.Unprotect(
                            Convert.FromBase64String(password),
                            entropy,
                            DataProtectionScope.CurrentUser);
                    s = Encoding.Unicode.GetString(decryptedData);
                    Array.Clear(decryptedData, 0, decryptedData.Length);
                    // ReSharper disable once RedundantAssignment
                    decryptedData = null;
                }
            }
            else
            {
                s = password;
            }

            s.MakeReadOnly();

            GC.Collect();
            var certificate = new X509Certificate2(section.CertificateLocation, s);
        }
    }
    catch
    { 
        return null; 
    }
}

EDIT:

because s = Encoding.Unicode.GetString(decryptedData); creates a string that is subject to

the memory persistence consequences of the immutable String class

we could instead use the SecureString's AppendChar(char) method to add them, which may be the way it was intended to be done in the first place.

foreach (char c in Encoding.Unicode.GetChars(decryptedData))
{
    s.AppendChar(c);
}

In my opinion, I would want to move the information to be secured into the SecureString as soon as possible.

The Original code inside the using block may be preferable, using the char[] array may be the best way to go. but I wouldn't make it too complicated if the section isn't encrypted to begin with, I would probably just put the section.Password directly into the new X509Certificate2 call, call the GC and dump out of the method

Another thing that I noticed was that the Password was moved into a string when it wasn't necessary to create another string. Instead we should use the section object the way an object was meant to be used and reference the attribute, like this:

byte[] decryptedData = ProtectedData.Unprotect(
   Convert.FromBase64String(section.Password),
   entropy,
   DataProtectionScope.CurrentUser);

by removing steps that perform manipulation of the sensitive data we make the code more secure, ie less places for accidents to happen.

---

one more thing that I noticed is that you don't have an else statement for if the registryEntry is null, and this puzzles me because now the SecureString (or the char[] that you had) will be empty when creating a new certificate.