josemakara2 edited a comment on pull request #1671:
URL: https://github.com/apache/fineract/pull/1671#issuecomment-808887055


   > 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!
   I still believe this is work in progress and would not be a quick win to 
exhaustively get rid of SQL Injection vulnerabilities.
   One way is to search the codebase on IDE for concatenations and fix to use 
parameterized queries. 
   
   The plan here is to use OWASP ZAP to automatically detect SQL Injections and 
attend to the list in the report from ZAP analysis. 
   FINERACT-969 has attached html report but that doesn't seem to have scanned 
Fineract APIs as the output is just on community-app.
   I have excluded everything else except SQL Injection on ZAP to scan across 
Fineract API. I have had to quickly setup local test site with OAuth off to 
simplify things with OWASP ZAP. 
   
![image](https://user-images.githubusercontent.com/21666131/112750767-36eb4280-9016-11eb-8b1b-11b896bbcd57.png)
   
   A quick look on `Alerts` tab shows the detected violations. I will use the 
public link https://www.fineract.dev offered by @vorburger and produce the 
report which will be analysed for priority fixes in jira sub-tasks. 
   
![image](https://user-images.githubusercontent.com/21666131/112751209-71ee7580-9018-11eb-9579-83dd340e90c5.png)
   
   cc @vorburger 
   
   Some other things like below will possibly change the design to effectively 
parameterize the queries. 
   ```
               sql.append("emo.error_message as errorMessage ");
               sql.append("from " + tableName() + " emo");
   ```
   Ideally SQL string should only be built from string constants and every 
parameter inserted at runtime as bind variable (placeholder like `?, :name or 
@name`) in the SQL string and its value set using the `setX()` methods of the 
`PreparedStatement` class. 
   
   Here `tableName` won't work as a parameter as `FROM` clause won't use setX() 
methods. This impacts both Security and Performance, and more performance hit 
if using databases that use a shared execution plan cache like PostgreSQL.
   
   
   
    


-- 
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.

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to