Skip to content

Commit 06776d1

Browse files
hvitvedhvitved
authored
Merge pull request github#4949 from luchua-bc/cs/hash-without-salt
C#: Query to detect hash without salt
2 parents fdd787b + e3afcb1 commit 06776d1

File tree

8 files changed

+615
-0
lines changed

8 files changed

+615
-0
lines changed

‎csharp/ql/src/experimental/Security Features/CWE-759/HashWithoutSalt.cs

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
public class Test
2+
{
3+
private const int SaltSize = 32;
4+
5+
// BAD - Hash without a salt.
6+
public static String HashPassword(string password, string strAlgName ="SHA256")
7+
{
8+
IBuffer passBuff = CryptographicBuffer.ConvertStringToBinary(password, BinaryStringEncoding.Utf8);
9+
HashAlgorithmProvider algProvider = HashAlgorithmProvider.OpenAlgorithm(strAlgName);
10+
IBuffer hashBuff = algProvider.HashData(passBuff);
11+
return CryptographicBuffer.EncodeToBase64String(hashBuff);
12+
}
13+
14+
// GOOD - Hash with a salt.
15+
public static string HashPassword2(string password, string salt, string strAlgName ="SHA256")
16+
{
17+
// Concatenate the salt with the password.
18+
IBuffer passBuff = CryptographicBuffer.ConvertStringToBinary(password+salt, BinaryStringEncoding.Utf8);
19+
HashAlgorithmProvider algProvider = HashAlgorithmProvider.OpenAlgorithm(strAlgName);
20+
IBuffer hashBuff = algProvider.HashData(passBuff);
21+
return CryptographicBuffer.EncodeToBase64String(hashBuff);
22+
}
23+
24+
// BAD - Hash without a salt.
25+
public static string HashPassword(string password)
26+
{
27+
SHA256 sha256Hash = SHA256.Create();
28+
byte[] passBytes = System.Text.Encoding.ASCII.GetBytes(password);
29+
byte[] hashBytes = sha256Hash.ComputeHash(passBytes);
30+
return Convert.ToBase64String(hashBytes);
31+
}
32+
33+
// GOOD - Hash with a salt.
34+
public static string HashPassword2(string password)
35+
{
36+
byte[] passBytes = System.Text.Encoding.ASCII.GetBytes(password);
37+
byte[] saltBytes = GenerateSalt();
38+
39+
// Add the salt to the hash.
40+
byte[] rawSalted = new byte[passBytes.Length + saltBytes.Length];
41+
passBytes.CopyTo(rawSalted, 0);
42+
saltBytes.CopyTo(rawSalted, passBytes.Length);
43+
44+
//Create the salted hash.
45+
SHA256 sha256 = SHA256.Create();
46+
byte[] saltedPassBytes = sha256.ComputeHash(rawSalted);
47+
48+
// Add the salt value to the salted hash.
49+
byte[] dbPassword = new byte[saltedPassBytes.Length + saltBytes.Length];
50+
saltedPassBytes.CopyTo(dbPassword, 0);
51+
saltBytes.CopyTo(dbPassword, saltedPassBytes.Length);
52+
53+
return Convert.ToBase64String(dbPassword);
54+
}
55+
56+
public static byte[] GenerateSalt()
57+
{
58+
using (var rng = new RNGCryptoServiceProvider())
59+
{
60+
var randomNumber = new byte[SaltSize];
61+
rng.GetBytes(randomNumber);
62+
return randomNumber;
63+
}
64+
}
65+
}

‎csharp/ql/src/experimental/Security Features/CWE-759/HashWithoutSalt.qhelp

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
2+
<qhelp>
3+
4+
<overview>
5+
<p>In cryptography, a salt is some random data used as an additional input to a one-way function that hashes a password or pass-phrase. It makes dictionary attacks more difficult.</p>
6+
7+
<p>Without a salt, it is much easier for attackers to pre-compute the hash value using dictionary attack techniques such as rainbow tables to crack passwords.</p>
8+
</overview>
9+
10+
<recommendation>
11+
<p>Use a long random salt of at least 32 bytes then use the combination of password and salt to hash a password or password phrase.</p>
12+
</recommendation>
13+
14+
<example>
15+
<p>The following example shows two ways of hashing. In the 'BAD' cases, no salt is provided. In the 'GOOD' cases, a salt is provided.</p>
16+
<sample src="HashWithoutSalt.cs" />
17+
</example>
18+
19+
<references>
20+
<li>
21+
DZone:
22+
<a href="https://dzone.com/articles/a-look-at-java-cryptography">A Look at Java Cryptography</a>
23+
</li>
24+
<li>
25+
CWE:
26+
<a href="https://cwe.mitre.org/data/definitions/759.html">CWE-759: Use of a One-Way Hash without a Salt</a>
27+
</li>
28+
</references>
29+
</qhelp>

‎csharp/ql/src/experimental/Security Features/CWE-759/HashWithoutSalt.ql

Lines changed: 200 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,200 @@
1+
/**
2+
* @name Use of a hash function without a salt
3+
* @description Hashed passwords without a salt are vulnerable to dictionary attacks.
4+
* @kind path-problem
5+
* @problem.severity error
6+
* @id cs/hash-without-salt
7+
* @tags security
8+
* external/cwe-759
9+
*/
10+
11+
import csharp
12+
import semmle.code.csharp.dataflow.DataFlow2
13+
import semmle.code.csharp.dataflow.TaintTracking
14+
import semmle.code.csharp.dataflow.TaintTracking2
15+
import DataFlow::PathGraph
16+
17+
/** The C# class `Windows.Security.Cryptography.Core.HashAlgorithmProvider`. */
18+
class HashAlgorithmProvider extends RefType {
19+
HashAlgorithmProvider() {
20+
this.hasQualifiedName("Windows.Security.Cryptography.Core", "HashAlgorithmProvider")
21+
}
22+
}
23+
24+
/** The C# class `System.Security.Cryptography.HashAlgorithm`. */
25+
class HashAlgorithm extends RefType {
26+
HashAlgorithm() { this.hasQualifiedName("System.Security.Cryptography", "HashAlgorithm") }
27+
}
28+
29+
/** The C# class `System.Security.Cryptography.KeyedHashAlgorithm`. */
30+
class KeyedHashAlgorithm extends RefType {
31+
KeyedHashAlgorithm() {
32+
this.hasQualifiedName("System.Security.Cryptography", "KeyedHashAlgorithm")
33+
}
34+
}
35+
36+
/**
37+
* The method `ComputeHash()`, `ComputeHashAsync`, `TryComputeHash`, `HashData`, or
38+
* `TryHashData` declared in `System.Security.Cryptography.HashAlgorithm` and the method
39+
* `HashData()` declared in `Windows.Security.Cryptography.Core.HashAlgorithmProvider`.
40+
*/
41+
class HashMethod extends Method {
42+
HashMethod() {
43+
this.getDeclaringType().getABaseType*() instanceof HashAlgorithm and
44+
this.getName().matches(["%ComputeHash%", "%HashData"])
45+
or
46+
this.getDeclaringType().getABaseType*() instanceof HashAlgorithmProvider and
47+
this.hasName("HashData")
48+
}
49+
}
50+
51+
/**
52+
* Gets a regular expression for matching common names of variables that indicate the
53+
* value being held is a password.
54+
*/
55+
string getPasswordRegex() { result = "(?i)pass(wd|word|code|phrase)" }
56+
57+
/** Finds variables that hold password information judging by their names. */
58+
class PasswordVarExpr extends Expr {
59+
PasswordVarExpr() {
60+
exists(Variable v | this = v.getAnAccess() | v.getName().regexpMatch(getPasswordRegex()))
61+
}
62+
}
63+
64+
/**
65+
* Holds if `mc` is a hashing method call or invokes a hashing method call
66+
* directly or indirectly.
67+
*/
68+
predicate isHashCall(MethodCall mc) {
69+
mc.getTarget() instanceof HashMethod
70+
or
71+
exists(MethodCall mcc |
72+
mc.getTarget().calls(mcc.getTarget()) and
73+
isHashCall(mcc) and
74+
DataFlow::localExprFlow(mc.getTarget().getAParameter().getAnAccess(), mcc.getAnArgument())
75+
)
76+
}
77+
78+
/** Holds if there is another hashing method call. */
79+
predicate hasAnotherHashCall(MethodCall mc) {
80+
exists(MethodCall mc2, DataFlow2::Node src, DataFlow2::Node sink |
81+
isHashCall(mc2) and
82+
mc2 != mc and
83+
(
84+
src.asExpr() = mc.getQualifier() or
85+
src.asExpr() = mc.getAnArgument() or
86+
src.asExpr() = mc
87+
) and
88+
(
89+
sink.asExpr() = mc2.getQualifier() or
90+
sink.asExpr() = mc2.getAnArgument()
91+
) and
92+
DataFlow::localFlow(src, sink)
93+
)
94+
}
95+
96+
/** Holds if a password hash without salt is further processed in another method call. */
97+
predicate hasFurtherProcessing(MethodCall mc) {
98+
mc.getTarget().fromLibrary() and
99+
(
100+
mc.getTarget().hasQualifiedName("System.Array", "Copy") or // Array.Copy(passwordHash, 0, password.Length), 0, key, 0, keyLen);
101+
mc.getTarget().hasQualifiedName("System.String", "Concat") or // string.Concat(passwordHash, saltkey)
102+
mc.getTarget().hasQualifiedName("System.Buffer", "BlockCopy") or // Buffer.BlockCopy(passwordHash, 0, allBytes, 0, 20)
103+
mc.getTarget().hasQualifiedName("System.String", "Format") // String.Format("{0}:{1}:{2}", username, salt, password)
104+
)
105+
}
106+
107+
/**
108+
* Holds if `mc` is part of a call graph that satisfies `isHashCall` but is not at the
109+
* top of the call hierarchy.
110+
*/
111+
predicate hasHashAncestor(MethodCall mc) {
112+
exists(MethodCall mpc |
113+
mpc.getTarget().calls(mc.getTarget()) and
114+
isHashCall(mpc) and
115+
DataFlow::localExprFlow(mpc.getTarget().getAParameter().getAnAccess(), mc.getAnArgument())
116+
)
117+
}
118+
119+
/**
120+
* Taint configuration tracking flow from an expression whose name suggests it holds
121+
* password data to a method call that generates a hash without a salt.
122+
*/
123+
class HashWithoutSaltConfiguration extends TaintTracking::Configuration {
124+
HashWithoutSaltConfiguration() { this = "HashWithoutSaltConfiguration" }
125+
126+
override predicate isSource(DataFlow::Node source) { source.asExpr() instanceof PasswordVarExpr }
127+
128+
override predicate isSink(DataFlow::Node sink) {
129+
exists(MethodCall mc |
130+
sink.asExpr() = mc.getArgument(0) and
131+
isHashCall(mc) and
132+
not hasAnotherHashCall(mc) and
133+
not hasHashAncestor(mc) and
134+
not exists(MethodCall mmc |
135+
hasFurtherProcessing(mmc) and
136+
DataFlow::localExprFlow(mc, mmc.getAnArgument())
137+
) and
138+
not exists(Call c |
139+
(
140+
c.getTarget().getDeclaringType().getABaseType*() instanceof HashAlgorithm or
141+
c.getTarget()
142+
.getDeclaringType()
143+
.getABaseType*()
144+
.hasQualifiedName("System.Security.Cryptography", "DeriveBytes")
145+
) and
146+
DataFlow::localExprFlow(mc, c.getAnArgument())
147+
)
148+
)
149+
}
150+
151+
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
152+
exists(MethodCall mc |
153+
mc.getTarget()
154+
.hasQualifiedName("Windows.Security.Cryptography.CryptographicBuffer",
155+
"ConvertStringToBinary") and
156+
mc.getArgument(0) = node1.asExpr() and
157+
mc = node2.asExpr()
158+
)
159+
}
160+
161+
/**
162+
* Holds if a password is concatenated with a salt then hashed together through calls such as `System.Array.CopyTo()`, for example,
163+
* `byte[] rawSalted = new byte[passBytes.Length + salt.Length];`
164+
* `passBytes.CopyTo(rawSalted, 0);`
165+
* `salt.CopyTo(rawSalted, passBytes.Length);`
166+
* `byte[] saltedPassword = sha256.ComputeHash(rawSalted);`
167+
* Or the password is concatenated with a salt as a string.
168+
*/
169+
override predicate isSanitizer(DataFlow::Node node) {
170+
exists(MethodCall mc |
171+
hasFurtherProcessing(mc) and
172+
mc.getAnArgument() = node.asExpr()
173+
)
174+
or
175+
exists(AddExpr e | node.asExpr() = e.getAnOperand()) // password+salt
176+
or
177+
exists(InterpolatedStringExpr e | node.asExpr() = e.getAnInsert())
178+
or
179+
exists(Call c |
180+
c.getTarget()
181+
.getDeclaringType()
182+
.getABaseType*()
183+
.hasQualifiedName("System.Security.Cryptography", "DeriveBytes")
184+
)
185+
or
186+
// a salt or key is included in subclasses of `KeyedHashAlgorithm`
187+
exists(MethodCall mc, Assignment a, ObjectCreation oc |
188+
a.getRValue() = oc and
189+
oc.getObjectType().getABaseType+() instanceof KeyedHashAlgorithm and
190+
mc.getTarget() instanceof HashMethod and
191+
a.getLValue() = mc.getQualifier().(VariableAccess).getTarget().getAnAccess() and
192+
mc.getArgument(0) = node.asExpr()
193+
)
194+
}
195+
}
196+
197+
from DataFlow::PathNode source, DataFlow::PathNode sink, HashWithoutSaltConfiguration c
198+
where c.hasFlowPath(source, sink)
199+
select sink.getNode(), source, sink, "$@ is hashed without a salt.", source.getNode(),
200+
"The password"

0 commit comments

Comments
 (0)