hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-05-02 04:05:14 +02:00
Code Issues Packages Projects Releases Wiki Activity

remove flowpaths that has a returns without a matching call

Browse Source
This commit is contained in:
Erik Krogh Kristensen
2021-04-26 10:23:27 +02:00
parent 6e754c70aa
commit 23908f9ec2
5 changed files with 50 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files

10
javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstruction.qll
View File

@@ -28,5 +28,15 @@ module UnsafeHtmlConstruction {
override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
DomBasedXss::isOptionallySanitizedEdge(pred, succ)
}
// override to require that there is a path without unmatched return steps
override predicate hasFlowPath(DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink) {
super.hasFlowPath(source, sink) and
requireMatchedReturn(source, sink)
}
override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
classFieldStep(pred, succ)
}
}
}

24
javascript/ql/src/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll
View File

@@ -165,4 +165,28 @@ module UnsafeHtmlConstruction {
override string describe() { result = "Markdown rendering" }
}
/**
* A taint step from the write of a field in a constructor to a read of the same field in an instance method.
*/
predicate classFieldStep(DataFlow::Node pred, DataFlow::Node succ) {
// flow-step from a property written in the constructor to a use in an instance method.
// "simulates" client usage of a class, and regains some flow-steps lost by `requireMatchedReturn` below.
exists(DataFlow::ClassNode clazz, string prop |
DataFlow::thisNode(clazz.getConstructor().getFunction()).getAPropertyWrite(prop).getRhs() =
pred and
DataFlow::thisNode(clazz.getAnInstanceMethod().getFunction()).getAPropertyRead(prop) = succ
)
}
/**
* Holds if there is a path without unmatched return steps from `source` to `sink`.
*/
predicate requireMatchedReturn(DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink) {
exists(DataFlow::MidPathNode mid |
source.getASuccessor*() = mid and
sink = mid.getASuccessor() and
mid.getPathSummary().hasReturn() = false
)
}
}

15
javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/UnsafeHtmlConstruction.expected
View File

@@ -15,6 +15,13 @@ nodes
| main.js:21:47:21:47 | s |
| main.js:22:34:22:34 | s |
| main.js:22:34:22:34 | s |
| main.js:46:17:46:17 | s |
| main.js:47:21:47:21 | s |
| main.js:52:65:52:73 | this.step |
| main.js:52:65:52:73 | this.step |
| main.js:57:41:57:41 | s |
| main.js:57:41:57:41 | s |
| main.js:58:20:58:20 | s |
| typed.ts:1:39:1:39 | s |
| typed.ts:1:39:1:39 | s |
| typed.ts:2:29:2:29 | s |
@@ -47,6 +54,12 @@ edges
| main.js:21:47:21:47 | s | main.js:22:34:22:34 | s |
| main.js:21:47:21:47 | s | main.js:22:34:22:34 | s |
| main.js:21:47:21:47 | s | main.js:22:34:22:34 | s |
| main.js:46:17:46:17 | s | main.js:47:21:47:21 | s |
| main.js:47:21:47:21 | s | main.js:52:65:52:73 | this.step |
| main.js:47:21:47:21 | s | main.js:52:65:52:73 | this.step |
| main.js:57:41:57:41 | s | main.js:58:20:58:20 | s |
| main.js:57:41:57:41 | s | main.js:58:20:58:20 | s |
| main.js:58:20:58:20 | s | main.js:46:17:46:17 | s |
| typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s |
| typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s |
| typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s |
@@ -67,6 +80,6 @@ edges
| main.js:12:49:12:49 | s | main.js:11:60:11:60 | s | main.js:12:49:12:49 | s | $@ based on $@ might later cause $@. | main.js:12:49:12:49 | s | XML parsing | main.js:11:60:11:60 | s | library input | main.js:16:21:16:35 | xml.cloneNode() | cross-site scripting |
| main.js:12:49:12:49 | s | main.js:11:60:11:60 | s | main.js:12:49:12:49 | s | $@ based on $@ might later cause $@. | main.js:12:49:12:49 | s | XML parsing | main.js:11:60:11:60 | s | library input | main.js:17:48:17:50 | tmp | cross-site scripting |
| main.js:22:34:22:34 | s | main.js:21:47:21:47 | s | main.js:22:34:22:34 | s | $@ based on $@ might later cause $@. | main.js:22:34:22:34 | s | Markdown rendering | main.js:21:47:21:47 | s | library input | main.js:23:53:23:56 | html | cross-site scripting |
| main.js:52:65:52:73 | this.step | main.js:57:41:57:41 | s | main.js:52:65:52:73 | this.step | $@ based on $@ might later cause $@. | main.js:52:65:52:73 | this.step | HTML construction | main.js:57:41:57:41 | s | library input | main.js:52:54:52:85 | "<span> ... /span>" | cross-site scripting |
| typed.ts:2:29:2:29 | s | typed.ts:1:39:1:39 | s | typed.ts:2:29:2:29 | s | $@ based on $@ might later cause $@. | typed.ts:2:29:2:29 | s | HTML construction | typed.ts:1:39:1:39 | s | library input | typed.ts:3:31:3:34 | html | cross-site scripting |
| typed.ts:8:40:8:40 | s | typed.ts:6:43:6:43 | s | typed.ts:8:40:8:40 | s | $@ based on $@ might later cause $@. | typed.ts:8:40:8:40 | s | HTML construction | typed.ts:6:43:6:43 | s | library input | typed.ts:8:29:8:52 | "<span> ... /span>" | cross-site scripting |
| typed.ts:17:29:17:29 | s | typed.ts:11:20:11:20 | s | typed.ts:17:29:17:29 | s | $@ based on $@ might later cause $@. | typed.ts:17:29:17:29 | s | HTML construction | typed.ts:11:20:11:20 | s | library input | typed.ts:18:31:18:34 | html | cross-site scripting |

2
javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/main.js
View File

@@ -44,7 +44,7 @@ class Foo {
doXss() {
// not called here, but still bad.
document.querySelector("#class").innerHTML = "<span>" + this.step + "</span>"; // NOT OK - but not flagged [INCONSISTENCY]
document.querySelector("#class").innerHTML = "<span>" + this.step + "</span>"; // NOT OK
}
}

2
javascript/ql/test/query-tests/Security/CWE-079/UnsafeHtmlConstruction/typed.ts
View File

@@ -14,7 +14,7 @@ export function id(s: string) {
export function notVulnerable() {
const s = id("x");
const html = "<span>" + s + "</span>"; // OK - but flagged due to step with unmatched call [INCONSISTENCY]
const html = "<span>" + s + "</span>"; // OK
document.body.innerHTML = html;
}