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, 01 Jun 2017 00:41:57 GMT
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)

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