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 Sat, 29 Apr 2017 02:42:58 GMT
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.ProvisioningEntriesDefinitionJsonDeserializer
(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.
> ProvisioningEntriesApiConstants.
> 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=1uLk3YPcjnXk7RqF8etsTzIuN59CDU
> 6sgBxpZul__1V4>
> > .
> >
> > Is there any particular reason to
> > have org.apache.fineract.accounting.provisioning.constant.
> ProvisioningEntriesApiConstants
> > 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=1TdwwHM2K1gMb6qILEX7gmzU8dVXcH
> GBdh569aFJfB2U>
> >>> [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=1uLk3YPcjnXk7RqF8etsTzIuN59CDU
> 6sgBxpZul__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=1uLk3YPcjnXk7RqF8etsTzIuN59CDU
> 6sgBxpZul__1V4>)
> >>> and solutions that I have suggested in the summary [3]
> >>> <https://drive.google.com/open?id=1TdwwHM2K1gMb6qILEX7gmzU8dVXcH
> GBdh569aFJfB2U>
> >>> .
> >>>
> >>> According to the findings [4]
> >>> <https://drive.google.com/open?id=1uLk3YPcjnXk7RqF8etsTzIuN59CDU
> 6sgBxpZul__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)

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