Thisura, Your second PR is also fine. I am moving forward to write a brief document about the security analysis, which you will get Thursday.
On Wed, May 31, 2017 at 4:08 PM, Mark Reynolds <[email protected]> wrote: > Thisura, > > My impression is that you add to the API, but you cannot modify the > argument list of any existing method. > Therefore, I think adding constant classes, and also removing these > constants from the interfaces, is fine. > I have reviewed your first PR and it looks great. I am working on your > second PR right now. I will also > create a brief document that addresses your security analysis. > > - Mark > > On Mon, May 29, 2017 at 10:59 PM, Thisura Philips <[email protected]> > wrote: > >> Hi all, >> >> Please note the that the documents[1][2] are updated with the latest >> findings. There are suggestions to improve the fixes as per my >> understanding. I am under the the impression of not doing API changes. >> There fore didn't create any new classes for constants. I have raised this >> problem above in this thread. AFAIK It would be better if we can create >> constant classes to keep these constant sets rather than trying to keep >> them in interfaces (as it was) or in constant classes in a seperate package >> (where we can't use protected key word) >> >> The problem with keeping those in the interfaces is the following >> vulnerabilities. >> >> - MITRE, CWE-582 <http://cwe.mitre.org/data/definitions/582.html> - >> Array Declared Public, Final, and Static >> - MITRE, CWE-607 <http://cwe.mitre.org/data/definitions/607.html> - >> Public Static Final Field References Mutable Object >> >> As per the fix, I have moved most of those sets which were in interfaces >> to the respective classes and made them private. As all of them were only >> used in that class it is not a problem. >> >> But if we take a class like DepositApiConstants.java [3] >> <https://github.com/apache/fineract/blob/develop/fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/DepositsApiConstants.java>, >> it is better if we can create a new consant class in the package >> apache.fineract.portfolio.savings.data package and make them protected. >> Or else we can extend the constant class from the consuming class, if the >> consuming classes don't extend other classes (which is the case at the >> moment). Then we can make those Sets protected easily. >> >> But doing so is a API change. However I kept those to discuss, since >> there were major changes. Lets dicuss and decide what to do with theose >> yellow colured issues in the document >> <https://docs.google.com/spreadsheets/d/1uLk3YPcjnXk7RqF8etsTzIuN59CDU6sgBxpZul__1V4?usp=gmail> >> . >> >> [1] https://docs.google.com/spreadsheets/d/1uLk3YPcjnXk7RqF8 >> etsTzIuN59CDU6sgBxpZul__1V4?usp=gmail >> [2] https://drive.google.com/open?id=1TdwwHM2K1gMb6qILEX7gmz >> U8dVXcHGBdh569aFJfB2U&usp=gmail >> [3] https://github.com/apache/fineract/blob/develop/fineract >> -provider/src/main/java/org/apache/fineract/portfolio/ >> savings/DepositsApiConstants.java >> >> Thanks and Regards. >> >> On Tue, May 30, 2017 at 5:13 AM, Ed Cable <[email protected]> wrote: >> >>> Adding Nazeer too. >>> >>> Ed >>> >>> On May 27, 2017 9:43 PM, "Thisura Philips" <[email protected]> wrote: >>> >>>> Hi all, >>>> >>>> I have sent a PR <https://github.com/apache/fineract/pull/357> [1] >>>> resolving the rest of the issues of FINERACT-436 >>>> <https://issues.apache.org/jira/browse/FINERACT-436> [2]. For tracking >>>> purpose I have created a new ticket at FINERACT-470 >>>> <https://issues.apache.org/jira/browse/FINERACT-470>. This is the >>>> second PR connected with the previously merged PR >>>> <https://github.com/apache/fineract/pull/343> [3]. >>>> >>>> Please review and let me know if any thing is needed to be changed. >>>> >>>> I am moving on to fixing the FINERACT-437 >>>> <https://issues.apache.org/jira/browse/FINERACT-437>. >>>> >>>> [1] https://github.com/apache/fineract/pull/357 >>>> [2] https://issues.apache.org/jira/browse/FINERACT-436 >>>> [3] https://github.com/apache/fineract/pull/343 >>>> >>>> Thanks & Regard >>>> >>>> On Thu, May 11, 2017 at 1:58 AM, Ed Cable <[email protected]> wrote: >>>> >>>>> Thisura, I have cc'd Mark on this email >>>>> >>>>> On May 10, 2017 14:07, "Thisura Philips" <[email protected]> wrote: >>>>> >>>>> > Hi Nikhil, >>>>> > >>>>> > I am really glad to get selected to GOSC 2017. I have being looking >>>>> on TOIF >>>>> > a little bit in the past few days. First I will complete the SONAR >>>>> issues. >>>>> > >>>>> > Since Mark Reynolds is the mentor of the project, I would like to >>>>> get him >>>>> > in this thread too. Couldn't find his email though. Please some one >>>>> provide >>>>> > me the email of Mark. >>>>> > >>>>> > Thanks & Regards >>>>> > >>>>> > >>>>> > >>>>> > >>>>> > >>>>> > On Sun, Apr 30, 2017 at 7:41 AM, Thisura Philips < >>>>> [email protected]> >>>>> > wrote: >>>>> > >>>>> > > Hi all, >>>>> > > >>>>> > > I have sent the PR [1] >>>>> > > <https://github.com/apache/incubator-fineract/pull/343> including >>>>> the >>>>> > > changes needed to resolve the FINERACT-436 [2] >>>>> > > <https://issues.apache.org/jira/browse/FINERACT-436> in >>>>> accounting and >>>>> > > infrastructure packages. Please see the updated scanning report [3] >>>>> > > <https://docs.google.com/spreadsheets/d/1uLk3YPcjnXk7RqF8ets >>>>> TzIuN59CDU >>>>> > 6sgBxpZul__1V4/> >>>>> > > . >>>>> > > >>>>> > > It would be great if you can review and give feedback to continue >>>>> fixing >>>>> > > other vulnerabilities. >>>>> > > >>>>> > > I have done the LAPSE scanning for a part of accounting package as >>>>> well. >>>>> > > Will send the results soon. >>>>> > > >>>>> > > [1] https://github.com/apache/incubator-fineract/pull/343 >>>>> > > [2] https://issues.apache.org/jira/browse/FINERACT-436 >>>>> > > [3] https://docs.google.com/spreadsheets/d/ >>>>> > 1uLk3YPcjnXk7RqF8etsTzIuN59CDU >>>>> > > 6sgBxpZul__1V4/ >>>>> > > >>>>> > > Thanks & Regards >>>>> > > >>>>> > > On Sat, Apr 29, 2017 at 8:12 AM, Thisura Philips < >>>>> [email protected]> >>>>> > > wrote: >>>>> > > >>>>> > >> Hi Myrle, >>>>> > >> >>>>> > >> Agreed with the concern in doing an API change. >>>>> > >> >>>>> > >> The problem is any one can change the values in these arrays >>>>> since they >>>>> > >> are accessible public. It is nice if we can protect these arrays, >>>>> (set) >>>>> > as >>>>> > >> they are mutable. >>>>> > >> But as we know we can't use protected or private access modifiers >>>>> in an >>>>> > >> interface. It would be perfect if we can move the Sets in to the >>>>> > respective >>>>> > >> classes where they are used. >>>>> > >> >>>>> > >> supportedParameters = > org.apache.fineract.accounting. >>>>> > >> provisioning.serialization.ProvisioningEntriesDefinitionJ >>>>> > sonDeserializer >>>>> > >> (Used only in this class) >>>>> > >> >>>>> > >> PROVISIONING_ENTRY_PARAMETERS => org.apache.fineract.accounting >>>>> > >> .provisioning.api.ProvisioningEntriesApiResource (Used only in >>>>> this >>>>> > >> class) >>>>> > >> >>>>> > >> ALL_PROVISIONING_ENTRIES => org.apache.fineract.accounting >>>>> > >> .provisioning.api.ProvisioningEntriesApiResource (Used only in >>>>> this >>>>> > >> class) >>>>> > >> >>>>> > >> Since all of these sets are used in the mentioned classes only, >>>>> we can >>>>> > >> make them private. >>>>> > >> >>>>> > >> >>>>> > >> Thanks & Regards >>>>> > >> >>>>> > >> >>>>> > >> On Fri, Apr 28, 2017 at 5:49 PM, Myrle Krantz <[email protected]> >>>>> wrote: >>>>> > >> >>>>> > >>> By constant class Thisura, do you mean a final class with a >>>>> private >>>>> > >>> constructor? >>>>> > >>> >>>>> > >>> It would be possible to change >>>>> > >>> org.apache.fineract.accounting.provisioning.constant.Provisi >>>>> > >>> oningEntriesApiConstants. >>>>> > >>> This would be a breaking change though. Given that the >>>>> interface has >>>>> > >>> no methods, and no one is likely to have derived from it, there's >>>>> > >>> probably no code to break by changing it, but you still have to >>>>> answer >>>>> > >>> the question "Why would I change that?" >>>>> > >>> >>>>> > >>> Yes, I know that Bloch ("Effective Java", 2nd Ed., Chapter 19) >>>>> said >>>>> > >>> it's bad. But his arguments only make sense when referring to an >>>>> > >>> interface which contains methods. This interface doesn't. So >>>>> given >>>>> > >>> that Apache Fineract is communicated with over a REST interface, >>>>> what >>>>> > >>> harm does this interface cause that would justify making an >>>>> > >>> API-breaking change (though a minor one) to remediate the >>>>> situation? >>>>> > >>> >>>>> > >>> Best Regards, >>>>> > >>> Myrle >>>>> > >>> >>>>> > >>> >>>>> > >>> On Sun, Apr 23, 2017 at 8:00 PM, Thisura Philips < >>>>> [email protected] >>>>> > > >>>>> > >>> wrote: >>>>> > >>> > Hi all, >>>>> > >>> > >>>>> > >>> > I have done some of the fixes for FINERACT-436 >>>>> > >>> > <https://issues.apache.org/jira/browse/FINERACT-436>. Please >>>>> see the >>>>> > >>> > updated document >>>>> > >>> > <https://drive.google.com/open?id=1uLk3YPcjnXk7RqF8etsTzIuN5 >>>>> > >>> 9CDU6sgBxpZul__1V4> >>>>> > >>> > . >>>>> > >>> > >>>>> > >>> > Is there any particular reason to >>>>> > >>> > have org.apache.fineract.accounting >>>>> .provisioning.constant.Provisi >>>>> > >>> oningEntriesApiConstants >>>>> > >>> > as an interface. It is true that variables in interfaces are >>>>> static, >>>>> > >>> final >>>>> > >>> > by default. But yet this can cause the following >>>>> vulnerabilities. >>>>> > >>> > >>>>> > >>> > - MITRE, CWE-582 <http://cwe.mitre.org/data/def >>>>> initions/582.html> >>>>> > - >>>>> > >>> > Array Declared Public, Final, and Static >>>>> > >>> > - MITRE, CWE-607 <http://cwe.mitre.org/data/def >>>>> initions/607.html> >>>>> > - >>>>> > >>> > Public Static Final Field References Mutable Object >>>>> > >>> > >>>>> > >>> > >>>>> > >>> > Can't we change this to a constant class? ASFAIK this is the >>>>> correct >>>>> > >>> way to >>>>> > >>> > maintain a set of constants. >>>>> > >>> > >>>>> > >>> > Thanks & Regards >>>>> > >>> > >>>>> > >>> > >>>>> > >>> > On Sun, Apr 23, 2017 at 5:53 PM, Thisura Philips < >>>>> > [email protected] >>>>> > >>> > >>>>> > >>> > wrote: >>>>> > >>> > >>>>> > >>> >> Hi all, >>>>> > >>> >> >>>>> > >>> >> I have created two tickets [1][2] to track the fixes for >>>>> security >>>>> > >>> >> vulnerabilities reported by sonar. >>>>> > >>> >> Thanks & Regards. >>>>> > >>> >> [1] https://issues.apache.org/jira/browse/FINERACT-436 >>>>> > >>> >> [2] https://issues.apache.org/jira/browse/FINERACT-437 >>>>> > >>> >> >>>>> > >>> >> On Fri, Apr 21, 2017 at 10:31 AM, Thisura Philips < >>>>> > >>> [email protected]> >>>>> > >>> >> wrote: >>>>> > >>> >> >>>>> > >>> >>> Hi all, >>>>> > >>> >>> >>>>> > >>> >>> As per the long discussion in the thread "[Mifos-developer] >>>>> > >>> Application >>>>> > >>> >>> for GSOC 2017( Static Analysis of Apache Fineract )", I have >>>>> > >>> >>> >>>>> > >>> >>> * done the static analysis with SonarQube >>>>> > >>> >>> * generated the vulnerability report, - sonarlint report [1] >>>>> > >>> >>> <https://drive.google.com/open?id=0B6WV3fK5Tak7Uy1uOWk0SW81W >>>>> m8>, >>>>> > >>> >>> sonarqube <https://drive.google.com/open >>>>> > >>> ?id=0B6WV3fK5Tak7OHJENF9oZFE2X2c> report >>>>> > >>> >>> [2] <https://drive.google.com/open >>>>> ?id=0B6WV3fK5Tak7OHJENF9oZFE2X2c >>>>> > > >>>>> > >>> >>> * summarized >>>>> > >>> >>> <https://drive.google.com/open?id=1TdwwHM2K1gMb6qILEX7gmzU8d >>>>> > >>> VXcHGBdh569aFJfB2U> >>>>> > >>> >>> [3] the types of vulnerabilities, >>>>> > >>> >>> * attended each of those vulnerabilities to check whether >>>>> they are >>>>> > >>> not >>>>> > >>> >>> false positives and >>>>> > >>> >>> * prepared the checklist [4] >>>>> > >>> >>> <https://drive.google.com/open?id=1uLk3YPcjnXk7RqF8etsTzIuN5 >>>>> > >>> 9CDU6sgBxpZul__1V4> >>>>> > >>> >>> of vulnerabilities with fixes >>>>> > >>> >>> >>>>> > >>> >>> All the reports which are generated using different plugins, >>>>> tools >>>>> > >>> can be >>>>> > >>> >>> found here [5] >>>>> > >>> >>> <https://drive.google.com/open?id=0B6WV3fK5Tak7ZVJkVGV3WVZ3O >>>>> E0>. >>>>> > >>> >>> >>>>> > >>> >>> Now we can go ahead and do the necessary changes to fix the >>>>> > reported >>>>> > >>> >>> vulnerabilities in the codebase. I am looking forward to >>>>> creating >>>>> > >>> tickets >>>>> > >>> >>> for each type of issues reported in summary. >>>>> > >>> >>> >>>>> > >>> >>> We need to verify the problems (vulnerabilities[4] >>>>> > >>> >>> <https://drive.google.com/open?id=1uLk3YPcjnXk7RqF8etsTzIuN5 >>>>> > >>> 9CDU6sgBxpZul__1V4>) >>>>> > >>> >>> and solutions that I have suggested in the summary [3] >>>>> > >>> >>> <https://drive.google.com/open?id=1TdwwHM2K1gMb6qILEX7gmzU8d >>>>> > >>> VXcHGBdh569aFJfB2U> >>>>> > >>> >>> . >>>>> > >>> >>> >>>>> > >>> >>> According to the findings [4] >>>>> > >>> >>> <https://drive.google.com/open?id=1uLk3YPcjnXk7RqF8etsTzIuN5 >>>>> > >>> 9CDU6sgBxpZul__1V4>, >>>>> > >>> >>> I will create two tickets for co-related fixes for each >>>>> topic >>>>> > (type >>>>> > >>> of >>>>> > >>> >>> vulnerability) such as >>>>> > >>> >>> >>>>> > >>> >>> * Mutable fields should not be "public static" && "enum" >>>>> fields >>>>> > >>> should >>>>> > >>> >>> not be publicly mutable && "public static" fields should be >>>>> > constant >>>>> > >>> >>> * Generic exceptions should never be thrown && Throwable and >>>>> Error >>>>> > >>> should >>>>> > >>> >>> not be caught >>>>> > >>> >>> >>>>> > >>> >>> Expect the community ideas regarding this to validate the >>>>> suggested >>>>> > >>> >>> solutions. >>>>> > >>> >>> >>>>> > >>> >>> [1] https://drive.google.com/open? >>>>> id=0B6WV3fK5Tak7Uy1uOWk0SW81Wm8 >>>>> > >>> >>> [2] https://drive.google.com/open? >>>>> id=0B6WV3fK5Tak7OHJENF9oZFE2X2c >>>>> > >>> >>> [3] https://drive.google.com/open?id=1TdwwHM2K1gMb6qILEX7gmz >>>>> > >>> >>> U8dVXcHGBdh569aFJfB2U >>>>> > >>> >>> [4] https://drive.google.com/open?id=1uLk3YPcjnXk7RqF8etsTzI >>>>> > >>> >>> uN59CDU6sgBxpZul__1V4 >>>>> > >>> >>> [5] https://drive.google.com/open? >>>>> id=0B6WV3fK5Tak7ZVJkVGV3WVZ3OE0 >>>>> > >>> >>> >>>>> > >>> >>> Thanks & regards >>>>> > >>> >>> -- >>>>> > >>> >>> T.T.C Philips (BSc.Eng (Undergrad)) >>>>> > >>> >>> Computer Science and Engineering, >>>>> > >>> >>> Sri Lanka Institute of Information Technology(SLIIT) >>>>> > >>> >>> >>>>> > >>> >>> >>>>> > >>> >>> >>>>> > >>> >>> >>>>> > >>> >> >>>>> > >>> >> >>>>> > >>> >> -- >>>>> > >>> >> T.T.C Philips (BSc.Eng (Undergrad)) >>>>> > >>> >> Computer Science and Engineering, >>>>> > >>> >> Sri Lanka Institute of Information Technology(SLIIT) >>>>> > >>> >> >>>>> > >>> >> >>>>> > >>> >> >>>>> > >>> >> >>>>> > >>> > >>>>> > >>> > >>>>> > >>> > -- >>>>> > >>> > T.T.C Philips (BSc.Eng (Undergrad)) >>>>> > >>> > Computer Science and Engineering, >>>>> > >>> > Sri Lanka Institute of Information Technology(SLIIT) >>>>> > >>> >>>>> > >> >>>>> > >> >>>>> > >> >>>>> > >> -- >>>>> > >> T.T.C Philips (BSc.Eng (Undergrad)) >>>>> > >> Computer Science and Engineering, >>>>> > >> Sri Lanka Institute of Information Technology(SLIIT) >>>>> > >> >>>>> > >> >>>>> > >> >>>>> > >> >>>>> > > >>>>> > > >>>>> > > -- >>>>> > > T.T.C Philips (BSc.Eng (Undergrad)) >>>>> > > Computer Science and Engineering, >>>>> > > Sri Lanka Institute of Information Technology(SLIIT) >>>>> > > >>>>> > > >>>>> > > >>>>> > > >>>>> > >>>>> > >>>>> > -- >>>>> > T.T.C Philips (BSc.Eng (Undergrad)) >>>>> > Computer Science and Engineering, >>>>> > Sri Lanka Institute of Information Technology(SLIIT) >>>>> > >>>>> >>>> >>>> >>>> >>>> -- >>>> T.T.C Philips (BSc.Eng (Undergrad)) >>>> Computer Science and Engineering, >>>> Sri Lanka Institute of Information Technology(SLIIT) >>>> >>>> >>>> >>>> >> >> >> -- >> T.T.C Philips (BSc.Eng (Undergrad)) >> Computer Science and Engineering, >> Sri Lanka Institute of Information Technology(SLIIT) >> >> >> >> >
