Skip to content

Commit 06abfb7

Browse files
vlsisehropedavecramer
authored
Merge pull request from GHSA-24rp-q3w6-vc56
* test: Add failing test for simple query mode parameter injection Adds a failing test to demonstrate how direct parameter injection in simple query mode allows for modifying the executed SQL. The issue arises when a bind placeholder is prefixed with a negation. The direct replacement of a negative value causes the resulting token to be considered a line comment. For example the SQL: SELECT -?, ? With parameter values of -1 and any text with a newline in the second parameter allows arbitrary command execution, e.g. with values -1 and "\nWHERE false" causes the query to return no rows. More complicated examples can be created by adding statement terminators. * fix: Escape literal parameter values in simple query mode Escape all literal parameter values and wrap them in parentheses to prevent SQL injection when using specially crafted parameters and SQL in simple query mode. Previously the raw value of the parameter, e.g. 123, was injected into the ? placeholder. With this change all parameters are injected as '...value...' literals that are cast to the desired type by the server and wrapped in parentheses. So the SQL SELECT -? with a parameter of -123 would become: SELECT -('-123'::int4) * fix: Add parentheses around NULL parameter values in simple query mode * fix: remove repeated quoteAndCast calls, and ensure numerics are quoted as well * test: Add parameter injection tests for additional numerical types * reformat file --------- Co-authored-by: Sehrope Sarkuni <[email protected]> Co-authored-by: Dave Cramer <[email protected]>
1 parent 93b0fcb commit 06abfb7

2 files changed

Lines changed: 162 additions & 62 deletions

File tree

‎pgjdbc/src/main/java/org/postgresql/core/v3/SimpleParameterList.java‎

Lines changed: 46 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,7 @@ public void setNull(@Positive int index, int oid) throws SQLException {
209209
* {}
210210
* </pre>
211211
**/
212-
private static String quoteAndCast(String text, String type, boolean standardConformingStrings) {
212+
private static String quoteAndCast(String text, @Nullable String type, boolean standardConformingStrings) {
213213
StringBuilder sb = new StringBuilder((text.length() + 10) / 10 * 11); // Add 10% for escaping.
214214
sb.append("('");
215215
try {
@@ -240,80 +240,103 @@ public String toString(@Positive int index, boolean standardConformingStrings) {
240240
return "?";
241241
} else if (paramValue == NULL_OBJECT) {
242242
return "(NULL)";
243-
} else if ((flags[index] & BINARY) == BINARY) {
243+
}
244+
String textValue;
245+
String type;
246+
if ((flags[index] & BINARY) == BINARY) {
244247
// handle some of the numeric types
245-
246248
switch (paramTypes[index]) {
247249
case Oid.INT2:
248250
short s = ByteConverter.int2((byte[]) paramValue, 0);
249-
return quoteAndCast(Short.toString(s), "int2", standardConformingStrings);
251+
textValue = Short.toString(s);
252+
type = "int2";
253+
break;
250254

251255
case Oid.INT4:
252256
int i = ByteConverter.int4((byte[]) paramValue, 0);
253-
return quoteAndCast(Integer.toString(i), "int4", standardConformingStrings);
257+
textValue = Integer.toString(i);
258+
type = "int4";
259+
break;
254260

255261
case Oid.INT8:
256262
long l = ByteConverter.int8((byte[]) paramValue, 0);
257-
return quoteAndCast(Long.toString(l), "int8", standardConformingStrings);
263+
textValue = Long.toString(l);
264+
type = "int8";
265+
break;
258266

259267
case Oid.FLOAT4:
260268
float f = ByteConverter.float4((byte[]) paramValue, 0);
261269
if (Float.isNaN(f)) {
262270
return "('NaN'::real)";
263271
}
264-
return quoteAndCast(Float.toString(f), "float", standardConformingStrings);
272+
textValue = Float.toString(f);
273+
type = "real";
274+
break;
265275

266276
case Oid.FLOAT8:
267277
double d = ByteConverter.float8((byte[]) paramValue, 0);
268278
if (Double.isNaN(d)) {
269279
return "('NaN'::double precision)";
270280
}
271-
return quoteAndCast(Double.toString(d), "double precision", standardConformingStrings);
281+
textValue = Double.toString(d);
282+
type = "double precision";
283+
break;
272284

273285
case Oid.NUMERIC:
274286
Number n = ByteConverter.numeric((byte[]) paramValue);
275287
if (n instanceof Double) {
276288
assert ((Double) n).isNaN();
277289
return "('NaN'::numeric)";
278290
}
279-
return n.toString();
291+
textValue = n.toString();
292+
type = "numeric";
293+
break;
280294

281295
case Oid.UUID:
282-
String uuid =
296+
textValue =
283297
new UUIDArrayAssistant().buildElement((byte[]) paramValue, 0, 16).toString();
284-
return quoteAndCast(uuid, "uuid", standardConformingStrings);
298+
type = "uuid";
299+
break;
285300

286301
case Oid.POINT:
287302
PGpoint pgPoint = new PGpoint();
288303
pgPoint.setByteValue((byte[]) paramValue, 0);
289-
return quoteAndCast(pgPoint.toString(), "point", standardConformingStrings);
304+
textValue = pgPoint.toString();
305+
type = "point";
306+
break;
290307

291308
case Oid.BOX:
292309
PGbox pgBox = new PGbox();
293310
pgBox.setByteValue((byte[]) paramValue, 0);
294-
return quoteAndCast(pgBox.toString(), "box", standardConformingStrings);
311+
textValue = pgBox.toString();
312+
type = "box";
313+
break;
314+
315+
default:
316+
return "?";
295317
}
296-
return "?";
297318
} else {
298-
String param = paramValue.toString();
319+
textValue = paramValue.toString();
299320
int paramType = paramTypes[index];
300321
if (paramType == Oid.TIMESTAMP) {
301-
return quoteAndCast(param, "timestamp", standardConformingStrings);
322+
type = "timestamp";
302323
} else if (paramType == Oid.TIMESTAMPTZ) {
303-
return quoteAndCast(param, "timestamp with time zone", standardConformingStrings);
324+
type = "timestamp with time zone";
304325
} else if (paramType == Oid.TIME) {
305-
return quoteAndCast(param, "time", standardConformingStrings);
326+
type = "time";
306327
} else if (paramType == Oid.TIMETZ) {
307-
return quoteAndCast(param, "time with time zone", standardConformingStrings);
328+
type = "time with time zone";
308329
} else if (paramType == Oid.DATE) {
309-
return quoteAndCast(param, "date", standardConformingStrings);
330+
type = "date";
310331
} else if (paramType == Oid.INTERVAL) {
311-
return quoteAndCast(param, "interval", standardConformingStrings);
332+
type = "interval";
312333
} else if (paramType == Oid.NUMERIC) {
313-
return quoteAndCast(param, "numeric", standardConformingStrings);
334+
type = "numeric";
335+
} else {
336+
type = null;
314337
}
315-
return quoteAndCast(param, null, standardConformingStrings);
316338
}
339+
return quoteAndCast(textValue, type, standardConformingStrings);
317340
}
318341

319342
@Override

‎pgjdbc/src/test/java/org/postgresql/jdbc/ParameterInjectionTest.java‎

Lines changed: 116 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -12,56 +12,133 @@
1212

1313
import org.junit.jupiter.api.Test;
1414

15+
import java.math.BigDecimal;
1516
import java.sql.Connection;
1617
import java.sql.PreparedStatement;
1718
import java.sql.ResultSet;
19+
import java.sql.SQLException;
1820

1921
public class ParameterInjectionTest {
20-
@Test
21-
public void negateParameter() throws Exception {
22-
try (Connection conn = TestUtil.openDB()) {
23-
PreparedStatement stmt = conn.prepareStatement("SELECT -?");
22+
private interface ParameterBinder {
23+
void bind(PreparedStatement stmt) throws SQLException;
24+
}
2425

25-
stmt.setInt(1, 1);
26-
try (ResultSet rs = stmt.executeQuery()) {
27-
assertTrue(rs.next());
28-
assertEquals(1, rs.getMetaData().getColumnCount(), "number of result columns must match");
29-
int value = rs.getInt(1);
30-
assertEquals(-1, value);
31-
}
26+
private void testParamInjection(ParameterBinder bindPositiveOne, ParameterBinder bindNegativeOne)
27+
throws SQLException {
28+
try (Connection conn = TestUtil.openDB()) {
29+
{
30+
PreparedStatement stmt = conn.prepareStatement("SELECT -?");
31+
bindPositiveOne.bind(stmt);
32+
try (ResultSet rs = stmt.executeQuery()) {
33+
assertTrue(rs.next());
34+
assertEquals(1, rs.getMetaData().getColumnCount(),
35+
"number of result columns must match");
36+
int value = rs.getInt(1);
37+
assertEquals(-1, value);
38+
}
39+
bindNegativeOne.bind(stmt);
40+
try (ResultSet rs = stmt.executeQuery()) {
41+
assertTrue(rs.next());
42+
assertEquals(1, rs.getMetaData().getColumnCount(),
43+
"number of result columns must match");
44+
int value = rs.getInt(1);
45+
assertEquals(1, value);
46+
}
47+
}
48+
{
49+
PreparedStatement stmt = conn.prepareStatement("SELECT -?, ?");
50+
bindPositiveOne.bind(stmt);
51+
stmt.setString(2, "\nWHERE false --");
52+
try (ResultSet rs = stmt.executeQuery()) {
53+
assertTrue(rs.next(), "ResultSet should contain a row");
54+
assertEquals(2, rs.getMetaData().getColumnCount(),
55+
"rs.getMetaData().getColumnCount(");
56+
int value = rs.getInt(1);
57+
assertEquals(-1, value);
58+
}
3259

33-
stmt.setInt(1, -1);
34-
try (ResultSet rs = stmt.executeQuery()) {
35-
assertTrue(rs.next());
36-
assertEquals(1, rs.getMetaData().getColumnCount(), "number of result columns must match");
37-
int value = rs.getInt(1);
38-
assertEquals(1, value);
39-
}
60+
bindNegativeOne.bind(stmt);
61+
stmt.setString(2, "\nWHERE false --");
62+
try (ResultSet rs = stmt.executeQuery()) {
63+
assertTrue(rs.next(), "ResultSet should contain a row");
64+
assertEquals(2, rs.getMetaData().getColumnCount(), "rs.getMetaData().getColumnCount(");
65+
int value = rs.getInt(1);
66+
assertEquals(1, value);
4067
}
68+
69+
}
4170
}
71+
}
4272

43-
@Test
44-
public void negateParameterWithContinuation() throws Exception {
45-
try (Connection conn = TestUtil.openDB()) {
46-
PreparedStatement stmt = conn.prepareStatement("SELECT -?, ?");
73+
@Test
74+
public void handleInt2() throws SQLException {
75+
testParamInjection(
76+
stmt -> {
77+
stmt.setShort(1, (short) 1);
78+
},
79+
stmt -> {
80+
stmt.setShort(1, (short) -1);
81+
}
82+
);
83+
}
4784

48-
stmt.setInt(1, 1);
49-
stmt.setString(2, "\nWHERE false --");
50-
try (ResultSet rs = stmt.executeQuery()) {
51-
assertTrue(rs.next(), "ResultSet should contain a row");
52-
assertEquals(2, rs.getMetaData().getColumnCount(), "rs.getMetaData().getColumnCount(");
53-
int value = rs.getInt(1);
54-
assertEquals(-1, value);
55-
}
85+
@Test
86+
public void handleInt4() throws SQLException {
87+
testParamInjection(
88+
stmt -> {
89+
stmt.setInt(1, 1);
90+
},
91+
stmt -> {
92+
stmt.setInt(1, -1);
93+
}
94+
);
95+
}
5696

57-
stmt.setInt(1, -1);
58-
stmt.setString(2, "\nWHERE false --");
59-
try (ResultSet rs = stmt.executeQuery()) {
60-
assertTrue(rs.next(), "ResultSet should contain a row");
61-
assertEquals(2, rs.getMetaData().getColumnCount(), "rs.getMetaData().getColumnCount(");
62-
int value = rs.getInt(1);
63-
assertEquals(1, value);
64-
}
97+
@Test
98+
public void handleBigInt() throws SQLException {
99+
testParamInjection(
100+
stmt -> {
101+
stmt.setLong(1, (long) 1);
102+
},
103+
stmt -> {
104+
stmt.setLong(1, (long) -1);
65105
}
66-
}
106+
);
107+
}
108+
109+
@Test
110+
public void handleNumeric() throws SQLException {
111+
testParamInjection(
112+
stmt -> {
113+
stmt.setBigDecimal(1, new BigDecimal("1"));
114+
},
115+
stmt -> {
116+
stmt.setBigDecimal(1, new BigDecimal("-1"));
117+
}
118+
);
119+
}
120+
121+
@Test
122+
public void handleFloat() throws SQLException {
123+
testParamInjection(
124+
stmt -> {
125+
stmt.setFloat(1, 1);
126+
},
127+
stmt -> {
128+
stmt.setFloat(1, -1);
129+
}
130+
);
131+
}
132+
133+
@Test
134+
public void handleDouble() throws SQLException {
135+
testParamInjection(
136+
stmt -> {
137+
stmt.setDouble(1, 1);
138+
},
139+
stmt -> {
140+
stmt.setDouble(1, -1);
141+
}
142+
);
143+
}
67144
}

0 commit comments

Comments
 (0)