Skip to content

Commit e4bb011

Browse files
Bruce Irschickbirschick-bq
Bruce Irschick
and
birschick-bq
authored
[AD-997] Add tests to ensure SQL or mongo injection is not possible. (#443)
* [AD-997] Add tests to ensure SQL or mongo injection is not possible. * Commit Code Coverage Badge * [AD-997] Add tests for handling single quotes. Improve "quote" method to escape the quote character. * [AD-997] Fix checkStyles issues. * Commit Code Coverage Badge * [AD-997] Address review comments. * Commit Code Coverage Badge Co-authored-by: birschick-bq <[email protected]>
1 parent f59af16 commit e4bb011

File tree

3 files changed

+335
-7
lines changed

3 files changed

+335
-7
lines changed

‎build.gradle

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -323,8 +323,8 @@ repositories {
323323

324324
dependencies {
325325
implementation group: 'commons-cli', name: 'commons-cli', version: '1.5.0'
326-
implementation group: 'com.fasterxml.jackson.core', name: 'jackson-core', version: '2.14.0'
327-
implementation group: 'com.fasterxml.jackson.datatype', name: 'jackson-datatype-guava', version: '2.14.0'
326+
implementation group: 'com.fasterxml.jackson.core', name: 'jackson-core', version: '2.14.1'
327+
implementation group: 'com.fasterxml.jackson.datatype', name: 'jackson-datatype-guava', version: '2.14.1'
328328
implementation group: 'com.google.guava', name: 'guava', version: '31.1-jre'
329329
implementation group: 'org.slf4j', name: 'slf4j-log4j12', version: '2.0.4'
330330
implementation group: 'org.apache.commons', name: 'commons-text', version: '1.10.0'

‎src/main/java/software/amazon/documentdb/jdbc/calcite/adapter/DocumentDbRules.java

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@
7373
import java.time.temporal.ChronoUnit;
7474
import java.util.AbstractList;
7575
import java.util.ArrayList;
76+
import java.util.Collections;
7677
import java.util.HashMap;
7778
import java.util.List;
7879
import java.util.Locale;
@@ -92,6 +93,13 @@
9293
public final class DocumentDbRules {
9394
private static final Logger LOGGER = CalciteTrace.getPlannerTracer();
9495
private static final Pattern OBJECT_ID_PATTERN = Pattern.compile("^[0-9a-zA-Z]{24}$");
96+
static final Map<String, String> ESCAPE_MAP;
97+
98+
static {
99+
final Map<String, String> escapeMap = new HashMap<>();
100+
escapeMap.put("[']", "\\'");
101+
ESCAPE_MAP = Collections.unmodifiableMap(escapeMap);
102+
}
95103

96104
private DocumentDbRules() { }
97105

@@ -176,24 +184,39 @@ static List<String> mongoFieldNames(final RelDataType rowType, final DocumentDbS
176184
}
177185

178186
static String maybeQuote(final String s) {
179-
if (!needsQuote(s)) {
187+
if (!needsQuote(s, '\'')) {
180188
return s;
181189
}
182-
return quote(s);
190+
return quote(s, '\'', ESCAPE_MAP);
183191
}
184192

185193
static String quote(final String s) {
186-
return "'" + s + "'"; // TODO: handle embedded quotes
194+
return quote(s, '\'', ESCAPE_MAP);
195+
}
196+
197+
/**
198+
* Quotes a string with the given quote character, handling any escape substitutions provided.
199+
* @param s the value to quote.
200+
* @param quoteChar the character to quote the value with.
201+
* @param escapeMap the map of regular expressions to escaped replacement values.
202+
* @return an escaped and quoted value.
203+
*/
204+
public static String quote(final String s, final char quoteChar, final Map<String, String> escapeMap) {
205+
String value = s;
206+
for (Map.Entry<String, String> entry : escapeMap.entrySet()) {
207+
value = value.replaceAll(entry.getKey(), entry.getValue());
208+
}
209+
return quoteChar + value + quoteChar;
187210
}
188211

189-
private static boolean needsQuote(final String s) {
212+
private static boolean needsQuote(final String s, final char quoteChar) {
190213
for (int i = 0, n = s.length(); i < n; i++) {
191214
final char c = s.charAt(i);
192215
// DocumentDB: modified - start
193216
// Add quotes for embedded documents (contains '.') and
194217
// for field names with ':'.
195218
if (!Character.isJavaIdentifierPart(c)
196-
|| c == '$' || c == '.' || c == ':') {
219+
|| c == quoteChar || c == '$' || c == '.' || c == ':') {
197220
return true;
198221
}
199222
// DocumentDB: modified - end
@@ -449,6 +472,7 @@ protected RexToMongoTranslator(final JavaTypeFactory typeFactory,
449472
+ "\", \"subType\": \"00\"}}";
450473
return new Operand(binaryFormat, binaryFormat, true);
451474
default:
475+
// Note: If type is [[LONG][N]VAR]CHAR, this call returns a properly escaped double-quoted string.
452476
final String simpleLiteral = RexToLixTranslator.translateLiteral(literal, literal.getType(),
453477
typeFactory, NullAs.NOT_POSSIBLE).toString();
454478
return new Operand("{\"$literal\": " + simpleLiteral + "}", simpleLiteral, true);

‎src/test/java/software/amazon/documentdb/jdbc/query/limitations/DocumentDbSqlInjectionTest.java

Lines changed: 304 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,304 @@
1+
/*
2+
* Copyright <2021> Amazon.com, Inc. or its affiliates. All Rights Reserved.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License").
5+
* You may not use this file except in compliance with the License.
6+
* A copy of the License is located at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* or in the "license" file accompanying this file. This file is distributed
11+
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
12+
* express or implied. See the License for the specific language governing
13+
* permissions and limitations under the License.
14+
*
15+
*/
16+
17+
package software.amazon.documentdb.jdbc.query.limitations;
18+
19+
import org.bson.BsonArray;
20+
import org.bson.BsonDocument;
21+
import org.bson.BsonValue;
22+
import org.bson.conversions.Bson;
23+
import org.checkerframework.checker.nullness.qual.NonNull;
24+
import org.junit.jupiter.api.Assertions;
25+
import org.junit.jupiter.api.BeforeAll;
26+
import org.junit.jupiter.api.Test;
27+
import org.junit.jupiter.api.extension.ExtendWith;
28+
import software.amazon.documentdb.jdbc.common.test.DocumentDbFlapDoodleExtension;
29+
import software.amazon.documentdb.jdbc.query.DocumentDbMqlQueryContext;
30+
import software.amazon.documentdb.jdbc.query.DocumentDbQueryMappingService;
31+
import software.amazon.documentdb.jdbc.query.DocumentDbQueryMappingServiceTest;
32+
33+
import java.sql.SQLException;
34+
import java.util.Collections;
35+
import java.util.List;
36+
import java.util.Map;
37+
38+
import static software.amazon.documentdb.jdbc.calcite.adapter.DocumentDbRules.quote;
39+
40+
@ExtendWith(DocumentDbFlapDoodleExtension.class)
41+
class DocumentDbSqlInjectionTest extends DocumentDbQueryMappingServiceTest {
42+
private static final String COLLECTION_NAME = "testCollectionInjectionTest";
43+
private static final String OTHER_COLLECTION_NAME = "otherTestCollectionInjectionTest";
44+
private DocumentDbQueryMappingService queryMapper;
45+
46+
@BeforeAll
47+
void beforeAll() throws SQLException {
48+
final BsonDocument document =
49+
BsonDocument.parse(
50+
"{ \"_id\" : \"key\", \"array\" : [ "
51+
+ "{ \"field\" : 1, \"field1\": \"value\" }, "
52+
+ "{ \"field\" : 2, \"field2\" : \"value\" , \"field3\" : { \"field4\": 3} } ]}");
53+
final BsonDocument otherDocument =
54+
BsonDocument.parse("{\"_id\": \"key1\", \"otherArray\": [" +
55+
"{\"field\": 1, \"field3\": \"value\"}, " +
56+
"{\"field\": 2, \"field3\": \"value\"}]}");
57+
insertBsonDocuments(COLLECTION_NAME, new BsonDocument[] {document});
58+
insertBsonDocuments(OTHER_COLLECTION_NAME, new BsonDocument[] {otherDocument});
59+
queryMapper = getQueryMappingService();
60+
}
61+
62+
@Test
63+
void testMongoInjections() throws SQLException {
64+
final String primaryKeyColumnName = COLLECTION_NAME + "__id";
65+
final String expectedKey = "$delete";
66+
final String injection = String.format(
67+
"\"}, {$delete: {\"%1$s\", \"1\"}",
68+
primaryKeyColumnName);
69+
final String query = String.format(
70+
"SELECT \"%1$s\", \"%2$s\", \"%3$s\" FROM \"%4$s\".\"%5$s\"" +
71+
" WHERE \"%1$s\" = '%6$s'",
72+
primaryKeyColumnName,
73+
"field",
74+
"field1",
75+
getDatabaseName(),
76+
COLLECTION_NAME + "_array",
77+
injection);
78+
final DocumentDbMqlQueryContext queryContext = queryMapper.get(query);
79+
Assertions.assertNotNull(queryContext);
80+
final List<Bson> aggregateOperations = queryContext.getAggregateOperations();
81+
// Assert that the attempted injection did not add a '$delete' operation.
82+
assertKeyNotExists(expectedKey, aggregateOperations);
83+
// Assert that the attempted injection is interpreted as a '$literal' value
84+
assertValueExists(injection, aggregateOperations);
85+
86+
final String query2 = String.format(
87+
"SELECT \"%1$s\", \"%2$s\", \"%3$s\" FROM (SELECT * FROM \"%4$s\".\"%5$s\"" +
88+
" WHERE \"%1$s\" = '%6$s')",
89+
primaryKeyColumnName,
90+
"field",
91+
"field1",
92+
getDatabaseName(),
93+
COLLECTION_NAME + "_array",
94+
injection);
95+
final DocumentDbMqlQueryContext queryContext2 = queryMapper.get(query2);
96+
Assertions.assertNotNull(queryContext2);
97+
final List<Bson> aggregateOperations2 = queryContext2.getAggregateOperations();
98+
assertKeyNotExists(expectedKey, aggregateOperations2);
99+
assertValueExists(injection, aggregateOperations2);
100+
101+
final String query3 = String.format(
102+
"SELECT \"%1$s\", \"%2$s\", \"%3$s\" FROM \"%4$s\".\"%5$s\"" +
103+
" WHERE \"%1$s\" = SUBSTRING('%6$s', 1, 2000)",
104+
primaryKeyColumnName,
105+
"field",
106+
"field1",
107+
getDatabaseName(),
108+
COLLECTION_NAME + "_array",
109+
injection);
110+
final DocumentDbMqlQueryContext queryContext3 = queryMapper.get(query3);
111+
Assertions.assertNotNull(queryContext3);
112+
final List<Bson> aggregateOperations3 = queryContext3.getAggregateOperations();
113+
assertKeyNotExists(expectedKey, aggregateOperations3);
114+
assertValueExists(injection, aggregateOperations3);
115+
116+
final String query4 = String.format(
117+
"SELECT \"%1$s\", \"%2$s\", \"%3$s\" FROM \"%4$s\".\"%5$s\"" +
118+
" WHERE \"%1$s\" = CONCAT('%6$s', '')",
119+
primaryKeyColumnName,
120+
"field",
121+
"field1",
122+
getDatabaseName(),
123+
COLLECTION_NAME + "_array",
124+
injection);
125+
final DocumentDbMqlQueryContext queryContext4 = queryMapper.get(query4);
126+
Assertions.assertNotNull(queryContext4);
127+
final List<Bson> aggregateOperations4 = queryContext4.getAggregateOperations();
128+
assertKeyNotExists(expectedKey, aggregateOperations4);
129+
assertValueExists(injection, aggregateOperations4);
130+
131+
final String query5 = String.format(
132+
"SELECT \"%1$s\", \"%2$s\", \"%3$s\" FROM \"%4$s\".\"%5$s\"" +
133+
" WHERE \"%1$s\" = REVERSE('%6$s')",
134+
primaryKeyColumnName,
135+
"field",
136+
"field1",
137+
getDatabaseName(),
138+
COLLECTION_NAME + "_array",
139+
new StringBuilder(injection).reverse());
140+
final DocumentDbMqlQueryContext queryContext5 = queryMapper.get(query5);
141+
Assertions.assertNotNull(queryContext5);
142+
final List<Bson> aggregateOperations5 = queryContext5.getAggregateOperations();
143+
assertKeyNotExists(expectedKey, aggregateOperations5);
144+
assertValueExists(injection, aggregateOperations5);
145+
146+
// Single-quotes
147+
final String injection6 = String.format(
148+
"'}, {$delete: {'%1$s', '1'}",
149+
primaryKeyColumnName);
150+
final String query6 = String.format(
151+
"SELECT \"%1$s\", \"%2$s\", \"%3$s\" FROM \"%4$s\".\"%5$s\"" +
152+
" WHERE \"%1$s\" = %6$s",
153+
primaryKeyColumnName,
154+
"field",
155+
"field1",
156+
getDatabaseName(),
157+
COLLECTION_NAME + "_array",
158+
quote(injection6, '\'', Collections.singletonMap("[']", "''")));
159+
final DocumentDbMqlQueryContext queryContext6 = queryMapper.get(query6);
160+
Assertions.assertNotNull(queryContext6);
161+
final List<Bson> aggregateOperations6 = queryContext6.getAggregateOperations();
162+
assertKeyNotExists(expectedKey, aggregateOperations6);
163+
assertValueExists(injection6, aggregateOperations6);
164+
}
165+
166+
@Test
167+
void testSqlInjections() throws SQLException {
168+
final String primaryKeyColumnName = COLLECTION_NAME + "__id";
169+
final String injection = String.format(
170+
"'; DELETE FROM \"%1$s\" WHERE \"%2$s\" <> '",
171+
COLLECTION_NAME,
172+
primaryKeyColumnName);
173+
final String query = String.format(
174+
"SELECT \"%1$s\", \"%2$s\", \"%3$s\" FROM \"%4$s\".\"%5$s\"" +
175+
" WHERE \"%1$s\" = '%6$s'",
176+
primaryKeyColumnName,
177+
"field",
178+
"field1",
179+
getDatabaseName(),
180+
COLLECTION_NAME + "_array",
181+
injection);
182+
final Exception exception = Assertions.assertThrows(SQLException.class, () -> queryMapper.get(query));
183+
Assertions.assertTrue(exception.getMessage().contains("Reason: 'parse failed: Encountered \";\" at line 1"));
184+
185+
final String query2 = String.format(
186+
"SELECT \"%1$s\", \"%2$s\", \"%3$s\" FROM (SELECT * FROM \"%4$s\".\"%5$s\"" +
187+
" WHERE \"%1$s\" = '%6$s')",
188+
primaryKeyColumnName,
189+
"field",
190+
"field1",
191+
getDatabaseName(),
192+
COLLECTION_NAME + "_array",
193+
injection);
194+
final Exception exception2 = Assertions.assertThrows(SQLException.class, () -> queryMapper.get(query2));
195+
Assertions.assertTrue(exception2.getMessage().contains("Reason: 'parse failed: Encountered \";\" at line 1"));
196+
197+
// Assume SQL application will correctly escape input strings, as below
198+
final String injection3 = "'--";
199+
final String query3 = String.format(
200+
"SELECT \"%1$s\", \"%2$s\", \"%3$s\" FROM \"%4$s\".\"%5$s\"" +
201+
" WHERE \"%1$s\" > %6$s AND \"%1$s\" < 'detect value'",
202+
primaryKeyColumnName,
203+
"field",
204+
"field1",
205+
getDatabaseName(),
206+
COLLECTION_NAME + "_array",
207+
quote(injection3, '\'', Collections.singletonMap("[']", "''")));
208+
final DocumentDbMqlQueryContext queryContext3 = queryMapper.get(query3);
209+
Assertions.assertNotNull(queryContext3);
210+
final List<Bson> aggregateOperations3 = queryContext3.getAggregateOperations();
211+
assertValueExists("detect value", aggregateOperations3);
212+
assertValueExists(injection3, aggregateOperations3);
213+
}
214+
215+
private static void assertKeyNotExists(final @NonNull String expectedKey, final List<Bson> aggregateOperations) {
216+
for (final Bson op : aggregateOperations) {
217+
final BsonDocument doc = op.toBsonDocument();
218+
assertKeyNotExists(expectedKey, doc);
219+
}
220+
}
221+
222+
private static void assertValueExists(final @NonNull String injection, final List<Bson> aggregateOperations) {
223+
boolean valueExists = false;
224+
for (final Bson op : aggregateOperations) {
225+
final BsonDocument doc = op.toBsonDocument();
226+
valueExists = isValueExists(injection, doc);
227+
if (valueExists) {
228+
break;
229+
}
230+
}
231+
Assertions.assertTrue(valueExists);
232+
}
233+
234+
private static boolean isValueExists(final @NonNull String injection, final BsonDocument doc) {
235+
boolean valueExists = false;
236+
for (final Map.Entry<String, BsonValue> entry : doc.entrySet()) {
237+
final BsonValue bsonValue = entry.getValue();
238+
if (bsonValue.isDocument()) {
239+
valueExists = isValueExists(injection, bsonValue.asDocument());
240+
if (valueExists) {
241+
break;
242+
}
243+
} else if (bsonValue.isArray()) {
244+
valueExists = isValueExists(injection, bsonValue.asArray());
245+
if (valueExists) {
246+
break;
247+
}
248+
} else if (bsonValue.isString()) {
249+
final String actualValue = bsonValue.asString().getValue();
250+
if (injection.equals(actualValue)) {
251+
valueExists = true;
252+
break;
253+
}
254+
}
255+
}
256+
return valueExists;
257+
}
258+
259+
private static boolean isValueExists(final @NonNull String injection, final BsonArray array) {
260+
boolean valueExists = false;
261+
for (final BsonValue arrayValue : array) {
262+
if (arrayValue.isDocument()) {
263+
valueExists = isValueExists(injection, arrayValue.asDocument());
264+
if (valueExists) {
265+
break;
266+
}
267+
} else if (arrayValue.isArray()) {
268+
valueExists = isValueExists(injection, arrayValue.asArray());
269+
if (valueExists) {
270+
break;
271+
}
272+
} else if (arrayValue.isString()) {
273+
final String actualValue = array.asString().getValue();
274+
if (injection.equals(actualValue)) {
275+
valueExists = true;
276+
break;
277+
}
278+
}
279+
}
280+
return valueExists;
281+
}
282+
283+
private static void assertKeyNotExists(final @NonNull String expectedKey, final BsonDocument doc) {
284+
for (final Map.Entry<String, BsonValue> entry : doc.entrySet()) {
285+
final String actualKey = entry.getKey();
286+
Assertions.assertNotEquals(expectedKey, actualKey);
287+
if (entry.getValue().isDocument()) {
288+
assertKeyNotExists(expectedKey, entry.getValue().asDocument());
289+
} else if (entry.getValue().isArray()) {
290+
assertKeyNotExists(expectedKey, entry.getValue().asArray());
291+
}
292+
}
293+
}
294+
295+
private static void assertKeyNotExists(final @NonNull String expectedKey, final BsonArray array) {
296+
for (final BsonValue value : array) {
297+
if (value.isDocument()) {
298+
assertKeyNotExists(expectedKey, value.asDocument());
299+
} else if (value.isArray()) {
300+
assertKeyNotExists(expectedKey, value.asArray());
301+
}
302+
}
303+
}
304+
}

0 commit comments

Comments
 (0)