Skip to content

Commit 0ef3eee

Browse files
luchua-bcluchua-bc
committed
Revamp the source and the sink of the query
1 parent 1784c20 commit 0ef3eee

File tree

3 files changed

+120
-49
lines changed

3 files changed

+120
-49
lines changed

‎java/ql/src/experimental/Security/CWE/CWE-759/HashWithoutSalt.ql

Lines changed: 72 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,26 @@
99

1010
import java
1111
import semmle.code.java.dataflow.TaintTracking
12-
import semmle.code.java.dataflow.TaintTracking2
1312
import DataFlow::PathGraph
1413

14+
/**
15+
* Gets a regular expression for matching common names of variables
16+
* that indicate the value being held is a password.
17+
*/
18+
string getPasswordRegex() { result = "(?i).*pass(wd|word|code|phrase).*" }
19+
20+
/** Finds variables that hold password information judging by their names. */
21+
class PasswordVarExpr extends VarAccess {
22+
PasswordVarExpr() {
23+
exists(string name | name = this.getVariable().getName().toLowerCase() |
24+
name.regexpMatch(getPasswordRegex()) and not name.matches("%hash%") // Exclude variable names such as `passwordHash` since their values were already hashed
25+
)
26+
}
27+
}
28+
29+
/** Holds if `Expr` e is a direct or indirect operand of `ae`. */
30+
predicate hasAddExprAncestor(AddExpr ae, Expr e) { ae.getAnOperand+() = e }
31+
1532
/** The Java class `java.security.MessageDigest`. */
1633
class MessageDigest extends RefType {
1734
MessageDigest() { this.hasQualifiedName("java.security", "MessageDigest") }
@@ -54,41 +71,66 @@ class MDHashMethodAccess extends MethodAccess {
5471
}
5572
}
5673

57-
/** Gets a regular expression for matching common names of variables that indicate the value being held is a password. */
58-
string getPasswordRegex() { result = "(?i).*pass(wd|word|code|phrase).*" }
74+
/**
75+
* Holds if `MethodAccess` ma is a method access of `MDHashMethodAccess` or
76+
* invokes a method access of `MDHashMethodAccess` directly or indirectly.
77+
*/
78+
predicate isHashAccess(MethodAccess ma) {
79+
ma instanceof MDHashMethodAccess
80+
or
81+
exists(MethodAccess mca |
82+
ma.getMethod().calls(mca.getMethod()) and
83+
isHashAccess(mca) and
84+
DataFlow::localExprFlow(ma.getMethod().getAParameter().getAnAccess(), mca.getAnArgument())
85+
)
86+
}
5987

60-
/** Finds variables that hold password information judging by their names. */
61-
class PasswordVarExpr extends VarAccess {
62-
PasswordVarExpr() {
63-
exists(string name | name = this.getVariable().getName().toLowerCase() |
64-
name.regexpMatch(getPasswordRegex()) and not name.matches("%hash%") // Exclude variable names such as `passwordHash` since their values were already hashed
88+
/**
89+
* Holds if there is a second method access that satisfies `isHashAccess` whose qualifier or argument
90+
* is the same as the method call `ma` that satisfies `isHashAccess`.
91+
*/
92+
predicate hasAnotherHashCall(MethodAccess ma) {
93+
isHashAccess(ma) and
94+
exists(MethodAccess ma2, VarAccess va |
95+
ma2 != ma and
96+
isHashAccess(ma2) and
97+
not va.getVariable().getType() instanceof PrimitiveType and
98+
(
99+
ma.getQualifier() = va and
100+
ma2.getQualifier() = va.getVariable().getAnAccess()
101+
or
102+
ma.getQualifier() = va and
103+
ma2.getAnArgument() = va.getVariable().getAnAccess()
104+
or
105+
ma.getAnArgument() = va and
106+
ma2.getQualifier() = va.getVariable().getAnAccess()
107+
or
108+
ma.getAnArgument() = va and
109+
ma2.getAnArgument() = va.getVariable().getAnAccess()
65110
)
66-
}
111+
)
67112
}
68113

69-
/** Holds if `Expr` e is a direct or indirect operand of `ae`. */
70-
predicate hasAddExprAncestor(AddExpr ae, Expr e) { ae.getAnOperand+() = e }
71-
72-
/** Holds if `MDHashMethodAccess ma` is a second `MDHashMethodAccess` call by the same object. */
73-
predicate hasAnotherHashCall(MDHashMethodAccess ma) {
74-
exists(MDHashMethodAccess ma2 |
75-
ma2 != ma and
76-
ma2.getQualifier() = ma.getQualifier().(VarAccess).getVariable().getAnAccess()
114+
/**
115+
* Holds if `MethodAccess` ma is part of a call graph that satisfies `isHashAccess`
116+
* but is not at the top of the call hierarchy.
117+
*/
118+
predicate hasHashAncestor(MethodAccess ma) {
119+
exists(MethodAccess mpa |
120+
mpa.getMethod().calls(ma.getMethod()) and
121+
isHashAccess(mpa) and
122+
DataFlow::localExprFlow(mpa.getMethod().getAParameter().getAnAccess(), ma.getAnArgument())
77123
)
78124
}
79125

80126
/** Holds if `MethodAccess` ma is a hashing call without a sibling node making another hashing call. */
81-
predicate isSingleHashMethodCall(MDHashMethodAccess ma) { not hasAnotherHashCall(ma) }
82-
83-
/** Holds if `MethodAccess` ma is invoked by `MethodAccess` ma2 either directly or indirectly. */
84-
predicate hasParentCall(MethodAccess ma2, MethodAccess ma) { ma.getCaller() = ma2.getMethod() }
85-
86-
/** Holds if `MethodAccess` is a single hashing call that is not invoked by a wrapper method. */
87-
predicate isSink(MethodAccess ma) {
88-
isSingleHashMethodCall(ma) and
89-
not hasParentCall(_, ma) // Not invoked by a wrapper method which could invoke MDHashMethod in another call stack. This reduces FPs.
127+
predicate isSingleHashMethodCall(MethodAccess ma) {
128+
isHashAccess(ma) and not hasAnotherHashCall(ma)
90129
}
91130

131+
/** Holds if `MethodAccess` ma is a single hashing call that is not invoked by a wrapper method. */
132+
predicate isSink(MethodAccess ma) { isSingleHashMethodCall(ma) and not hasHashAncestor(ma) }
133+
92134
/** Sink of hashing calls. */
93135
class HashWithoutSaltSink extends DataFlow::ExprNode {
94136
HashWithoutSaltSink() {
@@ -99,7 +141,10 @@ class HashWithoutSaltSink extends DataFlow::ExprNode {
99141
}
100142
}
101143

102-
/** 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. */
144+
/**
145+
* Taint configuration tracking flow from an expression whose name suggests it holds password data
146+
* to a method call that generates a hash without a salt.
147+
*/
103148
class HashWithoutSaltConfiguration extends TaintTracking::Configuration {
104149
HashWithoutSaltConfiguration() { this = "HashWithoutSaltConfiguration" }
105150

‎java/ql/test/experimental/query-tests/security/CWE-759/HashWithoutSalt.expected

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,19 @@
11
edges
22
| HashWithoutSalt.java:10:36:10:43 | password : String | HashWithoutSalt.java:10:36:10:54 | getBytes(...) |
3-
| HashWithoutSalt.java:17:13:17:20 | password : String | HashWithoutSalt.java:17:13:17:31 | getBytes(...) |
3+
| HashWithoutSalt.java:25:13:25:20 | password : String | HashWithoutSalt.java:25:13:25:31 | getBytes(...) |
4+
| HashWithoutSalt.java:93:22:93:29 | password : String | HashWithoutSalt.java:94:17:94:25 | passBytes |
5+
| HashWithoutSalt.java:111:22:111:29 | password : String | HashWithoutSalt.java:112:18:112:26 | passBytes |
46
nodes
57
| HashWithoutSalt.java:10:36:10:43 | password : String | semmle.label | password : String |
68
| HashWithoutSalt.java:10:36:10:54 | getBytes(...) | semmle.label | getBytes(...) |
7-
| HashWithoutSalt.java:17:13:17:20 | password : String | semmle.label | password : String |
8-
| HashWithoutSalt.java:17:13:17:31 | getBytes(...) | semmle.label | getBytes(...) |
9+
| HashWithoutSalt.java:25:13:25:20 | password : String | semmle.label | password : String |
10+
| HashWithoutSalt.java:25:13:25:31 | getBytes(...) | semmle.label | getBytes(...) |
11+
| HashWithoutSalt.java:93:22:93:29 | password : String | semmle.label | password : String |
12+
| HashWithoutSalt.java:94:17:94:25 | passBytes | semmle.label | passBytes |
13+
| HashWithoutSalt.java:111:22:111:29 | password : String | semmle.label | password : String |
14+
| HashWithoutSalt.java:112:18:112:26 | passBytes | semmle.label | passBytes |
915
#select
1016
| HashWithoutSalt.java:10:36:10:54 | getBytes(...) | HashWithoutSalt.java:10:36:10:43 | password : String | HashWithoutSalt.java:10:36:10:54 | getBytes(...) | $@ is hashed without a salt. | HashWithoutSalt.java:10:36:10:43 | password : String | The password |
11-
| HashWithoutSalt.java:17:13:17:31 | getBytes(...) | HashWithoutSalt.java:17:13:17:20 | password : String | HashWithoutSalt.java:17:13:17:31 | getBytes(...) | $@ is hashed without a salt. | HashWithoutSalt.java:17:13:17:20 | password : String | The password |
17+
| HashWithoutSalt.java:25:13:25:31 | getBytes(...) | HashWithoutSalt.java:25:13:25:20 | password : String | HashWithoutSalt.java:25:13:25:31 | getBytes(...) | $@ is hashed without a salt. | HashWithoutSalt.java:25:13:25:20 | password : String | The password |
18+
| HashWithoutSalt.java:94:17:94:25 | passBytes | HashWithoutSalt.java:93:22:93:29 | password : String | HashWithoutSalt.java:94:17:94:25 | passBytes | $@ is hashed without a salt. | HashWithoutSalt.java:93:22:93:29 | password : String | The password |
19+
| HashWithoutSalt.java:112:18:112:26 | passBytes | HashWithoutSalt.java:111:22:111:29 | password : String | HashWithoutSalt.java:112:18:112:26 | passBytes | $@ is hashed without a salt. | HashWithoutSalt.java:111:22:111:29 | password : String | The password |

‎java/ql/test/experimental/query-tests/security/CWE-759/HashWithoutSalt.java

Lines changed: 36 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -11,14 +11,6 @@ public String getSHA256Hash(String password) throws NoSuchAlgorithmException {
1111
return Base64.getEncoder().encodeToString(messageDigest);
1212
}
1313

14-
// BAD - Hash without a salt.
15-
public String getSHA256Hash2(String password) throws NoSuchAlgorithmException {
16-
MessageDigest md = MessageDigest.getInstance("SHA-256");
17-
md.update(password.getBytes());
18-
byte[] messageDigest = md.digest();
19-
return Base64.getEncoder().encodeToString(messageDigest);
20-
}
21-
2214
// GOOD - Hash with a salt.
2315
public String getSHA256Hash(String password, byte[] salt) throws NoSuchAlgorithmException {
2416
MessageDigest md = MessageDigest.getInstance("SHA-256");
@@ -27,6 +19,14 @@ public String getSHA256Hash(String password, byte[] salt) throws NoSuchAlgorithm
2719
return Base64.getEncoder().encodeToString(messageDigest);
2820
}
2921

22+
// BAD - Hash without a salt.
23+
public String getSHA256Hash2(String password) throws NoSuchAlgorithmException {
24+
MessageDigest md = MessageDigest.getInstance("SHA-256");
25+
md.update(password.getBytes());
26+
byte[] messageDigest = md.digest();
27+
return Base64.getEncoder().encodeToString(messageDigest);
28+
}
29+
3030
// GOOD - Hash with a salt.
3131
public String getSHA256Hash2(String password, byte[] salt) throws NoSuchAlgorithmException {
3232
MessageDigest md = MessageDigest.getInstance("SHA-256");
@@ -77,8 +77,8 @@ public void update(SHA256 sha256, byte[] foo, int start, int len) throws NoSuchA
7777
sha256.update(foo, start, len);
7878
}
7979

80-
// BAD - Invoking a wrapper implementation without a salt is not detected.
81-
public String getSHA256Hash4(String password) throws NoSuchAlgorithmException, ClassNotFoundException, IllegalAccessException, InstantiationException {
80+
// GOOD - Invoking a wrapper implementation through qualifier with a salt.
81+
public String getWrapperSHA256Hash(String password) throws NoSuchAlgorithmException, ClassNotFoundException, IllegalAccessException, InstantiationException {
8282
SHA256 sha256 = new SHA256();
8383
byte[] salt = getSalt();
8484
byte[] passBytes = password.getBytes();
@@ -87,8 +87,16 @@ public String getSHA256Hash4(String password) throws NoSuchAlgorithmException, C
8787
return Base64.getEncoder().encodeToString(sha256.digest());
8888
}
8989

90-
// BAD - Invoking a wrapper implementation without a salt is not detected.
91-
public String getSHA256Hash5(String password) throws NoSuchAlgorithmException {
90+
// BAD - Invoking a wrapper implementation through qualifier without a salt.
91+
public String getWrapperSHA256Hash2(String password) throws NoSuchAlgorithmException, ClassNotFoundException, IllegalAccessException, InstantiationException {
92+
SHA256 sha256 = new SHA256();
93+
byte[] passBytes = password.getBytes();
94+
sha256.update(passBytes, 0, passBytes.length);
95+
return Base64.getEncoder().encodeToString(sha256.digest());
96+
}
97+
98+
// GOOD - Invoking a wrapper implementation through qualifier and argument with a salt.
99+
public String getWrapperSHA256Hash3(String password) throws NoSuchAlgorithmException {
92100
SHA256 sha256 = new SHA256();
93101
byte[] salt = getSalt();
94102
byte[] passBytes = password.getBytes();
@@ -97,16 +105,26 @@ public String getSHA256Hash5(String password) throws NoSuchAlgorithmException {
97105
return Base64.getEncoder().encodeToString(sha256.digest());
98106
}
99107

100-
// BAD - Invoking a wrapper implementation without a salt is not detected.
101-
public String getSHA512Hash6(String password) throws NoSuchAlgorithmException {
102-
SHA512 sha512 = new SHA512();
108+
// BAD - Invoking a wrapper implementation through argument without a salt.
109+
public String getWrapperSHA256Hash4(String password) throws NoSuchAlgorithmException {
110+
SHA256 sha256 = new SHA256();
103111
byte[] passBytes = password.getBytes();
104-
sha512.update(passBytes, 0, passBytes.length);
105-
return Base64.getEncoder().encodeToString(sha512.digest());
112+
update(sha256, passBytes, 0, passBytes.length);
113+
return Base64.getEncoder().encodeToString(sha256.digest());
114+
}
115+
116+
// GOOD - Invoking a wrapper implementation through argument with a salt.
117+
public String getWrapperSHA256Hash5(String password) throws NoSuchAlgorithmException {
118+
SHA256 sha256 = new SHA256();
119+
byte[] salt = getSalt();
120+
byte[] passBytes = password.getBytes();
121+
update(sha256, passBytes, 0, passBytes.length);
122+
update(sha256, salt, 0, salt.length);
123+
return Base64.getEncoder().encodeToString(sha256.digest());
106124
}
107125

108126
// BAD - Invoke a wrapper implementation with a salt, which is not detected with an interface type variable.
109-
public String getSHA512Hash7(byte[] passphrase) throws NoSuchAlgorithmException, ClassNotFoundException, IllegalAccessException, InstantiationException {
127+
public String getSHA512Hash8(byte[] passphrase) throws NoSuchAlgorithmException, ClassNotFoundException, IllegalAccessException, InstantiationException {
110128
Class c = Class.forName("SHA512");
111129
HASH sha512 = (HASH) (c.newInstance());
112130
byte[] tmp = new byte[4];

0 commit comments

Comments
 (0)