Skip to content

Commit 8e57f57

Browse files
codeql-cicodeql-ci
authored
Merge pull request #4559 from erik-krogh/redis
Approved by asgerf
2 parents f3e2bd0 + 75d996a commit 8e57f57

File tree

3 files changed

+221
-0
lines changed

3 files changed

+221
-0
lines changed

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

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -737,3 +737,108 @@ private module MarsDB {
737737
override DataFlow::Node getACodeOperator() { result = qc.getACodeOperator() }
738738
}
739739
}
740+
741+
/**
742+
* Provides classes modeling the `Node Redis` library.
743+
*
744+
* Redis is an in-memory key-value store and not a database,
745+
* but `Node Redis` can be exploited similarly to a NoSQL database by giving a method an array as argument instead of a string.
746+
* As an example the below two invocations of `client.set` are equivalent:
747+
*
748+
* ```
749+
* const redis = require("redis");
750+
* const client = redis.createClient();
751+
* client.set("key", "value");
752+
* client.set(["key", "value"]);
753+
* ```
754+
*
755+
* ioredis is a very similar library. However, ioredis does not support array arguments in the same way, and is therefore not vulnerable to the same kind of type confusion.
756+
*/
757+
private module Redis {
758+
/**
759+
* Gets a `Node Redis` client.
760+
*/
761+
private API::Node client() {
762+
result = API::moduleImport("redis").getMember("createClient").getReturn()
763+
or
764+
result = API::moduleImport("redis").getMember("RedisClient").getInstance()
765+
or
766+
result = client().getMember("duplicate").getReturn()
767+
or
768+
result = client().getMember("duplicate").getLastParameter().getParameter(1)
769+
}
770+
771+
/**
772+
* Gets a (possibly chained) reference to a batch operation object.
773+
* These have the same API as a redis client, except the calls are chained, and the sequence is terminated with a `.exec` call.
774+
*/
775+
private API::Node multi() {
776+
result = client().getMember(["multi", "batch"]).getReturn()
777+
or
778+
result = multi().getAMember().getReturn()
779+
}
780+
781+
/**
782+
* Gets a `Node Redis` client instance. Either a client created using `createClient()`, or a batch operation object.
783+
*/
784+
private API::Node redis() { result = [client(), multi()] }
785+
786+
/**
787+
* Provides signatures for the query methods from Node Redis.
788+
*/
789+
module QuerySignatures {
790+
/**
791+
* Holds if `method` interprets parameter `argIndex` as a key, and a later parameter determines a value/field.
792+
* Thereby the method is vulnerable if parameter `argIndex` is unexpectedly an array instead of a string, as an attacker can control arguments to Redis that the attacker was not supposed to control.
793+
*
794+
* Only setters and similar methods are included.
795+
* For getter-like methods it is not generally possible to gain access "outside" of where you are supposed to have access,
796+
* it is at most possible to get a Redis call to return more results than expected (e.g. by adding more members to [`geohash`](https://redis.io/commands/geohash)).
797+
*/
798+
bindingset[argIndex]
799+
predicate argumentIsAmbiguousKey(string method, int argIndex) {
800+
method =
801+
["set", "publish", "append", "bitfield", "decrby", "getset", "hincrby", "hincrbyfloat",
802+
"hset", "hsetnx", "incrby", "incrbyfloat", "linsert", "lpush", "lpushx", "lset",
803+
"ltrim", "rename", "renamenx", "rpushx", "setbit", "setex", "smove", "zincrby",
804+
"zinterstore", "hdel", "lpush", "pfadd", "rpush", "sadd", "sdiffstore", "srem"] and
805+
argIndex = 0
806+
or
807+
method = ["bitop", "hmset", "mset", "msetnx", "geoadd"] and argIndex >= 0
808+
}
809+
}
810+
811+
/**
812+
* An expression that is interpreted as a key in a Node Redis call.
813+
*/
814+
class RedisKeyArgument extends NoSQL::Query {
815+
RedisKeyArgument() {
816+
exists(string method, int argIndex |
817+
QuerySignatures::argumentIsAmbiguousKey(method, argIndex)
818+
|
819+
this =
820+
[promisify(redis().getMember(method)), redis().getMember(method)]
821+
.getACall()
822+
.getArgument(argIndex)
823+
.asExpr()
824+
)
825+
}
826+
}
827+
828+
/**
829+
* Gets a promisified version of `method`.
830+
*/
831+
private API::Node promisify(API::Node method) {
832+
exists(API::Node promisify |
833+
promisify = API::moduleImport(["util", "bluebird"]).getMember("promisify").getReturn() and
834+
method
835+
.getAnImmediateUse()
836+
.flowsTo(promisify.getAnImmediateUse().(DataFlow::CallNode).getArgument(0))
837+
|
838+
result = promisify
839+
or
840+
result = promisify.getMember("bind").getReturn() and
841+
result.getAnImmediateUse().(DataFlow::CallNode).getNumArgument() = 1
842+
)
843+
}
844+
}

‎javascript/ql/test/query-tests/Security/CWE-089/untyped/SqlInjection.expected‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,34 @@ nodes
159159
| mongooseModelClient.js:12:22:12:29 | req.body |
160160
| mongooseModelClient.js:12:22:12:29 | req.body |
161161
| mongooseModelClient.js:12:22:12:32 | req.body.id |
162+
| redis.js:10:16:10:23 | req.body |
163+
| redis.js:10:16:10:23 | req.body |
164+
| redis.js:10:16:10:27 | req.body.key |
165+
| redis.js:10:16:10:27 | req.body.key |
166+
| redis.js:12:9:12:26 | key |
167+
| redis.js:12:15:12:22 | req.body |
168+
| redis.js:12:15:12:22 | req.body |
169+
| redis.js:12:15:12:26 | req.body.key |
170+
| redis.js:18:16:18:18 | key |
171+
| redis.js:18:16:18:18 | key |
172+
| redis.js:19:43:19:45 | key |
173+
| redis.js:19:43:19:45 | key |
174+
| redis.js:25:14:25:16 | key |
175+
| redis.js:25:14:25:16 | key |
176+
| redis.js:30:23:30:25 | key |
177+
| redis.js:30:23:30:25 | key |
178+
| redis.js:32:28:32:30 | key |
179+
| redis.js:32:28:32:30 | key |
180+
| redis.js:38:11:38:28 | key |
181+
| redis.js:38:17:38:24 | req.body |
182+
| redis.js:38:17:38:24 | req.body |
183+
| redis.js:38:17:38:28 | req.body.key |
184+
| redis.js:39:16:39:18 | key |
185+
| redis.js:39:16:39:18 | key |
186+
| redis.js:43:27:43:29 | key |
187+
| redis.js:43:27:43:29 | key |
188+
| redis.js:46:34:46:36 | key |
189+
| redis.js:46:34:46:36 | key |
162190
| socketio.js:10:25:10:30 | handle |
163191
| socketio.js:10:25:10:30 | handle |
164192
| socketio.js:11:12:11:53 | `INSERT ... andle}` |
@@ -432,6 +460,32 @@ edges
432460
| mongooseModelClient.js:12:22:12:29 | req.body | mongooseModelClient.js:12:22:12:32 | req.body.id |
433461
| mongooseModelClient.js:12:22:12:32 | req.body.id | mongooseModelClient.js:12:16:12:34 | { id: req.body.id } |
434462
| mongooseModelClient.js:12:22:12:32 | req.body.id | mongooseModelClient.js:12:16:12:34 | { id: req.body.id } |
463+
| redis.js:10:16:10:23 | req.body | redis.js:10:16:10:27 | req.body.key |
464+
| redis.js:10:16:10:23 | req.body | redis.js:10:16:10:27 | req.body.key |
465+
| redis.js:10:16:10:23 | req.body | redis.js:10:16:10:27 | req.body.key |
466+
| redis.js:10:16:10:23 | req.body | redis.js:10:16:10:27 | req.body.key |
467+
| redis.js:12:9:12:26 | key | redis.js:18:16:18:18 | key |
468+
| redis.js:12:9:12:26 | key | redis.js:18:16:18:18 | key |
469+
| redis.js:12:9:12:26 | key | redis.js:19:43:19:45 | key |
470+
| redis.js:12:9:12:26 | key | redis.js:19:43:19:45 | key |
471+
| redis.js:12:9:12:26 | key | redis.js:25:14:25:16 | key |
472+
| redis.js:12:9:12:26 | key | redis.js:25:14:25:16 | key |
473+
| redis.js:12:9:12:26 | key | redis.js:30:23:30:25 | key |
474+
| redis.js:12:9:12:26 | key | redis.js:30:23:30:25 | key |
475+
| redis.js:12:9:12:26 | key | redis.js:32:28:32:30 | key |
476+
| redis.js:12:9:12:26 | key | redis.js:32:28:32:30 | key |
477+
| redis.js:12:15:12:22 | req.body | redis.js:12:15:12:26 | req.body.key |
478+
| redis.js:12:15:12:22 | req.body | redis.js:12:15:12:26 | req.body.key |
479+
| redis.js:12:15:12:26 | req.body.key | redis.js:12:9:12:26 | key |
480+
| redis.js:38:11:38:28 | key | redis.js:39:16:39:18 | key |
481+
| redis.js:38:11:38:28 | key | redis.js:39:16:39:18 | key |
482+
| redis.js:38:11:38:28 | key | redis.js:43:27:43:29 | key |
483+
| redis.js:38:11:38:28 | key | redis.js:43:27:43:29 | key |
484+
| redis.js:38:11:38:28 | key | redis.js:46:34:46:36 | key |
485+
| redis.js:38:11:38:28 | key | redis.js:46:34:46:36 | key |
486+
| redis.js:38:17:38:24 | req.body | redis.js:38:17:38:28 | req.body.key |
487+
| redis.js:38:17:38:24 | req.body | redis.js:38:17:38:28 | req.body.key |
488+
| redis.js:38:17:38:28 | req.body.key | redis.js:38:11:38:28 | key |
435489
| socketio.js:10:25:10:30 | handle | socketio.js:11:46:11:51 | handle |
436490
| socketio.js:10:25:10:30 | handle | socketio.js:11:46:11:51 | handle |
437491
| socketio.js:11:46:11:51 | handle | socketio.js:11:12:11:53 | `INSERT ... andle}` |
@@ -500,6 +554,15 @@ edges
500554
| mongooseJsonParse.js:23:19:23:23 | query | mongooseJsonParse.js:20:30:20:43 | req.query.data | mongooseJsonParse.js:23:19:23:23 | query | This query depends on $@. | mongooseJsonParse.js:20:30:20:43 | req.query.data | a user-provided value |
501555
| mongooseModelClient.js:11:16:11:24 | { id: v } | mongooseModelClient.js:10:22:10:29 | req.body | mongooseModelClient.js:11:16:11:24 | { id: v } | This query depends on $@. | mongooseModelClient.js:10:22:10:29 | req.body | a user-provided value |
502556
| mongooseModelClient.js:12:16:12:34 | { id: req.body.id } | mongooseModelClient.js:12:22:12:29 | req.body | mongooseModelClient.js:12:16:12:34 | { id: req.body.id } | This query depends on $@. | mongooseModelClient.js:12:22:12:29 | req.body | a user-provided value |
557+
| redis.js:10:16:10:27 | req.body.key | redis.js:10:16:10:23 | req.body | redis.js:10:16:10:27 | req.body.key | This query depends on $@. | redis.js:10:16:10:23 | req.body | a user-provided value |
558+
| redis.js:18:16:18:18 | key | redis.js:12:15:12:22 | req.body | redis.js:18:16:18:18 | key | This query depends on $@. | redis.js:12:15:12:22 | req.body | a user-provided value |
559+
| redis.js:19:43:19:45 | key | redis.js:12:15:12:22 | req.body | redis.js:19:43:19:45 | key | This query depends on $@. | redis.js:12:15:12:22 | req.body | a user-provided value |
560+
| redis.js:25:14:25:16 | key | redis.js:12:15:12:22 | req.body | redis.js:25:14:25:16 | key | This query depends on $@. | redis.js:12:15:12:22 | req.body | a user-provided value |
561+
| redis.js:30:23:30:25 | key | redis.js:12:15:12:22 | req.body | redis.js:30:23:30:25 | key | This query depends on $@. | redis.js:12:15:12:22 | req.body | a user-provided value |
562+
| redis.js:32:28:32:30 | key | redis.js:12:15:12:22 | req.body | redis.js:32:28:32:30 | key | This query depends on $@. | redis.js:12:15:12:22 | req.body | a user-provided value |
563+
| redis.js:39:16:39:18 | key | redis.js:38:17:38:24 | req.body | redis.js:39:16:39:18 | key | This query depends on $@. | redis.js:38:17:38:24 | req.body | a user-provided value |
564+
| redis.js:43:27:43:29 | key | redis.js:38:17:38:24 | req.body | redis.js:43:27:43:29 | key | This query depends on $@. | redis.js:38:17:38:24 | req.body | a user-provided value |
565+
| redis.js:46:34:46:36 | key | redis.js:38:17:38:24 | req.body | redis.js:46:34:46:36 | key | This query depends on $@. | redis.js:38:17:38:24 | req.body | a user-provided value |
503566
| socketio.js:11:12:11:53 | `INSERT ... andle}` | socketio.js:10:25:10:30 | handle | socketio.js:11:12:11:53 | `INSERT ... andle}` | This query depends on $@. | socketio.js:10:25:10:30 | handle | a user-provided value |
504567
| tst2.js:9:27:9:84 | "select ... d + "'" | tst2.js:9:66:9:78 | req.params.id | tst2.js:9:27:9:84 | "select ... d + "'" | This query depends on $@. | tst2.js:9:66:9:78 | req.params.id | a user-provided value |
505568
| tst3.js:9:14:9:19 | query1 | tst3.js:8:16:8:34 | req.params.category | tst3.js:9:14:9:19 | query1 | This query depends on $@. | tst3.js:8:16:8:34 | req.params.category | a user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-089/untyped/redis.js‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
2+
const redis = require("redis");
3+
const client = redis.createClient();
4+
5+
const Express = require('express');
6+
const app = Express();
7+
app.use(require('body-parser').json());
8+
9+
app.post('/documents/find', (req, res) => {
10+
client.set(req.body.key, "value"); // NOT OK
11+
12+
var key = req.body.key;
13+
if (typeof key === "string") {
14+
client.set(key, "value"); // OK
15+
client.set(["key", "value"]);
16+
}
17+
18+
client.set(key, "value"); // NOT OK
19+
client.hmset("key", "field", "value", key, "value2"); // NOT OK
20+
21+
// chain commands
22+
client
23+
.multi()
24+
.set("constant", "value")
25+
.set(key, "value") // NOT OK
26+
.get(key) // OK
27+
.exec(function (err, replies) { });
28+
29+
client.duplicate((err, newClient) => {
30+
newClient.set(key, "value"); // NOT OK
31+
});
32+
client.duplicate().set(key, "value"); // NOT OK
33+
});
34+
35+
36+
import { promisify } from 'util';
37+
app.post('/documents/find', (req, res) => {
38+
const key = req.body.key;
39+
client.set(key, "value"); // NOT OK
40+
41+
const setAsync = promisify(client.set).bind(client);
42+
43+
const foo1 = setAsync(key, "value"); // NOT OK
44+
45+
client.setAsync = promisify(client.set);
46+
const foo2 = client.setAsync(key, "value"); // NOT OK
47+
48+
client.unrelated = promisify(() => {});
49+
const foo3 = client.unrelated(key, "value"); // OK
50+
51+
const unrelated = promisify(client.foobar).bind(client);
52+
const foo4 = unrelated(key, "value"); // OK
53+
});

0 commit comments

Comments
 (0)