Skip to content

Commit

Permalink
Fix some reporting issues including SQLi vulnerabilities (FINERACT-854)
Browse files Browse the repository at this point in the history
  • Loading branch information
@josemakara2 @ptuomola
josemakara2 authored and ptuomola committed Apr 12, 2021
1 parent 63e4273 commit b3d0025
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 25 deletions.
10 changes: 9 additions & 1 deletion ...c/main/java/org/apache/fineract/infrastructure/dataqueries/api/RunreportsApiResource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import javax.ws.rs.core.UriInfo;
import org.apache.fineract.infrastructure.core.api.ApiParameterHelper;
import org.apache.fineract.infrastructure.core.exception.PlatformServiceUnavailableException;
import org.apache.fineract.infrastructure.dataqueries.service.DatatableReportingProcessService;
import org.apache.fineract.infrastructure.dataqueries.service.ReadReportingService;
import org.apache.fineract.infrastructure.report.provider.ReportingProcessServiceProvider;
import org.apache.fineract.infrastructure.report.service.ReportingProcessService;
Expand All @@ -60,13 +61,16 @@ public class RunreportsApiResource {
private final PlatformSecurityContext context;
private final ReadReportingService readExtraDataAndReportingService;
private final ReportingProcessServiceProvider reportingProcessServiceProvider;
private final DatatableReportingProcessService datatableReportingProcessService;

@Autowired
public RunreportsApiResource(final PlatformSecurityContext context, final ReadReportingService readExtraDataAndReportingService,
final ReportingProcessServiceProvider reportingProcessServiceProvider) {
final ReportingProcessServiceProvider reportingProcessServiceProvider,
DatatableReportingProcessService aDatatableReportingProcessService) {
this.context = context;
this.readExtraDataAndReportingService = readExtraDataAndReportingService;
this.reportingProcessServiceProvider = reportingProcessServiceProvider;
datatableReportingProcessService = aDatatableReportingProcessService;
}

@GET
Expand Down Expand Up @@ -105,6 +109,10 @@ public Response runReport(@PathParam("reportName") @Parameter(description = "rep
// Pass through isSelfServiceUserReport so that ReportingProcessService implementations can use it
queryParams.putSingle(IS_SELF_SERVICE_USER_REPORT_PARAMETER, Boolean.toString(isSelfServiceUserReport));

if (parameterType) {
return datatableReportingProcessService.processRequest(reportName, queryParams);
}

String reportType = this.readExtraDataAndReportingService.getReportType(reportName, isSelfServiceUserReport);
ReportingProcessService reportingProcessService = this.reportingProcessServiceProvider.findReportingProcessService(reportType);
if (reportingProcessService == null) {
Expand Down
32 changes: 8 additions & 24 deletions ...java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
import java.util.Set;
import javax.sql.DataSource;
import javax.ws.rs.core.StreamingOutput;
import org.apache.commons.lang3.StringUtils;
import org.apache.fineract.infrastructure.core.domain.JdbcSupport;
import org.apache.fineract.infrastructure.core.exception.PlatformDataIntegrityException;
import org.apache.fineract.infrastructure.core.service.RoutingDataSource;
Expand All @@ -51,8 +50,6 @@
import org.apache.fineract.infrastructure.dataqueries.exception.ReportNotFoundException;
import org.apache.fineract.infrastructure.documentmanagement.contentrepository.FileSystemContentRepository;
import org.apache.fineract.infrastructure.security.service.PlatformSecurityContext;
import org.apache.fineract.infrastructure.security.utils.ColumnValidator;
import org.apache.fineract.infrastructure.security.utils.SQLInjectionException;
import org.apache.fineract.useradministration.domain.AppUser;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand All @@ -66,22 +63,19 @@
public class ReadReportingServiceImpl implements ReadReportingService {

private static final Logger LOG = LoggerFactory.getLogger(ReadReportingServiceImpl.class);
private static final String REPORT_NAME_REGEX_PATTERN = "^[a-zA-Z][a-zA-Z0-9\\-_\\s]{0,48}[a-zA-Z0-9\\s](\\([a-zA-Z]*\\))?$";

private final JdbcTemplate jdbcTemplate;
private final DataSource dataSource;
private final PlatformSecurityContext context;
private final GenericDataService genericDataService;
private final ColumnValidator columnValidator;

@Autowired
public ReadReportingServiceImpl(final PlatformSecurityContext context, final RoutingDataSource dataSource,
final GenericDataService genericDataService, final ColumnValidator columnValidator) {
final GenericDataService genericDataService) {
this.context = context;
this.dataSource = dataSource;
this.jdbcTemplate = new JdbcTemplate(this.dataSource);
this.genericDataService = genericDataService;
this.columnValidator = columnValidator;
}

@Override
Expand Down Expand Up @@ -204,13 +198,12 @@ private String getSQLtoRun(final String name, final String type, final Map<Strin
}

private String getSql(final String name, final String type) {
final String inputSql = "select " + type + "_sql as the_sql from stretchy_" + type + " where " + type + "_name = '" + name + "'";
validateReportName(name);
final String inputSql = "select " + type + "_sql as the_sql from stretchy_" + type + " where " + type + "_name = ?";

final String inputSqlWrapped = this.genericDataService.wrapSQL(inputSql);

// the return statement contains the exact sql required
final SqlRowSet rs = this.jdbcTemplate.queryForRowSet(inputSqlWrapped);
final SqlRowSet rs = this.jdbcTemplate.queryForRowSet(inputSqlWrapped, name);

if (rs.next() && rs.getString("the_sql") != null) {
return rs.getString("the_sql");
Expand All @@ -220,14 +213,11 @@ private String getSql(final String name, final String type) {

@Override
public String getReportType(final String reportName, final boolean isSelfServiceUserReport) {
final String sql = "SELECT ifnull(report_type,'') as report_type FROM `stretchy_report` where report_name = '" + reportName
+ "' and self_service_user_report = ?";
validateReportName(reportName);
this.columnValidator.validateSqlInjection(sql, reportName);
final String sql = "SELECT ifNull(report_type,'') AS report_type FROM `stretchy_report` WHERE report_name = ? AND self_service_user_report = ?";

final String sqlWrapped = this.genericDataService.wrapSQL(sql);

final SqlRowSet rs = this.jdbcTemplate.queryForRowSet(sqlWrapped, isSelfServiceUserReport);
final SqlRowSet rs = this.jdbcTemplate.queryForRowSet(sqlWrapped, reportName, isSelfServiceUserReport);

if (rs.next()) {
return rs.getString("report_type");
Expand Down Expand Up @@ -323,7 +313,8 @@ private Collection<ReportData> retrieveReports(final Long id) {

final String sql = rm.schema(id);

final Collection<ReportParameterJoinData> rpJoins = this.jdbcTemplate.query(sql, rm);
final Collection<ReportParameterJoinData> rpJoins = this.jdbcTemplate.query(sql, rm,
id != null ? new Object[] { id } : new Object[] {});

final Collection<ReportData> reportList = new ArrayList<>();
if (rpJoins == null || rpJoins.size() == 0) {
Expand Down Expand Up @@ -416,7 +407,7 @@ public String schema(final Long reportId) {
sql += " from stretchy_report r" + " left join stretchy_report_parameter rp on rp.report_id = r.id"
+ " left join stretchy_parameter p on p.id = rp.parameter_id";
if (reportId != null) {
sql += " where r.id = " + reportId;
sql += " where r.id = ?";
} else {
sql += " order by r.id, rp.parameter_id";
}
Expand Down Expand Up @@ -498,7 +489,6 @@ private String sqlToRunForSmsEmailCampaign(final String name, final String type,
final Set<String> keys = queryParams.keySet();
for (String key : keys) {
final String pValue = queryParams.get(key);
// LOG.info("(" + key + " : " + pValue + ")");
key = "${" + key + "}";
sql = this.genericDataService.replace(sql, key, pValue);
}
Expand Down Expand Up @@ -568,10 +558,4 @@ public ByteArrayOutputStream generatePentahoReportAsOutputStream(final String re
*/
return null;
}

private void validateReportName(final String name) {
if (!StringUtils.isBlank(name) && !name.matches(REPORT_NAME_REGEX_PATTERN)) {
throw new SQLInjectionException();
}
}
}

0 comments on commit b3d0025

Please sign in to comment.