From 90d473d88690a8ee52ff74d8e94fff4a13fa1fe8 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 15 May 2020 11:01:27 +0200 Subject: [PATCH 1/4] C++: Demonstrate lack of taint through getdelim --- .../dataflow/taint-tests/localTaint.expected | 9 +++++++++ .../library-tests/dataflow/taint-tests/taint.cpp | 14 ++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected index 2b38860c4534..777dac62808c 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected @@ -591,3 +591,12 @@ | taint.cpp:463:6:463:6 | 0 | taint.cpp:471:7:471:7 | y | | | taint.cpp:468:7:468:7 | ref arg x | taint.cpp:470:7:470:7 | x | | | taint.cpp:468:10:468:10 | ref arg y | taint.cpp:471:7:471:7 | y | | +| taint.cpp:480:26:480:32 | source1 | taint.cpp:483:28:483:34 | source1 | | +| taint.cpp:481:15:481:21 | 0 | taint.cpp:483:12:483:15 | line | | +| taint.cpp:481:15:481:21 | 0 | taint.cpp:485:7:485:10 | line | | +| taint.cpp:482:9:482:9 | n | taint.cpp:483:19:483:19 | n | | +| taint.cpp:483:11:483:15 | ref arg & ... | taint.cpp:483:12:483:15 | line [inner post update] | | +| taint.cpp:483:11:483:15 | ref arg & ... | taint.cpp:485:7:485:10 | line | | +| taint.cpp:483:12:483:15 | line | taint.cpp:483:11:483:15 | & ... | | +| taint.cpp:483:18:483:19 | ref arg & ... | taint.cpp:483:19:483:19 | n [inner post update] | | +| taint.cpp:483:19:483:19 | n | taint.cpp:483:18:483:19 | & ... | | diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp index 5d8327017fa0..3d09f075a40e 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp @@ -470,3 +470,17 @@ void test_swop() { sink(x); // clean [FALSE POSITIVE] sink(y); // tainted } + +// --- getdelim --- + +struct FILE; + +int getdelim(char ** lineptr, size_t * n, int delimiter, FILE *stream); + +void test_getdelim(FILE* source1) { + char* line = nullptr; + size_t n; + getdelim(&line, &n, '\n', source1); + + sink(line); +} \ No newline at end of file From e70f22c753d282967bc79ce66127b59bf8aec200 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 15 May 2020 11:05:57 +0200 Subject: [PATCH 2/4] C++: Model getdelim and friends --- cpp/ql/src/semmle/code/cpp/models/Models.qll | 1 + .../cpp/models/implementations/GetDelim.qll | 40 +++++++++++++++++++ 2 files changed, 41 insertions(+) create mode 100644 cpp/ql/src/semmle/code/cpp/models/implementations/GetDelim.qll diff --git a/cpp/ql/src/semmle/code/cpp/models/Models.qll b/cpp/ql/src/semmle/code/cpp/models/Models.qll index f02d05be7112..82ae1fdc4f0a 100644 --- a/cpp/ql/src/semmle/code/cpp/models/Models.qll +++ b/cpp/ql/src/semmle/code/cpp/models/Models.qll @@ -14,3 +14,4 @@ private import implementations.Strdup private import implementations.Strftime private import implementations.StdString private import implementations.Swap +private import implementations.GetDelim diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/GetDelim.qll b/cpp/ql/src/semmle/code/cpp/models/implementations/GetDelim.qll new file mode 100644 index 000000000000..ada11c86fd2c --- /dev/null +++ b/cpp/ql/src/semmle/code/cpp/models/implementations/GetDelim.qll @@ -0,0 +1,40 @@ +import semmle.code.cpp.models.interfaces.Taint +import semmle.code.cpp.models.interfaces.Alias +import semmle.code.cpp.models.interfaces.SideEffect +import semmle.code.cpp.models.interfaces.FlowSource + +/** + * The standard functions `getdelim`, `getwdelim` and the glibc variant `__getdelim`. + */ +class GetDelimFunction extends TaintFunction, AliasFunction, SideEffectFunction, RemoteFlowFunction { + GetDelimFunction() { hasGlobalName(["getdelim", "getwdelim", "__getdelim"]) } + + override predicate hasTaintFlow(FunctionInput i, FunctionOutput o) { + i.isParameter(3) and o.isParameterDeref(0) + } + + override predicate parameterNeverEscapes(int index) { index = [0, 1, 3] } + + override predicate parameterEscapesOnlyViaReturn(int index) { none() } + + override predicate parameterIsAlwaysReturned(int index) { none() } + + override predicate hasOnlySpecificReadSideEffects() { any() } + + override predicate hasOnlySpecificWriteSideEffects() { any() } + + override predicate hasSpecificWriteSideEffect(ParameterIndex i, boolean buffer, boolean mustWrite) { + i = [0, 1] and + buffer = false and + mustWrite = true + } + + override predicate hasSpecificReadSideEffect(ParameterIndex i, boolean buffer) { + i = 3 and buffer = false + } + + override predicate hasRemoteFlowSource(FunctionOutput output, string description) { + output.isParameterDeref(0) and + description = "String read by " + this.getName() + } +} From 866b1361ecb0b18118c47bc10ebc003c8c0f0c71 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 15 May 2020 11:12:47 +0200 Subject: [PATCH 3/4] C++: Accept tests --- .../test/library-tests/dataflow/taint-tests/localTaint.expected | 1 + cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected | 1 + cpp/ql/test/library-tests/dataflow/taint-tests/test_ir.expected | 1 + 3 files changed, 3 insertions(+) diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected index 777dac62808c..764cc9f24e99 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected @@ -600,3 +600,4 @@ | taint.cpp:483:12:483:15 | line | taint.cpp:483:11:483:15 | & ... | | | taint.cpp:483:18:483:19 | ref arg & ... | taint.cpp:483:19:483:19 | n [inner post update] | | | taint.cpp:483:19:483:19 | n | taint.cpp:483:18:483:19 | & ... | | +| taint.cpp:483:28:483:34 | source1 | taint.cpp:483:11:483:15 | ref arg & ... | TAINT | diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected index 59193d81722b..b9294f5b7ae1 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected @@ -67,3 +67,4 @@ | taint.cpp:465:7:465:7 | x | taint.cpp:462:6:462:11 | call to source | | taint.cpp:470:7:470:7 | x | taint.cpp:462:6:462:11 | call to source | | taint.cpp:471:7:471:7 | y | taint.cpp:462:6:462:11 | call to source | +| taint.cpp:485:7:485:10 | line | taint.cpp:480:26:480:32 | source1 | diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/test_ir.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/test_ir.expected index 3c94aab79db2..63ab09a46823 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/test_ir.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/test_ir.expected @@ -28,3 +28,4 @@ | taint.cpp:430:9:430:14 | member | taint.cpp:428:13:428:18 | call to source | | taint.cpp:465:7:465:7 | x | taint.cpp:462:6:462:11 | call to source | | taint.cpp:470:7:470:7 | x | taint.cpp:462:6:462:11 | call to source | +| taint.cpp:485:7:485:10 | line | taint.cpp:480:26:480:32 | source1 | From 7502c6f82172101f930a511620785f7cc7086a1b Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 15 May 2020 14:32:46 +0200 Subject: [PATCH 4/4] Set `mustWrite` to `false` in response to PR feedback Co-authored-by: Jonas Jensen --- cpp/ql/src/semmle/code/cpp/models/implementations/GetDelim.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/GetDelim.qll b/cpp/ql/src/semmle/code/cpp/models/implementations/GetDelim.qll index ada11c86fd2c..3f376cf2ff01 100644 --- a/cpp/ql/src/semmle/code/cpp/models/implementations/GetDelim.qll +++ b/cpp/ql/src/semmle/code/cpp/models/implementations/GetDelim.qll @@ -26,7 +26,7 @@ class GetDelimFunction extends TaintFunction, AliasFunction, SideEffectFunction, override predicate hasSpecificWriteSideEffect(ParameterIndex i, boolean buffer, boolean mustWrite) { i = [0, 1] and buffer = false and - mustWrite = true + mustWrite = false } override predicate hasSpecificReadSideEffect(ParameterIndex i, boolean buffer) {