From b2faac30c9ba56e6216a84f4bd98362714ce7556 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Thu, 28 Mar 2019 10:12:08 +0000 Subject: [PATCH 1/3] JavaScript: Add a few missing doc comments. --- .../javascript/dataflow/TypeTracking.qll | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll b/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll index 2164426f6af2..943d7402b373 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll @@ -126,6 +126,7 @@ class TypeTracker extends TTypeTracker { TypeTracker() { this = MkTypeTracker(hasCall, prop) } + /** Gets the summary resulting from appending `step` to this type-tracking summary. */ TypeTracker append(StepSummary step) { step = LevelStep() and result = this or @@ -140,6 +141,7 @@ class TypeTracker extends TTypeTracker { ) } + /** Gets a textual representation of this summary. */ string toString() { exists(string withCall, string withProp | (if hasCall = true then withCall = "with" else withCall = "without") and @@ -153,6 +155,9 @@ class TypeTracker extends TTypeTracker { */ predicate start() { hasCall = false and prop = "" } + /** + * Holds if this is the end point of type tracking. + */ predicate end() { prop = "" } /** @@ -162,6 +167,10 @@ class TypeTracker extends TTypeTracker { */ boolean hasCall() { result = hasCall } + /** + * Gets the property this type has been tracked into, or the empty string if + * it has not been tracked into a property. + */ string getProp() { result = prop } } @@ -206,6 +215,7 @@ class TypeBackTracker extends TTypeBackTracker { TypeBackTracker() { this = MkTypeBackTracker(hasReturn, prop) } + /** Gets the summary resulting from prepending `step` to this type-tracking summary. */ TypeBackTracker prepend(StepSummary step) { step = LevelStep() and result = this or @@ -220,6 +230,7 @@ class TypeBackTracker extends TTypeBackTracker { step = StoreStep(prop) and result = MkTypeBackTracker(hasReturn, "") } + /** Gets a textual representation of this summary. */ string toString() { exists(string withReturn, string withProp | (if hasReturn = true then withReturn = "with" else withReturn = "without") and @@ -233,6 +244,9 @@ class TypeBackTracker extends TTypeBackTracker { */ predicate start() { hasReturn = false and prop = "" } + /** + * Holds if this is the end point of type tracking. + */ predicate end() { prop = "" } /** @@ -242,6 +256,10 @@ class TypeBackTracker extends TTypeBackTracker { */ boolean hasReturn() { result = hasReturn } + /** + * Gets the property this type has been tracked into, or the empty string if + * it has not been tracked into a property. + */ string getProp() { result = prop } } From c097031c7e8b3ca32e8b855a8db18761d8bce784 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Thu, 28 Mar 2019 10:33:04 +0000 Subject: [PATCH 2/3] JavaScript: Fix uses of `TypeTracker` with custom flow steps. These steps need to check that the type hasn't been tracked into a property. --- .../ql/src/semmle/javascript/frameworks/SocketIO.qll | 5 +++++ .../test/library-tests/frameworks/SocketIO/tests.expected | 1 + .../ql/test/library-tests/frameworks/SocketIO/tst.js | 8 ++++++++ 3 files changed, 14 insertions(+) diff --git a/javascript/ql/src/semmle/javascript/frameworks/SocketIO.qll b/javascript/ql/src/semmle/javascript/frameworks/SocketIO.qll index c7264ab0b1dc..c1eb5aa754d0 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/SocketIO.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/SocketIO.qll @@ -51,6 +51,7 @@ module SocketIO { // exclude getter versions exists(mcn.getAnArgument()) and result = mcn and + t2.getProp() = "" and t = t2 ) ) @@ -110,6 +111,7 @@ module SocketIO { or // invocation of a chainable method result = pred.getAMethodCall(namespaceChainableMethod()) and + t2.getProp() = "" and t = t2 or // invocation of chainable getter method @@ -119,6 +121,7 @@ module SocketIO { m = "volatile" | result = pred.getAPropertyRead(m) and + t2.getProp() = "" and t = t2 ) ) @@ -171,6 +174,7 @@ module SocketIO { m = EventEmitter::chainableMethod() | result = pred.getAMethodCall(m) and + t2.getProp() = "" and t = t2 ) or @@ -182,6 +186,7 @@ module SocketIO { m = "volatile" | result = pred.getAPropertyRead(m) and + t2.getProp() = "" and t = t2 ) ) diff --git a/javascript/ql/test/library-tests/frameworks/SocketIO/tests.expected b/javascript/ql/test/library-tests/frameworks/SocketIO/tests.expected index 2bf7159de757..e174abd56b56 100644 --- a/javascript/ql/test/library-tests/frameworks/SocketIO/tests.expected +++ b/javascript/ql/test/library-tests/frameworks/SocketIO/tests.expected @@ -149,6 +149,7 @@ test_ServerNode | tst.js:15:1:15:15 | io.attach(http) | tst.js:1:12:1:33 | socket.io server | | tst.js:16:1:16:15 | io.bind(engine) | tst.js:1:12:1:33 | socket.io server | | tst.js:17:1:17:23 | io.onco ... socket) | tst.js:1:12:1:33 | socket.io server | +| tst.js:79:1:79:10 | obj.server | tst.js:1:12:1:33 | socket.io server | test_ClientSendNode_getAReceiver | client2.js:14:1:14:32 | sock.em ... there") | tst.js:72:3:72:43 | socket. ... => {}) | | client2.js:16:1:16:36 | sock.wr ... => {}) | tst.js:70:3:70:35 | socket. ... => {}) | diff --git a/javascript/ql/test/library-tests/frameworks/SocketIO/tst.js b/javascript/ql/test/library-tests/frameworks/SocketIO/tst.js index aebe2f413684..95ceac9d478a 100644 --- a/javascript/ql/test/library-tests/frameworks/SocketIO/tst.js +++ b/javascript/ql/test/library-tests/frameworks/SocketIO/tst.js @@ -71,3 +71,11 @@ ns.on('connection', (socket) => { socket.once('message', (data1, data2) => {}); socket.addListener(eventName(), () => {}); }); + +var obj = { + server: io, + serveClient: function() { return null; } +}; +obj.server; // SocketIO::ServerNode +obj.serveClient(false); // not a SocketIO::ServerNode +obj.serveClient(false).server; // not a SocketIO::ServerNode From 62c895de3eb57d7ee22e7a87866bcb0e30e026cb Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Fri, 29 Mar 2019 11:45:18 +0000 Subject: [PATCH 3/3] JavaScript: Introduce `Type(Back)Tracker::continue` predicate. --- .../semmle/javascript/dataflow/TypeTracking.qll | 16 ++++++++++------ .../semmle/javascript/frameworks/SocketIO.qll | 15 +++++---------- 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll b/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll index 943d7402b373..b7bd316d4a42 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll +++ b/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll @@ -168,10 +168,12 @@ class TypeTracker extends TTypeTracker { boolean hasCall() { result = hasCall } /** - * Gets the property this type has been tracked into, or the empty string if - * it has not been tracked into a property. + * Gets a type tracker that starts where this one has left off to allow continued + * tracking. + * + * This predicate is only defined if the type has not been tracked into a property. */ - string getProp() { result = prop } + TypeTracker continue() { prop = "" and result = this } } module TypeTracker { @@ -257,10 +259,12 @@ class TypeBackTracker extends TTypeBackTracker { boolean hasReturn() { result = hasReturn } /** - * Gets the property this type has been tracked into, or the empty string if - * it has not been tracked into a property. + * Gets a type tracker that starts where this one has left off to allow continued + * tracking. + * + * This predicate is only defined if the type has not been tracked into a property. */ - string getProp() { result = prop } + TypeBackTracker continue() { prop = "" and result = this } } module TypeBackTracker { diff --git a/javascript/ql/src/semmle/javascript/frameworks/SocketIO.qll b/javascript/ql/src/semmle/javascript/frameworks/SocketIO.qll index c1eb5aa754d0..69add8bef3e9 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/SocketIO.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/SocketIO.qll @@ -51,8 +51,7 @@ module SocketIO { // exclude getter versions exists(mcn.getAnArgument()) and result = mcn and - t2.getProp() = "" and - t = t2 + t = t2.continue() ) ) } @@ -111,8 +110,7 @@ module SocketIO { or // invocation of a chainable method result = pred.getAMethodCall(namespaceChainableMethod()) and - t2.getProp() = "" and - t = t2 + t = t2.continue() or // invocation of chainable getter method exists(string m | @@ -121,8 +119,7 @@ module SocketIO { m = "volatile" | result = pred.getAPropertyRead(m) and - t2.getProp() = "" and - t = t2 + t = t2.continue() ) ) } @@ -174,8 +171,7 @@ module SocketIO { m = EventEmitter::chainableMethod() | result = pred.getAMethodCall(m) and - t2.getProp() = "" and - t = t2 + t = t2.continue() ) or // invocation of a chainable getter method @@ -186,8 +182,7 @@ module SocketIO { m = "volatile" | result = pred.getAPropertyRead(m) and - t2.getProp() = "" and - t = t2 + t = t2.continue() ) ) }