Hi Mark, I have updated the PR[1] <https://github.com/apache/fineract/pull/357> adding new constant classes and making those sets protected, to make those sets secure. Please review and let me know if we need to update it.
[1] https://github.com/apache/fineract/pull/357 On Thu, Jun 1, 2017 at 6:11 AM, Thisura Philips <[email protected]> wrote: > Hi Mark, > > That's great. If I get the consent to create new constant classes I will > do that since that is the most appropriate solution. And will modify the PR > and will update the document. > > Thanks & Regards > > > > On Thu, Jun 1, 2017 at 3:08 AM, Mark Reynolds <[email protected]> wrote: > >> 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/saving >>>> s/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=0B6WV3fK5Tak7Uy1uOWk0SW81Wm8>, >>>>>>> > >>> >>> 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=0B6WV3fK5Tak7ZVJkVGV3WVZ3OE0>. >>>>>>> > >>> >>> >>>>>>> > >>> >>> 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) >>>> >>>> >>>> >>>> >>> >> > > > -- > 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)
