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 Sun, 28 May 2017 04:43:50 GMT
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/1uLk3YPcjnXk7RqF8etsTzIuN59CDU
> > 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/
> 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
> > >>> >
> > >>> >
> > >>> > 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)

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