1

I have a field in my database which is binary(32) for storing SHA-256 passwords. Since MSSQL store the hash in upper case and with 0x prefix, I've done this:

public static string getHashSha256(string text)
    {
        byte[] bytes = Encoding.UTF8.GetBytes(text);
        SHA256Managed hashstring = new SHA256Managed();
        byte[] hash = hashstring.ComputeHash(bytes);
        string hashString = string.Empty;
        foreach (byte x in hash)
        {
            hashString += String.Format("{0:x2}",  x);
        }
        return "0x" + hashString.ToUpper();
    }

Is this acceptable or there is a more appropriate way to do this?

jarlh
  • 42,561
  • 8
  • 45
  • 63
Black Panther
  • 145
  • 1
  • 12
  • 1
    You should be using a "salt". The salt should change every time the user changes their password. If you don't use a salt, everybody with the same password will have the same hash, which we've [already seen](https://nakedsecurity.sophos.com/2013/11/04/anatomy-of-a-password-disaster-adobes-giant-sized-cryptographic-blunder/) is a bad idea, and leaves compromised hashes vulnerable to dictionary attacks. – ProgrammingLlama Apr 26 '18 at 09:09
  • As for your comparison, if you have both values as byte arrays, you can use `.SequenceEqual()`. – ProgrammingLlama Apr 26 '18 at 09:09
  • If I use .SequenceEqual(), there will be no need for returning the hash with `0x` prefix and `.ToUpper()`? – Black Panther Apr 26 '18 at 09:14
  • 2
    "Since MSSQL store the hash in upper case and with 0x prefix" - that's not so. Column type in binary, so it stores it as binary, not as any string. So you should retrieve it from database as `byte[]` and compare accordingly. No need to involve strings anywhere in the process. – Evk Apr 26 '18 at 09:21
  • @Evk I see so If I got this right, It's upper case because for example the A is `01000001` and not `01100001` (a) in binary? – Black Panther Apr 26 '18 at 09:30
  • 2
    String you see is representation for display. Most likely it's hex. It's not stored like that and there is no reason for you to work with it in such form. – Evk Apr 26 '18 at 09:39
  • Even in SQL when I select from the table, It's just a representation? It looks like it. I just made the function above so It return byte and then compare it like you said and when I try to display that, I get XX-XX-XX-XX etc. and when I compare it to the database, the comparaison is successful, even though the database is not displaying the database values have no dashes. – Black Panther Apr 26 '18 at 09:54
  • 1
    You have to **Dispose** `SHA256Managed` instance since it implements `IDisposable`, otherwise you'll get resource leakage; see https://stackoverflow.com/questions/16999361/obtain-sha-256-string-of-a-string/17001289#17001289 – Dmitry Bychenko Apr 26 '18 at 10:19
  • @DmitryBychenko I should add `hash.Dispose()` just before the return? – Black Panther Apr 26 '18 at 11:13
  • @Abdou: try standard pattern: `using (SHA256Managed hashstring = new SHA256Managed()) {...return ...}` – Dmitry Bychenko Apr 26 '18 at 14:09
  • @DmitryBychenko yeah I just managed to understand that `using` does the dispose for you. Well, we learn every day :) Thanks for help man – Black Panther Apr 26 '18 at 14:14

1 Answers1

-1
public static string ConvertToHash(string dataToComputeHash)
    {
        var hash = "";
        try
        {
            var keyByte = encoding.GetBytes(key);
            using (var hmacsha256 = new HMACSHA256(keyByte))
            {
                hmacsha256.ComputeHash(encoding.GetBytes(dataToComputeHash));
                hash = ByteToString(hmacsha256.Hash);
            }
        }
        catch (Exception ex)
        {

        }
        return hash;
    }
ismail bhinder
  • 21
  • 5
  • 1
    Side note: please, never do/recommend this `catch (Exception ex) { }` - *ignoring all the exceptions*. We may ignore only some errors, and, francly speaking, I can' see any reason to ignore either `ArgumentNullException` or `ObjectDisposedException`: we should not *hide* the problem in the code. – Dmitry Bychenko Apr 26 '18 at 10:24