Skip to content

Commit 6a2c7d5

Browse files
luchua-bcluchua-bc
committed
Enhance the query to check more scenarios
1 parent 6bfe2f2 commit 6a2c7d5

File tree

4 files changed

+226
-40
lines changed

4 files changed

+226
-40
lines changed

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

Lines changed: 88 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,9 @@ class KeyedHashAlgorithm extends RefType {
3333
}
3434

3535
/**
36-
* The method `ComputeHash()`, `ComputeHashAsync`, `TryComputeHash`, `HashData`, or `TryHashData` declared in `System.Security.Cryptography.HashAlgorithm` and
37-
* the method `HashData()` declared in `Windows.Security.Cryptography.Core.HashAlgorithmProvider`.
36+
* The method `ComputeHash()`, `ComputeHashAsync`, `TryComputeHash`, `HashData`, or
37+
* `TryHashData` declared in `System.Security.Cryptography.HashAlgorithm` and the method
38+
* `HashData()` declared in `Windows.Security.Cryptography.Core.HashAlgorithmProvider`.
3839
*/
3940
class HashMethod extends Method {
4041
HashMethod() {
@@ -46,8 +47,11 @@ class HashMethod extends Method {
4647
}
4748
}
4849

49-
/** Gets a regular expression for matching common names of variables that indicate the value being held is a password. */
50-
string getPasswordRegex() { result = "(?i).*pass(wd|word|code|phrase).*" }
50+
/**
51+
* Gets a regular expression for matching common names of variables that indicate the
52+
* value being held is a password.
53+
*/
54+
string getPasswordRegex() { result = "(?i)pass(wd|word|code|phrase)" }
5155

5256
/** Finds variables that hold password information judging by their names. */
5357
class PasswordVarExpr extends Expr {
@@ -56,21 +60,65 @@ class PasswordVarExpr extends Expr {
5660
}
5761
}
5862

63+
/**
64+
* Holds if `mc` is a hashing method call or invokes a hashing method call
65+
* directly or indirectly.
66+
*/
67+
predicate isHashCall(MethodCall mc) {
68+
mc.getTarget() instanceof HashMethod
69+
or
70+
exists(MethodCall mcc |
71+
mc.getTarget().calls(mcc.getTarget()) and
72+
isHashCall(mcc) and
73+
DataFlow::localExprFlow(mc.getTarget().getAParameter().getAnAccess(), mcc.getAnArgument())
74+
)
75+
}
76+
5977
/** Holds if there is another hashing method call. */
6078
predicate hasAnotherHashCall(MethodCall mc) {
61-
exists(Call mc2, DataFlow2::Node src, DataFlow2::Node sink |
79+
exists(MethodCall mc2, DataFlow2::Node src, DataFlow2::Node sink |
80+
isHashCall(mc2) and
81+
mc2 != mc and
6282
(
63-
mc2.getTarget() instanceof HashMethod or
64-
mc2.(ObjectCreation).getObjectType().getABaseType+() instanceof HashAlgorithm
83+
src.asExpr() = mc.getQualifier() or
84+
src.asExpr() = mc.getAnArgument() or
85+
src.asExpr() = mc
6586
) and
66-
mc2 != mc and
67-
src.asExpr() = mc.getAReachableElement() and
68-
sink.asExpr() = mc2.getAChildExpr() and
69-
TaintTracking2::localTaint(src, sink)
87+
(
88+
sink.asExpr() = mc2.getQualifier() or
89+
sink.asExpr() = mc2.getAnArgument()
90+
) and
91+
DataFlow::localFlow(src, sink)
92+
)
93+
}
94+
95+
/** Holds if a password hash without salt is further processed in another method call. */
96+
predicate hasFurtherProcessing(MethodCall mc) {
97+
mc.getTarget().fromLibrary() and
98+
(
99+
mc.getTarget().hasQualifiedName("System.Array", "Copy") or // Array.Copy(passwordHash, 0, password.Length), 0, key, 0, keyLen);
100+
mc.getTarget().hasQualifiedName("System.String", "Concat") or // string.Concat(passwordHash, saltkey)
101+
mc.getTarget().hasQualifiedName("System.Buffer", "BlockCopy") or // Buffer.BlockCopy(passwordHash, 0, allBytes, 0, 20)
102+
mc.getTarget().hasQualifiedName("System.String", "Format") // String.Format("{0}:{1}:{2}", username, salt, password)
70103
)
71104
}
72105

73-
/** Taint configuration tracking flow from an expression whose name suggests it holds password data to a method call that generates a hash without a salt. */
106+
/**
107+
* Holds if `mc` is part of a call graph that satisfies `isHashCall` but is not at the
108+
* top of the call hierarchy.
109+
*/
110+
predicate hasHashAncestor(MethodCall mc) {
111+
exists(MethodCall mpc |
112+
mpc.getTarget().calls(mc.getTarget()) and
113+
isHashCall(mpc) and
114+
DataFlow::localExprFlow(mpc.getTarget().getAParameter().getAnAccess(), mc.getAnArgument())
115+
)
116+
}
117+
118+
/**
119+
* Taint configuration tracking flow from an expression whose name suggests it holds
120+
* password data to a method call that generates a hash without a salt.
121+
*/
74122
class HashWithoutSaltConfiguration extends TaintTracking::Configuration {
75123
HashWithoutSaltConfiguration() { this = "HashWithoutSaltConfiguration" }
76124

@@ -79,8 +127,23 @@ class HashWithoutSaltConfiguration extends TaintTracking::Configuration {
79127
override predicate isSink(DataFlow::Node sink) {
80128
exists(MethodCall mc |
81129
sink.asExpr() = mc.getArgument(0) and
82-
mc.getTarget() instanceof HashMethod and
83-
not hasAnotherHashCall(mc)
130+
isHashCall(mc) and
131+
not hasAnotherHashCall(mc) and
132+
not hasHashAncestor(mc) and
133+
not exists(MethodCall mmc |
134+
hasFurtherProcessing(mmc) and
135+
DataFlow::localExprFlow(mc, mmc.getAnArgument())
136+
) and
137+
not exists(Call c |
138+
(
139+
c.getTarget().getDeclaringType().getABaseType*() instanceof HashAlgorithm or
140+
c.getTarget()
141+
.getDeclaringType()
142+
.getABaseType*()
143+
.hasQualifiedName("System.Security.Cryptography", "DeriveBytes")
144+
) and
145+
DataFlow::localExprFlow(mc, c.getAnArgument())
146+
)
84147
)
85148
}
86149

@@ -104,26 +167,27 @@ class HashWithoutSaltConfiguration extends TaintTracking::Configuration {
104167
*/
105168
override predicate isSanitizer(DataFlow::Node node) {
106169
exists(MethodCall mc |
107-
mc.getTarget().fromLibrary() and
108-
(
109-
mc.getTarget().hasQualifiedName("System.Array", "CopyTo") or // passBytes.CopyTo(rawSalted, 0)
110-
mc.getTarget().hasQualifiedName("System.String", "Concat") or // string.Concat(password, saltkey)
111-
mc.getTarget().hasQualifiedName("System.Buffer", "BlockCopy") or // Buffer.BlockCopy(salt, 0, allBytes, 0, 20)
112-
mc.getTarget().hasQualifiedName("System.String", "Format") // String.Format("{0}:{1}:{2}", username, salt, password)
113-
) and
170+
hasFurtherProcessing(mc) and
114171
mc.getAnArgument() = node.asExpr()
115172
)
116173
or
117174
exists(AddExpr e | node.asExpr() = e.getAnOperand()) // password+salt
118175
or
119176
exists(InterpolatedStringExpr e | node.asExpr() = e.getAnInsert())
120177
or
178+
exists(Call c |
179+
c.getTarget()
180+
.getDeclaringType()
181+
.getABaseType*()
182+
.hasQualifiedName("System.Security.Cryptography", "DeriveBytes")
183+
)
184+
or
121185
// a salt or key is included in subclasses of `KeyedHashAlgorithm`
122-
exists(MethodCall mc, Assignment ass, ObjectCreation oc |
123-
ass.getRValue() = oc and
186+
exists(MethodCall mc, Assignment a, ObjectCreation oc |
187+
a.getRValue() = oc and
124188
oc.getObjectType().getABaseType+() instanceof KeyedHashAlgorithm and
125189
mc.getTarget() instanceof HashMethod and
126-
ass.getLValue() = mc.getQualifier().(VariableAccess).getTarget().getAnAccess() and
190+
a.getLValue() = mc.getQualifier().(VariableAccess).getTarget().getAnAccess() and
127191
mc.getArgument(0) = node.asExpr()
128192
)
129193
}

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

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// semmle-extractor-options: /r:System.Security.Cryptography.Primitives.dll /r:System.Security.Cryptography.Csp.dll /r:System.Security.Cryptography.Algorithms.dll
22

33
using System;
4+
using System.Text;
45
using System.Security.Cryptography;
56

67
using Windows.Security.Cryptography;
@@ -130,4 +131,72 @@ public static byte[] CalculateKeys(string password, string userid)
130131
}
131132
return hmacbytes;
132133
}
134+
135+
private byte[] TryDecrypt(byte[] buffer, int offset, int length, byte[] password, int keyLen) {
136+
byte[] key = new byte[16];
137+
Array.Copy(SHA1.Create().ComputeHash(password, 0, password.Length), 0, key, 0, keyLen);
138+
byte[] ret = Aes.Create().CreateDecryptor(key, null).TransformFinalBlock(buffer, offset, length);
139+
return ret;
140+
}
141+
142+
// GOOD - Use password hash without a salt having further processing.
143+
public byte[] encrypt(byte[] pass, byte[] salt, byte[] blob) {
144+
byte[] key = new byte[salt.Length + pass.Length];
145+
Array.Copy(salt, 0, key, 0, salt.Length);
146+
Array.Copy(pass, 0, key, salt.Length, pass.Length);
147+
byte[] pkb = TryDecrypt(blob, 8, blob.Length - 8, key, 16);
148+
return pkb;
149+
}
150+
151+
public string CreatePasswordHash(string password, string saltkey)
152+
{
153+
var saltAndPassword = string.Concat(password, saltkey);
154+
HashAlgorithm algorithm = SHA256.Create();
155+
var hashByteArray = algorithm.ComputeHash(Encoding.UTF8.GetBytes(saltAndPassword));
156+
return BitConverter.ToString(hashByteArray).Replace("-", "");
157+
}
158+
159+
private string GetMD5HashBinHex (string toBeHashed)
160+
{
161+
MD5 hash = MD5.Create ();
162+
byte[] result = hash.ComputeHash (Encoding.ASCII.GetBytes (toBeHashed));
163+
164+
StringBuilder sb = new StringBuilder ();
165+
foreach (byte b in result)
166+
sb.Append (b.ToString ("x2"));
167+
168+
return sb.ToString ();
169+
}
170+
171+
// GOOD: Password concatenated with other information before hashing
172+
public string CreatePasswordHash2(string username, string realm, string password)
173+
{
174+
string A1 = String.Format ("{0}:{1}:{2}", username, realm, password);
175+
176+
string HA1 = GetMD5HashBinHex (A1);
177+
return HA1;
178+
}
179+
180+
private byte[] Xor(byte[] array1, byte[] array2) {
181+
var result = new byte[array1.Length];
182+
183+
for (int i = 0; i < array1.Length; i++) {
184+
result[i] = (byte)(array1[i] ^ array2[i]);
185+
}
186+
187+
return result;
188+
}
189+
190+
// GOOD: Password hash without salt is further hashed with salt
191+
public byte[] GetScrable(byte[] password, byte[] decodedSalt) {
192+
var first20SaltBytes = new byte[20];
193+
Array.Copy(decodedSalt, first20SaltBytes, 20);
194+
195+
var step1 = Sha1Utils.Hash(password);
196+
var step2 = Sha1Utils.Hash(step1);
197+
var step3 = Sha1Utils.Hash(first20SaltBytes, step2);
198+
var scrambleBytes = Xor(step1, step3);
199+
200+
return scrambleBytes;
201+
}
133202
}

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

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,19 @@
11
edges
2-
| HashWithoutSalt.cs:17:70:17:77 | access to parameter password : String | HashWithoutSalt.cs:19:49:19:56 | access to local variable passBuff |
3-
| HashWithoutSalt.cs:37:28:37:72 | call to method GetBytes : Byte[] | HashWithoutSalt.cs:38:51:38:59 | access to local variable passBytes |
4-
| HashWithoutSalt.cs:37:64:37:71 | access to parameter password : String | HashWithoutSalt.cs:37:28:37:72 | call to method GetBytes : Byte[] |
5-
| HashWithoutSalt.cs:69:28:69:72 | call to method GetBytes : Byte[] | HashWithoutSalt.cs:70:48:70:56 | access to local variable passBytes |
6-
| HashWithoutSalt.cs:69:64:69:71 | access to parameter password : String | HashWithoutSalt.cs:69:28:69:72 | call to method GetBytes : Byte[] |
2+
| HashWithoutSalt.cs:18:70:18:77 | access to parameter password : String | HashWithoutSalt.cs:20:49:20:56 | access to local variable passBuff |
3+
| HashWithoutSalt.cs:38:28:38:72 | call to method GetBytes : Byte[] | HashWithoutSalt.cs:39:51:39:59 | access to local variable passBytes |
4+
| HashWithoutSalt.cs:38:64:38:71 | access to parameter password : String | HashWithoutSalt.cs:38:28:38:72 | call to method GetBytes : Byte[] |
5+
| HashWithoutSalt.cs:70:28:70:72 | call to method GetBytes : Byte[] | HashWithoutSalt.cs:71:48:71:56 | access to local variable passBytes |
6+
| HashWithoutSalt.cs:70:64:70:71 | access to parameter password : String | HashWithoutSalt.cs:70:28:70:72 | call to method GetBytes : Byte[] |
77
nodes
8-
| HashWithoutSalt.cs:17:70:17:77 | access to parameter password : String | semmle.label | access to parameter password : String |
9-
| HashWithoutSalt.cs:19:49:19:56 | access to local variable passBuff | semmle.label | access to local variable passBuff |
10-
| HashWithoutSalt.cs:37:28:37:72 | call to method GetBytes : Byte[] | semmle.label | call to method GetBytes : Byte[] |
11-
| HashWithoutSalt.cs:37:64:37:71 | access to parameter password : String | semmle.label | access to parameter password : String |
12-
| HashWithoutSalt.cs:38:51:38:59 | access to local variable passBytes | semmle.label | access to local variable passBytes |
13-
| HashWithoutSalt.cs:69:28:69:72 | call to method GetBytes : Byte[] | semmle.label | call to method GetBytes : Byte[] |
14-
| HashWithoutSalt.cs:69:64:69:71 | access to parameter password : String | semmle.label | access to parameter password : String |
15-
| HashWithoutSalt.cs:70:48:70:56 | access to local variable passBytes | semmle.label | access to local variable passBytes |
8+
| HashWithoutSalt.cs:18:70:18:77 | access to parameter password : String | semmle.label | access to parameter password : String |
9+
| HashWithoutSalt.cs:20:49:20:56 | access to local variable passBuff | semmle.label | access to local variable passBuff |
10+
| HashWithoutSalt.cs:38:28:38:72 | call to method GetBytes : Byte[] | semmle.label | call to method GetBytes : Byte[] |
11+
| HashWithoutSalt.cs:38:64:38:71 | access to parameter password : String | semmle.label | access to parameter password : String |
12+
| HashWithoutSalt.cs:39:51:39:59 | access to local variable passBytes | semmle.label | access to local variable passBytes |
13+
| HashWithoutSalt.cs:70:28:70:72 | call to method GetBytes : Byte[] | semmle.label | call to method GetBytes : Byte[] |
14+
| HashWithoutSalt.cs:70:64:70:71 | access to parameter password : String | semmle.label | access to parameter password : String |
15+
| HashWithoutSalt.cs:71:48:71:56 | access to local variable passBytes | semmle.label | access to local variable passBytes |
1616
#select
17-
| HashWithoutSalt.cs:19:49:19:56 | access to local variable passBuff | HashWithoutSalt.cs:17:70:17:77 | access to parameter password : String | HashWithoutSalt.cs:19:49:19:56 | access to local variable passBuff | $@ is hashed without a salt. | HashWithoutSalt.cs:17:70:17:77 | access to parameter password | The password |
18-
| HashWithoutSalt.cs:38:51:38:59 | access to local variable passBytes | HashWithoutSalt.cs:37:64:37:71 | access to parameter password : String | HashWithoutSalt.cs:38:51:38:59 | access to local variable passBytes | $@ is hashed without a salt. | HashWithoutSalt.cs:37:64:37:71 | access to parameter password | The password |
19-
| HashWithoutSalt.cs:70:48:70:56 | access to local variable passBytes | HashWithoutSalt.cs:69:64:69:71 | access to parameter password : String | HashWithoutSalt.cs:70:48:70:56 | access to local variable passBytes | $@ is hashed without a salt. | HashWithoutSalt.cs:69:64:69:71 | access to parameter password | The password |
17+
| HashWithoutSalt.cs:20:49:20:56 | access to local variable passBuff | HashWithoutSalt.cs:18:70:18:77 | access to parameter password : String | HashWithoutSalt.cs:20:49:20:56 | access to local variable passBuff | $@ is hashed without a salt. | HashWithoutSalt.cs:18:70:18:77 | access to parameter password | The password |
18+
| HashWithoutSalt.cs:39:51:39:59 | access to local variable passBytes | HashWithoutSalt.cs:38:64:38:71 | access to parameter password : String | HashWithoutSalt.cs:39:51:39:59 | access to local variable passBytes | $@ is hashed without a salt. | HashWithoutSalt.cs:38:64:38:71 | access to parameter password | The password |
19+
| HashWithoutSalt.cs:71:48:71:56 | access to local variable passBytes | HashWithoutSalt.cs:70:64:70:71 | access to parameter password : String | HashWithoutSalt.cs:71:48:71:56 | access to local variable passBytes | $@ is hashed without a salt. | HashWithoutSalt.cs:70:64:70:71 | access to parameter password | The password |

‎csharp/ql/test/experimental/Security Features/CWE-759/Sha1Utils.cs

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
using System;
2+
using System.Security.Cryptography;
3+
using System.Text;
4+
5+
internal static class Sha1Utils
6+
{
7+
public static byte[] Hash(string str)
8+
{
9+
var bytes = str == null ? new byte[0] : Encoding.UTF8.GetBytes(str);
10+
11+
return Hash(bytes);
12+
}
13+
14+
public static byte[] Hash(byte[] bytes)
15+
{
16+
var sha1 = SHA1.Create();
17+
var hashBytes = sha1.ComputeHash(bytes);
18+
19+
return hashBytes;
20+
}
21+
22+
public static string HexStringFromBytes(byte[] bytes)
23+
{
24+
var sb = new StringBuilder();
25+
foreach (var b in bytes)
26+
{
27+
var hex = b.ToString("x2");
28+
sb.Append(hex);
29+
}
30+
return sb.ToString();
31+
}
32+
33+
public static byte[] Hash(byte[] salt, byte[] str)
34+
{
35+
var salted = new byte[salt.Length + str.Length];
36+
Array.Copy(salt, salted, salt.Length);
37+
Array.Copy(str, 0, salted, salt.Length, str.Length);
38+
39+
return Hash(salted);
40+
}
41+
42+
public static byte[] Xor(byte[] array1, byte[] array2)
43+
{
44+
var result = new byte[array1.Length];
45+
46+
for (int i = 0; i < array1.Length; i++)
47+
{
48+
result[i] = (byte)(array1[i] ^ array2[i]);
49+
}
50+
51+
return result;
52+
}
53+
}

0 commit comments

Comments
 (0)