Skip to content

Commit eca6c90

Browse files
vedant-jaiswalVedant Jaiswal
vedant-jaiswal
and
Vedant Jaiswal
authored
Validate secret fields before JDBC URL construction (#334)
## Description ### Why is this change being made? 1. To prevent URL injection attacks (CWE-610) where malicious characters in secret fields could be used to manipulate JDBC URLs and bypass security controls or redirect connections to unintended databases. ### What is changing? 1. Added `validateSecretFields()` method in `AWSSecretsManagerDriver` that validates the `host`, `port`, and `dbname` fields from secrets before URL construction. ### Related Links - **Issue #, if available**: N/A --- ## Testing ### How was this tested? 1. Added new test methods for this change. ### When testing locally, provide testing artifact(s): 1. `mvn clean test` `Tests run: 155, Failures: 0, Errors: 0, Skipped: 0` --- ## Reviewee Checklist **Update the checklist after submitting the PR** - [x] I have reviewed, tested and understand all changes *If not, why:* - [x] I have filled out the Description and Testing sections above *If not, why:* - [x] Build and Unit tests are passing *If not, why:* - [x] Unit test coverage check is passing *If not, why:* - [x] I have ensured no sensitive information is leaking (i.e., no logging of sensitive fields, or otherwise) *If not, why:* - [x] I have added explanatory comments for complex logic, new classes/methods and new tests *If not, why:* - [ ] I have updated README/documentation (if needed) *If not, why:* - [ ] I have clearly called out breaking changes (if any) *If not, why:* --- ## Reviewer Checklist **All reviewers please ensure the following are true before reviewing:** - Reviewee checklist has been accurately filled out - Code changes align with stated purpose in description - Test coverage adequately validates the changes --- By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Co-authored-by: Vedant Jaiswal <vedjasy@amazon.com>
1 parent 62bfcd9 commit eca6c90

2 files changed

Lines changed: 130 additions & 0 deletions

File tree

‎src/main/java/com/amazonaws/secretsmanager/sql/AWSSecretsManagerDriver.java‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import java.util.Enumeration;
2323
import java.util.Properties;
2424
import java.util.logging.Logger;
25+
import java.util.regex.Pattern;
2526

2627
import com.amazonaws.secretsmanager.caching.SecretCache;
2728
import com.amazonaws.secretsmanager.caching.SecretCacheConfiguration;
@@ -108,6 +109,10 @@ public abstract class AWSSecretsManagerDriver implements Driver {
108109
*/
109110
public static final String INVALID_SECRET_STRING_JSON = "Could not parse SecretString JSON";
110111

112+
private static final Pattern INVALID_HOST_PATTERN = Pattern.compile(".*[?#&;/@\\\\\\s].*");
113+
private static final Pattern INVALID_DBNAME_PATTERN = Pattern.compile(".*[?#&;\\\\\\s].*");
114+
private static final Pattern DIGITS_ONLY_PATTERN = Pattern.compile("\\d+");
115+
111116
private SecretCache secretCache;
112117

113118
private String realDriverClass;
@@ -377,6 +382,26 @@ private Connection connectWithSecret(String unwrappedUrl, Properties info, Strin
377382
throw new SQLException("Connect failed to authenticate: reached max connection retries");
378383
}
379384

385+
/**
386+
* Validates that secret fields do not contain characters that could be used for URL injection.
387+
* Prevents CWE-610 (Externally Controlled Reference to a Resource in Another Sphere).
388+
*/
389+
private void validateSecretFields(String endpoint, String port, String dbname) throws SQLException {
390+
if (endpoint == null || endpoint.isEmpty()) {
391+
throw new SQLException("Secret 'host' field must not be null or empty.");
392+
}
393+
if (INVALID_HOST_PATTERN.matcher(endpoint).matches()) {
394+
throw new SQLException("Secret 'host' field contains invalid characters. "
395+
+ "A valid host must be a hostname or IP address without URL special characters.");
396+
}
397+
if (port != null && !port.isEmpty() && !DIGITS_ONLY_PATTERN.matcher(port).matches()) {
398+
throw new SQLException("Secret 'port' field must contain only digits.");
399+
}
400+
if (dbname != null && !dbname.isEmpty() && INVALID_DBNAME_PATTERN.matcher(dbname).matches()) {
401+
throw new SQLException("Secret 'dbname' field contains invalid characters.");
402+
}
403+
}
404+
380405
@Override
381406
@SuppressFBWarnings("THROWS_METHOD_THROWS_RUNTIMEEXCEPTION")
382407
public Connection connect(String url, Properties info) throws SQLException {
@@ -400,6 +425,7 @@ public Connection connect(String url, Properties info) throws SQLException {
400425
String port = portNode == null ? null : portNode.asText();
401426
JsonNode dbnameNode = jsonObject.get("dbname");
402427
String dbname = dbnameNode == null ? null : dbnameNode.asText();
428+
validateSecretFields(endpoint, port, dbname);
403429
unwrappedUrl = constructUrlFromEndpointPortDatabase(endpoint, port, dbname);
404430
} catch (IOException e) {
405431
// Most likely to occur in the event that the data is not JSON.

‎src/test/java/com/amazonaws/secretsmanager/sql/AWSSecretsManagerDriverTest.java‎

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -356,4 +356,108 @@ public void test_jdbcCompliant_propagatesToRealDriver() {
356356
assertEquals(true, sut.jdbcCompliant());
357357
assertEquals(1, DummyDriver.jdbcCompliantCallCount);
358358
}
359+
360+
/*******************************************************************************************************************
361+
* validateSecretFields Tests (URL Injection Prevention)
362+
******************************************************************************************************************/
363+
364+
@Test
365+
public void test_connect_throws_hostWithFragment() {
366+
Mockito.when(cache.getSecretString("MALICIOUS_HOST")).thenReturn(
367+
"{\"username\": \"user\", \"password\": \"pass\", \"host\": \"evil.com:3306/x?allowLoadLocalInfile=true#\", \"port\": \"3306\", \"dbname\": \"test\"}");
368+
Properties props = new Properties();
369+
props.setProperty("user", "user");
370+
assertThrows(SQLException.class, () -> sut.connect("MALICIOUS_HOST", props));
371+
assertEquals(0, DummyDriver.connectCallCount);
372+
}
373+
374+
@Test
375+
public void test_connect_throws_hostWithQuestionMark() {
376+
Mockito.when(cache.getSecretString("MALICIOUS_HOST_Q")).thenReturn(
377+
"{\"username\": \"user\", \"password\": \"pass\", \"host\": \"evil.com?param=val\", \"port\": \"3306\", \"dbname\": \"test\"}");
378+
Properties props = new Properties();
379+
props.setProperty("user", "user");
380+
assertThrows(SQLException.class, () -> sut.connect("MALICIOUS_HOST_Q", props));
381+
assertEquals(0, DummyDriver.connectCallCount);
382+
}
383+
384+
@Test
385+
public void test_connect_throws_hostWithSlash() {
386+
Mockito.when(cache.getSecretString("MALICIOUS_HOST_SLASH")).thenReturn(
387+
"{\"username\": \"user\", \"password\": \"pass\", \"host\": \"evil.com/path\", \"port\": \"3306\", \"dbname\": \"test\"}");
388+
Properties props = new Properties();
389+
props.setProperty("user", "user");
390+
assertThrows(SQLException.class, () -> sut.connect("MALICIOUS_HOST_SLASH", props));
391+
assertEquals(0, DummyDriver.connectCallCount);
392+
}
393+
394+
@Test
395+
public void test_connect_throws_hostWithAtSign() {
396+
Mockito.when(cache.getSecretString("MALICIOUS_HOST_AT")).thenReturn(
397+
"{\"username\": \"user\", \"password\": \"pass\", \"host\": \"user@evil.com\", \"port\": \"3306\", \"dbname\": \"test\"}");
398+
Properties props = new Properties();
399+
props.setProperty("user", "user");
400+
assertThrows(SQLException.class, () -> sut.connect("MALICIOUS_HOST_AT", props));
401+
assertEquals(0, DummyDriver.connectCallCount);
402+
}
403+
404+
@Test
405+
public void test_connect_throws_nonNumericPort() {
406+
Mockito.when(cache.getSecretString("BAD_PORT")).thenReturn(
407+
"{\"username\": \"user\", \"password\": \"pass\", \"host\": \"valid.host.com\", \"port\": \"3306;inject\", \"dbname\": \"test\"}");
408+
Properties props = new Properties();
409+
props.setProperty("user", "user");
410+
assertThrows(SQLException.class, () -> sut.connect("BAD_PORT", props));
411+
assertEquals(0, DummyDriver.connectCallCount);
412+
}
413+
414+
@Test
415+
public void test_connect_throws_dbnameWithFragment() {
416+
Mockito.when(cache.getSecretString("BAD_DBNAME")).thenReturn(
417+
"{\"username\": \"user\", \"password\": \"pass\", \"host\": \"valid.host.com\", \"port\": \"3306\", \"dbname\": \"db#fragment\"}");
418+
Properties props = new Properties();
419+
props.setProperty("user", "user");
420+
assertThrows(SQLException.class, () -> sut.connect("BAD_DBNAME", props));
421+
assertEquals(0, DummyDriver.connectCallCount);
422+
}
423+
424+
@Test
425+
public void test_connect_works_validHostPortDbname() {
426+
Mockito.when(cache.getSecretString("VALID_SECRET")).thenReturn(
427+
"{\"username\": \"user\", \"password\": \"pass\", \"host\": \"mydb.us-east-1.rds.amazonaws.com\", \"port\": \"5432\", \"dbname\": \"prod\"}");
428+
Properties props = new Properties();
429+
props.setProperty("user", "user");
430+
assertNotThrows(() -> sut.connect("VALID_SECRET", props));
431+
assertEquals(1, DummyDriver.connectCallCount);
432+
}
433+
434+
@Test
435+
public void test_connect_works_nullPortAndDbname() {
436+
Mockito.when(cache.getSecretString("MINIMAL_SECRET")).thenReturn(
437+
"{\"username\": \"user\", \"password\": \"pass\", \"host\": \"localhost\"}");
438+
Properties props = new Properties();
439+
props.setProperty("user", "user");
440+
assertNotThrows(() -> sut.connect("MINIMAL_SECRET", props));
441+
assertEquals(1, DummyDriver.connectCallCount);
442+
}
443+
444+
@Test
445+
public void test_connect_throws_emptyHost() {
446+
Mockito.when(cache.getSecretString("EMPTY_HOST")).thenReturn(
447+
"{\"username\": \"user\", \"password\": \"pass\", \"host\": \"\", \"port\": \"3306\", \"dbname\": \"test\"}");
448+
Properties props = new Properties();
449+
props.setProperty("user", "user");
450+
assertThrows(SQLException.class, () -> sut.connect("EMPTY_HOST", props));
451+
assertEquals(0, DummyDriver.connectCallCount);
452+
}
453+
454+
@Test
455+
public void test_connect_works_emptyPortAndDbname() {
456+
Mockito.when(cache.getSecretString("EMPTY_OPTIONAL")).thenReturn(
457+
"{\"username\": \"user\", \"password\": \"pass\", \"host\": \"valid.host.com\", \"port\": \"\", \"dbname\": \"\"}");
458+
Properties props = new Properties();
459+
props.setProperty("user", "user");
460+
assertNotThrows(() -> sut.connect("EMPTY_OPTIONAL", props));
461+
assertEquals(1, DummyDriver.connectCallCount);
462+
}
359463
}

0 commit comments

Comments
 (0)