From c6cfca313155c2f066586043d61399c0230a11b8 Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Mon, 6 Aug 2018 15:09:22 +0200 Subject: [PATCH 1/4] JS: add "verify" as an `Authorization` call word --- .../ql/src/semmle/javascript/security/SensitiveActions.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/semmle/javascript/security/SensitiveActions.qll b/javascript/ql/src/semmle/javascript/security/SensitiveActions.qll index 248f56fc2a4..68f5dd49556 100644 --- a/javascript/ql/src/semmle/javascript/security/SensitiveActions.qll +++ b/javascript/ql/src/semmle/javascript/security/SensitiveActions.qll @@ -143,7 +143,7 @@ class AuthorizationCall extends SensitiveAction, DataFlow::CallNode { exists(string s | s = astNode.getCalleeName() | // name contains `login` or `auth`, but not as part of `loginfo` or `unauth`; // also exclude `author` - s.regexpMatch("(?i).*(login(?!fo)|(? Date: Mon, 6 Aug 2018 15:09:48 +0200 Subject: [PATCH 2/4] JS: support "express-rate-limit" non-constructor calls --- .../semmle/javascript/security/dataflow/MissingRateLimiting.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/MissingRateLimiting.qll b/javascript/ql/src/semmle/javascript/security/dataflow/MissingRateLimiting.qll index a9c6d86c8b1..aa4c561047d 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/MissingRateLimiting.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/MissingRateLimiting.qll @@ -131,7 +131,7 @@ abstract class RateLimiter extends Express::RouteHandlerExpr { */ class ExpressRateLimit extends RateLimiter { ExpressRateLimit() { - DataFlow::moduleImport("express-rate-limit").getAnInstantiation().flowsToExpr(this) + DataFlow::moduleImport("express-rate-limit").getAnInvocation().flowsToExpr(this) } } From b6951d82497c4b15ac67816e3303be5f5f73efa3 Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Mon, 6 Aug 2018 15:10:35 +0200 Subject: [PATCH 3/4] JS: add tests for improved js/missing-rate-limiting --- .../query-tests/Security/CWE-770/MissingRateLimiting.expected | 1 + javascript/ql/test/query-tests/Security/CWE-770/tst.js | 3 +++ 2 files changed, 4 insertions(+) diff --git a/javascript/ql/test/query-tests/Security/CWE-770/MissingRateLimiting.expected b/javascript/ql/test/query-tests/Security/CWE-770/MissingRateLimiting.expected index a2f43fe540b..560be8bbb86 100644 --- a/javascript/ql/test/query-tests/Security/CWE-770/MissingRateLimiting.expected +++ b/javascript/ql/test/query-tests/Security/CWE-770/MissingRateLimiting.expected @@ -6,3 +6,4 @@ | tst.js:36:20:36:36 | expensiveHandler2 | This route handler performs $@, but is not rate-limited. | tst.js:15:40:15:73 | fs.writ ... quest") | a file system access | | tst.js:37:20:37:36 | expensiveHandler3 | This route handler performs $@, but is not rate-limited. | tst.js:16:40:16:70 | child_p ... /true") | a system command | | tst.js:38:20:38:36 | expensiveHandler4 | This route handler performs $@, but is not rate-limited. | tst.js:17:40:17:83 | connect ... ution') | a database access | +| tst.js:64:25:64:63 | functio ... req); } | This route handler performs $@, but is not rate-limited. | tst.js:64:46:64:60 | verifyUser(req) | authorization | diff --git a/javascript/ql/test/query-tests/Security/CWE-770/tst.js b/javascript/ql/test/query-tests/Security/CWE-770/tst.js index bb06fe9a87d..f38a9e83aa2 100644 --- a/javascript/ql/test/query-tests/Security/CWE-770/tst.js +++ b/javascript/ql/test/query-tests/Security/CWE-770/tst.js @@ -60,3 +60,6 @@ app2.get('/:path', bruteforce.prevent, expensiveHandler1); // OK var app3 = express(); var limiter = require('express-limiter')(app3); app3.get('/:path', expensiveHandler1); // OK + +express().get('/:path', function(req, res) { verifyUser(req); }); // NOT OK +express().get('/:path', RateLimit(), function(req, res) { verifyUser(req); }); // OK From fa90c53b4325b81f5ba80b712562cc7950240062 Mon Sep 17 00:00:00 2001 From: Esben Sparre Andreasen Date: Mon, 6 Aug 2018 15:14:15 +0200 Subject: [PATCH 4/4] JS: update change notes for improved js/missing-rate-limiting --- change-notes/1.18/analysis-javascript.md | 1 + 1 file changed, 1 insertion(+) diff --git a/change-notes/1.18/analysis-javascript.md b/change-notes/1.18/analysis-javascript.md index d507351eaa0..52162df67cb 100644 --- a/change-notes/1.18/analysis-javascript.md +++ b/change-notes/1.18/analysis-javascript.md @@ -40,6 +40,7 @@ | CORS misconfiguration for credentials transfer | More true-positive results | This rule now treats header names case-insensitively. | | Hard-coded credentials | More true-positive results | This rule now recognizes secret cryptographic keys. | | Insecure randomness | More true-positive results | This rule now recognizes secret cryptographic keys. | +| Missing rate limiting | More true-positive results, fewer false-positive results | This rule now recognizes additional rate limiters and expensive route handlers. | | Missing X-Frame-Options HTTP header | Fewer false-positive results | This rule now treats header names case-insensitively. | | Reflected cross-site scripting | Fewer false-positive results | This rule now treats header names case-insensitively. | | Server-side URL redirect | More true-positive results | This rule now treats header names case-insensitively. |