JS: More precise handling of default import fallback

2026-04-29 10:45:15 +02:00 · 2020-11-06 12:02:35 +00:00
parent d07e69e529
commit acb30e73bc
5 changed files with 68 additions and 12 deletions
--- a/javascript/ql/src/semmle/javascript/ES2015Modules.qll
+++ b/javascript/ql/src/semmle/javascript/ES2015Modules.qll
@@ -40,6 +40,40 @@ class ES2015Module extends Module {
  }
 }

+/**
+ * Holds if `mod` contains one or more named export declarations other than `default`.
+ */
+private predicate hasNamedExports(ES2015Module mod) {
+  mod.getAnExport().(ExportNamedDeclaration).getASpecifier().getExportedName() != "default"
+  or
+  exists(mod.getAnExport().(ExportNamedDeclaration).getAnExportedDecl())
+  or
+  // Bulk re-exports only export named bindings (not "default")
+  mod.getAnExport() instanceof BulkReExportDeclaration
+}
+
+/**
+ * Holds if this module contains a `default` export.
+ */
+private predicate hasDefaultExport(ES2015Module mod) {
+  // export default foo;
+  mod.getAnExport() instanceof ExportDefaultDeclaration
+  or
+  // export { foo as default };
+  mod.getAnExport().(ExportNamedDeclaration).getASpecifier().getExportedName() = "default"
+}
+
+/**
+ * Holds if `mod` contains both named and `default` exports.
+ *
+ * This is used to determine whether a default-import of the module should be reinterpreted
+ * as a namespace-import, to accomodate the non-standard behavior implemented by some compilers.
+ */
+private predicate hasBothNamedAndDefaultExports(ES2015Module mod) {
+  hasNamedExports(mod) and
+  hasDefaultExport(mod)
+}
+
 /**
 * An import declaration.
 *
@@ -70,6 +104,10 @@ class ImportDeclaration extends Stmt, Import, @import_declaration {
      is instanceof ImportNamespaceSpecifier and
      count(getASpecifier()) = 1
      or
+      // For compatibility with the non-standard implementation of default imports,
+      // treat default imports as namespace imports in cases where it can't cause ambiguity
+      // between named exports and the properties of a default-exported object.
+      not hasBothNamedAndDefaultExports(getImportedModule()) and
      is.getImportedName() = "default"
    )
    or
--- a/javascript/ql/src/semmle/javascript/NodeJS.qll
+++ b/javascript/ql/src/semmle/javascript/NodeJS.qll
@@ -205,15 +205,16 @@ private predicate isRequire(DataFlow::Node nd) {
  or
  isRequire(nd.getAPredecessor())
  or
-  // `import { createRequire } from 'module';` support.
-  // specialized to ES2015 modules to avoid recursion in the `DataFlow::moduleImport()` predicate.
-  exists(ImportDeclaration imp | imp.getImportedPath().getValue() = "module" |
-    nd =
-      imp
-          .getImportedModuleNode()
-          .(DataFlow::SourceNode)
-          .getAPropertyRead("createRequire")
-          .getACall()
+  // `import { createRequire } from 'module';`.
+  // specialized to ES2015 modules to avoid recursion in the `DataFlow::moduleImport()` predicate and to avoid
+  // negative recursion between `Import.getImportedModuleNode()` and `Import.getImportedModule()`.
+  exists(ImportDeclaration imp, DataFlow::SourceNode baseObj |
+    imp.getImportedPath().getValue() = "module"
+  |
+    baseObj =
+      [DataFlow::destructuredModuleImportNode(imp),
+          DataFlow::valueNode(imp.getASpecifier().(ImportNamespaceSpecifier))] and
+    nd = baseObj.getAPropertyRead("createRequire").getACall()
  )
 }

--- a/javascript/ql/test/library-tests/InterProceduralFlow/mixedExports.js
+++ b/javascript/ql/test/library-tests/InterProceduralFlow/mixedExports.js
@@ -0,0 +1,7 @@
+let source = 'tainted';
+
+export const x = source;
+
+export default {
+    x: 'safe'
+};
--- a/javascript/ql/test/library-tests/InterProceduralFlow/mixedExportsClient.js
+++ b/javascript/ql/test/library-tests/InterProceduralFlow/mixedExportsClient.js
@@ -0,0 +1,7 @@
+import defaultValue from './mixedExports';
+import { x } from './mixedExports';
+import * as ns from './mixedExports';
+
+let sink1 = defaultValue.x; // OK
+let sink2 = x; // NOT OK
+let sink3 = ns.x; // NOT OK
--- a/javascript/ql/test/library-tests/InterProceduralFlow/tests.expected
+++ b/javascript/ql/test/library-tests/InterProceduralFlow/tests.expected
@@ -1,6 +1,5 @@
 dataFlow
 | a.js:1:15:1:23 | "tainted" | b.js:4:13:4:40 | whoKnow ... Tainted |
-| a.js:1:15:1:23 | "tainted" | b.js:6:13:6:13 | x |
 | a.js:2:15:2:28 | "also tainted" | b.js:5:13:5:29 | notTaintedTrustMe |
 | async.js:2:16:2:23 | "source" | async.js:8:15:8:27 | await async() |
 | async.js:2:16:2:23 | "source" | async.js:13:15:13:20 | sync() |
@@ -26,6 +25,8 @@ dataFlow
 | global.js:2:15:2:24 | "tainted2" | global.js:10:13:10:22 | g(source2) |
 | global.js:5:22:5:35 | "also tainted" | global.js:9:13:9:22 | g(source1) |
 | global.js:5:22:5:35 | "also tainted" | global.js:10:13:10:22 | g(source2) |
+| mixedExports.js:1:14:1:22 | 'tainted' | mixedExportsClient.js:6:13:6:13 | x |
+| mixedExports.js:1:14:1:22 | 'tainted' | mixedExportsClient.js:7:13:7:16 | ns.x |
 | nodeJsLib.js:2:15:2:23 | "tainted" | esClient.js:7:13:7:18 | nj.foo |
 | nodeJsLib.js:2:15:2:23 | "tainted" | esClient.js:10:13:10:17 | njFoo |
 | nodeJsLib.js:2:15:2:23 | "tainted" | nodeJsClient.js:4:13:4:18 | nj.foo |
@@ -77,7 +78,6 @@ flowLabels
 | tst5.mjs:15:8:15:19 | source(flow) | tst5.mjs:16:13:16:16 | flow |
 taintTracking
 | a.js:1:15:1:23 | "tainted" | b.js:4:13:4:40 | whoKnow ... Tainted |
-| a.js:1:15:1:23 | "tainted" | b.js:6:13:6:13 | x |
 | a.js:2:15:2:28 | "also tainted" | b.js:5:13:5:29 | notTaintedTrustMe |
 | async.js:2:16:2:23 | "source" | async.js:7:14:7:20 | async() |
 | async.js:2:16:2:23 | "source" | async.js:8:15:8:27 | await async() |
@@ -106,6 +106,8 @@ taintTracking
 | global.js:2:15:2:24 | "tainted2" | global.js:10:13:10:22 | g(source2) |
 | global.js:5:22:5:35 | "also tainted" | global.js:9:13:9:22 | g(source1) |
 | global.js:5:22:5:35 | "also tainted" | global.js:10:13:10:22 | g(source2) |
+| mixedExports.js:1:14:1:22 | 'tainted' | mixedExportsClient.js:6:13:6:13 | x |
+| mixedExports.js:1:14:1:22 | 'tainted' | mixedExportsClient.js:7:13:7:16 | ns.x |
 | nodeJsLib.js:1:15:1:23 | "tainted" | esClient.js:7:13:7:18 | nj.foo |
 | nodeJsLib.js:1:15:1:23 | "tainted" | esClient.js:10:13:10:17 | njFoo |
 | nodeJsLib.js:1:15:1:23 | "tainted" | nodeJsClient.js:4:13:4:18 | nj.foo |
@@ -182,7 +184,6 @@ taintTracking
 | underscore.js:19:17:19:22 | "src5" | underscore.js:20:15:20:44 | _.map([ ... ource5) |
 germanFlow
 | a.js:1:15:1:23 | "tainted" | b.js:4:13:4:40 | whoKnow ... Tainted |
-| a.js:1:15:1:23 | "tainted" | b.js:6:13:6:13 | x |
 | a.js:2:15:2:28 | "also tainted" | b.js:5:13:5:29 | notTaintedTrustMe |
 | async.js:2:16:2:23 | "source" | async.js:8:15:8:27 | await async() |
 | async.js:2:16:2:23 | "source" | async.js:13:15:13:20 | sync() |
@@ -209,6 +210,8 @@ germanFlow
 | global.js:2:15:2:24 | "tainted2" | global.js:10:13:10:22 | g(source2) |
 | global.js:5:22:5:35 | "also tainted" | global.js:9:13:9:22 | g(source1) |
 | global.js:5:22:5:35 | "also tainted" | global.js:10:13:10:22 | g(source2) |
+| mixedExports.js:1:14:1:22 | 'tainted' | mixedExportsClient.js:6:13:6:13 | x |
+| mixedExports.js:1:14:1:22 | 'tainted' | mixedExportsClient.js:7:13:7:16 | ns.x |
 | nodeJsLib.js:2:15:2:23 | "tainted" | esClient.js:7:13:7:18 | nj.foo |
 | nodeJsLib.js:2:15:2:23 | "tainted" | esClient.js:10:13:10:17 | njFoo |
 | nodeJsLib.js:2:15:2:23 | "tainted" | nodeJsClient.js:4:13:4:18 | nj.foo |