You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2022/09/30 18:21:20 UTC
[GitHub] [fineract] ezolnbl-dpc commented on a diff in pull request #2624: FINERACT-1747: add generic datatable query
ezolnbl-dpc commented on code in PR #2624:
URL: https://github.com/apache/fineract/pull/2624#discussion_r984795760
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/security/utils/SQLInjectionValidator.java:
##########
@@ -60,99 +60,72 @@ public static void validateSQLInput(final String sqlSearch) {
}
}
- // Removing the space before and after '=' operator
- // String s = " \" OR 1 = 1"; For the cases like this
- boolean injectionFound = false;
- String inputSqlString = lowerCaseSQL;
- while (inputSqlString.indexOf(" =") > 0) { // Don't remove space before
- // = operator
- inputSqlString = inputSqlString.replaceAll(" =", "=");
- }
+ patternMatchSqlInjection(sqlSearch, lowerCaseSQL);
+ }
- while (inputSqlString.indexOf("= ") > 0) { // Don't remove space after =
- // operator
- inputSqlString = inputSqlString.replaceAll("= ", "=");
+ public static void validateAdhocQuery(final String sqlSearch) {
+ if (StringUtils.isBlank(sqlSearch)) {
+ return;
}
-
- StringTokenizer tokenizer = new StringTokenizer(inputSqlString, " ");
- while (tokenizer.hasMoreTokens()) {
- String token = tokenizer.nextToken().trim();
- if (token.equals("'")) {
- if (tokenizer.hasMoreElements()) {
- String nextToken = tokenizer.nextToken().trim();
- if (!nextToken.equals("'")) {
- injectionFound = true;
- break;
- }
- } else {
- injectionFound = true;
- break;
- }
- }
- if (token.equals("\"")) {
- if (tokenizer.hasMoreElements()) {
- String nextToken = tokenizer.nextToken().trim();
- if (!nextToken.equals("\"")) {
- injectionFound = true;
- break;
- }
- } else {
- injectionFound = true;
- break;
- }
- } else if (token.indexOf('=') > 0) {
- StringTokenizer operatorToken = new StringTokenizer(token, "=");
- String operand = operatorToken.nextToken().trim();
- if (!operatorToken.hasMoreTokens()) {
- injectionFound = true;
- break;
- }
- String value = operatorToken.nextToken().trim();
- if (operand.equals(value)) {
- injectionFound = true;
- break;
- }
+ String lowerCaseSQL = sqlSearch.toLowerCase().trim();
+ for (String ddl : DDL_COMMANDS) {
+ if (lowerCaseSQL.startsWith(ddl)) {
+ throw new SQLInjectionException();
}
}
- if (injectionFound) {
- throw new SQLInjectionException();
- }
- Pattern pattern = Pattern.compile(SQL_PATTERN);
- Matcher matcher = pattern.matcher(sqlSearch);
- if (!matcher.matches()) {
- throw new SQLInjectionException();
+ for (String comments : COMMENTS) {
+ if (lowerCaseSQL.contains(comments)) {
+ throw new SQLInjectionException();
+ }
}
+
+ // Removing the space before and after '=' operator
+ // String s = " \" OR 1 = 1"; For the cases like this
+ patternMatchSqlInjection(sqlSearch, lowerCaseSQL);
}
- public static void validateAdhocQuery(final String sqlSearch) {
+ public static void validateDynamicQuery(final String sqlSearch) {
Review Comment:
both sql validator method is bad for this case because they either use "contains" or "startsWith" and the filters can use the preserved words at any place, I didn't want regression that's why I created a new method
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadWriteNonCoreDataServiceImpl.java:
##########
@@ -183,6 +185,44 @@ public DatatableData retrieveDatatable(final String datatable) {
return datatableData;
}
+ @Override
+ public List<JsonObject> queryDataTable(String datatable, String columnFilter, String valueFilter, String resultColumns) {
+ Arrays.asList(datatable, columnFilter, valueFilter, resultColumns).forEach(SQLInjectionValidator::validateDynamicQuery);
+ if(columnFilter == null || valueFilter == null) {
Review Comment:
changing the code accordingly
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadWriteNonCoreDataServiceImpl.java:
##########
@@ -183,6 +185,44 @@ public DatatableData retrieveDatatable(final String datatable) {
return datatableData;
}
+ @Override
+ public List<JsonObject> queryDataTable(String datatable, String columnFilter, String valueFilter, String resultColumns) {
+ Arrays.asList(datatable, columnFilter, valueFilter, resultColumns).forEach(SQLInjectionValidator::validateDynamicQuery);
+ if(columnFilter == null || valueFilter == null) {
+ return new ArrayList<>(0);
+ }
+
+ List<String> resultColumnNames = resultColumns == null ? Collections.emptyList() : Stream.of(resultColumns.split(",")).toList();
+ final String sql = "select " + (resultColumns == null ? "" : resultColumns.trim()) + " from " + datatable + " where "
Review Comment:
changing the code accordingly
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/security/utils/SQLInjectionValidator.java:
##########
@@ -60,99 +60,72 @@ public static void validateSQLInput(final String sqlSearch) {
}
}
- // Removing the space before and after '=' operator
- // String s = " \" OR 1 = 1"; For the cases like this
- boolean injectionFound = false;
- String inputSqlString = lowerCaseSQL;
- while (inputSqlString.indexOf(" =") > 0) { // Don't remove space before
- // = operator
- inputSqlString = inputSqlString.replaceAll(" =", "=");
- }
+ patternMatchSqlInjection(sqlSearch, lowerCaseSQL);
+ }
- while (inputSqlString.indexOf("= ") > 0) { // Don't remove space after =
- // operator
- inputSqlString = inputSqlString.replaceAll("= ", "=");
+ public static void validateAdhocQuery(final String sqlSearch) {
+ if (StringUtils.isBlank(sqlSearch)) {
+ return;
}
-
- StringTokenizer tokenizer = new StringTokenizer(inputSqlString, " ");
- while (tokenizer.hasMoreTokens()) {
- String token = tokenizer.nextToken().trim();
- if (token.equals("'")) {
- if (tokenizer.hasMoreElements()) {
- String nextToken = tokenizer.nextToken().trim();
- if (!nextToken.equals("'")) {
- injectionFound = true;
- break;
- }
- } else {
- injectionFound = true;
- break;
- }
- }
- if (token.equals("\"")) {
- if (tokenizer.hasMoreElements()) {
- String nextToken = tokenizer.nextToken().trim();
- if (!nextToken.equals("\"")) {
- injectionFound = true;
- break;
- }
- } else {
- injectionFound = true;
- break;
- }
- } else if (token.indexOf('=') > 0) {
- StringTokenizer operatorToken = new StringTokenizer(token, "=");
- String operand = operatorToken.nextToken().trim();
- if (!operatorToken.hasMoreTokens()) {
- injectionFound = true;
- break;
- }
- String value = operatorToken.nextToken().trim();
- if (operand.equals(value)) {
- injectionFound = true;
- break;
- }
+ String lowerCaseSQL = sqlSearch.toLowerCase().trim();
+ for (String ddl : DDL_COMMANDS) {
+ if (lowerCaseSQL.startsWith(ddl)) {
+ throw new SQLInjectionException();
}
}
- if (injectionFound) {
- throw new SQLInjectionException();
- }
- Pattern pattern = Pattern.compile(SQL_PATTERN);
- Matcher matcher = pattern.matcher(sqlSearch);
- if (!matcher.matches()) {
- throw new SQLInjectionException();
+ for (String comments : COMMENTS) {
+ if (lowerCaseSQL.contains(comments)) {
+ throw new SQLInjectionException();
+ }
}
+
+ // Removing the space before and after '=' operator
+ // String s = " \" OR 1 = 1"; For the cases like this
+ patternMatchSqlInjection(sqlSearch, lowerCaseSQL);
}
- public static void validateAdhocQuery(final String sqlSearch) {
+ public static void validateDynamicQuery(final String sqlSearch) {
if (StringUtils.isBlank(sqlSearch)) {
return;
}
- String lowerCaseSQL = sqlSearch.toLowerCase().trim();
+
+ String lowerCaseSQL = sqlSearch.toLowerCase();
for (String ddl : DDL_COMMANDS) {
- if (lowerCaseSQL.startsWith(ddl)) {
+ if (ddl.equals(lowerCaseSQL)) {
Review Comment:
see previous comment, I did not change the rest of the pattern based logic and that will catch this type of problem, please run this testcase:
@Test
public void testSqlInjectionCaughtQueryDataTable2() {
Assertions.assertThrows(SQLInjectionException.class, () -> {
underTest.queryDataTable("table", "cf1", "vf1", "1; DROP TABLE m_loan; SELECT");
});
}
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org