fineract-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Thisura Philips <ttcphil...@gmail.com>
Subject Re: [GSOC2017] Fixing security vulnerabilities reported by sonar scan
Date Thu, 08 Jun 2017 08:22:08 GMT
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 <ttcphilips@gmail.com>
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 <markreyn@bu.edu> 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 <markreyn@bu.edu> 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 <ttcphilips@gmail.com>
>>> 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 <edcable@mifos.org> wrote:
>>>>
>>>>> Adding Nazeer too.
>>>>>
>>>>> Ed
>>>>>
>>>>> On May 27, 2017 9:43 PM, "Thisura Philips" <ttcphilips@gmail.com>
>>>>> 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 <edcable@mifos.org>
wrote:
>>>>>>
>>>>>>> Thisura, I have cc'd Mark on this email
>>>>>>>
>>>>>>> On May 10, 2017 14:07, "Thisura Philips" <ttcphilips@gmail.com>
>>>>>>> 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 <
>>>>>>> ttcphilips@gmail.com>
>>>>>>> > 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 <
>>>>>>> ttcphilips@gmail.com>
>>>>>>> > > 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 <myrle@apache.org>
>>>>>>> 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
<
>>>>>>> ttcphilips@gmail.com
>>>>>>> > >
>>>>>>> > >>> 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 <
>>>>>>> > ttcphilips@gmail.com
>>>>>>> > >>> >
>>>>>>> > >>> > 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 <
>>>>>>> > >>> ttcphilips@gmail.com>
>>>>>>> > >>> >> 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)

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message