FINERACT-2624: Prepared statement, input parameter sanitisation

[terencemo]

Description

This PR enhances stretchy reporting in Fineract by:

1. Sanitising input parameters based on type definitions
2. Using Prepared statement to execute stretchy reports

Integrations tests have been added which invoke runreports with both valid and invalid inputs. Numeric parameter (officeId) positive and negative tests (numeric and non-numeric input) - some of the invalid inputs include SLEEP and pg_sleep commands. Also UNION ALL inputs passed attempting SQL injection.

Besides this, unregistered parameter passing is covered where a parameter not in stretchy_report_parameter for the given report is passed. Additional integration tests can be added to cover date and string parameter types.

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

• Write the commit message as per our guidelines
• Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
• Create/update unit or integration tests for verifying the changes made.
• Follow our coding conventions.
• Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
• This PR must not be a "code dump". Large changes can be made in a branch, with assistance. Ask for help on the developer mailing list.

Your assigned reviewer(s) will follow our guidelines for code reviews.

[IOhacker]

No visible allow‑list, enum, or sanitization is applied to tableName or columnName

[terencemo]

No visible allow‑list, enum, or sanitization is applied to tableName or columnName

Sanitisation is not based on allow-list or enum. Can you explain what you mean by tableName / columnName? Are you referring to ReadReportingServiceImpl class methods getQuotedTableName and getQuotedColumnName ? My logic is not based upon those methods as explained in the document shared on the PMC list, we identify the parameter types.

[vidakovic]

@terencemo Question: why didn't we use just this service

fineract/fineract-core/src/main/java/org/apache/fineract/infrastructure/security/service/SqlValidator.java

Line 23 in 9e37bc3

public interface SqlValidator {

to validate? I think that thing should cover most if not all what is done here... and in case something would be missing: just add a pattern in the application.properties at

fineract/fineract-provider/src/main/resources/application.properties

Line 225 in 9e37bc3

# sql validation

If I understood the code here correctly then the only thing left would be the type checking for the columns, that could stay... but in any way, why not re-use the SQL validator, add any missing pattern and then the entire code base could profit? Should also simplify the proposed changes significantly.

As for the "allow-list" that @IOhacker mentioned: we could use the JDBC driver's metadata, extract all existing tables in the database at boot time and collect a static final list of table and column names in a Map<String, List<String>> variable (key = allowed table name, value = list of allowed column names in that table) and validate against that. Additionally we could again use the SqlValidator and maybe configure a separate profile with regex patterns to check the table and column names for any malicious characters... but I think that whitelist based on driver's metadata should be enough.

[adamsaghy]

I like the idea, but second @meonkeys and @vidakovic regarding we should have a central place for sql validation and rules and reuse that.

[IOhacker]

I support to reuse and improve the existing SQL validator as pointed by @alexivanov instead of applying a local fix.

[meonkeys]

I like the idea, but second @meonkeys and @vidakovic regarding we should have a central place for sql validation and rules and reuse that.

I did not say that and I'm not reviewing this PR, but FWIW it sounds like a good idea?

[adamsaghy]

I like the idea, but second @meonkeys and @vidakovic regarding we should have a central place for sql validation and rules and reuse that.

I did not say that and I'm not reviewing this PR, but FWIW it sounds like a good idea?

@meonkeys To the idea of using prepared statements wherever possible and input value sanitization and validation: yes, I do think it’s a good direction.

To implement it locally: not so much. I would probably wire and move this logic into the SqlValidator. There, I would define rules (which could be hardcoded, but probably we should have an SQL dictionary) and regular expressions or user-defined rules that could be configurable (to some extent).

I suppose the main point here is to avoid having a local solution but a centralized one.