Skip to content

Commit 6a6727f

Browse files
luchua-bcluchua-bc
committed
Reduce the scope of the query to reduce FPs
1 parent ff1ed3a commit 6a6727f

File tree

4 files changed

+125
-91
lines changed

4 files changed

+125
-91
lines changed

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

Lines changed: 85 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,8 @@
88
*/
99

1010
import java
11-
import semmle.code.java.dataflow.DataFlow3
1211
import semmle.code.java.dataflow.TaintTracking
1312
import semmle.code.java.dataflow.TaintTracking2
14-
import semmle.code.java.dataflow.TaintTracking3
1513
import DataFlow::PathGraph
1614

1715
/** The Java class `java.security.MessageDigest`. */
@@ -44,38 +42,91 @@ class MDUpdateMethod extends Method {
4442
}
4543
}
4644

45+
/** The hashing method that could taint the input. */
46+
class MDHashMethodAccess extends MethodAccess {
47+
MDHashMethodAccess() {
48+
(
49+
this.getMethod() instanceof MDDigestMethod or
50+
this.getMethod() instanceof MDUpdateMethod
51+
) and
52+
this.getNumArgument() != 0
53+
}
54+
}
55+
4756
/** Gets a regular expression for matching common names of variables that indicate the value being held is a password. */
4857
string getPasswordRegex() { result = "(?i).*pass(wd|word|code|phrase).*" }
4958

5059
/** Finds variables that hold password information judging by their names. */
5160
class PasswordVarExpr extends Expr {
5261
PasswordVarExpr() {
53-
exists(Variable v | this = v.getAnAccess() |
54-
(
55-
v.getName().toLowerCase().regexpMatch(getPasswordRegex()) and
56-
not v.getName().toLowerCase().matches("%hash%") // Exclude variable names such as `passwordHash` since their values were already hashed
57-
)
58-
)
62+
this.(VarAccess).getVariable().getName().toLowerCase().regexpMatch(getPasswordRegex()) and
63+
not this.(VarAccess).getVariable().getName().toLowerCase().matches("%hash%") // Exclude variable names such as `passwordHash` since their values were already hashed
5964
}
6065
}
6166

62-
/** 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. */
63-
class PasswordHashConfiguration extends TaintTracking3::Configuration {
64-
PasswordHashConfiguration() { this = "PasswordHashConfiguration" }
65-
66-
override predicate isSource(DataFlow3::Node source) { source.asExpr() instanceof PasswordVarExpr }
67-
68-
override predicate isSink(DataFlow3::Node sink) {
69-
exists(
70-
MethodAccess ma // invoke `md.update(password)` without the call of `md.update(digest)`
71-
|
72-
sink.asExpr() = ma.getArgument(0) and
73-
(
74-
ma.getMethod() instanceof MDUpdateMethod or // md.update(password)
75-
ma.getMethod() instanceof MDDigestMethod // md.digest(password)
76-
)
67+
/** Holds if `Expr` e is an operand of `AddExpr`. */
68+
predicate hasAddExpr(AddExpr ae, Expr e) {
69+
ae.getAnOperand() = e or
70+
hasAddExpr(ae.getAnOperand(), e)
71+
}
72+
73+
/** Holds if `MethodAccess` ma has a flow to another `MDHashMethodAccess` call. */
74+
predicate hasAnotherHashCall(MethodAccess ma) {
75+
exists(MethodAccess ma2, DataFlow2::Node node1, DataFlow2::Node node2 |
76+
ma2 instanceof MDHashMethodAccess and
77+
ma2 != ma and
78+
node1.asExpr() = ma.getAChildExpr() and
79+
node2.asExpr() = ma2.getAChildExpr() and
80+
(
81+
TaintTracking2::localTaint(node1, node2) or
82+
TaintTracking2::localTaint(node2, node1)
83+
)
84+
)
85+
}
86+
87+
/** Holds if `MethodAccess` ma is a hashing call without a sibling node making another hashing call. */
88+
predicate isSingleHashMethodCall(MethodAccess ma) {
89+
(
90+
ma instanceof MDHashMethodAccess and
91+
not hasAnotherHashCall(ma)
92+
)
93+
}
94+
95+
/** Holds if `MethodAccess` ma is invoked by `MethodAccess` ma2 either directly or indirectly. */
96+
predicate hasParentCall(MethodAccess ma2, MethodAccess ma) {
97+
ma.getCaller() = ma2.getMethod() and
98+
not ma2 instanceof MDHashMethodAccess
99+
or
100+
exists(MethodAccess ma3 |
101+
ma.getCaller() = ma3.getMethod() and
102+
not ma3 instanceof MDHashMethodAccess and
103+
hasParentCall(ma2, ma3)
104+
)
105+
}
106+
107+
/** Holds if `MethodAccess` is a single hashing call. */
108+
predicate isSink(MethodAccess ma) {
109+
isSingleHashMethodCall(ma) and
110+
not exists(MethodAccess ma2 | hasParentCall(ma2, ma))
111+
}
112+
113+
/** Sink of hashing calls. */
114+
class HashWithoutSaltSink extends DataFlow::ExprNode {
115+
HashWithoutSaltSink() {
116+
exists(MethodAccess ma |
117+
this.asExpr() = ma.getAnArgument() and
118+
isSink(ma)
77119
)
78120
}
121+
}
122+
123+
/** 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. */
124+
class HashWithoutSaltConfiguration extends TaintTracking::Configuration {
125+
HashWithoutSaltConfiguration() { this = "HashWithoutSaltConfiguration" }
126+
127+
override predicate isSource(DataFlow::Node source) { source.asExpr() instanceof PasswordVarExpr }
128+
129+
override predicate isSink(DataFlow::Node sink) { sink instanceof HashWithoutSaltSink }
79130

80131
/**
81132
* Holds if a password is concatenated with a salt then hashed together through the call `System.arraycopy(password.getBytes(), ...)`, for example,
@@ -84,60 +135,26 @@ class PasswordHashConfiguration extends TaintTracking3::Configuration {
84135
* `byte[] messageDigest = md.digest(allBytes);`
85136
* Or the password is concatenated with a salt as a string.
86137
*/
87-
override predicate isSanitizer(DataFlow3::Node node) {
138+
override predicate isSanitizer(DataFlow::Node node) {
88139
exists(MethodAccess ma |
89140
ma.getMethod().getDeclaringType().hasQualifiedName("java.lang", "System") and
90141
ma.getMethod().hasName("arraycopy") and
91142
ma.getArgument(0) = node.asExpr()
92143
) // System.arraycopy(password.getBytes(), ...)
93144
or
94-
exists(AddExpr e | node.asExpr() = e.getAnOperand()) // password+salt
95-
}
96-
}
97-
98-
class PasswordDigestConfiguration extends TaintTracking2::Configuration {
99-
PasswordDigestConfiguration() { this = "PasswordDigestConfiguration" }
100-
101-
override predicate isSource(DataFlow2::Node source) {
102-
exists(MDConstructor mc | source.asExpr() = mc)
103-
}
104-
105-
override predicate isSink(DataFlow2::Node sink) {
106-
exists(MethodAccess ma |
107-
(
108-
ma.getMethod() instanceof MDUpdateMethod or
109-
ma.getMethod() instanceof MDDigestMethod
110-
) and
111-
exists(PasswordHashConfiguration cc | cc.hasFlowToExpr(ma.getAnArgument())) and
112-
sink.asExpr() = ma.getQualifier()
113-
)
114-
}
115-
}
116-
117-
class HashWithoutSaltConfiguration extends TaintTracking::Configuration {
118-
HashWithoutSaltConfiguration() { this = "HashWithoutSaltConfiguration" }
119-
120-
override predicate isSource(DataFlow::Node source) {
121-
exists(PasswordDigestConfiguration pc | pc.hasFlow(source, _))
122-
}
123-
124-
override predicate isSink(DataFlow::Node sink) {
145+
exists(AddExpr e | hasAddExpr(e, node.asExpr())) // password+salt
146+
or
147+
exists(ConditionalExpr ce | ce = node.asExpr()) // useSalt?password+":"+salt:password
148+
or
125149
exists(MethodAccess ma |
126-
ma.getMethod() instanceof MDDigestMethod and // md.digest(password)
127-
sink.asExpr() = ma.getQualifier()
150+
ma.getMethod().getDeclaringType().hasQualifiedName("java.lang", "StringBuilder") and
151+
ma.getMethod().hasName("append") and
152+
ma.getArgument(0) = node.asExpr() // stringBuilder.append(password).append(salt)
128153
)
129-
}
130-
131-
/** Holds if `md.update` or `md.digest` calls integrate something other than the password, perhaps a salt. */
132-
override predicate isSanitizer(DataFlow::Node node) {
154+
or
133155
exists(MethodAccess ma |
134-
(
135-
ma.getMethod() instanceof MDUpdateMethod
136-
or
137-
ma.getMethod() instanceof MDDigestMethod and ma.getNumArgument() != 0
138-
) and
139-
node.asExpr() = ma.getQualifier() and
140-
not exists(PasswordHashConfiguration cc | cc.hasFlowToExpr(ma.getAnArgument()))
156+
ma.getQualifier().(VarAccess).getVariable().getType() instanceof Interface and
157+
ma.getAnArgument() = node.asExpr() // Method access of interface type variables requires runtime determination
141158
)
142159
}
143160
}

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

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,11 @@
11
edges
22
| HashWithoutSalt.java:10:36:10:43 | password : String | HashWithoutSalt.java:10:36:10:54 | getBytes(...) |
33
| HashWithoutSalt.java:17:13:17:20 | password : String | HashWithoutSalt.java:17:13:17:31 | getBytes(...) |
4-
| HashWithoutSalt.java:98:22:98:29 | password : String | HashWithoutSalt.java:99:17:99:25 | passBytes : byte[] |
5-
| HashWithoutSalt.java:99:17:99:25 | passBytes : byte[] | SHA256.java:14:22:14:31 | foo : byte[] |
6-
| SHA256.java:14:22:14:31 | foo : byte[] | SHA256.java:15:15:15:17 | foo |
74
nodes
85
| HashWithoutSalt.java:10:36:10:43 | password : String | semmle.label | password : String |
96
| HashWithoutSalt.java:10:36:10:54 | getBytes(...) | semmle.label | getBytes(...) |
107
| HashWithoutSalt.java:17:13:17:20 | password : String | semmle.label | password : String |
118
| HashWithoutSalt.java:17:13:17:31 | getBytes(...) | semmle.label | getBytes(...) |
12-
| HashWithoutSalt.java:98:22:98:29 | password : String | semmle.label | password : String |
13-
| HashWithoutSalt.java:99:17:99:25 | passBytes : byte[] | semmle.label | passBytes : byte[] |
14-
| SHA256.java:14:22:14:31 | foo : byte[] | semmle.label | foo : byte[] |
15-
| SHA256.java:15:15:15:17 | foo | semmle.label | foo |
169
#select
17-
| 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 | The password |
18-
| 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 | The password |
19-
| SHA256.java:15:15:15:17 | foo | HashWithoutSalt.java:98:22:98:29 | password : String | SHA256.java:15:15:15:17 | foo | $@ is hashed without a salt. | HashWithoutSalt.java:98:22:98:29 | password | The password |
10+
| 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 |

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

Lines changed: 17 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,12 @@ public String getSHA256Hash3(String password, byte[] salt) throws NoSuchAlgorith
5454

5555
// GOOD - Hash with a given salt stored somewhere else.
5656
public String getSHA256Hash(String password, String salt) throws NoSuchAlgorithmException {
57-
return hash(password+salt);
57+
return hash(password+":"+salt);
58+
}
59+
60+
// GOOD - Hash with a given salt stored somewhere else.
61+
public String getSHA256Hash2(String password, String salt, boolean useSalt) throws NoSuchAlgorithmException {
62+
return hash(useSalt?password+":"+salt:password);
5863
}
5964

6065
// GOOD - Hash with a salt for a variable named passwordHash, whose value is a hash used as an input for a hashing function.
@@ -71,9 +76,9 @@ public void update(SHA256 sha256, byte[] foo, int start, int len) throws NoSuchA
7176
public void update2(SHA256 sha256, byte[] foo, int start, int len) throws NoSuchAlgorithmException {
7277
sha256.update(foo, start, len);
7378
}
74-
75-
// GOOD - Invoke a wrapper implementation with a salt.
76-
public String getSHA256Hash4(String password) throws NoSuchAlgorithmException {
79+
80+
// BAD - Invoking a wrapper implementation without a salt is not detected.
81+
public String getSHA256Hash4(String password) throws NoSuchAlgorithmException, ClassNotFoundException, IllegalAccessException, InstantiationException {
7782
SHA256 sha256 = new SHA256();
7883
byte[] salt = getSalt();
7984
byte[] passBytes = password.getBytes();
@@ -82,7 +87,7 @@ public String getSHA256Hash4(String password) throws NoSuchAlgorithmException {
8287
return Base64.getEncoder().encodeToString(sha256.digest());
8388
}
8489

85-
// GOOD - Invoke a wrapper implementation with a salt.
90+
// BAD - Invoking a wrapper implementation without a salt is not detected.
8691
public String getSHA256Hash5(String password) throws NoSuchAlgorithmException {
8792
SHA256 sha256 = new SHA256();
8893
byte[] salt = getSalt();
@@ -92,26 +97,25 @@ public String getSHA256Hash5(String password) throws NoSuchAlgorithmException {
9297
return Base64.getEncoder().encodeToString(sha256.digest());
9398
}
9499

95-
// BAD - Invoke a wrapper implementation without a salt.
100+
// BAD - Invoking a wrapper implementation without a salt is not detected.
96101
public String getSHA256Hash6(String password) throws NoSuchAlgorithmException {
97102
SHA256 sha256 = new SHA256();
98103
byte[] passBytes = password.getBytes();
99104
sha256.update(passBytes, 0, passBytes.length);
100105
return Base64.getEncoder().encodeToString(sha256.digest());
101106
}
102107

103-
// GOOD - Invoke a wrapper implementation with a salt, which is only detectable when a class type is declared (not interface).
108+
// BAD - Invoke a wrapper implementation with a salt, which is not detected with an interface type variable.
104109
public String getSHA256Hash7(byte[] passphrase) throws NoSuchAlgorithmException, ClassNotFoundException, IllegalAccessException, InstantiationException {
105-
Class c = Class.forName("SHA256");
106-
HASH sha256 = (HASH) (c.newInstance());
110+
Class c = Class.forName("SHA512");
111+
HASH sha512 = (HASH) (c.newInstance());
107112
byte[] tmp = new byte[4];
108113
byte[] key = new byte[32 * 2];
109114
for (int i = 0; i < 2; i++) {
110-
sha256.init();
115+
sha512.init();
111116
tmp[3] = (byte) i;
112-
sha256.update(tmp, 0, tmp.length);
113-
sha256.update(passphrase, 0, passphrase.length);
114-
System.arraycopy(sha256.digest(), 0, key, i * 32, 32);
117+
sha512.update(passphrase, 0, passphrase.length);
118+
System.arraycopy(sha512.digest(), 0, key, i * 32, 32);
115119
}
116120
return Base64.getEncoder().encodeToString(key);
117121
}

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

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
import java.security.MessageDigest;
2+
import java.security.NoSuchAlgorithmException;
3+
4+
public class SHA512 implements HASH {
5+
MessageDigest md;
6+
public int getBlockSize() {return 32;}
7+
public void init() throws NoSuchAlgorithmException {
8+
try { md = MessageDigest.getInstance("SHA-512"); }
9+
catch (Exception e){
10+
System.err.println(e);
11+
}
12+
}
13+
14+
public void update(byte[] foo, int start, int len) throws NoSuchAlgorithmException {
15+
md.update(foo, start, len);
16+
}
17+
18+
public byte[] digest() throws NoSuchAlgorithmException {
19+
return md.digest();
20+
}
21+
}

0 commit comments

Comments
 (0)