diff --git a/change-notes/1.20/analysis-javascript.md b/change-notes/1.20/analysis-javascript.md index eb1902195d01..338e3bb9cc44 100644 --- a/change-notes/1.20/analysis-javascript.md +++ b/change-notes/1.20/analysis-javascript.md @@ -4,8 +4,11 @@ * 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/) +* The taint tracking library now recognizes flow through persistent storage, this may give more results for the security queries. + ## New queries | **Query** | **Tags** | **Purpose** | diff --git a/javascript/ql/src/javascript.qll b/javascript/ql/src/javascript.qll index 4202d86ce1ea..71a991be843a 100644 --- a/javascript/ql/src/javascript.qll +++ b/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 diff --git a/javascript/ql/src/semmle/javascript/Concepts.qll b/javascript/ql/src/semmle/javascript/Concepts.qll index dddd27caf14a..53220a149789 100644 --- a/javascript/ql/src/semmle/javascript/Concepts.qll +++ b/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(); + +} diff --git a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll index 4fff201ff8f3..a00d4f64077d 100644 --- a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll +++ b/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. */ diff --git a/javascript/ql/src/semmle/javascript/frameworks/CookieLibraries.qll b/javascript/ql/src/semmle/javascript/frameworks/CookieLibraries.qll new file mode 100644 index 000000000000..498117b86528 --- /dev/null +++ b/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) } + } +} diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/DOM.qll b/javascript/ql/src/semmle/javascript/security/dataflow/DOM.qll index 5352f6bde7dd..747100bc93b5 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/DOM.qll +++ b/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. */ diff --git a/javascript/ql/test/library-tests/frameworks/Concepts/PersistentReadAccess.expected b/javascript/ql/test/library-tests/frameworks/Concepts/PersistentReadAccess.expected new file mode 100644 index 000000000000..f3ee359f2e61 --- /dev/null +++ b/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') | diff --git a/javascript/ql/test/library-tests/frameworks/Concepts/PersistentReadAccess.ql b/javascript/ql/test/library-tests/frameworks/Concepts/PersistentReadAccess.ql new file mode 100644 index 000000000000..dcd2e65f7f9a --- /dev/null +++ b/javascript/ql/test/library-tests/frameworks/Concepts/PersistentReadAccess.ql @@ -0,0 +1,4 @@ +import javascript + +from PersistentReadAccess read +select read \ No newline at end of file diff --git a/javascript/ql/test/library-tests/frameworks/Concepts/PersistentReadAccess_getAWrite.expected b/javascript/ql/test/library-tests/frameworks/Concepts/PersistentReadAccess_getAWrite.expected new file mode 100644 index 000000000000..0f2f5f12c226 --- /dev/null +++ b/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) | diff --git a/javascript/ql/test/library-tests/frameworks/Concepts/PersistentReadAccess_getAWrite.ql b/javascript/ql/test/library-tests/frameworks/Concepts/PersistentReadAccess_getAWrite.ql new file mode 100644 index 000000000000..c318b13c1abe --- /dev/null +++ b/javascript/ql/test/library-tests/frameworks/Concepts/PersistentReadAccess_getAWrite.ql @@ -0,0 +1,4 @@ +import javascript + +from PersistentReadAccess read +select read, read.getAWrite() \ No newline at end of file diff --git a/javascript/ql/test/library-tests/frameworks/Concepts/PersistentWriteAccess.expected b/javascript/ql/test/library-tests/frameworks/Concepts/PersistentWriteAccess.expected new file mode 100644 index 000000000000..da1a085664c6 --- /dev/null +++ b/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 | diff --git a/javascript/ql/test/library-tests/frameworks/Concepts/PersistentWriteAccess.ql b/javascript/ql/test/library-tests/frameworks/Concepts/PersistentWriteAccess.ql new file mode 100644 index 000000000000..e67f2a0b0e88 --- /dev/null +++ b/javascript/ql/test/library-tests/frameworks/Concepts/PersistentWriteAccess.ql @@ -0,0 +1,4 @@ +import javascript + +from PersistentWriteAccess write +select write, write.getValue() \ No newline at end of file diff --git a/javascript/ql/test/library-tests/frameworks/Concepts/persistence.js b/javascript/ql/test/library-tests/frameworks/Concepts/persistence.js new file mode 100644 index 000000000000..49436007e24a --- /dev/null +++ b/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'); +}); diff --git a/javascript/ql/test/library-tests/frameworks/CookieLibraries/CookieReadAccess.expected b/javascript/ql/test/library-tests/frameworks/CookieLibraries/CookieReadAccess.expected new file mode 100644 index 000000000000..31d1adc07663 --- /dev/null +++ b/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'] | diff --git a/javascript/ql/test/library-tests/frameworks/CookieLibraries/CookieReadAccess.ql b/javascript/ql/test/library-tests/frameworks/CookieLibraries/CookieReadAccess.ql new file mode 100644 index 000000000000..dcd2e65f7f9a --- /dev/null +++ b/javascript/ql/test/library-tests/frameworks/CookieLibraries/CookieReadAccess.ql @@ -0,0 +1,4 @@ +import javascript + +from PersistentReadAccess read +select read \ No newline at end of file diff --git a/javascript/ql/test/library-tests/frameworks/CookieLibraries/CookieReadAccess_getAWrite.expected b/javascript/ql/test/library-tests/frameworks/CookieLibraries/CookieReadAccess_getAWrite.expected new file mode 100644 index 000000000000..75cd09241d29 --- /dev/null +++ b/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') | diff --git a/javascript/ql/test/library-tests/frameworks/CookieLibraries/CookieReadAccess_getAWrite.ql b/javascript/ql/test/library-tests/frameworks/CookieLibraries/CookieReadAccess_getAWrite.ql new file mode 100644 index 000000000000..c318b13c1abe --- /dev/null +++ b/javascript/ql/test/library-tests/frameworks/CookieLibraries/CookieReadAccess_getAWrite.ql @@ -0,0 +1,4 @@ +import javascript + +from PersistentReadAccess read +select read, read.getAWrite() \ No newline at end of file diff --git a/javascript/ql/test/library-tests/frameworks/CookieLibraries/CookieWriteAccess.expected b/javascript/ql/test/library-tests/frameworks/CookieLibraries/CookieWriteAccess.expected new file mode 100644 index 000000000000..0536d0df02d9 --- /dev/null +++ b/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' | diff --git a/javascript/ql/test/library-tests/frameworks/CookieLibraries/CookieWriteAccess.ql b/javascript/ql/test/library-tests/frameworks/CookieLibraries/CookieWriteAccess.ql new file mode 100644 index 000000000000..e67f2a0b0e88 --- /dev/null +++ b/javascript/ql/test/library-tests/frameworks/CookieLibraries/CookieWriteAccess.ql @@ -0,0 +1,4 @@ +import javascript + +from PersistentWriteAccess write +select write, write.getValue() \ No newline at end of file diff --git a/javascript/ql/test/library-tests/frameworks/CookieLibraries/tst.js b/javascript/ql/test/library-tests/frameworks/CookieLibraries/tst.js new file mode 100644 index 000000000000..86c096e3f631 --- /dev/null +++ b/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']; +}); diff --git a/javascript/ql/test/query-tests/Security/CWE-079/StoredXss.expected b/javascript/ql/test/query-tests/Security/CWE-079/StoredXss.expected index 4b941d5e730b..aa803d24106e 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/StoredXss.expected +++ b/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 | @@ -262,6 +268,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() | diff --git a/javascript/ql/test/query-tests/Security/CWE-079/Xss.expected b/javascript/ql/test/query-tests/Security/CWE-079/Xss.expected index c4810e82c135..7c97e38b5b6a 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/Xss.expected +++ b/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 | @@ -177,6 +183,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() | @@ -296,6 +306,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 | diff --git a/javascript/ql/test/query-tests/Security/CWE-079/stored-xss.js b/javascript/ql/test/query-tests/Security/CWE-079/stored-xss.js new file mode 100644 index 000000000000..4a9cc51bce7b --- /dev/null +++ b/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 +});