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?

  • 2
    Obfuscating is not good security practice. The secret should always belong to the key alone. Assume that the mechanism can be compromised (as with the Enigma machines during WWII). Why do you have a password in your configuration file? Can you use the Process-identity instead to access your resource(s)? – Frode May 12 '16 at 8:40
  • Later i want an administrator to be able to change the used certificate. In this process i need to secure the entered password for the certificate to be able to load the private key. So if i take the process identity wouldn't decryption fail when i close the app and start it again, cause it gets another PID? – BoeseB May 12 '16 at 8:59

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.

  • Thanks for your feedback. The purpose of creating a char array instead of creating a string like you suggested is, that i can clear the char array from memory. I read, that clearing a string is a problem cause you don't know when it will be cleaned from memory (I don't remember the source). I agree with the certificate part. In your last comment you are right, this is a bug i didn't think about. – BoeseB Feb 13 '17 at 10:43
  • Here some statement from How to: Use strings in a secure manner with SecureString class: "System.String can’t be disposed on demand. No one can predict when a specific string will be reclaimed by the Garbage Collector (GC). It is possible that some strings to survive long time after the application was closed." Thats why i used the char array – BoeseB Feb 13 '17 at 10:54
  • @BoeseB, here is the thing with your statement, the Secure String is being used either way, I am not creating another String, so your logic in using the Char array instead of just going straight into the SecureString is flawed. The SecureString class makes use of the IDisposable Method and can be disposed on demand, I am not using a regular string anywhere in the code I presented, and once scope leaves the using block, the secure string is removed from memory, it is immediately "Reclaimed by the Garbage Collector" – Malachi Feb 13 '17 at 13:51
  • Doesn't Encoding.Unicode.GetString(decryptedData) in your code create a regular string, which the statement applies to? I have no control over this string instace. Another thing i just noticed, you can't assign a string to SecureString directly like you suggest in the following line s = Encoding.Unicode.GetString(decryptedData); – BoeseB Feb 13 '17 at 14:34
  • you are already passing the password to a string as well, so you have lost that amount of security already. you could always use section.Password instead of assigning it to a String. I see your dilemma with the char[] though – Malachi Feb 13 '17 at 16:35

Your Answer

 

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.