Merge pull request #20 from esben-semmle/js/more-auth-calls-and-rate-limiters

Approved by xiemaisi

Author: semmle-qlci

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. |

javascript/ql/src/semmle/javascript/security/SensitiveActions.qll

@@ -143,7 +143,7 @@ class AuthorizationCall extends SensitiveAction, DataFlow::CallNode {
     exists(string s | s = getCalleeName() |
       // name contains `login` or `auth`, but not as part of `loginfo` or `unauth`;
       // also exclude `author`
-      s.regexpMatch("(?i).*(login(?!fo)|(?<!un)auth(?!or\\b)).*") and
+      s.regexpMatch("(?i).*(login(?!fo)|(?<!un)auth(?!or\\b)|verify).*") and
       // but it does not start with `get` or `set`
       not s.regexpMatch("(?i)(get|set).*")
     )

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)
   }
 }
 

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 |

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