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 21:38:22 GMT
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/
>> 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