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/03/28 23:54:44 UTC
[GitHub] [fineract] taskain7 opened a new pull request #2205: FINERACT-1483: Fix sonar vulnerabilities with Blocker and Critical severity
taskain7 opened a new pull request #2205:
URL: https://github.com/apache/fineract/pull/2205
## Description
Owasp library has been added to validate dynamic SQL queries.
Dynamic SQL queries have been changed to Prepared Statement
Some Sonar findings have been tagged with `//NOSONAR` so let Sonar skip them in the future
--
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
[GitHub] [fineract] galovics commented on a change in pull request #2205: FINERACT-1483: Fix sonar vulnerabilities with Blocker and Critical severity
Posted by GitBox <gi...@apache.org>.
galovics commented on a change in pull request #2205:
URL: https://github.com/apache/fineract/pull/2205#discussion_r837090141
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/service/PaginationHelper.java
##########
@@ -41,15 +43,17 @@ public PaginationHelper(DatabaseSpecificSQLGenerator sqlGenerator, DatabaseTypeR
public <E> Page<E> fetchPage(final JdbcTemplate jt, final String sqlFetchRows, final Object[] args, final RowMapper<E> rowMapper) {
- final List<E> items = jt.query(sqlFetchRows, rowMapper, args);
+ final String validatedSqlFetchRows = ESAPI.encoder().encodeForSQL(new MySQLCodec(MySQLCodec.Mode.STANDARD), sqlFetchRows);
Review comment:
This is probably not the best idea here.
On one hand, let's not duplicate this logic everywhere (getting an encoder and passing a MySQL codec everywhere) but let's extract this to somewhere.
Plus, on the other hand, this codec is encoding for MySQL only while Fineract supports PostgreSQL as well. The encoding could simply result in incorrectly formatted SQL strings which is not what we want.
As far as I can tell the OWASP library doesn't have a PostgreSQL specific codec but another thing I quickly found was this: https://stackoverflow.com/questions/43645748/is-using-org-postgresql-core-utils-escapeliteral-enough-to-prevent-sql-injection
Take a look please. Thanks!
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadWriteNonCoreDataServiceImpl.java
##########
@@ -368,10 +376,10 @@ public void deregisterDatatable(final String datatable) {
final String deleteFromConfigurationSql = "delete from c_configuration where name ='" + datatable + "'";
String[] sqlArray = new String[4];
- sqlArray[0] = deleteRolePermissionsSql;
- sqlArray[1] = deletePermissionsSql;
- sqlArray[2] = deleteRegisteredDatatableSql;
- sqlArray[3] = deleteFromConfigurationSql;
+ sqlArray[0] = ESAPI.encoder().encodeForSQL(new MySQLCodec(MySQLCodec.Mode.STANDARD), deleteRolePermissionsSql);
Review comment:
Same deal as above.
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadWriteNonCoreDataServiceImpl.java
##########
@@ -187,21 +190,27 @@ public DatatableData retrieveDatatable(final String datatable) {
final String sql = "select application_table_name, registered_table_name, entity_subtype" + " from x_registered_table "
+ " where exists" + " (select 'f'" + " from m_appuser_role ur " + " join m_role r on r.id = ur.role_id"
+ " left join m_role_permission rp on rp.role_id = r.id" + " left join m_permission p on p.id = rp.permission_id"
- + " where ur.appuser_id = " + this.context.authenticatedUser().getId() + " and registered_table_name='" + datatable + "'"
- + " and (p.code in ('ALL_FUNCTIONS', 'ALL_FUNCTIONS_READ') or p.code = concat('READ_', registered_table_name))) "
+ + " where ur.appuser_id = ? and registered_table_name=? and (p.code in ('ALL_FUNCTIONS', "
+ + "'ALL_FUNCTIONS_READ') or p.code = concat('READ_', registered_table_name))) "
+ " order by application_table_name, registered_table_name";
- final SqlRowSet rs = this.jdbcTemplate.queryForRowSet(sql);
-
DatatableData datatableData = null;
- while (rs.next()) {
- final String appTableName = rs.getString("application_table_name");
- final String registeredDatatableName = rs.getString("registered_table_name");
- final String entitySubType = rs.getString("entity_subtype");
- final List<ResultsetColumnHeaderData> columnHeaderData = this.genericDataService
- .fillResultsetColumnHeaders(registeredDatatableName);
-
- datatableData = DatatableData.create(appTableName, registeredDatatableName, entitySubType, columnHeaderData);
+ try (Connection connection = jdbcTemplate.getDataSource().getConnection()) {
Review comment:
Same as above.
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java
##########
@@ -248,7 +248,7 @@ public String retrieveReportPDF(final String reportName, final String type, fina
final Document document = new Document(PageSize.B0.rotate());
- PdfWriter.getInstance(document, new FileOutputStream(new File(fileLocation + reportName + ".pdf")));
+ PdfWriter.getInstance(document, new FileOutputStream(fileLocation + reportName + ".pdf")); // NOSONAR
Review comment:
Wouldn't the ESAPI UnixCodec help here? https://www.javadoc.io/doc/org.owasp.esapi/esapi/2.1.0.1/org/owasp/esapi/codecs/UnixCodec.html
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadWriteNonCoreDataServiceImpl.java
##########
@@ -146,34 +152,31 @@ public ReadWriteNonCoreDataServiceImpl(final JdbcTemplate jdbcTemplate, final Na
@Override
public List<DatatableData> retrieveDatatableNames(final String appTable) {
- String andClause;
- if (appTable == null) {
- andClause = "";
- } else {
- validateAppTable(appTable);
- SQLInjectionValidator.validateSQLInput(appTable);
- andClause = " and application_table_name = '" + appTable + "'";
- }
-
// PERMITTED datatables
final String sql = "select application_table_name, registered_table_name, entity_subtype " + " from x_registered_table "
+ " where exists" + " (select 'f'" + " from m_appuser_role ur " + " join m_role r on r.id = ur.role_id"
+ " left join m_role_permission rp on rp.role_id = r.id" + " left join m_permission p on p.id = rp.permission_id"
- + " where ur.appuser_id = " + this.context.authenticatedUser().getId()
- + " and (p.code in ('ALL_FUNCTIONS', 'ALL_FUNCTIONS_READ') or p.code = concat('READ_', registered_table_name))) "
- + andClause + " order by application_table_name, registered_table_name";
-
- final SqlRowSet rs = this.jdbcTemplate.queryForRowSet(sql);
+ + " where ur.appuser_id = ? and (p.code in ('ALL_FUNCTIONS', 'ALL_FUNCTIONS_READ') or p.code = concat"
+ + "('READ_', registered_table_name))) "
+ + " and application_table_name like ? order by application_table_name, registered_table_name";
final List<DatatableData> datatables = new ArrayList<>();
- while (rs.next()) {
- final String appTableName = rs.getString("application_table_name");
- final String registeredDatatableName = rs.getString("registered_table_name");
- final String entitySubType = rs.getString("entity_subtype");
- final List<ResultsetColumnHeaderData> columnHeaderData = this.genericDataService
- .fillResultsetColumnHeaders(registeredDatatableName);
-
- datatables.add(DatatableData.create(appTableName, registeredDatatableName, entitySubType, columnHeaderData));
+ try (Connection connection = jdbcTemplate.getDataSource().getConnection()) {
Review comment:
Question, why do we want to use a plain connection here instead of executing the PreparedStatement through the JdbcTemplate itself since internally it creates its own?
--
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
[GitHub] [fineract] vidakovic commented on pull request #2205: FINERACT-1483: Fix sonar vulnerabilities with Blocker and Critical severity
Posted by GitBox <gi...@apache.org>.
vidakovic commented on pull request #2205:
URL: https://github.com/apache/fineract/pull/2205#issuecomment-1081403741
@taskain7 ... the fix here should be relatively simple: it seems that there is a transitive dependency on log4j. The only thing to do here is to define an exclude on the library that drags it in (we are using Slf4j... which is why this check is triggered).
Running:
```
./gradlew :fineract-provider:dependencies > /tmp/dependencies.txt
```
should give you a quick way to find the library that includes log4j.
And if you want to test locally if everything is good just execute this test:
```
resources/features/infrastructure/infrastructure.classpath.feature
```
--
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
[GitHub] [fineract] galovics commented on a change in pull request #2205: FINERACT-1483: Fix sonar vulnerabilities with Blocker and Critical severity
Posted by GitBox <gi...@apache.org>.
galovics commented on a change in pull request #2205:
URL: https://github.com/apache/fineract/pull/2205#discussion_r838695671
##########
File path: fineract-provider/src/main/resources/esapi-java-logging.properties
##########
@@ -0,0 +1,24 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+handlers= java.util.logging.ConsoleHandler
Review comment:
Can't we hook it up with SLF4J?
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java
##########
@@ -248,7 +250,8 @@ public String retrieveReportPDF(final String reportName, final String type, fina
final Document document = new Document(PageSize.B0.rotate());
- PdfWriter.getInstance(document, new FileOutputStream(new File(fileLocation + reportName + ".pdf")));
+ String validatedFileLocation = ESAPI.encoder().encodeForOS(new UnixCodec(), fileLocation + reportName + ".pdf");
Review comment:
Have you been able to test this? I wanna make sure we are not breaking anything.
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/service/PaginationHelper.java
##########
@@ -41,18 +43,32 @@ public PaginationHelper(DatabaseSpecificSQLGenerator sqlGenerator, DatabaseTypeR
public <E> Page<E> fetchPage(final JdbcTemplate jt, final String sqlFetchRows, final Object[] args, final RowMapper<E> rowMapper) {
- final List<E> items = jt.query(sqlFetchRows, rowMapper, args);
+ final List<E> items = jt.query(connection -> {
+ final PreparedStatement preparedStatement = connection.prepareStatement(sqlFetchRows);
Review comment:
Why do we need to construct the prepared statement manually? We could use the jdbcTemplate's query method with pure SQL strings. Internally that'll be turned into prepared statements.
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/security/utils/EsapiUtils.java
##########
@@ -0,0 +1,49 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.fineract.infrastructure.security.utils;
+
+import java.sql.SQLException;
+import org.apache.fineract.infrastructure.core.service.database.DatabaseTypeResolver;
+import org.owasp.esapi.ESAPI;
+import org.owasp.esapi.codecs.Codec;
+import org.owasp.esapi.codecs.MySQLCodec;
+import org.postgresql.core.Utils;
+
+public final class EsapiUtils {
Review comment:
Also, I'd probably rename it and not call it EsapiUtils since the PostgreSQL encoding doesn't involve ESAPI.
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/security/utils/EsapiUtils.java
##########
@@ -0,0 +1,49 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.fineract.infrastructure.security.utils;
+
+import java.sql.SQLException;
+import org.apache.fineract.infrastructure.core.service.database.DatabaseTypeResolver;
+import org.owasp.esapi.ESAPI;
+import org.owasp.esapi.codecs.Codec;
+import org.owasp.esapi.codecs.MySQLCodec;
+import org.postgresql.core.Utils;
+
+public final class EsapiUtils {
+
+ private static final Codec<?> MYSQL_CODEC = new MySQLCodec(MySQLCodec.Mode.STANDARD);
+
+ private EsapiUtils() {
+ throw new IllegalStateException("Utility class");
+ }
+
+ public static String encodeSql(String literal, DatabaseTypeResolver databaseTypeResolver) {
+ if (databaseTypeResolver.isMySQL()) {
+ return ESAPI.encoder().encodeForSQL(MYSQL_CODEC, literal);
+ } else if (databaseTypeResolver.isPostgreSQL()) {
+ try {
+ return Utils.escapeLiteral(null, literal, true).toString();
+ } catch (SQLException e) {
+ return literal;
Review comment:
You sure? If there's an exception, that should indicate some error case, wouldn't it? Probably not the best idea to swallow here. What do you think?
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/security/utils/EsapiUtils.java
##########
@@ -0,0 +1,49 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.fineract.infrastructure.security.utils;
+
+import java.sql.SQLException;
+import org.apache.fineract.infrastructure.core.service.database.DatabaseTypeResolver;
+import org.owasp.esapi.ESAPI;
+import org.owasp.esapi.codecs.Codec;
+import org.owasp.esapi.codecs.MySQLCodec;
+import org.postgresql.core.Utils;
+
+public final class EsapiUtils {
Review comment:
I mean since this class needs the DatabaseTypeResolver bean anyway, why don't we just make this a Spring Bean as well so we can simply autowire it instead of passing it over and over again for each invocation. Thoughts?
--
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