From 140051cc9ad92c90d59b432ece9d8d57f0b8f709 Mon Sep 17 00:00:00 2001 From: Marc Waldman <54793456+marcrepo@users.noreply.github.com> Date: Fri, 17 Jan 2020 04:38:15 -0500 Subject: [PATCH 01/34] Removed word "file" from description (see Issue 2623) This pull request is in reference to Issue #2623 - "DescriptorNeverClosed.ql identifies only sockets (not file handles)" --- cpp/ql/src/Critical/DescriptorNeverClosed.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/src/Critical/DescriptorNeverClosed.ql b/cpp/ql/src/Critical/DescriptorNeverClosed.ql index f06708f4ae3..ae50e625602 100644 --- a/cpp/ql/src/Critical/DescriptorNeverClosed.ql +++ b/cpp/ql/src/Critical/DescriptorNeverClosed.ql @@ -1,6 +1,6 @@ /** * @name Open descriptor never closed - * @description Functions that always return before closing the socket or file they opened leak resources. + * @description Functions that always return before closing the socket they opened leak resources. * @kind problem * @id cpp/descriptor-never-closed * @problem.severity warning From 0d409b2d69dd0ce70f339223352ca3baf1a5f9ff Mon Sep 17 00:00:00 2001 From: Marc Waldman <54793456+marcrepo@users.noreply.github.com> Date: Fri, 17 Jan 2020 04:46:10 -0500 Subject: [PATCH 02/34] Documentation update for Issue #2623 Changes based on Issue #2623 - DescriptorNeverClosed.ql identifies only sockets (not file handles) --- cpp/ql/src/Critical/DescriptorNeverClosed.qhelp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/ql/src/Critical/DescriptorNeverClosed.qhelp b/cpp/ql/src/Critical/DescriptorNeverClosed.qhelp index 1a14f7981dd..a63c85f3399 100644 --- a/cpp/ql/src/Critical/DescriptorNeverClosed.qhelp +++ b/cpp/ql/src/Critical/DescriptorNeverClosed.qhelp @@ -6,7 +6,7 @@

-This rule finds calls to open or socket where there is no corresponding close call in the program analyzed. +This rule finds calls to socket where there is no corresponding close call in the program analyzed. Leaving descriptors open will cause a resource leak that will persist even after the program terminates.

@@ -14,7 +14,7 @@ Leaving descriptors open will cause a resource leak that will persist even after -

Ensure that all file or socket descriptors allocated by the program are freed before it terminates.

+

Ensure that all socket descriptors allocated by the program are freed before it terminates.

 From 839fd8f848e4336c8301b2a082d220d908b66a99 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 17 Jan 2020 16:01:11 +0000 Subject: [PATCH 03/34] CPP: Fix typo. --- .../code/cpp/ir/implementation/aliased_ssa/Instruction.qll | 2 +- .../src/semmle/code/cpp/ir/implementation/raw/Instruction.qll | 2 +- .../code/cpp/ir/implementation/unaliased_ssa/Instruction.qll | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll index a4a9718ab95..3aa5d80359e 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll @@ -974,7 +974,7 @@ class InheritanceConversionInstruction extends UnaryInstruction { /** * Represents an instruction that converts from the address of a derived class - * to the address of a direct non-virtual base class. + * to the address of a direct base class. */ class ConvertToBaseInstruction extends InheritanceConversionInstruction { ConvertToBaseInstruction() { getOpcode() instanceof ConvertToBaseOpcode } diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll index a4a9718ab95..3aa5d80359e 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll @@ -974,7 +974,7 @@ class InheritanceConversionInstruction extends UnaryInstruction { /** * Represents an instruction that converts from the address of a derived class - * to the address of a direct non-virtual base class. + * to the address of a direct base class. */ class ConvertToBaseInstruction extends InheritanceConversionInstruction { ConvertToBaseInstruction() { getOpcode() instanceof ConvertToBaseOpcode } diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll index a4a9718ab95..3aa5d80359e 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll @@ -974,7 +974,7 @@ class InheritanceConversionInstruction extends UnaryInstruction { /** * Represents an instruction that converts from the address of a derived class - * to the address of a direct non-virtual base class. + * to the address of a direct base class. */ class ConvertToBaseInstruction extends InheritanceConversionInstruction { ConvertToBaseInstruction() { getOpcode() instanceof ConvertToBaseOpcode } From e4139fe42770d636a6eb0f6a7b51a4c66c7663b8 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 17 Jan 2020 17:20:37 +0000 Subject: [PATCH 04/34] Apply suggestions from code review Additional corrections. Co-Authored-By: Dave Bartolomeo --- .../code/cpp/ir/implementation/aliased_ssa/Instruction.qll | 2 +- .../src/semmle/code/cpp/ir/implementation/raw/Instruction.qll | 2 +- .../code/cpp/ir/implementation/unaliased_ssa/Instruction.qll | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll index 3aa5d80359e..1ac811c7eb0 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll @@ -974,7 +974,7 @@ class InheritanceConversionInstruction extends UnaryInstruction { /** * Represents an instruction that converts from the address of a derived class - * to the address of a direct base class. + * to the address of a base class. */ class ConvertToBaseInstruction extends InheritanceConversionInstruction { ConvertToBaseInstruction() { getOpcode() instanceof ConvertToBaseOpcode } diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll index 3aa5d80359e..1ac811c7eb0 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll @@ -974,7 +974,7 @@ class InheritanceConversionInstruction extends UnaryInstruction { /** * Represents an instruction that converts from the address of a derived class - * to the address of a direct base class. + * to the address of a base class. */ class ConvertToBaseInstruction extends InheritanceConversionInstruction { ConvertToBaseInstruction() { getOpcode() instanceof ConvertToBaseOpcode } diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll index 3aa5d80359e..1ac811c7eb0 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll @@ -974,7 +974,7 @@ class InheritanceConversionInstruction extends UnaryInstruction { /** * Represents an instruction that converts from the address of a derived class - * to the address of a direct base class. + * to the address of a base class. */ class ConvertToBaseInstruction extends InheritanceConversionInstruction { ConvertToBaseInstruction() { getOpcode() instanceof ConvertToBaseOpcode } From be68b6f93884eb6c87ae4d431b438328fc4e712b Mon Sep 17 00:00:00 2001 From: Calum Grant Date: Tue, 21 Jan 2020 13:24:48 +0000 Subject: [PATCH 05/34] C#: Add precision to queries --- .../CWE-020/RuntimeChecksBypass.ql | 5 +---- .../Security Features/CWE-091/XMLInjection.ql | 5 +---- .../CWE-114/AssemblyPathInjection.ql | 17 ++--------------- .../CWE-327/InsecureSQLConnection.ql | 7 ++----- 4 files changed, 6 insertions(+), 28 deletions(-) diff --git a/csharp/ql/src/Security Features/CWE-020/RuntimeChecksBypass.ql b/csharp/ql/src/Security Features/CWE-020/RuntimeChecksBypass.ql index e090f3d4b1f..2f2b29d33a6 100644 --- a/csharp/ql/src/Security Features/CWE-020/RuntimeChecksBypass.ql +++ b/csharp/ql/src/Security Features/CWE-020/RuntimeChecksBypass.ql @@ -4,14 +4,11 @@ * @kind problem * @id cs/serialization-check-bypass * @problem.severity warning + * @precision medium * @tags security * external/cwe/cwe-20 */ -/* - * consider: @precision medium - */ - import semmle.code.csharp.serialization.Serialization import semmle.code.csharp.controlflow.Guards diff --git a/csharp/ql/src/Security Features/CWE-091/XMLInjection.ql b/csharp/ql/src/Security Features/CWE-091/XMLInjection.ql index 16e897ab2e9..6b9acf69e3c 100644 --- a/csharp/ql/src/Security Features/CWE-091/XMLInjection.ql +++ b/csharp/ql/src/Security Features/CWE-091/XMLInjection.ql @@ -5,14 +5,11 @@ * @kind problem * @id cs/xml-injection * @problem.severity error + * @precision high * @tags security * external/cwe/cwe-091 */ -/* - * consider: @precision high - */ - import csharp import semmle.code.csharp.dataflow.flowsources.Remote import semmle.code.csharp.frameworks.system.Xml diff --git a/csharp/ql/src/Security Features/CWE-114/AssemblyPathInjection.ql b/csharp/ql/src/Security Features/CWE-114/AssemblyPathInjection.ql index 7adcaa22d09..2b87d61f193 100644 --- a/csharp/ql/src/Security Features/CWE-114/AssemblyPathInjection.ql +++ b/csharp/ql/src/Security Features/CWE-114/AssemblyPathInjection.ql @@ -6,27 +6,14 @@ * @kind problem * @id cs/assembly-path-injection * @problem.severity error + * @precision high * @tags security * external/cwe/cwe-114 */ -/* - * consider: @precision high - */ - import csharp import semmle.code.csharp.dataflow.flowsources.Remote - -class MainMethod extends Method { - MainMethod() { - this.hasName("Main") and - this.isStatic() and - (this.getReturnType() instanceof VoidType or this.getReturnType() instanceof IntType) and - if this.getNumberOfParameters() = 1 - then this.getParameter(0).getType().(ArrayType).getElementType() instanceof StringType - else this.getNumberOfParameters() = 0 - } -} +import semmle.code.csharp.commons.Util /** * A taint-tracking configuration for untrusted user input used to load a DLL. diff --git a/csharp/ql/src/Security Features/CWE-327/InsecureSQLConnection.ql b/csharp/ql/src/Security Features/CWE-327/InsecureSQLConnection.ql index 78bcc1c19e5..b8d407b4eb2 100644 --- a/csharp/ql/src/Security Features/CWE-327/InsecureSQLConnection.ql +++ b/csharp/ql/src/Security Features/CWE-327/InsecureSQLConnection.ql @@ -1,17 +1,14 @@ /** * @name Insecure SQL connection - * @description Using an SQL Server connection without enforcing encryption is a security vulnerability. + * @description Using a SQL Server connection without enforcing encryption is a security vulnerability. * @kind path-problem * @id cs/insecure-sql-connection * @problem.severity error + * @precision medium * @tags security * external/cwe/cwe-327 */ -/* - * consider: @precision high - */ - import csharp import DataFlow::PathGraph From 6692e61fa2e931feba5d10f1daaf059caee37cf2 Mon Sep 17 00:00:00 2001 From: Calum Grant Date: Tue, 21 Jan 2020 13:55:32 +0000 Subject: [PATCH 06/34] C#: Analysis change notes --- change-notes/1.24/analysis-csharp.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/change-notes/1.24/analysis-csharp.md b/change-notes/1.24/analysis-csharp.md index 8ac4656e310..88657dbbc21 100644 --- a/change-notes/1.24/analysis-csharp.md +++ b/change-notes/1.24/analysis-csharp.md @@ -6,8 +6,12 @@ The following changes in version 1.24 affect C# analysis in all applications. | **Query** | **Tags** | **Purpose** | |-----------------------------|-----------|--------------------------------------------------------------------| +| Assembly path injection (`cs/assembly-path-injection`) | security, external/cwe/cwe-114 | Finds user-controlled data used to load an assembly. | | Insecure configuration for ASP.NET requestValidationMode (`cs/insecure-request-validation-mode`) | security, external/cwe/cwe-016 | Finds where this attribute has been set to a value less than 4.5, which turns off some validation features and makes the application less secure. | +| Insecure SQL connection (`cs/insecure-sql-connection`) | security, external/cwe/cwe-327 | Finds unencrypted SQL connection strings. | | Page request validation is disabled (`cs/web/request-validation-disabled`) | security, frameworks/asp.net, external/cwe/cwe-016 | Finds where ASP.NET page request validation has been disabled, which could makes the application less secure. | +| Serialization check bypass (`cs/serialization-check-bypass`) | security, external/cwe/cwe-20 | Finds where data is not validated in a deserialization method. | +| XML injection (`cs/xml-injection`) | security, external/cwe/cwe-091 | Finds user-controlled data that is used to write directly to an XML document. | ## Changes to existing queries From adc557fd664f10a5a698965003f86dc907683c85 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Wed, 22 Jan 2020 16:50:25 +0100 Subject: [PATCH 07/34] C++: Reformat a predicate This allows adding a multi-line case without the auto-formatting changes becoming too disruptive. --- .../code/cpp/ir/dataflow/internal/DataFlowUtil.qll | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll b/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll index 081cf513aae..81d4ab83fde 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll @@ -261,11 +261,15 @@ predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) { } private predicate simpleInstructionLocalFlowStep(Instruction iFrom, Instruction iTo) { - iTo.(CopyInstruction).getSourceValue() = iFrom or - iTo.(PhiInstruction).getAnOperand().getDef() = iFrom or + iTo.(CopyInstruction).getSourceValue() = iFrom + or + iTo.(PhiInstruction).getAnOperand().getDef() = iFrom + or // Treat all conversions as flow, even conversions between different numeric types. - iTo.(ConvertInstruction).getUnary() = iFrom or - iTo.(InheritanceConversionInstruction).getUnary() = iFrom or + iTo.(ConvertInstruction).getUnary() = iFrom + or + iTo.(InheritanceConversionInstruction).getUnary() = iFrom + or // A chi instruction represents a point where a new value (the _partial_ // operand) may overwrite an old value (the _total_ operand), but the alias // analysis couldn't determine that it surely will overwrite every bit of it or From 7376daf16e037bc4212dbb991fd5edf39efa88ec Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Wed, 22 Jan 2020 17:06:21 +0100 Subject: [PATCH 08/34] C++: Some data flow through partial chi operands --- .../cpp/ir/dataflow/internal/DataFlowUtil.qll | 16 +++++++++++----- .../defaulttainttracking.cpp | 4 ++-- .../DefaultTaintTracking/tainted.expected | 1 + .../library-tests/dataflow/taint-tests/taint.cpp | 6 +++--- .../dataflow/taint-tests/test_diff.expected | 3 +++ .../dataflow/taint-tests/test_ir.expected | 3 +++ 6 files changed, 23 insertions(+), 10 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll b/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll index 81d4ab83fde..7d6fe1ba2b6 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll @@ -278,12 +278,18 @@ private predicate simpleInstructionLocalFlowStep(Instruction iFrom, Instruction // By allowing flow through the total operand, we ensure that flow is not lost // due to shortcomings of the alias analysis. We may get false flow in cases // where the data is indeed overwritten. - // - // Allowing flow through the partial operand would be more noisy, especially - // for variables that have escaped: for soundness, the IR has to assume that - // every write to an unknown address can affect every escaped variable, and - // this assumption shows up as data flowing through partial chi operands. iTo.getAnOperand().(ChiTotalOperand).getDef() = iFrom + or + // Flow through the partial operand must be restricted a bit more. For + // soundness, the IR has to assume that every write to an unknown address can + // affect every escaped variable, and this assumption shows up as data flowing + // through partial chi operands. The chi instructions for all escaped data can + // be recognized by having unknown types. For all other chi instructions, flow + // through partial operands is more likely to be real. + exists(ChiInstruction chi | iTo = chi | + iFrom = chi.getPartial() and + not chi.getResultIRType() instanceof IRUnknownType + ) } /** diff --git a/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/defaulttainttracking.cpp b/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/defaulttainttracking.cpp index 87fd3a9fec8..0562b79a7e3 100644 --- a/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/defaulttainttracking.cpp +++ b/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/defaulttainttracking.cpp @@ -21,8 +21,8 @@ int main(int argc, char *argv[]) { char buf[100] = "VAR = "; sink(strcat(buf, getenv("VAR"))); - sink(buf); // BUG: no taint - sink(untainted_buf); // the two buffers would be conflated if we added flow through partial chi inputs + sink(buf); + sink(untainted_buf); // the two buffers would be conflated if we added flow through all partial chi inputs return 0; } diff --git a/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/tainted.expected b/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/tainted.expected index 3de4c3d2fc7..b0f173bd1e5 100644 --- a/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/tainted.expected +++ b/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/tainted.expected @@ -25,6 +25,7 @@ | defaulttainttracking.cpp:22:20:22:25 | call to getenv | defaulttainttracking.cpp:22:8:22:33 | (const char *)... | | defaulttainttracking.cpp:22:20:22:25 | call to getenv | defaulttainttracking.cpp:22:20:22:25 | call to getenv | | defaulttainttracking.cpp:22:20:22:25 | call to getenv | defaulttainttracking.cpp:22:20:22:32 | (const char *)... | +| defaulttainttracking.cpp:22:20:22:25 | call to getenv | defaulttainttracking.cpp:24:8:24:10 | buf | | defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:32:11:32:26 | p#0 | | defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:38:11:38:21 | env_pointer | | defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:38:25:38:30 | call to getenv | 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 a162f6d49d6..49b2fd06e9c 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp @@ -107,9 +107,9 @@ void array_test(int i) { arr3[5] = 0; sink(arr1[5]); // tainted - sink(arr1[i]); // tainted [NOT DETECTED] - sink(arr2[5]); // tainted [NOT DETECTED] - sink(arr2[i]); // tainted [NOT DETECTED] + sink(arr1[i]); // tainted [NOT DETECTED with AST] + sink(arr2[5]); // tainted [NOT DETECTED with AST] + sink(arr2[i]); // tainted [NOT DETECTED with AST] sink(arr3[5]); sink(arr3[i]); } diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected index 60ef72073f4..6477371af38 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected @@ -7,6 +7,9 @@ | taint.cpp:93:11:93:11 | taint.cpp:71:22:71:27 | AST only | | taint.cpp:94:11:94:11 | taint.cpp:72:7:72:12 | AST only | | taint.cpp:109:7:109:13 | taint.cpp:105:12:105:17 | IR only | +| taint.cpp:110:7:110:13 | taint.cpp:105:12:105:17 | IR only | +| taint.cpp:111:7:111:13 | taint.cpp:106:12:106:17 | IR only | +| taint.cpp:112:7:112:13 | taint.cpp:106:12:106:17 | IR only | | taint.cpp:130:7:130:9 | taint.cpp:127:8:127:13 | IR only | | taint.cpp:137:7:137:9 | taint.cpp:120:11:120:16 | AST only | | taint.cpp:173:8:173:13 | taint.cpp:164:19:164:24 | AST only | 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 56fd106e503..f5bd94474c8 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 @@ -2,6 +2,9 @@ | taint.cpp:16:8:16:14 | source1 | taint.cpp:12:22:12:27 | call to source | | taint.cpp:17:8:17:16 | ++ ... | taint.cpp:12:22:12:27 | call to source | | taint.cpp:109:7:109:13 | access to array | taint.cpp:105:12:105:17 | call to source | +| taint.cpp:110:7:110:13 | access to array | taint.cpp:105:12:105:17 | call to source | +| taint.cpp:111:7:111:13 | access to array | taint.cpp:106:12:106:17 | call to source | +| taint.cpp:112:7:112:13 | access to array | taint.cpp:106:12:106:17 | call to source | | taint.cpp:129:7:129:9 | * ... | taint.cpp:120:11:120:16 | call to source | | taint.cpp:130:7:130:9 | * ... | taint.cpp:127:8:127:13 | call to source | | taint.cpp:134:7:134:9 | * ... | taint.cpp:120:11:120:16 | call to source | From 6743d6d6e5530b1b85a3ceb6567e5af1e4e4a1a5 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 23 Jan 2020 10:22:36 +0000 Subject: [PATCH 09/34] C#: sync-indentical-files. --- .../semmle/code/csharp/ir/implementation/raw/Instruction.qll | 2 +- .../code/csharp/ir/implementation/unaliased_ssa/Instruction.qll | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/csharp/ql/src/semmle/code/csharp/ir/implementation/raw/Instruction.qll b/csharp/ql/src/semmle/code/csharp/ir/implementation/raw/Instruction.qll index a4a9718ab95..1ac811c7eb0 100644 --- a/csharp/ql/src/semmle/code/csharp/ir/implementation/raw/Instruction.qll +++ b/csharp/ql/src/semmle/code/csharp/ir/implementation/raw/Instruction.qll @@ -974,7 +974,7 @@ class InheritanceConversionInstruction extends UnaryInstruction { /** * Represents an instruction that converts from the address of a derived class - * to the address of a direct non-virtual base class. + * to the address of a base class. */ class ConvertToBaseInstruction extends InheritanceConversionInstruction { ConvertToBaseInstruction() { getOpcode() instanceof ConvertToBaseOpcode } diff --git a/csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/Instruction.qll b/csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/Instruction.qll index a4a9718ab95..1ac811c7eb0 100644 --- a/csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/Instruction.qll +++ b/csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/Instruction.qll @@ -974,7 +974,7 @@ class InheritanceConversionInstruction extends UnaryInstruction { /** * Represents an instruction that converts from the address of a derived class - * to the address of a direct non-virtual base class. + * to the address of a base class. */ class ConvertToBaseInstruction extends InheritanceConversionInstruction { ConvertToBaseInstruction() { getOpcode() instanceof ConvertToBaseOpcode } From 406c6eb9813ca048d89613e94498b58762230243 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Thu, 23 Jan 2020 14:11:46 +0000 Subject: [PATCH 10/34] JS: Sharpen missing CSRF middleware query --- change-notes/1.24/analysis-javascript.md | 1 + .../Security/CWE-352/MissingCsrfMiddleware.ql | 53 ++++++++++++++++++- 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/change-notes/1.24/analysis-javascript.md b/change-notes/1.24/analysis-javascript.md index 5e0dea684c9..8f36921b149 100644 --- a/change-notes/1.24/analysis-javascript.md +++ b/change-notes/1.24/analysis-javascript.md @@ -33,6 +33,7 @@ | Unbound event handler receiver (`js/unbound-event-handler-receiver`) | Fewer false positive results | This query now recognizes additional ways event handler receivers can be bound. | | Expression has no effect (`js/useless-expression`) | Fewer false positive results | The query now recognizes block-level flow type annotations and ignores the first statement of a try block. | | Use of call stack introspection in strict mode (`js/strict-mode-call-stack-introspection`) | Fewer false positive results | The query no longer flags expression statements. | +| Missing CSRF middleware (`js/missing-token-validation`) | Fewer false positive results | The query reports fewer duplicates and only flags handlers that explicitly access cookie data. | ## Changes to libraries diff --git a/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql b/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql index 192f32dc79b..bf1f6215b53 100644 --- a/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql +++ b/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql @@ -12,6 +12,45 @@ import javascript +/** Gets a data flow node that flows to the base of an access to `cookies` or `session`. */ +private DataFlow::SourceNode nodeLeadingToCookieAccess(DataFlow::TypeBackTracker t) { + t.start() and + exists(string name | name = "session" or name = "cookies" | + exists(result.getAPropertyRead(name)) + ) + or + exists(DataFlow::TypeBackTracker t2 | + result = nodeLeadingToCookieAccess(t2).backtrack(t2, t) + ) +} + +/** Gets a data flow node that flows to the base of an access to `cookies` or `session`. */ +DataFlow::SourceNode nodeLeadingToCookieAccess() { + result = nodeLeadingToCookieAccess(DataFlow::TypeBackTracker::end()) +} + +/** + * Holds if `handler` uses cookies. + */ +predicate isRouteHandlerUsingCookies(Express::RouteHandler handler) { + DataFlow::parameterNode(handler.getRequestParameter()) = nodeLeadingToCookieAccess() +} + +/** Gets a data flow node referring to a route handler that uses cookies. */ +private DataFlow::SourceNode getARouteUsingCookies(DataFlow::TypeTracker t) { + t.start() and + isRouteHandlerUsingCookies(result) + or + exists(DataFlow::TypeTracker t2 | + result = getARouteUsingCookies(t2).track(t2, t) + ) +} + +/** Gets a data flow node referring to a route handler that uses cookies. */ +DataFlow::SourceNode getARouteUsingCookies() { + result = getARouteUsingCookies(DataFlow::TypeTracker::end()) +} + /** * Checks if `expr` is preceded by the cookie middleware `cookie`. * @@ -63,11 +102,23 @@ from where router = setup.getRouter() and handler = setup.getARouteHandlerExpr() and + + // Require that the handler uses cookies and has cookie middleware. + // + // In practice, handlers that use cookies always have the cookie middleware or + // the handler wouldn't work. However, if we can't find the cookie middleware, it + // indicates that our middleware model is too incomplete, so in that case we + // don't trust it to detect the presence of CSRF middleware either. + getARouteUsingCookies().flowsToExpr(handler.getPreviousMiddleware*()) and hasCookieMiddleware(handler, cookie) and + + // Only flag the first cookie parser registered first. + not hasCookieMiddleware(cookie, _) and + not hasCsrfMiddleware(handler) and // Only warn for the last handler in a chain. handler.isLastHandler() and - // Only warn for dangerous for handlers, such as for POST and PUT. + // Only warn for dangerous handlers, such as for POST and PUT. not setup.getRequestMethod().isSafe() select cookie, "This cookie middleware is serving a request handler $@ without CSRF protection.", handler, "here" From b1ec3e1bf252e78ff33b329a6f3dbf6040563b49 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Thu, 23 Jan 2020 14:59:03 +0000 Subject: [PATCH 11/34] JS: Add test and dont check predecessors --- .../Security/CWE-352/MissingCsrfMiddleware.ql | 2 +- .../CWE-352/MissingCsrfMiddleware.expected | 11 +++++----- .../CWE-352/MissingCsrfMiddlewareBad.js | 1 + .../CWE-352/MissingCsrfMiddlewareGood.js | 1 + .../Security/CWE-352/csurf_api_example.js | 4 ++++ .../Security/CWE-352/csurf_example.js | 3 +++ .../Security/CWE-352/lusca_example.js | 5 +++++ .../Security/CWE-352/unused_cookies.js | 20 +++++++++++++++++++ 8 files changed, 41 insertions(+), 6 deletions(-) create mode 100644 javascript/ql/test/query-tests/Security/CWE-352/unused_cookies.js diff --git a/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql b/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql index bf1f6215b53..7683e53610c 100644 --- a/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql +++ b/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql @@ -109,7 +109,7 @@ where // the handler wouldn't work. However, if we can't find the cookie middleware, it // indicates that our middleware model is too incomplete, so in that case we // don't trust it to detect the presence of CSRF middleware either. - getARouteUsingCookies().flowsToExpr(handler.getPreviousMiddleware*()) and + getARouteUsingCookies().flowsToExpr(handler) and hasCookieMiddleware(handler, cookie) and // Only flag the first cookie parser registered first. diff --git a/javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddleware.expected b/javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddleware.expected index 6e063b6454b..6e644f6779e 100644 --- a/javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddleware.expected +++ b/javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddleware.expected @@ -1,5 +1,6 @@ -| MissingCsrfMiddlewareBad.js:7:9:7:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | MissingCsrfMiddlewareBad.js:10:26:11:1 | functio ... es) {\\n} | here | -| csurf_api_example.js:39:37:39:50 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | csurf_api_example.js:39:53:41:3 | functio ... e')\\n } | here | -| csurf_example.js:18:9:18:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | csurf_example.js:29:40:31:1 | functio ... sed')\\n} | here | -| lusca_example.js:9:9:9:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | lusca_example.js:23:42:25:1 | functio ... sed')\\n} | here | -| lusca_example.js:9:9:9:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | lusca_example.js:27:40:29:1 | functio ... sed')\\n} | here | +| MissingCsrfMiddlewareBad.js:7:9:7:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | MissingCsrfMiddlewareBad.js:10:26:12:1 | functio ... il"];\\n} | here | +| csurf_api_example.js:42:37:42:50 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | csurf_api_example.js:42:53:45:3 | functio ... e')\\n } | here | +| csurf_example.js:18:9:18:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | csurf_example.js:31:40:34:1 | functio ... sed')\\n} | here | +| lusca_example.js:9:9:9:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | lusca_example.js:26:42:29:1 | functio ... sed')\\n} | here | +| lusca_example.js:9:9:9:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | lusca_example.js:31:40:34:1 | functio ... sed')\\n} | here | +| unused_cookies.js:6:9:6:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | unused_cookies.js:8:34:13:1 | (req, r ... Ok');\\n} | here | diff --git a/javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddlewareBad.js b/javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddlewareBad.js index ff3784ca844..247dde680d2 100644 --- a/javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddlewareBad.js +++ b/javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddlewareBad.js @@ -8,4 +8,5 @@ app.use(cookieParser()) app.use(passport.authorize({ session: true })) app.post('/changeEmail', function (req, res) { + let newEmail = req.cookies["newEmail"]; }) diff --git a/javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddlewareGood.js b/javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddlewareGood.js index 90361e28868..d56cddf4694 100644 --- a/javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddlewareGood.js +++ b/javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddlewareGood.js @@ -10,4 +10,5 @@ app.use(passport.authorize({ session: true })) app.use(csrf({ cookie:true })) app.post('/changeEmail', function (req, res) { + let newEmail = req.cookies["newEmail"]; }) diff --git a/javascript/ql/test/query-tests/Security/CWE-352/csurf_api_example.js b/javascript/ql/test/query-tests/Security/CWE-352/csurf_api_example.js index cc012d60c27..af5752e7344 100644 --- a/javascript/ql/test/query-tests/Security/CWE-352/csurf_api_example.js +++ b/javascript/ql/test/query-tests/Security/CWE-352/csurf_api_example.js @@ -21,11 +21,13 @@ app.use(cookieParser()) app.use(csrf({ cookie: true })) app.get('/form', function (req, res) { + let newEmail = req.cookies["newEmail"]; // pass the csrfToken to the view res.render('send', { csrfToken: req.csrfToken() }) }) app.post('/process', function (req, res) { // OK + let newEmail = req.cookies["newEmail"]; res.send('csrf was required to get here') }) @@ -33,10 +35,12 @@ function createApiRouter () { var router = new express.Router() router.post('/getProfile', function (req, res) { // OK - cookies are not parsed + let newEmail = req.cookies["newEmail"]; res.send('no csrf to get here') }) router.post('/getProfile_unsafe', cookieParser(), function (req, res) { // NOT OK - may use cookies + let newEmail = req.cookies["newEmail"]; res.send('no csrf to get here') }) diff --git a/javascript/ql/test/query-tests/Security/CWE-352/csurf_example.js b/javascript/ql/test/query-tests/Security/CWE-352/csurf_example.js index 7a117e1b31d..e464240f7cc 100644 --- a/javascript/ql/test/query-tests/Security/CWE-352/csurf_example.js +++ b/javascript/ql/test/query-tests/Security/CWE-352/csurf_example.js @@ -18,14 +18,17 @@ var app = express() app.use(cookieParser()) app.get('/form', csrfProtection, function (req, res) { // OK + let newEmail = req.cookies["newEmail"]; // pass the csrfToken to the view res.render('send', { csrfToken: req.csrfToken() }) }) app.post('/process', parseForm, csrfProtection, function (req, res) { // OK + let newEmail = req.cookies["newEmail"]; res.send('data is being processed') }) app.post('/process_unsafe', parseForm, function (req, res) { // NOT OK + let newEmail = req.cookies["newEmail"]; res.send('data is being processed') }) diff --git a/javascript/ql/test/query-tests/Security/CWE-352/lusca_example.js b/javascript/ql/test/query-tests/Security/CWE-352/lusca_example.js index ee91d060dd1..ffc6ba474f2 100644 --- a/javascript/ql/test/query-tests/Security/CWE-352/lusca_example.js +++ b/javascript/ql/test/query-tests/Security/CWE-352/lusca_example.js @@ -9,21 +9,26 @@ var app = express() app.use(cookieParser()) app.post('/process', parseForm, lusca.csrf(), function (req, res) { // OK + let newEmail = req.cookies["newEmail"]; res.send('data is being processed') }) app.post('/process', parseForm, lusca({csrf:true}), function (req, res) { // OK + let newEmail = req.cookies["newEmail"]; res.send('data is being processed') }) app.post('/process', parseForm, lusca({csrf:{}}), function (req, res) { // OK + let newEmail = req.cookies["newEmail"]; res.send('data is being processed') }) app.post('/process', parseForm, lusca(), function (req, res) { // NOT OK - missing csrf option + let newEmail = req.cookies["newEmail"]; res.send('data is being processed') }) app.post('/process_unsafe', parseForm, function (req, res) { // NOT OK + let newEmail = req.cookies["newEmail"]; res.send('data is being processed') }) diff --git a/javascript/ql/test/query-tests/Security/CWE-352/unused_cookies.js b/javascript/ql/test/query-tests/Security/CWE-352/unused_cookies.js new file mode 100644 index 00000000000..66ed915a5c2 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-352/unused_cookies.js @@ -0,0 +1,20 @@ +let express = require('express'); +let cookieParser = require('cookie-parser'); + +let app = express(); + +app.use(cookieParser()); + +app.post('/doSomethingTerrible', (req, res) => { // NOT OK - uses cookies + if (req.cookies['secret'] === app.secret) { + somethingTerrible(); + } + res.end('Ok'); +}); + +app.post('/doSomethingElse', (req, res) => { // OK - doesn't actually use cookies + somethingElse(req.query['data']); + res.end('Ok'); +}); + +app.listen(); From a68bb9ffd19708eb01986dd8305a481418cf8335 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Thu, 23 Jan 2020 15:32:05 +0000 Subject: [PATCH 12/34] JS: Ignore calls and csrf/captcha access --- .../src/Security/CWE-352/MissingCsrfMiddleware.ql | 15 +++++++++++++-- .../Security/CWE-352/unused_cookies.js | 9 +++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql b/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql index 7683e53610c..cc71a469574 100644 --- a/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql +++ b/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql @@ -12,11 +12,22 @@ import javascript +/** Gets the string `session` or `cookies`, the parts of `req` containing cookie data. */ +string sessionOrCookies() { + result = "session" or result = "cookies" +} + /** Gets a data flow node that flows to the base of an access to `cookies` or `session`. */ private DataFlow::SourceNode nodeLeadingToCookieAccess(DataFlow::TypeBackTracker t) { t.start() and - exists(string name | name = "session" or name = "cookies" | - exists(result.getAPropertyRead(name)) + exists(DataFlow::PropRead value | + value = result.getAPropertyRead(sessionOrCookies()).getAPropertyRead() and + + // Ignore accesses to values that are part of a CSRF or captcha check + not value.getPropertyName().regexpMatch("(?i).*(csrf|xsrf|captcha).*") and + + // Ignore calls like `req.session.save()` + not value = any(DataFlow::InvokeNode call).getCalleeNode() ) or exists(DataFlow::TypeBackTracker t2 | diff --git a/javascript/ql/test/query-tests/Security/CWE-352/unused_cookies.js b/javascript/ql/test/query-tests/Security/CWE-352/unused_cookies.js index 66ed915a5c2..bbadcb8bfe5 100644 --- a/javascript/ql/test/query-tests/Security/CWE-352/unused_cookies.js +++ b/javascript/ql/test/query-tests/Security/CWE-352/unused_cookies.js @@ -17,4 +17,13 @@ app.post('/doSomethingElse', (req, res) => { // OK - doesn't actually use cookie res.end('Ok'); }); +app.post('/doWithCaptcha', (req, res) => { // OK - attacker can't guess the captcha value either + if (req.session['captcha'] !== req.query['captcha']) { + res.end("You guessed wrong, that 'u' was actually a 'U'. Try again."); + return; + } + somethingElse(req.query['data']); + res.end('Ok'); +}); + app.listen(); From b98db62e82115edc170901fde3a93ebe2e3f9dec Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Fri, 24 Jan 2020 09:44:08 +0000 Subject: [PATCH 13/34] JS: Recognize req.user a cookie access --- .../ql/src/Security/CWE-352/MissingCsrfMiddleware.ql | 8 ++++---- .../Security/CWE-352/MissingCsrfMiddleware.expected | 2 ++ .../query-tests/Security/CWE-352/unused_cookies.js | 10 ++++++++++ 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql b/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql index cc71a469574..04d1afebd16 100644 --- a/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql +++ b/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql @@ -12,16 +12,16 @@ import javascript -/** Gets the string `session` or `cookies`, the parts of `req` containing cookie data. */ -string sessionOrCookies() { - result = "session" or result = "cookies" +/** Gets a property name of `req` which refers to data usually derived from cookie data. */ +string cookieProperty() { + result = "session" or result = "cookies" or result = "user" } /** Gets a data flow node that flows to the base of an access to `cookies` or `session`. */ private DataFlow::SourceNode nodeLeadingToCookieAccess(DataFlow::TypeBackTracker t) { t.start() and exists(DataFlow::PropRead value | - value = result.getAPropertyRead(sessionOrCookies()).getAPropertyRead() and + value = result.getAPropertyRead(cookieProperty()).getAPropertyRead() and // Ignore accesses to values that are part of a CSRF or captcha check not value.getPropertyName().regexpMatch("(?i).*(csrf|xsrf|captcha).*") and diff --git a/javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddleware.expected b/javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddleware.expected index 6e644f6779e..1f886776d23 100644 --- a/javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddleware.expected +++ b/javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddleware.expected @@ -4,3 +4,5 @@ | lusca_example.js:9:9:9:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | lusca_example.js:26:42:29:1 | functio ... sed')\\n} | here | | lusca_example.js:9:9:9:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | lusca_example.js:31:40:34:1 | functio ... sed')\\n} | here | | unused_cookies.js:6:9:6:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | unused_cookies.js:8:34:13:1 | (req, r ... Ok');\\n} | here | +| unused_cookies.js:6:9:6:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | unused_cookies.js:29:19:32:1 | (req, r ... Ok');\\n} | here | +| unused_cookies.js:6:9:6:22 | cookieParser() | This cookie middleware is serving a request handler $@ without CSRF protection. | unused_cookies.js:34:22:37:1 | (req, r ... Ok');\\n} | here | diff --git a/javascript/ql/test/query-tests/Security/CWE-352/unused_cookies.js b/javascript/ql/test/query-tests/Security/CWE-352/unused_cookies.js index bbadcb8bfe5..133304faac9 100644 --- a/javascript/ql/test/query-tests/Security/CWE-352/unused_cookies.js +++ b/javascript/ql/test/query-tests/Security/CWE-352/unused_cookies.js @@ -26,4 +26,14 @@ app.post('/doWithCaptcha', (req, res) => { // OK - attacker can't guess the capt res.end('Ok'); }); +app.post('/user', (req, res) => { // NOT OK - access to req.user is unprotected + somethingElse(req.user.name); + res.end('Ok'); +}); + +app.post('/session', (req, res) => { // NOT OK - access to req.session is unprotected + somethingElse(req.session.name); + res.end('Ok'); +}); + app.listen(); From 06f5720cd5bf831070f53b8b390de2e139f6551d Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 24 Jan 2020 15:34:47 +0000 Subject: [PATCH 14/34] C++: Add taint tests of formatting functions. --- .../dataflow/taint-tests/format.cpp | 134 ++++++++++++++++++ .../dataflow/taint-tests/localTaint.expected | 74 ++++++++++ 2 files changed, 208 insertions(+) create mode 100644 cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp b/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp new file mode 100644 index 00000000000..e94d0ca164b --- /dev/null +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp @@ -0,0 +1,134 @@ + +typedef unsigned long size_t; +typedef struct {} FILE; + +int snprintf(char *s, size_t n, const char *format, ...); +int sprintf(char *s, const char *format, ...); +int swprintf(wchar_t *s, size_t n, const wchar_t *format, ...); + +typedef void *va_list; +#define va_start(ap, parmN) +#define va_end(ap) +#define va_arg(ap, type) ((type)0) + +int vsnprintf(char *s, size_t n, const char *format, va_list arg); + +int mysprintf(char *s, size_t n, const char *format, ...) +{ + va_list args; + va_start(args, format); + vsnprintf(s, n, format, args); + va_end(args); +} + +int sscanf(const char *s, const char *format, ...); + +// ---------- + +int source(); +void sink(...) {}; + +namespace string +{ + char *source(); +}; + +namespace wstring +{ + wchar_t *source(); +}; + +// ---------- + +void test1() +{ + { + char buffer[256] = {0}; + sink(snprintf(buffer, 256, "%s", "Hello.")); + sink(buffer); + } + { + char buffer[256] = {0}; + sink(snprintf(buffer, 256, "%s", string::source())); + sink(buffer); // tainted [NOT DETECTED] + } + { + char buffer[256] = {0}; + sink(snprintf(buffer, 256, string::source(), "Hello.")); + sink(buffer); // tainted [NOT DETECTED] + } + { + char buffer[256] = {0}; + sink(snprintf(buffer, 256, "%s %s %s", "a", "b", string::source())); + sink(buffer); // tainted [NOT DETECTED] + } + { + char buffer[256] = {0}; + sink(snprintf(buffer, 256, "%.*s", 10, string::source())); + sink(buffer); // tainted [NOT DETECTED] + } + + { + char buffer[256] = {0}; + sink(snprintf(buffer, 256, "%i", 0)); + sink(buffer); + } + { + char buffer[256] = {0}; + sink(snprintf(buffer, 256, "%i", source())); + sink(buffer); // tainted + } + { + char buffer[256] = {0}; + sink(snprintf(buffer, 256, "%.*s", source(), "Hello.")); + sink(buffer); // tainted + } + + { + char buffer[256] = {0}; + sink(snprintf(buffer, 256, "%p", string::source())); + sink(buffer); + } + + { + char buffer[256] = {0}; + sink(sprintf(buffer, "%s", string::source())); + sink(buffer); // tainted [NOT DETECTED] + } + { + char buffer[256] = {0}; + sink(sprintf(buffer, "%ls", wstring::source())); + sink(buffer); // tainted [NOT DETECTED] + } + { + wchar_t wbuffer[256] = {0}; + sink(swprintf(wbuffer, 256, L"%s", wstring::source())); + sink(wbuffer); // tainted [NOT DETECTED] + } + { + char buffer[256] = {0}; + sink(mysprintf(buffer, 256, "%s", string::source())); + sink(buffer); // tainted [NOT DETECTED] + } + + { + int i = 0; + sink(sscanf("123", "%i", &i)); + sink(i); + } + { + int i = 0; + sink(sscanf(string::source(), "%i", &i)); + sink(i); // tainted [NOT DETECTED] + } + { + char buffer[256] = {0}; + sink(sscanf("Hello.", "%s", &buffer)); + sink(buffer); + } + { + char buffer[256] = {0}; + sink(sscanf(string::source(), "%s", &buffer)); + sink(buffer); // tainted [NOT DETECTED] + } +} 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 10a25da812b..ccf41532640 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected @@ -3,6 +3,80 @@ | file://:0:0:0:0 | p#0 | file://:0:0:0:0 | p#0 | | | file://:0:0:0:0 | p#0 | file://:0:0:0:0 | p#0 | | | file://:0:0:0:0 | p#0 | file://:0:0:0:0 | p#0 | | +| format.cpp:16:21:16:21 | s | format.cpp:20:13:20:13 | s | | +| format.cpp:16:31:16:31 | n | format.cpp:20:16:20:16 | n | | +| format.cpp:16:46:16:51 | format | format.cpp:20:19:20:24 | format | | +| format.cpp:18:10:18:13 | args | format.cpp:20:27:20:30 | args | | +| format.cpp:46:21:46:24 | {...} | format.cpp:47:17:47:22 | buffer | | +| format.cpp:46:21:46:24 | {...} | format.cpp:48:8:48:13 | buffer | | +| format.cpp:46:23:46:23 | 0 | format.cpp:46:21:46:24 | {...} | TAINT | +| format.cpp:47:17:47:22 | ref arg buffer | format.cpp:48:8:48:13 | buffer | | +| format.cpp:51:21:51:24 | {...} | format.cpp:52:17:52:22 | buffer | | +| format.cpp:51:21:51:24 | {...} | format.cpp:53:8:53:13 | buffer | | +| format.cpp:51:23:51:23 | 0 | format.cpp:51:21:51:24 | {...} | TAINT | +| format.cpp:52:17:52:22 | ref arg buffer | format.cpp:53:8:53:13 | buffer | | +| format.cpp:56:21:56:24 | {...} | format.cpp:57:17:57:22 | buffer | | +| format.cpp:56:21:56:24 | {...} | format.cpp:58:8:58:13 | buffer | | +| format.cpp:56:23:56:23 | 0 | format.cpp:56:21:56:24 | {...} | TAINT | +| format.cpp:57:17:57:22 | ref arg buffer | format.cpp:58:8:58:13 | buffer | | +| format.cpp:61:21:61:24 | {...} | format.cpp:62:17:62:22 | buffer | | +| format.cpp:61:21:61:24 | {...} | format.cpp:63:8:63:13 | buffer | | +| format.cpp:61:23:61:23 | 0 | format.cpp:61:21:61:24 | {...} | TAINT | +| format.cpp:62:17:62:22 | ref arg buffer | format.cpp:63:8:63:13 | buffer | | +| format.cpp:66:21:66:24 | {...} | format.cpp:67:17:67:22 | buffer | | +| format.cpp:66:21:66:24 | {...} | format.cpp:68:8:68:13 | buffer | | +| format.cpp:66:23:66:23 | 0 | format.cpp:66:21:66:24 | {...} | TAINT | +| format.cpp:67:17:67:22 | ref arg buffer | format.cpp:68:8:68:13 | buffer | | +| format.cpp:72:21:72:24 | {...} | format.cpp:73:17:73:22 | buffer | | +| format.cpp:72:21:72:24 | {...} | format.cpp:74:8:74:13 | buffer | | +| format.cpp:72:23:72:23 | 0 | format.cpp:72:21:72:24 | {...} | TAINT | +| format.cpp:73:17:73:22 | ref arg buffer | format.cpp:74:8:74:13 | buffer | | +| format.cpp:77:21:77:24 | {...} | format.cpp:78:17:78:22 | buffer | | +| format.cpp:77:21:77:24 | {...} | format.cpp:79:8:79:13 | buffer | | +| format.cpp:77:23:77:23 | 0 | format.cpp:77:21:77:24 | {...} | TAINT | +| format.cpp:78:17:78:22 | ref arg buffer | format.cpp:79:8:79:13 | buffer | | +| format.cpp:82:21:82:24 | {...} | format.cpp:83:17:83:22 | buffer | | +| format.cpp:82:21:82:24 | {...} | format.cpp:84:8:84:13 | buffer | | +| format.cpp:82:23:82:23 | 0 | format.cpp:82:21:82:24 | {...} | TAINT | +| format.cpp:83:17:83:22 | ref arg buffer | format.cpp:84:8:84:13 | buffer | | +| format.cpp:88:21:88:24 | {...} | format.cpp:89:17:89:22 | buffer | | +| format.cpp:88:21:88:24 | {...} | format.cpp:90:8:90:13 | buffer | | +| format.cpp:88:23:88:23 | 0 | format.cpp:88:21:88:24 | {...} | TAINT | +| format.cpp:89:17:89:22 | ref arg buffer | format.cpp:90:8:90:13 | buffer | | +| format.cpp:94:21:94:24 | {...} | format.cpp:95:16:95:21 | buffer | | +| format.cpp:94:21:94:24 | {...} | format.cpp:96:8:96:13 | buffer | | +| format.cpp:94:23:94:23 | 0 | format.cpp:94:21:94:24 | {...} | TAINT | +| format.cpp:95:16:95:21 | ref arg buffer | format.cpp:96:8:96:13 | buffer | | +| format.cpp:99:21:99:24 | {...} | format.cpp:100:16:100:21 | buffer | | +| format.cpp:99:21:99:24 | {...} | format.cpp:101:8:101:13 | buffer | | +| format.cpp:99:23:99:23 | 0 | format.cpp:99:21:99:24 | {...} | TAINT | +| format.cpp:100:16:100:21 | ref arg buffer | format.cpp:101:8:101:13 | buffer | | +| format.cpp:104:25:104:28 | {...} | format.cpp:105:17:105:23 | wbuffer | | +| format.cpp:104:25:104:28 | {...} | format.cpp:106:8:106:14 | wbuffer | | +| format.cpp:104:27:104:27 | 0 | format.cpp:104:25:104:28 | {...} | TAINT | +| format.cpp:105:17:105:23 | ref arg wbuffer | format.cpp:106:8:106:14 | wbuffer | | +| format.cpp:109:21:109:24 | {...} | format.cpp:110:18:110:23 | buffer | | +| format.cpp:109:21:109:24 | {...} | format.cpp:111:8:111:13 | buffer | | +| format.cpp:109:23:109:23 | 0 | format.cpp:109:21:109:24 | {...} | TAINT | +| format.cpp:110:18:110:23 | ref arg buffer | format.cpp:111:8:111:13 | buffer | | +| format.cpp:115:10:115:11 | 0 | format.cpp:116:29:116:29 | i | | +| format.cpp:115:10:115:11 | 0 | format.cpp:117:8:117:8 | i | | +| format.cpp:116:28:116:29 | ref arg & ... | format.cpp:117:8:117:8 | i | | +| format.cpp:116:29:116:29 | i | format.cpp:116:28:116:29 | & ... | | +| format.cpp:120:10:120:11 | 0 | format.cpp:121:40:121:40 | i | | +| format.cpp:120:10:120:11 | 0 | format.cpp:122:8:122:8 | i | | +| format.cpp:121:39:121:40 | ref arg & ... | format.cpp:122:8:122:8 | i | | +| format.cpp:121:40:121:40 | i | format.cpp:121:39:121:40 | & ... | | +| format.cpp:125:21:125:24 | {...} | format.cpp:126:32:126:37 | buffer | | +| format.cpp:125:21:125:24 | {...} | format.cpp:127:8:127:13 | buffer | | +| format.cpp:125:23:125:23 | 0 | format.cpp:125:21:125:24 | {...} | TAINT | +| format.cpp:126:31:126:37 | ref arg & ... | format.cpp:127:8:127:13 | buffer | | +| format.cpp:126:32:126:37 | buffer | format.cpp:126:31:126:37 | & ... | | +| format.cpp:130:21:130:24 | {...} | format.cpp:131:40:131:45 | buffer | | +| format.cpp:130:21:130:24 | {...} | format.cpp:132:8:132:13 | buffer | | +| format.cpp:130:23:130:23 | 0 | format.cpp:130:21:130:24 | {...} | TAINT | +| format.cpp:131:39:131:45 | ref arg & ... | format.cpp:132:8:132:13 | buffer | | +| format.cpp:131:40:131:45 | buffer | format.cpp:131:39:131:45 | & ... | | | taint.cpp:4:27:4:33 | source1 | taint.cpp:6:13:6:19 | source1 | | | taint.cpp:4:40:4:45 | clean1 | taint.cpp:5:8:5:13 | clean1 | | | taint.cpp:4:40:4:45 | clean1 | taint.cpp:6:3:6:8 | clean1 | | From 1d46971bb786cc05b79954752469993c1a62d7b4 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 24 Jan 2020 17:10:25 +0000 Subject: [PATCH 15/34] C++: Add an ArrayFunction model to FormattingFunction. --- .../models/interfaces/FormattingFunction.qll | 26 +++++++++++++++++-- .../NoSpaceForZeroTerminator.expected | 1 + .../semmle/NoSpaceForZeroTerminator/test.cpp | 2 +- 3 files changed, 26 insertions(+), 3 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/models/interfaces/FormattingFunction.qll b/cpp/ql/src/semmle/code/cpp/models/interfaces/FormattingFunction.qll index 130c067f70c..901cf6e70ae 100644 --- a/cpp/ql/src/semmle/code/cpp/models/interfaces/FormattingFunction.qll +++ b/cpp/ql/src/semmle/code/cpp/models/interfaces/FormattingFunction.qll @@ -6,7 +6,7 @@ * `FormattingFunction` to match the flow within that function. */ -import semmle.code.cpp.Function +import semmle.code.cpp.models.interfaces.ArrayFunction private Type stripTopLevelSpecifiersOnly(Type t) { result = stripTopLevelSpecifiersOnly(t.(SpecifiedType).getBaseType()) @@ -39,7 +39,7 @@ private Type getAFormatterWideTypeOrDefault() { /** * A standard library function that uses a `printf`-like formatting string. */ -abstract class FormattingFunction extends Function { +abstract class FormattingFunction extends ArrayFunction { /** Gets the position at which the format parameter occurs. */ abstract int getFormatParameterIndex(); @@ -133,4 +133,26 @@ abstract class FormattingFunction extends Function { * Gets the position of the buffer size argument, if any. */ int getSizeParameterIndex() { none() } + + override predicate hasArrayWithNullTerminator(int bufParam) { + bufParam = getFormatParameterIndex() + } + + override predicate hasArrayWithVariableSize(int bufParam, int countParam) { + bufParam = getOutputParameterIndex() and + countParam = getSizeParameterIndex() + } + + override predicate hasArrayWithUnknownSize(int bufParam) { + bufParam = getOutputParameterIndex() and + not exists(getSizeParameterIndex()) + } + + predicate hasArrayInput(int bufParam) { + bufParam = getFormatParameterIndex() + } + + predicate hasArrayOutput(int bufParam) { + bufParam = getOutputParameterIndex() + } } diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/NoSpaceForZeroTerminator/NoSpaceForZeroTerminator.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/NoSpaceForZeroTerminator/NoSpaceForZeroTerminator.expected index 4500779812b..de52927e1e8 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/NoSpaceForZeroTerminator/NoSpaceForZeroTerminator.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/NoSpaceForZeroTerminator/NoSpaceForZeroTerminator.expected @@ -4,6 +4,7 @@ | test.c:32:20:32:25 | call to malloc | This allocation does not include space to null-terminate the string. | | test.c:49:20:49:25 | call to malloc | This allocation does not include space to null-terminate the string. | | test.cpp:24:35:24:40 | call to malloc | This allocation does not include space to null-terminate the string. | +| test.cpp:45:28:45:33 | call to malloc | This allocation does not include space to null-terminate the string. | | test.cpp:63:28:63:33 | call to malloc | This allocation does not include space to null-terminate the string. | | test.cpp:71:28:71:33 | call to malloc | This allocation does not include space to null-terminate the string. | | test.cpp:79:28:79:33 | call to malloc | This allocation does not include space to null-terminate the string. | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/NoSpaceForZeroTerminator/test.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/NoSpaceForZeroTerminator/test.cpp index 5be98851b3d..b0db8dea6ef 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/NoSpaceForZeroTerminator/test.cpp +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/NoSpaceForZeroTerminator/test.cpp @@ -41,7 +41,7 @@ void good1(wchar_t *wstr) { } void bad3(char *str) { - // BAD -- zero-termination proved by sprintf (as destination) [NOT DETECTED] + // BAD -- zero-termination proved by sprintf (as destination) char *buffer = (char *)malloc(strlen(str)); sprintf(buffer, "%s", str); free(buffer); From 30580e97dcd30e26c76bf3b2c7ffa1fca7a49aff Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 24 Jan 2020 17:25:09 +0000 Subject: [PATCH 16/34] C++: Add a TaintFunction model to FormattingFunction. --- .../cpp/models/interfaces/FormattingFunction.qll | 8 +++++++- .../library-tests/dataflow/taint-tests/format.cpp | 2 +- .../dataflow/taint-tests/localTaint.expected | 12 ++++++++++++ .../dataflow/taint-tests/taint.expected | 1 + .../dataflow/taint-tests/test_diff.expected | 1 + 5 files changed, 22 insertions(+), 2 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/models/interfaces/FormattingFunction.qll b/cpp/ql/src/semmle/code/cpp/models/interfaces/FormattingFunction.qll index 901cf6e70ae..71bb5ebac33 100644 --- a/cpp/ql/src/semmle/code/cpp/models/interfaces/FormattingFunction.qll +++ b/cpp/ql/src/semmle/code/cpp/models/interfaces/FormattingFunction.qll @@ -7,6 +7,7 @@ */ import semmle.code.cpp.models.interfaces.ArrayFunction +import semmle.code.cpp.models.interfaces.Taint private Type stripTopLevelSpecifiersOnly(Type t) { result = stripTopLevelSpecifiersOnly(t.(SpecifiedType).getBaseType()) @@ -39,7 +40,7 @@ private Type getAFormatterWideTypeOrDefault() { /** * A standard library function that uses a `printf`-like formatting string. */ -abstract class FormattingFunction extends ArrayFunction { +abstract class FormattingFunction extends ArrayFunction, TaintFunction { /** Gets the position at which the format parameter occurs. */ abstract int getFormatParameterIndex(); @@ -155,4 +156,9 @@ abstract class FormattingFunction extends ArrayFunction { predicate hasArrayOutput(int bufParam) { bufParam = getOutputParameterIndex() } + + override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) { + input.isParameterDeref(getFormatParameterIndex()) and + output.isParameterDeref(getOutputParameterIndex()) + } } diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp b/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp index e94d0ca164b..7af01d9809e 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp @@ -55,7 +55,7 @@ void test1() { char buffer[256] = {0}; sink(snprintf(buffer, 256, string::source(), "Hello.")); - sink(buffer); // tainted [NOT DETECTED] + sink(buffer); // tainted } { char buffer[256] = {0}; 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 ccf41532640..ac3209259e9 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected @@ -11,50 +11,62 @@ | format.cpp:46:21:46:24 | {...} | format.cpp:48:8:48:13 | buffer | | | format.cpp:46:23:46:23 | 0 | format.cpp:46:21:46:24 | {...} | TAINT | | format.cpp:47:17:47:22 | ref arg buffer | format.cpp:48:8:48:13 | buffer | | +| format.cpp:47:30:47:33 | %s | format.cpp:47:17:47:22 | ref arg buffer | TAINT | | format.cpp:51:21:51:24 | {...} | format.cpp:52:17:52:22 | buffer | | | format.cpp:51:21:51:24 | {...} | format.cpp:53:8:53:13 | buffer | | | format.cpp:51:23:51:23 | 0 | format.cpp:51:21:51:24 | {...} | TAINT | | format.cpp:52:17:52:22 | ref arg buffer | format.cpp:53:8:53:13 | buffer | | +| format.cpp:52:30:52:33 | %s | format.cpp:52:17:52:22 | ref arg buffer | TAINT | | format.cpp:56:21:56:24 | {...} | format.cpp:57:17:57:22 | buffer | | | format.cpp:56:21:56:24 | {...} | format.cpp:58:8:58:13 | buffer | | | format.cpp:56:23:56:23 | 0 | format.cpp:56:21:56:24 | {...} | TAINT | | format.cpp:57:17:57:22 | ref arg buffer | format.cpp:58:8:58:13 | buffer | | +| format.cpp:57:30:57:43 | call to source | format.cpp:57:17:57:22 | ref arg buffer | TAINT | | format.cpp:61:21:61:24 | {...} | format.cpp:62:17:62:22 | buffer | | | format.cpp:61:21:61:24 | {...} | format.cpp:63:8:63:13 | buffer | | | format.cpp:61:23:61:23 | 0 | format.cpp:61:21:61:24 | {...} | TAINT | | format.cpp:62:17:62:22 | ref arg buffer | format.cpp:63:8:63:13 | buffer | | +| format.cpp:62:30:62:39 | %s %s %s | format.cpp:62:17:62:22 | ref arg buffer | TAINT | | format.cpp:66:21:66:24 | {...} | format.cpp:67:17:67:22 | buffer | | | format.cpp:66:21:66:24 | {...} | format.cpp:68:8:68:13 | buffer | | | format.cpp:66:23:66:23 | 0 | format.cpp:66:21:66:24 | {...} | TAINT | | format.cpp:67:17:67:22 | ref arg buffer | format.cpp:68:8:68:13 | buffer | | +| format.cpp:67:30:67:35 | %.*s | format.cpp:67:17:67:22 | ref arg buffer | TAINT | | format.cpp:72:21:72:24 | {...} | format.cpp:73:17:73:22 | buffer | | | format.cpp:72:21:72:24 | {...} | format.cpp:74:8:74:13 | buffer | | | format.cpp:72:23:72:23 | 0 | format.cpp:72:21:72:24 | {...} | TAINT | | format.cpp:73:17:73:22 | ref arg buffer | format.cpp:74:8:74:13 | buffer | | +| format.cpp:73:30:73:33 | %i | format.cpp:73:17:73:22 | ref arg buffer | TAINT | | format.cpp:77:21:77:24 | {...} | format.cpp:78:17:78:22 | buffer | | | format.cpp:77:21:77:24 | {...} | format.cpp:79:8:79:13 | buffer | | | format.cpp:77:23:77:23 | 0 | format.cpp:77:21:77:24 | {...} | TAINT | | format.cpp:78:17:78:22 | ref arg buffer | format.cpp:79:8:79:13 | buffer | | +| format.cpp:78:30:78:33 | %i | format.cpp:78:17:78:22 | ref arg buffer | TAINT | | format.cpp:82:21:82:24 | {...} | format.cpp:83:17:83:22 | buffer | | | format.cpp:82:21:82:24 | {...} | format.cpp:84:8:84:13 | buffer | | | format.cpp:82:23:82:23 | 0 | format.cpp:82:21:82:24 | {...} | TAINT | | format.cpp:83:17:83:22 | ref arg buffer | format.cpp:84:8:84:13 | buffer | | +| format.cpp:83:30:83:35 | %.*s | format.cpp:83:17:83:22 | ref arg buffer | TAINT | | format.cpp:88:21:88:24 | {...} | format.cpp:89:17:89:22 | buffer | | | format.cpp:88:21:88:24 | {...} | format.cpp:90:8:90:13 | buffer | | | format.cpp:88:23:88:23 | 0 | format.cpp:88:21:88:24 | {...} | TAINT | | format.cpp:89:17:89:22 | ref arg buffer | format.cpp:90:8:90:13 | buffer | | +| format.cpp:89:30:89:33 | %p | format.cpp:89:17:89:22 | ref arg buffer | TAINT | | format.cpp:94:21:94:24 | {...} | format.cpp:95:16:95:21 | buffer | | | format.cpp:94:21:94:24 | {...} | format.cpp:96:8:96:13 | buffer | | | format.cpp:94:23:94:23 | 0 | format.cpp:94:21:94:24 | {...} | TAINT | | format.cpp:95:16:95:21 | ref arg buffer | format.cpp:96:8:96:13 | buffer | | +| format.cpp:95:24:95:27 | %s | format.cpp:95:16:95:21 | ref arg buffer | TAINT | | format.cpp:99:21:99:24 | {...} | format.cpp:100:16:100:21 | buffer | | | format.cpp:99:21:99:24 | {...} | format.cpp:101:8:101:13 | buffer | | | format.cpp:99:23:99:23 | 0 | format.cpp:99:21:99:24 | {...} | TAINT | | format.cpp:100:16:100:21 | ref arg buffer | format.cpp:101:8:101:13 | buffer | | +| format.cpp:100:24:100:28 | %ls | format.cpp:100:16:100:21 | ref arg buffer | TAINT | | format.cpp:104:25:104:28 | {...} | format.cpp:105:17:105:23 | wbuffer | | | format.cpp:104:25:104:28 | {...} | format.cpp:106:8:106:14 | wbuffer | | | format.cpp:104:27:104:27 | 0 | format.cpp:104:25:104:28 | {...} | TAINT | | format.cpp:105:17:105:23 | ref arg wbuffer | format.cpp:106:8:106:14 | wbuffer | | +| format.cpp:105:31:105:35 | %s | format.cpp:105:17:105:23 | ref arg wbuffer | TAINT | | format.cpp:109:21:109:24 | {...} | format.cpp:110:18:110:23 | buffer | | | format.cpp:109:21:109:24 | {...} | format.cpp:111:8:111:13 | buffer | | | format.cpp:109:23:109:23 | 0 | format.cpp:109:21:109:24 | {...} | 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 a507f6b7b83..5322d05b854 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected @@ -1,3 +1,4 @@ +| format.cpp:58:8:58:13 | buffer | format.cpp:57:30:57:43 | call to source | | taint.cpp:8:8:8:13 | clean1 | taint.cpp:4:27:4:33 | source1 | | taint.cpp:16:8:16:14 | source1 | taint.cpp:12:22:12:27 | call to source | | taint.cpp:17:8:17:16 | ++ ... | taint.cpp:12:22:12:27 | call to source | diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected index 1d875493db0..a3f795f4ca3 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected @@ -1,3 +1,4 @@ +| format.cpp:58:8:58:13 | format.cpp:57:30:57:43 | AST only | | taint.cpp:41:7:41:13 | taint.cpp:35:12:35:17 | AST only | | taint.cpp:42:7:42:13 | taint.cpp:35:12:35:17 | AST only | | taint.cpp:43:7:43:13 | taint.cpp:37:22:37:27 | AST only | From 1ddabee1b8ebd8ab1ede25cccabf432d49eafa0c Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 24 Jan 2020 17:45:01 +0000 Subject: [PATCH 17/34] C++: Change note. --- change-notes/1.24/analysis-cpp.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/change-notes/1.24/analysis-cpp.md b/change-notes/1.24/analysis-cpp.md index 94256075a88..3135c50dcf8 100644 --- a/change-notes/1.24/analysis-cpp.md +++ b/change-notes/1.24/analysis-cpp.md @@ -40,4 +40,4 @@ The following changes in version 1.24 affect C/C++ analysis in all applications. * The taint tracking library (`semmle.code.cpp.dataflow.TaintTracking`) has had the following improvements: * The library now models data flow through `strdup` and similar functions. - \ No newline at end of file + * The library now models data flow through formatting functions such as `sprintf`. Only fixed parameters (not varargs) are included in the model. From 01dc3661b7b58fb48fe2a2af35c0c41518cb6802 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Tue, 28 Jan 2020 12:17:56 +0000 Subject: [PATCH 18/34] C++: Autoformat. --- .../models/interfaces/FormattingFunction.qll | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/models/interfaces/FormattingFunction.qll b/cpp/ql/src/semmle/code/cpp/models/interfaces/FormattingFunction.qll index 71bb5ebac33..d25fa5dadfc 100644 --- a/cpp/ql/src/semmle/code/cpp/models/interfaces/FormattingFunction.qll +++ b/cpp/ql/src/semmle/code/cpp/models/interfaces/FormattingFunction.qll @@ -136,26 +136,22 @@ abstract class FormattingFunction extends ArrayFunction, TaintFunction { int getSizeParameterIndex() { none() } override predicate hasArrayWithNullTerminator(int bufParam) { - bufParam = getFormatParameterIndex() - } + bufParam = getFormatParameterIndex() + } override predicate hasArrayWithVariableSize(int bufParam, int countParam) { - bufParam = getOutputParameterIndex() and - countParam = getSizeParameterIndex() + bufParam = getOutputParameterIndex() and + countParam = getSizeParameterIndex() } override predicate hasArrayWithUnknownSize(int bufParam) { - bufParam = getOutputParameterIndex() and - not exists(getSizeParameterIndex()) + bufParam = getOutputParameterIndex() and + not exists(getSizeParameterIndex()) } - predicate hasArrayInput(int bufParam) { - bufParam = getFormatParameterIndex() - } + predicate hasArrayInput(int bufParam) { bufParam = getFormatParameterIndex() } - predicate hasArrayOutput(int bufParam) { - bufParam = getOutputParameterIndex() - } + predicate hasArrayOutput(int bufParam) { bufParam = getOutputParameterIndex() } override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) { input.isParameterDeref(getFormatParameterIndex()) and From 310dd05185621fe4cf7fb5636251e162da1e591f Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 28 Jan 2020 12:46:34 +0000 Subject: [PATCH 19/34] Update javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql Co-Authored-By: Erik Krogh Kristensen --- javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql b/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql index 04d1afebd16..f60c857b79e 100644 --- a/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql +++ b/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql @@ -17,7 +17,7 @@ string cookieProperty() { result = "session" or result = "cookies" or result = "user" } -/** Gets a data flow node that flows to the base of an access to `cookies` or `session`. */ +/** Gets a data flow node that flows to the base of an access to `cookies`, `session`, or `user`. */ private DataFlow::SourceNode nodeLeadingToCookieAccess(DataFlow::TypeBackTracker t) { t.start() and exists(DataFlow::PropRead value | From 701d9989be2b08952fddb5306901a72ca470a810 Mon Sep 17 00:00:00 2001 From: Asger F Date: Tue, 28 Jan 2020 12:46:51 +0000 Subject: [PATCH 20/34] Apply suggestions from code review Co-Authored-By: Erik Krogh Kristensen --- javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql b/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql index f60c857b79e..f7db881fdf0 100644 --- a/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql +++ b/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql @@ -35,7 +35,7 @@ private DataFlow::SourceNode nodeLeadingToCookieAccess(DataFlow::TypeBackTracker ) } -/** Gets a data flow node that flows to the base of an access to `cookies` or `session`. */ +/** Gets a data flow node that flows to the base of an access to `cookies`, `session`, or `user`. */ DataFlow::SourceNode nodeLeadingToCookieAccess() { result = nodeLeadingToCookieAccess(DataFlow::TypeBackTracker::end()) } @@ -123,7 +123,7 @@ where getARouteUsingCookies().flowsToExpr(handler) and hasCookieMiddleware(handler, cookie) and - // Only flag the first cookie parser registered first. + // Only flag the cookie parser registered first. not hasCookieMiddleware(cookie, _) and not hasCsrfMiddleware(handler) and From b1f66ae8250f060112d195fdc672c894cb338782 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Tue, 28 Jan 2020 14:46:12 +0000 Subject: [PATCH 21/34] C++: Fix warnings. --- .../semmle/code/cpp/models/interfaces/FormattingFunction.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/models/interfaces/FormattingFunction.qll b/cpp/ql/src/semmle/code/cpp/models/interfaces/FormattingFunction.qll index d25fa5dadfc..873de9bc750 100644 --- a/cpp/ql/src/semmle/code/cpp/models/interfaces/FormattingFunction.qll +++ b/cpp/ql/src/semmle/code/cpp/models/interfaces/FormattingFunction.qll @@ -149,9 +149,9 @@ abstract class FormattingFunction extends ArrayFunction, TaintFunction { not exists(getSizeParameterIndex()) } - predicate hasArrayInput(int bufParam) { bufParam = getFormatParameterIndex() } + override predicate hasArrayInput(int bufParam) { bufParam = getFormatParameterIndex() } - predicate hasArrayOutput(int bufParam) { bufParam = getOutputParameterIndex() } + override predicate hasArrayOutput(int bufParam) { bufParam = getOutputParameterIndex() } override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) { input.isParameterDeref(getFormatParameterIndex()) and From 8b215c155e4132e47390824cf05f8e73c61ab2c8 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Tue, 28 Jan 2020 14:05:49 +0000 Subject: [PATCH 22/34] C++: Correct a few test comments. --- cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp b/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp index 7af01d9809e..6d138755155 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp @@ -76,12 +76,12 @@ void test1() { char buffer[256] = {0}; sink(snprintf(buffer, 256, "%i", source())); - sink(buffer); // tainted + sink(buffer); // tainted [NOT DETECTED] } { char buffer[256] = {0}; sink(snprintf(buffer, 256, "%.*s", source(), "Hello.")); - sink(buffer); // tainted + sink(buffer); // tainted [NOT DETECTED] } { From d66f608d41530f8ef4884d9f44cbe151628ebdc0 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Tue, 28 Jan 2020 14:04:53 +0000 Subject: [PATCH 23/34] C++: Taint from FormattingFunction varargs. --- .../cpp/models/interfaces/FormattingFunction.qll | 10 ++++++++-- .../library-tests/dataflow/taint-tests/format.cpp | 14 +++++++------- .../dataflow/taint-tests/localTaint.expected | 12 ++++++++++++ .../dataflow/taint-tests/taint.expected | 7 +++++++ .../dataflow/taint-tests/test_diff.expected | 7 +++++++ 5 files changed, 41 insertions(+), 9 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/models/interfaces/FormattingFunction.qll b/cpp/ql/src/semmle/code/cpp/models/interfaces/FormattingFunction.qll index 873de9bc750..7c035072916 100644 --- a/cpp/ql/src/semmle/code/cpp/models/interfaces/FormattingFunction.qll +++ b/cpp/ql/src/semmle/code/cpp/models/interfaces/FormattingFunction.qll @@ -154,7 +154,13 @@ abstract class FormattingFunction extends ArrayFunction, TaintFunction { override predicate hasArrayOutput(int bufParam) { bufParam = getOutputParameterIndex() } override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) { - input.isParameterDeref(getFormatParameterIndex()) and - output.isParameterDeref(getOutputParameterIndex()) + exists(int arg | + ( + arg = getFormatParameterIndex() or + arg >= getFirstFormatArgumentIndex() + ) and + input.isParameterDeref(arg) and + output.isParameterDeref(getOutputParameterIndex()) + ) } } diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp b/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp index 6d138755155..79b3f45be03 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp @@ -50,7 +50,7 @@ void test1() { char buffer[256] = {0}; sink(snprintf(buffer, 256, "%s", string::source())); - sink(buffer); // tainted [NOT DETECTED] + sink(buffer); // tainted } { char buffer[256] = {0}; @@ -76,34 +76,34 @@ void test1() { char buffer[256] = {0}; sink(snprintf(buffer, 256, "%i", source())); - sink(buffer); // tainted [NOT DETECTED] + sink(buffer); // tainted } { char buffer[256] = {0}; sink(snprintf(buffer, 256, "%.*s", source(), "Hello.")); - sink(buffer); // tainted [NOT DETECTED] + sink(buffer); // tainted } { char buffer[256] = {0}; sink(snprintf(buffer, 256, "%p", string::source())); - sink(buffer); + sink(buffer); // tainted (debatable) } { char buffer[256] = {0}; sink(sprintf(buffer, "%s", string::source())); - sink(buffer); // tainted [NOT DETECTED] + sink(buffer); // tainted } { char buffer[256] = {0}; sink(sprintf(buffer, "%ls", wstring::source())); - sink(buffer); // tainted [NOT DETECTED] + sink(buffer); // tainted } { wchar_t wbuffer[256] = {0}; sink(swprintf(wbuffer, 256, L"%s", wstring::source())); - sink(wbuffer); // tainted [NOT DETECTED] + sink(wbuffer); // tainted } { char buffer[256] = {0}; 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 ac3209259e9..2d35ac5dcfb 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected @@ -12,61 +12,73 @@ | format.cpp:46:23:46:23 | 0 | format.cpp:46:21:46:24 | {...} | TAINT | | format.cpp:47:17:47:22 | ref arg buffer | format.cpp:48:8:48:13 | buffer | | | format.cpp:47:30:47:33 | %s | format.cpp:47:17:47:22 | ref arg buffer | TAINT | +| format.cpp:47:36:47:43 | Hello. | format.cpp:47:17:47:22 | ref arg buffer | TAINT | | format.cpp:51:21:51:24 | {...} | format.cpp:52:17:52:22 | buffer | | | format.cpp:51:21:51:24 | {...} | format.cpp:53:8:53:13 | buffer | | | format.cpp:51:23:51:23 | 0 | format.cpp:51:21:51:24 | {...} | TAINT | | format.cpp:52:17:52:22 | ref arg buffer | format.cpp:53:8:53:13 | buffer | | | format.cpp:52:30:52:33 | %s | format.cpp:52:17:52:22 | ref arg buffer | TAINT | +| format.cpp:52:36:52:49 | call to source | format.cpp:52:17:52:22 | ref arg buffer | TAINT | | format.cpp:56:21:56:24 | {...} | format.cpp:57:17:57:22 | buffer | | | format.cpp:56:21:56:24 | {...} | format.cpp:58:8:58:13 | buffer | | | format.cpp:56:23:56:23 | 0 | format.cpp:56:21:56:24 | {...} | TAINT | | format.cpp:57:17:57:22 | ref arg buffer | format.cpp:58:8:58:13 | buffer | | | format.cpp:57:30:57:43 | call to source | format.cpp:57:17:57:22 | ref arg buffer | TAINT | +| format.cpp:57:48:57:55 | Hello. | format.cpp:57:17:57:22 | ref arg buffer | TAINT | | format.cpp:61:21:61:24 | {...} | format.cpp:62:17:62:22 | buffer | | | format.cpp:61:21:61:24 | {...} | format.cpp:63:8:63:13 | buffer | | | format.cpp:61:23:61:23 | 0 | format.cpp:61:21:61:24 | {...} | TAINT | | format.cpp:62:17:62:22 | ref arg buffer | format.cpp:63:8:63:13 | buffer | | | format.cpp:62:30:62:39 | %s %s %s | format.cpp:62:17:62:22 | ref arg buffer | TAINT | +| format.cpp:62:42:62:44 | a | format.cpp:62:17:62:22 | ref arg buffer | TAINT | | format.cpp:66:21:66:24 | {...} | format.cpp:67:17:67:22 | buffer | | | format.cpp:66:21:66:24 | {...} | format.cpp:68:8:68:13 | buffer | | | format.cpp:66:23:66:23 | 0 | format.cpp:66:21:66:24 | {...} | TAINT | | format.cpp:67:17:67:22 | ref arg buffer | format.cpp:68:8:68:13 | buffer | | | format.cpp:67:30:67:35 | %.*s | format.cpp:67:17:67:22 | ref arg buffer | TAINT | +| format.cpp:67:38:67:39 | 10 | format.cpp:67:17:67:22 | ref arg buffer | TAINT | | format.cpp:72:21:72:24 | {...} | format.cpp:73:17:73:22 | buffer | | | format.cpp:72:21:72:24 | {...} | format.cpp:74:8:74:13 | buffer | | | format.cpp:72:23:72:23 | 0 | format.cpp:72:21:72:24 | {...} | TAINT | | format.cpp:73:17:73:22 | ref arg buffer | format.cpp:74:8:74:13 | buffer | | | format.cpp:73:30:73:33 | %i | format.cpp:73:17:73:22 | ref arg buffer | TAINT | +| format.cpp:73:36:73:36 | 0 | format.cpp:73:17:73:22 | ref arg buffer | TAINT | | format.cpp:77:21:77:24 | {...} | format.cpp:78:17:78:22 | buffer | | | format.cpp:77:21:77:24 | {...} | format.cpp:79:8:79:13 | buffer | | | format.cpp:77:23:77:23 | 0 | format.cpp:77:21:77:24 | {...} | TAINT | | format.cpp:78:17:78:22 | ref arg buffer | format.cpp:79:8:79:13 | buffer | | | format.cpp:78:30:78:33 | %i | format.cpp:78:17:78:22 | ref arg buffer | TAINT | +| format.cpp:78:36:78:41 | call to source | format.cpp:78:17:78:22 | ref arg buffer | TAINT | | format.cpp:82:21:82:24 | {...} | format.cpp:83:17:83:22 | buffer | | | format.cpp:82:21:82:24 | {...} | format.cpp:84:8:84:13 | buffer | | | format.cpp:82:23:82:23 | 0 | format.cpp:82:21:82:24 | {...} | TAINT | | format.cpp:83:17:83:22 | ref arg buffer | format.cpp:84:8:84:13 | buffer | | | format.cpp:83:30:83:35 | %.*s | format.cpp:83:17:83:22 | ref arg buffer | TAINT | +| format.cpp:83:38:83:43 | call to source | format.cpp:83:17:83:22 | ref arg buffer | TAINT | | format.cpp:88:21:88:24 | {...} | format.cpp:89:17:89:22 | buffer | | | format.cpp:88:21:88:24 | {...} | format.cpp:90:8:90:13 | buffer | | | format.cpp:88:23:88:23 | 0 | format.cpp:88:21:88:24 | {...} | TAINT | | format.cpp:89:17:89:22 | ref arg buffer | format.cpp:90:8:90:13 | buffer | | | format.cpp:89:30:89:33 | %p | format.cpp:89:17:89:22 | ref arg buffer | TAINT | +| format.cpp:89:36:89:49 | call to source | format.cpp:89:17:89:22 | ref arg buffer | TAINT | | format.cpp:94:21:94:24 | {...} | format.cpp:95:16:95:21 | buffer | | | format.cpp:94:21:94:24 | {...} | format.cpp:96:8:96:13 | buffer | | | format.cpp:94:23:94:23 | 0 | format.cpp:94:21:94:24 | {...} | TAINT | | format.cpp:95:16:95:21 | ref arg buffer | format.cpp:96:8:96:13 | buffer | | | format.cpp:95:24:95:27 | %s | format.cpp:95:16:95:21 | ref arg buffer | TAINT | +| format.cpp:95:30:95:43 | call to source | format.cpp:95:16:95:21 | ref arg buffer | TAINT | | format.cpp:99:21:99:24 | {...} | format.cpp:100:16:100:21 | buffer | | | format.cpp:99:21:99:24 | {...} | format.cpp:101:8:101:13 | buffer | | | format.cpp:99:23:99:23 | 0 | format.cpp:99:21:99:24 | {...} | TAINT | | format.cpp:100:16:100:21 | ref arg buffer | format.cpp:101:8:101:13 | buffer | | | format.cpp:100:24:100:28 | %ls | format.cpp:100:16:100:21 | ref arg buffer | TAINT | +| format.cpp:100:31:100:45 | call to source | format.cpp:100:16:100:21 | ref arg buffer | TAINT | | format.cpp:104:25:104:28 | {...} | format.cpp:105:17:105:23 | wbuffer | | | format.cpp:104:25:104:28 | {...} | format.cpp:106:8:106:14 | wbuffer | | | format.cpp:104:27:104:27 | 0 | format.cpp:104:25:104:28 | {...} | TAINT | | format.cpp:105:17:105:23 | ref arg wbuffer | format.cpp:106:8:106:14 | wbuffer | | | format.cpp:105:31:105:35 | %s | format.cpp:105:17:105:23 | ref arg wbuffer | TAINT | +| format.cpp:105:38:105:52 | call to source | format.cpp:105:17:105:23 | ref arg wbuffer | TAINT | | format.cpp:109:21:109:24 | {...} | format.cpp:110:18:110:23 | buffer | | | format.cpp:109:21:109:24 | {...} | format.cpp:111:8:111:13 | buffer | | | format.cpp:109:23:109:23 | 0 | format.cpp:109:21:109:24 | {...} | 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 5322d05b854..3999ab74f7d 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected @@ -1,4 +1,11 @@ +| format.cpp:53:8:53:13 | buffer | format.cpp:52:36:52:49 | call to source | | format.cpp:58:8:58:13 | buffer | format.cpp:57:30:57:43 | call to source | +| format.cpp:79:8:79:13 | buffer | format.cpp:78:36:78:41 | call to source | +| format.cpp:84:8:84:13 | buffer | format.cpp:83:38:83:43 | call to source | +| format.cpp:90:8:90:13 | buffer | format.cpp:89:36:89:49 | call to source | +| format.cpp:96:8:96:13 | buffer | format.cpp:95:30:95:43 | call to source | +| format.cpp:101:8:101:13 | buffer | format.cpp:100:31:100:45 | call to source | +| format.cpp:106:8:106:14 | wbuffer | format.cpp:105:38:105:52 | call to source | | taint.cpp:8:8:8:13 | clean1 | taint.cpp:4:27:4:33 | source1 | | taint.cpp:16:8:16:14 | source1 | taint.cpp:12:22:12:27 | call to source | | taint.cpp:17:8:17:16 | ++ ... | taint.cpp:12:22:12:27 | call to source | diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected index a3f795f4ca3..0bdab4f4088 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected @@ -1,4 +1,11 @@ +| format.cpp:53:8:53:13 | format.cpp:52:36:52:49 | AST only | | format.cpp:58:8:58:13 | format.cpp:57:30:57:43 | AST only | +| format.cpp:79:8:79:13 | format.cpp:78:36:78:41 | AST only | +| format.cpp:84:8:84:13 | format.cpp:83:38:83:43 | AST only | +| format.cpp:90:8:90:13 | format.cpp:89:36:89:49 | AST only | +| format.cpp:96:8:96:13 | format.cpp:95:30:95:43 | AST only | +| format.cpp:101:8:101:13 | format.cpp:100:31:100:45 | AST only | +| format.cpp:106:8:106:14 | format.cpp:105:38:105:52 | AST only | | taint.cpp:41:7:41:13 | taint.cpp:35:12:35:17 | AST only | | taint.cpp:42:7:42:13 | taint.cpp:35:12:35:17 | AST only | | taint.cpp:43:7:43:13 | taint.cpp:37:22:37:27 | AST only | From f02ffcbbd282fdcf020a04402b8360445a3520d3 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Tue, 28 Jan 2020 14:34:46 +0000 Subject: [PATCH 24/34] C++: Modify ParameterIndex to account for varargs. --- cpp/ql/src/semmle/code/cpp/Parameter.qll | 5 ++++- cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp | 6 +++--- .../library-tests/dataflow/taint-tests/localTaint.expected | 4 ++++ .../test/library-tests/dataflow/taint-tests/taint.expected | 2 ++ .../library-tests/dataflow/taint-tests/test_diff.expected | 2 ++ 5 files changed, 15 insertions(+), 4 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/Parameter.qll b/cpp/ql/src/semmle/code/cpp/Parameter.qll index 8b391101c6c..1fbd8b0f071 100644 --- a/cpp/ql/src/semmle/code/cpp/Parameter.qll +++ b/cpp/ql/src/semmle/code/cpp/Parameter.qll @@ -163,5 +163,8 @@ class Parameter extends LocalScopeVariable, @parameter { * An `int` that is a parameter index for some function. This is needed for binding in certain cases. */ class ParameterIndex extends int { - ParameterIndex() { exists(Parameter p | this = p.getIndex()) } + ParameterIndex() { + exists(Parameter p | this = p.getIndex()) or + exists(Call c | exists(c.getArgument(this))) // permit indexing varargs + } } diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp b/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp index 79b3f45be03..2080707f17f 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp @@ -60,12 +60,12 @@ void test1() { char buffer[256] = {0}; sink(snprintf(buffer, 256, "%s %s %s", "a", "b", string::source())); - sink(buffer); // tainted [NOT DETECTED] + sink(buffer); // tainted } { char buffer[256] = {0}; sink(snprintf(buffer, 256, "%.*s", 10, string::source())); - sink(buffer); // tainted [NOT DETECTED] + sink(buffer); // tainted } { @@ -108,7 +108,7 @@ void test1() { char buffer[256] = {0}; sink(mysprintf(buffer, 256, "%s", string::source())); - sink(buffer); // tainted [NOT DETECTED] + sink(buffer); // tainted [NOT DETECTED - implement UserDefinedFormattingFunction.getOutputParameterIndex()] } { 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 2d35ac5dcfb..648159b4944 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected @@ -31,12 +31,15 @@ | format.cpp:62:17:62:22 | ref arg buffer | format.cpp:63:8:63:13 | buffer | | | format.cpp:62:30:62:39 | %s %s %s | format.cpp:62:17:62:22 | ref arg buffer | TAINT | | format.cpp:62:42:62:44 | a | format.cpp:62:17:62:22 | ref arg buffer | TAINT | +| format.cpp:62:47:62:49 | b | format.cpp:62:17:62:22 | ref arg buffer | TAINT | +| format.cpp:62:52:62:65 | call to source | format.cpp:62:17:62:22 | ref arg buffer | TAINT | | format.cpp:66:21:66:24 | {...} | format.cpp:67:17:67:22 | buffer | | | format.cpp:66:21:66:24 | {...} | format.cpp:68:8:68:13 | buffer | | | format.cpp:66:23:66:23 | 0 | format.cpp:66:21:66:24 | {...} | TAINT | | format.cpp:67:17:67:22 | ref arg buffer | format.cpp:68:8:68:13 | buffer | | | format.cpp:67:30:67:35 | %.*s | format.cpp:67:17:67:22 | ref arg buffer | TAINT | | format.cpp:67:38:67:39 | 10 | format.cpp:67:17:67:22 | ref arg buffer | TAINT | +| format.cpp:67:42:67:55 | call to source | format.cpp:67:17:67:22 | ref arg buffer | TAINT | | format.cpp:72:21:72:24 | {...} | format.cpp:73:17:73:22 | buffer | | | format.cpp:72:21:72:24 | {...} | format.cpp:74:8:74:13 | buffer | | | format.cpp:72:23:72:23 | 0 | format.cpp:72:21:72:24 | {...} | TAINT | @@ -55,6 +58,7 @@ | format.cpp:83:17:83:22 | ref arg buffer | format.cpp:84:8:84:13 | buffer | | | format.cpp:83:30:83:35 | %.*s | format.cpp:83:17:83:22 | ref arg buffer | TAINT | | format.cpp:83:38:83:43 | call to source | format.cpp:83:17:83:22 | ref arg buffer | TAINT | +| format.cpp:83:48:83:55 | Hello. | format.cpp:83:17:83:22 | ref arg buffer | TAINT | | format.cpp:88:21:88:24 | {...} | format.cpp:89:17:89:22 | buffer | | | format.cpp:88:21:88:24 | {...} | format.cpp:90:8:90:13 | buffer | | | format.cpp:88:23:88:23 | 0 | format.cpp:88:21:88:24 | {...} | 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 3999ab74f7d..46146094e53 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected @@ -1,5 +1,7 @@ | format.cpp:53:8:53:13 | buffer | format.cpp:52:36:52:49 | call to source | | format.cpp:58:8:58:13 | buffer | format.cpp:57:30:57:43 | call to source | +| format.cpp:63:8:63:13 | buffer | format.cpp:62:52:62:65 | call to source | +| format.cpp:68:8:68:13 | buffer | format.cpp:67:42:67:55 | call to source | | format.cpp:79:8:79:13 | buffer | format.cpp:78:36:78:41 | call to source | | format.cpp:84:8:84:13 | buffer | format.cpp:83:38:83:43 | call to source | | format.cpp:90:8:90:13 | buffer | format.cpp:89:36:89:49 | call to source | diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected index 0bdab4f4088..659ea724637 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected @@ -1,5 +1,7 @@ | format.cpp:53:8:53:13 | format.cpp:52:36:52:49 | AST only | | format.cpp:58:8:58:13 | format.cpp:57:30:57:43 | AST only | +| format.cpp:63:8:63:13 | format.cpp:62:52:62:65 | AST only | +| format.cpp:68:8:68:13 | format.cpp:67:42:67:55 | AST only | | format.cpp:79:8:79:13 | format.cpp:78:36:78:41 | AST only | | format.cpp:84:8:84:13 | format.cpp:83:38:83:43 | AST only | | format.cpp:90:8:90:13 | format.cpp:89:36:89:49 | AST only | From fc1816cbd7e66c0db358267979a1200b2010417d Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Tue, 28 Jan 2020 14:45:23 +0000 Subject: [PATCH 25/34] C++: Update change note. --- change-notes/1.24/analysis-cpp.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/change-notes/1.24/analysis-cpp.md b/change-notes/1.24/analysis-cpp.md index 3135c50dcf8..aa4bbdf5887 100644 --- a/change-notes/1.24/analysis-cpp.md +++ b/change-notes/1.24/analysis-cpp.md @@ -40,4 +40,4 @@ The following changes in version 1.24 affect C/C++ analysis in all applications. * The taint tracking library (`semmle.code.cpp.dataflow.TaintTracking`) has had the following improvements: * The library now models data flow through `strdup` and similar functions. - * The library now models data flow through formatting functions such as `sprintf`. Only fixed parameters (not varargs) are included in the model. + * The library now models data flow through formatting functions such as `sprintf`. From 928b0c50d28f2a51130999bbe0ffeb4d59b9fdd2 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 28 Jan 2020 17:26:23 +0100 Subject: [PATCH 26/34] C++: Add test demonstrating false negative when using dynamic_cast --- .../defaulttainttracking.cpp | 39 ++++++++++++++ .../DefaultTaintTracking/tainted.expected | 54 +++++++++++++++++++ 2 files changed, 93 insertions(+) diff --git a/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/defaulttainttracking.cpp b/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/defaulttainttracking.cpp index 8958616c0d3..747f71f90d4 100644 --- a/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/defaulttainttracking.cpp +++ b/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/defaulttainttracking.cpp @@ -39,3 +39,42 @@ void test_indirect_arg_to_model() { inet_addr_retval a = inet_addr((const char *)&env_pointer); sink(a); } + +class B { + public: + virtual void f(const char*) = 0; +}; + +class D1 : public B {}; + +class D2 : public D1 { + public: + void f(const char* p) override {} +}; + +class D3 : public D2 { + public: + void f(const char* p) override { + sink(p); + } +}; + +void test_dynamic_cast() { + B* b = new D3(); + b->f(getenv("VAR")); // tainted + + ((D2*)b)->f(getenv("VAR")); // tainted + static_cast(b)->f(getenv("VAR")); // tainted + dynamic_cast(b)->f(getenv("VAR")); // tainted [NOT DETECTED] + reinterpret_cast(b)->f(getenv("VAR")); // tainted + + B* b2 = new D2(); + b2->f(getenv("VAR")); + + ((D2*)b2)->f(getenv("VAR")); + static_cast(b2)->f(getenv("VAR")); + dynamic_cast(b2)->f(getenv("VAR")); + reinterpret_cast(b2)->f(getenv("VAR")); + + dynamic_cast(b2)->f(getenv("VAR")); +} \ No newline at end of file diff --git a/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/tainted.expected b/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/tainted.expected index d2b48f25bf9..eed97407faa 100644 --- a/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/tainted.expected +++ b/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/tainted.expected @@ -30,6 +30,60 @@ | defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:39:26:39:34 | call to inet_addr | | defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:39:50:39:61 | & ... | | defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:40:10:40:10 | a | +| defaulttainttracking.cpp:64:10:64:15 | call to getenv | defaulttainttracking.cpp:9:11:9:20 | p#0 | +| defaulttainttracking.cpp:64:10:64:15 | call to getenv | defaulttainttracking.cpp:45:20:45:29 | p#0 | +| defaulttainttracking.cpp:64:10:64:15 | call to getenv | defaulttainttracking.cpp:52:24:52:24 | p | +| defaulttainttracking.cpp:64:10:64:15 | call to getenv | defaulttainttracking.cpp:57:24:57:24 | p | +| defaulttainttracking.cpp:64:10:64:15 | call to getenv | defaulttainttracking.cpp:58:14:58:14 | p | +| defaulttainttracking.cpp:64:10:64:15 | call to getenv | defaulttainttracking.cpp:64:10:64:15 | call to getenv | +| defaulttainttracking.cpp:64:10:64:15 | call to getenv | defaulttainttracking.cpp:64:10:64:22 | (const char *)... | +| defaulttainttracking.cpp:64:10:64:15 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 | +| defaulttainttracking.cpp:66:17:66:22 | call to getenv | defaulttainttracking.cpp:9:11:9:20 | p#0 | +| defaulttainttracking.cpp:66:17:66:22 | call to getenv | defaulttainttracking.cpp:52:24:52:24 | p | +| defaulttainttracking.cpp:66:17:66:22 | call to getenv | defaulttainttracking.cpp:57:24:57:24 | p | +| defaulttainttracking.cpp:66:17:66:22 | call to getenv | defaulttainttracking.cpp:58:14:58:14 | p | +| defaulttainttracking.cpp:66:17:66:22 | call to getenv | defaulttainttracking.cpp:66:17:66:22 | call to getenv | +| defaulttainttracking.cpp:66:17:66:22 | call to getenv | defaulttainttracking.cpp:66:17:66:29 | (const char *)... | +| defaulttainttracking.cpp:66:17:66:22 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 | +| defaulttainttracking.cpp:67:28:67:33 | call to getenv | defaulttainttracking.cpp:9:11:9:20 | p#0 | +| defaulttainttracking.cpp:67:28:67:33 | call to getenv | defaulttainttracking.cpp:52:24:52:24 | p | +| defaulttainttracking.cpp:67:28:67:33 | call to getenv | defaulttainttracking.cpp:57:24:57:24 | p | +| defaulttainttracking.cpp:67:28:67:33 | call to getenv | defaulttainttracking.cpp:58:14:58:14 | p | +| defaulttainttracking.cpp:67:28:67:33 | call to getenv | defaulttainttracking.cpp:67:28:67:33 | call to getenv | +| defaulttainttracking.cpp:67:28:67:33 | call to getenv | defaulttainttracking.cpp:67:28:67:40 | (const char *)... | +| defaulttainttracking.cpp:67:28:67:33 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 | +| defaulttainttracking.cpp:68:29:68:34 | call to getenv | defaulttainttracking.cpp:52:24:52:24 | p | +| defaulttainttracking.cpp:68:29:68:34 | call to getenv | defaulttainttracking.cpp:68:29:68:34 | call to getenv | +| defaulttainttracking.cpp:68:29:68:34 | call to getenv | defaulttainttracking.cpp:68:29:68:41 | (const char *)... | +| defaulttainttracking.cpp:69:33:69:38 | call to getenv | defaulttainttracking.cpp:9:11:9:20 | p#0 | +| defaulttainttracking.cpp:69:33:69:38 | call to getenv | defaulttainttracking.cpp:52:24:52:24 | p | +| defaulttainttracking.cpp:69:33:69:38 | call to getenv | defaulttainttracking.cpp:57:24:57:24 | p | +| defaulttainttracking.cpp:69:33:69:38 | call to getenv | defaulttainttracking.cpp:58:14:58:14 | p | +| defaulttainttracking.cpp:69:33:69:38 | call to getenv | defaulttainttracking.cpp:69:33:69:38 | call to getenv | +| defaulttainttracking.cpp:69:33:69:38 | call to getenv | defaulttainttracking.cpp:69:33:69:45 | (const char *)... | +| defaulttainttracking.cpp:69:33:69:38 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 | +| defaulttainttracking.cpp:72:11:72:16 | call to getenv | defaulttainttracking.cpp:45:20:45:29 | p#0 | +| defaulttainttracking.cpp:72:11:72:16 | call to getenv | defaulttainttracking.cpp:52:24:52:24 | p | +| defaulttainttracking.cpp:72:11:72:16 | call to getenv | defaulttainttracking.cpp:72:11:72:16 | call to getenv | +| defaulttainttracking.cpp:72:11:72:16 | call to getenv | defaulttainttracking.cpp:72:11:72:23 | (const char *)... | +| defaulttainttracking.cpp:74:18:74:23 | call to getenv | defaulttainttracking.cpp:52:24:52:24 | p | +| defaulttainttracking.cpp:74:18:74:23 | call to getenv | defaulttainttracking.cpp:74:18:74:23 | call to getenv | +| defaulttainttracking.cpp:74:18:74:23 | call to getenv | defaulttainttracking.cpp:74:18:74:30 | (const char *)... | +| defaulttainttracking.cpp:75:29:75:34 | call to getenv | defaulttainttracking.cpp:52:24:52:24 | p | +| defaulttainttracking.cpp:75:29:75:34 | call to getenv | defaulttainttracking.cpp:75:29:75:34 | call to getenv | +| defaulttainttracking.cpp:75:29:75:34 | call to getenv | defaulttainttracking.cpp:75:29:75:41 | (const char *)... | +| defaulttainttracking.cpp:76:30:76:35 | call to getenv | defaulttainttracking.cpp:52:24:52:24 | p | +| defaulttainttracking.cpp:76:30:76:35 | call to getenv | defaulttainttracking.cpp:76:30:76:35 | call to getenv | +| defaulttainttracking.cpp:76:30:76:35 | call to getenv | defaulttainttracking.cpp:76:30:76:42 | (const char *)... | +| defaulttainttracking.cpp:77:34:77:39 | call to getenv | defaulttainttracking.cpp:52:24:52:24 | p | +| defaulttainttracking.cpp:77:34:77:39 | call to getenv | defaulttainttracking.cpp:77:34:77:39 | call to getenv | +| defaulttainttracking.cpp:77:34:77:39 | call to getenv | defaulttainttracking.cpp:77:34:77:46 | (const char *)... | +| defaulttainttracking.cpp:79:30:79:35 | call to getenv | defaulttainttracking.cpp:9:11:9:20 | p#0 | +| defaulttainttracking.cpp:79:30:79:35 | call to getenv | defaulttainttracking.cpp:57:24:57:24 | p | +| defaulttainttracking.cpp:79:30:79:35 | call to getenv | defaulttainttracking.cpp:58:14:58:14 | p | +| defaulttainttracking.cpp:79:30:79:35 | call to getenv | defaulttainttracking.cpp:79:30:79:35 | call to getenv | +| defaulttainttracking.cpp:79:30:79:35 | call to getenv | defaulttainttracking.cpp:79:30:79:42 | (const char *)... | +| defaulttainttracking.cpp:79:30:79:35 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 | | test_diff.cpp:92:10:92:13 | argv | defaulttainttracking.cpp:9:11:9:20 | p#0 | | test_diff.cpp:92:10:92:13 | argv | test_diff.cpp:1:11:1:20 | p#0 | | test_diff.cpp:92:10:92:13 | argv | test_diff.cpp:92:10:92:13 | argv | From 46ce228bce52689295cca77fca53cd286d2b702d Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 28 Jan 2020 17:36:17 +0100 Subject: [PATCH 27/34] C++: Add instruction for CheckedConvertOrNull and handle it in alias analysis and data flow --- .../src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll | 1 + .../code/cpp/ir/implementation/aliased_ssa/Instruction.qll | 4 ++++ .../ir/implementation/aliased_ssa/internal/AliasAnalysis.qll | 4 ++++ .../src/semmle/code/cpp/ir/implementation/raw/Instruction.qll | 4 ++++ .../code/cpp/ir/implementation/unaliased_ssa/Instruction.qll | 4 ++++ .../implementation/unaliased_ssa/internal/AliasAnalysis.qll | 4 ++++ 6 files changed, 21 insertions(+) diff --git a/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll b/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll index 24598bdaf12..30b26b59ad9 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll @@ -268,6 +268,7 @@ private predicate simpleInstructionLocalFlowStep(Instruction iFrom, Instruction iTo.(PhiInstruction).getAnOperand().getDef() = iFrom or // Treat all conversions as flow, even conversions between different numeric types. iTo.(ConvertInstruction).getUnary() = iFrom or + iTo.(CheckedConvertOrNullInstruction).getUnary() = iFrom or iTo.(InheritanceConversionInstruction).getUnary() = iFrom or // A chi instruction represents a point where a new value (the _partial_ // operand) may overwrite an old value (the _total_ operand), but the alias diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll index 7514f580813..4f4b0eafe62 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll @@ -947,6 +947,10 @@ class ConvertInstruction extends UnaryInstruction { ConvertInstruction() { getOpcode() instanceof Opcode::Convert } } +class CheckedConvertOrNullInstruction extends UnaryInstruction { + CheckedConvertOrNullInstruction() { getOpcode() instanceof Opcode::CheckedConvertOrNull } +} + /** * Represents an instruction that converts between two addresses * related by inheritance. diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasAnalysis.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasAnalysis.qll index 99f6b545806..6fe55c49d8f 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasAnalysis.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasAnalysis.qll @@ -96,6 +96,10 @@ private predicate operandIsPropagated(Operand operand, IntValue bitOffset) { bitOffset = Ints::mul(convert.getDerivation().getByteOffset(), 8) ) or + // Conversion using dynamic_cast results in an unknown offset + instr instanceof CheckedConvertOrNullInstruction and + bitOffset = Ints::unknown() + or // Converting to a derived class subtracts the offset of the base class. exists(ConvertToDerivedInstruction convert | convert = instr and diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll index 7514f580813..4f4b0eafe62 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll @@ -947,6 +947,10 @@ class ConvertInstruction extends UnaryInstruction { ConvertInstruction() { getOpcode() instanceof Opcode::Convert } } +class CheckedConvertOrNullInstruction extends UnaryInstruction { + CheckedConvertOrNullInstruction() { getOpcode() instanceof Opcode::CheckedConvertOrNull } +} + /** * Represents an instruction that converts between two addresses * related by inheritance. diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll index 7514f580813..4f4b0eafe62 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll @@ -947,6 +947,10 @@ class ConvertInstruction extends UnaryInstruction { ConvertInstruction() { getOpcode() instanceof Opcode::Convert } } +class CheckedConvertOrNullInstruction extends UnaryInstruction { + CheckedConvertOrNullInstruction() { getOpcode() instanceof Opcode::CheckedConvertOrNull } +} + /** * Represents an instruction that converts between two addresses * related by inheritance. diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/AliasAnalysis.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/AliasAnalysis.qll index 99f6b545806..6fe55c49d8f 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/AliasAnalysis.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/AliasAnalysis.qll @@ -96,6 +96,10 @@ private predicate operandIsPropagated(Operand operand, IntValue bitOffset) { bitOffset = Ints::mul(convert.getDerivation().getByteOffset(), 8) ) or + // Conversion using dynamic_cast results in an unknown offset + instr instanceof CheckedConvertOrNullInstruction and + bitOffset = Ints::unknown() + or // Converting to a derived class subtracts the offset of the base class. exists(ConvertToDerivedInstruction convert | convert = instr and From c1091a03d016856c1d45530709c67e55763056da Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 28 Jan 2020 17:38:35 +0100 Subject: [PATCH 28/34] C++: Accept output --- .../DefaultTaintTracking/defaulttainttracking.cpp | 4 ++-- .../dataflow/DefaultTaintTracking/tainted.expected | 8 ++++++++ .../dataflow/DefaultTaintTracking/test_diff.expected | 5 +---- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/defaulttainttracking.cpp b/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/defaulttainttracking.cpp index 747f71f90d4..d4137762dcb 100644 --- a/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/defaulttainttracking.cpp +++ b/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/defaulttainttracking.cpp @@ -65,7 +65,7 @@ void test_dynamic_cast() { ((D2*)b)->f(getenv("VAR")); // tainted static_cast(b)->f(getenv("VAR")); // tainted - dynamic_cast(b)->f(getenv("VAR")); // tainted [NOT DETECTED] + dynamic_cast(b)->f(getenv("VAR")); // tainted reinterpret_cast(b)->f(getenv("VAR")); // tainted B* b2 = new D2(); @@ -76,5 +76,5 @@ void test_dynamic_cast() { dynamic_cast(b2)->f(getenv("VAR")); reinterpret_cast(b2)->f(getenv("VAR")); - dynamic_cast(b2)->f(getenv("VAR")); + dynamic_cast(b2)->f(getenv("VAR")); // tainted [FALSE POSITIVE] } \ No newline at end of file diff --git a/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/tainted.expected b/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/tainted.expected index eed97407faa..5c9655fe748 100644 --- a/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/tainted.expected +++ b/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/tainted.expected @@ -52,9 +52,13 @@ | defaulttainttracking.cpp:67:28:67:33 | call to getenv | defaulttainttracking.cpp:67:28:67:33 | call to getenv | | defaulttainttracking.cpp:67:28:67:33 | call to getenv | defaulttainttracking.cpp:67:28:67:40 | (const char *)... | | defaulttainttracking.cpp:67:28:67:33 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 | +| defaulttainttracking.cpp:68:29:68:34 | call to getenv | defaulttainttracking.cpp:9:11:9:20 | p#0 | | defaulttainttracking.cpp:68:29:68:34 | call to getenv | defaulttainttracking.cpp:52:24:52:24 | p | +| defaulttainttracking.cpp:68:29:68:34 | call to getenv | defaulttainttracking.cpp:57:24:57:24 | p | +| defaulttainttracking.cpp:68:29:68:34 | call to getenv | defaulttainttracking.cpp:58:14:58:14 | p | | defaulttainttracking.cpp:68:29:68:34 | call to getenv | defaulttainttracking.cpp:68:29:68:34 | call to getenv | | defaulttainttracking.cpp:68:29:68:34 | call to getenv | defaulttainttracking.cpp:68:29:68:41 | (const char *)... | +| defaulttainttracking.cpp:68:29:68:34 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 | | defaulttainttracking.cpp:69:33:69:38 | call to getenv | defaulttainttracking.cpp:9:11:9:20 | p#0 | | defaulttainttracking.cpp:69:33:69:38 | call to getenv | defaulttainttracking.cpp:52:24:52:24 | p | | defaulttainttracking.cpp:69:33:69:38 | call to getenv | defaulttainttracking.cpp:57:24:57:24 | p | @@ -177,7 +181,11 @@ | test_diff.cpp:126:43:126:46 | argv | test_diff.cpp:126:43:126:46 | argv | | test_diff.cpp:126:43:126:46 | argv | test_diff.cpp:126:43:126:49 | (const char *)... | | test_diff.cpp:126:43:126:46 | argv | test_diff.cpp:126:43:126:49 | access to array | +| test_diff.cpp:128:44:128:47 | argv | defaulttainttracking.cpp:9:11:9:20 | p#0 | +| test_diff.cpp:128:44:128:47 | argv | test_diff.cpp:1:11:1:20 | p#0 | | test_diff.cpp:128:44:128:47 | argv | test_diff.cpp:76:24:76:24 | p | +| test_diff.cpp:128:44:128:47 | argv | test_diff.cpp:81:24:81:24 | p | +| test_diff.cpp:128:44:128:47 | argv | test_diff.cpp:82:14:82:14 | p | | test_diff.cpp:128:44:128:47 | argv | test_diff.cpp:128:44:128:47 | argv | | test_diff.cpp:128:44:128:47 | argv | test_diff.cpp:128:44:128:50 | (const char *)... | | test_diff.cpp:128:44:128:47 | argv | test_diff.cpp:128:44:128:50 | access to array | diff --git a/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/test_diff.expected b/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/test_diff.expected index 04c0498a617..209ce48e6e4 100644 --- a/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/test_diff.expected +++ b/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/test_diff.expected @@ -9,6 +9,7 @@ | defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:31:40:31:53 | dotted_address | AST only | | defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:39:36:39:61 | (const char *)... | AST only | | defaulttainttracking.cpp:38:25:38:30 | call to getenv | defaulttainttracking.cpp:39:51:39:61 | env_pointer | AST only | +| defaulttainttracking.cpp:64:10:64:15 | call to getenv | defaulttainttracking.cpp:52:24:52:24 | p | IR only | | test_diff.cpp:104:12:104:15 | argv | test_diff.cpp:104:11:104:20 | (...) | IR only | | test_diff.cpp:108:10:108:13 | argv | test_diff.cpp:36:24:36:24 | p | AST only | | test_diff.cpp:111:10:111:13 | argv | defaulttainttracking.cpp:9:11:9:20 | p#0 | AST only | @@ -16,7 +17,3 @@ | test_diff.cpp:111:10:111:13 | argv | test_diff.cpp:29:24:29:24 | p | AST only | | test_diff.cpp:111:10:111:13 | argv | test_diff.cpp:30:14:30:14 | p | AST only | | test_diff.cpp:124:19:124:22 | argv | test_diff.cpp:76:24:76:24 | p | IR only | -| test_diff.cpp:128:44:128:47 | argv | defaulttainttracking.cpp:9:11:9:20 | p#0 | AST only | -| test_diff.cpp:128:44:128:47 | argv | test_diff.cpp:1:11:1:20 | p#0 | AST only | -| test_diff.cpp:128:44:128:47 | argv | test_diff.cpp:81:24:81:24 | p | AST only | -| test_diff.cpp:128:44:128:47 | argv | test_diff.cpp:82:14:82:14 | p | AST only | From 67d29e31ccf05e335aff6352fae34bcc3ae85d49 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 28 Jan 2020 17:52:45 +0100 Subject: [PATCH 29/34] C#: Sync identical files --- .../semmle/code/csharp/ir/implementation/raw/Instruction.qll | 4 ++++ .../csharp/ir/implementation/unaliased_ssa/Instruction.qll | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/csharp/ql/src/semmle/code/csharp/ir/implementation/raw/Instruction.qll b/csharp/ql/src/semmle/code/csharp/ir/implementation/raw/Instruction.qll index 7514f580813..4f4b0eafe62 100644 --- a/csharp/ql/src/semmle/code/csharp/ir/implementation/raw/Instruction.qll +++ b/csharp/ql/src/semmle/code/csharp/ir/implementation/raw/Instruction.qll @@ -947,6 +947,10 @@ class ConvertInstruction extends UnaryInstruction { ConvertInstruction() { getOpcode() instanceof Opcode::Convert } } +class CheckedConvertOrNullInstruction extends UnaryInstruction { + CheckedConvertOrNullInstruction() { getOpcode() instanceof Opcode::CheckedConvertOrNull } +} + /** * Represents an instruction that converts between two addresses * related by inheritance. diff --git a/csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/Instruction.qll b/csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/Instruction.qll index 7514f580813..4f4b0eafe62 100644 --- a/csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/Instruction.qll +++ b/csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/Instruction.qll @@ -947,6 +947,10 @@ class ConvertInstruction extends UnaryInstruction { ConvertInstruction() { getOpcode() instanceof Opcode::Convert } } +class CheckedConvertOrNullInstruction extends UnaryInstruction { + CheckedConvertOrNullInstruction() { getOpcode() instanceof Opcode::CheckedConvertOrNull } +} + /** * Represents an instruction that converts between two addresses * related by inheritance. From aff0a7534c9490f4e5ee9f4f2d23dc3c09a54312 Mon Sep 17 00:00:00 2001 From: Calum Grant <42069085+calumgrant@users.noreply.github.com> Date: Wed, 29 Jan 2020 11:44:17 +0000 Subject: [PATCH 30/34] Update change-notes/1.24/analysis-csharp.md Fix indentation Co-Authored-By: James Fletcher <42464962+jf205@users.noreply.github.com> --- change-notes/1.24/analysis-csharp.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/change-notes/1.24/analysis-csharp.md b/change-notes/1.24/analysis-csharp.md index 88657dbbc21..ecff3a7b436 100644 --- a/change-notes/1.24/analysis-csharp.md +++ b/change-notes/1.24/analysis-csharp.md @@ -9,7 +9,7 @@ The following changes in version 1.24 affect C# analysis in all applications. | Assembly path injection (`cs/assembly-path-injection`) | security, external/cwe/cwe-114 | Finds user-controlled data used to load an assembly. | | Insecure configuration for ASP.NET requestValidationMode (`cs/insecure-request-validation-mode`) | security, external/cwe/cwe-016 | Finds where this attribute has been set to a value less than 4.5, which turns off some validation features and makes the application less secure. | | Insecure SQL connection (`cs/insecure-sql-connection`) | security, external/cwe/cwe-327 | Finds unencrypted SQL connection strings. | -| Page request validation is disabled (`cs/web/request-validation-disabled`) | security, frameworks/asp.net, external/cwe/cwe-016 | Finds where ASP.NET page request validation has been disabled, which could makes the application less secure. | +| Page request validation is disabled (`cs/web/request-validation-disabled`) | security, frameworks/asp.net, external/cwe/cwe-016 | Finds where ASP.NET page request validation has been disabled, which could make the application less secure. | | Serialization check bypass (`cs/serialization-check-bypass`) | security, external/cwe/cwe-20 | Finds where data is not validated in a deserialization method. | | XML injection (`cs/xml-injection`) | security, external/cwe/cwe-091 | Finds user-controlled data that is used to write directly to an XML document. | @@ -33,4 +33,3 @@ The following changes in version 1.24 affect C# analysis in all applications. * Expression nullability flow state is given by the predicates `Expr.hasNotNullFlowState()` and `Expr.hasMaybeNullFlowState()`. ## Changes to autobuilder - From c0379cc3f1319e9450cb72ead5b885e08fcd3eee Mon Sep 17 00:00:00 2001 From: Calum Grant Date: Wed, 29 Jan 2020 11:46:28 +0000 Subject: [PATCH 31/34] C#: Address review comment: an SQL --- .../ql/src/Security Features/CWE-327/InsecureSQLConnection.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/csharp/ql/src/Security Features/CWE-327/InsecureSQLConnection.ql b/csharp/ql/src/Security Features/CWE-327/InsecureSQLConnection.ql index b8d407b4eb2..ffdbc531ade 100644 --- a/csharp/ql/src/Security Features/CWE-327/InsecureSQLConnection.ql +++ b/csharp/ql/src/Security Features/CWE-327/InsecureSQLConnection.ql @@ -1,6 +1,6 @@ /** * @name Insecure SQL connection - * @description Using a SQL Server connection without enforcing encryption is a security vulnerability. + * @description Using an SQL Server connection without enforcing encryption is a security vulnerability. * @kind path-problem * @id cs/insecure-sql-connection * @problem.severity error From 7bed6ad63b47d24a3b330c890c52a3974574c737 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Wed, 29 Jan 2020 15:40:26 +0100 Subject: [PATCH 32/34] C++: Add taint from gets through memcpy --- .../semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll b/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll index 5005d694a15..39d3eb27597 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll @@ -199,7 +199,11 @@ private Instruction getACallArgumentOrIndirection(CallInstruction call, int argu private predicate modelTaintToParameter(Function f, int parameterIn, int parameterOut) { exists(FunctionInput modelIn, FunctionOutput modelOut | - f.(TaintFunction).hasTaintFlow(modelIn, modelOut) and + ( + f.(DataFlowFunction).hasDataFlow(modelIn, modelOut) + or + f.(TaintFunction).hasTaintFlow(modelIn, modelOut) + ) and (modelIn.isParameter(parameterIn) or modelIn.isParameterDeref(parameterIn)) and modelOut.isParameterDeref(parameterOut) ) From 0436caecdcc72e9f18b0a8000af2482fb81b7676 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Wed, 29 Jan 2020 16:03:35 +0100 Subject: [PATCH 33/34] C++: Always use the old library for the diff test This change ensures that the diff test will show the difference between the old and the new library even after we switch the default implementation of `security.TaintTracking` to be the new one. --- .../library-tests/dataflow/DefaultTaintTracking/test_diff.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/test_diff.ql b/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/test_diff.ql index eed0e793770..b31147fd423 100644 --- a/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/test_diff.ql +++ b/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/test_diff.ql @@ -1,6 +1,6 @@ import cpp import semmle.code.cpp.security.Security -import semmle.code.cpp.security.TaintTracking as ASTTaintTracking +import semmle.code.cpp.security.TaintTrackingImpl as ASTTaintTracking import semmle.code.cpp.ir.dataflow.DefaultTaintTracking as IRDefaultTaintTracking predicate astFlow(Expr source, Element sink) { ASTTaintTracking::tainted(source, sink) } From 52d2bebd1c81804ad42bd635dc89926069e640f2 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Wed, 29 Jan 2020 17:54:49 +0100 Subject: [PATCH 34/34] C++: Taint through most partial chi operands This changes the flow to be taint rather than data flow, and it extends it to include chi instructions with unknown type as long as they're not for the `AliasedVirtualVariable`. We're losing three good test results because these tests are not affected by `DefaultTaintTracking.qll`. The taint step added here can later be ported to `TaintTrackingUtil.qll` to recover these results, but we probably want a better API than transitive-closure search through instructions before doing that. --- .../cpp/ir/dataflow/DefaultTaintTracking.qll | 16 ++++++++++++++++ .../cpp/ir/dataflow/internal/DataFlowUtil.qll | 14 +++----------- .../library-tests/dataflow/taint-tests/taint.cpp | 6 +++--- .../dataflow/taint-tests/test_diff.expected | 3 --- .../dataflow/taint-tests/test_ir.expected | 3 --- 5 files changed, 22 insertions(+), 20 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll b/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll index 5005d694a15..d8ee2e92261 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll @@ -149,6 +149,9 @@ private predicate instructionTaintStep(Instruction i1, Instruction i2) { or i2.(UnaryInstruction).getUnary() = i1 or + i2.(ChiInstruction).getPartial() = i1 and + not isChiForAllAliasedMemory(i2) + or exists(BinaryInstruction bin | bin = i2 and predictableInstruction(i2.getAnOperand().getDef()) and @@ -205,6 +208,19 @@ private predicate modelTaintToParameter(Function f, int parameterIn, int paramet ) } +/** + * Holds if `chi` is on the chain of chi-instructions for all aliased memory. + * Taint shoud not pass through these instructions since they tend to mix up + * unrelated objects. + */ +private predicate isChiForAllAliasedMemory(Instruction instr) { + instr.(ChiInstruction).getTotal() instanceof AliasedDefinitionInstruction + or + isChiForAllAliasedMemory(instr.(ChiInstruction).getTotal()) + or + isChiForAllAliasedMemory(instr.(PhiInstruction).getAnInput()) +} + private predicate modelTaintToReturnValue(Function f, int parameterIn) { // Taint flow from parameter to return value exists(FunctionInput modelIn, FunctionOutput modelOut | diff --git a/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll b/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll index 5ebd2f12b00..ab7bfb6d0c9 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll @@ -283,18 +283,10 @@ private predicate simpleInstructionLocalFlowStep(Instruction iFrom, Instruction // By allowing flow through the total operand, we ensure that flow is not lost // due to shortcomings of the alias analysis. We may get false flow in cases // where the data is indeed overwritten. + // + // Flow through the partial operand belongs in the taint-tracking libraries + // for now. iTo.getAnOperand().(ChiTotalOperand).getDef() = iFrom - or - // Flow through the partial operand must be restricted a bit more. For - // soundness, the IR has to assume that every write to an unknown address can - // affect every escaped variable, and this assumption shows up as data flowing - // through partial chi operands. The chi instructions for all escaped data can - // be recognized by having unknown types. For all other chi instructions, flow - // through partial operands is more likely to be real. - exists(ChiInstruction chi | iTo = chi | - iFrom = chi.getPartial() and - not chi.getResultIRType() instanceof IRUnknownType - ) } /** 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 4e802d4a4bb..3fff2fd7229 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp @@ -107,9 +107,9 @@ void array_test(int i) { arr3[5] = 0; sink(arr1[5]); // tainted - sink(arr1[i]); // tainted [NOT DETECTED with AST] - sink(arr2[5]); // tainted [NOT DETECTED with AST] - sink(arr2[i]); // tainted [NOT DETECTED with AST] + sink(arr1[i]); // tainted [NOT DETECTED] + sink(arr2[5]); // tainted [NOT DETECTED] + sink(arr2[i]); // tainted [NOT DETECTED] sink(arr3[5]); sink(arr3[i]); } diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected index 47ef298f093..659ea724637 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected @@ -17,9 +17,6 @@ | taint.cpp:93:11:93:11 | taint.cpp:71:22:71:27 | AST only | | taint.cpp:94:11:94:11 | taint.cpp:72:7:72:12 | AST only | | taint.cpp:109:7:109:13 | taint.cpp:105:12:105:17 | IR only | -| taint.cpp:110:7:110:13 | taint.cpp:105:12:105:17 | IR only | -| taint.cpp:111:7:111:13 | taint.cpp:106:12:106:17 | IR only | -| taint.cpp:112:7:112:13 | taint.cpp:106:12:106:17 | IR only | | taint.cpp:130:7:130:9 | taint.cpp:127:8:127:13 | IR only | | taint.cpp:137:7:137:9 | taint.cpp:120:11:120:16 | AST only | | taint.cpp:173:8:173:13 | taint.cpp:164:19:164:24 | AST only | 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 47d7030a12d..06010367fd7 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 @@ -2,9 +2,6 @@ | taint.cpp:16:8:16:14 | source1 | taint.cpp:12:22:12:27 | call to source | | taint.cpp:17:8:17:16 | ++ ... | taint.cpp:12:22:12:27 | call to source | | taint.cpp:109:7:109:13 | access to array | taint.cpp:105:12:105:17 | call to source | -| taint.cpp:110:7:110:13 | access to array | taint.cpp:105:12:105:17 | call to source | -| taint.cpp:111:7:111:13 | access to array | taint.cpp:106:12:106:17 | call to source | -| taint.cpp:112:7:112:13 | access to array | taint.cpp:106:12:106:17 | call to source | | taint.cpp:129:7:129:9 | * ... | taint.cpp:120:11:120:16 | call to source | | taint.cpp:130:7:130:9 | * ... | taint.cpp:127:8:127:13 | call to source | | taint.cpp:134:7:134:9 | * ... | taint.cpp:120:11:120:16 | call to source |