-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use prepared statements instead of string concatenated SQL everywhere - PART 1 (FINERACT-854) #1671
Conversation
…QL everywhere PART 1
@josemakara2 hey, thank you so much for the PR. |
… - WIP (FINERACT-854)
|
Close button accidentally clicked :) UPDATE: format fixed = no
Will carry on with the changes for now |
Looks like the integration test: testGlobalConfigurations is failing. |
If you think the tests that failed are not related to your PR (which happens quite a few times here :P ) close and reopen this PR to re-run tests. I will shortly link an issue here where you can report the failing test. |
… - WIP (FINERACT-854)
… - WIP (FINERACT-854)
… - WIP (FINERACT-854)
Thanks for the update, tests pass now. Will review this tomorrow. |
No problems @thesmallstar, appreciated if you can please have a look as the work progresses. Many thanks! The plan here is to use OWASP ZAP to automatically detect SQL Injections and attend to the list in the report from ZAP analysis. A quick look on cc @vorburger Some other things like below will possibly change the design to effectively parameterize the queries.
Ideally SQL string should only be built from string constants and every parameter inserted at runtime as bind variable (placeholder like Here |
The first one to fix here will be as shown in the screenshot.
The page results were successfully manipulated using the Boolean conditions [default%] and [%e%]
Followed by application error in tomcat logs .. The vulnerability was detected by successfully restricting the data originally returned, by manipulating the parameter Fix:
A explain on this query shows it is doing full-table scan on
|
@thesmallstar This is ready for review. I have updated jira FINERACT-969 related to OWASP ZAP tool. There is additional fix (not related to SQL Injection) for duplicate entries error whilst running tests |
@josemakara2 thanks for the PR, will review it ASAP. |
@josemakara2 @thesmallstar |
|
And thanks for accepting to test as well.
|
.../src/main/java/org/apache/fineract/infrastructure/dataqueries/api/RunreportsApiResource.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!
Does this PR cover all SQL string concatenations? I have removed a few last time...and I remember there to be a few more of this.
I see that you have not used the SQL builder module, It LGTM and the code is clean. I would still request @vorburger to see if that's okay.
This PR also covers https://issues.apache.org/jira/browse/FINERACT-1336 @francisguchie thanks for testing this!
I have gone through the thread in detail, seems like this were some serious security issues floating around! |
Yes here It makes sense to have =, I am not able to understand why was 'like' added in the first place, do you see any possible use-case we are breaking here?
Wow this was a nice find! The PR LGTM, I am requesting @vorburger and @ptuomola to also review this.. we can merge this ASAP :) |
Thanks. I have edited |
Yes - it covers fixes to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. ping @vorburger , @ptuomola , @vidakovic . This looks interesting and important.
@josemakara2 Thank you very much for your fix. Keep up the good work you are doing at Apache Fineract. Make sure to join the mailing list. Your contributions are valuable. Welcome to the team. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks for contributing these changes - great work! Provided some feedback re the changes - would be great if you could take a look. Thanks!
...org/apache/fineract/infrastructure/campaigns/email/service/EmailReadPlatformServiceImpl.java
Outdated
Show resolved
Hide resolved
...org/apache/fineract/infrastructure/campaigns/email/service/EmailReadPlatformServiceImpl.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/apache/fineract/infrastructure/dataqueries/api/RunreportsApiResource.java
Outdated
Show resolved
Hide resolved
.../main/java/org/apache/fineract/infrastructure/security/service/JdbcTenantDetailsService.java
Outdated
Show resolved
Hide resolved
...rovider/src/main/resources/sql/migrations/core_db/V365__reportCategoryList-FINERACT-1306.sql
Outdated
Show resolved
Hide resolved
@ptuomola |
.append("(pcd.min_age <= GREATEST(datediff(").append(formattedDate).append(",sch.duedate),0) and ") | ||
.append("GREATEST(datediff(").append(formattedDate).append(",sch.duedate),0) <= pcd.max_age) and ") | ||
.append("pcd.criteria_id is not null ").append("LEFT JOIN m_client mclient ON mclient.id = loan.client_id ") | ||
.append("(pcd.min_age <= GREATEST(datediff(?").append(",sch.duedate),0) and ").append("GREATEST(datediff(?") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.append("(pcd.min_age <= GREATEST(datediff(?").append(",sch.duedate),0) and ").append("GREATEST(datediff(?") | |
.append("(pcd.min_age <= GREATEST(datediff(?,sch.duedate),0) and GREATEST(datediff(?") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Petri.
😇
It wasn't in first review but if it improves the code base why not 🙂. Will add fix tonight when I get to my laptop with this project.
There are other things there which I didn't bother with initially but can improve readability. No change to functionality but helps understand things.
Prefer
- lowerCamel case for SQL functions, and uppercase for other keywords
- lowerCamel case for column names -
dueDate
notduedate
- Only break line if it violates 120 character line limit in the FINERACT eclipse formatter
StringBuilder
only if it is a conditional query, with if conditions, otherwise just a single query as below suffice.
Simply return
public String schema() {
return "" +
"SELECT if(" +
" loan.loan_type_enum = 1, mclient.office_id, mgroup.office_id" +
" ) AS office_id," +
" loan.loan_type_enum," +
" pcd.criteria_id AS criteriaid," +
" loan.product_id," +
" loan.currency_code," +
" greatest(dateDiff(?, sch.duedate), 0) AS numberofdaysoverdue," +
" sch.duedate," +
" pcd.category_id," +
" pcd.provision_percentage," +
" loan.total_outstanding_derived AS outstandingbalance," +
" pcd.liability_account," +
" pcd.expense_account " +
"FROM m_loan_repayment_schedule sch " +
"LEFT JOIN m_loan loan ON sch.loan_id = loan.id " +
"INNER JOIN m_loanproduct_provisioning_mapping lpm ON lpm.product_id = loan.product_id " +
"INNER JOIN m_provisioning_criteria_definition pcd " +
" ON pcd.criteria_id = lpm.criteria_id " +
" AND (" +
" pcd.min_age <= greatest(dateDiff(?, sch.duedate), 0) AND greatest(dateDiff(?, sch.duedate), 0) <= pcd.max_age" +
" )" +
" AND pcd.criteria_id IS NOT NULL" +
"LEFT JOIN m_client mclient ON mclient.id = loan.client_id " +
"LEFT JOIN m_group mgroup ON mgroup.id = loan.group_id " +
"WHERE loan.loan_status_id = 300 AND sch.duedate = (" +
" SELECT min(sch1.duedate) " +
" FROM m_loan_repayment_schedule sch1 " +
" WHERE sch1.loan_id=loan.id AND sch1.completed_derived = FALSE" +
")";
}
But fineractdevprojectformatter
won't allows this 😬 and instead its formats is not SQL readable.
as opposed to
sqlQuery = new StringBuilder().append(
"select if(loan.loan_type_enum=1, mclient.office_id, mgroup.office_id) as office_id, loan.loan_type_enum, pcd.criteria_id as criteriaid, loan.product_id,loan.currency_code,")
.append("GREATEST(datediff(?")
.append(",sch.duedate),0) as numberofdaysoverdue,sch.duedate, pcd.category_id, pcd.provision_percentage,")
.append("loan.total_outstanding_derived as outstandingbalance, pcd.liability_account, pcd.expense_account from m_loan_repayment_schedule sch")
.append(" LEFT JOIN m_loan loan on sch.loan_id = loan.id")
.append(" JOIN m_loanproduct_provisioning_mapping lpm on lpm.product_id = loan.product_id")
.append(" JOIN m_provisioning_criteria_definition pcd on pcd.criteria_id = lpm.criteria_id and ")
.append("(pcd.min_age <= GREATEST(datediff(?,sch.duedate),0) and GREATEST(datediff(?")
.append(",sch.duedate),0) <= pcd.max_age) and ").append("pcd.criteria_id is not null ")
.append("LEFT JOIN m_client mclient ON mclient.id = loan.client_id ")
.append("LEFT JOIN m_group mgroup ON mgroup.id = loan.group_id ")
.append("where loan.loan_status_id=300 and sch.duedate = ")
.append("(select MIN(sch1.duedate) from m_loan_repayment_schedule sch1 where sch1.loan_id=loan.id and sch1.completed_derived=false)");
We can open a separate jira vote to fix formatter?
...che/fineract/accounting/provisioning/service/ProvisioningEntriesReadPlatformServiceImpl.java
Outdated
Show resolved
Hide resolved
thanks again @josemakara2! There was a lot of work behind this.. |
@josemakara2 Would you mind telling the mysql version you've used? |
@BLasan MySQL 8.0.23. |
So the best option is to use mysql 8.0.25 right? |
I'm using java 14. Hope it's fine with this mysql version. I'm getting an error
|
I don’t think we have tested Fineract with MySQL 8. At least when I did the MySQL upgrade last time, I tested only to 5.7. Which of course does not mean that it won’t work...
But of course testing with MySQL 8 (as well as any patches required) would be a great idea - so please do go ahead
Regards
Petri
… On 14 May 2021, at 3:57 PM, Benura Abeywardena ***@***.***> wrote:
@josemakara2 <https://github.com/josemakara2> Would you mind telling the mysql version you've used?
@BLasan <https://github.com/BLasan> MySQL 8.0.23.
We have also tested this in MySQL 8.0.25 as it fixes some other unrelated issues we have been having with MySQL 5.7
I'm using java 14. Hope it's fine with this mysql version. I'm getting an error
* What went wrong:
Execution failed for task ':fineract-provider:createDB'.
> java.sql.SQLException: Could not connect: Client does not support authentication protocol requested by server; consider upgrading MySQL client
* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.
* Get more help at https://help.gradle.org
Deprecated Gradle features were used in this build, making it incompatible with Gradle 7.0.
Use '--warning-mode all' to show the individual deprecation warnings.
See https://docs.gradle.org/6.8.3/userguide/command_line_interface.html#sec:command_line_warnings
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub <#1671 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AASJVCQYRH2NMS2ONA7USZ3TNTJVXANCNFSM4ZRMCVNA>.
|
Tried mysql 5.7 as well. And the gradle version has been downgraded in the latest codebase to 6.8. It's not using gradle 7.0 which was perfectly executed without any issue. After rebasing I got this error |
@ptuomola Another question is why we have downgraded the gradle version? |
I don’t think anyone has downgraded Gradle. The latest version that Fineract has been tested with was 6.8.3.
There is an automatic pull request from Renovate to upgrade to Gradle 7.0 and at least that has failed - which would suggest some changes are required: #1690 <#1690>. But I haven’t looked at the reasons for failure - perhaps it’s an easy fix?
Please feel free to do testing with Gradle 7.0 and submit a pull request with any changes required for it to work...
Regards
Petri
… On 14 May 2021, at 4:05 PM, Benura Abeywardena ***@***.*** ***@***.***>> wrote:
@ptuomola <https://github.com/ptuomola> Another question is why we have downgraded the gradle version?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#1671 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AASJVCWW5EFBX3AOY5HNJWDTNTKUFANCNFSM4ZRMCVNA>.
|
@BLasan This link has 3 options for the issue you are facing and might help you run Fineract in MySQL 5.7.x series |
Description
Replace SQL query concatenations with JDBC prepared statements with ? parameters in the code base.
Work done
Fineract-Platform-TenantId
SQL Injection appearing in 29 places on OWASP ZAP scanThis is fixed in 2 places:-
BasicAuthTenantDetailsServiceJdbc.java
andJdbcTenantDetailsService.java
tenants
, related to 1 above. Query no longer appearing in MySQL slow logrunreports
SQL Injections reported in OWASP ZAP scan seeReadReportingServiceImpl.java
FINERATC-1336
,FINERATC-1338
and ended up fixing that in
RunreportsApiResource.java
Next I plan to open 2 more pull requests
FINERATC-854
)FINERATC-969
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
Write the commit message as per https://github.com/apache/fineract/#pull-requests
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 coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/api-docs/apiLive.htm with details of any API changes
Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)
FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.