Skip to content

Commit 9aafe82

Browse files
codeql-cicodeql-ci
authored
Merge pull request github#6271 from erik-krogh/logs
Approved by asgerf
2 parents ef9d096 + 36de24a commit 9aafe82

File tree

4 files changed

+135
-0
lines changed

4 files changed

+135
-0
lines changed

‎javascript/change-notes/2021-07-12-logs.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
lgtm,codescanning
2+
* The `js/log-injection` query now recognizes more logging frameworks.
3+
Affected packages are
4+
[pino](https://npmjs.com/package/pino)

‎javascript/ql/src/semmle/javascript/frameworks/Logging.qll

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -338,6 +338,45 @@ class StripAnsiStep extends TaintTracking::SharedTaintStep {
338338
}
339339
}
340340

341+
/**
342+
* Provides classes and predicates for working with the `pino` library.
343+
*/
344+
private module Pino {
345+
/**
346+
* Gets a logger instance created by importing the `pino` library.
347+
*/
348+
private API::Node pinoApi() {
349+
result = API::moduleImport("pino").getReturn()
350+
or
351+
result = pinoApi().getMember("child").getReturn()
352+
}
353+
354+
/**
355+
* Gets a logger instance from the `pino` library.
356+
*/
357+
private API::Node pino() {
358+
result = pinoApi()
359+
or
360+
// `pino` is installed as the "log" property on the request object in `Express` and similar libraries.
361+
// in `Hapi` the property is "logger".
362+
exists(HTTP::RequestExpr req, API::Node reqNode |
363+
reqNode.getAnImmediateUse() = req.flow().getALocalSource() and
364+
result = reqNode.getMember(["log", "logger"])
365+
)
366+
}
367+
368+
/**
369+
* A logging call to the `pino` library.
370+
*/
371+
private class PinoCall extends LoggerCall {
372+
PinoCall() {
373+
this = pino().getMember(["trace", "debug", "info", "warn", "error", "fatal"]).getACall()
374+
}
375+
376+
override DataFlow::Node getAMessageComponent() { result = getAnArgument() }
377+
}
378+
}
379+
341380
/**
342381
* A step through the [`ansi-to-html`](https://npmjs.org/package/ansi-to-html) library.
343382
*/

‎javascript/ql/test/query-tests/Security/CWE-117/LogInjection.expected

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,24 @@ nodes
7676
| logInjectionBad.js:66:17:66:43 | prettyj ... ername) |
7777
| logInjectionBad.js:66:17:66:43 | prettyj ... ername) |
7878
| logInjectionBad.js:66:35:66:42 | username |
79+
| logInjectionBad.js:72:9:72:36 | q |
80+
| logInjectionBad.js:72:13:72:36 | url.par ... , true) |
81+
| logInjectionBad.js:72:23:72:29 | req.url |
82+
| logInjectionBad.js:72:23:72:29 | req.url |
83+
| logInjectionBad.js:73:9:73:35 | username |
84+
| logInjectionBad.js:73:20:73:20 | q |
85+
| logInjectionBad.js:73:20:73:26 | q.query |
86+
| logInjectionBad.js:73:20:73:35 | q.query.username |
87+
| logInjectionBad.js:75:15:75:22 | username |
88+
| logInjectionBad.js:75:15:75:22 | username |
89+
| logInjectionBad.js:82:30:82:37 | username |
90+
| logInjectionBad.js:82:30:82:37 | username |
91+
| logInjectionBad.js:91:26:91:33 | username |
92+
| logInjectionBad.js:91:26:91:33 | username |
93+
| logInjectionBad.js:99:26:99:33 | username |
94+
| logInjectionBad.js:99:26:99:33 | username |
95+
| logInjectionBad.js:113:37:113:44 | username |
96+
| logInjectionBad.js:113:37:113:44 | username |
7997
edges
8098
| logInjectionBad.js:19:9:19:36 | q | logInjectionBad.js:20:20:20:20 | q |
8199
| logInjectionBad.js:19:13:19:36 | url.par ... , true) | logInjectionBad.js:19:9:19:36 | q |
@@ -151,6 +169,23 @@ edges
151169
| logInjectionBad.js:64:20:64:35 | q.query.username | logInjectionBad.js:64:9:64:35 | username |
152170
| logInjectionBad.js:66:35:66:42 | username | logInjectionBad.js:66:17:66:43 | prettyj ... ername) |
153171
| logInjectionBad.js:66:35:66:42 | username | logInjectionBad.js:66:17:66:43 | prettyj ... ername) |
172+
| logInjectionBad.js:72:9:72:36 | q | logInjectionBad.js:73:20:73:20 | q |
173+
| logInjectionBad.js:72:13:72:36 | url.par ... , true) | logInjectionBad.js:72:9:72:36 | q |
174+
| logInjectionBad.js:72:23:72:29 | req.url | logInjectionBad.js:72:13:72:36 | url.par ... , true) |
175+
| logInjectionBad.js:72:23:72:29 | req.url | logInjectionBad.js:72:13:72:36 | url.par ... , true) |
176+
| logInjectionBad.js:73:9:73:35 | username | logInjectionBad.js:75:15:75:22 | username |
177+
| logInjectionBad.js:73:9:73:35 | username | logInjectionBad.js:75:15:75:22 | username |
178+
| logInjectionBad.js:73:9:73:35 | username | logInjectionBad.js:82:30:82:37 | username |
179+
| logInjectionBad.js:73:9:73:35 | username | logInjectionBad.js:82:30:82:37 | username |
180+
| logInjectionBad.js:73:9:73:35 | username | logInjectionBad.js:91:26:91:33 | username |
181+
| logInjectionBad.js:73:9:73:35 | username | logInjectionBad.js:91:26:91:33 | username |
182+
| logInjectionBad.js:73:9:73:35 | username | logInjectionBad.js:99:26:99:33 | username |
183+
| logInjectionBad.js:73:9:73:35 | username | logInjectionBad.js:99:26:99:33 | username |
184+
| logInjectionBad.js:73:9:73:35 | username | logInjectionBad.js:113:37:113:44 | username |
185+
| logInjectionBad.js:73:9:73:35 | username | logInjectionBad.js:113:37:113:44 | username |
186+
| logInjectionBad.js:73:20:73:20 | q | logInjectionBad.js:73:20:73:26 | q.query |
187+
| logInjectionBad.js:73:20:73:26 | q.query | logInjectionBad.js:73:20:73:35 | q.query.username |
188+
| logInjectionBad.js:73:20:73:35 | q.query.username | logInjectionBad.js:73:9:73:35 | username |
154189
#select
155190
| logInjectionBad.js:22:18:22:43 | `[INFO] ... rname}` | logInjectionBad.js:19:23:19:29 | req.url | logInjectionBad.js:22:18:22:43 | `[INFO] ... rname}` | $@ flows to log entry. | logInjectionBad.js:19:23:19:29 | req.url | User-provided value |
156191
| logInjectionBad.js:23:37:23:44 | username | logInjectionBad.js:19:23:19:29 | req.url | logInjectionBad.js:23:37:23:44 | username | $@ flows to log entry. | logInjectionBad.js:19:23:19:29 | req.url | User-provided value |
@@ -168,3 +203,8 @@ edges
168203
| logInjectionBad.js:57:17:57:48 | chalk.u ... ername) | logInjectionBad.js:46:23:46:29 | req.url | logInjectionBad.js:57:17:57:48 | chalk.u ... ername) | $@ flows to log entry. | logInjectionBad.js:46:23:46:29 | req.url | User-provided value |
169204
| logInjectionBad.js:58:17:58:59 | stripAn ... rname)) | logInjectionBad.js:46:23:46:29 | req.url | logInjectionBad.js:58:17:58:59 | stripAn ... rname)) | $@ flows to log entry. | logInjectionBad.js:46:23:46:29 | req.url | User-provided value |
170205
| logInjectionBad.js:66:17:66:43 | prettyj ... ername) | logInjectionBad.js:63:23:63:29 | req.url | logInjectionBad.js:66:17:66:43 | prettyj ... ername) | $@ flows to log entry. | logInjectionBad.js:63:23:63:29 | req.url | User-provided value |
206+
| logInjectionBad.js:75:15:75:22 | username | logInjectionBad.js:72:23:72:29 | req.url | logInjectionBad.js:75:15:75:22 | username | $@ flows to log entry. | logInjectionBad.js:72:23:72:29 | req.url | User-provided value |
207+
| logInjectionBad.js:82:30:82:37 | username | logInjectionBad.js:72:23:72:29 | req.url | logInjectionBad.js:82:30:82:37 | username | $@ flows to log entry. | logInjectionBad.js:72:23:72:29 | req.url | User-provided value |
208+
| logInjectionBad.js:91:26:91:33 | username | logInjectionBad.js:72:23:72:29 | req.url | logInjectionBad.js:91:26:91:33 | username | $@ flows to log entry. | logInjectionBad.js:72:23:72:29 | req.url | User-provided value |
209+
| logInjectionBad.js:99:26:99:33 | username | logInjectionBad.js:72:23:72:29 | req.url | logInjectionBad.js:99:26:99:33 | username | $@ flows to log entry. | logInjectionBad.js:72:23:72:29 | req.url | User-provided value |
210+
| logInjectionBad.js:113:37:113:44 | username | logInjectionBad.js:72:23:72:29 | req.url | logInjectionBad.js:113:37:113:44 | username | $@ flows to log entry. | logInjectionBad.js:72:23:72:29 | req.url | User-provided value |

‎javascript/ql/test/query-tests/Security/CWE-117/logInjectionBad.js

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,4 +64,56 @@ const server3 = http.createServer((req, res) => {
6464
let username = q.query.username;
6565

6666
console.log(prettyjson.render(username)); // NOT OK
67+
68+
});
69+
70+
const pino = require('pino')()
71+
const server4 = http.createServer((req, res) => {
72+
let q = url.parse(req.url, true);
73+
let username = q.query.username;
74+
75+
pino.info(username); // NOT OK
76+
77+
function fastify() {
78+
const fastify = require('fastify')({
79+
logger: true
80+
});
81+
fastify.get('/', async (request, reply) => {
82+
request.log.info(username); // NOT OK
83+
return { hello: 'world' }
84+
});
85+
}
86+
87+
function express() {
88+
const express = require('express');
89+
const app = express();
90+
app.get('/', (req, res) => {
91+
req.log.info(username); // NOT OK
92+
res.send({ hello: 'world' });
93+
});
94+
}
95+
96+
function http() {
97+
const http = require('http');
98+
const server = http.createServer((req, res) => {
99+
req.log.info(username); // NOT OK
100+
res.end('Hello World\n');
101+
});
102+
server.listen(3000);
103+
}
104+
105+
function hapi() {
106+
const Hapi = require('hapi');
107+
const server = new Hapi.Server();
108+
server.connection({ port: 3000 });
109+
server.route({
110+
method: 'GET',
111+
path: '/',
112+
handler: (request, reply) => {
113+
request.logger.info(username); // NOT OK
114+
reply({ hello: 'world' });
115+
}
116+
});
117+
server.start();
118+
}
67119
});

0 commit comments

Comments
 (0)