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/1uLk3YPcjnXk7RqF8etsTzIuN59CDU6sgBxpZul__1V4?usp=gmail [2] https://drive.google.com/open?id=1TdwwHM2K1gMb6qILEX7gmzU8dVXcHGBdh569aFJfB2U&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=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)
