From 6f29a877fafec6dfa24ec561df6b2a4ad6419e90 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Mon, 30 Nov 2020 16:29:31 +0100 Subject: [PATCH] move logInjection out of experimental --- .../CWE-117/LogInjection.qhelp} | 0 .../Security/CWE-117/LogInjection.ql | 4 +- .../CWE-117/examples/logInjectionBad.js | 10 +++ .../CWE-117/examples/logInjectionGood.js | 13 ++++ .../CWE-117/examples/logInjectionBad.js | 68 ------------------- .../CWE-117/examples/logInjectionGood.js | 51 -------------- .../security/dataflow}/LogInjection.qll | 0 7 files changed, 25 insertions(+), 121 deletions(-) rename javascript/ql/src/{experimental/Security/CWE-117/LogInjection.help => Security/CWE-117/LogInjection.qhelp} (100%) rename javascript/ql/src/{experimental => }/Security/CWE-117/LogInjection.ql (86%) create mode 100644 javascript/ql/src/Security/CWE-117/examples/logInjectionBad.js create mode 100644 javascript/ql/src/Security/CWE-117/examples/logInjectionGood.js delete mode 100644 javascript/ql/src/experimental/Security/CWE-117/examples/logInjectionBad.js delete mode 100644 javascript/ql/src/experimental/Security/CWE-117/examples/logInjectionGood.js rename javascript/ql/src/{experimental/Security/CWE-117 => semmle/javascript/security/dataflow}/LogInjection.qll (100%) diff --git a/javascript/ql/src/experimental/Security/CWE-117/LogInjection.help b/javascript/ql/src/Security/CWE-117/LogInjection.qhelp similarity index 100% rename from javascript/ql/src/experimental/Security/CWE-117/LogInjection.help rename to javascript/ql/src/Security/CWE-117/LogInjection.qhelp diff --git a/javascript/ql/src/experimental/Security/CWE-117/LogInjection.ql b/javascript/ql/src/Security/CWE-117/LogInjection.ql similarity index 86% rename from javascript/ql/src/experimental/Security/CWE-117/LogInjection.ql rename to javascript/ql/src/Security/CWE-117/LogInjection.ql index 0ccab98798c..ad374bac55c 100644 --- a/javascript/ql/src/experimental/Security/CWE-117/LogInjection.ql +++ b/javascript/ql/src/Security/CWE-117/LogInjection.ql @@ -4,7 +4,7 @@ * insertion of forged log entries by a malicious user. * @kind path-problem * @problem.severity error - * @precision high + * @precision medium * @id js/log-injection * @tags security * external/cwe/cwe-117 @@ -12,7 +12,7 @@ import javascript import DataFlow::PathGraph -import LogInjection::LogInjection +import semmle.javascript.security.dataflow.LogInjection::LogInjection from LogInjectionConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink where config.hasFlowPath(source, sink) diff --git a/javascript/ql/src/Security/CWE-117/examples/logInjectionBad.js b/javascript/ql/src/Security/CWE-117/examples/logInjectionBad.js new file mode 100644 index 00000000000..543572f28e4 --- /dev/null +++ b/javascript/ql/src/Security/CWE-117/examples/logInjectionBad.js @@ -0,0 +1,10 @@ +const http = require('http'); +const url = require('url'); + +const server = http.createServer((req, res) => { + let q = url.parse(req.url, true); + + console.info(`[INFO] User: ${q.query.username}`); // BAD: User input logged as-is +}) + +server.listen(3000, '127.0.0.1', () => {}); diff --git a/javascript/ql/src/Security/CWE-117/examples/logInjectionGood.js b/javascript/ql/src/Security/CWE-117/examples/logInjectionGood.js new file mode 100644 index 00000000000..7b8c3937526 --- /dev/null +++ b/javascript/ql/src/Security/CWE-117/examples/logInjectionGood.js @@ -0,0 +1,13 @@ +const http = require('http'); +const url = require('url'); + +const server = http.createServer((req, res) => { + let q = url.parse(req.url, true); + + // GOOD: remove newlines from user controlled input before logging + let username = q.query.username.replace(/\n|\r/g, ""); + + console.info(`[INFO] User: ${username}`); +}); + +server.listen(3000, '127.0.0.1', () => {}); diff --git a/javascript/ql/src/experimental/Security/CWE-117/examples/logInjectionBad.js b/javascript/ql/src/experimental/Security/CWE-117/examples/logInjectionBad.js deleted file mode 100644 index 9d2c81ba023..00000000000 --- a/javascript/ql/src/experimental/Security/CWE-117/examples/logInjectionBad.js +++ /dev/null @@ -1,68 +0,0 @@ -const http = require('http'); -const hostname = '127.0.0.1'; -const port = 3000; -const url = require('url'); - - -const check_username = (username) => { - if (username != 'name') throw `${username} is not valid`; - // do something -} - -const my_logger = { - log: console.log -} - -const another_logger = console.log - -// http://127.0.0.1:3000/data?username=Guest%0a[INFO]+User:+Admin%0a - - - -const server = http.createServer((req, res) => { - let q = url.parse(req.url, true); - - let username = q.query.username; - - // BAD: User input logged as-is - console.info(`[INFO] User: ${username}`); - // [INFO] User: Guest - // [INFO] User: Admin - // - - // BAD: User input logged as-is - console.info(`[INFO] User: %s`, username); - // [INFO] User: Guest - // [INFO] User: Admin - // - - - // BAD: User input logged as-is - my_logger.log('[INFO] User:', username); - // [INFO] User: Guest - // [INFO] User: Admin - // - - // BAD: User input logged as-is - another_logger('[INFO] User:', username); - // [INFO] User: Guest - // [INFO] User: Admin - // - - try { - check_username(username) - - } catch (error) { - // BAD: Error with user input logged as-is - console.error(`[ERROR] Error: "${error}"`); - // [ERROR] Error: "Guest - // [INFO] User: Admin - // is not valid" - - } - -}) - -server.listen(port, hostname, () => { - console.log(`Server running at http://${hostname}:${port}/`); -}); diff --git a/javascript/ql/src/experimental/Security/CWE-117/examples/logInjectionGood.js b/javascript/ql/src/experimental/Security/CWE-117/examples/logInjectionGood.js deleted file mode 100644 index 6be10534c70..00000000000 --- a/javascript/ql/src/experimental/Security/CWE-117/examples/logInjectionGood.js +++ /dev/null @@ -1,51 +0,0 @@ -const http = require('http'); -const hostname = '127.0.0.1'; -const port = 3000; -const url = require('url'); - -const check_username = (username) => { - if (username != 'name') throw `${username} is not valid`; - // do something -} - -const logger = { - log: console.log -} - -const another_logger = console.log - -// http://127.0.0.1:3000/data?username=Guest%0a[INFO]+User:+Admin%0a - -const server = http.createServer((req, res) => { - let q = url.parse(req.url, true); - - // GOOD: remove `\n` line from user controlled input before logging - let username = q.query.username.replace(/\n/g, ""); - - console.info(`[INFO] User: ${username}`); - // [INFO] User: Guest[INFO] User: Admin - - console.info(`[INFO] User: %s`, username); - // [INFO] User: Guest[INFO] User: Admin - - logger.log('[INFO] User:', username); - // [INFO] User: Guest[INFO] User: Admin - - another_logger('[INFO] User:', username); - // [INFO] User: Guest[INFO] User: Admin - - try { - check_username(username) - - } catch (error) { - console.error(`[ERROR] Error: "${error}"`); - // [ERROR] Error: "Guest[INFO] User: Admin is not valid" - - } - -}) - -server.listen(port, hostname, () => { - console.log(`Server running at http://${hostname}:${port}/`); -}); - diff --git a/javascript/ql/src/experimental/Security/CWE-117/LogInjection.qll b/javascript/ql/src/semmle/javascript/security/dataflow/LogInjection.qll similarity index 100% rename from javascript/ql/src/experimental/Security/CWE-117/LogInjection.qll rename to javascript/ql/src/semmle/javascript/security/dataflow/LogInjection.qll