fineract-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Reynolds <markr...@bu.edu>
Subject Re: [GSOC2017] Fixing security vulnerabilities reported by sonar scan
Date Wed, 31 May 2017 20:08:46 GMT
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/1uLk3YPcjnXk7RqF8etsTzIuN59CDU
> 6sgBxpZul__1V4?usp=gmail
> [2] https://drive.google.com/open?id=1TdwwHM2K1gMb6qILEX7gmzU8dVXcH
> GBdh569aFJfB2U&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 <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=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)
>
>
>
>

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