Identityserver4: Secret Hashing

[dpbevin]

When a Secret is used as part of a non-human Client, it's effectively a password that needs to be protected. One of my go-to resources for security is the OWASP website and they recommend using a "salt" value when storing passwords:
https://www.owasp.org/index.php/Password_Storage_Cheat_Sheet

The ASP.NET Core documentation suggests, and provides examples for, a similar technique:
https://docs.asp.net/en/latest/security/data-protection/consumer-apis/password-hashing.html

Do you think it would be worthwhile added a "Salt" property to the Secret then modifying the HashedSharedSecretValidator to handle this?

I'm happy to submit a PR for this kinda change when you're happy with the proposal. It might also be worthwhile providing an ICryptoGenerator via Dependency Injection to complement the HashExtensions class for generating Salt values.

[brockallen]

A salt is to thwart dictionary-style attacks. Given that we're using high entropy secrets, a salt isn't really necessary.

[brockallen]

Sorry -- I should have added that if you disagree, please feel free to express your concerns.

[leastprivilege]

Right - the recommendation for client secrets is to use random numbers - which already have a high enough entropy.

[dpbevin]

Maybe I'm just old school but without a salt, if two clients used the same secret, then they'd end up with the same "Sha256" value. Correct?

[leastprivilege]

True. But secrets should be random keys to start with.

Sent from my iPhone

On 12.09.2016, at 14:29, dpbevin [email protected] wrote:

Maybe I'm just old school but without a salt, if two clients used the same secret, then they'd end up with the same "Sha256" value. Correct?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

[brockallen]

Also, you should be generating these from your client registration. Look into this API: https://github.com/IdentityModel/IdentityModelv2/blob/dev/src/IdentityModel/CryptoRandom.cs#L34

[dpbevin]

Thanks for the tip

[ischultz]

I share the same concern as @dpbevin, though I admit that I am very new to IdentityServer/OAuth/OpenId Connect. It seems that we should be using salts and a more secure algorithm to hash the secrets, not because the secrets aren't high enough entropy, but in order to prevent brute force attacks if, say, the database storing the secrets were to become compromised. Am I thinking about this incorrectly or missing something?

[brockallen]

A 128 bit entropy secret hashed once should be sufficient. If you have anything to show this is not sufficient, please share. Also, you can always use a larger key space.

[ischultz]

I have nothing to share at this time that would prove this to be insufficient. I suppose that I'm just a bit paranoid of the situation in which vulnerabilities or implementation errors do happen to be found with Sha256. While salts may not necessarily be a cure-all depending on the type of vulnerability, I would imagine that they could end up helping to minimize the attack surface and overall exposure at a relatively low cost if such a situation were to arise, no?

[leastprivilege]

If Sha256 is vulnerable we'll have bigger problems than our client secrets ;)

Anyways - you can use Sha512 as well - just make sure the secrets have a good entropy to start with - or implement your own secret validator with your preferred hashing algorithm.

[ischultz]

Good point.. I appreciate your feedback! Thanks for everything you guys do!

[pwen090]

@leastprivilege @brockallen The vulnerability is not within SHA256 but hashing function weaknesses in general when not leveraging a salt. Many pre-computed databases of common hash functions exist that allow for an attacker to simply lookup a hash to know the pre-hashed string. E.g. Rainbow tables. The issue is not in remotely brute forcing against IdentityServer but an attacker whom is able to compromise ID Secrets database and then gather pre-hash secrets/passwords via a simple lookup in a table. I just wanted to add this note so that if any other developers find this they know unsalted hashes used for authentication is not a best practice. You can obviously work around this with your own modifications in IdentityServer but I would recommend this not be how IdentityServer works by default as plenty of people are probably not fixing this themselves.

[scottbrady91]

My client secret is TNW+nxBGh/WRwE3XOpK8iicEtwA1detrhBwJUDGKuOo=. Do you know of any lookup tables that contain a pre-calculated hash of values like that?

[pwen090]

@scottbrady91 As a lead in Identity and Access control it is your position that you should not salt hashes that are used for authentication?

[scottbrady91]

For from it. But I'm failing to see how your concerns address Brock's previous statements:

A salt is to thwart dictionary-style attacks. Given that we're using high entropy secrets, a salt isn't really necessary.

A 128 bit entropy secret hashed once should be sufficient. If you have anything to show this is not sufficient, please share. Also, you can always use a larger key space.

[brockallen]

Many pre-computed databases of common hash functions exist that allow for an attacker to simply lookup a hash to know the pre-hashed string. E.g. Rainbow tables

The size of said database would be large:

Look at this way: Suppose you could somehow get this algorithm to run a quintillion times faster, so it finishes in under a year. Your output file is going to be 2¹²⁸ × 16 = 2¹³² bytes in size. That's around 10²⁷ terabytes. One terabyte of SSD storage weighs around 100 grams. The mass of the earth is 10²⁴ kilograms. Therefore, before you run this program, you will need to acquire 100 earth-sized planets and convert them all to SSDs.

from: https://blogs.msdn.microsoft.com/oldnewthing/20131029-00/?p=2803/

Salting is to address collisions in the secret, and is useful when the secret is human created (not machine created).

[pwen090]

I hear you guys, I guess I was coming from the perspective of a salt could help people that do not know better but probably the better thing is just making more clear in the documentation how to generate a proper secret. One quick Q what function were you recommending above in CryptoRandom.cs as the link is old? Thanks much

[scottbrady91]

It's the CryptoRandom class in IdentityModel.

[marcKoutzarov]

Adding Salt property in the client object will make Databases that store passwords/secrets compliant. Not supporting this functionality creates a lot of compliance issues.

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.