Merge pull request #629 from esben-semmle/js/persistent-read-taint

JS: add persistent storage taint steps

change-notes/1.20/analysis-javascript.md

@@ -4,9 +4,12 @@
 
 * Support for popular libraries has been improved. Consequently, queries may produce more results on code bases that use the following features:
   - client-side code, for example [React](https://reactjs.org/)
+  - cookies and webstorage, for example [js-cookie](https://github.com/js-cookie/js-cookie)
   - server-side code, for example [hapi](https://hapijs.com/)
 * File classification has been improved to recognize additional generated files, for example files from [HTML Tidy](html-tidy.org).
 
+* The taint tracking library now recognizes flow through persistent storage, this may give more results for the security queries. 
+
 ## New queries
 
 | **Query**                                     | **Tags**                                             | **Purpose**                                                                                                                                                                 |

javascript/ql/src/javascript.qll

@@ -60,6 +60,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.CookieLibraries
 import semmle.javascript.frameworks.Credentials
 import semmle.javascript.frameworks.CryptoLibraries
 import semmle.javascript.frameworks.DigitalOcean

javascript/ql/src/semmle/javascript/Concepts.qll

@@ -65,3 +65,27 @@ abstract class DatabaseAccess extends DataFlow::Node {
   /** Gets an argument to this database access that is interpreted as a query. */
   abstract DataFlow::Node getAQueryArgument();
 }
+
+/**
+ * A data flow node that reads persistent data.
+ */
+abstract class PersistentReadAccess extends DataFlow::Node {
+
+  /**
+   * Gets a corresponding persistent write, if any.
+   */
+  abstract PersistentWriteAccess getAWrite();
+
+}
+
+/**
+ * A data flow node that writes persistent data.
+ */
+abstract class PersistentWriteAccess extends DataFlow::Node {
+
+  /**
+   * Gets the data flow node corresponding to the written value.
+   */
+  abstract DataFlow::Node getValue();
+
+}

javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll

@@ -232,6 +232,24 @@ module TaintTracking {
     }
   }
 
+  /**
+   * A taint propagating data flow edge through persistent storage.
+   */
+  private class StorageTaintStep extends AdditionalTaintStep {
+
+    PersistentReadAccess read;
+
+    StorageTaintStep() {
+      this = read
+    }
+
+    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+        pred = read.getAWrite().getValue() and
+        succ = read
+    }
+
+  }
+
   /**
    * A taint propagating data flow edge caused by the builtin array functions.
    */

javascript/ql/src/semmle/javascript/frameworks/CookieLibraries.qll

@@ -0,0 +1,93 @@
+
+/**
+ * Provides classes for reasoning about cookies.
+ */
+
+import javascript
+
+/**
+ * A model of the `js-cookie` library (https://github.com/js-cookie/js-cookie).
+ */
+private module JsCookie {
+  /**
+   * Gets a function call that invokes method `name` of the `js-cookie` library.
+   */
+  DataFlow::CallNode libMemberCall(string name) {
+    result = DataFlow::globalVarRef("Cookie").getAMemberCall(name) or
+    result = DataFlow::globalVarRef("Cookie").getAMemberCall("noConflict").getAMemberCall(name) or
+    result = DataFlow::moduleMember("js-cookie", name).getACall()
+  }
+
+  class ReadAccess extends PersistentReadAccess, DataFlow::CallNode {
+    ReadAccess() { this = libMemberCall("get") }
+
+    override PersistentWriteAccess getAWrite() {
+      getArgument(0).mayHaveStringValue(result.(WriteAccess).getKey())
+    }
+  }
+
+  class WriteAccess extends PersistentWriteAccess, DataFlow::CallNode {
+    WriteAccess() { this = libMemberCall("set") }
+
+    string getKey() { getArgument(0).mayHaveStringValue(result) }
+
+    override DataFlow::Node getValue() { result = getArgument(1) }
+  }
+}
+
+/**
+ * A model of the `browser-cookies` library (https://github.com/voltace/browser-cookies).
+ */
+private module BrowserCookies {
+  /**
+   * Gets a function call that invokes method `name` of the `browser-cookies` library.
+   */
+  DataFlow::CallNode libMemberCall(string name) {
+    result = DataFlow::moduleMember("browser-cookies", name).getACall()
+  }
+
+  class ReadAccess extends PersistentReadAccess, DataFlow::CallNode {
+    ReadAccess() { this = libMemberCall("get") }
+
+    override PersistentWriteAccess getAWrite() {
+      getArgument(0).mayHaveStringValue(result.(WriteAccess).getKey())
+    }
+  }
+
+  class WriteAccess extends PersistentWriteAccess, DataFlow::CallNode {
+    WriteAccess() { this = libMemberCall("set") }
+
+    string getKey() { getArgument(0).mayHaveStringValue(result) }
+
+    override DataFlow::Node getValue() { result = getArgument(1) }
+  }
+}
+
+/**
+ * A model of the `cookie` library (https://github.com/jshttp/cookie).
+ */
+private module LibCookie {
+  /**
+   * Gets a function call that invokes method `name` of the `cookie` library.
+   */
+  DataFlow::CallNode libMemberCall(string name) {
+    result = DataFlow::moduleMember("cookie", name).getACall()
+  }
+
+  class ReadAccess extends PersistentReadAccess {
+    string key;
+    ReadAccess() { this = libMemberCall("parse").getAPropertyRead(key) }
+
+    override PersistentWriteAccess getAWrite() {
+      key = result.(WriteAccess).getKey()
+    }
+  }
+
+  class WriteAccess extends PersistentWriteAccess, DataFlow::CallNode {
+    WriteAccess() { this = libMemberCall("serialize") }
+
+    string getKey() { getArgument(0).mayHaveStringValue(result) }
+
+    override DataFlow::Node getValue() { result = getArgument(1) }
+  }
+}

javascript/ql/src/semmle/javascript/security/dataflow/DOM.qll

@@ -167,6 +167,44 @@ class WebStorageWrite extends Expr {
   }
 }
 
+/**
+ * Persistent storage through web storage such as `localStorage` or `sessionStorage`.
+ */
+private module PersistentWebStorage {
+  private DataFlow::SourceNode webStorage(string kind) {
+    (kind = "localStorage" or kind = "sessionStorage") and
+    result = DataFlow::globalVarRef(kind)
+  }
+
+  /**
+   * A read access.
+   */
+  class ReadAccess extends PersistentReadAccess, DataFlow::CallNode {
+    string kind;
+
+    ReadAccess() { this = webStorage(kind).getAMethodCall("getItem") }
+
+    override PersistentWriteAccess getAWrite() {
+      getArgument(0).mayHaveStringValue(result.(WriteAccess).getKey()) and
+      result.(WriteAccess).getKind() = kind
+    }
+  }
+
+  /**
+   * A write access.
+   */
+  class WriteAccess extends PersistentWriteAccess, DataFlow::CallNode {
+    string kind;
+
+    WriteAccess() { this = webStorage(kind).getAMethodCall("setItem") }
+
+    string getKey() { getArgument(0).mayHaveStringValue(result) }
+
+    string getKind() { result = kind }
+
+    override DataFlow::Node getValue() { result = getArgument(1) }
+  }
+}
 /**
  * An event handler that handles `postMessage` events.
  */

javascript/ql/test/library-tests/frameworks/Concepts/PersistentReadAccess.expected

@@ -0,0 +1,4 @@
+| persistence.js:3:5:3:33 | localSt ... prop1') |
+| persistence.js:6:5:6:35 | session ... prop2') |
+| persistence.js:10:5:10:33 | localSt ... prop4') |
+| persistence.js:13:5:13:35 | session ... prop5') |

javascript/ql/test/library-tests/frameworks/Concepts/PersistentReadAccess.ql

@@ -0,0 +1,4 @@
+import javascript
+
+from PersistentReadAccess read
+select read

javascript/ql/test/library-tests/frameworks/Concepts/PersistentReadAccess_getAWrite.expected

@@ -0,0 +1,2 @@
+| persistence.js:3:5:3:33 | localSt ... prop1') | persistence.js:2:5:2:37 | localSt ... 1', v1) |
+| persistence.js:6:5:6:35 | session ... prop2') | persistence.js:5:5:5:39 | session ... 2', v2) |

javascript/ql/test/library-tests/frameworks/Concepts/PersistentReadAccess_getAWrite.ql

@@ -0,0 +1,4 @@
+import javascript
+
+from PersistentReadAccess read
+select read, read.getAWrite()

javascript/ql/test/library-tests/frameworks/Concepts/PersistentWriteAccess.expected

@@ -0,0 +1,4 @@
+| persistence.js:2:5:2:37 | localSt ... 1', v1) | persistence.js:2:35:2:36 | v1 |
+| persistence.js:5:5:5:39 | session ... 2', v2) | persistence.js:5:37:5:38 | v2 |
+| persistence.js:8:5:8:37 | localSt ... 3', v3) | persistence.js:8:35:8:36 | v3 |
+| persistence.js:12:5:12:37 | localSt ... 5', v5) | persistence.js:12:35:12:36 | v5 |

javascript/ql/test/library-tests/frameworks/Concepts/PersistentWriteAccess.ql

@@ -0,0 +1,4 @@
+import javascript
+
+from PersistentWriteAccess write
+select write, write.getValue()

javascript/ql/test/library-tests/frameworks/Concepts/persistence.js

@@ -0,0 +1,14 @@
+(function(){
+    localStorage.setItem('prop1', v1);
+    localStorage.getItem('prop1');
+
+    sessionStorage.setItem('prop2', v2);
+    sessionStorage.getItem('prop2');
+
+    localStorage.setItem('prop3', v3);
+
+    localStorage.getItem('prop4');
+
+    localStorage.setItem('prop5', v5);
+    sessionStorage.getItem('prop5');
+});

javascript/ql/test/library-tests/frameworks/CookieLibraries/CookieReadAccess.expected

@@ -0,0 +1,3 @@
+| tst.js:7:2:7:21 | js_cookie.get('key') |
+| tst.js:12:2:12:27 | browser ... ('key') |
+| tst.js:18:2:18:22 | cookie. ... ['key'] |

javascript/ql/test/library-tests/frameworks/CookieLibraries/CookieReadAccess.ql

@@ -0,0 +1,4 @@
+import javascript
+
+from PersistentReadAccess read
+select read

javascript/ql/test/library-tests/frameworks/CookieLibraries/CookieReadAccess_getAWrite.expected

@@ -0,0 +1,3 @@
+| tst.js:7:2:7:21 | js_cookie.get('key') | tst.js:6:2:6:30 | js_cook ... value') |
+| tst.js:12:2:12:27 | browser ... ('key') | tst.js:11:2:11:36 | browser ... value') |
+| tst.js:18:2:18:22 | cookie. ... ['key'] | tst.js:17:2:17:33 | cookie. ... value') |

javascript/ql/test/library-tests/frameworks/CookieLibraries/CookieReadAccess_getAWrite.ql

@@ -0,0 +1,4 @@
+import javascript
+
+from PersistentReadAccess read
+select read, read.getAWrite()

javascript/ql/test/library-tests/frameworks/CookieLibraries/CookieWriteAccess.expected

@@ -0,0 +1,3 @@
+| tst.js:6:2:6:30 | js_cook ... value') | tst.js:6:23:6:29 | 'value' |
+| tst.js:11:2:11:36 | browser ... value') | tst.js:11:29:11:35 | 'value' |
+| tst.js:17:2:17:33 | cookie. ... value') | tst.js:17:26:17:32 | 'value' |

javascript/ql/test/library-tests/frameworks/CookieLibraries/CookieWriteAccess.ql

@@ -0,0 +1,4 @@
+import javascript
+
+from PersistentWriteAccess write
+select write, write.getValue()

javascript/ql/test/library-tests/frameworks/CookieLibraries/tst.js

@@ -0,0 +1,19 @@
+const js_cookie = require('js-cookie'),
+      browser_cookies = require('browser-cookies'),
+      cookie = require('cookie');
+
+(function() {
+	js_cookie.set('key', 'value');
+	js_cookie.get('key');
+});
+
+(function() {
+	browser_cookies.set('key', 'value');
+	browser_cookies.get('key');
+});
+
+
+(function() {
+	cookie.serialize('key', 'value');
+	cookie.parse()['key'];
+});

javascript/ql/test/query-tests/Security/CWE-079/StoredXss.expected

@@ -54,6 +54,12 @@ nodes
 | react-native.js:7:17:7:33 | req.param("code") |
 | react-native.js:8:18:8:24 | tainted |
 | react-native.js:9:27:9:33 | tainted |
+| stored-xss.js:2:39:2:55 | document.location |
+| stored-xss.js:2:39:2:62 | documen ... .search |
+| stored-xss.js:3:35:3:51 | document.location |
+| stored-xss.js:3:35:3:58 | documen ... .search |
+| stored-xss.js:5:20:5:52 | session ... ssion') |
+| stored-xss.js:8:20:8:48 | localSt ... local') |
 | string-manipulations.js:3:16:3:32 | document.location |
 | string-manipulations.js:4:16:4:32 | document.location |
 | string-manipulations.js:4:16:4:37 | documen ... on.href |
@@ -271,6 +277,10 @@ edges
 | react-native.js:7:7:7:33 | tainted | react-native.js:8:18:8:24 | tainted |
 | react-native.js:7:7:7:33 | tainted | react-native.js:9:27:9:33 | tainted |
 | react-native.js:7:17:7:33 | req.param("code") | react-native.js:7:7:7:33 | tainted |
+| stored-xss.js:2:39:2:55 | document.location | stored-xss.js:2:39:2:62 | documen ... .search |
+| stored-xss.js:2:39:2:62 | documen ... .search | stored-xss.js:5:20:5:52 | session ... ssion') |
+| stored-xss.js:3:35:3:51 | document.location | stored-xss.js:3:35:3:58 | documen ... .search |
+| stored-xss.js:3:35:3:58 | documen ... .search | stored-xss.js:8:20:8:48 | localSt ... local') |
 | string-manipulations.js:4:16:4:32 | document.location | string-manipulations.js:4:16:4:37 | documen ... on.href |
 | string-manipulations.js:5:16:5:32 | document.location | string-manipulations.js:5:16:5:37 | documen ... on.href |
 | string-manipulations.js:5:16:5:37 | documen ... on.href | string-manipulations.js:5:16:5:47 | documen ... lueOf() |

javascript/ql/test/query-tests/Security/CWE-079/Xss.expected

@@ -17,6 +17,12 @@ nodes
 | react-native.js:7:17:7:33 | req.param("code") |
 | react-native.js:8:18:8:24 | tainted |
 | react-native.js:9:27:9:33 | tainted |
+| stored-xss.js:2:39:2:55 | document.location |
+| stored-xss.js:2:39:2:62 | documen ... .search |
+| stored-xss.js:3:35:3:51 | document.location |
+| stored-xss.js:3:35:3:58 | documen ... .search |
+| stored-xss.js:5:20:5:52 | session ... ssion') |
+| stored-xss.js:8:20:8:48 | localSt ... local') |
 | string-manipulations.js:3:16:3:32 | document.location |
 | string-manipulations.js:4:16:4:32 | document.location |
 | string-manipulations.js:4:16:4:37 | documen ... on.href |
@@ -186,6 +192,10 @@ edges
 | react-native.js:7:7:7:33 | tainted | react-native.js:8:18:8:24 | tainted |
 | react-native.js:7:7:7:33 | tainted | react-native.js:9:27:9:33 | tainted |
 | react-native.js:7:17:7:33 | req.param("code") | react-native.js:7:7:7:33 | tainted |
+| stored-xss.js:2:39:2:55 | document.location | stored-xss.js:2:39:2:62 | documen ... .search |
+| stored-xss.js:2:39:2:62 | documen ... .search | stored-xss.js:5:20:5:52 | session ... ssion') |
+| stored-xss.js:3:35:3:51 | document.location | stored-xss.js:3:35:3:58 | documen ... .search |
+| stored-xss.js:3:35:3:58 | documen ... .search | stored-xss.js:8:20:8:48 | localSt ... local') |
 | string-manipulations.js:4:16:4:32 | document.location | string-manipulations.js:4:16:4:37 | documen ... on.href |
 | string-manipulations.js:5:16:5:32 | document.location | string-manipulations.js:5:16:5:37 | documen ... on.href |
 | string-manipulations.js:5:16:5:37 | documen ... on.href | string-manipulations.js:5:16:5:47 | documen ... lueOf() |
@@ -310,6 +320,8 @@ edges
 | nodemailer.js:13:11:13:69 | `Hi, yo ... sage}.` | nodemailer.js:13:50:13:66 | req.query.message | nodemailer.js:13:11:13:69 | `Hi, yo ... sage}.` | HTML injection vulnerability due to $@. | nodemailer.js:13:50:13:66 | req.query.message | user-provided value |
 | react-native.js:8:18:8:24 | tainted | react-native.js:7:17:7:33 | req.param("code") | react-native.js:8:18:8:24 | tainted | Cross-site scripting vulnerability due to $@. | react-native.js:7:17:7:33 | req.param("code") | user-provided value |
 | react-native.js:9:27:9:33 | tainted | react-native.js:7:17:7:33 | req.param("code") | react-native.js:9:27:9:33 | tainted | Cross-site scripting vulnerability due to $@. | react-native.js:7:17:7:33 | req.param("code") | user-provided value |
+| stored-xss.js:5:20:5:52 | session ... ssion') | stored-xss.js:2:39:2:55 | document.location | stored-xss.js:5:20:5:52 | session ... ssion') | Cross-site scripting vulnerability due to $@. | stored-xss.js:2:39:2:55 | document.location | user-provided value |
+| stored-xss.js:8:20:8:48 | localSt ... local') | stored-xss.js:3:35:3:51 | document.location | stored-xss.js:8:20:8:48 | localSt ... local') | Cross-site scripting vulnerability due to $@. | stored-xss.js:3:35:3:51 | document.location | user-provided value |
 | string-manipulations.js:3:16:3:32 | document.location | string-manipulations.js:3:16:3:32 | document.location | string-manipulations.js:3:16:3:32 | document.location | Cross-site scripting vulnerability due to $@. | string-manipulations.js:3:16:3:32 | document.location | user-provided value |
 | string-manipulations.js:4:16:4:37 | documen ... on.href | string-manipulations.js:4:16:4:32 | document.location | string-manipulations.js:4:16:4:37 | documen ... on.href | Cross-site scripting vulnerability due to $@. | string-manipulations.js:4:16:4:32 | document.location | user-provided value |
 | string-manipulations.js:5:16:5:47 | documen ... lueOf() | string-manipulations.js:5:16:5:32 | document.location | string-manipulations.js:5:16:5:47 | documen ... lueOf() | Cross-site scripting vulnerability due to $@. | string-manipulations.js:5:16:5:32 | document.location | user-provided value |

javascript/ql/test/query-tests/Security/CWE-079/stored-xss.js

@@ -0,0 +1,9 @@
+(function() {
+    sessionStorage.setItem('session', document.location.search);
+    localStorage.setItem('local', document.location.search);
+
+    $('myId').html(sessionStorage.getItem('session')); // NOT OK
+    $('myId').html(localStorage.getItem('session')); // OK
+    $('myId').html(sessionStorage.getItem('local')); // OK
+    $('myId').html(localStorage.getItem('local')); // NOT OK
+});