diff --git a/javascript/ql/lib/semmle/javascript/dataflow/AdditionalTaintSteps.qll b/javascript/ql/lib/semmle/javascript/dataflow/AdditionalTaintSteps.qll index fa52b5681738..a0752d768fa5 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/AdditionalTaintSteps.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/AdditionalTaintSteps.qll @@ -232,10 +232,7 @@ class LegacyTaintStep extends Unit { cached private module Cached { cached - predicate forceStage() { - // TODO: ensure that this stage is only evaluated if using the old data flow library - Stages::Taint::ref() - } + predicate forceStage() { Stages::Taint::ref() } /** * Holds if `pred` → `succ` should be considered a taint-propagating diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll index f015cc1e5c90..0e95d3511559 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll @@ -20,7 +20,6 @@ private class Node = DataFlow::Node; class PostUpdateNode = DataFlow::PostUpdateNode; -// TODO: this bypasses refinement nodes, and therefore some sanitisers class SsaUseNode extends DataFlow::Node, TSsaUseNode { private ControlFlowNode expr; @@ -524,7 +523,7 @@ private predicate isArgumentNodeImpl(Node n, DataFlowCall call, ArgumentPosition // receiver of accessor call pos.isThis() and n = call.asAccessorCall().getBase() or - // argument to setter (TODO: this has no post-update node) + // argument to setter pos.asPositional() = 0 and n = call.asAccessorCall().(DataFlow::PropWrite).getRhs() or FlowSummaryImpl::Private::summaryArgumentNode(call.(SummaryCall).getReceiver(), @@ -640,7 +639,7 @@ predicate nodeIsHidden(Node node) { node instanceof CaptureNode or // Hide function expressions, as capture-flow causes them to appear in unhelpful ways - // TODO: Instead hide PathNodes with a capture content as the head of its access path? + // In the future we could hide PathNodes with a capture content as the head of its access path. node.asExpr() instanceof Function or // Also hide post-update nodes for function expressions @@ -1402,7 +1401,7 @@ predicate readStep(Node node1, ContentSet c, Node node2) { // shift known array indices c.asSingleton().asArrayIndex() = content.asArrayIndex() + restIndex or - content.isUnknownArrayElement() and // TODO: don't read unknown array elements from static array + content.isUnknownArrayElement() and c = ContentSet::arrayElementUnknown() ) or diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/VariableCapture.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/VariableCapture.qll index 6e381a792efe..32ce88169e67 100644 --- a/javascript/ql/lib/semmle/javascript/dataflow/internal/VariableCapture.qll +++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/VariableCapture.qll @@ -114,7 +114,6 @@ module VariableCaptureConfig implements InputSig { class Callable extends js::StmtContainer { predicate isConstructor() { - // TODO: clarify exactly what the library wants to know here as the meaning of "constructor" varies between languages. // JS constructors should not be seen as "constructors" in this context. none() } @@ -254,7 +253,7 @@ js::DataFlow::Node getNodeFromClosureNode(VariableCaptureOutput::ClosureNode nod TValueNode(node.(VariableCaptureOutput::ParameterNode) .getParameter() .asLocalVariable() - .getADeclaration()) // TODO: is this subsumed by the ExprNode case? + .getADeclaration()) or result = TThisNode(node.(VariableCaptureOutput::ParameterNode).getParameter().asThisContainer()) or diff --git a/javascript/ql/lib/semmle/javascript/internal/flow_summaries/Arrays.qll b/javascript/ql/lib/semmle/javascript/internal/flow_summaries/Arrays.qll index 702936b46c3c..d9f0836d739d 100644 --- a/javascript/ql/lib/semmle/javascript/internal/flow_summaries/Arrays.qll +++ b/javascript/ql/lib/semmle/javascript/internal/flow_summaries/Arrays.qll @@ -140,7 +140,7 @@ class CopyWithin extends SummarizedCallable { input = "Argument[this].WithArrayElement" and output = "ReturnValue" or - // TODO: workaround for WithArrayElement not being converted to a taint step + // Explicitly add a taint step since WithArrayElement is not implicitly converted to a taint step preservesValue = false and input = "Argument[this]" and output = "ReturnValue" @@ -186,7 +186,7 @@ class Filter extends SummarizedCallable { output = "ReturnValue" ) or - // TODO: workaround for WithArrayElement not being converted to a taint step + // Explicitly add a taint step since WithArrayElement is not implicitly converted to a taint step preservesValue = false and input = "Argument[this]" and output = "ReturnValue" @@ -328,10 +328,7 @@ class From1Arg extends SummarizedCallable { output = "ReturnValue[exception]" ) or - // TODO: we currently convert ArrayElement read/store steps to taint steps, but this does not - // work for WithArrayElement because it's just an expectsContent node, and there's no way easy - // to omit the expectsContent restriction in taint tracking. - // Work around this for now. + // Explicitly add a taint step since WithArrayElement is not implicitly converted to a taint step preservesValue = false and input = "Argument[0]" and output = "ReturnValue" @@ -561,7 +558,7 @@ class ArrayCoercionPackage extends FunctionalPackageSummary { output = "ReturnValue.ArrayElement" ) or - // TODO: workaround for WithArrayElement not being converted to a taint step + // Explicitly add a taint step since WithArrayElement is not implicitly converted to a taint step preservesValue = false and input = "Argument[0]" and output = "ReturnValue" diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/DomBasedXssQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/DomBasedXssQuery.qll index 6979ec12a2e7..f27f61822a82 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/DomBasedXssQuery.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/DomBasedXssQuery.qll @@ -105,7 +105,6 @@ module DomBasedXssConfig implements DataFlow::StateConfigSig { state1.isTaintedUrlSuffix() and state2.isTaintedPrefix() or - // FIXME: this fails to work in the test case at jquery.js:37 exists(DataFlow::FunctionNode callback, DataFlow::Node arg | any(JQuery::MethodCall c).interpretsArgumentAsHtml(arg) and callback = arg.getABoundFunctionValue(_) and diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentQuery.qll index e385640d6960..2793757add8d 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentQuery.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentQuery.qll @@ -65,10 +65,7 @@ module PrototypePollutingAssignmentConfig implements DataFlow::StateConfigSig { state = FlowState::objectPrototype() } - predicate isBarrierIn(DataFlow::Node node, FlowState state) { - // FIXME: This should only be an in-barrier for the corresponding flow state, but flow-state specific in-barriers are not supported right now. - isSource(node, state) - } + predicate isBarrierIn(DataFlow::Node node, FlowState state) { isSource(node, state) } predicate isAdditionalFlowStep( DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2 @@ -99,11 +96,6 @@ module PrototypePollutingAssignmentConfig implements DataFlow::StateConfigSig { state2 = FlowState::objectPrototype() ) or - // TODO: local field step becomes a jump step, resulting in FPs (closure-lib) - // TODO: localFieldStep is too expensive with dataflow2 - // DataFlow::localFieldStep(pred, succ) - none() - or state1 = FlowState::taint() and TaintTracking::defaultTaintStep(node1, node2) and state1 = state2 diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeCodeConstruction.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeCodeConstruction.qll index fc1e6c79b384..a7580b161ecc 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeCodeConstruction.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeCodeConstruction.qll @@ -29,10 +29,6 @@ module UnsafeCodeConstruction { predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { // HTML sanitizers are insufficient protection against code injection node1 = node2.(HtmlSanitizerCall).getInput() - or - none() - // TODO: localFieldStep is too expensive with dataflow2 - // DataFlow::localFieldStep(pred, succ) } DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext } diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll index 5fdf3825405f..700c47565fc3 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll @@ -47,12 +47,6 @@ module UnsafeHtmlConstructionConfig implements DataFlow::StateConfigSig { predicate isAdditionalFlowStep( DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2 ) { - // TODO: localFieldStep is too expensive with dataflow2 - // DataFlow::localFieldStep(pred, succ) and - // inlbl.isTaint() and - // outlbl.isTaint() - none() - or TaintedObject::isAdditionalFlowStep(node1, state1, node2, state2) or // property read from a tainted object is considered tainted diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeJQueryPluginQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeJQueryPluginQuery.qll index e7bf16cf0c32..61bb129aa377 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeJQueryPluginQuery.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeJQueryPluginQuery.qll @@ -11,7 +11,9 @@ import UnsafeJQueryPluginCustomizations::UnsafeJQueryPlugin * A taint-tracking configuration for reasoning about XSS in unsafe jQuery plugins. */ module UnsafeJQueryPluginConfig implements DataFlow::ConfigSig { - // TODO: PropertyPresenceSanitizer should not block values in a content. + // Note: This query currently misses some results due to two issues: + // - PropertyPresenceSanitizer blocks values in a content + // - localFieldStep has been omitted for performance reaons predicate isSource(DataFlow::Node source) { source instanceof Source } predicate isSink(DataFlow::Node sink) { sink instanceof Sink } @@ -23,11 +25,6 @@ module UnsafeJQueryPluginConfig implements DataFlow::ConfigSig { } predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node sink) { - // jQuery plugins tend to be implemented as classes that store data in fields initialized by the constructor. - // TODO: localFieldStep is too expensive with dataflow2 - // DataFlow::localFieldStep(pred, succ) - none() - or aliasPropertyPresenceStep(node1, sink) } diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionQuery.qll index 5eb11826bffd..54fa18795d8f 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionQuery.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionQuery.qll @@ -14,8 +14,6 @@ import UnsafeShellCommandConstructionCustomizations::UnsafeShellCommandConstruct * A taint-tracking configuration for reasoning about shell command constructed from library input vulnerabilities. */ module UnsafeShellCommandConstructionConfig implements DataFlow::ConfigSig { - // TODO: we get a FP in the test case due to SanitizingRegExpTest not being able to generate a barrier edge - // for an edge into a phi node. predicate isSource(DataFlow::Node source) { source instanceof Source } predicate isSink(DataFlow::Node sink) { sink instanceof Sink } @@ -26,12 +24,6 @@ module UnsafeShellCommandConstructionConfig implements DataFlow::ConfigSig { node = TaintTracking::AdHocWhitelistCheckSanitizer::getABarrierNode() } - predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { - none() - // TODO: localFieldStep is too expensive with dataflow2 - // DataFlow::localFieldStep(pred, succ) - } - DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext } } diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/XssThroughDomQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/XssThroughDomQuery.qll index 2313751d9e70..74a840b05b26 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/XssThroughDomQuery.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/XssThroughDomQuery.qll @@ -12,8 +12,6 @@ private import semmle.javascript.security.dataflow.UnsafeJQueryPluginCustomizati * A taint-tracking configuration for reasoning about XSS through the DOM. */ module XssThroughDomConfig implements DataFlow::ConfigSig { - // NOTE: Gained FP in Lucifier due to spurious source but with more data flow (I think). - // TODO: Seen unexplained FP in meteor, likely due to spurious flow into a callback coming from another call site predicate isSource(DataFlow::Node source) { source instanceof Source } predicate isSink(DataFlow::Node sink) { sink instanceof DomBasedXss::Sink } diff --git a/javascript/ql/lib/semmle/javascript/security/regexp/PolynomialReDoSQuery.qll b/javascript/ql/lib/semmle/javascript/security/regexp/PolynomialReDoSQuery.qll index 0c0f502bb06b..8b21f8e98b39 100644 --- a/javascript/ql/lib/semmle/javascript/security/regexp/PolynomialReDoSQuery.qll +++ b/javascript/ql/lib/semmle/javascript/security/regexp/PolynomialReDoSQuery.qll @@ -22,11 +22,7 @@ module PolynomialReDoSConfig implements DataFlow::ConfigSig { DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext } - predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { - none() - // TODO: localFieldStep is too expensive with dataflow2 - // DataFlow::localFieldStep(pred, succ) - } + predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { none() } int fieldFlowBranchLimit() { result = 1 } // library inputs are too expensive on some projects } diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/lib/lib.js b/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/lib/lib.js index 09488f0a8871..75fda0090000 100644 --- a/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/lib/lib.js +++ b/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/lib/lib.js @@ -250,7 +250,7 @@ module.exports.goodSanitizer = function (name) { var cleaned = cleanInput(name); - cp.exec("rm -rf " + cleaned); // OK + cp.exec("rm -rf " + cleaned); // OK - But FP due to SanitizingRegExpTest not being able to generate a barrier edge for an edge into a phi node. } var fs = require("fs");