From ea3560fe076c521b0511efda46139727b40ab296 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Fri, 5 Jun 2020 11:34:43 +0100 Subject: [PATCH 1/3] JS: Ignore document.all checks explicitly --- .../UnneededDefensiveProgramming.ql | 28 +++++++++++++++++-- .../javascript/DefensiveProgramming.qll | 13 +++++++-- .../UnneededDefensiveProgramming/tst2.js | 8 ++++++ 3 files changed, 44 insertions(+), 5 deletions(-) diff --git a/javascript/ql/src/Expressions/UnneededDefensiveProgramming.ql b/javascript/ql/src/Expressions/UnneededDefensiveProgramming.ql index 099d033b992..810f48e2275 100644 --- a/javascript/ql/src/Expressions/UnneededDefensiveProgramming.ql +++ b/javascript/ql/src/Expressions/UnneededDefensiveProgramming.ql @@ -13,19 +13,43 @@ import javascript import semmle.javascript.DefensiveProgramming +/** + * Holds if `e` looks like a check for `document.all`, which is an unusual browser object which coerces + * to `false` and has typeof `undefined`. + */ +predicate isFalsyObjectCheck(LogicalBinaryExpr e) { + exists(Variable v | + e.getAnOperand().(DefensiveExpressionTest::TypeofUndefinedTest).getOperand() = v.getAnAccess() and + e.getAnOperand().(DefensiveExpressionTest::UndefinedComparison).getOperand() = v.getAnAccess() + ) +} + +/** + * Holds if `e` is part of a check for `document.all`. + */ +predicate isPartOfFalsyObjectCheck(Expr e) { + exists(LogicalBinaryExpr binary | + isFalsyObjectCheck(binary) and + e = binary.getAnOperand() + ) + or + isFalsyObjectCheck(e) +} + from DefensiveExpressionTest e, boolean cv where + not isPartOfFalsyObjectCheck(e.asExpr()) and e.getTheTestResult() = cv and // whitelist not ( // module environment detection exists(VarAccess access, string name | name = "exports" or name = "module" | - e.asExpr().(Internal::TypeofUndefinedTest).getOperand() = access and + e.asExpr().(DefensiveExpressionTest::TypeofUndefinedTest).getOperand() = access and access.getName() = name and not exists(access.getVariable().getADeclaration()) ) or // too benign in practice - e instanceof Internal::DefensiveInit + e instanceof DefensiveExpressionTest::DefensiveInit ) select e, "This guard always evaluates to " + cv + "." diff --git a/javascript/ql/src/semmle/javascript/DefensiveProgramming.qll b/javascript/ql/src/semmle/javascript/DefensiveProgramming.qll index 4590f2c5f3b..5f4e16aae10 100644 --- a/javascript/ql/src/semmle/javascript/DefensiveProgramming.qll +++ b/javascript/ql/src/semmle/javascript/DefensiveProgramming.qll @@ -14,9 +14,9 @@ abstract class DefensiveExpressionTest extends DataFlow::ValueNode { } /** - * INTERNAL: Do not use directly; use `DefensiveExpressionTest` instead. + * Provides classes for specific kinds of defensive programming patterns. */ -module Internal { +module DefensiveExpressionTest { /** * A defensive truthiness check that may be worth keeping, even if it * is strictly speaking useless. @@ -187,6 +187,13 @@ module Internal { override Expr getOperand() { result = operand } } + /** + * Comparison against `undefined`, such as `x === undefined`. + */ + class UndefinedComparison extends NullUndefinedComparison { + UndefinedComparison() { op2type = TTUndefined() } + } + /** * An expression that throws an exception if one of its subexpressions evaluates to `null` or `undefined`. * @@ -380,7 +387,7 @@ module Internal { /** * A test for `undefined` using a `typeof` expression. * - * Example: `typeof x === undefined'. + * Example: `typeof x === "undefined"'. */ class TypeofUndefinedTest extends UndefinedNullTest { TypeofTest test; diff --git a/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/tst2.js b/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/tst2.js index 4c6ad02a6bf..588844f9c75 100644 --- a/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/tst2.js +++ b/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/tst2.js @@ -8,3 +8,11 @@ } }); }); + +const isFalsyObject = (v) => typeof v === 'undefined' && v !== undefined; // OK + +function f(v) { + if (typeof v === 'undefined' && v !== undefined) { // OK + doSomething(v); + } +} From a109c1fc968a04dc56d6ddd217ed88aa812b1f33 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Fri, 5 Jun 2020 16:37:56 +0100 Subject: [PATCH 2/3] JS: Change note --- change-notes/1.25/analysis-javascript.md | 1 + 1 file changed, 1 insertion(+) diff --git a/change-notes/1.25/analysis-javascript.md b/change-notes/1.25/analysis-javascript.md index 122dc7c9551..2d55aea65ad 100644 --- a/change-notes/1.25/analysis-javascript.md +++ b/change-notes/1.25/analysis-javascript.md @@ -63,6 +63,7 @@ | Uncontrolled command line (`js/command-line-injection`) | More results | This query now recognizes additional command execution calls. | | Uncontrolled data used in path expression (`js/path-injection`) | More results | This query now recognizes additional file system calls. | | Unknown directive (`js/unknown-directive`) | Fewer results | This query no longer flags directives generated by the Babel compiler. | +| Unneeded defensive code (`js/unneeded-defensive-code`) | Fewer false-positive results | This query now recognizes checks meant to handle the `document.all` object. | | Unused property (`js/unused-property`) | Fewer results | This query no longer flags properties of objects that are operands of `yield` expressions. | | Zip Slip (`js/zipslip`) | More results | This query now recognizes additional vulnerabilities. | From f9b796231b584c1b72c28d5430191e138dd89ce5 Mon Sep 17 00:00:00 2001 From: Asger Feldthaus Date: Thu, 25 Jun 2020 11:10:27 +0100 Subject: [PATCH 3/3] JS: Add regression tests --- .../UnneededDefensiveProgramming.expected | 2 ++ .../UnneededDefensiveProgramming/regression.js | 15 +++++++++++++++ 2 files changed, 17 insertions(+) create mode 100644 javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/regression.js diff --git a/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/UnneededDefensiveProgramming.expected b/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/UnneededDefensiveProgramming.expected index 0bb0b3c589c..7af4b09b32e 100644 --- a/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/UnneededDefensiveProgramming.expected +++ b/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/UnneededDefensiveProgramming.expected @@ -1,4 +1,6 @@ | module-environment-detection.js:23:8:23:36 | typeof ... efined' | This guard always evaluates to true. | +| regression.js:9:9:9:12 | date | This guard always evaluates to true. | +| regression.js:13:25:13:40 | obj != undefined | This guard always evaluates to true. | | tst2.js:4:12:4:35 | typeof ... efined" | This guard always evaluates to true. | | tst.js:18:5:18:5 | u | This guard always evaluates to false. | | tst.js:19:5:19:5 | n | This guard always evaluates to false. | diff --git a/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/regression.js b/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/regression.js new file mode 100644 index 00000000000..cfc6f1e6df7 --- /dev/null +++ b/javascript/ql/test/query-tests/Expressions/UnneededDefensiveProgramming/regression.js @@ -0,0 +1,15 @@ +function getDate() { + var date; + if (something()) { + date = new Date(); + } else { + return null; + } + console.log(date); + return date && date.getTime(); // NOT OK +} + +function isNotNullOrString(obj) { + return obj != null && obj != undefined && // NOT OK + typeof obj != 'string'; +}