Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

The application needs to keep a secret which is known to logged in windows user. User enters secret in windows form's text box which is read by application. This is in form of System.String. Now I have to persist this secret into a XML file. And later able to decipher it back and use it.

Questions:

  1. Do you see any concern with encoding done below?
  2. Is there any concern with using System.String to begin with? I was considering to use System.Security.SecureString but at some point down the line the application needs to have base underlying string in clear text.

I'm using System.Security.Cryptography.ProtectedData to do the encryption.

The code

using System.Security.Cryptography;

static public string Encrypt(string password, string salt)
{
    byte[] passwordBytes = Encoding.Unicode.GetBytes(password);
    byte[] saltBytes = Encoding.Unicode.GetBytes(salt);

    byte[] cipherBytes = ProtectedData.Protect(passwordBytes, saltBytes, DataProtectionScope.CurrentUser);

    return Convert.ToBase64String(cipherBytes);
}

static public string Decrypt(string cipher, string salt)
{
    byte[] cipherBytes = Convert.FromBase64String(cipher);
    byte[] saltBytes = Encoding.Unicode.GetBytes(salt);

    byte[] passwordBytes = ProtectedData.Unprotect(cipherBytes, saltBytes, DataProtectionScope.CurrentUser);

    return Encoding.Unicode.GetString(passwordBytes);
}

Typical usage

[TestMethod()]
public void EncryptDecryptTest()
{
    string password = "Gussme!";
    string salt = new Random().Next().ToString();

    string cipher = Authenticator.Encrypt(password, salt);
    Assert.IsFalse(cipher.Contains(password), "Unable to encrypt");
    Assert.IsFalse(cipher.Contains(salt), "Unable to encrypt");

    string decipher = Authenticator.Decrypt(cipher, salt);
    Assert.AreEqual(password, decipher);
}
share|improve this question
    
Just a small point, in cryptography unencrypted data is referred to as clear text and encrypted data is referred to as cipher text. –  Trevor Pilley Oct 12 '12 at 17:21
add comment

1 Answer

  1. Your code is correct. I usually use Encoding.UTF8 because it is more compact for typical character sets.
  2. SecureString is kind of a useless class. It is meant to never keep a secret unencrypted even in memory. This would only help if an attacker could read your processes memory. If he can do that you have lost already because he can read every character of your password the moment you use it or add it to the SecureString. Anyway, what attack scenario on earth allows an attacker to read your webserver process memory and not do other more dangerous things at the same time? In that case your box is probably compromised entirely. So you are right not using this class.
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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