You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2020/03/06 11:15:41 UTC
[GitHub] [fineract] thesmallstar opened a new pull request #723:
FINERACT-808 FIXES: Some Action names do not filter audit trails
thesmallstar opened a new pull request #723: FINERACT-808 FIXES: Some Action names do not filter audit trails
URL: https://github.com/apache/fineract/pull/723
## Description
please refer: https://issues.apache.org/jira/browse/FINERACT-808
The filter did not work as backend would treat "UPDATE" and similar strings as SQL injection.
Since, it is in where clause this *probably* need not be verified(I have researched, but would still require a confirmation from mentors).
To solve this all the functions where the keywords such as update where verified are removed particularly for this statement.
-thankyou
## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
- [ ] Commit message starts with the issue number from https://issues.apache.org/jira/projects/FINERACT/. Ex: FINERACT-646 Pockets API.
- [ ] Coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions have been followed.
- [ ] API documentation at https://github.com/apache/fineract/blob/develop/api-docs/apiLive.htm has been updated with details of any API changes.
- [ ] Integration tests have been created/updated for verifying the changes made.
- [ ] All Integrations tests are passing with the new commits.
- [ ] Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the list.)
Our guidelines for code reviews is at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] vorburger commented on issue #723: FINERACT-808 FIXES:
Some Action names do not filter audit trails
Posted by GitBox <gi...@apache.org>.
vorburger commented on issue #723: FINERACT-808 FIXES: Some Action names do not filter audit trails
URL: https://github.com/apache/fineract/pull/723#issuecomment-596211460
> Since, it is in where clause this probably need not be verified (I have researched, but would still require a confirmation from mentors).
Reading e.g. https://www.sqlinjection.net/tutorial/, I don't think it's true in general that "Since, it is in where clause this probably need not be verified" ... however, looking more into the code:
For the change (removal of SQL validation to prevent injection) in `AuditReadPlatformServiceImpl` we can see that `retrievePaginatedAuditEntries()` is only ever called from `AuditsApiResource`'s `retrieveAuditEntries()`, where the `extraCriteria` are constructed in `getExtraCriteria()`, which itself already uses `ApiParameterHelper.sqlEncodeString()` for all String args, so removing `validateSqlInjection()` for `extraCriteria` looks safe, to me (but, again, **NOT** because "it's just a WHERE", but because it seems to previously have been checked already).
For the other proposed change in `AuditsApiResource.getExtraCriteria()` I'm much more sceptical, because that `actionName` which is currently `ApiParameterHelper.sqlEncodeString` which does a `SQLInjectionValidator.validateSQLInput`, but which with this change wouldn't do that anymore, comes straight from the outside world user as String parameter {{actionName}} in {{retrieveAuditEntries()}} ... removing that seems dangerous, to me.
I'm not entirely sure what a better solution is. I'd welcome more eyes on this though, and will email the list for additional input.
PS: Whatever solution we end up ultimately identifying and accepting, IMHO it could be useful for future code archaeology to include (a summary or copy/paste of) above into the commit message?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar edited a comment on issue #723:
FINERACT-808 FIXES: Some Action names do not filter audit trails
Posted by GitBox <gi...@apache.org>.
thesmallstar edited a comment on issue #723: FINERACT-808 FIXES: Some Action names do not filter audit trails
URL: https://github.com/apache/fineract/pull/723#issuecomment-596221641
@vorburger
Update(not final but helpful conclusions):
For SQL injection to be possible inside a where query:
(something I tested):
Here we expected the parts in city Paris with weight 17, SQL injection would end the quote and then add a semicolon(and maybe a new query ahead). We skipped the validation: city = Paris (This could have been a password?)
Now, in `sqlEncodedStringWithoutValidation` we add a quote to the start and end (since we need an initial quote and an end quote.
this made the search query from 17'; to (start)'17(end)';' next we replace each ' with ' ' -> '17' ';' this would escape the quote that means we could not end the initial start of the search string, the new search query would be (start)'17;' '(end)(in all cases).
**Use of double quotes?**
Should not be a problem, SQL injection needs to use a single quote(since we append a single quote).
Well, I could not still remove **probably**, but :P probably we would not need the keywords to be compared in this special case? :P
Edit: I am still trying and researching, will update accordingly.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar edited a comment on issue #723:
FINERACT-808 FIXES: Some Action names do not filter audit trails
Posted by GitBox <gi...@apache.org>.
thesmallstar edited a comment on issue #723: FINERACT-808 FIXES: Some Action names do not filter audit trails
URL: https://github.com/apache/fineract/pull/723#issuecomment-596214718
@vorburger Thank you so much for the detailed review.
I am working on the proposed changes, by the time please see this. I found this in code( which made me think, it is probably not needed :P ), I will also do the required research we should find some good solution as this should not be the first time this problem occurred :D (Will give an update shortly)
https://github.com/apache/fineract/blob/a7ebef478129516c6aa6d437fc2ec15268ab4776/fineract-provider/src/main/java/org/apache/fineract/portfolio/loanproduct/service/LoanProductReadPlatformServiceImpl.java#L124-L125
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar commented on issue #723: FINERACT-808
FIXES: Some Action names do not filter audit trails
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on issue #723: FINERACT-808 FIXES: Some Action names do not filter audit trails
URL: https://github.com/apache/fineract/pull/723#issuecomment-596214718
Thank you so much for the detailed review.
I am working on the proposed changes, by the time I found this in code( which made me think, it is probably not needed :P ), I will also do the required research we should find some good solution as this should not be the first time this problem occurred :D (Will give an update shortly)
https://github.com/apache/fineract/blob/a7ebef478129516c6aa6d437fc2ec15268ab4776/fineract-provider/src/main/java/org/apache/fineract/portfolio/loanproduct/service/LoanProductReadPlatformServiceImpl.java#L124-L125
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] vorburger commented on a change in pull request #723:
FINERACT-808 FIXES: Some Action names do not filter audit trails
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #723: FINERACT-808 FIXES: Some Action names do not filter audit trails
URL: https://github.com/apache/fineract/pull/723#discussion_r389373158
##########
File path: fineract-provider/src/main/java/org/apache/fineract/commands/service/AuditReadPlatformServiceImpl.java
##########
@@ -213,7 +213,8 @@ public AuditData mapRow(final ResultSet rs, @SuppressWarnings("unused") final in
sqlBuilder.append("select SQL_CALC_FOUND_ROWS ");
sqlBuilder.append(rm.schema(includeJson, hierarchy));
sqlBuilder.append(' ').append(updatedExtraCriteria);
- this.columnValidator.validateSqlInjection(sqlBuilder.toString(), extraCriteria);
+ //Do not validate this is inside a where query
+ //this.columnValidator.validateSqlInjection(sqlBuilder.toString(), extraCriteria);
Review comment:
just remove this, instead of commenting it out, it's clearer
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar closed pull request #723: FINERACT-808
FIXES: Some Action names do not filter audit trails
Posted by GitBox <gi...@apache.org>.
thesmallstar closed pull request #723: FINERACT-808 FIXES: Some Action names do not filter audit trails
URL: https://github.com/apache/fineract/pull/723
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] vorburger commented on a change in pull request #723:
FINERACT-808 FIXES: Some Action names do not filter audit trails
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #723: FINERACT-808 FIXES: Some Action names do not filter audit trails
URL: https://github.com/apache/fineract/pull/723#discussion_r389374375
##########
File path: fineract-provider/src/main/java/org/apache/fineract/commands/api/AuditsApiResource.java
##########
@@ -165,7 +165,7 @@ private String getExtraCriteria(final String actionName, final String entityName
String extraCriteria = "";
if (actionName != null) {
- extraCriteria += " and aud.action_name = " + ApiParameterHelper.sqlEncodeString(actionName);
+ extraCriteria += " and aud.action_name = " + ApiParameterHelper.sqlEncodedStringForWhere(actionName);
Review comment:
I'm not sure this is a good idea, see longer comment in main Conversation.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] awasum commented on issue #723: FINERACT-808 FIXES: Some
Action names do not filter audit trails
Posted by GitBox <gi...@apache.org>.
awasum commented on issue #723: FINERACT-808 FIXES: Some Action names do not filter audit trails
URL: https://github.com/apache/fineract/pull/723#issuecomment-596057004
@luckyman20 and @vorburger , when you get the time, review this and merge at your discretion.
Everything looks good from my end...this requires some functional testing.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] awasum commented on issue #723: FINERACT-808 FIXES: Some
Action names do not filter audit trails
Posted by GitBox <gi...@apache.org>.
awasum commented on issue #723: FINERACT-808 FIXES: Some Action names do not filter audit trails
URL: https://github.com/apache/fineract/pull/723#issuecomment-595760785
THanks for the PR @thesmallstar .
Please fix the checkstyle issue here: https://travis-ci.org/apache/fineract/builds/659110350?utm_source=github_status&utm_medium=notification
Each time run ./gradlew clean build check before sending a PR..to make sure everything works on your local Dev env.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar edited a comment on issue #723:
FINERACT-808 FIXES: Some Action names do not filter audit trails
Posted by GitBox <gi...@apache.org>.
thesmallstar edited a comment on issue #723: FINERACT-808 FIXES: Some Action names do not filter audit trails
URL: https://github.com/apache/fineract/pull/723#issuecomment-596214718
@vorburger Thank you so much for the detailed review.
I am working on the proposed changes, by the time I found this in code( which made me think, it is probably not needed :P ), I will also do the required research we should find some good solution as this should not be the first time this problem occurred :D (Will give an update shortly)
https://github.com/apache/fineract/blob/a7ebef478129516c6aa6d437fc2ec15268ab4776/fineract-provider/src/main/java/org/apache/fineract/portfolio/loanproduct/service/LoanProductReadPlatformServiceImpl.java#L124-L125
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar commented on issue #723: FINERACT-808
FIXES: Some Action names do not filter audit trails
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on issue #723: FINERACT-808 FIXES: Some Action names do not filter audit trails
URL: https://github.com/apache/fineract/pull/723#issuecomment-596221641
@vorburger
Update(not final but helpful conclusions):
For SQL injection to be possible inside a where query:
(something I tested):
Here we expected the parts in city Paris with weight 17, SQL injection would end the quote and then add a semicolon(and maybe a new query ahead). We skipped the validation: city = Paris (This could have been a password?)
Now, in `sqlEncodedStringWithoutValidation` we add a quote to the start and end (since we need an initial quote and an end quote.
this made the search query from 17'; to (start)'17(end)';' next we replace each ' with ' ' -> '17' ';' this would escape the quote that means we could not end the initial start of the search string, the new search query would be (start)'17;' '(end)(in all cases).
**Use of double quotes?**
Should not be a problem, SQL injection needs to use a single quote(since we append a single quote).
Well, I could not still remove **probably**, but :P probably we would not need the keywords to be compared in this special case? :P
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] vorburger commented on a change in pull request #723:
FINERACT-808 FIXES: Some Action names do not filter audit trails
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #723: FINERACT-808 FIXES: Some Action names do not filter audit trails
URL: https://github.com/apache/fineract/pull/723#discussion_r389373529
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/api/ApiParameterHelper.java
##########
@@ -169,6 +169,12 @@ public static String sqlEncodeString(final String str) {
return singleQuote + StringUtils.replace(str, singleQuote, twoSingleQuotes, -1) + singleQuote;
}
+ public static String sqlEncodedStringForWhere(final String str){
Review comment:
```suggestion
public static String sqlEncodedStringWithoutValidation(final String str){
```
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] vorburger commented on issue #723: FINERACT-808 FIXES:
Some Action names do not filter audit trails
Posted by GitBox <gi...@apache.org>.
vorburger commented on issue #723: FINERACT-808 FIXES: Some Action names do not filter audit trails
URL: https://github.com/apache/fineract/pull/723#issuecomment-596215262
@thesmallstar this is an interesting problem, and I got unexpectedly :smile_cat: motivating to have closer look into what an "ideal" (correct) solution to this problem in general would have to look like... I have an idea I'm trying out myself, will share if I get anywhere interesting. Please don't let this block you, we can work in parallel; in fact I would be very interested in your view and feedback and perhaps testing, if I find an alternative worth looking at (FYI I don't actually have a full set up to be able to really reproduce and test FINERACT-808 fully end-to-end, although I think I kind of understood what the problem is).
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [fineract] thesmallstar commented on issue #723: FINERACT-808
FIXES: Some Action names do not filter audit trails
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on issue #723: FINERACT-808 FIXES: Some Action names do not filter audit trails
URL: https://github.com/apache/fineract/pull/723#issuecomment-596048143
@awasum ,I am currently traveling, will do the required changes asap.
Thanks- Manthan
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services