diff --git a/change-notes/1.24/analysis-cpp.md b/change-notes/1.24/analysis-cpp.md index 94256075a88..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. - \ No newline at end of file + * The library now models data flow through formatting functions such as `sprintf`. diff --git a/change-notes/1.24/analysis-csharp.md b/change-notes/1.24/analysis-csharp.md index 2df9934dc48..88f4abe91cc 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. | -| 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. | +| 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 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. | ## Changes to existing queries @@ -30,4 +34,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 - diff --git a/change-notes/1.24/analysis-javascript.md b/change-notes/1.24/analysis-javascript.md index d367d235a8c..1405a26d186 100644 --- a/change-notes/1.24/analysis-javascript.md +++ b/change-notes/1.24/analysis-javascript.md @@ -38,6 +38,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/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.

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 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/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll b/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll index 5005d694a15..f6ad5246d8e 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 @@ -199,12 +202,29 @@ 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) ) } +/** + * 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 24598bdaf12..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 @@ -264,11 +264,17 @@ 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.(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 // analysis couldn't determine that it surely will overwrite every bit of it or @@ -278,10 +284,8 @@ private predicate simpleInstructionLocalFlowStep(Instruction iFrom, Instruction // 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. + // Flow through the partial operand belongs in the taint-tracking libraries + // for now. iTo.getAnOperand().(ChiTotalOperand).getDef() = iFrom } 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 d7a99025cc9..2acd2b824c0 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 @@ -949,6 +949,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. @@ -989,7 +993,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/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 6edca9ccc21..45c945c07ef 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 @@ -84,6 +84,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 d7a99025cc9..2acd2b824c0 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 @@ -949,6 +949,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. @@ -989,7 +993,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/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 d7a99025cc9..2acd2b824c0 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 @@ -949,6 +949,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. @@ -989,7 +993,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/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 6edca9ccc21..45c945c07ef 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 @@ -84,6 +84,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/models/interfaces/FormattingFunction.qll b/cpp/ql/src/semmle/code/cpp/models/interfaces/FormattingFunction.qll index 130c067f70c..7c035072916 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,8 @@ * `FormattingFunction` to match the flow within that function. */ -import semmle.code.cpp.Function +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 Function { +abstract class FormattingFunction extends ArrayFunction, TaintFunction { /** Gets the position at which the format parameter occurs. */ abstract int getFormatParameterIndex(); @@ -133,4 +134,33 @@ 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()) + } + + override predicate hasArrayInput(int bufParam) { bufParam = getFormatParameterIndex() } + + override predicate hasArrayOutput(int bufParam) { bufParam = getOutputParameterIndex() } + + override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) { + 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/DefaultTaintTracking/defaulttainttracking.cpp b/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/defaulttainttracking.cpp index 8958616c0d3..ebe38f1f060 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; } @@ -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 + 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")); // 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 d2b48f25bf9..92948eeffc6 100644 --- a/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/tainted.expected +++ b/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/tainted.expected @@ -21,6 +21,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:22:20:22:25 | call to getenv | test_diff.cpp:1:11:1:20 | p#0 | | 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 | @@ -30,6 +31,64 @@ | 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: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 | +| 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 | @@ -123,7 +182,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..c557f43f1c7 100644 --- a/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/test_diff.expected +++ b/cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/test_diff.expected @@ -5,10 +5,10 @@ | defaulttainttracking.cpp:22:20:22:25 | call to getenv | defaulttainttracking.cpp:3:21:3:22 | s1 | AST only | | defaulttainttracking.cpp:22:20:22:25 | call to getenv | defaulttainttracking.cpp:21:8:21:10 | buf | AST only | | defaulttainttracking.cpp:22:20:22:25 | call to getenv | defaulttainttracking.cpp:22:15:22:17 | buf | AST only | -| defaulttainttracking.cpp:22:20:22:25 | call to getenv | defaulttainttracking.cpp:24:8:24:10 | buf | AST only | | 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 +16,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 | 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) } 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..2080707f17f --- /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 + } + { + char buffer[256] = {0}; + sink(snprintf(buffer, 256, string::source(), "Hello.")); + sink(buffer); // tainted + } + { + char buffer[256] = {0}; + sink(snprintf(buffer, 256, "%s %s %s", "a", "b", string::source())); + sink(buffer); // tainted + } + { + char buffer[256] = {0}; + sink(snprintf(buffer, 256, "%.*s", 10, string::source())); + sink(buffer); // tainted + } + + { + 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); // tainted (debatable) + } + + { + char buffer[256] = {0}; + sink(sprintf(buffer, "%s", string::source())); + sink(buffer); // tainted + } + { + char buffer[256] = {0}; + sink(sprintf(buffer, "%ls", wstring::source())); + sink(buffer); // tainted + } + { + wchar_t wbuffer[256] = {0}; + sink(swprintf(wbuffer, 256, L"%s", wstring::source())); + sink(wbuffer); // tainted + } + { + char buffer[256] = {0}; + sink(mysprintf(buffer, 256, "%s", string::source())); + sink(buffer); // tainted [NOT DETECTED - implement UserDefinedFormattingFunction.getOutputParameterIndex()] + } + + { + 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..648159b4944 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,108 @@ | 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: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: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 | +| 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: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 | +| 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 | +| 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 | | 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..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,3 +1,13 @@ +| 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 | +| 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 1d875493db0..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,3 +1,13 @@ +| 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 | +| 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 | 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); 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..ffdbc531ade 100644 --- a/csharp/ql/src/Security Features/CWE-327/InsecureSQLConnection.ql +++ b/csharp/ql/src/Security Features/CWE-327/InsecureSQLConnection.ql @@ -4,14 +4,11 @@ * @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 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 d7a99025cc9..2acd2b824c0 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 @@ -949,6 +949,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. @@ -989,7 +993,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 d7a99025cc9..2acd2b824c0 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 @@ -949,6 +949,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. @@ -989,7 +993,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/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql b/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql index 192f32dc79b..f7db881fdf0 100644 --- a/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql +++ b/javascript/ql/src/Security/CWE-352/MissingCsrfMiddleware.ql @@ -12,6 +12,56 @@ import javascript +/** 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`, `session`, or `user`. */ +private DataFlow::SourceNode nodeLeadingToCookieAccess(DataFlow::TypeBackTracker t) { + t.start() and + exists(DataFlow::PropRead value | + 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 + + // Ignore calls like `req.session.save()` + not value = any(DataFlow::InvokeNode call).getCalleeNode() + ) + 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`, `session`, or `user`. */ +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 +113,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) and hasCookieMiddleware(handler, cookie) and + + // Only flag the 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" 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..1f886776d23 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,8 @@ -| 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 | +| 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/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..133304faac9 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-352/unused_cookies.js @@ -0,0 +1,39 @@ +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.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.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();