Your submission is now in status SecLab review.

For information, the evaluation workflow is the following:
CodeQL initial assessment > SecLab review > CodeQL review > SecLab finalize > Pay > Closed

Some FP:
nopSolutions/nopCommerce
ikoiko/DatingApp
asukhodko/dotnet-tarantool-client
Kation/ComBoost
GhostPack/SharpDPAPI
NexusForever/NexusForever
duplicati/httpserver

Not sure if all FP can be eliminated, because the sink is hashing function, but the hash can be mixed with the salt later.

Some additional possible sinks:
The HashAlgorithm class the following methods that can be used for hashin: ComputeHashAsync, TryComputeHash, TransformFinalBlock. MD5,SHA1,SHA256, SHA384, SHA512 classes has these static methods: HashData, TryHashData.

Thanks @JarLob for the suggestions. The C# query seems to have the same issue of wrapper method calls similar to the Java query #4920. It turned out to be pretty complex to model the call stack correctly. However, addressing one will help to address the other. I will work on this one once the issue with the Java one is resolved.

@JarLob I've revamped the query to handle more cases. Now ALL FPs with listed sample projects are eliminated:-)

A lot of changes have been made after extensive testing. Now the query is much more comprehensive and powerful. I will add additional sinks as a separate commit, which will probably be done tomorrow. And please let me know if more scenarios could be covered.

Great, lets wait for the new sinks and have a new LGTM run then.

@JarLob I've committed the change of adding new sinks. As TransformFinalBlock/TransformBlock does byte array manipulation, I'm afraid it may have a higher rate of FPs. Also it already makes the password hash harder to guess therefore I didn't include it. Please let me know if you still want to add it.

Please review.

Thanks,
@luchua-bc

I have started a new LGTM run and will get back to you.

Your submission is now in status CodeQL review.

For information, the evaluation workflow is the following:
CodeQL initial assessment > SecLab review > CodeQL review > SecLab finalize > Pay > Closed

Your submission is now in status SecLab review.

For information, the evaluation workflow is the following:
SecLab review > FP Check > CodeQL review > SecLab finalize > Pay > Closed

Your submission is now in status FP Check.

For information, the evaluation workflow is the following:
SecLab review > FP Check > CodeQL review > SecLab finalize > Pay > Closed

Hi @luchua-bc,
Sorry for delay, the issue was not properly assigned to a team.

Unfortunately the query produces too many FPs:
progaudi/progaudi.tarantool
jellyfin/jellyfin
Wndrr/HaveIBeenPwned
andreasbotsikas/pvk2pfxcore
mono/mono
grandnode/grandnode
mirzaevolution/MirzaCryptoHelpersV2
ststeiger/Lumisoft.Net.Core

The FP ratio needs to be reduced before the query could be accepted.

Hi @JarLob,

Thanks for reviewing the PR.

The jellyfin/jellyfin doesn't seem to be FP since it allows hashing without salt (empty array) in the following call stack:

            var hashedPasswordBytes = _cryptoProvider.ComputeHash("SHA1", Encoding.ASCII.GetBytes(password), Array.Empty<byte>());
...
            using var h = HashAlgorithm.Create(hashMethod);
            if (salt.Length == 0)
            {
                return h.ComputeHash(bytes);
            }

And for Wndrr/HaveIBeenPwned, it doesn't seem to be FP either since the processing is encoding instead of salting:

            var hash = Utils.GetSha1Hash(plainTextPassword);

        public static string GetSha1Hash(string input)
        {
            using (var sha1 = new SHA1Managed())
            {
                var hash = sha1.ComputeHash(Encoding.UTF8.GetBytes(input));
                var stringBuilder = new StringBuilder(hash.Length * 2);

                foreach (var b in hash)
                {
                    // can be "x2" if you want lowercase
                    stringBuilder.Append(b.ToString("X2"));
                }

                return stringBuilder.ToString();
            }
        }

Please let me know if they are actually FPs in case I miss something.

For the other 6 projects, the hash is pre-joined with a salt, serves as inputs for further processing, or isn't a password. I will improve the query to reduce FPs.

@luchua-bc

In case of jellyfin it falls under the if (salt.Length == 0) so technically can be TP, however since the code anticipates that the salt may be absent really is a FP and would annoy developers.
The Wndrr/HaveIBeenPwned case is an example when it really meant to calculate unsalted hash.
It think if you could get rid of 6 of the FPs that would be a progress.

One more FP: neo-project/neo

The NEO project (neo-project/neo) just hashes a password twice without using a salt so it's not a FP for this check of password hash without salt in my opinion:

            using (SHA256 sha256 = SHA256.Create())
            {
                byte[] passwordBytes = password.ToArray();
                byte[] passwordHash = sha256.ComputeHash(passwordBytes);
                byte[] passwordHash2 = sha256.ComputeHash(passwordHash);
                Array.Clear(passwordBytes, 0, passwordBytes.Length);
                Array.Clear(passwordHash, 0, passwordHash.Length);
                return passwordHash2;
            }

However, the change I'm going to make shall address this issue, which is the scenario of serving as inputs for further processing and is one of the three enhanced scenarios that the query will handle.

It adds salt and hashes it again in the calling function like password.ToAesKey().Concat(salt).ToArray().Sha256().SequenceEqual(LoadStoredData("PasswordHash")); See https://github.com/neo-project/neo/blob/master/src/neo/Wallets/SQLite/UserWallet.cs

You are right. This will be handled in third scenario of serving as inputs for further processing.

I've modified the query to check those scenarios. Basically those repositories either use the password as the hash or salt to generate hashes of other pieces of information, or further process after a password hash is generated without a salt.

Now the results are much cleaner. Please validate. Thanks.

Your submission is now in status CodeQL review.

For information, the evaluation workflow is the following:
SecLab review > Generate Query Results > FP Check > CodeQL review > SecLab finalize > Pay > Closed

Hi @JarLob,

I'm wondering whether there is an update since the PR has been in the "CodeQL review" status for several months. Would you please investigate when you have a chance?

Thanks,
@luchua-bc

@ghsecuritylab It has been several months without any movement on this PR. It would be greatly appreciated if its status could be checked. Thanks in advance.

Thanks for the help from @hvitved, the relevant PR has been approved and merged into main. Would you please help to move this submission to the next status stage? Thanks.

@xcorail - would you please have its status updated and close this PR as well? It's an old submission so it doesn't have the new review stages/gates but it's been approved for almost 4 weeks. Thanks.

Your submission is now in status Final decision.

For information, the evaluation workflow is the following:
Initial triage > Test run > Results analysis > Query review > Final decision > Pay > Closed

Your submission is now in status Pay.

For information, the evaluation workflow is the following:
Initial triage > Test run > Results analysis > Query review > Final decision > Pay > Closed

Your submission is now in status Closed.

For information, the evaluation workflow is the following:
Initial triage > Test run > Results analysis > Query review > Final decision > Pay > Closed

Created Hackerone report 1484086 for bounty 369630 : [233] [C#] CWE-759: Query to detect password hash without a salt

Apologies about the delay @luchua-bc

Thanks @xcorail a lot for the quick turn-around and the bounty:-)