Merge pull request #982 from asger-semmle/closure-string-lib

JS: model string functions from closure library
2026-04-28 10:15:14 +02:00 · 2019-02-26 08:26:14 +00:00
parent 53de2d8d3e 93440014a0
commit c2a5350bf2
13 changed files with 110 additions and 0 deletions
--- a/javascript/ql/src/javascript.qll
+++ b/javascript/ql/src/javascript.qll
@@ -61,6 +61,7 @@ import semmle.javascript.frameworks.Azure
 import semmle.javascript.frameworks.Babel
 import semmle.javascript.frameworks.ComposedFunctions
 import semmle.javascript.frameworks.ClientRequests
+import semmle.javascript.frameworks.ClosureLibrary
 import semmle.javascript.frameworks.CookieLibraries
 import semmle.javascript.frameworks.Credentials
 import semmle.javascript.frameworks.CryptoLibraries
--- a/javascript/ql/src/semmle/javascript/HtmlSanitizers.qll
+++ b/javascript/ql/src/semmle/javascript/HtmlSanitizers.qll
@@ -54,6 +54,8 @@ private class DefaultHtmlSanitizerCall extends HtmlSanitizerCall {
              .getAPropertyRead(name) or
        callee = DataFlow::moduleMember("html-entities", _).getAPropertyRead(name)
      )
+      or
+      callee = Closure::moduleImport("goog.string.htmlEscape")
    )
    or
    // Match home-made sanitizers by name.
--- a/javascript/ql/src/semmle/javascript/StringConcatenation.qll
+++ b/javascript/ql/src/semmle/javascript/StringConcatenation.qll
@@ -47,6 +47,11 @@ module StringConcatenation {
        n = 0
      )
    )
+    or
+    exists(DataFlow::CallNode call | node = call |
+      call = Closure::moduleImport("goog.string.buildString").getACall() and
+      result = call.getArgument(n)
+    )
  }

  /** Gets an operand to the string concatenation defining `node`. */
--- a/javascript/ql/src/semmle/javascript/frameworks/ClosureLibrary.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/ClosureLibrary.qll
@@ -0,0 +1,56 @@
+/**
+ * Provides models for miscellaneous utility functions in the closure standard library.
+ */
+
+import javascript
+
+module ClosureLibrary {
+  private import DataFlow
+
+  private class StringStep extends TaintTracking::AdditionalTaintStep, CallNode {
+    Node pred;
+
+    StringStep() {
+      exists (string name | this = Closure::moduleImport("goog.string." + name).getACall() |
+        pred = getAnArgument() and
+        (
+          name = "canonicalizeNewlines" or 
+          name = "capitalize" or
+          name = "collapseBreakingSpaces" or
+          name = "collapseWhitespace" or
+          name = "format" or
+          name = "makeSafe" or // makeSafe just guards against null and undefined
+          name = "newLineOrBr" or
+          name = "normalizeSpaces" or
+          name = "normalizeWhitespace" or
+          name = "preserveSpaces" or
+          name = "remove" or // removes first occurrence of a substring
+          name = "repeat" or
+          name = "splitLimit" or
+          name = "stripNewlines" or
+          name = "subs" or
+          name = "toCamelCase" or
+          name = "toSelectorCase" or
+          name = "toTitleCase" or
+          name = "trim" or
+          name = "trimLeft" or
+          name = "trimRight" or
+          name = "unescapeEntities" or
+          name = "whitespaceEscape"
+        )
+        or
+        pred = getArgument(0) and
+        (
+          name = "truncate" or
+          name = "truncateMiddle" or
+          name = "unescapeEntitiesWithDocument"
+        )
+      )
+    }
+
+    override predicate step(Node src, Node dst) {
+      src = pred and
+      dst = this
+    }
+  }
+}
--- a/javascript/ql/src/semmle/javascript/frameworks/UriLibraries.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/UriLibraries.qll
@@ -355,6 +355,13 @@ private module ClosureLibraryUri {
        name = "setPath" or
        name = "split"
      )
+      or
+      // static methods in goog.string
+      arg = 0 and
+      exists(string name | this = Closure::moduleImport("goog.string." + name).getACall() |
+        name = "urlDecode" or
+        name = "urlEncode"
+      )
    }

    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
--- a/javascript/ql/test/library-tests/HtmlSanitizers/HtmlSanitizerCalls.expected
+++ b/javascript/ql/test/library-tests/HtmlSanitizers/HtmlSanitizerCalls.expected
@@ -1,3 +1,4 @@
+| closure.js:5:1:5:29 | checkEs ... ipt>')) | OK |
 | tst.js:17:1:17:47 | checkEs ... ipt>')) | OK |
 | tst.js:18:1:18:56 | checkEs ... ipt>')) | OK |
 | tst.js:19:1:19:55 | checkEs ... ipt>')) | OK |
--- a/javascript/ql/test/library-tests/HtmlSanitizers/closure.js
+++ b/javascript/ql/test/library-tests/HtmlSanitizers/closure.js
@@ -0,0 +1,5 @@
+goog.module('test');
+
+let esc = goog.require('goog.string.htmlEscape');
+
+checkEscaped(esc('<script>'));
--- a/javascript/ql/test/library-tests/StringConcatenation/ContainsTwo.expected
+++ b/javascript/ql/test/library-tests/StringConcatenation/ContainsTwo.expected
@@ -1,3 +1,7 @@
+| closure.js:5:1:5:37 | build(' ... 'four') |
+| closure.js:5:1:5:46 | build(' ...  'five' |
+| closure.js:5:14:5:18 | 'two' |
+| closure.js:5:14:5:28 | 'two' + 'three' |
 | tst.js:3:3:3:12 | x += "two" |
 | tst.js:3:8:3:12 | "two" |
 | tst.js:4:3:4:3 | x |
--- a/javascript/ql/test/library-tests/StringConcatenation/closure.js
+++ b/javascript/ql/test/library-tests/StringConcatenation/closure.js
@@ -0,0 +1,5 @@
+goog.module('test');
+
+let build = goog.require('goog.string.buildString');
+
+build('one', 'two' + 'three', 'four') + 'five';
--- a/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected
+++ b/javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected
@@ -9,6 +9,9 @@
 | callbacks.js:44:17:44:24 | source() | callbacks.js:41:10:41:10 | x |
 | callbacks.js:50:18:50:25 | source() | callbacks.js:30:29:30:29 | y |
 | callbacks.js:51:18:51:25 | source() | callbacks.js:30:29:30:29 | y |
+| closure.js:6:15:6:22 | source() | closure.js:8:8:8:31 | string. ... (taint) |
+| closure.js:6:15:6:22 | source() | closure.js:9:8:9:25 | string.trim(taint) |
+| closure.js:6:15:6:22 | source() | closure.js:10:8:10:33 | string. ... nt, 50) |
 | constructor-calls.js:4:18:4:25 | source() | constructor-calls.js:18:8:18:14 | c.taint |
 | constructor-calls.js:4:18:4:25 | source() | constructor-calls.js:22:8:22:19 | c_safe.taint |
 | constructor-calls.js:10:16:10:23 | source() | constructor-calls.js:26:8:26:14 | d.taint |
--- a/javascript/ql/test/library-tests/TaintTracking/closure.js
+++ b/javascript/ql/test/library-tests/TaintTracking/closure.js
@@ -0,0 +1,14 @@
+goog.module('test');
+
+let string = goog.require('goog.string');
+
+function test() {
+  let taint = source();
+  
+  sink(string.capitalize(taint)); // NOT OK
+  sink(string.trim(taint)); // NOT OK
+  sink(string.truncate(taint, 50)); // NOT OK
+  sink(string.truncate('hey', taint)); // OK
+
+  sink(string.escapeString(taint)); // OK
+}
--- a/javascript/ql/test/library-tests/frameworks/UriLibraries/UriLibraryStep.expected
+++ b/javascript/ql/test/library-tests/frameworks/UriLibraries/UriLibraryStep.expected
@@ -22,6 +22,8 @@
 | closureUri.js:18:1:18:39 | uri.set ... heme(z) | closureUri.js:18:38:18:38 | z | closureUri.js:5:11:5:20 | new Uri(x) |
 | closureUri.js:22:1:22:25 | utils.a ... uri, z) | closureUri.js:22:19:22:21 | uri | closureUri.js:22:1:22:25 | utils.a ... uri, z) |
 | closureUri.js:23:1:23:18 | utils.getPath(uri) | closureUri.js:23:15:23:17 | uri | closureUri.js:23:1:23:18 | utils.getPath(uri) |
+| closureUri.js:27:1:27:23 | stringU ... code(x) | closureUri.js:27:22:27:22 | x | closureUri.js:27:1:27:23 | stringU ... code(x) |
+| closureUri.js:28:1:28:23 | stringU ... code(x) | closureUri.js:28:22:28:22 | x | closureUri.js:28:1:28:23 | stringU ... code(x) |
 | punycode.js:3:9:3:26 | punycode.decode(x) | punycode.js:3:25:3:25 | x | punycode.js:3:9:3:26 | punycode.decode(x) |
 | punycode.js:5:5:5:22 | punycode.encode(x) | punycode.js:5:21:5:21 | x | punycode.js:5:5:5:22 | punycode.encode(x) |
 | punycode.js:7:5:7:25 | punycod ... code(x) | punycode.js:7:24:7:24 | x | punycode.js:7:5:7:25 | punycod ... code(x) |
--- a/javascript/ql/test/library-tests/frameworks/UriLibraries/closureUri.js
+++ b/javascript/ql/test/library-tests/frameworks/UriLibraries/closureUri.js
@@ -21,3 +21,8 @@ let utils = goog.require('goog.uri.utils');

 utils.appendParam(uri, z);
 utils.getPath(uri);
+
+let stringUtil = goog.require('goog.string');
+
+stringUtil.urlEncode(x);
+stringUtil.urlDecode(x);