Skip to content

Commit a726de1

Browse files
jeongsoolee09jeongsoolee09
authored
Merge pull request #96 from advanced-security/mbaluda/non-cap-sink-tests
Separate CAP from non-CAP alerts
2 parents 19dd204 + 18673dc commit a726de1

File tree

14 files changed

+2104
-2262
lines changed

14 files changed

+2104
-2262
lines changed

‎.github/workflows/javascript.sarif.expected‎

Lines changed: 1832 additions & 2069 deletions
Large diffs are not rendered by default.

‎javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ module CDS {
106106
* ```
107107
* not sure how else to know which service is registering the handler
108108
*/
109-
class RequestSource extends EventPhaseNodeParam {
109+
class RequestSource extends EventPhaseNodeParam, RemoteFlowSource {
110110
RequestSource() {
111111
// TODO : consider - do we need to actually ever know which service the handler is associated to?
112112
exists(UserDefinedApplicationService svc, FunctionNode init |
@@ -119,6 +119,8 @@ module CDS {
119119
this.getEventPhaseNode().getEnclosingFunction() = pa.getFunction()
120120
)
121121
}
122+
123+
override string getSourceType() { result = "CAP request source" }
122124
}
123125

124126
class ApplicationService extends API::Node {

‎javascript/frameworks/cap/src/sqlinjection/SqlInjection.md‎ renamed to ‎javascript/frameworks/cap/src/cqlinjection/CqlInjection.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
# SQL Injection
1+
# CQL query built from user-controlled sources
22

33
If a database query is built from user-provided data without sufficient sanitization, a malicious user may be able to run malicious database queries.
44

‎javascript/frameworks/cap/src/sqlinjection/SqlInjection.ql‎ renamed to ‎javascript/frameworks/cap/src/cqlinjection/CqlInjection.ql‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/**
2-
* @name Database query built from user-controlled sources with additional heuristic sources
3-
* @description Building a database query from user-controlled sources is vulnerable to insertion of
2+
* @name CQL query built from user-controlled sources
3+
* @description Building a CQL query from user-controlled sources is vulnerable to insertion of
44
* malicious code by the user.
55
* @kind path-problem
66
* @problem.severity error
@@ -13,18 +13,18 @@
1313
import javascript
1414
import DataFlow::PathGraph
1515
import semmle.javascript.security.dataflow.SqlInjectionCustomizations::SqlInjection
16-
import advanced_security.javascript.frameworks.cap.CDS
1716
import advanced_security.javascript.frameworks.cap.CQL
1817

19-
class Configuration extends TaintTracking::Configuration {
20-
Configuration() { this = "CapSqlInjection" }
18+
class CqlIConfiguration extends TaintTracking::Configuration {
19+
CqlIConfiguration() { this = "CqlInjection" }
2120

22-
override predicate isSource(DataFlow::Node source) {
23-
source instanceof Source or source instanceof CDS::RequestSource
24-
}
21+
override predicate isSource(DataFlow::Node source) { source instanceof Source }
22+
23+
override predicate isSink(DataFlow::Node sink) { sink instanceof CQL::CQLSink }
2524

26-
override predicate isSink(DataFlow::Node sink) {
27-
sink instanceof Sink or sink instanceof CQL::CQLSink
25+
override predicate isSanitizer(DataFlow::Node node) {
26+
super.isSanitizer(node) or
27+
node instanceof Sanitizer
2828
}
2929

3030
override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
@@ -42,7 +42,7 @@ class Configuration extends TaintTracking::Configuration {
4242
}
4343
}
4444

45-
from Configuration sql, DataFlow::PathNode source, DataFlow::PathNode sink
45+
from CqlIConfiguration sql, DataFlow::PathNode source, DataFlow::PathNode sink
4646
where sql.hasFlowPath(source, sink)
4747
select sink.getNode(), source, sink, "This query depends on a $@.", source.getNode(),
4848
"user-provided value"

‎javascript/frameworks/cap/src/loginjection/LogInjection.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
# Log Injection
1+
# CAP Log Injection
22

33
If unsanitized user input is written to a log entry using the CAP Node.js logging API, a malicious user may be able to forge new log entries.
44

‎javascript/frameworks/cap/src/loginjection/LogInjection.ql‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
/**
2-
* @name Uncontrolled data in logging call
2+
* @name CAP Log injection
33
* @description Building log entries from user-controlled sources is vulnerable to
44
* insertion of forged log entries by a malicious user.
55
* @kind path-problem
66
* @problem.severity error
7-
* @security-severity 7.8
7+
* @security-severity 6.1
88
* @precision medium
99
* @id js/cap-log-injection
1010
* @tags security
@@ -16,16 +16,19 @@ import semmle.javascript.security.dataflow.LogInjectionQuery
1616
import advanced_security.javascript.frameworks.cap.CDS
1717

1818
/**
19-
* A source of remote user controlled input.
19+
* A taint-tracking configuration for untrusted user input used in log entries.
2020
*/
21-
class CapRemoteSource extends Source, CDS::RequestSource { }
21+
class CapLogIConfiguration extends TaintTracking::Configuration {
22+
CapLogIConfiguration() { this = "CapLogInjection" }
2223

23-
/**
24-
* An argument to a logging mechanism.
25-
*/
26-
class CapLoggingSink extends Sink, CDS::CdsLogSink { }
24+
override predicate isSource(DataFlow::Node source) { source instanceof Source }
25+
26+
override predicate isSink(DataFlow::Node sink) { sink instanceof CDS::CdsLogSink }
27+
28+
override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
29+
}
2730

28-
from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
31+
from CapLogIConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink
2932
where config.hasFlowPath(source, sink)
3033
select sink.getNode(), source, sink, "Log entry depends on a $@.", source.getNode(),
3134
"user-provided value"

‎javascript/frameworks/cap/test/queries/cqlinjection/cqlinjection.expected‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
nodes
2+
| cqlinjection.js:7:34:7:36 | req |
3+
| cqlinjection.js:7:34:7:36 | req |
4+
| cqlinjection.js:8:13:8:30 | { book, quantity } |
5+
| cqlinjection.js:8:13:8:41 | book |
6+
| cqlinjection.js:8:15:8:18 | book |
7+
| cqlinjection.js:8:34:8:36 | req |
8+
| cqlinjection.js:8:34:8:41 | req.data |
9+
| cqlinjection.js:12:11:12:56 | query |
10+
| cqlinjection.js:12:19:12:56 | SELECT. ... book}`) |
11+
| cqlinjection.js:12:44:12:55 | `ID=${book}` |
12+
| cqlinjection.js:12:50:12:53 | book |
13+
| cqlinjection.js:13:36:13:40 | query |
14+
| cqlinjection.js:13:36:13:40 | query |
15+
| cqlinjection.js:15:27:15:64 | SELECT. ... book}`) |
16+
| cqlinjection.js:15:27:15:64 | SELECT. ... book}`) |
17+
| cqlinjection.js:15:52:15:63 | `ID=${book}` |
18+
| cqlinjection.js:15:58:15:61 | book |
19+
| cqlinjection.js:17:11:17:57 | query2 |
20+
| cqlinjection.js:17:20:17:57 | SELECT. ... + book) |
21+
| cqlinjection.js:17:45:17:56 | 'ID=' + book |
22+
| cqlinjection.js:17:53:17:56 | book |
23+
| cqlinjection.js:18:37:18:42 | query2 |
24+
| cqlinjection.js:18:37:18:42 | query2 |
25+
| cqlinjection.js:20:27:20:64 | SELECT. ... + book) |
26+
| cqlinjection.js:20:27:20:64 | SELECT. ... + book) |
27+
| cqlinjection.js:20:52:20:63 | 'ID=' + book |
28+
| cqlinjection.js:20:60:20:63 | book |
29+
| cqlinjection.js:27:11:27:62 | cqn |
30+
| cqlinjection.js:27:17:27:62 | CQL`SEL ... + book |
31+
| cqlinjection.js:27:59:27:62 | book |
32+
| cqlinjection.js:28:39:28:41 | cqn |
33+
| cqlinjection.js:28:39:28:41 | cqn |
34+
| cqlinjection.js:30:11:30:60 | cqn1 |
35+
| cqlinjection.js:30:18:30:60 | cds.par ... + book) |
36+
| cqlinjection.js:30:32:30:59 | `SELECT ... + book |
37+
| cqlinjection.js:30:56:30:59 | book |
38+
| cqlinjection.js:31:39:31:42 | cqn1 |
39+
| cqlinjection.js:31:39:31:42 | cqn1 |
40+
edges
41+
| cqlinjection.js:7:34:7:36 | req | cqlinjection.js:8:34:8:36 | req |
42+
| cqlinjection.js:7:34:7:36 | req | cqlinjection.js:8:34:8:36 | req |
43+
| cqlinjection.js:8:13:8:30 | { book, quantity } | cqlinjection.js:8:15:8:18 | book |
44+
| cqlinjection.js:8:13:8:41 | book | cqlinjection.js:12:50:12:53 | book |
45+
| cqlinjection.js:8:13:8:41 | book | cqlinjection.js:15:58:15:61 | book |
46+
| cqlinjection.js:8:13:8:41 | book | cqlinjection.js:17:53:17:56 | book |
47+
| cqlinjection.js:8:13:8:41 | book | cqlinjection.js:20:60:20:63 | book |
48+
| cqlinjection.js:8:13:8:41 | book | cqlinjection.js:27:59:27:62 | book |
49+
| cqlinjection.js:8:13:8:41 | book | cqlinjection.js:30:56:30:59 | book |
50+
| cqlinjection.js:8:15:8:18 | book | cqlinjection.js:8:13:8:41 | book |
51+
| cqlinjection.js:8:34:8:36 | req | cqlinjection.js:8:34:8:41 | req.data |
52+
| cqlinjection.js:8:34:8:41 | req.data | cqlinjection.js:8:13:8:30 | { book, quantity } |
53+
| cqlinjection.js:12:11:12:56 | query | cqlinjection.js:13:36:13:40 | query |
54+
| cqlinjection.js:12:11:12:56 | query | cqlinjection.js:13:36:13:40 | query |
55+
| cqlinjection.js:12:19:12:56 | SELECT. ... book}`) | cqlinjection.js:12:11:12:56 | query |
56+
| cqlinjection.js:12:44:12:55 | `ID=${book}` | cqlinjection.js:12:19:12:56 | SELECT. ... book}`) |
57+
| cqlinjection.js:12:50:12:53 | book | cqlinjection.js:12:44:12:55 | `ID=${book}` |
58+
| cqlinjection.js:15:52:15:63 | `ID=${book}` | cqlinjection.js:15:27:15:64 | SELECT. ... book}`) |
59+
| cqlinjection.js:15:52:15:63 | `ID=${book}` | cqlinjection.js:15:27:15:64 | SELECT. ... book}`) |
60+
| cqlinjection.js:15:58:15:61 | book | cqlinjection.js:15:52:15:63 | `ID=${book}` |
61+
| cqlinjection.js:17:11:17:57 | query2 | cqlinjection.js:18:37:18:42 | query2 |
62+
| cqlinjection.js:17:11:17:57 | query2 | cqlinjection.js:18:37:18:42 | query2 |
63+
| cqlinjection.js:17:20:17:57 | SELECT. ... + book) | cqlinjection.js:17:11:17:57 | query2 |
64+
| cqlinjection.js:17:45:17:56 | 'ID=' + book | cqlinjection.js:17:20:17:57 | SELECT. ... + book) |
65+
| cqlinjection.js:17:53:17:56 | book | cqlinjection.js:17:45:17:56 | 'ID=' + book |
66+
| cqlinjection.js:20:52:20:63 | 'ID=' + book | cqlinjection.js:20:27:20:64 | SELECT. ... + book) |
67+
| cqlinjection.js:20:52:20:63 | 'ID=' + book | cqlinjection.js:20:27:20:64 | SELECT. ... + book) |
68+
| cqlinjection.js:20:60:20:63 | book | cqlinjection.js:20:52:20:63 | 'ID=' + book |
69+
| cqlinjection.js:27:11:27:62 | cqn | cqlinjection.js:28:39:28:41 | cqn |
70+
| cqlinjection.js:27:11:27:62 | cqn | cqlinjection.js:28:39:28:41 | cqn |
71+
| cqlinjection.js:27:17:27:62 | CQL`SEL ... + book | cqlinjection.js:27:11:27:62 | cqn |
72+
| cqlinjection.js:27:59:27:62 | book | cqlinjection.js:27:17:27:62 | CQL`SEL ... + book |
73+
| cqlinjection.js:30:11:30:60 | cqn1 | cqlinjection.js:31:39:31:42 | cqn1 |
74+
| cqlinjection.js:30:11:30:60 | cqn1 | cqlinjection.js:31:39:31:42 | cqn1 |
75+
| cqlinjection.js:30:18:30:60 | cds.par ... + book) | cqlinjection.js:30:11:30:60 | cqn1 |
76+
| cqlinjection.js:30:32:30:59 | `SELECT ... + book | cqlinjection.js:30:18:30:60 | cds.par ... + book) |
77+
| cqlinjection.js:30:56:30:59 | book | cqlinjection.js:30:32:30:59 | `SELECT ... + book |
78+
#select
79+
| cqlinjection.js:13:36:13:40 | query | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:13:36:13:40 | query | This query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value |
80+
| cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:15:27:15:64 | SELECT. ... book}`) | This query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value |
81+
| cqlinjection.js:18:37:18:42 | query2 | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:18:37:18:42 | query2 | This query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value |
82+
| cqlinjection.js:20:27:20:64 | SELECT. ... + book) | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:20:27:20:64 | SELECT. ... + book) | This query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value |
83+
| cqlinjection.js:28:39:28:41 | cqn | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:28:39:28:41 | cqn | This query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value |
84+
| cqlinjection.js:31:39:31:42 | cqn1 | cqlinjection.js:7:34:7:36 | req | cqlinjection.js:31:39:31:42 | cqn1 | This query depends on a $@. | cqlinjection.js:7:34:7:36 | req | user-provided value |

‎javascript/frameworks/cap/test/queries/cqlinjection/cqlinjection.js‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
import cds from '@sap/cds'
2+
const { Books } = cds.entities('sap.capire.bookshop')
3+
4+
class SampleVulnService extends cds.ApplicationService {
5+
init() {
6+
// contains a sample CQL injection
7+
this.on('submitOrder', async req => {
8+
const { book, quantity } = req.data
9+
10+
let { stock } = await SELECT`stock`.from(Books, book)
11+
12+
let query = SELECT.from`Books`.where(`ID=${book}`)
13+
let books = await cds.db.run(query) // CQL injection alert
14+
15+
let books11 = await SELECT.from`Books`.where(`ID=${book}`) // CQL injection alert
16+
17+
let query2 = SELECT.from`Books`.where('ID=' + book)
18+
let books2 = await cds.db.run(query2) // CQL injection alert
19+
20+
let books22 = await SELECT.from`Books`.where('ID=' + book) // CQL injection alert
21+
22+
let books3 = await SELECT.from`Books`.where`ID=${book}` //safe
23+
24+
let id = 2
25+
let books33 = await SELECT.from`Books`.where('ID=' + id) //safe
26+
27+
let cqn = CQL`SELECT col1, col2, col3 from Books` + book
28+
let books222 = await cds.db.run(cqn) // CQL injection alert
29+
30+
let cqn1 = cds.parse.cql(`SELECT * from Books` + book)
31+
let books111 = await cds.db.run(cqn1) // CQL injection alert
32+
33+
const pg = require("pg"),
34+
pool = new pg.Pool(config);
35+
pool.query(req.params.category, [], function (err, results) { // non-CQL injection alert from CAP source
36+
// process results
37+
});
38+
39+
const app = require("express")();
40+
app.get("search", function handler(req2, res) {
41+
pool.query(req2.params.category, [], function (err, results) { // non-CQL injection alert from non-CAP source
42+
// process results
43+
});
44+
});
45+
46+
return super.init()
47+
})
48+
}
49+
}
50+
export { SampleVulnService }

‎javascript/frameworks/cap/test/queries/cqlinjection/cqlinjection.qlref‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
cqlinjection/CqlInjection.ql

0 commit comments

Comments
 (0)