1

I'm trying to learn some good practice for handling passwords. I will supply some code snippets from my project and explain what I'm worried and curious about.

Lets start with the code that gets the user input, my button event code:

string username = txtUser.Text;
string password = Hash.EncryptString(txtPass.Text);

Here my idea is that, storing a password in a string in clear text is probably bad practice? I'm aware this is probably not the solution to that (especially since I'm sending it in clear text to another method which then stores it in a string anyway), but here I'm calling a method I've created to make the password into a hash. The EncryptString method in the "Hash" class:

   public static string EncryptString(string text) {
        var sha1 = System.Security.Cryptography.SHA1.Create();
        var inputBytes = Encoding.ASCII.GetBytes(text);
        text = ""; //clear string
        var hash = sha1.ComputeHash(inputBytes);

        var sb = new StringBuilder();
        for (var i = 0; i < hash.Length; i++)
            sb.Append(hash[i].ToString("X2"));

        return sb.ToString();
    }

So not much to say here, I make a hash of the password with SHA1 encryption. I thought it would be smart to clear the string after I used it so that the password isn't stored anymore?

Later in the code where I'm authenticating or adding the user, I'm getting or creating a unique salt and mixing it with the hashed password and using the EncryptString method again, before comitting to the DB.

In the name of privacy and security, is this good practice? Or rather, what vulnerabilities are in my code at the moment and how can I fix them?

heap1
  • 151
  • 11
  • 1
    minor note: that is a badly named method - it is *named* like it is doing the **wrong** thing (encrypting the password), when actually it is doing *sort of* the right thing (hashing the password). Salt would go a long way, of course, and there are lots of sha1 decryption tables etc available. – Marc Gravell Apr 30 '17 at 20:33
  • In addition to @MarcGravell's comment, you really should be using pbkdf2, bcrypt, scrypt, or argon2 to hash passwords. Of the four, my personal recommendation is bcrypt. – TheGreatContini Apr 30 '17 at 21:47

2 Answers2

1

What scenario are you trying to protect against? The main scenario you are at threat from here is a memory dump containing the string password, or other memory analysis debugging tools. Now, that may be a legitimate threat, or it may not, depending on a lot more context. But if txtPass.Text is a client-side control, then frankly by the time the memory tools come into play, you're more at risk of a key-logger simply grabbing the input for every app as it is entered.

Note that:

text = ""; //clear string

does not remove the string from the unmanaged heap. It just changes the reference in the variable called text to be the interned zero-length string aka string.Empty. The actual string still exists on the managed heap, and probably still in various unmanaged places relating to the input mechanisms underneath txtPass.

Marc Gravell
  • 1,026,079
  • 266
  • 2,566
  • 2,900
  • Thanks for the quick reply and clearing up some misunderstandings. Well, this is just a school project which is supposed to be a web app for traffic education. So the security part isn't really that important at all, I'm just trying to learn a few extra things along the way. – heap1 Apr 30 '17 at 20:46
1

There are two issues here:

  1. the one you know about -- unprotected memory in heap, and
  2. the one you don't know about -- you shouldn't be hashing passwords with SHA1.

Let's address both:

(1) Many security people will recommend SecureString to protect your heap memory. However, it turns out that SecureString is not quite as good as advertised. If you want to understand why, you can watch this SecureString design review on youtube. It is long, but it is excellent, and you really only need to watch 10 or 15 minutes to see the issues with it.

In the specific context of a web application, you can try all sorts of stunts to prevent cleartext passwords from being in memory, but at the end of the day you are getting the object as a string from a Request Object. You have no control over the garbage collection of that Request Object. Trying to protect the memory after you get your hands on it is analogous to putting band-aids on a sieve.

Bottom line: don't worry about it. You can't fix that problem, which is inherent to the framework.

(2) Your ideas for password storage are falling under #4 in Top 10 Developer Crypto Mistakes.

Troy Hunt has an excellent article showing how passwords are cracked by one who gets access to the database, and how to prevent such attacks by using bcrypt or pbkdf2 (bcrypt is better).

TheGreatContini
  • 6,429
  • 2
  • 27
  • 37