[FINERACT-854] Use prepared statements instead of string concatenated SQL everywhere

• Type: Improvement
• Status: In Progress
• Priority: Major
• Resolution: Unresolved
• Fix Version/s: 3.0.0
• Component/s: Security
• Labels: beginner scalability security technical
• Assignee: Joseph Makara
• Reporter: Michael Vorburger
• Created: 08/Mar/20 16:53
• Updated: 07/Nov/23 00:44

Description

The Fineract code base in many places creates SQL statements through String concatenation. This is prone to SQL injection. This is mitigated by the use of helpers utilities such as org.apache.fineract.infrastructure.core.api.ApiParameterHelper.sqlEncodeString(String) and org.apache.fineract.infrastructure.security.utils.SQLInjectionValidator.validateSQLInput(String) but I opine that those are workarounds... the better solution, both for security and likely also helping with performance (at least a little bit, knowing how much would require measuring it...), would be to use JDBC prepared statements with '?' placeholders and passing all raw arguments, instead of embedding them in the query String.

FINERACT-808 root cause analysis brought this up, and I'm about to raise a PR for FINERACT-808 which makes a start; the goal of this issue is to use the new org.apache.fineract.infrastructure.security.utils.SQLBuilder everywhere, and eventually be able to get completely rid of ApiParameterHelper and SQLInjectionValidator.

This issue should also include work to scan the code base for places where SQL Strings are concatenated without even using the existing helpers. FINERACT-853 could potentially help with that.

Issue Links

• blocks: FINERACT-865 Strengthen/Harden Fineract 1.x to LTS Version by Upgrading Java & Improving Code Coverage of Tests
• blocks: FINERACT-867 Scalability & Performance Enhancements for Supporting Millions of Clients, High TPS, and Concurrent Users
• blocks: FINERACT-967 lgtm.com Security Vulnerability Assessment Scanning
• blocks: FINERACT-988 Snyk.io Security Vulnerability Assessment Scanning
• incorporates: FINERACT-1338 SQL Injection - While "runreports" api is trying to load report parameters
• is blocked by: FINERACT-1095 Remove direct sqlSearch support from /clients and all other APIs [Security & Performance]
• is related to: FINERACT-969 Run OWASP zaproxy.org against Fineract (e.g. fineract.dev)
• relates to: FINERACT-1058 Add support for "limit" and "order by" query in SQLBuilder
• requires: FINERACT-808 Some action names do not filter audit trails
• requires: FINERACT-853 Use find-sec-bugs SpotBugs plugin to detect SQL injection issues (and other security related problems)
• links to: GitHub Pull Request #1123
• links to: GitHub Pull Request #1171
• links to: GitHub Pull Request #1279
• links to: GitHub Pull Request #1287
• links to: GitHub Pull Request #1671

Sub-Tasks

• PART 2 - Use prepared statements instead of string concatenated SQL everywhere: In Progress